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

UnhandledPromiseRejection: Improper headers on http2 response #3830

Closed
FallingSnow opened this issue Oct 15, 2018 · 6 comments
Closed

UnhandledPromiseRejection: Improper headers on http2 response #3830

FallingSnow opened this issue Oct 15, 2018 · 6 comments
Assignees
Labels
bug Bug or defect

Comments

@FallingSnow
Copy link

FallingSnow commented Oct 15, 2018

Are you sure this is an issue with the hapi core module or are you just looking for some help?

Issue with hapi core.

Is this a security related issue?

No.

What are you trying to achieve or the steps to reproduce?

Make a http2 request that triggers the following:

!(isInjection || request._core.started) ||
(request._isPayloadPending && !request.raw.req._readableState.ended)

What was the result you received?

No response to request.

(node:8018) UnhandledPromiseRejectionWarning: TypeError [ERR_HTTP2_INVALID_CONNECTION_HEADERS]: HTTP/1 Connection specific headers are forbidden: "connection"
    at mapToHeaders (internal/http2/util.js:453:16)
    at ServerHttp2Stream.respond (internal/http2/core.js:2273:25)
    at Http2ServerResponse.[begin-send] (internal/http2/compat.js:691:19)
    at Http2ServerResponse.writeHead (internal/http2/compat.js:589:21)
    at Object.internals.writeHead ([redacted]/server/node_modules/hapi/lib/transmit.js:340:13)
    at Object.internals.transmit ([redacted]/server/node_modules/hapi/lib/transmit.js:105:15)
    at Object.internals.fail ([redacted]/server/node_modules/hapi/lib/transmit.js:79:22)
    at process._tickCallback (internal/process/next_tick.js:68:7)

What did you expect?

A response of some kind, if this error was handled, an error response.

Context

Fix

From MDN:

Also, Connection and Keep-Alive are ignored in HTTP/2; connection management is handled by other mechanisms there.

Replace:

hapi/lib/transmit.js

Lines 96 to 101 in 7ffd301

const isInjection = Shot.isInjection(request.raw.req);
if (!(isInjection || request._core.started) ||
(request._isPayloadPending && !request.raw.req._readableState.ended)) {
response._header('connection', 'close');
}

With:

const isH2 = request.httpVersion === '2.0';
const isInjection = Shot.isInjection(request.raw.req);
if (!isH2 && (!(isInjection || request._core.started) ||
   (request._isPayloadPending && !request.raw.req._readableState.ended))) {

   response._header('connection', 'close');
}

Afterthoughts

Should this even be allowed to happen? This would suggest that somewhere along the response chain a possible error is not accounted for. What should have really happened is an internal error response to the client and an error message to the console stating: TypeError [ERR_HTTP2_INVALID_CONNECTION_HEADERS]: HTTP/1 Connection specific headers are forbidden: "connection".

@kanongil
Copy link
Contributor

There are some bits of hapi internals that are not guarded against promise rejections. Mostly, this is not a problem due to the extensive test suite, but occasionally something like this can slip through.

Normally, hapi works similar to what you expect, however you have run into a corner case, where an already errorred transmission, causes another error in the attempt to send the error response.

@kanongil kanongil added the bug Bug or defect label Oct 16, 2018
@kanongil
Copy link
Contributor

kanongil commented Oct 16, 2018

The fact that adding a connection: close header to the response, causes it to throw an error, seems like a bug in the nodejs http2 Compatibility API.

@FallingSnow
Copy link
Author

FallingSnow commented Oct 16, 2018

There are some bits of hapi internals that are not guarded against promise rejections.

Is this an issue or is this just accepted behaviour?

The fact that adding a connection: close header to the response, causes it to throw an error, seems like a bug in the nodejs http2 Compatibility API.

I agree, I think the fact that adding a connection header causes the response (I assume) to fail breaks the compatibility API. I assume there is a reason they fail rather than just ignoring it though.

Either way I suppose this is something that should be brought upstream but for the time being I feel the changes I purposed should be implemented, hopefully as a short term patch. If the node team does decide that this is intended behaviour then it just becomes a permanent patch.

However before I open an issue upstream, the compatibility API states:

This API targets only the public API of the HTTP/1. However many modules use internal methods or state, and those are not supported as it is a completely different implementation.

Do you know if the connection header falls under internal methods or state?

FallingSnow added a commit to FallingSnow/hapi that referenced this issue Oct 17, 2018
@kanongil
Copy link
Contributor

Do you know if the connection header falls under internal methods or state?

Definitely not. It's just an argument for the very public response.setHeader() API method.

@kanongil
Copy link
Contributor

It seems that a node core fix is on its way for your actual issue: nodejs/node#23908.

Even when fixed, this still leaves hapi with potential UnhandledPromiseRejection errors if something throws while transmitting a "transmission error" error response.

@hueniverse
Copy link
Contributor

Replaced by #3974

@lock lock bot locked as resolved and limited conversation to collaborators Mar 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Bug or defect
Projects
None yet
Development

No branches or pull requests

3 participants