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

Incorrect Content-Length header for HEAD requests #3084

Closed
tib opened this issue Oct 11, 2023 · 2 comments
Closed

Incorrect Content-Length header for HEAD requests #3084

tib opened this issue Oct 11, 2023 · 2 comments
Assignees
Labels
bug Something isn't working wontfix This will not be worked on

Comments

@tib
Copy link

tib commented Oct 11, 2023

Hello,

I strongly believe that I've discovered an issue related to HTTP HEAD requests.
Consider the following route handler:

app.on(.HEAD, "test") { req async throws -> Response in
    .init(
        status: .ok,
        version: req.version,
        headers: [
            "Content-Length": "42",
            "foo": "bar",
        ],
        body: .empty
    )
}

Now if you run a basic Vapor application using the snippet above and send the following curl request against the server:

curl -i --head http://localhost:8080/test  

According to the HTTP specification, you should get back 42 as a value for the Content-Length header, but since the body is empty, Vapor returns 0 as the header value.

This works as expected when I use other web frameworks, such as Hummingbird.

Anyway, thank you very much if you take a look at this issue, I'm happy to submit a PR if somebody gives me some guidenance where to fix this problem in the codebase. 🙏

@tib tib added the bug Something isn't working label Oct 11, 2023
@gwynne
Copy link
Member

gwynne commented Oct 11, 2023

Yep, this is a real issue. We currently rely on NIOHTTP1 to provide framing headers (including Content-Length), and as described by apple/swift-nio#2508, NIO currently can't handle this case properly. (Big thanks to @Lukasa for helping clarify this for me!)

What we can do is handle this in Vapor's part of the channel pipeline. I'll try to get a PR up for it shortly.

@gwynne gwynne self-assigned this Oct 11, 2023
@gwynne gwynne added the wontfix This will not be worked on label Oct 11, 2023
@gwynne
Copy link
Member

gwynne commented Oct 11, 2023

Belated correction: This is not an NIO issue at all. NIO actually does the right thing already here, and I misread where the problem was happening. It's actually an issue with how Vapor's Response is implemented. If you write your route like this instead, it works as intended:

app.on(.HEAD, "test") { req async in
    let res = Response(version: req.version, headers: ["foo": "bar"])
    res.headers.replaceOrAdd(name: .contentLength, value: "42")
    return res
}

(Note: The only critical change here is setting the content length separately; the other differences were just me removing redundancy.)

$ curl -i --head http://localhost:8080/test
HTTP/1.1 200 OK
foo: bar
content-length: 42
connection: keep-alive
date: Wed, 11 Oct 2023 17:09:23 GMT

There's nothing I can do about the awkwardness of this without breaking existing public API, unfortunately, so I have to close this issue as a wontfix 😕.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

2 participants