Allow fetch/XMLHttpRequest APIs to set a user-agent [chromium/src : main]
Adam Rice (Gerrit)
Adam Rice added 1 comment![]( "Open in Gerrit")
header.key == "User-Agent") {You should add "User-Agent" to NetworkContext::cors_exempt_header_list somewhere rather than special-casing it.
Related details
- Andrew Brown
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Brown (Gerrit)
Andrew Brown uploaded new patchset![]( "Open in Gerrit")
Related details
- Andrew Brown
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Brown (Gerrit)
Andrew Brown added 1 comment![]( "Open in Gerrit")
header.key == "User-Agent") {You should add "User-Agent" to NetworkContext::cors_exempt_header_list somewhere rather than special-casing it.
I wasn't sure about adding it to that list, as the headers on that list seem to be almost exclusively "variations headers" for whatever lives in components/variations.
I've added it in SystemNetworkContextManager::ConfigureDefaultNetworkContextParams.
Can't shake the feeling I should be leaving the browser user-agent till later - I noticed NetworkContext has a user_agent field, also filled out in ConfigureDefaultNetworkContextParams. I might investigate that and see if it's possible for the browser user-agent to be set from that value instead of setting it in third_party/blink/renderer/platform/loader/fetch/url_loader/request_conversion.cc.
Related details
- Adam Rice
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Adam Rice (Gerrit)
Adam Rice added 1 comment![]( "Open in Gerrit")
header.key == "User-Agent") {Andrew BrownYou should add "User-Agent" to NetworkContext::cors_exempt_header_list somewhere rather than special-casing it.
I wasn't sure about adding it to that list, as the headers on that list seem to be almost exclusively "variations headers" for whatever lives in components/variations.
I've added it in SystemNetworkContextManager::ConfigureDefaultNetworkContextParams.
Can't shake the feeling I should be leaving the browser user-agent till later - I noticed NetworkContext has a user_agent field, also filled out in ConfigureDefaultNetworkContextParams. I might investigate that and see if it's possible for the browser user-agent to be set from that value instead of setting it in third_party/blink/renderer/platform/loader/fetch/url_loader/request_conversion.cc.
We need to use the value from the render process, as it may have been overridden by DevTools. I don't know if there is any way to access the DevTools override from within the network service. My guess would be not.
Another method would be to add a new `default_user_agent` field to `network::ResourceRequest`, but it would require changes in quite a few places.
Related details
- Andrew Brown
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Adam Rice (Gerrit)
Adam Rice added 1 comment![]( "Open in Gerrit")
Please run
```
third_party/blink/tools run_web_tests.py external/wpt/fetch --reset
```
to reset the test expectations and include any changes to *-expected.txt files in the CL.
Related details
- Andrew Brown
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Brown (Gerrit)
Andrew Brown uploaded new patchset![]( "Open in Gerrit")
Related details
- Andrew Brown
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Brown (Gerrit)
Andrew Brown added 2 comments![]( "Open in Gerrit")
Please run
```
third_party/blink/tools run_web_tests.py external/wpt/fetch --reset
```
to reset the test expectations and include any changes to *-expected.txt files in the CL.
Just done this (apologies for delay again).
There's a *lot* of changes. Like, way more than expected. Is it possible that the test-running processes need an update with this change? I imagine it might be possible that a heap of blink web tests are now "failing" because the runner doesn't adequately respond with CORS headers for custom user-agents.
Maybe it is OK, I'm not sure.
header.key == "User-Agent") {Andrew BrownYou should add "User-Agent" to NetworkContext::cors_exempt_header_list somewhere rather than special-casing it.
Adam RiceI wasn't sure about adding it to that list, as the headers on that list seem to be almost exclusively "variations headers" for whatever lives in components/variations.
I've added it in SystemNetworkContextManager::ConfigureDefaultNetworkContextParams.
Can't shake the feeling I should be leaving the browser user-agent till later - I noticed NetworkContext has a user_agent field, also filled out in ConfigureDefaultNetworkContextParams. I might investigate that and see if it's possible for the browser user-agent to be set from that value instead of setting it in third_party/blink/renderer/platform/loader/fetch/url_loader/request_conversion.cc.
We need to use the value from the render process, as it may have been overridden by DevTools. I don't know if there is any way to access the DevTools override from within the network service. My guess would be not.
Another method would be to add a new `default_user_agent` field to `network::ResourceRequest`, but it would require changes in quite a few places.
Oh, true, I'd completely forgotten that DevTools lets you override your browser's default.
...Hm, should *that* value be CORS-exempt? (In this CL, it will be.) Probably a question for the whatwg peoplem, not here.
Related details
- Adam Rice
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Adam Rice (Gerrit)
Adam Rice added 2 comments![]( "Open in Gerrit")
Andrew BrownPlease run
```
third_party/blink/tools run_web_tests.py external/wpt/fetch --reset
```
to reset the test expectations and include any changes to *-expected.txt files in the CL.
Just done this (apologies for delay again).
There's a *lot* of changes. Like, way more than expected. Is it possible that the test-running processes need an update with this change? I imagine it might be possible that a heap of blink web tests are now "failing" because the runner doesn't adequately respond with CORS headers for custom user-agents.
Maybe it is OK, I'm not sure.
It looks like that didn't work right, since the expectations for a lot of unrelated tests are being deleted. I've started a Dry Run of the bots to see what they say.
header.key == "User-Agent") {Andrew BrownYou should add "User-Agent" to NetworkContext::cors_exempt_header_list somewhere rather than special-casing it.
Adam RiceI wasn't sure about adding it to that list, as the headers on that list seem to be almost exclusively "variations headers" for whatever lives in components/variations.
I've added it in SystemNetworkContextManager::ConfigureDefaultNetworkContextParams.
Can't shake the feeling I should be leaving the browser user-agent till later - I noticed NetworkContext has a user_agent field, also filled out in ConfigureDefaultNetworkContextParams. I might investigate that and see if it's possible for the browser user-agent to be set from that value instead of setting it in third_party/blink/renderer/platform/loader/fetch/url_loader/request_conversion.cc.
Andrew BrownWe need to use the value from the render process, as it may have been overridden by DevTools. I don't know if there is any way to access the DevTools override from within the network service. My guess would be not.
Another method would be to add a new `default_user_agent` field to `network::ResourceRequest`, but it would require changes in quite a few places.
Oh, true, I'd completely forgotten that DevTools lets you override your browser's default.
...Hm, should *that* value be CORS-exempt? (In this CL, it will be.) Probably a question for the whatwg peoplem, not here.
DevTools is generally considered part of the browser, so a User-Agent set by DevTools has the same authority as one set by the browser itself.
Related details
- Andrew Brown
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Adam Rice (Gerrit)
Andrew Brown has uploaded the change for review![]( "Open in Gerrit")
Adam Rice would like Takashi Toyoshima to review this change authored by Andrew Brown.
Commit message
Allow fetch/XMLHttpRequest APIs to set a user-agent
Per spec, fetch & XMLHttpRequest should be able to set custom
user-agents. This fix has been blocked as API-set user-agents should
cause CORS preflight checks, but browser-set ones should not. We can now
preserve this behavior using cors_exempt_headers.
See discussion at https://groups.google.com/a/chromium.org/g/blink-network-dev/c/iJLX5X3r6As
Change diff
Change information
- M AUTHORS
- M chrome/browser/net/system_network_context_manager.cc
- M net/http/http_util.cc
- M third_party/blink/renderer/core/loader/frame_fetch_context.cc
- M third_party/blink/renderer/core/loader/worker_fetch_context.cc
- M third_party/blink/renderer/platform/loader/fetch/resource_request.h
- M third_party/blink/renderer/platform/loader/fetch/url_loader/request_conversion.cc
- D third_party/blink/web_tests/external/wpt/fetch/api/abort/general.any-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/api/abort/general.any.serviceworker-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/api/abort/general.any.sharedworker-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/api/abort/general.any.worker-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/api/abort/request.any-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/api/abort/request.any.serviceworker-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/api/abort/request.any.sharedworker-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/api/abort/request.any.worker-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/api/basic/header-value-combining.any.serviceworker-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/api/basic/http-response-code.any-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/api/basic/http-response-code.any.serviceworker-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/api/basic/http-response-code.any.sharedworker-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/api/basic/http-response-code.any.worker-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/api/basic/scheme-blob.sub.any-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/api/basic/scheme-blob.sub.any.worker-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/api/body/formdata.any-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/api/body/formdata.any.worker-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/api/body/mime-type.any-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/api/body/mime-type.any.worker-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/api/cors/cors-preflight-redirect.any-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/api/cors/cors-preflight-redirect.any.sharedworker-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/api/cors/cors-preflight-redirect.any.worker-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/api/cors/cors-preflight-star.any-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/api/cors/cors-preflight-star.any.worker-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/api/cors/cors-preflight.any-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/api/cors/cors-preflight.any.worker-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/api/idlharness.any-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/api/idlharness.any.serviceworker-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/api/idlharness.any.sharedworker-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/api/idlharness.any.worker-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/api/policies/referrer-origin-when-cross-origin-service-worker.https-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/api/redirect/redirect-empty-location.any-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/api/redirect/redirect-empty-location.any.serviceworker-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/api/redirect/redirect-empty-location.any.sharedworker-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/api/redirect/redirect-empty-location.any.worker-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/api/redirect/redirect-location.any-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/api/redirect/redirect-location.any.serviceworker-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/api/redirect/redirect-location.any.sharedworker-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/api/redirect/redirect-location.any.worker-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/api/redirect/redirect-origin.any-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/api/redirect/redirect-origin.any.worker-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/api/request/request-consume-empty.any-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/api/request/request-consume-empty.any.serviceworker-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/api/request/request-consume-empty.any.sharedworker-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/api/request/request-consume-empty.any.worker-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/api/request/request-disturbed.any-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/api/request/request-disturbed.any.serviceworker-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/api/request/request-disturbed.any.sharedworker-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/api/request/request-disturbed.any.worker-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/api/request/request-error.any-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/api/request/request-error.any.serviceworker-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/api/request/request-error.any.sharedworker-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/api/request/request-error.any.worker-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/api/request/request-headers.any-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/api/request/request-headers.any.serviceworker-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/api/request/request-headers.any.sharedworker-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/api/request/request-headers.any.worker-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/api/request/request-reset-attributes.https-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/api/request/request-structure.any-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/api/request/request-structure.any.serviceworker-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/api/request/request-structure.any.sharedworker-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/api/request/request-structure.any.worker-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/api/response/response-body-read-task-handling-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/api/response/response-consume-empty.any-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/api/response/response-consume-empty.any.serviceworker-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/api/response/response-consume-empty.any.sharedworker-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/api/response/response-consume-empty.any.worker-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/api/response/response-error-from-stream.any-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/api/response/response-error-from-stream.any.serviceworker-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/api/response/response-error-from-stream.any.sharedworker-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/api/response/response-error-from-stream.any.worker-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/api/response/response-stream-disturbed-by-pipe.any-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/api/response/response-stream-disturbed-by-pipe.any.serviceworker-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/api/response/response-stream-disturbed-by-pipe.any.sharedworker-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/api/response/response-stream-disturbed-by-pipe.any.worker-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/content-type/script.window-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/corb/response_block.tentative.https-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/data-urls/processing.any-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/data-urls/processing.any.serviceworker-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/data-urls/processing.any.sharedworker-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/data-urls/processing.any.worker-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/h1-parsing/lone-cr.window-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/h1-parsing/status-code.window-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/metadata/generated/appcache-manifest.https.sub-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/metadata/generated/element-a.https.sub-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/metadata/generated/element-area.https.sub-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/metadata/generated/element-audio.https.sub-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/metadata/generated/element-embed.https.sub-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/metadata/generated/element-img.https.sub-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/metadata/generated/element-input-image.https.sub-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/metadata/generated/element-link-prefetch.https.optional.sub-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/metadata/generated/element-video.https.sub-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/metadata/generated/header-link.https.sub-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/metadata/generated/header-link.https.sub.tentative-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/metadata/generated/header-link.sub-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/metadata/generated/svg-image.https.sub-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/orb/tentative/content-range.sub.any-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/orb/tentative/content-range.sub.any.worker-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/orb/tentative/img-mime-types-coverage.tentative.sub-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/orb/tentative/known-mime-type.sub.any.worker-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/orb/tentative/nosniff.sub.any-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/orb/tentative/nosniff.sub.any.worker-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/orb/tentative/status.sub.any-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/orb/tentative/status.sub.any.worker-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/origin/assorted.window-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/private-network-access/anchor.tentative.window-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/private-network-access/fenced-frame-no-preflight-required.tentative.https.window-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/private-network-access/fenced-frame.tentative.https.window-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/private-network-access/iframe.tentative.window-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/private-network-access/mixed-content-fetch.tentative.https.window-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/private-network-access/nested-worker.tentative.https.window-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/private-network-access/nested-worker.tentative.window-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/private-network-access/shared-worker-blob-fetch.tentative.https.window-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/private-network-access/shared-worker-blob-fetch.tentative.window-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/private-network-access/shared-worker-fetch.tentative.https.window-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/private-network-access/shared-worker-fetch.tentative.window-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/private-network-access/shared-worker.tentative.window-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/private-network-access/websocket.tentative.window-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/private-network-access/window-open-existing.tentative.window-expected.txt
- M third_party/blink/web_tests/external/wpt/fetch/private-network-access/window-open.tentative.https.window-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/private-network-access/window-open.tentative.window-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/private-network-access/worker-fetch.tentative.https.window-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/private-network-access/worker-fetch.tentative.window-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/private-network-access/worker.tentative.window-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/range/blob.any-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/range/blob.any.worker-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/range/general.any.serviceworker-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/range/general.window-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/range/sw.https.window-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/redirects/subresource-fragments-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/security/1xx-response.any-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/security/1xx-response.any.worker-expected.txt
- D third_party/blink/web_tests/external/wpt/fetch/security/dangling-markup/dangling-markup-mitigation-allowed-apis-expected.txt
- D third_party/blink/web_tests/platform/linux/external/wpt/fetch/orb/tentative/known-mime-type.sub.any-expected.txt
Related details
- Andrew Brown
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Adam Rice (Gerrit)
Adam Rice added 2 comments![]( "Open in Gerrit")
+toyoshim to get a second opinion on whether this use of cors_exempt_header_list makes sense.
network_context_params->cors_exempt_header_list.push_back(This only affects chrome, and not other users of the network service like content_shell and webview, which is why so many tests are failing.
I think despite the fact that it tells you not to do it, you should add the header in StoragePartitionImpl::InitNetworkContext().
Related details
- Andrew Brown
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Brown (Gerrit)
Andrew Brown added 3 comments![]( "Open in Gerrit")
Andrew BrownPlease run
```
third_party/blink/tools run_web_tests.py external/wpt/fetch --reset
```
to reset the test expectations and include any changes to *-expected.txt files in the CL.
Adam RiceJust done this (apologies for delay again).
There's a *lot* of changes. Like, way more than expected. Is it possible that the test-running processes need an update with this change? I imagine it might be possible that a heap of blink web tests are now "failing" because the runner doesn't adequately respond with CORS headers for custom user-agents.
Maybe it is OK, I'm not sure.
It looks like that didn't work right, since the expectations for a lot of unrelated tests are being deleted. I've started a Dry Run of the bots to see what they say.
Is it possible that the 4 test suites reporting important failures are outside Blink? (i.e. net_unittests and services_unittests for the iPhone, the ones that are actually reporting failure)
There's a lot of different ways to view "which tests are failing" (with different lists each time) and it's a little hard to parse which ones need my attention.
network_context_params->cors_exempt_header_list.push_back(This only affects chrome, and not other users of the network service like content_shell and webview, which is why so many tests are failing.
I think despite the fact that it tells you not to do it, you should add the header in StoragePartitionImpl::InitNetworkContext().
Ohhhhh. Right, that makes a lot more sense now. I'll look into the StoragePartition then!
header.key == "User-Agent") {Andrew BrownYou should add "User-Agent" to NetworkContext::cors_exempt_header_list somewhere rather than special-casing it.
Adam RiceI wasn't sure about adding it to that list, as the headers on that list seem to be almost exclusively "variations headers" for whatever lives in components/variations.
I've added it in SystemNetworkContextManager::ConfigureDefaultNetworkContextParams.
Can't shake the feeling I should be leaving the browser user-agent till later - I noticed NetworkContext has a user_agent field, also filled out in ConfigureDefaultNetworkContextParams. I might investigate that and see if it's possible for the browser user-agent to be set from that value instead of setting it in third_party/blink/renderer/platform/loader/fetch/url_loader/request_conversion.cc.
Andrew BrownWe need to use the value from the render process, as it may have been overridden by DevTools. I don't know if there is any way to access the DevTools override from within the network service. My guess would be not.
Another method would be to add a new `default_user_agent` field to `network::ResourceRequest`, but it would require changes in quite a few places.
Adam RiceOh, true, I'd completely forgotten that DevTools lets you override your browser's default.
...Hm, should *that* value be CORS-exempt? (In this CL, it will be.) Probably a question for the whatwg peoplem, not here.
DevTools is generally considered part of the browser, so a User-Agent set by DevTools has the same authority as one set by the browser itself.
- Adam Rice
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Adam Rice (Gerrit)
Adam Rice added 1 comment![]( "Open in Gerrit")
Andrew BrownPlease run
```
third_party/blink/tools run_web_tests.py external/wpt/fetch --reset
```
to reset the test expectations and include any changes to *-expected.txt files in the CL.
Adam RiceJust done this (apologies for delay again).
There's a *lot* of changes. Like, way more than expected. Is it possible that the test-running processes need an update with this change? I imagine it might be possible that a heap of blink web tests are now "failing" because the runner doesn't adequately respond with CORS headers for custom user-agents.
Maybe it is OK, I'm not sure.
Andrew BrownIt looks like that didn't work right, since the expectations for a lot of unrelated tests are being deleted. I've started a Dry Run of the bots to see what they say.
Is it possible that the 4 test suites reporting important failures are outside Blink? (i.e. net_unittests and services_unittests for the iPhone, the ones that are actually reporting failure)
There's a lot of different ways to view "which tests are failing" (with different lists each time) and it's a little hard to parse which ones need my attention.
I generally deal with any failures on the linux-rel bot first. It runs the largest cross-section of tests and is less prone to weird failures than some other bots.
Usually getting linux-rel passing is sufficient to clear several other bots. If mac-rel or win-rel had failures they would be my next priority. After that I'd look at whatever was still failing.
Related details
- Andrew Brown
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Takashi Toyoshima (Gerrit)
Takashi Toyoshima added 1 comment![]( "Open in Gerrit")
This is a wrong use of cors_exempt_header_list. It should not be used for headers that JavaScript can set arbitrary values, but only be allowed for headers that browser internal trusted code set.
If we allow JavaScript to set it via the API with exempted from CORS checks, it should be listed in the CORS-safelitsed request-header.
https://fetch.spec.whatwg.org/#cors-safelisted-request-header
So, I think the current behavior is intentional, and if you want to change this, spec discussion is needed. But I think the current behavior is a conclusion of a security discussion.
Related details
- Andrew Brown
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Takashi Toyoshima (Gerrit)
Takashi Toyoshima added 1 comment![]( "Open in Gerrit")
This is a wrong use of cors_exempt_header_list. It should not be used for headers that JavaScript can set arbitrary values, but only be allowed for headers that browser internal trusted code set.
If we allow JavaScript to set it via the API with exempted from CORS checks, it should be listed in the CORS-safelitsed request-header.
https://fetch.spec.whatwg.org/#cors-safelisted-request-headerSo, I think the current behavior is intentional, and if you want to change this, spec discussion is needed. But I think the current behavior is a conclusion of a security discussion.
Maybe I should add mode information.
So, there are 3 groups of headers from the viewpoint of CORS and the Fetch and other APIs.
1. forbidden request headers (https://fetch.spec.whatwg.org/#forbidden-request-header)
JavaScript cannot set them.
2. safelisted request headers (https://fetch.spec.whatwg.org/#cors-safelisted-request-header)
JavaScript can set, and doesn't cause CORS preflight if their use is reasonable, but there is a rule to trigger preflight even if they are in the safelist.
3. other request headers
JavaScript can set, but trigger CORS preflight.
IIUC, the User-Agent is classified into the 3 in the current spec.
Related details
- Andrew Brown
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Brown (Gerrit)
Andrew Brown added 1 comment![]( "Open in Gerrit")
Takashi ToyoshimaThis is a wrong use of cors_exempt_header_list. It should not be used for headers that JavaScript can set arbitrary values, but only be allowed for headers that browser internal trusted code set.
If we allow JavaScript to set it via the API with exempted from CORS checks, it should be listed in the CORS-safelitsed request-header.
https://fetch.spec.whatwg.org/#cors-safelisted-request-headerSo, I think the current behavior is intentional, and if you want to change this, spec discussion is needed. But I think the current behavior is a conclusion of a security discussion.
Maybe I should add mode information.
So, there are 3 groups of headers from the viewpoint of CORS and the Fetch and other APIs.1. forbidden request headers (https://fetch.spec.whatwg.org/#forbidden-request-header)
JavaScript cannot set them.2. safelisted request headers (https://fetch.spec.whatwg.org/#cors-safelisted-request-header)
JavaScript can set, and doesn't cause CORS preflight if their use is reasonable, but there is a rule to trigger preflight even if they are in the safelist.3. other request headers
JavaScript can set, but trigger CORS preflight.IIUC, the User-Agent is classified into the 3 in the current spec.
Hmm. You're right, Firefox treats it a bit like #2 - if JavaScript sets it, send a preflight, and if the browser sets it, don't - but the spec does appear to describe it as #3.
I wasn't aware of the real semantic meaning of cors_exempt_header_list, so it sounds like that's not what I want. Functionally, in this CL, I've treated User-Agent as if it was #2 - but there's no mechanism in NetworkContext for "sometimes exempt", so the NetworkContext just always exempts it and trusts Blink to know what it's doing (I'll admit, not an ideal approach, but one that I can understand and implement).
If User-Agent really is #3, this CL could be almost as simple as just removing it from the list in /net/http/http_util.cc, as by default it will then always trigger a preflight. I'd still need to ensure it's hidden from ServiceWorkers, but the current setup (with the browser User-Agent kept in a separate area until the NetworkRequest is constructed) should (?) do that already.
Should I open an issue/send an email/something to ask the whatwg people for clarification? Or is it OK to just forge on with a never-cors-exempt User-Agent? (I personally suspect this could break a lot of things, as websites may not realise they have to respond to an OPTIONS request to say that setting a User-Agent is OK, and treating User-Agent as #3 would result in every request triggering an OPTIONS request to ask about the browser-supplied User-Agent.)
Related details
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Takashi Toyoshima (Gerrit)
Takashi Toyoshima added 1 comment![]( "Open in Gerrit")
Takashi ToyoshimaThis is a wrong use of cors_exempt_header_list. It should not be used for headers that JavaScript can set arbitrary values, but only be allowed for headers that browser internal trusted code set.
If we allow JavaScript to set it via the API with exempted from CORS checks, it should be listed in the CORS-safelitsed request-header.
https://fetch.spec.whatwg.org/#cors-safelisted-request-headerSo, I think the current behavior is intentional, and if you want to change this, spec discussion is needed. But I think the current behavior is a conclusion of a security discussion.
Andrew BrownMaybe I should add mode information.
So, there are 3 groups of headers from the viewpoint of CORS and the Fetch and other APIs.1. forbidden request headers (https://fetch.spec.whatwg.org/#forbidden-request-header)
JavaScript cannot set them.2. safelisted request headers (https://fetch.spec.whatwg.org/#cors-safelisted-request-header)
JavaScript can set, and doesn't cause CORS preflight if their use is reasonable, but there is a rule to trigger preflight even if they are in the safelist.3. other request headers
JavaScript can set, but trigger CORS preflight.IIUC, the User-Agent is classified into the 3 in the current spec.
Hmm. You're right, Firefox treats it a bit like #2 - if JavaScript sets it, send a preflight, and if the browser sets it, don't - but the spec does appear to describe it as #3.
I wasn't aware of the real semantic meaning of cors_exempt_header_list, so it sounds like that's not what I want. Functionally, in this CL, I've treated User-Agent as if it was #2 - but there's no mechanism in NetworkContext for "sometimes exempt", so the NetworkContext just always exempts it and trusts Blink to know what it's doing (I'll admit, not an ideal approach, but one that I can understand and implement).
If User-Agent really is #3, this CL could be almost as simple as just removing it from the list in /net/http/http_util.cc, as by default it will then always trigger a preflight. I'd still need to ensure it's hidden from ServiceWorkers, but the current setup (with the browser User-Agent kept in a separate area until the NetworkRequest is constructed) should (?) do that already.
Should I open an issue/send an email/something to ask the whatwg people for clarification? Or is it OK to just forge on with a never-cors-exempt User-Agent? (I personally suspect this could break a lot of things, as websites may not realise they have to respond to an OPTIONS request to say that setting a User-Agent is OK, and treating User-Agent as #3 would result in every request triggering an OPTIONS request to ask about the browser-supplied User-Agent.)
JavaScript can easily embed a risky byte sequence into the User-Agent value. So, sending preflight to cross origin requests is the right direction. If the server doesn't know OPTIONS, it means the server is NOT ready to receive a request containing arbitrary header value. So failing it is not a compatibility break but just a right safety to avoid security problem as it's so easy to cause a buffer overflow and so on against such servers.
So, I'm fine with the direction just to remove it from the forbidden list in the http_util.cc, and I think whatwg would have the same opinion as I have been involved in the CORS discussion.
As it wasn't permitted before, this would not introduce a new compatibility breakage, but just relax the existing restriction a little?
Related details
- Andrew Brown
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Adam Rice (Gerrit)
Adam Rice added 1 comment![]( "Open in Gerrit")
I'm really confused, so let me try to clarify.
Current behaviour:
1. Blink C++ code sets user-agent to "Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/121.0.0.0 Safari/537.36": sent to server without preflight
2. DevTools sets user-agent to "Banana/1.0": sent to server without preflight
3. Extension sets user-agent to "Banana/1.0": sent to server without preflight
4. JavaScript sets user-agent to "Banana/1.0": ignored, not sent to server
Desired behaviour after this change:
1. Blink C++ code sets user-agent to "Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/121.0.0.0 Safari/537.36": sent to server without preflight
2. DevTools sets user-agent to "Banana/1.0": sent to server without preflight
3. Extension sets user-agent to "Banana/1.0": sent to server without preflight
4. JavaScript sets user-agent to "Banana/1.0": sent to server with a preflight
Number 4. is the only case that should change. Is that right?
Related details
- Andrew Brown
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Brown (Gerrit)
Andrew Brown added 3 comments![]( "Open in Gerrit")
Andrew BrownPlease run
```
third_party/blink/tools run_web_tests.py external/wpt/fetch --reset
```
to reset the test expectations and include any changes to *-expected.txt files in the CL.
Adam RiceJust done this (apologies for delay again).
There's a *lot* of changes. Like, way more than expected. Is it possible that the test-running processes need an update with this change? I imagine it might be possible that a heap of blink web tests are now "failing" because the runner doesn't adequately respond with CORS headers for custom user-agents.
Maybe it is OK, I'm not sure.
Andrew BrownIt looks like that didn't work right, since the expectations for a lot of unrelated tests are being deleted. I've started a Dry Run of the bots to see what they say.
Adam RiceIs it possible that the 4 test suites reporting important failures are outside Blink? (i.e. net_unittests and services_unittests for the iPhone, the ones that are actually reporting failure)
There's a lot of different ways to view "which tests are failing" (with different lists each time) and it's a little hard to parse which ones need my attention.
I generally deal with any failures on the linux-rel bot first. It runs the largest cross-section of tests and is less prone to weird failures than some other bots.
Usually getting linux-rel passing is sufficient to clear several other bots. If mac-rel or win-rel had failures they would be my next priority. After that I'd look at whatever was still failing.
Sweet. I'll also git-reset when I do the next patchset so we don't have heaps of test changes clogging up the diff, if that's alright.
I've possibly been confused by the #1/2/3 groups - what you've just described is my understanding, it's Firefox's implementation, and it's what this CL does. Takashi can correct me if that's wrong - the spec is somewhat vague, and I'm unsure if user-agent has this behaviour nicely described anywhere.
My post above was out of concern that #3 meant *always* sending a preflight, not only when JavaScript sets the user-agent.
Either way I will have to investigate where to pass it through if cors_exempt_... isn't the place for it. Possibly this will mean creating a new mechanism for passing it through that doesn't trigger preflights.
network_context_params->cors_exempt_header_list.push_back(Andrew BrownThis only affects chrome, and not other users of the network service like content_shell and webview, which is why so many tests are failing.
I think despite the fact that it tells you not to do it, you should add the header in StoragePartitionImpl::InitNetworkContext().
Ohhhhh. Right, that makes a lot more sense now. I'll look into the StoragePartition then!
- Adam Rice
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Takashi Toyoshima (Gerrit)
Takashi Toyoshima added 7 comments![]( "Open in Gerrit")
Hi, I'm now refreshing my memory, and let me correct my previous answer.
Probably your current change is in the right direction.
I left some notes on the cors implementation for reference, and in order to transfer my knowledge to Adam.
And, also I left some implementation comments to fix and polish current implementation. Using the cors_exempt_headers are a little tricky from the security perspective, and I want to make the interface clearer so that developers don't use it for a wrong purpose.
Desired behaviors Adam describes need a small fix for 4.
Preflight will be sent only when JavaScript sets it for cross-origin requests.
Regarding the behavior, the Fetch spec defines what we need for the public web platform, but behaviors for #2 and #3 are vendor specific and the spec doesn't explain well, I think.
Also, let me refresh my memory on the chromium's CORS implementation.
My previous explanation wasn't accurate. Sorry for that.
Here are notes to share my knowledge.
We have two request header lists in network::ResourceRequest.
```
net::HttpRequestHeaders headers;
net::HttpRequestHeaders cors_exempt_headers;
```
Usually, we use `headers` to hold the request headers. But if internal code needs to set CORS impacting headers, we push them into `cors_exempt_headers` instead. Headers in the list escape from the CORS checks. But even for the same request header, it should not be used for the case JavaScript set it because JavaScript setting headers always need CORS checks.
For instance, if Blink code wants to set the default User-Agent, using `cors_exempt_headers` is fine. DevTools can use it too. But when JavaScript sets it via Fetch or XHR, we push it into `headers`.
These two headers are merged here:
https://source.chromium.org/chromium/chromium/src/+/main:services/network/url_loader.cc;l=656;bpv=0;bpt=0
So, you may need to manage the header entry is exclusive in both lists to avoid unexpected merge results.
But, using `cors_exempt_headers` needs another setup. That's the `NetworkContextParams::cors_exempt_headers_list`. This is setup via the trusted code in the browser process, and only headers listed in the list works in the `ResourceRequest::cors_exempt_headers`. If there are unknown headers there, we detect it as the requester, the initiating renderer, is compromised.
Another CORS escaping path is to set the header after the CORS checks. Some Extensions API goes to this route, and /net code and their deligates also go on this path.
network_context_params->cors_exempt_header_list.push_back(Andrew BrownThis only affects chrome, and not other users of the network service like content_shell and webview, which is why so many tests are failing.
I think despite the fact that it tells you not to do it, you should add the header in StoragePartitionImpl::InitNetworkContext().
Andrew BrownOhhhhh. Right, that makes a lot more sense now. I'll look into the StoragePartition then!
Done
Your change is not only for Google Chrome, but for all chromium based UA.
So, the right place to set it up is here:
https://source.chromium.org/chromium/chromium/src/+/main:content/browser/storage_partition_impl.cc;l=3304;drc=f4a00cc248dd2dc8ec8759fb51620d47b5114090;bpv=1;bpt=1
by the way, I could not remember how this default UA header bypassed the cors checks until today. do you explore?
// the safe_user_agent_ is the browser-supplied user agent. It doesn't triggerI'm a bit concern about other developers mistakenly calling this for wrong uses.
Also, "CorsExempt" is confusing.
Can we change the direction to have a FetchContext::GetDefaultUserAgent()?
See my another comment on request_conversion.cc.
if (!user_agent_set) {slightly prefer `headers.HasHeader(kUserAgent)` rather than managing a flag during the conversion manually.
src.CorsExemptHTTPUserAgent().Utf8());Can we obtain the default user-agent header via FetchContext rather than holding it in the src?
Now, ResourceLoader and FetchManager call this PopulateResourceRequest method. Both callers can access FetchContext via ResourceFetcher.
FetchContext's GetDefaultUserAgent needs to be implemented in the inherit classes for frames, and workers respectively. So, we can add a 4th argument to this method to provide the default user-agent.
Related details
- Adam Rice
- Andrew Brown
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Brown (Gerrit)
Andrew Brown added 8 comments![]( "Open in Gerrit")
Andrew BrownPlease run
```
third_party/blink/tools run_web_tests.py external/wpt/fetch --reset
```
to reset the test expectations and include any changes to *-expected.txt files in the CL.
Adam RiceJust done this (apologies for delay again).
There's a *lot* of changes. Like, way more than expected. Is it possible that the test-running processes need an update with this change? I imagine it might be possible that a heap of blink web tests are now "failing" because the runner doesn't adequately respond with CORS headers for custom user-agents.
Maybe it is OK, I'm not sure.
Andrew BrownIt looks like that didn't work right, since the expectations for a lot of unrelated tests are being deleted. I've started a Dry Run of the bots to see what they say.
Adam RiceIs it possible that the 4 test suites reporting important failures are outside Blink? (i.e. net_unittests and services_unittests for the iPhone, the ones that are actually reporting failure)
There's a lot of different ways to view "which tests are failing" (with different lists each time) and it's a little hard to parse which ones need my attention.
Andrew BrownI generally deal with any failures on the linux-rel bot first. It runs the largest cross-section of tests and is less prone to weird failures than some other bots.
Usually getting linux-rel passing is sufficient to clear several other bots. If mac-rel or win-rel had failures they would be my next priority. After that I'd look at whatever was still failing.
Sweet. I'll also git-reset when I do the next patchset so we don't have heaps of test changes clogging up the diff, if that's alright.
Done
This explanation is fantastic, much appreciated.
Hi, I'm now refreshing my memory, and let me correct my previous answer.
Probably your current change is in the right direction.I left some notes on the cors implementation for reference, and in order to transfer my knowledge to Adam.
And, also I left some implementation comments to fix and polish current implementation. Using the cors_exempt_headers are a little tricky from the security perspective, and I want to make the interface clearer so that developers don't use it for a wrong purpose.
I've gone through your comments, but - I've made a super rookie mistake, I've `git clean`'ed a bit too hard and now I can't work out how to get `git cl upload` to link back to this CL. I guess I've accidentally cleared out whatever file `git cl` stores its information in.
Any way to link back to this manually, or is the only option to submit a separate CL? Sorry about that, it'll probably be a huge pain.
network_context_params->cors_exempt_header_list.push_back(Andrew BrownThis only affects chrome, and not other users of the network service like content_shell and webview, which is why so many tests are failing.
I think despite the fact that it tells you not to do it, you should add the header in StoragePartitionImpl::InitNetworkContext().
Andrew BrownOhhhhh. Right, that makes a lot more sense now. I'll look into the StoragePartition then!
Takashi ToyoshimaDone
Your change is not only for Google Chrome, but for all chromium based UA.
So, the right place to set it up is here:
https://source.chromium.org/chromium/chromium/src/+/main:content/browser/storage_partition_impl.cc;l=3304;drc=f4a00cc248dd2dc8ec8759fb51620d47b5114090;bpv=1;bpt=1
Done
by the way, I could not remember how this default UA header bypassed the cors checks until today. do you explore?
I believe the browser process checks headers against the `kForbiddenHeaderList` field using `!HttpUtil::IsSafeHeader()` before CORS. Basically, if it sees the request has any "unsafe" headers, it assumes they don't need CORS checks, because it can't have come from JavaScript.
See services/network/cors/cors_util.cc for where I think that check happens - I believe that function returns a list of headers that CORS needs to ask about, and skips any headers it sees that are "unsafe".
Unfortunately we have to take it off the forbidden list for JavaScript to use it, hence passing it around through the cors_exempt_headers.
// the safe_user_agent_ is the browser-supplied user agent. It doesn't triggerI'm a bit concern about other developers mistakenly calling this for wrong uses.
Also, "CorsExempt" is confusing.Can we change the direction to have a FetchContext::GetDefaultUserAgent()?
See my another comment on request_conversion.cc.
Makes sense - done!
if (!user_agent_set) {slightly prefer `headers.HasHeader(kUserAgent)` rather than managing a flag during the conversion manually.
Done
src.CorsExemptHTTPUserAgent().Utf8());Can we obtain the default user-agent header via FetchContext rather than holding it in the src?
Now, ResourceLoader and FetchManager call this PopulateResourceRequest method. Both callers can access FetchContext via ResourceFetcher.
FetchContext's GetDefaultUserAgent needs to be implemented in the inherit classes for frames, and workers respectively. So, we can add a 4th argument to this method to provide the default user-agent.
The inheritance of `FetchContext` is a little funky - I've had to implement a weird default-version in `FetchContext` returning an empty string.
In return, I've stuck in a DCHECK macro to make sure it's actually been overridden by the time we want to set it in this file.
Related details
- Adam Rice
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Takashi Toyoshima (Gerrit)
Takashi Toyoshima added 1 comment![]( "Open in Gerrit")
Andrew BrownHi, I'm now refreshing my memory, and let me correct my previous answer.
Probably your current change is in the right direction.I left some notes on the cors implementation for reference, and in order to transfer my knowledge to Adam.
And, also I left some implementation comments to fix and polish current implementation. Using the cors_exempt_headers are a little tricky from the security perspective, and I want to make the interface clearer so that developers don't use it for a wrong purpose.
I've gone through your comments, but - I've made a super rookie mistake, I've `git clean`'ed a bit too hard and now I can't work out how to get `git cl upload` to link back to this CL. I guess I've accidentally cleared out whatever file `git cl` stores its information in.
Any way to link back to this manually, or is the only option to submit a separate CL? Sorry about that, it'll probably be a huge pain.
You can set up your current branch with 'git cl issue 5273743`.
This is a command to bind the current branch with a specified review entry.
So, once you run it, `git cl upload` will upload your current local changes to the bound review page.
Related details
- Adam Rice
- Andrew Brown
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Takashi Toyoshima (Gerrit)
Takashi Toyoshima added 1 comment![]( "Open in Gerrit")
Andrew BrownHi, I'm now refreshing my memory, and let me correct my previous answer.
Probably your current change is in the right direction.I left some notes on the cors implementation for reference, and in order to transfer my knowledge to Adam.
And, also I left some implementation comments to fix and polish current implementation. Using the cors_exempt_headers are a little tricky from the security perspective, and I want to make the interface clearer so that developers don't use it for a wrong purpose.
Takashi ToyoshimaI've gone through your comments, but - I've made a super rookie mistake, I've `git clean`'ed a bit too hard and now I can't work out how to get `git cl upload` to link back to this CL. I guess I've accidentally cleared out whatever file `git cl` stores its information in.
Any way to link back to this manually, or is the only option to submit a separate CL? Sorry about that, it'll probably be a huge pain.
You can set up your current branch with 'git cl issue 5273743`.
This is a command to bind the current branch with a specified review entry.
So, once you run it, `git cl upload` will upload your current local changes to the bound review page.
sorry, the quotation was broken...
Takashi Toyoshima (Gerrit)
Takashi Toyoshima added 1 comment![]( "Open in Gerrit")
Oops, I mistakenly uploaded my local changes here...
I forgot that I tried the operation locally for doublechecking lol
Can you override again?
Related details
- Adam Rice
- Andrew Brown
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Brown (Gerrit)
Andrew Brown added 2 comments![]( "Open in Gerrit")
Andrew BrownHi, I'm now refreshing my memory, and let me correct my previous answer.
Probably your current change is in the right direction.I left some notes on the cors implementation for reference, and in order to transfer my knowledge to Adam.
And, also I left some implementation comments to fix and polish current implementation. Using the cors_exempt_headers are a little tricky from the security perspective, and I want to make the interface clearer so that developers don't use it for a wrong purpose.
Takashi ToyoshimaI've gone through your comments, but - I've made a super rookie mistake, I've `git clean`'ed a bit too hard and now I can't work out how to get `git cl upload` to link back to this CL. I guess I've accidentally cleared out whatever file `git cl` stores its information in.
Any way to link back to this manually, or is the only option to submit a separate CL? Sorry about that, it'll probably be a huge pain.
Takashi ToyoshimaYou can set up your current branch with 'git cl issue 5273743`.
This is a command to bind the current branch with a specified review entry.
So, once you run it, `git cl upload` will upload your current local changes to the bound review page.
sorry, the quotation was broken...
You can set up your current branch with `git cl issue 5273743`.
This is a command to bind the current branch with a specified review entry.
So, once you run it, `git cl upload` will upload your current local changes to the bound review page.
You're an absolute lifesaver, cheers! Uploaded now.
Oops, I mistakenly uploaded my local changes here...
I forgot that I tried the operation locally for doublechecking lol
Can you override again?
- Adam Rice
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Adam Rice (Gerrit)
Adam Rice added 4 comments![]( "Open in Gerrit")
const String& user_agent;Why is it safe for this to be a reference? What is keeping it alive?
const String FrameFetchContext::GetDefaultUserAgent() const {Making the return type `const` here doesn't really do anything useful, since the caller will usually store it in a mutable variable anyway. It may defeat return value optimisation and force the copy constructor to be called.
virtual const String GetDefaultUserAgent() const { return ""; }When is this called? Can we make the body `NOTREACHED_NORETURN();` instead?
DCHECK_NE(context_default_user_agent, "");What happens if someone uses DevTools to set the user agent to an empty string?
Related details
- Andrew Brown
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Brown (Gerrit)
Andrew Brown added 6 comments![]( "Open in Gerrit")
This is a wrong use of cors_exempt_header_list. It should not be used for headers that JavaScript can set arbitrary values, but only be allowed for headers that browser internal trusted code set.
Done
Why is it safe for this to be a reference? What is keeping it alive?
Uhh, it's probably not, then. My C++ is pretty rusty, especially in the &-vs-*-vs-unique_ptr department. Switched it to a regular value instead.
const String FrameFetchContext::GetDefaultUserAgent() const {Making the return type `const` here doesn't really do anything useful, since the caller will usually store it in a mutable variable anyway. It may defeat return value optimisation and force the copy constructor to be called.
Just checking - sounds good to me, so I've changed it, but this means changing the types of all the `GetDefaultUserAgent()` versions (not just in `FrameFetchContext`), including up to the default in `fetch_context.h` and back down to the implementation in `worker_fetch_context.cc`.
virtual const String GetDefaultUserAgent() const { return ""; }When is this called? Can we make the body `NOTREACHED_NORETURN();` instead?
It should never be called, hence the `DCHECK` later on. Switching to `NOTREACHED_NORETURN();` lets me get rid of that, so, done. (I had no idea that macro even existed.)
What happens if someone uses DevTools to set the user agent to an empty string?
Ah, stung by DevTools again. If I switch out the body of the default impl to be `NOTREACHED_NORETURN();`, then I can just remove this DCHECK and have the same effect, without needing to error out on some sentinel value.
src.CorsExemptHTTPUserAgent().Utf8());Andrew BrownCan we obtain the default user-agent header via FetchContext rather than holding it in the src?
Now, ResourceLoader and FetchManager call this PopulateResourceRequest method. Both callers can access FetchContext via ResourceFetcher.
FetchContext's GetDefaultUserAgent needs to be implemented in the inherit classes for frames, and workers respectively. So, we can add a 4th argument to this method to provide the default user-agent.
The inheritance of `FetchContext` is a little funky - I've had to implement a weird default-version in `FetchContext` returning an empty string.
In return, I've stuck in a DCHECK macro to make sure it's actually been overridden by the time we want to set it in this file.
Following Adam's review comments, the inheritance thing is much nicer - `NOTREACHED_NORETURN();` in the default, no `DCHECK();` needed.
Related details
- Adam Rice
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Adam Rice (Gerrit)
Adam Rice added 3 comments![]( "Open in Gerrit")
const String user_agent,You should make this non-const, and then initialise the member variable with `std::move()`, ie.
```
user_agent(std::move(user_agent)),
```
This will avoid reference-count churn on the String.
const String FrameFetchContext::GetDefaultUserAgent() const {Andrew BrownMaking the return type `const` here doesn't really do anything useful, since the caller will usually store it in a mutable variable anyway. It may defeat return value optimisation and force the copy constructor to be called.
Just checking - sounds good to me, so I've changed it, but this means changing the types of all the `GetDefaultUserAgent()` versions (not just in `FrameFetchContext`), including up to the default in `fetch_context.h` and back down to the implementation in `worker_fetch_context.cc`.
- Andrew Brown
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Brown (Gerrit)
Andrew Brown added 1 comment![]( "Open in Gerrit")
const String user_agent,You should make this non-const, and then initialise the member variable with `std::move()`, ie.
```
user_agent(std::move(user_agent)),
```
This will avoid reference-count churn on the String.
Hm, actually - I just noticed the current (live) version is `const String&`, which we just changed away from, so I did a bit more digging.
At the moment, `FrozenState` stores a `const String&`. The constructor is only used by `Detach()` on line #789, where the user_agent is provided by `GetUserAgent()`. There, the backing string for the ref is provided by `GetFrame()->Loader().UserAgent()`.
In my updated implementation, that function is just renamed to `GetDefaultUserAgent()`. So we should be safe to just keep using `const String&` here, backed by the Loader, as it's just a different function name on the same code. However, continuing to switch to use a non-const non-ref String is probably also inconsequential, if you'd like to just do that.
Related details
- Adam Rice
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Takashi Toyoshima (Gerrit)
Takashi Toyoshima added 5 comments![]( "Open in Gerrit")
Thank you for the update.
It looks good to me.
I left some comments on style and minor optimization nits, but they are optional.
const String user_agent,Andrew BrownYou should make this non-const, and then initialise the member variable with `std::move()`, ie.
```
user_agent(std::move(user_agent)),
```
This will avoid reference-count churn on the String.
Hm, actually - I just noticed the current (live) version is `const String&`, which we just changed away from, so I did a bit more digging.
At the moment, `FrozenState` stores a `const String&`. The constructor is only used by `Detach()` on line #789, where the user_agent is provided by `GetUserAgent()`. There, the backing string for the ref is provided by `GetFrame()->Loader().UserAgent()`.
In my updated implementation, that function is just renamed to `GetDefaultUserAgent()`. So we should be safe to just keep using `const String&` here, backed by the Loader, as it's just a different function name on the same code. However, continuing to switch to use a non-const non-ref String is probably also inconsequential, if you'd like to just do that.
As of the fast String's copy is not expensive as you noticed, the current code is not inefficient even if we make a copy here.
If we want to improve it a little, as Adam suggested, taking `String` and using `std::move` will be the best. Another benefit is people can know the code is effective even if they don't know the String specific copy's internal optimization.
DCHECK(!user_agent.IsNull());After a recent guildeline change, CHECK is preferred than DCHECK.
https://chromium.googlesource.com/chromium/src/+/main/styleguide/c++/checks.md#invariant_verification-mechanisms
virtual String GetDefaultUserAgent() const { NOTREACHED_NORETURN(); }For the same reason, returning `const String&`, and making a copy in a caller side might be a not-surprising interface that doesn't rely on the copy's optimization.
const String context_default_user_agent);Let's use `const String&` until we need an actual copy.
Related details
- Adam Rice
- Andrew Brown
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Takashi Toyoshima (Gerrit)
Takashi Toyoshima added 1 comment![]( "Open in Gerrit")
Andrew Brown <m...@ajbrown.au>just in case for Adam: I just checked the SignCLA site and ensureed the address is already recorded as we received CLAs.
Andrew Brown (Gerrit)
Andrew Brown added 5 comments![]( "Open in Gerrit")
const String user_agent,Andrew BrownYou should make this non-const, and then initialise the member variable with `std::move()`, ie.
```
user_agent(std::move(user_agent)),
```
This will avoid reference-count churn on the String.
Takashi ToyoshimaHm, actually - I just noticed the current (live) version is `const String&`, which we just changed away from, so I did a bit more digging.
At the moment, `FrozenState` stores a `const String&`. The constructor is only used by `Detach()` on line #789, where the user_agent is provided by `GetUserAgent()`. There, the backing string for the ref is provided by `GetFrame()->Loader().UserAgent()`.
In my updated implementation, that function is just renamed to `GetDefaultUserAgent()`. So we should be safe to just keep using `const String&` here, backed by the Loader, as it's just a different function name on the same code. However, continuing to switch to use a non-const non-ref String is probably also inconsequential, if you'd like to just do that.
As of the fast String's copy is not expensive as you noticed, the current code is not inefficient even if we make a copy here.
If we want to improve it a little, as Adam suggested, taking `String` and using `std::move` will be the best. Another benefit is people can know the code is effective even if they don't know the String specific copy's internal optimization.
Ok, hang on, it's not as simple as I thought :(.
It looks like both "sources" of user-agent give me a `String`, not a `String&`. So it's a little hard to provide
const String user_agent,Andrew BrownYou should make this non-const, and then initialise the member variable with `std::move()`, ie.
```
user_agent(std::move(user_agent)),
```
This will avoid reference-count churn on the String.
Takashi ToyoshimaHm, actually - I just noticed the current (live) version is `const String&`, which we just changed away from, so I did a bit more digging.
At the moment, `FrozenState` stores a `const String&`. The constructor is only used by `Detach()` on line #789, where the user_agent is provided by `GetUserAgent()`. There, the backing string for the ref is provided by `GetFrame()->Loader().UserAgent()`.
In my updated implementation, that function is just renamed to `GetDefaultUserAgent()`. So we should be safe to just keep using `const String&` here, backed by the Loader, as it's just a different function name on the same code. However, continuing to switch to use a non-const non-ref String is probably also inconsequential, if you'd like to just do that.
As of the fast String's copy is not expensive as you noticed, the current code is not inefficient even if we make a copy here.
If we want to improve it a little, as Adam suggested, taking `String` and using `std::move` will be the best. Another benefit is people can know the code is effective even if they don't know the String specific copy's internal optimization.
`String` and `std::move` sounds like a plan to me. Just noting that I was wrong earlier about what backs the ref, in case using a `const String&` might actually be better.
DCHECK(!user_agent.IsNull());After a recent guildeline change, CHECK is preferred than DCHECK.
https://chromium.googlesource.com/chromium/src/+/main/styleguide/c++/checks.md#invariant_verification-mechanisms
Done
virtual String GetDefaultUserAgent() const { NOTREACHED_NORETURN(); }For the same reason, returning `const String&`, and making a copy in a caller side might be a not-surprising interface that doesn't rely on the copy's optimization.
The trouble with making `GetDefaultUserAgent()` return a `const String&` is that both "sources" of user-agent (either `GetFrame()->Loader().UserAgent();` in `FrameFetchContext` or `global_scope_->UserAgent();` in `WorkerFetchContext`) give me `String`, so I'd need to allocate space for them somehow in both of my `GetDefaultUserAgent()` implementations.
I guess I could go into each of those spots and follow it right the way back to make it `String&` the entire way through, but that also seems like a less-than-ideal approach.
const String context_default_user_agent);Let's use `const String&` until we need an actual copy.
- Adam Rice
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Brown (Gerrit)
Andrew Brown added 1 comment![]( "Open in Gerrit")
const String user_agent,Andrew BrownYou should make this non-const, and then initialise the member variable with `std::move()`, ie.
```
user_agent(std::move(user_agent)),
```
This will avoid reference-count churn on the String.
Takashi ToyoshimaHm, actually - I just noticed the current (live) version is `const String&`, which we just changed away from, so I did a bit more digging.
At the moment, `FrozenState` stores a `const String&`. The constructor is only used by `Detach()` on line #789, where the user_agent is provided by `GetUserAgent()`. There, the backing string for the ref is provided by `GetFrame()->Loader().UserAgent()`.
In my updated implementation, that function is just renamed to `GetDefaultUserAgent()`. So we should be safe to just keep using `const String&` here, backed by the Loader, as it's just a different function name on the same code. However, continuing to switch to use a non-const non-ref String is probably also inconsequential, if you'd like to just do that.
Andrew BrownAs of the fast String's copy is not expensive as you noticed, the current code is not inefficient even if we make a copy here.
If we want to improve it a little, as Adam suggested, taking `String` and using `std::move` will be the best. Another benefit is people can know the code is effective even if they don't know the String specific copy's internal optimization.
Ok, hang on, it's not as simple as I thought :(.
It looks like both "sources" of user-agent give me a `String`, not a `String&`. So it's a little hard to provide
Oops, ignore the second message (about not being as simple as I thought). That was related to the other comments in `fetch_context.h`.
Related details
- Adam Rice
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Takashi Toyoshima (Gerrit)
Takashi Toyoshima voted and added 2 comments![]( "Open in Gerrit")
Votes added by Takashi Toyoshima
| Code-Review | +1 |
2 comments
thanks. lgtm, and you'll need the second lgtm from Adam to submit this.
virtual String GetDefaultUserAgent() const { NOTREACHED_NORETURN(); }Andrew BrownFor the same reason, returning `const String&`, and making a copy in a caller side might be a not-surprising interface that doesn't rely on the copy's optimization.
The trouble with making `GetDefaultUserAgent()` return a `const String&` is that both "sources" of user-agent (either `GetFrame()->Loader().UserAgent();` in `FrameFetchContext` or `global_scope_->UserAgent();` in `WorkerFetchContext`) give me `String`, so I'd need to allocate space for them somehow in both of my `GetDefaultUserAgent()` implementations.
I guess I could go into each of those spots and follow it right the way back to make it `String&` the entire way through, but that also seems like a less-than-ideal approach.
- Adam Rice
- Andrew Brown
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Adam Rice (Gerrit)
Adam Rice voted and added 1 comment![]( "Open in Gerrit")
Votes added by Adam Rice
| Code-Review | +1 |
1 comment
- Andrew Brown
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Brown (Gerrit)
Andrew Brown added 4 comments![]( "Open in Gerrit")
Thanks for all the help!
I think just need someone with dry run access to run the CQ tests for me, and I need review from an owner of `content/browser/storage_partition_impl.cc`. I've pinged Bo, who was first on the list of suggested owners :) - given the nearby comment about deprecated APIs, there may be a better way to access cors_exempt_headers.
const String user_agent,Andrew BrownYou should make this non-const, and then initialise the member variable with `std::move()`, ie.
```
user_agent(std::move(user_agent)),
```
This will avoid reference-count churn on the String.
Takashi ToyoshimaHm, actually - I just noticed the current (live) version is `const String&`, which we just changed away from, so I did a bit more digging.
At the moment, `FrozenState` stores a `const String&`. The constructor is only used by `Detach()` on line #789, where the user_agent is provided by `GetUserAgent()`. There, the backing string for the ref is provided by `GetFrame()->Loader().UserAgent()`.
In my updated implementation, that function is just renamed to `GetDefaultUserAgent()`. So we should be safe to just keep using `const String&` here, backed by the Loader, as it's just a different function name on the same code. However, continuing to switch to use a non-const non-ref String is probably also inconsequential, if you'd like to just do that.
Andrew BrownAs of the fast String's copy is not expensive as you noticed, the current code is not inefficient even if we make a copy here.
If we want to improve it a little, as Adam suggested, taking `String` and using `std::move` will be the best. Another benefit is people can know the code is effective even if they don't know the String specific copy's internal optimization.
Andrew BrownOk, hang on, it's not as simple as I thought :(.
It looks like both "sources" of user-agent give me a `String`, not a `String&`. So it's a little hard to provide
Oops, ignore the second message (about not being as simple as I thought). That was related to the other comments in `fetch_context.h`.
Done
Andrew Brownby the way, I could not remember how this default UA header bypassed the cors checks until today. do you explore?
I believe the browser process checks headers against the `kForbiddenHeaderList` field using `!HttpUtil::IsSafeHeader()` before CORS. Basically, if it sees the request has any "unsafe" headers, it assumes they don't need CORS checks, because it can't have come from JavaScript.
See services/network/cors/cors_util.cc for where I think that check happens - I believe that function returns a list of headers that CORS needs to ask about, and skips any headers it sees that are "unsafe".
Unfortunately we have to take it off the forbidden list for JavaScript to use it, hence passing it around through the cors_exempt_headers.
Done
src.CorsExemptHTTPUserAgent().Utf8());Andrew BrownCan we obtain the default user-agent header via FetchContext rather than holding it in the src?
Now, ResourceLoader and FetchManager call this PopulateResourceRequest method. Both callers can access FetchContext via ResourceFetcher.
FetchContext's GetDefaultUserAgent needs to be implemented in the inherit classes for frames, and workers respectively. So, we can add a 4th argument to this method to provide the default user-agent.
Andrew BrownThe inheritance of `FetchContext` is a little funky - I've had to implement a weird default-version in `FetchContext` returning an empty string.
In return, I've stuck in a DCHECK macro to make sure it's actually been overridden by the time we want to set it in this file.
Following Adam's review comments, the inheritance thing is much nicer - `NOTREACHED_NORETURN();` in the default, no `DCHECK();` needed.
- Bo Liu
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Bo Liu (Gerrit)
Bo Liu voted and added 1 comment![]( "Open in Gerrit")
Votes added by Bo Liu
| Code-Review | +1 |
1 comment
- Andrew Brown
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Brown (Gerrit)
Andrew Brown added 1 comment![]( "Open in Gerrit")
Thanks for all the help!
I think just need someone with dry run access to run the CQ tests for me, and I need review from an owner of `content/browser/storage_partition_impl.cc`. I've pinged Bo, who was first on the list of suggested owners :) - given the nearby comment about deprecated APIs, there may be a better way to access cors_exempt_headers.
Going to ping Adam for this, as he was helping me with tests earlier :)
This still needs to run through the CQ tests, but before we do that, I probably have to update the expectations a bit. My local testing setup is a bit wonky; the testing docs say that some tests will just fail because I'm running a newer version than Ubuntu 10. (I have 147 regressions, a few of which are completely unrelated things like `media/W3C/video/events/`). Is there a better setup doc that I should go through, or anything better than just running it through the CQ tests again?
I've also gone through the tests manually and updated things that I *know* are needed to change (e.g. "can this header be set" type stuff). I'm wary of the `http/tests/inspector-protocol/` tests, as I'm not actually sure what they're testing for, but I think those might also need changes.
Related details
- Adam Rice
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Brown (Gerrit)
Andrew Brown added 1 comment![]( "Open in Gerrit")
Andrew BrownThanks for all the help!
I think just need someone with dry run access to run the CQ tests for me, and I need review from an owner of `content/browser/storage_partition_impl.cc`. I've pinged Bo, who was first on the list of suggested owners :) - given the nearby comment about deprecated APIs, there may be a better way to access cors_exempt_headers.
Going to ping Adam for this, as he was helping me with tests earlier :)
This still needs to run through the CQ tests, but before we do that, I probably have to update the expectations a bit. My local testing setup is a bit wonky; the testing docs say that some tests will just fail because I'm running a newer version than Ubuntu 10. (I have 147 regressions, a few of which are completely unrelated things like `media/W3C/video/events/`). Is there a better setup doc that I should go through, or anything better than just running it through the CQ tests again?
I've also gone through the tests manually and updated things that I *know* are needed to change (e.g. "can this header be set" type stuff). I'm wary of the `http/tests/inspector-protocol/` tests, as I'm not actually sure what they're testing for, but I think those might also need changes.
Ah, sorry if my inability to test makes this a bit brute-forcey.
I've fixed the issue that caused the vast majority of those test failures - found a `MockFetchContext` (and some other variants) that were missing their override for `GetDefaultUserAgent`, so they ended up calling the default version, and predictably, `NOTREACHED_NORETURN()` caused them all to explode.
There's definitely several other necessary test updates hidden among the mess - I found a few and fixed them in this CL, but the assumption that `user-agent` is forbidden is baked into a *lot* of tests. The non-blink tests seem to be working OK for me as of this CL - except (sorry if this is a stupid question), how do I actually compile and run every test group? E.g. I can run `blink_platform_unittests` by compiling `blink_tests` and running the `blink_platform_unittests` binary, but I can't find a binary for `webkit_unit_tests`, and `webkit_tests` isn't a valid target for compilation. (Same with `net_unittests` and `services_unittests`, but there's only one failure there, and it's easy to track down.)
Given the number of tests I've run into that expect `user-agent` to be a forbidden header, I'll endeavour to add a few more tests that expect the correct behaviour in the next patchset.
Related details
- Adam Rice
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Adam Rice (Gerrit)
Adam Rice added 2 comments![]( "Open in Gerrit")
Andrew BrownThanks for all the help!
I think just need someone with dry run access to run the CQ tests for me, and I need review from an owner of `content/browser/storage_partition_impl.cc`. I've pinged Bo, who was first on the list of suggested owners :) - given the nearby comment about deprecated APIs, there may be a better way to access cors_exempt_headers.
Andrew BrownGoing to ping Adam for this, as he was helping me with tests earlier :)
This still needs to run through the CQ tests, but before we do that, I probably have to update the expectations a bit. My local testing setup is a bit wonky; the testing docs say that some tests will just fail because I'm running a newer version than Ubuntu 10. (I have 147 regressions, a few of which are completely unrelated things like `media/W3C/video/events/`). Is there a better setup doc that I should go through, or anything better than just running it through the CQ tests again?
I've also gone through the tests manually and updated things that I *know* are needed to change (e.g. "can this header be set" type stuff). I'm wary of the `http/tests/inspector-protocol/` tests, as I'm not actually sure what they're testing for, but I think those might also need changes.
Ah, sorry if my inability to test makes this a bit brute-forcey.
I've fixed the issue that caused the vast majority of those test failures - found a `MockFetchContext` (and some other variants) that were missing their override for `GetDefaultUserAgent`, so they ended up calling the default version, and predictably, `NOTREACHED_NORETURN()` caused them all to explode.
There's definitely several other necessary test updates hidden among the mess - I found a few and fixed them in this CL, but the assumption that `user-agent` is forbidden is baked into a *lot* of tests. The non-blink tests seem to be working OK for me as of this CL - except (sorry if this is a stupid question), how do I actually compile and run every test group? E.g. I can run `blink_platform_unittests` by compiling `blink_tests` and running the `blink_platform_unittests` binary, but I can't find a binary for `webkit_unit_tests`, and `webkit_tests` isn't a valid target for compilation. (Same with `net_unittests` and `services_unittests`, but there's only one failure there, and it's easy to track down.)
Given the number of tests I've run into that expect `user-agent` to be a forbidden header, I'll endeavour to add a few more tests that expect the correct behaviour in the next patchset.
I think `webkit_unit_tests` actually refers to `blink_unittests`, which you should be able to compile and run normally.
`webkit_tests` maybe refers to the web_tests? In which case you can run
```
third_party/blink/tools/run_web_tests.py external/wpt/fetch
```
to run all the tests under `external/wpt/fetch`. `run_web_tests.py` can also be run with the `--reset` option to automatically update expectations.
You should be able to build `net_unittests` and `services_unittests` normally with
```
autoninja net_unittests services_unittests
```
Please post a quick message to the bug when you want me to do a dry run, as I don't get notified when you upload a new patchset.
Related details
- Andrew Brown
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Adam Rice (Gerrit)
Adam Rice added 2 comments![]( "Open in Gerrit")
Looks like the ability to set the User-Agent via the inspector protocol (ie. DevTools) might have been broken by this change:
https://ci.chromium.org/ui/p/chromium/builders/try/linux-rel/1731255/test-results?sortby=&groupby=
Can you try running the failing tests locally and see if you can reproduce?
just in case for Adam: I just checked the SignCLA site and ensureed the address is already recorded as we received CLAs.
- Andrew Brown
- Takashi Toyoshima
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Brown (Gerrit)
Andrew Brown added 1 comment![]( "Open in Gerrit")
Looks like the ability to set the User-Agent via the inspector protocol (ie. DevTools) might have been broken by this change:
https://ci.chromium.org/ui/p/chromium/builders/try/linux-rel/1731255/test-results?sortby=&groupby=Can you try running the failing tests locally and see if you can reproduce?
Sorry for the radio silence! I am still definitely intending to work on this, but my university studies are taking the bulk of my time.
In the past month or so I've fixed a lot of the easy failures - I should be down to 17 individual failing tests (but each one will be failing on several platforms). The inspector-protocol stuff is the largest group of tests still failing, so I've had it on my list to actually learn and figure out how all that works so I can debug. (I'm unfamilar with devtools' advanced usage, so it's hard to debug without experience.)
It definitely *looks* like this CL has stuffed up the inspector protocol stuff, and I can reproduce with local tests, but I haven't been able to reproduce manually, so it may be another matter of a missing testing interface or chrome-specific code. I'm not sure when I'll be super available to get into it, but maybe later in this month - sorry again for the low activity.
Related details
- Adam Rice
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Adam Rice (Gerrit)
Adam Rice added 1 comment![]( "Open in Gerrit")
Andrew BrownLooks like the ability to set the User-Agent via the inspector protocol (ie. DevTools) might have been broken by this change:
https://ci.chromium.org/ui/p/chromium/builders/try/linux-rel/1731255/test-results?sortby=&groupby=Can you try running the failing tests locally and see if you can reproduce?
Sorry for the radio silence! I am still definitely intending to work on this, but my university studies are taking the bulk of my time.
In the past month or so I've fixed a lot of the easy failures - I should be down to 17 individual failing tests (but each one will be failing on several platforms). The inspector-protocol stuff is the largest group of tests still failing, so I've had it on my list to actually learn and figure out how all that works so I can debug. (I'm unfamilar with devtools' advanced usage, so it's hard to debug without experience.)
It definitely *looks* like this CL has stuffed up the inspector protocol stuff, and I can reproduce with local tests, but I haven't been able to reproduce manually, so it may be another matter of a missing testing interface or chrome-specific code. I'm not sure when I'll be super available to get into it, but maybe later in this month - sorry again for the low activity.
- Andrew Brown
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Brown (Gerrit)
Andrew Brown added 1 comment![]( "Open in Gerrit")
Happy new year everyone! Apologies for dragging this up right as everyone returns from a well-deserved break :)
Figured I could use some of my summer free time to close this out for good. I worked out (some of, hopefully all) the failing devtools tests, which needed changes inside the preflight-summoning code - tagged Min Qin for review. I'm a little wary that I ended up needing to have weird case handling in the preflight code.
I have two outstanding questions:
- I've commented out the assertion in a test for frame_fetch_context. It's expecting that the User-Agent is set after calling PrepareRequest, but because this CL moves it to its own thing, that's no longer the case. Is this test checking something "deeper" that I should work out, or can it be safely removed? (I suspect this is the root cause of a few other test failures, but I never have been able to accurately run the full suite locally.)
- Do my cases for User-Agent handling make sense: a fetch-set User-Agent overrides a devtools User-Agent overrides the default? I think we've settled this, but it may be different when you consider which User-Agent should be present on preflight OPTIONS requests. At this point, it follows the same order.
Related details
- Adam Rice
- Min Qin
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Min Qin (Gerrit)
Min Qin voted Code-Review+1![]( "Open in Gerrit")
- Adam Rice
- Andrew Brown
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Adam Rice (Gerrit)
Adam Rice added 1 comment![]( "Open in Gerrit")
cors_exempt_headers. The default (browser or devtools generated) user-agent is now stored in the FetchContext, and stored into the cors_exempt_headers only if JavaScript didn't set one first.Please wrap the commit message to 72 columns so it looks nice in "git log" output. One day the UI will do this right, but that day has not yet come.
Related details
- Andrew Brown
Adam Rice (Gerrit)
Adam Rice added 1 comment![]( "Open in Gerrit")
if (user_agent) {
preflight_request->cors_exempt_headers.SetHeader(
net::HttpRequestHeaders::kUserAgent, *user_agent);
} else {
// The User-Agent is now passed through the cors_exempt_headers, but we
// still need this fallback to handle Devtools usage.
user_agent = request.headers.GetHeader(net::HttpRequestHeaders::kUserAgent);
if (user_agent) {
preflight_request->cors_exempt_headers.SetHeader(
net::HttpRequestHeaders::kUserAgent, *user_agent);
}We can avoid repeating the SetHeader() call like this:
```suggestion
if (!user_agent) {
// The User-Agent is now passed through the cors_exempt_headers, but we
// still need this fallback to handle Devtools usage.
user_agent = request.headers.GetHeader(net::HttpRequestHeaders::kUserAgent);
}
if (user_agent) {
preflight_request->cors_exempt_headers.SetHeader(
net::HttpRequestHeaders::kUserAgent, *user_agent);
}
```
Andrew Brown (Gerrit)
Andrew Brown added 5 comments![]( "Open in Gerrit")
Andrew BrownThanks for all the help!
I think just need someone with dry run access to run the CQ tests for me, and I need review from an owner of `content/browser/storage_partition_impl.cc`. I've pinged Bo, who was first on the list of suggested owners :) - given the nearby comment about deprecated APIs, there may be a better way to access cors_exempt_headers.
Andrew BrownGoing to ping Adam for this, as he was helping me with tests earlier :)
This still needs to run through the CQ tests, but before we do that, I probably have to update the expectations a bit. My local testing setup is a bit wonky; the testing docs say that some tests will just fail because I'm running a newer version than Ubuntu 10. (I have 147 regressions, a few of which are completely unrelated things like `media/W3C/video/events/`). Is there a better setup doc that I should go through, or anything better than just running it through the CQ tests again?
I've also gone through the tests manually and updated things that I *know* are needed to change (e.g. "can this header be set" type stuff). I'm wary of the `http/tests/inspector-protocol/` tests, as I'm not actually sure what they're testing for, but I think those might also need changes.
Adam RiceAh, sorry if my inability to test makes this a bit brute-forcey.
I've fixed the issue that caused the vast majority of those test failures - found a `MockFetchContext` (and some other variants) that were missing their override for `GetDefaultUserAgent`, so they ended up calling the default version, and predictably, `NOTREACHED_NORETURN()` caused them all to explode.
There's definitely several other necessary test updates hidden among the mess - I found a few and fixed them in this CL, but the assumption that `user-agent` is forbidden is baked into a *lot* of tests. The non-blink tests seem to be working OK for me as of this CL - except (sorry if this is a stupid question), how do I actually compile and run every test group? E.g. I can run `blink_platform_unittests` by compiling `blink_tests` and running the `blink_platform_unittests` binary, but I can't find a binary for `webkit_unit_tests`, and `webkit_tests` isn't a valid target for compilation. (Same with `net_unittests` and `services_unittests`, but there's only one failure there, and it's easy to track down.)
Given the number of tests I've run into that expect `user-agent` to be a forbidden header, I'll endeavour to add a few more tests that expect the correct behaviour in the next patchset.
I think `webkit_unit_tests` actually refers to `blink_unittests`, which you should be able to compile and run normally.
`webkit_tests` maybe refers to the web_tests? In which case you can run
```
third_party/blink/tools/run_web_tests.py external/wpt/fetch
```
to run all the tests under `external/wpt/fetch`. `run_web_tests.py` can also be run with the `--reset` option to automatically update expectations.You should be able to build `net_unittests` and `services_unittests` normally with
```
autoninja net_unittests services_unittests
```
Much appreciated - I did finally manage to get local tests going.
Happy new year everyone! Apologies for dragging this up right as everyone returns from a well-deserved break :)
Figured I could use some of my summer free time to close this out for good. I worked out (some of, hopefully all) the failing devtools tests, which needed changes inside the preflight-summoning code - tagged Min Qin for review. I'm a little wary that I ended up needing to have weird case handling in the preflight code.
I have two outstanding questions:
- I've commented out the assertion in a test for frame_fetch_context. It's expecting that the User-Agent is set after calling PrepareRequest, but because this CL moves it to its own thing, that's no longer the case. Is this test checking something "deeper" that I should work out, or can it be safely removed? (I suspect this is the root cause of a few other test failures, but I never have been able to accurately run the full suite locally.)
- Do my cases for User-Agent handling make sense: a fetch-set User-Agent overrides a devtools User-Agent overrides the default? I think we've settled this, but it may be different when you consider which User-Agent should be present on preflight OPTIONS requests. At this point, it follows the same order.
Ok, not quite all the failing devtools tests :(
Some remaining failures I've traced back down to `ThrottlingURLLoader::StartNow()`, where it enters IPC/inheritance hell. I am almost willing to bet this will characterise all the remianing failures: issues in the various implementations of `URLLoaderFactory::CreateLoaderAndStart`.
Basically, I have reason to believe there are one or more `URLLoader` implementations that don't combine the `cors_exempt_headers` in with the normal `headers` on a request - similar to the change I've already made in `preflight_controller.cc`. There are different `URLLoader`'s for different things, and I bet the one that deals with extensions will have a problem, and so will the one that deals with non-returning delayed requests for `fetchLater()`.
Do you have any tips for tracking down which one(s) live on the path below the DevTools (and extension) code? The debug docs are fantastic for working bottom-up, where you have a crash and you need to work out where, but not so helpful for working top-down, where I have correct behaviour and I don't know where it stops.
There are ~43 different types of `CreateLoaderAndStart` that could lead to my problem, each with their own very large stack of behaviour, and often calling into each other in hard-to-grasp ways. I can work through them eventually, but it's going to take a long time.
cors_exempt_headers. The default (browser or devtools generated) user-agent is now stored in the FetchContext, and stored into the cors_exempt_headers only if JavaScript didn't set one first.Please wrap the commit message to 72 columns so it looks nice in "git log" output. One day the UI will do this right, but that day has not yet come.
I think this is done? Checked using `git cl description` and it looks like it wraps properly now.
if (user_agent) {
preflight_request->cors_exempt_headers.SetHeader(
net::HttpRequestHeaders::kUserAgent, *user_agent);
} else {
// The User-Agent is now passed through the cors_exempt_headers, but we
// still need this fallback to handle Devtools usage.
user_agent = request.headers.GetHeader(net::HttpRequestHeaders::kUserAgent);
if (user_agent) {
preflight_request->cors_exempt_headers.SetHeader(
net::HttpRequestHeaders::kUserAgent, *user_agent);
}We can avoid repeating the SetHeader() call like this:
```suggestion
if (!user_agent) {
// The User-Agent is now passed through the cors_exempt_headers, but we
// still need this fallback to handle Devtools usage.
user_agent = request.headers.GetHeader(net::HttpRequestHeaders::kUserAgent);
}
if (user_agent) {
preflight_request->cors_exempt_headers.SetHeader(
net::HttpRequestHeaders::kUserAgent, *user_agent);
}
```
Oh, that's much smarter.
// EXPECT_EQ("hi", request.HttpHeaderField(http_names::kUserAgent));This test is funny - it's now always going to fail, because there's simply no need for `PrepareRequest` to put the useragent in the headers. The title of this test implies it's testing something more meaningful than just the user-agent-setting behaviour - what sort of backup should I add instead?
Related details
- Adam Rice
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Brown (Gerrit)
Andrew Brown added 1 comment![]( "Open in Gerrit")
...ah, I'm terribly sorry - I thought that would just avoid pinging you both again, not delete your reviews entirely :(
Related details
- Adam Rice
- Bo Liu
- Min Qin
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Bo Liu (Gerrit)
Bo Liu voted and added 1 comment![]( "Open in Gerrit")
Votes added by Bo Liu
| Code-Review | +1 |
1 comment
- Adam Rice
- Andrew Brown
- Min Qin
Min Qin (Gerrit)
Min Qin added 1 comment![]( "Open in Gerrit")
chrome/browser/extensions/api/downloads/downloads_api_browsertest.cc lgtm
Related details
- Andrew Brown
Min Qin (Gerrit)
Min Qin voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Andrew Brown
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Adam Rice (Gerrit)
Adam Rice voted and added 2 comments![]( "Open in Gerrit")
Votes added by Adam Rice
| Commit-Queue | +1 |
2 comments
Is this waiting on me?
AFAICT this doesn't have a chromestatus.com entry. Do you need someone to create one?
cors_exempt_headers. The default (browser or devtools generated) user-agent is now stored in the FetchContext, and stored into the cors_exempt_headers only if JavaScript didn't set one first.Andrew BrownPlease wrap the commit message to 72 columns so it looks nice in "git log" output. One day the UI will do this right, but that day has not yet come.
I think this is done? Checked using `git cl description` and it looks like it wraps properly now.
- Adam Rice
- Andrew Brown
Andrew Brown (Gerrit)
Andrew Brown added 1 comment![]( "Open in Gerrit")
Is this waiting on me?
AFAICT this doesn't have a chromestatus.com entry. Do you need someone to create one?
It's not waiting on you, you're all good :) I don't know what chromestatus.com is, though. Maybe?
At this point it's probably still not able to be merged, as there's still ~10-20 code paths I need to investigate to work out the last failing tests.
My current theory for some of the remaining test fails (hopefully most of them, but idk) is that many of the `URLLoaders` (or the things that implement `CreateLoaderAndStart`) don't currently merge `cors_exempt_headers` into their actual web request, so they don't see the new User-Agent. Basically, Blink now puts the default user-agent in `cors_exempt_header` all the time, which is great, but some of the browser process backends don't know they need to look there.
(Correct me if any of that sounds obviously incorrect.)
I'm investigating these, but it's really slow going, as they all call into each other in weird and opaque ways. Also my real life need to work, and sleep, seems to be hampering my free time a bit :(
Also there's a merge conflict, but I incidentally think I already fixed that last night, so I can upload a non-conflicting one shortly if you want to run it through the bots properly (I'm just compiling it locally to check nothing's regressed, but I can upload in an hour or two once it's done and I've checked it).
Related details
- Adam Rice
Adam Rice (Gerrit)
Adam Rice added 1 comment![]( "Open in Gerrit")
Andrew BrownIs this waiting on me?
AFAICT this doesn't have a chromestatus.com entry. Do you need someone to create one?
It's not waiting on you, you're all good :) I don't know what chromestatus.com is, though. Maybe?
At this point it's probably still not able to be merged, as there's still ~10-20 code paths I need to investigate to work out the last failing tests.
My current theory for some of the remaining test fails (hopefully most of them, but idk) is that many of the `URLLoaders` (or the things that implement `CreateLoaderAndStart`) don't currently merge `cors_exempt_headers` into their actual web request, so they don't see the new User-Agent. Basically, Blink now puts the default user-agent in `cors_exempt_header` all the time, which is great, but some of the browser process backends don't know they need to look there.
(Correct me if any of that sounds obviously incorrect.)
I'm investigating these, but it's really slow going, as they all call into each other in weird and opaque ways. Also my real life need to work, and sleep, seems to be hampering my free time a bit :(
Also there's a merge conflict, but I incidentally think I already fixed that last night, so I can upload a non-conflicting one shortly if you want to run it through the bots properly (I'm just compiling it locally to check nothing's regressed, but I can upload in an hour or two once it's done and I've checked it).
In short, whenever we ship a web platform feature that affects developers, we need approval from a group of people called "API owners". In long, https://www.chromium.org/blink/launching-features/.
Because the "Implementation of existing standard" case applies here, the process is lightweight, but it is still needed.
I wonder if we could make it simpler by only putting the header in `cors_exempt_headers` when it would make a difference? But I'm not sure whether Blink has enough information to know if it would make a difference or not.
Related details
- Andrew Brown
Andrew Brown (Gerrit)
Andrew Brown added 1 comment![]( "Open in Gerrit")
Andrew BrownIs this waiting on me?
AFAICT this doesn't have a chromestatus.com entry. Do you need someone to create one?
Adam RiceIt's not waiting on you, you're all good :) I don't know what chromestatus.com is, though. Maybe?
At this point it's probably still not able to be merged, as there's still ~10-20 code paths I need to investigate to work out the last failing tests.
My current theory for some of the remaining test fails (hopefully most of them, but idk) is that many of the `URLLoaders` (or the things that implement `CreateLoaderAndStart`) don't currently merge `cors_exempt_headers` into their actual web request, so they don't see the new User-Agent. Basically, Blink now puts the default user-agent in `cors_exempt_header` all the time, which is great, but some of the browser process backends don't know they need to look there.
(Correct me if any of that sounds obviously incorrect.)
I'm investigating these, but it's really slow going, as they all call into each other in weird and opaque ways. Also my real life need to work, and sleep, seems to be hampering my free time a bit :(
Also there's a merge conflict, but I incidentally think I already fixed that last night, so I can upload a non-conflicting one shortly if you want to run it through the bots properly (I'm just compiling it locally to check nothing's regressed, but I can upload in an hour or two once it's done and I've checked it).
In short, whenever we ship a web platform feature that affects developers, we need approval from a group of people called "API owners". In long, https://www.chromium.org/blink/launching-features/.
Because the "Implementation of existing standard" case applies here, the process is lightweight, but it is still needed.
I wonder if we could make it simpler by only putting the header in `cors_exempt_headers` when it would make a difference? But I'm not sure whether Blink has enough information to know if it would make a difference or not.
Ah, ok. Just read those, think I get it now. There definitely isn't a chromestatus entry, unless someone's sneakily put one in without telling me.
I'm on board with creating one now, but a lot of those things have like, labels and dates and planned timeframes, which scares me a little, as I don't think I'm active enough on this to commit to a timeframe (unless I take up a lot more of your time than I really should). If there's a way to post just like, "someone's working on it" in advance, then that would be good?
The trouble is that the "core fix" for 571722 was actually just removing `User-Agent` from the list in `net/http/http_util.cc`. After that, I hit the point where users are allowed to set it (yay) but the browser version is never cors exempt (boo), and the rest of the CL is attempting to sneak browser versions around in order to miss that requirement. I don't think it's possible to only add it to `cors_exempt_headers` sometimes, because it's really a problem on every web request that the browser makes.
Also, if I can ask - I'm getting a bunch of `Base (test name) in virtual suite "webnn-service-on-cpu" must refer to a real file or directory` errors that are preventing me from running `git cl upload` - do you know anything about that? Have I just merged something weird or got a symlink in the wrong spot or something?
Related details
- Adam Rice
Adam Rice (Gerrit)
Adam Rice added 1 comment![]( "Open in Gerrit")
// EXPECT_EQ("hi", request.HttpHeaderField(http_names::kUserAgent));This test is funny - it's now always going to fail, because there's simply no need for `PrepareRequest` to put the useragent in the headers. The title of this test implies it's testing something more meaningful than just the user-agent-setting behaviour - what sort of backup should I add instead?
It's verifying that resetting `dummy_page_holder` results in a call to `client->UserAgent()`. It wants to verify that `"hi"` will actually be set as the User-Agent request header on the request, but I don't know how to do that here.
Related details
- Andrew Brown
Andrew Brown (Gerrit)
Andrew Brown added 9 comments![]( "Open in Gerrit")
Andrew BrownHappy new year everyone! Apologies for dragging this up right as everyone returns from a well-deserved break :)
Figured I could use some of my summer free time to close this out for good. I worked out (some of, hopefully all) the failing devtools tests, which needed changes inside the preflight-summoning code - tagged Min Qin for review. I'm a little wary that I ended up needing to have weird case handling in the preflight code.
I have two outstanding questions:
- I've commented out the assertion in a test for frame_fetch_context. It's expecting that the User-Agent is set after calling PrepareRequest, but because this CL moves it to its own thing, that's no longer the case. Is this test checking something "deeper" that I should work out, or can it be safely removed? (I suspect this is the root cause of a few other test failures, but I never have been able to accurately run the full suite locally.)
- Do my cases for User-Agent handling make sense: a fetch-set User-Agent overrides a devtools User-Agent overrides the default? I think we've settled this, but it may be different when you consider which User-Agent should be present on preflight OPTIONS requests. At this point, it follows the same order.
Ok, not quite all the failing devtools tests :(
Some remaining failures I've traced back down to `ThrottlingURLLoader::StartNow()`, where it enters IPC/inheritance hell. I am almost willing to bet this will characterise all the remianing failures: issues in the various implementations of `URLLoaderFactory::CreateLoaderAndStart`.
Basically, I have reason to believe there are one or more `URLLoader` implementations that don't combine the `cors_exempt_headers` in with the normal `headers` on a request - similar to the change I've already made in `preflight_controller.cc`. There are different `URLLoader`'s for different things, and I bet the one that deals with extensions will have a problem, and so will the one that deals with non-returning delayed requests for `fetchLater()`.
Do you have any tips for tracking down which one(s) live on the path below the DevTools (and extension) code? The debug docs are fantastic for working bottom-up, where you have a crash and you need to work out where, but not so helpful for working top-down, where I have correct behaviour and I don't know where it stops.
There are ~43 different types of `CreateLoaderAndStart` that could lead to my problem, each with their own very large stack of behaviour, and often calling into each other in hard-to-grasp ways. I can work through them eventually, but it's going to take a long time.
I was *almost* correct; I actually had to tell the parallel stack that the inspector protocol uses, that it still needs to go find the UA elsewhere.
This means unfortunately all the work to keep the UA inside the FetchContext is useless, because the inspector protocol has no idea what context the request was made in, so there's just no way that the context-based approach is compatible with the behaviour inspector needs. I've gone back to the *original* way I did it, with a new field on the ResourceRequest.
This lets us keep basically all of the original behaviour (stuff like PrepareRequest being the function call that actually sets the UA onto the blink::ResourceRequest, instead of it happening much later when populating the network::ResourceRequest). So a lot of this CL is now semi-superfluous, as there's still some remnants of the move-to-context (though these remnants are pretty ok, in my estimation. Maybe some string copying behaviour could be optimised, idk.)
If it works as expected, there should (fingers crossed) be only a few failing tests:
- ExtensionWebRequestApiPrerenderingTest.Load
- ExtensionWebRequestApiPrerenderingTest.LoadIntoNewTab (next goal/s)
- OutOfProcessPPAPITest.URLLoader3
- PPAPINaClNewlibTest.URLLoader3 (ChromeOS only, so I can't test these locally)
It may be time for a chromestatus.com entry? I feel like there's a lot of implementation-defined behaviour here that would be good to get more eyes on, and I'm planning to be able to work on this for the next 4-5 weeks straight.
...ah, I'm terribly sorry - I thought that would just avoid pinging you both again, not delete your reviews entirely :(
Done
net::HttpRequestHeaders::kUserAgent, *user_agent);I think this code has a problem, which is that if the user has set a UA, we don't bother setting the context UA onto the cors_exempt_headers (see below comment in `request_conversion.cc`). So if there's a user UA, a preflight will be sent asking whether UA is an OK header, but *using* the custom UA that the user already set. Which is daft.
I don't know if we can fix it nicely, as I think we avoided setting UA onto both sets of headers in order to avoid merging issues with them later (header lists have no guaranteed merging behaviour, I think?). So we might have to add a special UA hiding spot to the `network::ResourceRequest` too, which is not great.
}There has to be a better way to use `getString()` here, or use another function instead, but I could not for the life of me find documentation for `DictionaryValue`, and the code I found for it was autogenerated and didn't look like it had much of a better approach available.
headers, std::make_optional(request.GetContextUserAgent())))I don't think CORS preflights go through this function, as CORS preflights still appear to be UA-less in Devtools (the actual request still has the header, just the inspector's `Network.requestWillBeSent` event is wrong.)
But also, this behaviour isn't tested ATM (I think?) - the existing tests don't appear to be checking the inspector's view of the preflight, just the network's view of it (in `http/tests/inspector-protocol/emulation/user-agent-override-cors-preflight.js`)
So, outstanding questions (the last of which I can answer myself, given time):
- Should there be a test for this behaviour (sending the right UA on a preflight, and making sure Devtools sees it too)?
- Where do preflights spawn into the inspector code? Preflights are spawned in the network process, as best I can tell (code lives in services/network, not third_party/blink), so I don't know how that information gets sent back to the render process's inspector stuff.
.setHeaders(BuildObjectForHeaders(headers_map, std::nullopt))I'm also not fully convinced that optionals are what we want here, but it feels better than creating a `BuildObjectForRequestHeaders` function. Maybe not. Thoughts welcome.
// EXPECT_EQ("hi", request.HttpHeaderField(http_names::kUserAgent));Adam RiceThis test is funny - it's now always going to fail, because there's simply no need for `PrepareRequest` to put the useragent in the headers. The title of this test implies it's testing something more meaningful than just the user-agent-setting behaviour - what sort of backup should I add instead?
It's verifying that resetting `dummy_page_holder` results in a call to `client->UserAgent()`. It wants to verify that `"hi"` will actually be set as the User-Agent request header on the request, but I don't know how to do that here.
Alright, I fiddled with it in patchset 19, but ended up needing to put the UA *back* into the ResourceRequest (unfortunately) due to inspector protocol things, so the new test just checks that the UA is correctly set inside it's new hiding spot.
PrepareRequestWhenWhenDetached) {Typo, will fix in next patchset.
}See above comment in `preflight_controller.cc` - it's both these pieces of code together.
The key thing is at this point, we can't put the context UA onto the cors_exempt_headers without potentially accepting dodgy merge behaviour later when they get combined. So we have a `network::ResourceRequest` that has a user UA on the normal headers and *no* UA on the cors_exempt headers, which then goes to `preflight_controller`, which uses the user UA.
I guess we could work out where the merge happens and make sure we remove the context UA from the `cors_exempt_headers` *if* the user UA is set? But then we need to make sure preflights avoid copying the user UA, too.
Related details
- Adam Rice
- Bo Liu
- Min Qin
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Adam Rice (Gerrit)
Adam Rice added 2 comments![]( "Open in Gerrit")
I think you're going to need to add a DevTools expert as a reviewer to be able to answer some of these questions.
if (context_ua.has_value() andNit: use `&&` not `and` in C++ code.
Related details
- Andrew Brown
- Bo Liu
- Min Qin
Andrew Brown (Gerrit)
Andrew Brown added 3 comments![]( "Open in Gerrit")
Added Alex Rudenko to review for devtools knowledge... first alphabetical owner, sorry, I get it, my name starts with A too - I have a few changes that touch devtools stuff (in `inspector_network_agent.cc`) that feel like they could be done in smarter ways. Thoughts welcome.
Also if someone could put it through a CQ dry run again that would be hugely appreciated - locally it looks like the devtools changes might have ended up fixing the remaining tests, but it would be good to check.
Nit: use `&&` not `and` in C++ code.
Done
Typo, will fix in next patchset.
- Adam Rice
- Alex Rudenko
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Rudenko (Gerrit)
Alex Rudenko added 1 comment![]( "Open in Gerrit")
@sa...@chromium.org could you please do the initial round of reviews for the devtools code?
Related details
- Adam Rice
- Andrew Brown
- Maksim Sadym
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Maksim Sadym (Gerrit)
Maksim Sadym added 1 comment![]( "Open in Gerrit")
@sa...@chromium.org could you please do the initial round of reviews for the devtools code?
- Adam Rice
- Alex Rudenko
- Andrew Brown
Maksim Sadym (Gerrit)
Maksim Sadym added 3 comments![]( "Open in Gerrit")
I took a look only at the `.../inspector_network_agent.cc`. I will leave the judgement WRT `GetContextUserAgent` method to the fetch owners.
// We need to make special care in the case of outgoing ResourceRequestsPlease move this block to `BuildObjectForResourceRequest`.
headers.Set(http_names::kReferer, AtomicString(request.ReferrerString()));As long as this is the only place the `context_ua` is set, please move the logic for adding the user agent header from `BuildObjectForHeaders` extra parameter here.
Alex Rudenko (Gerrit)
Alex Rudenko added 2 comments![]( "Open in Gerrit")
Per spec, fetch & XMLHttpRequest should be able to set customcould you please link to the parts of the spec that define this behavior?
headers, std::make_optional(request.GetContextUserAgent())))I don't think CORS preflights go through this function, as CORS preflights still appear to be UA-less in Devtools (the actual request still has the header, just the inspector's `Network.requestWillBeSent` event is wrong.)
But also, this behaviour isn't tested ATM (I think?) - the existing tests don't appear to be checking the inspector's view of the preflight, just the network's view of it (in `http/tests/inspector-protocol/emulation/user-agent-override-cors-preflight.js`)
So, outstanding questions (the last of which I can answer myself, given time):
- Should there be a test for this behaviour (sending the right UA on a preflight, and making sure Devtools sees it too)?
- Where do preflights spawn into the inspector code? Preflights are spawned in the network process, as best I can tell (code lives in services/network, not third_party/blink), so I don't know how that information gets sent back to the render process's inspector stuff.
I do not think we need to add extra headers in this event. We have requestWillBeSentExtraInfo that should report actual headers set by the network service. Could you please clarify why requestWillBeSentExtraInfo is not sufficient here?
Related details
- Adam Rice
- Andrew Brown
Andrew Brown (Gerrit)
Andrew Brown added 7 comments![]( "Open in Gerrit")
I've spent all week compiling and attempting to test and recompiling. I still have no clue if it's actually better, but it will at least not have the stupid failures the last CL run had (the bad optional access stuff) - if someone would be so kind as to hit the tryjob button that would probably save me ~6 more hours running stuff locally...
Per spec, fetch & XMLHttpRequest should be able to set customcould you please link to the parts of the spec that define this behavior?
It all derives from https://fetch.spec.whatwg.org/#forbidden-request-header -- this list was updated to remove User-Agent in 2015.
Now User-Agent isn't forbidden, it should be settable by users, and should not be on `net/http/http_util.cc`'s forbidden header list.
Unfortunately, once you remove it from that list, it also starts always generating CORS preflights. Whether it should send preflights is not directly answered by the spec (it's ambiguous), but we are emulating Firefox's behaviour, as it seems unintuitive to send preflights for every single fetch request ever asking for "User-Agent". Plus, it will break large swathes of the web for things that aren't expecting to have to explicitly say that "User-Agent" is OK.
There's other discussion on this CL about this, but to summarise where we're at:
- user-agent set by user: preflight sent
- user-agent set by devtools or browser: no preflight sent
So the solution to the CORS issue is that we take the (devtools or browser) UA from the FetchContext, and instead of putting it straight in the headers, we have to hide it around until we get to a state where it won't trigger a CORS preflight.
Unfortunately this means I've had to run around to a heap of random spots and make sure they're looking in the new spot for the UA, but eh. I probably have to update that description to summarise the new behaviour soon, as it seems to change every 2nd patchset.
// We need to make special care in the case of outgoing ResourceRequestsPlease move this block to `BuildObjectForResourceRequest`.
Done - your approach looks miles better.
There has to be a better way to use `getString()` here, or use another function instead, but I could not for the life of me find documentation for `DictionaryValue`, and the code I found for it was autogenerated and didn't look like it had much of a better approach available.
Done
headers.Set(http_names::kReferer, AtomicString(request.ReferrerString()));As long as this is the only place the `context_ua` is set, please move the logic for adding the user agent header from `BuildObjectForHeaders` extra parameter here.
Yeah, it is - moved over, done!
headers, std::make_optional(request.GetContextUserAgent())))Alex RudenkoI don't think CORS preflights go through this function, as CORS preflights still appear to be UA-less in Devtools (the actual request still has the header, just the inspector's `Network.requestWillBeSent` event is wrong.)
But also, this behaviour isn't tested ATM (I think?) - the existing tests don't appear to be checking the inspector's view of the preflight, just the network's view of it (in `http/tests/inspector-protocol/emulation/user-agent-override-cors-preflight.js`)
So, outstanding questions (the last of which I can answer myself, given time):
- Should there be a test for this behaviour (sending the right UA on a preflight, and making sure Devtools sees it too)?
- Where do preflights spawn into the inspector code? Preflights are spawned in the network process, as best I can tell (code lives in services/network, not third_party/blink), so I don't know how that information gets sent back to the render process's inspector stuff.
I do not think we need to add extra headers in this event. We have requestWillBeSentExtraInfo that should report actual headers set by the network service. Could you please clarify why requestWillBeSentExtraInfo is not sufficient here?
I'll be honest I am completely unfamiliar with DevTools and didn't know that existed; everything that I've done has been prompted by a few failing tests. My impression (and these changes) are to solve the fact that the new, cors-free "hiding" spot for the UA also needs to be given to the inspector.
Looking at requestWillBeSentExtraInfo, that seems like exactly what we want here, but this will require some tests to be updated - there's at least 8 tests failing in `web_tests/http/tests/inspector_protocol` that all are expecting a User-Agent to be present on the original `requestWillBeSent` event (which it won't be, anymore).
I could update those 8 tests to check requestWillBeSentExtraInfo, and then ensure the User-Agent is present there?
.setHeaders(BuildObjectForHeaders(headers_map, std::nullopt))I'm also not fully convinced that optionals are what we want here, but it feels better than creating a `BuildObjectForRequestHeaders` function. Maybe not. Thoughts welcome.
- Adam Rice
- Alex Rudenko
- Maksim Sadym
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Rudenko (Gerrit)
Alex Rudenko added 1 comment![]( "Open in Gerrit")
headers, std::make_optional(request.GetContextUserAgent())))Alex RudenkoI don't think CORS preflights go through this function, as CORS preflights still appear to be UA-less in Devtools (the actual request still has the header, just the inspector's `Network.requestWillBeSent` event is wrong.)
But also, this behaviour isn't tested ATM (I think?) - the existing tests don't appear to be checking the inspector's view of the preflight, just the network's view of it (in `http/tests/inspector-protocol/emulation/user-agent-override-cors-preflight.js`)
So, outstanding questions (the last of which I can answer myself, given time):
- Should there be a test for this behaviour (sending the right UA on a preflight, and making sure Devtools sees it too)?
- Where do preflights spawn into the inspector code? Preflights are spawned in the network process, as best I can tell (code lives in services/network, not third_party/blink), so I don't know how that information gets sent back to the render process's inspector stuff.
Andrew BrownI do not think we need to add extra headers in this event. We have requestWillBeSentExtraInfo that should report actual headers set by the network service. Could you please clarify why requestWillBeSentExtraInfo is not sufficient here?
I'll be honest I am completely unfamiliar with DevTools and didn't know that existed; everything that I've done has been prompted by a few failing tests. My impression (and these changes) are to solve the fact that the new, cors-free "hiding" spot for the UA also needs to be given to the inspector.
Looking at requestWillBeSentExtraInfo, that seems like exactly what we want here, but this will require some tests to be updated - there's at least 8 tests failing in `web_tests/http/tests/inspector_protocol` that all are expecting a User-Agent to be present on the original `requestWillBeSent` event (which it won't be, anymore).
I could update those 8 tests to check requestWillBeSentExtraInfo, and then ensure the User-Agent is present there?
I think it would be better to not special case the header when reporting from the renderer and update the tests that do not contain the header anymore and make sure that ExtraInfo events from the network service actually contain the correct user agent header via a new test. I would suggest the network reviewers to review the non-DevTools changes first to make sure those are correct.
Related details
- Adam Rice
- Andrew Brown
- Maksim Sadym
Alex Rudenko (Gerrit)
Alex Rudenko added 1 comment![]( "Open in Gerrit")
headers, std::make_optional(request.GetContextUserAgent())))Alex RudenkoI don't think CORS preflights go through this function, as CORS preflights still appear to be UA-less in Devtools (the actual request still has the header, just the inspector's `Network.requestWillBeSent` event is wrong.)
But also, this behaviour isn't tested ATM (I think?) - the existing tests don't appear to be checking the inspector's view of the preflight, just the network's view of it (in `http/tests/inspector-protocol/emulation/user-agent-override-cors-preflight.js`)
So, outstanding questions (the last of which I can answer myself, given time):
- Should there be a test for this behaviour (sending the right UA on a preflight, and making sure Devtools sees it too)?
- Where do preflights spawn into the inspector code? Preflights are spawned in the network process, as best I can tell (code lives in services/network, not third_party/blink), so I don't know how that information gets sent back to the render process's inspector stuff.
Andrew BrownI do not think we need to add extra headers in this event. We have requestWillBeSentExtraInfo that should report actual headers set by the network service. Could you please clarify why requestWillBeSentExtraInfo is not sufficient here?
Alex RudenkoI'll be honest I am completely unfamiliar with DevTools and didn't know that existed; everything that I've done has been prompted by a few failing tests. My impression (and these changes) are to solve the fact that the new, cors-free "hiding" spot for the UA also needs to be given to the inspector.
Looking at requestWillBeSentExtraInfo, that seems like exactly what we want here, but this will require some tests to be updated - there's at least 8 tests failing in `web_tests/http/tests/inspector_protocol` that all are expecting a User-Agent to be present on the original `requestWillBeSent` event (which it won't be, anymore).
I could update those 8 tests to check requestWillBeSentExtraInfo, and then ensure the User-Agent is present there?
I think it would be better to not special case the header when reporting from the renderer and update the tests that do not contain the header anymore and make sure that ExtraInfo events from the network service actually contain the correct user agent header via a new test. I would suggest the network reviewers to review the non-DevTools changes first to make sure those are correct.
I was reviewing it again and the user agent actually is set by the client code, right? I wonder why is it no present in `request.HttpHeaderFields()`? It does sound like it should contain it if it is provided by the client.
Alex Rudenko (Gerrit)
Alex Rudenko added 1 comment![]( "Open in Gerrit")
context_user_agent_header_ = user_agent;should this still write to the http header field via SetHttpHeaderField even if we need context_user_agent_header_ for the other logic?
Andrew Brown (Gerrit)
Andrew Brown added 2 comments![]( "Open in Gerrit")
headers, std::make_optional(request.GetContextUserAgent())))Alex RudenkoI don't think CORS preflights go through this function, as CORS preflights still appear to be UA-less in Devtools (the actual request still has the header, just the inspector's `Network.requestWillBeSent` event is wrong.)
But also, this behaviour isn't tested ATM (I think?) - the existing tests don't appear to be checking the inspector's view of the preflight, just the network's view of it (in `http/tests/inspector-protocol/emulation/user-agent-override-cors-preflight.js`)
So, outstanding questions (the last of which I can answer myself, given time):
- Should there be a test for this behaviour (sending the right UA on a preflight, and making sure Devtools sees it too)?
- Where do preflights spawn into the inspector code? Preflights are spawned in the network process, as best I can tell (code lives in services/network, not third_party/blink), so I don't know how that information gets sent back to the render process's inspector stuff.
Andrew BrownI do not think we need to add extra headers in this event. We have requestWillBeSentExtraInfo that should report actual headers set by the network service. Could you please clarify why requestWillBeSentExtraInfo is not sufficient here?
Alex RudenkoI'll be honest I am completely unfamiliar with DevTools and didn't know that existed; everything that I've done has been prompted by a few failing tests. My impression (and these changes) are to solve the fact that the new, cors-free "hiding" spot for the UA also needs to be given to the inspector.
Looking at requestWillBeSentExtraInfo, that seems like exactly what we want here, but this will require some tests to be updated - there's at least 8 tests failing in `web_tests/http/tests/inspector_protocol` that all are expecting a User-Agent to be present on the original `requestWillBeSent` event (which it won't be, anymore).
I could update those 8 tests to check requestWillBeSentExtraInfo, and then ensure the User-Agent is present there?
Alex RudenkoI think it would be better to not special case the header when reporting from the renderer and update the tests that do not contain the header anymore and make sure that ExtraInfo events from the network service actually contain the correct user agent header via a new test. I would suggest the network reviewers to review the non-DevTools changes first to make sure those are correct.
I was reviewing it again and the user agent actually is set by the client code, right? I wonder why is it no present in `request.HttpHeaderFields()`? It does sound like it should contain it if it is provided by the client.
I'm not sure if I'm interpreting this right, but I'll attempt to explain; let me know if this helps. (My terminology is crappy, sorry; it's hard to overcome my non-chromium intuition of the definitions of "browser" and "client".)
Firstly, you're right, the user-agent is set javascript code onto the normal `request.HttpHeaderFields()`. But, any User-Agent value that exists in these headers will cause a CORS OPTIONS request. (The reason it doesn't do this in real Chrome right now is because of the list in `/net/http/http_util.cc`, but that's like, the actual spec bug we're fixing.)
If we put our internal value (the one that looks like `Mozilla/5.0 Chrome 140.0` etc etc) onto those headers, we'll end up triggering a CORS preflight request on every single web request we ever make.
So we have to put our internal value somewhere else, so that it can be all combined together later. That somewhere else is (for the moment) the `request.GetContextUserAgent()` (named because the internal value comes from the `FetchContext`).
Unfortunately, anywhere that we can put the UA that hides it from CORS, also hides it from a lot of other things (including DevTools).
The alternative approach was to send the context value straight to the network process later on (patchset 19), but I thought I had to make sure it was accessible through a blink::ResourceRequest somewhere in order for DevTools to see it... *but* now I've learned that it's ok for DevTools to just get it in requestWillBeSentExtraInfo later on. So I think I'm going back to the patchset 19 approach.
context_user_agent_header_ = user_agent;should this still write to the http header field via SetHttpHeaderField even if we need context_user_agent_header_ for the other logic?
I attempted to explain in the other comment. In brief, we have to have a separate ContextUserAgent that isn't just an ordinary http header because if we put the special browser user-agent into the ordinary http headers, we end up making CORS preflights on every single request, which is bad.
Thanks to the suggestion you made about requestWillBeSetExtraInfo, I'm probably going to go back to the approach in patchset 19, where the browser/context/safe User-Agent goes straight onto the `network::ResourceRequest` and doesn't need any special-casing inside resource_request.h.
So the `SetContextUserAgent()` stuff is not long for this world - I'll get to it tomorrow (AEST), and once that's uploaded, I'll re-ping Adam & Takashi to get another do-over/more thoughts from the network/fetch side. The context-based approach was their suggestion to begin with, but it will inevitably need some tweaks for some weirder (& worse) preflight behaviour that the DevTools testing made me spot.
Related details
- Adam Rice
- Alex Rudenko
- Maksim Sadym
Andrew Brown (Gerrit)
Andrew Brown added 8 comments![]( "Open in Gerrit")
Andrew BrownHappy new year everyone! Apologies for dragging this up right as everyone returns from a well-deserved break :)
Figured I could use some of my summer free time to close this out for good. I worked out (some of, hopefully all) the failing devtools tests, which needed changes inside the preflight-summoning code - tagged Min Qin for review. I'm a little wary that I ended up needing to have weird case handling in the preflight code.
I have two outstanding questions:
- I've commented out the assertion in a test for frame_fetch_context. It's expecting that the User-Agent is set after calling PrepareRequest, but because this CL moves it to its own thing, that's no longer the case. Is this test checking something "deeper" that I should work out, or can it be safely removed? (I suspect this is the root cause of a few other test failures, but I never have been able to accurately run the full suite locally.)
- Do my cases for User-Agent handling make sense: a fetch-set User-Agent overrides a devtools User-Agent overrides the default? I think we've settled this, but it may be different when you consider which User-Agent should be present on preflight OPTIONS requests. At this point, it follows the same order.
Andrew BrownOk, not quite all the failing devtools tests :(
Some remaining failures I've traced back down to `ThrottlingURLLoader::StartNow()`, where it enters IPC/inheritance hell. I am almost willing to bet this will characterise all the remianing failures: issues in the various implementations of `URLLoaderFactory::CreateLoaderAndStart`.
Basically, I have reason to believe there are one or more `URLLoader` implementations that don't combine the `cors_exempt_headers` in with the normal `headers` on a request - similar to the change I've already made in `preflight_controller.cc`. There are different `URLLoader`'s for different things, and I bet the one that deals with extensions will have a problem, and so will the one that deals with non-returning delayed requests for `fetchLater()`.
Do you have any tips for tracking down which one(s) live on the path below the DevTools (and extension) code? The debug docs are fantastic for working bottom-up, where you have a crash and you need to work out where, but not so helpful for working top-down, where I have correct behaviour and I don't know where it stops.
There are ~43 different types of `CreateLoaderAndStart` that could lead to my problem, each with their own very large stack of behaviour, and often calling into each other in hard-to-grasp ways. I can work through them eventually, but it's going to take a long time.
I was *almost* correct; I actually had to tell the parallel stack that the inspector protocol uses, that it still needs to go find the UA elsewhere.
This means unfortunately all the work to keep the UA inside the FetchContext is useless, because the inspector protocol has no idea what context the request was made in, so there's just no way that the context-based approach is compatible with the behaviour inspector needs. I've gone back to the *original* way I did it, with a new field on the ResourceRequest.
This lets us keep basically all of the original behaviour (stuff like PrepareRequest being the function call that actually sets the UA onto the blink::ResourceRequest, instead of it happening much later when populating the network::ResourceRequest). So a lot of this CL is now semi-superfluous, as there's still some remnants of the move-to-context (though these remnants are pretty ok, in my estimation. Maybe some string copying behaviour could be optimised, idk.)
If it works as expected, there should (fingers crossed) be only a few failing tests:
- ExtensionWebRequestApiPrerenderingTest.Load
- ExtensionWebRequestApiPrerenderingTest.LoadIntoNewTab (next goal/s)
- OutOfProcessPPAPITest.URLLoader3
- PPAPINaClNewlibTest.URLLoader3 (ChromeOS only, so I can't test these locally)
It may be time for a chromestatus.com entry? I feel like there's a lot of implementation-defined behaviour here that would be good to get more eyes on, and I'm planning to be able to work on this for the next 4-5 weeks straight.
Done
Added Alex Rudenko to review for devtools knowledge... first alphabetical owner, sorry, I get it, my name starts with A too - I have a few changes that touch devtools stuff (in `inspector_network_agent.cc`) that feel like they could be done in smarter ways. Thoughts welcome.
Also if someone could put it through a CQ dry run again that would be hugely appreciated - locally it looks like the devtools changes might have ended up fixing the remaining tests, but it would be good to check.
Done
Basically everything except a handful of Extension tests (discussed soon) should pass now -- @ Adam, CQ dry run please?
That said, I have a few outstanding questions that may need broader conversations to resolve. I don't know if these are answerable with the reviewers I already have, or whether this is time for a chromestatus.com entry and/or emails to other mailing lists for different areas.
- comments in `request_conversion.cc` are wondering how best to get the context UA over to the network process for OPTIONS requests to use
- comments in `framework.js` are wondering what to do about any tests that rely/relied on DevTools for stuff
- comments in `frame_fetch_context_test.cc` are wondering whether the new version of `PrepareRequestWhenDetached` is even remotely useful or sensible
Importantly, though:
- lots of tests have changed, especially those that relied on DevTools to see UAs in a `requestWillBeSent` event. How many new tests or modifications to old tests, are needed to make sure the UA is still present on a `requestWillBeSentExtraInfo`?
- followup, which cases are sufficiently distinct that they're worth different tests for this? I've edited tests that involve redirects, cross-site redirects, etc etc -- can we assume it's all good with one more test for "UA on ExtraInfo" or do we need "UA on ExtraInfo for redirects", "UA on ExtraInfo for preflights", etc?
Per spec, fetch & XMLHttpRequest should be able to set customAndrew Browncould you please link to the parts of the spec that define this behavior?
It all derives from https://fetch.spec.whatwg.org/#forbidden-request-header -- this list was updated to remove User-Agent in 2015.
Now User-Agent isn't forbidden, it should be settable by users, and should not be on `net/http/http_util.cc`'s forbidden header list.
Unfortunately, once you remove it from that list, it also starts always generating CORS preflights. Whether it should send preflights is not directly answered by the spec (it's ambiguous), but we are emulating Firefox's behaviour, as it seems unintuitive to send preflights for every single fetch request ever asking for "User-Agent". Plus, it will break large swathes of the web for things that aren't expecting to have to explicitly say that "User-Agent" is OK.
There's other discussion on this CL about this, but to summarise where we're at:
- user-agent set by user: preflight sent
- user-agent set by devtools or browser: no preflight sent
So the solution to the CORS issue is that we take the (devtools or browser) UA from the FetchContext, and instead of putting it straight in the headers, we have to hide it around until we get to a state where it won't trigger a CORS preflight.
Unfortunately this means I've had to run around to a heap of random spots and make sure they're looking in the new spot for the UA, but eh. I probably have to update that description to summarise the new behaviour soon, as it seems to change every 2nd patchset.
Description is back to being accurate now...
function checkUserAgent(headers) {This is the root cause of a couple remaining failures:
- ExtensionWebRequestApiPrerenderingTest.Load
- ExtensionWebRequestApiPrerenderingTest.LoadIntoNewTab
They're using User-Agent as a litmus test for whether the headers on a request are valid, which is fine, but User-Agent is not going to be present on a DevTools "requestWillBeSent" event anymore. So we need a new way to do this.
EXPECT_EQ("hi", GetFetchContext()->GetDefaultUserAgent());I would resurrect my comments from patchset 14, but they don't link through to the new code.
Is this test sensible? I feel like it's not, because the name suggests to me that instead of checking UA the new way, we should instead be looking at whether PrepareRequest does it's job -- just using a different litmus test than User-Agent. Advice welcome.
Andrew Brownshould this still write to the http header field via SetHttpHeaderField even if we need context_user_agent_header_ for the other logic?
I attempted to explain in the other comment. In brief, we have to have a separate ContextUserAgent that isn't just an ordinary http header because if we put the special browser user-agent into the ordinary http headers, we end up making CORS preflights on every single request, which is bad.
Thanks to the suggestion you made about requestWillBeSetExtraInfo, I'm probably going to go back to the approach in patchset 19, where the browser/context/safe User-Agent goes straight onto the `network::ResourceRequest` and doesn't need any special-casing inside resource_request.h.
So the `SetContextUserAgent()` stuff is not long for this world - I'll get to it tomorrow (AEST), and once that's uploaded, I'll re-ping Adam & Takashi to get another do-over/more thoughts from the network/fetch side. The context-based approach was their suggestion to begin with, but it will inevitably need some tweaks for some weirder (& worse) preflight behaviour that the DevTools testing made me spot.
Now updated and removed.
See above comment in `preflight_controller.cc` - it's both these pieces of code together.
The key thing is at this point, we can't put the context UA onto the cors_exempt_headers without potentially accepting dodgy merge behaviour later when they get combined. So we have a `network::ResourceRequest` that has a user UA on the normal headers and *no* UA on the cors_exempt headers, which then goes to `preflight_controller`, which uses the user UA.
I guess we could work out where the merge happens and make sure we remove the context UA from the `cors_exempt_headers` *if* the user UA is set? But then we need to make sure preflights avoid copying the user UA, too.
So the state of affairs is thus:
- If the user provides a UA, it sits on the normal headers. In `request_conversion.cc`, we *can't* put the context UA onto the cors_exempt_headers, because that will actually clobber the user-provided one.
- If the user doesn't provide a UA, then this code does exactly what it says; there are no issues here about different handling of a DevTools-set value vs an intrinsic browser value.
But then, later (in `preflight_controller.cc`):
- If the user didn't provide a UA, then the context UA is present on the cors_exempt_headers, so we can use that and we're good.
- But if the user did provide a UA, it's present on the normal headers, and so there is no trace of the context UA. This is a big problem, because it means the only UA we have access to for our OPTIONS request is the user one, resulting in silly behaviour.
I can think of a few ways to resolve this. Thoughts from you as reviewers would be appreciated but also this is getting to be touching stuff outside blink, so if there's another mailing list I should go contact, I'm happy to do that too.
We could:
- induce magic behaviour for a User-Agent at the point of merging the two sets of headers (only take the cors_exempt_headers version if the normal isn't there)
- provide a different place for the context UA to live inside a `network::ResourceRequest` (so that it doesn't get auto-merged from the cors_exempt_headers)
- we could set this all the time, and never use the cors_exempt_headers, or
- we could set this only when the user also sets one, so the special-case to look for the new header only occurs in `preflight_controller.cc`)
I'm going to do a dummy impl of the last option when I next have time, but I'm expecting it will need changes, so hopefully with your thoughts I can get ahead of the curve on a better approach.
Related details
- Adam Rice
- Alex Rudenko
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Takashi Toyoshima (Gerrit)
Takashi Toyoshima added 8 comments![]( "Open in Gerrit")
I tried refreshing my memory, but I'm sorry if I forget the previous discussion and say something inconsistent.
Fixed: 571722I think this is an ID for the old bug trackker, and now 40450316 is the right id for this. Can you check if https://crbug.com/40450316 is the right link for your intended bug, and update this number?
----
Please fix this WARNING reported by Commit Message Validation: Did you intend to reference an old bug id? Please ensure this is a valid Buganizer ID and not an external system such as Monorail.
Analyzer Description: Validates that commit messages conform to a specific format.
Owner: ayeay...@google.com
Did you intend to reference an old bug id? Please ensure this is a valid Buganizer ID and not an external system such as Monorail.
To rerun the analyzer locally, run: `alint -- -c CommitMatch`
For more information, see go/alint.
function checkUserAgent(headers) {This is the root cause of a couple remaining failures:
- ExtensionWebRequestApiPrerenderingTest.Load
- ExtensionWebRequestApiPrerenderingTest.LoadIntoNewTab
They're using User-Agent as a litmus test for whether the headers on a request are valid, which is fine, but User-Agent is not going to be present on a DevTools "requestWillBeSent" event anymore. So we need a new way to do this.
Does this need help from qinmin@ ?
IIUC, this means the webRequest API exposed header is changed, and we need to update the API document, and need an announcement for Extensions developers and enterprise users?
if (user_agent) {
preflight_request->cors_exempt_headers.SetHeader(
net::HttpRequestHeaders::kUserAgent, *user_agent);
}This is a duplicated operation as we run the same code at line 220 eventually.
Can you remove?
// we can't use cors_exempt_headers, so the User-Agent will appear hereMaybe typo?
I think you means the User-Agent in the normal headers here.
So, s/cors_exempt_headers/it as it is not trustworthy/ ?
Also expected behavior here is:
1. if there is a DevTools provided User-Agent (in cors_exempt_headers), we use it
2. if there is a JavaScript provided User-Agent (in headers), we ignore it, and use the context given User-Agent.
Implementation-wise, this can be done simply by
```
std::optional<std::string> user_agent = request.cors_exempt_headers.GetHeader(
net::HttpRequestHeaders::kUserAgent);
if (user_agent) {
// Set DevTools given User-Agent to the preflight request
preflight_request->headers.SetHeader(
net::HttpRequestHeaders::kUserAgent, *user_agent);
} else {
// Remove JavaScript given User-Agent if it exists, and delegate to //net to set
// a context given User-Agent
preflight_request->headers.RemoveHeader(
net::HttpRequestHeaders::kUserAgent);
}
```
net::HttpRequestHeaders::kUserAgent, *user_agent);I think this code has a problem, which is that if the user has set a UA, we don't bother setting the context UA onto the cors_exempt_headers (see below comment in `request_conversion.cc`). So if there's a user UA, a preflight will be sent asking whether UA is an OK header, but *using* the custom UA that the user already set. Which is daft.
I don't know if we can fix it nicely, as I think we avoided setting UA onto both sets of headers in order to avoid merging issues with them later (header lists have no guaranteed merging behaviour, I think?). So we might have to add a special UA hiding spot to the `network::ResourceRequest` too, which is not great.
My comment above can be an answer for this problem?
// LINT.ThenChange(//tools/metrics/histograms/metadata/network/enums.xml:PrefetchMatchesResourceRequestField)Please fix this ERROR reported by If This Then That: Changes in the preceding block may need to be reflected in these files: /tools/metrics/histograms/metadata/network/enums.xml If this does not apply, add 'NO_IFTTT=some reason...' to your commit message
Analyzer Description: Reminders to change files when other files change
Owner: ayeay...@google.com
Changes in the preceding block may need to be reflected in these files: /tools/metrics/histograms/metadata/network/enums.xml If this does not apply, add 'NO_IFTTT=some reason...' to your commit message
To rerun the analyzer locally, run: `alint -- -c IfThisThenAnalyzer`
For more information, see go/alint.
// Browser-intrinsic User-Agent string, to be used for preflight requestsThis might not be needed if my comment on the preflight_controller.cc works.
Related details
- Adam Rice
- Andrew Brown
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Takashi Toyoshima (Gerrit)
Takashi Toyoshima added 1 comment![]( "Open in Gerrit")
cc: tnak as this is a User-Agent related CL.
Related details
- Adam Rice
- Andrew Brown
- Takashi Nakayama
Andrew Brown (Gerrit)
Andrew Brown added 7 comments![]( "Open in Gerrit")
I tried refreshing my memory, but I'm sorry if I forget the previous discussion and say something inconsistent.
That's totally ok, this CL is so old & complex now I keep forgetting bits too.
Apologies, the next patchset will take some time for me to upload as `git-credential-luci` has logged me out and is now forcing me to update my system to glibc 2.38. I've been meaning to switch distros for a while now so I guess this is as good an excuse as any.
I think this is an ID for the old bug trackker, and now 40450316 is the right id for this. Can you check if https://crbug.com/40450316 is the right link for your intended bug, and update this number?
----
Please fix this WARNING reported by Commit Message Validation: Did you intend to reference an old bug id? Please ensure this is a valid Buganizer ID and not an external system such as Monorail.
Analyzer Description: Validates that commit messages conform to a specific format.
Owner: ayeay...@google.comDid you intend to reference an old bug id? Please ensure this is a valid Buganizer ID and not an external system such as Monorail.
To rerun the analyzer locally, run: `alint -- -c CommitMatch`
For more information, see go/alint.
Done :)
function checkUserAgent(headers) {Takashi ToyoshimaThis is the root cause of a couple remaining failures:
- ExtensionWebRequestApiPrerenderingTest.Load
- ExtensionWebRequestApiPrerenderingTest.LoadIntoNewTab
They're using User-Agent as a litmus test for whether the headers on a request are valid, which is fine, but User-Agent is not going to be present on a DevTools "requestWillBeSent" event anymore. So we need a new way to do this.
Does this need help from qinmin@ ?
IIUC, this means the webRequest API exposed header is changed, and we need to update the API document, and need an announcement for Extensions developers and enterprise users?
Hmm. I think at the moment, yes, but that should actually be fixable.
My first idea is to ensure that the extensions stack combines the `cors_exempt_headers` into any headers returned to the user. I don't know if this is acceptable from a duplication-of-code-behaviour standpoint -- is it ok to have a bunch of places scattered around the code that pseudo-implement the network process's header merging behaviour?
The thing is, I'm uncovering a lot of behaviour that's always been "wrong", but has always been "good enough" -- lots of places where the code only considers `network::ResourceRequest`'s |headers| without combining the |cors_exempt_headers|.
Until now, |cors_exempt_headers| were niche, so it didn't matter, but now, User-Agent is on there... and lots of things care about User-Agent.
I'll try to "fix" the tests by reintroducing the User-Agent to the webRequest API first, and then get qinmin@ 's thoughts on implementation detail.
if (user_agent) {
preflight_request->cors_exempt_headers.SetHeader(
net::HttpRequestHeaders::kUserAgent, *user_agent);
}This is a duplicated operation as we run the same code at line 220 eventually.
Can you remove?
Yeah, I can condense that down. Good spot.
// we can't use cors_exempt_headers, so the User-Agent will appear hereMaybe typo?
I think you means the User-Agent in the normal headers here.
So, s/cors_exempt_headers/it as it is not trustworthy/ ?Also expected behavior here is:
1. if there is a DevTools provided User-Agent (in cors_exempt_headers), we use it
2. if there is a JavaScript provided User-Agent (in headers), we ignore it, and use the context given User-Agent.Implementation-wise, this can be done simply by
```
std::optional<std::string> user_agent = request.cors_exempt_headers.GetHeader(
net::HttpRequestHeaders::kUserAgent);
if (user_agent) {
// Set DevTools given User-Agent to the preflight request
preflight_request->headers.SetHeader(
net::HttpRequestHeaders::kUserAgent, *user_agent);
} else {
// Remove JavaScript given User-Agent if it exists, and delegate to //net to set
// a context given User-Agent
preflight_request->headers.RemoveHeader(
net::HttpRequestHeaders::kUserAgent);
}
```
I think my comments here aren't quite clear about the obscure boxes that we have to tick. In this case, the problem is when we have *both* a UA from fetch and a UA from DevTools.
There are 4(+2) cases:
- No UA at all: no preflight / send request with intrinsic UA
- Fetch UA present: send preflight with intrinsic UA / send request with fetch UA
- DevTools override & No UA on fetch: no preflight / send request with DevTools UA
- DevTools override & Fetch UA present: send preflight with DevTools UA / send request with fetch UA
- Plus,
- no UA at all, but preflight generated by another header: send preflight and request with intrinsic UA
- DevTools override & preflight generated by another header: send preflight and request with DevTools UA
(The problem is case 4, and *not* either of the bonus cases - i.e. the problem only happens when a DevTools UA *and* a fetch UA are both present.)
The reason this is an issue is because in `request_conversion.cc`, there's already a fetch UA on the `headers`. We can't put the devtools UA on the `cors_exempt_headers`, because that overrides the fetch UA on the normal `headers`.
If we don't do anything extra (like the code you suggest above) then it's fine, but in case 4, what happens is blink simply doesn't have the ability to send through the DevTools-overridden User-Agent, so the preflight is made with the original, browser-intrinsic string.
My current (local) code correctly handles all of these cases, but needs to use the `context_user_agent_for_preflights` as a bonus field to hold the DevTools UA when a fetch UA is already on the normal |headers|. I think the name is not particularly clear, and will clean up comments & naming in the next patchset.
// LINT.ThenChange(//tools/metrics/histograms/metadata/network/enums.xml:PrefetchMatchesResourceRequestField)Please fix this ERROR reported by If This Then That: Changes in the preceding block may need to be reflected in these files: /tools/metrics/histograms/metadata/network/enums.xml If this does not apply, add 'NO_IFTTT=some reason...' to your commit message
Analyzer Description: Reminders to change files when other files change
Owner: ayeay...@google.comChanges in the preceding block may need to be reflected in these files: /tools/metrics/histograms/metadata/network/enums.xml If this does not apply, add 'NO_IFTTT=some reason...' to your commit message
To rerun the analyzer locally, run: `alint -- -c IfThisThenAnalyzer`
For more information, see go/alint.
- Adam Rice
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Takashi Toyoshima (Gerrit)
Takashi Toyoshima added 6 comments![]( "Open in Gerrit")
Ah, OK. I think you are saying about the MergeFrom() operations at ConfigureUrRequest in //services/network/url_loader_util.cc or URLLoader::FollowRedirect in //services/network/url_loader.cc where we override `headers` content with `cors_exempt_headers`.
So, in such a case, how about clarifying the User-Agent handling more clearly.
- headers['User-Agent']: value that was set by JavaScript
- content_user_agent: default value that is defined by the browser (and can be overridden by DevTools)
- cors_exempt_headers['User-Agent']: forbidden (remove 'User-Agent' exempt from the cors_exempt_header_list to ensure there is no other use cases)
// LINT.ThenChange(//tools/metrics/histograms/metadata/network/enums.xml:PrefetchMatchesResourceRequestField)Andrew BrownPlease fix this ERROR reported by If This Then That: Changes in the preceding block may need to be reflected in these files: /tools/metrics/histograms/metadata/network/enums.xml If this does not apply, add 'NO_IFTTT=some reason...' to your commit message
Analyzer Description: Reminders to change files when other files change
Owner: ayeay...@google.comChanges in the preceding block may need to be reflected in these files: /tools/metrics/histograms/metadata/network/enums.xml If this does not apply, add 'NO_IFTTT=some reason...' to your commit message
To rerun the analyzer locally, run: `alint -- -c IfThisThenAnalyzer`
For more information, see go/alint.
Done.
Not yet?
We need to make an update also on //tools/metrics/histograms/metadata/network/enums.xml as the LINT error suggested.
std::optional<std::string> context_user_agent_for_preflights = std::nullopt;I prefer to drop "_for_preflights" suffix here as this information itself doesn't have any restriction for the usage.
Also, I think this is not working over IPC now?
As ResourceRequest is mapped to mojom::URLRequest, we also need to update url_request.mojom, and url_request_mojom_traits.*.
You can check services/network/public/mojom/BUILD.gn to check the typemap for this class.
virtual String GetDefaultUserAgent() const { NOTREACHED(); }Can you drop the default implementation instead of adding NOTREACHED?
We will prefer the compile time check rather than runtime check for this.
// The browser-supplied User-Agent string (comes from NetworkContext)Do you mean FetchContext?
dest->cors_exempt_headers.SetHeader(net::HttpRequestHeaders::kUserAgent,
context_default_user_agent.Utf8());So, can we set it always to the context_user_agent_for_preflights?
Related details
- Adam Rice
- Andrew Brown
Andrew Brown (Gerrit)
Andrew Brown added 12 comments![]( "Open in Gerrit")
Basically everything except a handful of Extension tests (discussed soon) should pass now -- @ Adam, CQ dry run please?
That said, I have a few outstanding questions that may need broader conversations to resolve. I don't know if these are answerable with the reviewers I already have, or whether this is time for a chromestatus.com entry and/or emails to other mailing lists for different areas.
- comments in `request_conversion.cc` are wondering how best to get the context UA over to the network process for OPTIONS requests to use
- comments in `framework.js` are wondering what to do about any tests that rely/relied on DevTools for stuff
- comments in `frame_fetch_context_test.cc` are wondering whether the new version of `PrepareRequestWhenDetached` is even remotely useful or sensible
Importantly, though:
- lots of tests have changed, especially those that relied on DevTools to see UAs in a `requestWillBeSent` event. How many new tests or modifications to old tests, are needed to make sure the UA is still present on a `requestWillBeSentExtraInfo`?
- followup, which cases are sufficiently distinct that they're worth different tests for this? I've edited tests that involve redirects, cross-site redirects, etc etc -- can we assume it's all good with one more test for "UA on ExtraInfo" or do we need "UA on ExtraInfo for redirects", "UA on ExtraInfo for preflights", etc?
Done
That sounds like a good idea to me -- implemented. A CQ run will probably identify a ton of places I've missed that need the new UA merged, but que sera.
net::HttpRequestHeaders::kUserAgent, *user_agent);Takashi ToyoshimaI think this code has a problem, which is that if the user has set a UA, we don't bother setting the context UA onto the cors_exempt_headers (see below comment in `request_conversion.cc`). So if there's a user UA, a preflight will be sent asking whether UA is an OK header, but *using* the custom UA that the user already set. Which is daft.
I don't know if we can fix it nicely, as I think we avoided setting UA onto both sets of headers in order to avoid merging issues with them later (header lists have no guaranteed merging behaviour, I think?). So we might have to add a special UA hiding spot to the `network::ResourceRequest` too, which is not great.
My comment above can be an answer for this problem?
Done via content_user_agent.
// LINT.ThenChange(//tools/metrics/histograms/metadata/network/enums.xml:PrefetchMatchesResourceRequestField)Andrew BrownPlease fix this ERROR reported by If This Then That: Changes in the preceding block may need to be reflected in these files: /tools/metrics/histograms/metadata/network/enums.xml If this does not apply, add 'NO_IFTTT=some reason...' to your commit message
Analyzer Description: Reminders to change files when other files change
Owner: ayeay...@google.comChanges in the preceding block may need to be reflected in these files: /tools/metrics/histograms/metadata/network/enums.xml If this does not apply, add 'NO_IFTTT=some reason...' to your commit message
To rerun the analyzer locally, run: `alint -- -c IfThisThenAnalyzer`
For more information, see go/alint.
Takashi ToyoshimaDone.
Not yet?
We need to make an update also on //tools/metrics/histograms/metadata/network/enums.xml as the LINT error suggested.
Sorry, yes, I had done it locally, but couldn't upload (glibc issues, now resolved).
std::optional<std::string> context_user_agent_for_preflights = std::nullopt;I prefer to drop "_for_preflights" suffix here as this information itself doesn't have any restriction for the usage.
Also, I think this is not working over IPC now?
As ResourceRequest is mapped to mojom::URLRequest, we also need to update url_request.mojom, and url_request_mojom_traits.*.
You can check services/network/public/mojom/BUILD.gn to check the typemap for this class.
Yeah, I discovered that locally too. It should be fully implemented now.
// Browser-intrinsic User-Agent string, to be used for preflight requestsAndrew BrownThis might not be needed if my comment on the preflight_controller.cc works.
Done
// already set a User-Agent via fetch.Is the terminology in this comment sensible?
headers, std::make_optional(request.GetContextUserAgent())))Alex RudenkoI don't think CORS preflights go through this function, as CORS preflights still appear to be UA-less in Devtools (the actual request still has the header, just the inspector's `Network.requestWillBeSent` event is wrong.)
But also, this behaviour isn't tested ATM (I think?) - the existing tests don't appear to be checking the inspector's view of the preflight, just the network's view of it (in `http/tests/inspector-protocol/emulation/user-agent-override-cors-preflight.js`)
So, outstanding questions (the last of which I can answer myself, given time):
- Should there be a test for this behaviour (sending the right UA on a preflight, and making sure Devtools sees it too)?
- Where do preflights spawn into the inspector code? Preflights are spawned in the network process, as best I can tell (code lives in services/network, not third_party/blink), so I don't know how that information gets sent back to the render process's inspector stuff.
Andrew BrownI do not think we need to add extra headers in this event. We have requestWillBeSentExtraInfo that should report actual headers set by the network service. Could you please clarify why requestWillBeSentExtraInfo is not sufficient here?
Alex RudenkoI'll be honest I am completely unfamiliar with DevTools and didn't know that existed; everything that I've done has been prompted by a few failing tests. My impression (and these changes) are to solve the fact that the new, cors-free "hiding" spot for the UA also needs to be given to the inspector.
Looking at requestWillBeSentExtraInfo, that seems like exactly what we want here, but this will require some tests to be updated - there's at least 8 tests failing in `web_tests/http/tests/inspector_protocol` that all are expecting a User-Agent to be present on the original `requestWillBeSent` event (which it won't be, anymore).
I could update those 8 tests to check requestWillBeSentExtraInfo, and then ensure the User-Agent is present there?
Alex RudenkoI think it would be better to not special case the header when reporting from the renderer and update the tests that do not contain the header anymore and make sure that ExtraInfo events from the network service actually contain the correct user agent header via a new test. I would suggest the network reviewers to review the non-DevTools changes first to make sure those are correct.
Andrew BrownI was reviewing it again and the user agent actually is set by the client code, right? I wonder why is it no present in `request.HttpHeaderFields()`? It does sound like it should contain it if it is provided by the client.
I'm not sure if I'm interpreting this right, but I'll attempt to explain; let me know if this helps. (My terminology is crappy, sorry; it's hard to overcome my non-chromium intuition of the definitions of "browser" and "client".)
Firstly, you're right, the user-agent is set javascript code onto the normal `request.HttpHeaderFields()`. But, any User-Agent value that exists in these headers will cause a CORS OPTIONS request. (The reason it doesn't do this in real Chrome right now is because of the list in `/net/http/http_util.cc`, but that's like, the actual spec bug we're fixing.)
If we put our internal value (the one that looks like `Mozilla/5.0 Chrome 140.0` etc etc) onto those headers, we'll end up triggering a CORS preflight request on every single web request we ever make.
So we have to put our internal value somewhere else, so that it can be all combined together later. That somewhere else is (for the moment) the `request.GetContextUserAgent()` (named because the internal value comes from the `FetchContext`).
Unfortunately, anywhere that we can put the UA that hides it from CORS, also hides it from a lot of other things (including DevTools).
The alternative approach was to send the context value straight to the network process later on (patchset 19), but I thought I had to make sure it was accessible through a blink::ResourceRequest somewhere in order for DevTools to see it... *but* now I've learned that it's ok for DevTools to just get it in requestWillBeSentExtraInfo later on. So I think I'm going back to the patchset 19 approach.
Done
virtual String GetDefaultUserAgent() const { NOTREACHED(); }Can you drop the default implementation instead of adding NOTREACHED?
We will prefer the compile time check rather than runtime check for this.
I don't think we can, unfortunately -- e.g. `GetFeatureContext()` or `GetPermissionsPolicy()` just below. I think there has to be a default implementation.
I'm not 100% confident on C++ internals, but I think the issue is that `fetch_context.h/cc` actually gets compiled into some real code at some point, so if you don't have a default implementation, the linker errors out.
// The browser-supplied User-Agent string (comes from NetworkContext)Andrew BrownDo you mean FetchContext?
Actually, this code isn't needed anymore -- removed entirely.
Andrew BrownSee above comment in `preflight_controller.cc` - it's both these pieces of code together.
The key thing is at this point, we can't put the context UA onto the cors_exempt_headers without potentially accepting dodgy merge behaviour later when they get combined. So we have a `network::ResourceRequest` that has a user UA on the normal headers and *no* UA on the cors_exempt headers, which then goes to `preflight_controller`, which uses the user UA.
I guess we could work out where the merge happens and make sure we remove the context UA from the `cors_exempt_headers` *if* the user UA is set? But then we need to make sure preflights avoid copying the user UA, too.
So the state of affairs is thus:
- If the user provides a UA, it sits on the normal headers. In `request_conversion.cc`, we *can't* put the context UA onto the cors_exempt_headers, because that will actually clobber the user-provided one.
- If the user doesn't provide a UA, then this code does exactly what it says; there are no issues here about different handling of a DevTools-set value vs an intrinsic browser value.
But then, later (in `preflight_controller.cc`):
- If the user didn't provide a UA, then the context UA is present on the cors_exempt_headers, so we can use that and we're good.
- But if the user did provide a UA, it's present on the normal headers, and so there is no trace of the context UA. This is a big problem, because it means the only UA we have access to for our OPTIONS request is the user one, resulting in silly behaviour.
I can think of a few ways to resolve this. Thoughts from you as reviewers would be appreciated but also this is getting to be touching stuff outside blink, so if there's another mailing list I should go contact, I'm happy to do that too.
We could:
- induce magic behaviour for a User-Agent at the point of merging the two sets of headers (only take the cors_exempt_headers version if the normal isn't there)
- provide a different place for the context UA to live inside a `network::ResourceRequest` (so that it doesn't get auto-merged from the cors_exempt_headers)
- we could set this all the time, and never use the cors_exempt_headers, or
- we could set this only when the user also sets one, so the special-case to look for the new header only occurs in `preflight_controller.cc`)I'm going to do a dummy impl of the last option when I next have time, but I'm expecting it will need changes, so hopefully with your thoughts I can get ahead of the curve on a better approach.
Done
dest->cors_exempt_headers.SetHeader(net::HttpRequestHeaders::kUserAgent,
context_default_user_agent.Utf8());So, can we set it always to the context_user_agent_for_preflights?
Yeah, we can now just always put the context version on the content_user_agent; the actual value from the |headers| doesn't matter at this point.
Related details
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Brown (Gerrit)
Andrew Brown added 1 comment![]( "Open in Gerrit")
Apologies, the next patchset will take some time for me to upload as `git-credential-luci` has logged me out and is now forcing me to update my system to glibc 2.38. I've been meaning to switch distros for a while now so I guess this is as good an excuse as any.
Done.
Takashi Toyoshima (Gerrit)
Takashi Toyoshima voted and added 1 comment![]( "Open in Gerrit")
Votes added by Takashi Toyoshima
| Commit-Queue | +1 |
1 comment
- Andrew Brown
Andrew Brown (Gerrit)
Andrew Brown added 1 comment![]( "Open in Gerrit")
Quick fixes, should pass everything except `external/wpt/xhr/preserve-ua-header-on-redirect.htm` -- I'll look into that now.
Related details
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Brown (Gerrit)
Not quite, actually -- the extension issues are a bit more formidable than I anticipated (and I've also been suffering through openSUSE build incompatibilities). Still working on it, though I might end up needing to ask for an Extensions expert opinion sooner than I thought.
Andrew Brown (Gerrit)
Andrew Brown added 1 comment![]( "Open in Gerrit")
Adding dtapuska@ to review for `content/public/test/url_loader_interceptor.cc`
Hopefully ok, but interested to hear thoughts on performing header merges in different locations.
Related details
- Dave Tapuska
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dave Tapuska (Gerrit)
Dave Tapuska added 1 comment![]( "Open in Gerrit")
Going to defer to Alex here, since it something related to CORS.
Related details
- Alex Moshchuk
- Andrew Brown
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Oliver Dunk (Gerrit)
Oliver Dunk added 1 comment![]( "Open in Gerrit")
Hi @rdevlin...@chromium.org, could you take a look at this one? As described in [1], it sounds like this could impact if the `User-Agent` header is exposed in the webRequest API.
Related details
- Alex Moshchuk
- Andrew Brown
- Devlin Cronin
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Devlin Cronin (Gerrit)
Devlin Cronin added 1 comment![]( "Open in Gerrit")
Hi @rdevlin...@chromium.org, could you take a look at this one? As described in [1], it sounds like this could impact if the `User-Agent` header is exposed in the webRequest API.
Thanks for tagging me in, Oliver! I agree that this is concerning for extensions, since it's a potentially large behavior change.
Would it be possible to preserve the existing behavior? The comment [here](https://chromium-review.googlesource.com/c/chromium/src/+/5273743/33/chrome/test/data/extensions/api_test/webrequest/framework.js) implies it is, with a bit of refactoring?
Related details
- Alex Moshchuk
- Andrew Brown
- Takashi Toyoshima
Andrew Brown (Gerrit)
Andrew Brown added 2 comments![]( "Open in Gerrit")
Devlin CroninHi @rdevlin...@chromium.org, could you take a look at this one? As described in [1], it sounds like this could impact if the `User-Agent` header is exposed in the webRequest API.
Thanks for tagging me in, Oliver! I agree that this is concerning for extensions, since it's a potentially large behavior change.
Would it be possible to preserve the existing behavior? The comment [here](https://chromium-review.googlesource.com/c/chromium/src/+/5273743/33/chrome/test/data/extensions/api_test/webrequest/framework.js) implies it is, with a bit of refactoring?
I wasn't sure when I posted to that mailing list, as the standard pattern for header merging isn't compatible with `OnBeforeChangeHeaders` taking a pointer to the headers, but upon further look, it appears possible to preserve the existing behaviour by just merging the new `content_user_agent` in the lead up to extensions triggering the `OnBeforeSendHeaders` event.
This feels suspect, as it appears the headers sent to the event get re-placed back onto the `network::ResourceRequest`, so I don't know if there are other implications of merging the UA back in at this point, but I'm unsure -- thoughts welcome.
Something in one of the rebase(s) is now causing a dangling ref when the browser closes :( so the tests are all failing. (Technically I can still run them and see if they pass the "test" section of the test, but they all error out immediately afterwards.) Got my weekend plans cut out for me.
Related details
- Alex Moshchuk
- Devlin Cronin
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Moshchuk (Gerrit)
Alex Moshchuk added 1 comment![]( "Open in Gerrit")
Adding dtapuska@ to review for `content/public/test/url_loader_interceptor.cc`
Hopefully ok, but interested to hear thoughts on performing header merges in different locations.
I think this is a better question for network service and loading experts (some of whom are already reviewing this CL). That change looks good mechanically (I did wonder if it's beneficial at all to do the merging in a shared helper, so that this doesn't get out of sync if other headers are added in the future, but not sure how many other places actually end up doing this), but I don't really know much about the code involved here to review this deeper, so I'll have to defer to other reviewers. Happy to approve content/ once everybody's happy with the CL and outstanding questions are resolved.
Related details
- Andrew Brown
- Devlin Cronin
- Takashi Toyoshima
Devlin Cronin (Gerrit)
Devlin Cronin added 1 comment![]( "Open in Gerrit")
Devlin CroninHi @rdevlin...@chromium.org, could you take a look at this one? As described in [1], it sounds like this could impact if the `User-Agent` header is exposed in the webRequest API.
Andrew BrownThanks for tagging me in, Oliver! I agree that this is concerning for extensions, since it's a potentially large behavior change.
Would it be possible to preserve the existing behavior? The comment [here](https://chromium-review.googlesource.com/c/chromium/src/+/5273743/33/chrome/test/data/extensions/api_test/webrequest/framework.js) implies it is, with a bit of refactoring?
I wasn't sure when I posted to that mailing list, as the standard pattern for header merging isn't compatible with `OnBeforeChangeHeaders` taking a pointer to the headers, but upon further look, it appears possible to preserve the existing behaviour by just merging the new `content_user_agent` in the lead up to extensions triggering the `OnBeforeSendHeaders` event.
This feels suspect, as it appears the headers sent to the event get re-placed back onto the `network::ResourceRequest`, so I don't know if there are other implications of merging the UA back in at this point, but I'm unsure -- thoughts welcome.
I think that's probably mostly a question for net experts -- maybe @ri...@chromium.org has thoughts?
Related details
- Andrew Brown
- Takashi Toyoshima
Adam Rice (Gerrit)
Adam Rice added 1 comment![]( "Open in Gerrit")
Devlin CroninHi @rdevlin...@chromium.org, could you take a look at this one? As described in [1], it sounds like this could impact if the `User-Agent` header is exposed in the webRequest API.
Andrew BrownThanks for tagging me in, Oliver! I agree that this is concerning for extensions, since it's a potentially large behavior change.
Would it be possible to preserve the existing behavior? The comment [here](https://chromium-review.googlesource.com/c/chromium/src/+/5273743/33/chrome/test/data/extensions/api_test/webrequest/framework.js) implies it is, with a bit of refactoring?
Devlin CroninI wasn't sure when I posted to that mailing list, as the standard pattern for header merging isn't compatible with `OnBeforeChangeHeaders` taking a pointer to the headers, but upon further look, it appears possible to preserve the existing behaviour by just merging the new `content_user_agent` in the lead up to extensions triggering the `OnBeforeSendHeaders` event.
This feels suspect, as it appears the headers sent to the event get re-placed back onto the `network::ResourceRequest`, so I don't know if there are other implications of merging the UA back in at this point, but I'm unsure -- thoughts welcome.
I think that's probably mostly a question for net experts -- maybe @ri...@chromium.org has thoughts?
As far as I can tell from reading the code, the `request_` only changes the view of the world that extensions have and isn't passed back to the network stack, so changing it should be safe.
Andrew Brown (Gerrit)
Andrew Brown added 4 comments![]( "Open in Gerrit")
Alex MoshchukAdding dtapuska@ to review for `content/public/test/url_loader_interceptor.cc`
Hopefully ok, but interested to hear thoughts on performing header merges in different locations.
I think this is a better question for network service and loading experts (some of whom are already reviewing this CL). That change looks good mechanically (I did wonder if it's beneficial at all to do the merging in a shared helper, so that this doesn't get out of sync if other headers are added in the future, but not sure how many other places actually end up doing this), but I don't really know much about the code involved here to review this deeper, so I'll have to defer to other reviewers. Happy to approve content/ once everybody's happy with the CL and outstanding questions are resolved.
Excellent -- thanks for the assist.
Devlin CroninHi @rdevlin...@chromium.org, could you take a look at this one? As described in [1], it sounds like this could impact if the `User-Agent` header is exposed in the webRequest API.
Andrew BrownThanks for tagging me in, Oliver! I agree that this is concerning for extensions, since it's a potentially large behavior change.
Would it be possible to preserve the existing behavior? The comment [here](https://chromium-review.googlesource.com/c/chromium/src/+/5273743/33/chrome/test/data/extensions/api_test/webrequest/framework.js) implies it is, with a bit of refactoring?
Devlin CroninI wasn't sure when I posted to that mailing list, as the standard pattern for header merging isn't compatible with `OnBeforeChangeHeaders` taking a pointer to the headers, but upon further look, it appears possible to preserve the existing behaviour by just merging the new `content_user_agent` in the lead up to extensions triggering the `OnBeforeSendHeaders` event.
This feels suspect, as it appears the headers sent to the event get re-placed back onto the `network::ResourceRequest`, so I don't know if there are other implications of merging the UA back in at this point, but I'm unsure -- thoughts welcome.
Adam RiceI think that's probably mostly a question for net experts -- maybe @ri...@chromium.org has thoughts?
As far as I can tell from reading the code, the `request_` only changes the view of the world that extensions have and isn't passed back to the network stack, so changing it should be safe.
Huh, that's excellent. I don't think I would have gotten that aspect, thanks for working that out!
Something in one of the rebase(s) is now causing a dangling ref when the browser closes :( so the tests are all failing. (Technically I can still run them and see if they pass the "test" section of the test, but they all error out immediately afterwards.) Got my weekend plans cut out for me.
@toyo...@chromium.org Could you put this through the bots/dry run again?
I'm still unable to test after a painful week of git tree corruption & completely impenetrable dangling pointer issues. Basically all I managed was to confirm that the dangling pointer occurs on `main` for me too, so it's clearly an issue with my build environment, not the actual contents of the CL.
If my understanding is correct, the only remaining fail should be `external/wpt/xhr/preserve-ua-header-on-redirect.htm`.
function checkUserAgent(headers) {Takashi ToyoshimaThis is the root cause of a couple remaining failures:
- ExtensionWebRequestApiPrerenderingTest.Load
- ExtensionWebRequestApiPrerenderingTest.LoadIntoNewTab
They're using User-Agent as a litmus test for whether the headers on a request are valid, which is fine, but User-Agent is not going to be present on a DevTools "requestWillBeSent" event anymore. So we need a new way to do this.
Andrew BrownDoes this need help from qinmin@ ?
IIUC, this means the webRequest API exposed header is changed, and we need to update the API document, and need an announcement for Extensions developers and enterprise users?
Hmm. I think at the moment, yes, but that should actually be fixable.
My first idea is to ensure that the extensions stack combines the `cors_exempt_headers` into any headers returned to the user. I don't know if this is acceptable from a duplication-of-code-behaviour standpoint -- is it ok to have a bunch of places scattered around the code that pseudo-implement the network process's header merging behaviour?
The thing is, I'm uncovering a lot of behaviour that's always been "wrong", but has always been "good enough" -- lots of places where the code only considers `network::ResourceRequest`'s |headers| without combining the |cors_exempt_headers|.
Until now, |cors_exempt_headers| were niche, so it didn't matter, but now, User-Agent is on there... and lots of things care about User-Agent.
I'll try to "fix" the tests by reintroducing the User-Agent to the webRequest API first, and then get qinmin@ 's thoughts on implementation detail.
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Devlin Cronin (Gerrit)
Devlin Cronin voted and added 2 comments![]( "Open in Gerrit")
Votes added by Devlin Cronin
| Code-Review | +1 |
2 comments
Thanks, Andrew! Extensions LGTM % nit, since there's now no behavior change.
request_.headers.MergeAndAddUA(request_.cors_exempt_headers,nit: maybe add a brief comment about why we need to do this, since there's been a good amount of exploration / discussion?
Related details
- Andrew Brown
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Brown (Gerrit)
Andrew Brown added 2 comments![]( "Open in Gerrit")
Thanks, Andrew! Extensions LGTM % nit, since there's now no behavior change.
Unfortunately the current test results do make this seem like it's not yet fixed.
The few failures I've actually drilled into are *all* requests that have gone through the extensions stack, now needing a specific `User-Agent` CORS preflight response.
This means the User-Agent getting added onto the headers in the `OnBeforeSendHeaders` event is actually affecting the real network request somehow, causing it to spot a non-CORS-exempt header on the headers and send a preflight (which the tests don't correctly respond to).
So all requests that go through the Extensions stack are generating a CORS preflight for the User-Agent header, which most things won't expect and respond to. We need to find an alternative approach; merging here isn't suitable.
My first guess is that the associated callback function might be our solution, if it lets me pass the `content_user_agent` back up to the front-end consumer for merging outside of all of this business? I don't know if that's feasible/reasonable at all though, considering that goes back into the black hole of mojom; it looks like it would need a lot of modifications all over the place in unrelated contexts.
Other than that... I'm not sure.
request_.headers.MergeAndAddUA(request_.cors_exempt_headers,nit: maybe add a brief comment about why we need to do this, since there's been a good amount of exploration / discussion?
- Devlin Cronin
Takashi Toyoshima (Gerrit)
Takashi Toyoshima added 2 comments![]( "Open in Gerrit")
Sorry, I missed this reply.
virtual String GetDefaultUserAgent() const { NOTREACHED(); }Andrew BrownCan you drop the default implementation instead of adding NOTREACHED?
We will prefer the compile time check rather than runtime check for this.
I don't think we can, unfortunately -- e.g. `GetFeatureContext()` or `GetPermissionsPolicy()` just below. I think there has to be a default implementation.
I'm not 100% confident on C++ internals, but I think the issue is that `fetch_context.h/cc` actually gets compiled into some real code at some point, so if you don't have a default implementation, the linker errors out.
Ah, you are right. Sorry, I forgot the fact that this class could be directly used too.
Related details
- Andrew Brown
- Devlin Cronin
Devlin Cronin (Gerrit)
Devlin Cronin added 1 comment![]( "Open in Gerrit")
Andrew BrownThanks, Andrew! Extensions LGTM % nit, since there's now no behavior change.
Unfortunately the current test results do make this seem like it's not yet fixed.
The few failures I've actually drilled into are *all* requests that have gone through the extensions stack, now needing a specific `User-Agent` CORS preflight response.
This means the User-Agent getting added onto the headers in the `OnBeforeSendHeaders` event is actually affecting the real network request somehow, causing it to spot a non-CORS-exempt header on the headers and send a preflight (which the tests don't correctly respond to).
So all requests that go through the Extensions stack are generating a CORS preflight for the User-Agent header, which most things won't expect and respond to. We need to find an alternative approach; merging here isn't suitable.
My first guess is that the associated callback function might be our solution, if it lets me pass the `content_user_agent` back up to the front-end consumer for merging outside of all of this business? I don't know if that's feasible/reasonable at all though, considering that goes back into the black hole of mojom; it looks like it would need a lot of modifications all over the place in unrelated contexts.
Other than that... I'm not sure.
Thanks for the continued investigation!
Unfortunately, I'm not sure of what the right solution here is, either. Since this is mostly touching the net stack, I'd defer to one of those owners. @ri...@chromium.org?
Related details
- Andrew Brown
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Recitation-Check
Devlin Cronin (Gerrit)
Devlin Cronin voted Code-Review+0![]( "Open in Gerrit")
| Code-Review | +0 |
Andrew Brown (Gerrit)
Andrew Brown added 5 comments![]( "Open in Gerrit")
Andrew BrownSomething in one of the rebase(s) is now causing a dangling ref when the browser closes :( so the tests are all failing. (Technically I can still run them and see if they pass the "test" section of the test, but they all error out immediately afterwards.) Got my weekend plans cut out for me.
@toyo...@chromium.org Could you put this through the bots/dry run again?
I'm still unable to test after a painful week of git tree corruption & completely impenetrable dangling pointer issues. Basically all I managed was to confirm that the dangling pointer occurs on `main` for me too, so it's clearly an issue with my build environment, not the actual contents of the CL.
If my understanding is correct, the only remaining fail should be `external/wpt/xhr/preserve-ua-header-on-redirect.htm`.
Done
Andrew BrownThanks, Andrew! Extensions LGTM % nit, since there's now no behavior change.
Devlin CroninUnfortunately the current test results do make this seem like it's not yet fixed.
The few failures I've actually drilled into are *all* requests that have gone through the extensions stack, now needing a specific `User-Agent` CORS preflight response.
This means the User-Agent getting added onto the headers in the `OnBeforeSendHeaders` event is actually affecting the real network request somehow, causing it to spot a non-CORS-exempt header on the headers and send a preflight (which the tests don't correctly respond to).
So all requests that go through the Extensions stack are generating a CORS preflight for the User-Agent header, which most things won't expect and respond to. We need to find an alternative approach; merging here isn't suitable.
My first guess is that the associated callback function might be our solution, if it lets me pass the `content_user_agent` back up to the front-end consumer for merging outside of all of this business? I don't know if that's feasible/reasonable at all though, considering that goes back into the black hole of mojom; it looks like it would need a lot of modifications all over the place in unrelated contexts.
Other than that... I'm not sure.
Thanks for the continued investigation!
Unfortunately, I'm not sure of what the right solution here is, either. Since this is mostly touching the net stack, I'd defer to one of those owners. @ri...@chromium.org?
I don't know if this is taboo to ping a second person for the same stuff, but I'll also add @cdu...@chromium.org for thoughts -- I was snooping the `OWNERS` of nearby code and he sounds like he might have expertise in this area (specifically the overlap between extensions & networking).
(Relevant file is `extensions/browser/api/web_request/web_request_proxying_url_loader_factory.cc` w/r/t "How do we get the `content_user_agent` into Extensions before `OnBeforeSendHeaders` fires, without merging it into the request's normal headers?")
I've temporarily commented out the mystery-tbd-Extensions-merging stuff that we're still working out -- could someone put this CL through a dryrun so I can check the redirect implementation is all working? I've made changes that fixed the last dubious test locally; just want to check it hasn't broken anything else in the process.
It should pass everything *but* the two Extensions tests.
request_.headers.MergeAndAddUA(request_.cors_exempt_headers,Andrew Brownnit: maybe add a brief comment about why we need to do this, since there's been a good amount of exploration / discussion?
Can do, but TBD.
I'll resolve this comment for now, but I'll make sure there's a good few comments around whatever the final implementation ends up being.
I'll just leave this one unresolved because it has the nicest summary of the whole setup we've got going here.
Related details
- Adam Rice
- Clark DuVall
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Adam Rice (Gerrit)
Adam Rice added 3 comments![]( "Open in Gerrit")
Andrew BrownThanks, Andrew! Extensions LGTM % nit, since there's now no behavior change.
Devlin CroninUnfortunately the current test results do make this seem like it's not yet fixed.
The few failures I've actually drilled into are *all* requests that have gone through the extensions stack, now needing a specific `User-Agent` CORS preflight response.
This means the User-Agent getting added onto the headers in the `OnBeforeSendHeaders` event is actually affecting the real network request somehow, causing it to spot a non-CORS-exempt header on the headers and send a preflight (which the tests don't correctly respond to).
So all requests that go through the Extensions stack are generating a CORS preflight for the User-Agent header, which most things won't expect and respond to. We need to find an alternative approach; merging here isn't suitable.
My first guess is that the associated callback function might be our solution, if it lets me pass the `content_user_agent` back up to the front-end consumer for merging outside of all of this business? I don't know if that's feasible/reasonable at all though, considering that goes back into the black hole of mojom; it looks like it would need a lot of modifications all over the place in unrelated contexts.
Other than that... I'm not sure.
Thanks for the continued investigation!
Unfortunately, I'm not sure of what the right solution here is, either. Since this is mostly touching the net stack, I'd defer to one of those owners. @ri...@chromium.org?
I misunderstood the code when I looked at it before. This is happening before we call `CreateLoaderAndStart()`, so any modifications to `request_` will affect the real request. So we can't modify `request_` here.
What I propose instead is to create a new `headers_with_user_agent_` member variable in the `InProgressRequest` class, which is a copy of `request_.headers` but with the User-Agent header added. Then anywhere we are currently calling a WebRequestEventRouter with a pointer to `request_.headers`, pass a pointer to `headers_with_user_agent_` instead.
Any code which modifies `request_.headers`, for example WebRequestProxyingURLLoaderFactory::InProgressRequest::FollowRedirect(), must recreate `headers_with_user_agent_` to make sure it remains correct. Possibly it will be sufficient to put that code in the UpdateRequestInfo() method?
The extra cost of copying the headers on the request fast path bothers me, but I can't see any way around it.
void MergeAndAddUA(const HttpRequestHeaders& other,Since this method is rather specialised and doesn't require access to HttpRequestHeaders internals, I would prefer if it was not a method on the class. A static function in `net::HttpUtil` would be preferable. Something like
```
static HttpRequestHeaders MergeHeadersAndAddUserAgent(
net::HttpRequestHeaders info,
const net::HttpRequestHeaders& from,
std::optional<std::string> user_agent);
```
The asymmetry between the passing convention for the parameters is so you can do
```
existing =
net::HttpUtil::MergeHeadersAndAddUserAgent(std::move(existing), extra_headers, user_agent);
```
and it will efficiently move the `existing` object to the output, or
```
net::HttpRequestHeaders merged =
net::HttpUtil::MargeHeadersAndAddUserAgent(base, extra_headers, user_agent);
```
when you need a new object.
// already set a User-Agent via fetch.Is the terminology in this comment sensible?
How about something like:
```
The correct value for the "User-Agent" header from the point-of-view of the
caller, after DevTools and other overrides have been applied. If this is not set, the "User-Agent" value configured on the NetworkContext will be used instead.
This can be overridden by setting a "User-Agent" in `headers`, but that will
require a CORS preflight for an untrusted client.
The `User-Agent` header cannot be set both in `cors_exempt_headers` as it will
not be merged correctly if it was also set in `headers`.
```
Related details
- Adam Rice
- Clark DuVall
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Adam Rice (Gerrit)
Adam Rice added 1 comment![]( "Open in Gerrit")
EXPECT_EQ("hi", GetFetchContext()->GetDefaultUserAgent());Adam RiceI would resurrect my comments from patchset 14, but they don't link through to the new code.
Is this test sensible? I feel like it's not, because the name suggests to me that instead of checking UA the new way, we should instead be looking at whether PrepareRequest does it's job -- just using a different litmus test than User-Agent. Advice welcome.
Looking at the original CL, there does seem to be a point to this:
https://chromium-review.googlesource.com/c/chromium/src/+/518743
Detaching the page causes the user agent string to be cached, and so the test is verifying that the code is doing this correctly.
Maybe add some comments, but keep it the same? Or add another test that does actually call `PrepareRequest()` and verifies something else it is supposed to be doing?
Related details
- Andrew Brown
- Clark DuVall
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Brown (Gerrit)
Andrew Brown added 5 comments![]( "Open in Gerrit")
Andrew BrownThanks, Andrew! Extensions LGTM % nit, since there's now no behavior change.
Devlin CroninUnfortunately the current test results do make this seem like it's not yet fixed.
The few failures I've actually drilled into are *all* requests that have gone through the extensions stack, now needing a specific `User-Agent` CORS preflight response.
This means the User-Agent getting added onto the headers in the `OnBeforeSendHeaders` event is actually affecting the real network request somehow, causing it to spot a non-CORS-exempt header on the headers and send a preflight (which the tests don't correctly respond to).
So all requests that go through the Extensions stack are generating a CORS preflight for the User-Agent header, which most things won't expect and respond to. We need to find an alternative approach; merging here isn't suitable.
My first guess is that the associated callback function might be our solution, if it lets me pass the `content_user_agent` back up to the front-end consumer for merging outside of all of this business? I don't know if that's feasible/reasonable at all though, considering that goes back into the black hole of mojom; it looks like it would need a lot of modifications all over the place in unrelated contexts.
Other than that... I'm not sure.
Adam RiceThanks for the continued investigation!
Unfortunately, I'm not sure of what the right solution here is, either. Since this is mostly touching the net stack, I'd defer to one of those owners. @ri...@chromium.org?
I misunderstood the code when I looked at it before. This is happening before we call `CreateLoaderAndStart()`, so any modifications to `request_` will affect the real request. So we can't modify `request_` here.
What I propose instead is to create a new `headers_with_user_agent_` member variable in the `InProgressRequest` class, which is a copy of `request_.headers` but with the User-Agent header added. Then anywhere we are currently calling a WebRequestEventRouter with a pointer to `request_.headers`, pass a pointer to `headers_with_user_agent_` instead.
Any code which modifies `request_.headers`, for example WebRequestProxyingURLLoaderFactory::InProgressRequest::FollowRedirect(), must recreate `headers_with_user_agent_` to make sure it remains correct. Possibly it will be sufficient to put that code in the UpdateRequestInfo() method?
The extra cost of copying the headers on the request fast path bothers me, but I can't see any way around it.
I've implemented this (I think?) -- dryrun please.
I've called it `user_visible_headers_` with the logic that they're the headers that get sent back up to the "user" (i.e. extensions). Terminology check would be appreciated.
My first attempt (which is now uploaded) seemed to pass the tests that I ran locally, which scares me a little, but if it works it works.
Having now written it, I definitely can see where you're not keen on the extra copy... it feels bad to be making the `user_visible_headers_` in the constructor and then completely remaking them again in `ContinueToBeforeSendHeaders`. There might be a way around it if we try to ensure that `user_visible_headers_` are updated wherever the `headers` change? But that seems like a lot of code complexity, so eh.
I've temporarily commented out the mystery-tbd-Extensions-merging stuff that we're still working out -- could someone put this CL through a dryrun so I can check the redirect implementation is all working? I've made changes that fixed the last dubious test locally; just want to check it hasn't broken anything else in the process.
It should pass everything *but* the two Extensions tests.
Done
void MergeAndAddUA(const HttpRequestHeaders& other,Since this method is rather specialised and doesn't require access to HttpRequestHeaders internals, I would prefer if it was not a method on the class. A static function in `net::HttpUtil` would be preferable. Something like
```
static HttpRequestHeaders MergeHeadersAndAddUserAgent(
net::HttpRequestHeaders info,
const net::HttpRequestHeaders& from,
std::optional<std::string> user_agent);
```
The asymmetry between the passing convention for the parameters is so you can do
```
existing =
net::HttpUtil::MergeHeadersAndAddUserAgent(std::move(existing), extra_headers, user_agent);
```
and it will efficiently move the `existing` object to the output, or
```
net::HttpRequestHeaders merged =
net::HttpUtil::MargeHeadersAndAddUserAgent(base, extra_headers, user_agent);
```
when you need a new object.
Huh, nifty. Done!
// already set a User-Agent via fetch.Adam RiceIs the terminology in this comment sensible?
How about something like:
```
The correct value for the "User-Agent" header from the point-of-view of the
caller, after DevTools and other overrides have been applied. If this is not set, the "User-Agent" value configured on the NetworkContext will be used instead.
This can be overridden by setting a "User-Agent" in `headers`, but that will
require a CORS preflight for an untrusted client.The `User-Agent` header cannot be set both in `cors_exempt_headers` as it will
not be merged correctly if it was also set in `headers`.
```
Added with a few slight changes.
EXPECT_EQ("hi", GetFetchContext()->GetDefaultUserAgent());Adam RiceI would resurrect my comments from patchset 14, but they don't link through to the new code.
Is this test sensible? I feel like it's not, because the name suggests to me that instead of checking UA the new way, we should instead be looking at whether PrepareRequest does it's job -- just using a different litmus test than User-Agent. Advice welcome.
Looking at the original CL, there does seem to be a point to this:
https://chromium-review.googlesource.com/c/chromium/src/+/518743Detaching the page causes the user agent string to be cached, and so the test is verifying that the code is doing this correctly.
Maybe add some comments, but keep it the same? Or add another test that does actually call `PrepareRequest()` and verifies something else it is supposed to be doing?
I've added a new `PrepareRequestWhenDetached` that checks the UkmSourceID.
I think that should be equivalent(?), as far as the old test was checking that you could call PrepareRequest while detached and it would still do some sort of stuff.
Related details
- Adam Rice
- Clark DuVall
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Adam Rice (Gerrit)
Adam Rice voted and added 6 comments![]( "Open in Gerrit")
Votes added by Adam Rice
| Commit-Queue | +1 |
6 comments
net::HttpRequestHeaders user_visible_headers_;How about `script_exposed_headers_` as an alternative name?
// that has to happen further down (after CORS checks).I suggest "has to happen in the network service" would be clearer.
user_visible_headers_ = net::HttpUtil::MergeHeadersAndAddUserAgent(I think this should go in the member initialiser section above. Also in the other constructor below.
*(user_agent));Nit: `()` around `user_agent` are not needed.
net::HttpRequestHeaders::kUserAgent, *(content_user_agent_));Nit: `()` not needed around `content_user_agent_`.
EXPECT_EQ(ukm::kInvalidSourceId, request.GetUkmSourceId());Unfortunately `ukm::kInvalidSourceId` is the default value for this field, so it doesn't really prove that `PrepareRequest()` did anything.
Related details
- Adam Rice
- Andrew Brown
- Clark DuVall
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Brown (Gerrit)
Andrew Brown added 1 comment![]( "Open in Gerrit")
EXPECT_EQ(ukm::kInvalidSourceId, request.GetUkmSourceId());Unfortunately `ukm::kInvalidSourceId` is the default value for this field, so it doesn't really prove that `PrepareRequest()` did anything.
Ah, so it is. I spotted in `t/b/r/core/testing/dummy_page_holder.cc:134` that the document source ID for these tests was initialised specifically as `kInvalidSourceId`, so I figured it would be a decent test example, but didn't check the ResourceRequest default.
Would it be possible to add a quick constant to ukm, call it `ukm::kTestingSourceID`, that could be set at `dummy_page_holder.cc:134` and used for this test? I'm a little surprised there's not one already, but I guess it would be a bit niche.
Otherwise the next option would be looking into `request.SetStorageApiAccess` in `FrameFetchContext::PrepareRequest` to see if it sets a value we can test.
Related details
- Adam Rice
- Clark DuVall
Adam Rice (Gerrit)
Adam Rice added 1 comment![]( "Open in Gerrit")
EXPECT_EQ(ukm::kInvalidSourceId, request.GetUkmSourceId());Andrew BrownUnfortunately `ukm::kInvalidSourceId` is the default value for this field, so it doesn't really prove that `PrepareRequest()` did anything.
Ah, so it is. I spotted in `t/b/r/core/testing/dummy_page_holder.cc:134` that the document source ID for these tests was initialised specifically as `kInvalidSourceId`, so I figured it would be a decent test example, but didn't check the ResourceRequest default.
Would it be possible to add a quick constant to ukm, call it `ukm::kTestingSourceID`, that could be set at `dummy_page_holder.cc:134` and used for this test? I'm a little surprised there's not one already, but I guess it would be a bit niche.
Otherwise the next option would be looking into `request.SetStorageApiAccess` in `FrameFetchContext::PrepareRequest` to see if it sets a value we can test.
On further inspection, PrepareRequest() only does two things when detached, and the only one that has a side effect is setting the user agent. So there's nothing here we can test. I think we should just delete this "PrepareRequestWhenDetached" test because it isn't adding any value.
Related details
- Andrew Brown
- Clark DuVall
Andrew Brown (Gerrit)
Andrew Brown added 8 comments![]( "Open in Gerrit")
Andrew BrownThanks, Andrew! Extensions LGTM % nit, since there's now no behavior change.
Devlin CroninUnfortunately the current test results do make this seem like it's not yet fixed.
The few failures I've actually drilled into are *all* requests that have gone through the extensions stack, now needing a specific `User-Agent` CORS preflight response.
This means the User-Agent getting added onto the headers in the `OnBeforeSendHeaders` event is actually affecting the real network request somehow, causing it to spot a non-CORS-exempt header on the headers and send a preflight (which the tests don't correctly respond to).
So all requests that go through the Extensions stack are generating a CORS preflight for the User-Agent header, which most things won't expect and respond to. We need to find an alternative approach; merging here isn't suitable.
My first guess is that the associated callback function might be our solution, if it lets me pass the `content_user_agent` back up to the front-end consumer for merging outside of all of this business? I don't know if that's feasible/reasonable at all though, considering that goes back into the black hole of mojom; it looks like it would need a lot of modifications all over the place in unrelated contexts.
Other than that... I'm not sure.
Adam RiceThanks for the continued investigation!
Unfortunately, I'm not sure of what the right solution here is, either. Since this is mostly touching the net stack, I'd defer to one of those owners. @ri...@chromium.org?
Andrew BrownI misunderstood the code when I looked at it before. This is happening before we call `CreateLoaderAndStart()`, so any modifications to `request_` will affect the real request. So we can't modify `request_` here.
What I propose instead is to create a new `headers_with_user_agent_` member variable in the `InProgressRequest` class, which is a copy of `request_.headers` but with the User-Agent header added. Then anywhere we are currently calling a WebRequestEventRouter with a pointer to `request_.headers`, pass a pointer to `headers_with_user_agent_` instead.
Any code which modifies `request_.headers`, for example WebRequestProxyingURLLoaderFactory::InProgressRequest::FollowRedirect(), must recreate `headers_with_user_agent_` to make sure it remains correct. Possibly it will be sufficient to put that code in the UpdateRequestInfo() method?
The extra cost of copying the headers on the request fast path bothers me, but I can't see any way around it.
I've implemented this (I think?) -- dryrun please.
I've called it `user_visible_headers_` with the logic that they're the headers that get sent back up to the "user" (i.e. extensions). Terminology check would be appreciated.
My first attempt (which is now uploaded) seemed to pass the tests that I ran locally, which scares me a little, but if it works it works.
Having now written it, I definitely can see where you're not keen on the extra copy... it feels bad to be making the `user_visible_headers_` in the constructor and then completely remaking them again in `ContinueToBeforeSendHeaders`. There might be a way around it if we try to ensure that `user_visible_headers_` are updated wherever the `headers` change? But that seems like a lot of code complexity, so eh.
Continuing this discussion, I think we have further problems.
The entire extensions stack seems to pass around the one headers object for all modifications, including the actual modifications that extensions could make (e.g. by DeclarativeNetRequest). So we have to somehow get these modifications back to the real request somehow (*without* passing the content_user_agent and triggering CORS).
I can think of two ways forwards to try to maintain the existing behaviour:
- Literally just check the value of the User-Agent header when the `proxying_url_loader` gets the headers back, and if it's an exact match for the `content_user_agent`, remove it.
Problems: 1. we would actually have to check string equality for the header value, which has a bit of of a bad code smell, and 2. What happens in the pathological case where an extension manually sets the header to the same value?
- Somehow special-case the User-Agent *inside* net::HttpRequestHeaders. Basically, store some sort of flag for "has the UA been modified," and only remove the UA if the headers object reports it as unchanged. (I attempted to originally do this by making an actual subclass of `net::HttpRequestHeaders` inside the `InProgressRequest` that had special user-agent-tracking behaviour. I was probably messing up the inheritance somehow but it didn't seem like a good solution.)
Problems: modifying the actual `net::HttpRequestHeaders` for this seems like a bad idea.
How about `script_exposed_headers_` as an alternative name?
Done
I suggest "has to happen in the network service" would be clearer.
Done
user_visible_headers_ = net::HttpUtil::MergeHeadersAndAddUserAgent(I think this should go in the member initialiser section above. Also in the other constructor below.
Done
Nit: `()` around `user_agent` are not needed.
Done
net::HttpRequestHeaders::kUserAgent, *(content_user_agent_));Nit: `()` not needed around `content_user_agent_`.
Done
EXPECT_EQ("hi", GetFetchContext()->GetDefaultUserAgent());Adam RiceI would resurrect my comments from patchset 14, but they don't link through to the new code.
Is this test sensible? I feel like it's not, because the name suggests to me that instead of checking UA the new way, we should instead be looking at whether PrepareRequest does it's job -- just using a different litmus test than User-Agent. Advice welcome.
Andrew BrownLooking at the original CL, there does seem to be a point to this:
https://chromium-review.googlesource.com/c/chromium/src/+/518743Detaching the page causes the user agent string to be cached, and so the test is verifying that the code is doing this correctly.
Maybe add some comments, but keep it the same? Or add another test that does actually call `PrepareRequest()` and verifies something else it is supposed to be doing?
I've added a new `PrepareRequestWhenDetached` that checks the UkmSourceID.
I think that should be equivalent(?), as far as the old test was checking that you could call PrepareRequest while detached and it would still do some sort of stuff.
Done
EXPECT_EQ(ukm::kInvalidSourceId, request.GetUkmSourceId());Andrew BrownUnfortunately `ukm::kInvalidSourceId` is the default value for this field, so it doesn't really prove that `PrepareRequest()` did anything.
Adam RiceAh, so it is. I spotted in `t/b/r/core/testing/dummy_page_holder.cc:134` that the document source ID for these tests was initialised specifically as `kInvalidSourceId`, so I figured it would be a decent test example, but didn't check the ResourceRequest default.
Would it be possible to add a quick constant to ukm, call it `ukm::kTestingSourceID`, that could be set at `dummy_page_holder.cc:134` and used for this test? I'm a little surprised there's not one already, but I guess it would be a bit niche.
Otherwise the next option would be looking into `request.SetStorageApiAccess` in `FrameFetchContext::PrepareRequest` to see if it sets a value we can test.
On further inspection, PrepareRequest() only does two things when detached, and the only one that has a side effect is setting the user agent. So there's nothing here we can test. I think we should just delete this "PrepareRequestWhenDetached" test because it isn't adding any value.
- Adam Rice
- Clark DuVall
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |