Skip to content
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

Merged
merged 18 commits into from
May 22, 2019
Merged

[http] add handling of bad request #419

merged 18 commits into from
May 22, 2019

Conversation

zekth
Copy link
Contributor

@zekth zekth commented May 20, 2019

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?

@ry
Copy link
Member

ry commented May 20, 2019

Can you add a test?

@zekth
Copy link
Contributor Author

zekth commented May 20, 2019

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.

@zekth
Copy link
Contributor Author

zekth commented May 20, 2019

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.

@zekth
Copy link
Contributor Author

zekth commented May 21, 2019

So i've exported ReadRequest to check if with malformed or unreadable headers it does not crash the server. Atm it only returns EOF. But in the refactor of iterator this has been introduced:

// TODO(ry): send something back like a HTTP 500 status.

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?

@zekth
Copy link
Contributor Author

zekth commented May 21, 2019

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 HttpError it's a proposal. Not sure if it's the best. Let me know what you think.

EDIT: Reverted to regular error.

@ry ry requested a review from piscisaureus May 21, 2019 16:53
http/server_test.ts Outdated Show resolved Hide resolved
http/server_test.ts Outdated Show resolved Hide resolved
http/server_test.ts Outdated Show resolved Hide resolved
http/server_test.ts Outdated Show resolved Hide resolved
http/server.ts Outdated Show resolved Hide resolved
http/server.ts Outdated Show resolved Hide resolved
Copy link
Member

@piscisaureus piscisaureus left a 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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

catch (e) {
  bufStateErr = e
}

@piscisaureus piscisaureus self-requested a review May 22, 2019 23:19
Copy link
Member

@piscisaureus piscisaureus left a 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 piscisaureus merged commit 7620fe1 into denoland:master May 22, 2019
@zekth
Copy link
Contributor Author

zekth commented May 23, 2019

@piscisaureus resolve was unecessary?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

deno 0.5.0 - http server crash with incoming https request
3 participants