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

http: setEncoding error for incoming socket connections #19344

Closed
wants to merge 9 commits into from
Closed

http: setEncoding error for incoming socket connections #19344

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
a7e2aab
http: setEncoding override for incoming packets
iSkore Feb 4, 2018
345ff3c
test: added tests for setEncoding error check
iSkore Feb 4, 2018
4643c63
docs, errors, http: disallow setEncoding on incoming packets
iSkore Mar 6, 2018
f3c429c
docs, errors: note RFC compliance in error message and documentation
iSkore Mar 14, 2018
3613e8d
tests, http: fixed call to ERR_HTTP_INCOMING_SOCKET_ENCODING
iSkore Mar 14, 2018
190ef7e
docs, http: minor format changes per review request
iSkore Mar 29, 2018
06a34eb
docs: update ERR_HTTP_INCOMING_SOCKET_ENCODING per request
iSkore Apr 3, 2018
cd2c3ce
http: minor change per review request
iSkore Apr 19, 2018
a8c605e
Merge branch 'master' into http/issue_18118
iSkore Aug 27, 2018
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
8 changes: 8 additions & 0 deletions doc/api/errors.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -794,6 +794,12 @@ An invalid symlink type was passed to the [`fs.symlink()`][] or

An attempt was made to add more headers after the headers had already been sent.

<a id="ERR_HTTP_INCOMING_SOCKET_ENCODING"></a>
### ERR_HTTP_INCOMING_SOCKET_ENCODING

An attempt to set text encoding of a HTTP request or response `Socket`
is not permitted. [RFC7230 Section 3][rfc7230-3]

<a id="ERR_HTTP_INVALID_HEADER_VALUE"></a>
### ERR_HTTP_INVALID_HEADER_VALUE

Expand Down Expand Up @@ -1696,3 +1702,5 @@ Creation of a [`zlib`][] object failed due to incorrect configuration.
[vm]: vm.html
[WHATWG Supported Encodings]: util.html#util_whatwg_supported_encodings
[`zlib`]: zlib.html

Copy link
Member

Choose a reason for hiding this comment

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

The added new line has to be removed again.

