-
Notifications
You must be signed in to change notification settings - Fork 635
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[http] add handling of bad request #419
Conversation
Can you add a test? |
added. The test of FS with timestamps is flaky on the CI even on my macbook i encounter this issue. |
I have changed the code because my handling was serving the data to the User code and we don't want it, it has to be handled on Deno side. Working on tests now. |
So i've exported ReadRequest to check if with malformed or unreadable headers it does not crash the server. Atm it only returns Would it be revelant do declare a new Error type for server error and do all the handling of standard error messages in this place? |
So i've ported the headers from : https://github.com/golang/go/blob/master/src/net/http/request_test.go#L428 as suggested. Some tests case are commented because it has shown errors in our code which i'll fix in another PR. About EDIT: Reverted to regular error. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking pretty good and I'm very happy to see some test cases.
I had two comments/questions, please respond to them. Otherwise, looking good to me.
http/server.ts
Outdated
[req, bufStateErr] = await readRequest(conn, bufr); | ||
try { | ||
[req, bufStateErr] = await readRequest(conn, bufr); | ||
} catch { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
catch (e) {
bufStateErr = e
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made some final changes myself, LGTM
@piscisaureus |
This is meant to fix : denoland/deno#2346
It's not that pretty but ATM i haven't found any other way to do it.
So if, there is an issue in the request with malformed headers or encrypted due to HTTPS, an error is attached to the server request, if there is a respond of it the ServerResponse is automaticaly rewritten.
This fixes bad headers and wrong protocol (somehow). The problem with https is we got :
ssl3_get_record:wrong version number
because the client is trying to do the handshake.Note: i've noticed we have multiple set of
new TextEncoder()
. Would it be better to have it as a const in the module?