Safelist request headers starting with Sec-

[yoavweiss]

As discussed with @annevk on w3c/resource-hints#74 (comment), this PR makes sure that Sec- prefixed request headers are always CORS-safe.

---

Preview | Diff

[toyoshim]

Yoav,
Sec- prefix is already in the Fetch spec; https://fetch.spec.whatwg.org/#forbidden-header-name .
See my comments in the previous thread. Sec- prefix should not be in the safelist, but in the forbidden header name list. Header names in the latter list are not permitted for JavaScript to set from XHR or Fetch API, but only user-agents can set. So since the name is 'forbidden', in terms of CORS request, it's allowed for user-agents to use and it does not trigger a CORS preflight.

[yoavweiss]

See my comments in the previous thread

Any pointers to that thread?

Sec- prefix should not be in the safelist, but in the forbidden header name list.

IIUC, it should be in both. I agree only user agents should be able to set it (hence it should be in the forbidden list), but then, when a user agent invokes e.g. main fetch, in step 5, it needs to verify that the CORS unsafe request header names list is empty. That, in turn, triggers the CORS safelisted request header algorithm on all header fields, checking if they are safe-listed. Not including Sec- in that last check would mean preflights would trigger for those requests.

Since we want to avoid those preflights, I think sec- checks should be in both the safelist and the forbidden list.

[annevk]

So the flow you seem to want is that a context can set a Sec--prefixed header on a request and pass that to fetch, rather than pass some data to fetch that fetch then turns into a Sec--prefixed header before hitting the network.

The problem with this approach is service workers. You've now put privileged headers in a Request object. Trying to do anything with that Request object will get those headers removed due to the way the Request constructor operates and https://fetch.spec.whatwg.org/#concept-headers-append in particular which it invokes.

Perhaps that is what should happen as the context is different too, but I strongly suspect it's not what you intended to happen.

[yoavweiss]

OK, so the right flow would be that any Sec- headers are added as part of Fetch's algorithms?

[annevk]

I haven't really put a lot of thought into that. This came to mind when I tried to remember what problems we've run into with trying to extend this in the past.

[toyoshim]

w3c/resource-hints#74 (comment)
This is the thread I meant.

Yep, since the user-agents' managed headers are injected later but before making network requests, they won't appear in the header list when we run several algorithms for the fetch and XHR specs, and these headers don't have a chance to set unsafe-request flag or use-CORS-preflight flag. That's my understanding.

But, if you need to say something explicitly, can we say that 'if the header name matches the forbidden header names' rather than 'if the header name is Sec-*'? Actually, that's Chrome implementation.
https://cs.chromium.org/chromium/src/services/network/cors/cors_url_loader.cc?q=NeedsPreflight
https://cs.chromium.org/chromium/src/services/network/public/cpp/cors/cors.cc?q=CorsUnsafeNotForbiddenRequestHeaderNames

[yoavweiss]

@annevk

I haven't really put a lot of thought into that. This came to mind when I tried to remember what problems we've run into with trying to extend this in the past.

In the current Client Hints PR, the CH headers are added as part of fetch steps 1.8.3

IIUC, since main fetch runs below that, and this is where the safelist test happens, I'd need to move it lower, after the safelist test.

I can definitely do that. And you're right that if all UA-added headers will be added below that check, there's no need to add the current clause. I'll close this PR, and shuffle things around in the CH PR.

@toyoshim

Yep, since the user-agents' managed headers are injected later but before making network requests, they won't appear in the header list

Yeah, I'll move my header additions lower, so that they won't require such an exception.

But, if you need to say something explicitly, can we say that 'if the header name matches the forbidden header names' rather than 'if the header name is Sec-*'?

Good point, but I think we can avoid it altogether now. Thanks!

[annevk]

@yoavweiss not just below the safelist test, below handing things off to service workers.

[yoavweiss]

@yoavweiss not just below the safelist test, below handing things off to service workers.

That's not what we discussed in the past. Client Hints need to be observable by SWs in order to satisfy their use-cases, and in order for SW to respond to those resources from the cache properly.

[annevk]

I know, see #880 (comment) though.

[yoavweiss]

Can't we add the values before as well as after SWs? (in case they were removed)

[annevk]

Maybe? It's not immediately clear to me that wouldn't create other problems.

[jakearchibald]

Is the pattern we used for Range useful here? https://fetch.spec.whatwg.org/#privileged-no-cors-request-header-name

[annevk]

@jakearchibald that would not prevent a preflight for requests whose mode is "cors". Though maybe it should?

[yoavweiss]

The problem with this approach is service workers. You've now put privileged headers in a Request object. Trying to do anything with that Request object will get those headers removed due to the way the Request constructor operates and https://fetch.spec.whatwg.org/#concept-headers-append in particular which it invokes.

My understanding going over that constructor algorithm is that:

• Step 13 copies the cloned input request's header list to the new request's headers list
• Step 31 creates a new Headers object from that headers list, which includes those Sec- headers.
• Step 32 then appends those headers to the new Headers object, only if init is not empty

Is that correct? If so, is that the intended behavior?

I feel like adding Sec- to the safe list, and allowing it to be copied from the original Request (but not from RequestInit) somewhere around step 32 would have solved what we're trying to do here.

[annevk]

Yeah, I overlooked the fact that if you do not modify the request the headers are preserved. My bad.

[yoavweiss]

Is that the intended behavior?

If it is, than I'd assume step 32.2 should skip the other substeps if init["headers"] doesn't exist (and then we're good as far as Sec- headers go)

If it isn't and we need to append all the headers anyway, maybe we can carve out Sec- or forbidden headers in general that come from the Request and not from init?

[annevk]

No, if any init members are set, then headers are to be "reinitialized". Only if it's a "pass-through" request things can stay the way they were. I thought that was okay for your scenario though?

[toyoshim]

Any update?
If this algorithm discussion will take a long time, can we standardize the 'purpose' header alternative name separately?

i.e., any chance to have 'Sec-Fetch-Purpose' in the 'Fetch Metadata Request Headers' spec?
https://w3c.github.io/webappsec-fetch-metadata (cc: @mikewest)
Sounds like the use of Purpose header can be classified into the same group.

[yoavweiss]

Obsoleted by #1000