[rfc7230-3]: https://tools.ietf.org/html/rfc7230#section-3
7 changes: 7 additions & 0 deletions lib/_http_server.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ const {
const { IncomingMessage } = require('_http_incoming');
const {
ERR_HTTP_HEADERS_SENT,
ERR_HTTP_INCOMING_SOCKET_ENCODING,
ERR_HTTP_INVALID_STATUS_CODE,
ERR_INVALID_CHAR
} = require('internal/errors').codes;
Expand Down Expand Up @@ -383,6 +384,8 @@ function connectionListenerInternal(server, socket) {

// Override on to unconsume on `data`, `readable` listeners
socket.on = socketOnWrap;

Copy link
Member

Choose a reason for hiding this comment

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

Looks like this line has trailing space (should be 0 length).

socket.setEncoding = socketSetEncoding;
Copy link
Member

Choose a reason for hiding this comment

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

nit: either a comment or a new line here would be nice, since this line isn't related to

  // Override on to unconsume on `data`, `readable` listeners

Copy link
Contributor Author

@iSkore iSkore Mar 29, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I agree, because I didn't see that done in errors.md(aside from internal page links) I tried to follow suit. I'll update that.


// We only consume the socket if it has never been consumed before.
if (socket._handle) {
Expand Down Expand Up @@ -683,6 +686,10 @@ function onSocketPause() {
}
}

function socketSetEncoding() {
throw new ERR_HTTP_INCOMING_SOCKET_ENCODING('setEncoding');
Copy link
Member

Choose a reason for hiding this comment

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

There is no dynamic argument in this error, so there is no need to pass through any arguments.

}

function unconsume(parser, socket) {
if (socket._handle) {
if (parser._consumed)
Expand Down
3 changes: 3 additions & 0 deletions lib/internal/errors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -762,6 +762,9 @@ E('ERR_HTTP2_STREAM_SELF_DEPENDENCY',
E('ERR_HTTP2_UNSUPPORTED_PROTOCOL', 'protocol "%s" is unsupported.', Error);
E('ERR_HTTP_HEADERS_SENT',
'Cannot %s headers after they are sent to the client', Error);
E('ERR_HTTP_INCOMING_SOCKET_ENCODING',
'Changing the incoming socket encoding is not possible (RFC7230 Section 3)',
Error);
E('ERR_HTTP_INVALID_HEADER_VALUE',
'Invalid value "%s" for header "%s"', TypeError);
E('ERR_HTTP_INVALID_STATUS_CODE', 'Invalid status code: %s', RangeError);
Expand Down
23 changes: 23 additions & 0 deletions test/parallel/test-http-socket-encoding-error.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
'use strict';

const common = require('../common');
const http = require('http');

const server = http.createServer().listen(0, connectToServer);

server.on('connection', (socket) => {
common.expectsError(() => socket.setEncoding(''),
Copy link
Member

@apapirovski apapirovski Apr 19, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I think you might still be testing the old setEncoding here. I think you might be better off just using normal request callback and then using req.socket.setEncoding.

But maybe I'm missing something else that's off about this... not sure. It is throwing an AssertionError though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That req part isn't an option in the 'connection' event. Are you saying to test with an incoming request after the 'connection'? This wouldn't throw an error because the request is parsed by then.

For example:

http.createServer( ( req, res ) => {
    req.socket.setEncoding( 'utf8' );
    res.end( 'OK' );
} )

is ok to do. It's only "pre-parse" attempts to manipulate the incoming buffer

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, you can ignore me. It's complaining about the fact that you're providing an argument to the error, as mentioned above by @BridgeAR.

But if you're saying it's ok then this PR isn't quite valid because calling that req.socket.setEncoding('utf8') will still throw as things exist currently.

Copy link
Member

@apapirovski apapirovski Apr 19, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

But also this isn't strictly correct. It's definitely not OK to call req.socket.setEncoding until after the end event. (Basically: it's never OK within the lifetime of its usefulness so we might as well just throw unconditionally.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm. Do you know any history behind setEncoding and how it even came about?

Right now I override setEncoding in the connectionListenerInternal method. This causes a fatal error as detailed in #18118. Are you recommending we remove it completely from the HTTP module?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, we got our wires crossed. I'm just referencing this bit...

Are you saying to test with an incoming request after the 'connection'? This wouldn't throw an error because the request is parsed by then.

It would throw throughout the requests lifetime. Sockets that are used for http should never have their encoding changed. I'm just saying that this is also correct behaviour.

To simplify: I'm fine with this PR as is except for the linting issue and the one pointed out by @BridgeAR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This comment from @addaleax on the original PR suggests there are other uses of setEncoding when possibly responding to a protocol change request or connection upgrade requests.

There's a slight chance it could be necessary when using abstract protocols such as Internet Printing Protocol/1.0 or some other media protocols. Although, as I discussed in that PR, this should not be allowed in the scope of HTTP. HTTP in, HTTP out.

So this would require moving socket.setEncoding = socketSetEncoding; to somewhere in the httpSocketSetup method or something?

Copy link
Member

Choose a reason for hiding this comment

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

@iSkore Thanks for the link. I might need to think about this more. I think that particular use-case would be addressed by resetting the socket setEncoding back within the the upgrade code path but there might be others that are not addressed. This might also affect how modules like ws operate. Let me do some research or feel free to do it yourself — but it seems like this PR will need to be updated to account for at least upgrade, if not more.

{
code: 'ERR_HTTP_INCOMING_SOCKET_ENCODING',
type: Error
});

socket.end();
});

function connectToServer() {
const client = new http.Agent().createConnection(this.address().port, () => {
client.end();
})
.on('end', () => server.close());
}