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

Conversation

3 participants
@zekth
Copy link
Contributor

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?

@zekth zekth force-pushed the zekth:process_rq branch from 8b31d98 to cb80293 May 20, 2019

@ry

This comment has been minimized.

Copy link
Contributor

commented May 20, 2019

Can you add a test?

@zekth

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Contributor Author

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

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_test.ts Outdated
Show resolved Hide resolved http/server.ts Outdated
Show resolved Hide resolved http/server.ts Outdated
@piscisaureus
Copy link
Contributor

left a comment

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.

[req, bufStateErr] = await readRequest(conn, bufr);
try {
[req, bufStateErr] = await readRequest(conn, bufr);
} catch {

This comment has been minimized.

Copy link
@piscisaureus

piscisaureus May 22, 2019

Contributor
catch (e) {
  bufStateErr = e
}

zekth added some commits May 20, 2019

zekth added some commits May 22, 2019

@piscisaureus piscisaureus force-pushed the zekth:process_rq branch from adf775a to 33a2fee May 22, 2019

@piscisaureus piscisaureus self-requested a review May 22, 2019

@piscisaureus
Copy link
Contributor

left a comment

Made some final changes myself, LGTM

@piscisaureus piscisaureus merged commit 7620fe1 into denoland:master May 22, 2019

5 checks passed

denoland.deno_std Build #20190522.22 succeeded
Details
denoland.deno_std (Linux) Linux succeeded
Details
denoland.deno_std (Mac) Mac succeeded
Details
denoland.deno_std (Windows) Windows succeeded
Details
license/cla Contributor License Agreement is signed.
Details
@zekth

This comment has been minimized.

Copy link
Contributor Author

commented May 23, 2019

@piscisaureus resolve was unecessary?

@zekth zekth deleted the zekth:process_rq branch May 23, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.