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.

Sign up for GitHub

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

feat: add 400 response for broken client request to instead of empty response #1829

Merged
merged 6 commits into from
Dec 15, 2017
Merged

feat: add 400 response for broken client request to instead of empty response #1829

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9d2a647
feat: add 400 response for broken client request to instead of empty …
XadillaX Dec 14, 2017
98dc445
r
XadillaX Dec 14, 2017
00568d8
r
XadillaX Dec 14, 2017
9aac3cc
r
XadillaX Dec 14, 2017
ebbd6ab
r
XadillaX Dec 14, 2017
9362034
r
XadillaX Dec 14, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,3 +24,4 @@ docs/plugins.png
package-lock.json
yarn.lock
!test/fixtures/apps/loader-plugin/node_modules
.editorconfig
30 changes: 29 additions & 1 deletion lib/application.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,20 @@ const EGG_LOADER = Symbol.for('egg#loader');
const EGG_PATH = Symbol.for('egg#eggPath');
const CLUSTER_CLIENTS = Symbol.for('egg#clusterClients');

// client error => 400 Bad Request
// Refs: https://nodejs.org/dist/latest-v8.x/docs/api/http.html#http_event_clienterror
const DEFAULT_BAD_REQUEST_HTML = `<html>
<head><title>400 Bad Request</title></head>
<body bgcolor="white">
<center><h1>400 Bad Request</h1></center>
<hr><center>❤</center>
</body>
</html>`;
const DEFAULT_BAD_REQUEST_HTML_LENGTH = Buffer.byteLength(DEFAULT_BAD_REQUEST_HTML);
const DEFAULT_BAD_REQUEST_RESPONSE =
`HTTP/1.1 400 Bad Request\r\nContent-Length: ${DEFAULT_BAD_REQUEST_HTML_LENGTH}` +
`\r\n\r\n${DEFAULT_BAD_REQUEST_HTML}`;

/**
* Singleton instance in App Worker, extend {@link EggApplication}
* @extends EggApplication
Expand Down Expand Up @@ -55,6 +69,18 @@ class Application extends EggApplication {
return path.join(__dirname, '..');
}

onClientError(err, socket) {
this.logger.error('A client (%s:%d) error [%s] occurred: %s',
Copy link
Member Author

Choose a reason for hiding this comment

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

提了个 PR：nodejs/node#17672

不知道人家收不收。

如果收了的话，这个 logger 还能顺便将错误的 Packet Buffer 给输出来。

If this PR can be merged, we can output the raw packet buffer via logger.

socket.remoteAddress,
socket.remotePort,
err.code,
err.message);

// because it's a raw socket object, we should return the raw HTTP response
// packet.
socket.end(DEFAULT_BAD_REQUEST_RESPONSE);
}

onServer(server) {
/* istanbul ignore next */
graceful({
Expand All @@ -66,6 +92,8 @@ class Application extends EggApplication {
this.coreLogger.error(err);
},
});

server.on('clientError', (err, socket) => this.onClientError(err, socket));
}

/**
Expand Down Expand Up @@ -247,7 +275,7 @@ class Application extends EggApplication {
ctx.coreLogger.error(err);
});
// expose server to support websocket
this.on('server', server => this.onServer(server));
this.once('server', server => this.onServer(server));
}

/**
Expand Down
43 changes: 43 additions & 0 deletions test/lib/cluster/app_worker.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,49 @@ describe('test/lib/cluster/app_worker.test.js', () => {
.expect('true');
});

it('should response 400 bad request when HTTP request packet broken', async () => {
const test1 = app.httpRequest()
// Node.js (http-parser) will occur an error while the raw URI in HTTP
// request packet containing space.
//
// Refs: https://zhuanlan.zhihu.com/p/31966196
.get('/foo bar');
const test2 = app.httpRequest().get('/foo baz');

// app.httpRequest().expect() will encode the uri so that we cannot
// request the server with raw `/foo bar` to emit 400 status code.
//
// So we generate `test.req` via `test.request()` first and override the
// encoded uri.
//
// `test.req` will only generated once:
//
// ```
// function Request::request() {
// if (this.req) return this.req;
//
// // code to generate this.req
//
// return this.req;
// }
// ```
test1.request().path = '/foo bar';
test2.request().path = '/foo baz';

const html = `<html>
<head><title>400 Bad Request</title></head>
<body bgcolor="white">
<center><h1>400 Bad Request</h1></center>
<hr><center>❤</center>
</body>
</html>`;

await Promise.all([
test1.expect(html).expect(400),
test2.expect(html).expect(400),
]);
});

describe('listen hostname', () => {
let app;
before(() => {
Expand Down