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

http2 - modifying compat req pseudo headers #15312

Closed
ronag opened this issue Sep 10, 2017 · 4 comments
Closed

http2 - modifying compat req pseudo headers #15312

ronag opened this issue Sep 10, 2017 · 4 comments
Labels
doc Issues and PRs related to the documentations. http2 Issues or PRs related to the http2 subsystem.

Comments

@ronag
Copy link
Member

ronag commented Sep 10, 2017

When modifying a compat request headers some properties that depend on pseudo headers stop working. I'm not sure if this would count as compat breaking?

e.g.

removeAllHeaders(req)
assert(req.url) // uhoh? This worked before http/2

@apapirovski

@mscdex mscdex added the http2 Issues or PRs related to the http2 subsystem. label Sep 10, 2017
@apapirovski
Copy link
Member

Thanks for the report @ronag! It looks like we might need to specifically protect url & method.

@apapirovski
Copy link
Member

apapirovski commented Sep 10, 2017

After thinking about this a bit longer, I'm not this can be protected in a way that's satisfactory. In h2, url & method are both a part of the headers unlike in http1. While the goal is to be as compatible with the http1 API as possible, I think there are going to be areas where the differences will come through a little bit. If someone is deleting all headers then that's probably outside of our control.

@mcollina Any thoughts? As I see this, :path and :method can't just be taken out of the headers object and we can't maintain a reasonable link between a protected value & the value within the headers object.

@mcollina mcollina added the doc Issues and PRs related to the documentations. label Sep 12, 2017
@mcollina
Copy link
Member

IMHO this needs to be documented, we cannot do much about it.
Would you like to send a PR?

cc @jasnell.

@jasnell
Copy link
Member

jasnell commented Sep 12, 2017

Yeah, I agree. Protecting those headers is certainly possible but not without a significant cost to performance that just would not be worth it. Documenting would be best.

jasnell added a commit to jasnell/node that referenced this issue Nov 26, 2017
MylesBorins pushed a commit that referenced this issue Dec 12, 2017
PR-URL: #17329
Fixes: #15312
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
MylesBorins pushed a commit that referenced this issue Dec 12, 2017
PR-URL: #17329
Fixes: #15312
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
MylesBorins pushed a commit that referenced this issue Dec 12, 2017
PR-URL: #17329
Fixes: #15312
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
gibfahn pushed a commit that referenced this issue Dec 19, 2017
PR-URL: #17329
Fixes: #15312
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
gibfahn pushed a commit that referenced this issue Dec 20, 2017
PR-URL: #17329
Fixes: #15312
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. http2 Issues or PRs related to the http2 subsystem.
Projects
None yet
Development

No branches or pull requests

5 participants