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 3 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
31 changes: 30 additions & 1 deletion lib/application.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,26 @@ const KEYS = Symbol('Application#keys');
const HELPER = Symbol('Application#Helper');
const LOCALS = Symbol('Application#locals');
const BIND_EVENTS = Symbol('Application#bindEvents');
const ON_CLIENT_ERROR = Symbol('Application#onClientError');
const WARN_CONFUSED_CONFIG = Symbol('Application#warnConfusedConfig');
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 +70,18 @@ class Application extends EggApplication {
return path.join(__dirname, '..');
}

[ON_CLIENT_ERROR](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 +93,8 @@ class Application extends EggApplication {
this.coreLogger.error(err);
},
});

server.on('clientError', this[ON_CLIENT_ERROR].bind(this));
Copy link
Member

Choose a reason for hiding this comment

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

server.on('clientError', err => this[ON_CLIENT_ERROR](err)) 统一风格

Copy link
Member

Choose a reason for hiding this comment

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

另外我建议这里 onClientError 的 handler 不要用 symbol，给使用者一个覆盖的机会

Copy link
Member

Choose a reason for hiding this comment

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

有需要覆盖的需求再加吧，感觉这是一个兜底的，应该很少会自定义吧

Copy link
Member Author

Choose a reason for hiding this comment

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

@dead-horse 你指的是使用者在自己应用中继承这个类然后重载 onClientError 函数吗？

Copy link
Member Author

Choose a reason for hiding this comment

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

@atian25 如果我是使用者，我觉得我会想去自定义

Copy link
Member

Choose a reason for hiding this comment

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

这种路径不是正常用户能触发的吧

Copy link
Member Author

@XadillaX XadillaX Dec 14, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@atian25 这只是一种情况，clientError 可不止这一种情况。举个最简单的例子，用户的请求被运营商劫持了，然后运营商代码有误，把请求包改错了。

Copy link
Member

Choose a reason for hiding this comment

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

使用者在自己应用中继承这个类然后重载 onClientError 函数吗

使用者直接在 extends/app.js 中就可以覆盖默认的 onClientError 实现了

}

/**
Expand Down Expand Up @@ -247,7 +276,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
44 changes: 44 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,50 @@ describe('test/lib/cluster/app_worker.test.js', () => {
.expect('true');
});

it('should response 400 bad request when HTTP request packet broken', function* () {
Copy link
Member

Choose a reason for hiding this comment

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

改为 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>`;

yield [
test1.expect(html).expect(400),
test2.expect(html).expect(400)
];
});


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