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

Should synthetic responses be validated? #1571

Closed
annevk opened this issue Mar 10, 2021 · 8 comments
Closed

Should synthetic responses be validated? #1571

annevk opened this issue Mar 10, 2021 · 8 comments

Comments

@annevk
Copy link
Member

annevk commented Mar 10, 2021

In web-platform-tests/wpt#27837 (comment) @mfalken suggested that synthetic responses should receive validation similar to network responses. A simple example I can think of would be:

e.respondWith(new Response("...", { headers: [["content-length":"1"],["content-length":"2"]] }))

I tend to think this should be fine and we just ignore the content-length header as it's pointless for synthetic responses. Am I missing something here?

(I'm going to merge that PR as it matches the specification as written, but I figured I should at least file a follow-up.)

@annevk annevk changed the title Should synthetic responses be validated Should synthetic responses be validated? Mar 10, 2021
@wanderview
Copy link
Member

What do we do if the server gives us a malformed content-length? (I assume in this case the values get combined into content-length:1,2?)

@annevk
Copy link
Member Author

annevk commented Mar 10, 2021

In the case where they don't match it becomes a network error in HTTP/1 (maybe also other versions, unclear). https://fetch.spec.whatwg.org/#content-length-header is the start toward documenting that.

However, we are not using Content-Length to parse the response as we do when a response comes from the network. E.g., a Content-Length on a synthetic response that's too long or too short doesn't influence the body of the response one way or another. So I don't think looking for precedent there is meaningful.

@wanderview
Copy link
Member

Ignoring it sounds good to me. If for no other reason trying to lock it down now would require work to measure that its compatible. Without a compelling reason to do so, it seems easier to leave the status quo.

@annevk
Copy link
Member Author

annevk commented Mar 10, 2021

That makes sense to me, but note that browsers do something weird for the case above, or at least when the headers have an identical value. Rather than combine into 1, 1 I'd just get 1. I found the cause for this in Firefox, but haven't tried to look into it for Chrome. And Chrome only does it for synthetic responses.

@jakearchibald
Copy link
Contributor

jakearchibald commented Mar 12, 2021

In the case where they don't match it becomes a network error in HTTP/1

This is key. Some headers relate to the transport, others relate to web platform features. Things that relate to the transport no longer apply when they're coming from the HTTP cache or a synthetic response, and it feels like Content-Length is part of that.

Same goes for things like chunked encoding, and caching headers.

Here's a practical example of why we shouldn't validate Content-Length:

addEventListener('fetch', (event) => {
  event.respondWith((async function() {
    const response = await fetch(event.request);
    // Let's pretend the response isn't opaque 
    const body = await response.blob();
    return new Response(body, { headers: response.headers });
  })());
});

In this case I'm creating a new synthetic response with the same headers as the original response. However, this will also copy things like Content-Length and Content-Encoding. The original Content-Length may be 100123 due to brotli compression, but the synthetic response's body may be many megabytes.

@annevk
Copy link
Member Author

annevk commented Mar 12, 2021

Well, it sounds like we're on the same page. If @mfalken still feels differently that'd be good to know, but otherwise I think this can be closed.

And to be clear, I think the resolution here is that where headers are parsed and have an effect is a case-by-case decision and you can tell from where it happens in fetch (or callers of fetch) whether it will impact synthetic responses.

@mfalken
Copy link
Member

mfalken commented Mar 15, 2021

Yep I chatted with Jake and it makes sense to me to not validate. To be clear, in the example in #1571 (comment), is the expected behavior to coalesce "content-length" into "1,2" or to just take the first value, or something else?

@annevk
Copy link
Member Author

annevk commented Mar 15, 2021

If you "get" the header the value would be 1, 2 (including space). This is what a web developer would see through the Headers API.

The browser gets the header in the networking stack for response processing. That would not apply here as the networking stack is not involved.

The browser also gets the header in XMLHttpRequest, to set up progress events. That does apply here. As 1, 2 is failure per https://fetch.spec.whatwg.org/#header-list-extract-a-length XMLHttpRequest would say that the length is not computable for its progress events.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants