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

Correctly handle responses to HEAD requests that use Content-Length #3456

Merged
merged 3 commits into from
Oct 24, 2020

Conversation

zenhack
Copy link
Collaborator

@zenhack zenhack commented Oct 22, 2020

Previously, if the bridge saw a Content-Length header, but the length of
the body did not match, it would throw an error. However, for HEAD
requests this is incorrect, so with this patch the check only applies to
non-HEAD requests; for HEAD requests we will expect an empty body.

This also changes WebSessionBridge to ignore the body of HEAD requests

Fixes #3454


@rs22, want to test?

@ocdtrekkie ocdtrekkie added app-platform App/Sandstorm integration features ready-for-review We think this is ready for review labels Oct 22, 2020
@rs22
Copy link
Contributor

rs22 commented Oct 22, 2020

Thanks a lot! This solves my problem 🚀

I noticed that the Content-Length header in HEAD responses still is not passed on to the requesting client by Sandstorm. This is no problem in my particular case, but other applications might depend on this?

The RFC says that it's optional:

A server MAY send a Content-Length header field in a response to a HEAD request

from https://tools.ietf.org/html/rfc7230#section-3.3.2

The server SHOULD send the same header fields in response to a HEAD request as it would have sent if the request had been a GET, except that the payload header fields MAY be omitted.

from https://tools.ietf.org/html/rfc7231#section-4.3.2

Previously, if the bridge saw a Content-Length header, but the length of
the body did not match, it would throw an error. However, for HEAD
requests this is incorrect, so with this patch the check only applies to
non-HEAD requests; for HEAD requests we will expect an empty body.

This also changes WebSessionBridge to ignore the body of HEAD requests

Fixes sandstorm-io#3454
@zenhack
Copy link
Collaborator Author

zenhack commented Oct 22, 2020

Yeah, that's a bit fiddly to do since right now the WebSession API infers the Content-Length from the actual size of the body -- I think it would work to use the streaming api and just call expectSize() without actually writing the data afterwards, but that's a bit gross because the docs for ByteStream suggest that that should be considered an error... probably a "cleaner" solution would be to tweak the interfaces somehow, but I'm inclined to say that should probably be a follow-on issue then.

Copy link
Member

@kentonv kentonv left a comment

Choose a reason for hiding this comment

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

I think the changes to web-session-bridge.c++ are redundant as KJ's HTTP implementation will already ignore the body for a HEAD request. If so can we revert that part?

src/sandstorm/web-session-bridge.c++ Outdated Show resolved Hide resolved
src/sandstorm/web-session-bridge.c++ Outdated Show resolved Hide resolved
@ocdtrekkie ocdtrekkie removed the ready-for-review We think this is ready for review label Oct 24, 2020
zenhack and others added 2 commits October 24, 2020 16:39
Suggested by @kentonv

Co-authored-by: Kenton Varda <kenton@sandstorm.io>
This is already handled by kj's http support, so no need to do it
in web-session-bridge.
@zenhack
Copy link
Collaborator Author

zenhack commented Oct 24, 2020

I think the changes to web-session-bridge.c++ are redundant as KJ's HTTP implementation will already ignore the body for a HEAD request. If so can we revert that part?

Looks like you're right, done.

@ocdtrekkie ocdtrekkie added the ready-for-review We think this is ready for review label Oct 24, 2020
@kentonv kentonv merged commit eab89cb into sandstorm-io:master Oct 24, 2020
@ocdtrekkie ocdtrekkie removed the ready-for-review We think this is ready for review label Oct 24, 2020
@zenhack zenhack deleted the fix-3454 branch October 24, 2020 23:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
app-platform App/Sandstorm integration features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Server responds to HEAD requests with HTTP 500
4 participants