issue 571722

[Andrew Brown]

Hi, external hopefully-contributor-soon here. I've had a solid look at crbug 571722 recently and I believe it's pretty fixable, including the blocking issue 595993, because the grand-father blocking issue 963260 has been incidentally implemented by something else.

(links)

https://bugs.chromium.org/p/chromium/issues/detail?id=571722

https://bugs.chromium.org/p/chromium/issues/detail?id=595993

https://bugs.chromium.org/p/chromium/issues/detail?id=963260

and then https://bugs.chromium.org/p/chromium/issues/detail?id=940331#c39 for the comment suggesting |cors_exempt_headers| as a possible fix for 963260.

I've put together a patch that fixes 571722 and I believe checks off the user-agent half of 595993, but I'm holding off on submitting a CL while I iron out a few final kinks and do a bit of testing. I'm expecting to have to go back in and add Origin to close out 595993 properly, but I'll cross that bridge when I get there. Is there anything else I should be aware of, anything I could miss, suggestions, thoughts, etc, before I submit a CL? (I reckon I'd be ambitious to say I'll submit this week, at the pace I'm working.)

Also, kinda new to this sort of thing, so apologies if this should be elsewhere or what have you. Cheers :)

[Adam Rice]

I think you're in the right place.

If https://crbug.com/963260 is implemented, could you close it with a comment linking to the CL(s) that did it?

Feel free to upload a rough patch if you'd like to get early feedback, although I can't guarantee I'd get to it quickly.

--
You received this message because you are subscribed to the Google Groups "blink-network-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to blink-network-...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-network-dev/c9155443-57c5-40e4-90f6-0a2b5c8ae82en%40chromium.org.

[Andrew Brown]

That's a relief :)

I'll be honest, I don't know if 963260 is fully implemented, or just enough that I can hide headers from a ServiceWorker. The concept of header layer division is fairly vague in the spec (..."most headers are set in the network and cache layer" - which ones?). I'd want to look into it a fair bit more before actually trying to close the bug and certify that it's actually completed. Just to cover it fully, like, do we then need tests to cover the expected behaviour? Better documentation so it's clearer what code lines up with different spec concepts? Are all the browser-controlled headers mentioned in the fetch spec using the layering correctly? I'm not sure.

My patch is currently in a state of "it works on my machine" - if I compile it and run, I can pull up the JS console and make fetch requests with the user-agent following the spec (permitting modification and sending cors preflights as necessary), but it still fails the relevant auto-tests, so I'm a little hesitant to put it up for review. I'll push something up this week, but absolutely no worries about review speed - this is just out of interest for me, so I understand if you've got bigger fish to fry.

[Adam Rice]

Just to cover it fully, like, do we then need tests to cover the expected behaviour? Better documentation so it's clearer what code lines up with different spec concepts? Are all the browser-controlled headers mentioned in the fetch spec using the layering correctly? I'm not sure.

One option would be to close the original issue and file new issue(s) for the follow-up work.

It would be good to at least have web-platform-tests so that we can see if we are aligned with other browsers. But this is getting outside the scope of what you are working on.

Thanks for working on this.

[Andrew Brown]

Just put up a rough CL - apologies for disappearing for three weeks; I got busy, and then my CORS preflights were being cached, so I thought the changes had randomly stopped working.
https://chromium-review.googlesource.com/c/chromium/src/+/5273743

It's *very* rough, fair warning. Hopefully it's enough that I can get guidance on my lingering questions:

When I place a header onto cors_exempt_headers in third_party/blink/renderer/platform/loader/fetch/url_loader/request_conversion.cc this triggers a secondary check outside blink in services/network/cors/cors_url_loader_factory.cc. I'm unsure how other headers pass this check (e.g. X-Requested-With), as the only headers on the list were Google-specific ones. I've cobbled in a quick-and-dirty override so that I could test the rest of the changes, but if you could point me towards "doing it properly" that would be much appreciated.

Secondarily, the tests are still behaving strangely (i.e. failing). Is it possible that this change actually needs web tests to be updated? It seems possible that the testing web-server that the requests are going to doesn't realise it has to respond with an OPTIONS request for User-Agent now. That assumption still doesn't exactly line up with what I'm seeing, though (I would expect an error response, not just an empty user-agent), so again - any guidance would be massively appreciated.

I'm still flitting in and out of activity on this, so no rush on a review or anything :) Cheers for helping me out.