[Blink/Network] Allow extensions to set Origin header in fetch requests [chromium/src : main]
Justin Lulejian (Gerrit)
Justin Lulejian added 2 comments![]( "Open in Gerrit")
Hi Devlin! Could you review the .*extensions.* changes and overall approach? I scoped this down to just the Origin header since that was the only header mentioned in the proposal. We could unrestrict this in the future, but it seemed better to allowlist headers as-needed at first.
Note: This script duplicates logic from `../fetch_common_tests.js`. IIUC user scripts cannot import other scripts so we have to duplicate it?
Related details
- Devlin Cronin
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Devlin Cronin (Gerrit)
Devlin Cronin added 11 comments![]( "Open in Gerrit")
Thanks, Justin! General approach LG.
ResultCatcher content_script_catcher;nit: user script, not content script
class ServiceWorkerFetchTest : public ServiceWorkerTest,nit: can you move these in a precursor CL so that one CL is just copy-paste, and it's easier to tell what's new in this CL?
await new Promise((resolve) => chrome.test.getConfig(resolve));nit: since we're here, this can be simplified to just `const config = await chrome.test.getConfig()`. We allow promises in all contexts
try {nit: no need to wrap in a try-catch. The test infrastructure will throw an error if an exception is thrown (here and below)
await new Promise((resolve) => chrome.test.getConfig(resolve));nit: cache this above, i.e. above all tests have
```
let config;
```
and then in the first test:
```
config = ...;
```
that way, we don't have to refetch.
config.testServer.port}/fetch/fetch_forbidden.html?test=fetchAndSet`;nit: maybe extract to a getUrl() helper method?
// https://developer.mozilla.org/en-US/docs/Glossary/Forbidden_request_headernit:expand to note that this should still succeed, and silently drop the header
if (config.customArg !== 'run_content_script_tests') {instead of this, could we use different hosts or paths for the user script and content script? e.g., inject the content script on content-scripts.test and the user script on user-scripts.test, and then we only ever inject one of them.
Note: This script duplicates logic from `../fetch_common_tests.js`. IIUC user scripts cannot import other scripts so we have to duplicate it?
yeah, that's pretty much correct. There could be some workarounds, but not worth it for the relatively small amount here.
// request header, return. The user agent may choose to skip this step andI'll leave it for blink owners to decide, but this hasn't _yet_ been accepted into the fetch spec.
I'd put it as a non-quoted comment below.
Related details
- Justin Lulejian
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Lulejian (Gerrit)
Justin Lulejian added 9 comments![]( "Open in Gerrit")
nit: user script, not content script
Done
class ServiceWorkerFetchTest : public ServiceWorkerTest,nit: can you move these in a precursor CL so that one CL is just copy-paste, and it's easier to tell what's new in this CL?
Done as [Split out tests moving and fix comments/expectations (7614727)](https://crrev.com/c/7614727)
await new Promise((resolve) => chrome.test.getConfig(resolve));nit: since we're here, this can be simplified to just `const config = await chrome.test.getConfig()`. We allow promises in all contexts
Done
nit: no need to wrap in a try-catch. The test infrastructure will throw an error if an exception is thrown (here and below)
Done
await new Promise((resolve) => chrome.test.getConfig(resolve));nit: cache this above, i.e. above all tests have
```
let config;
```and then in the first test:
```
config = ...;
```that way, we don't have to refetch.
Done
config.testServer.port}/fetch/fetch_forbidden.html?test=fetchAndSet`;nit: maybe extract to a getUrl() helper method?
Done
// https://developer.mozilla.org/en-US/docs/Glossary/Forbidden_request_headernit:expand to note that this should still succeed, and silently drop the header
Done
instead of this, could we use different hosts or paths for the user script and content script? e.g., inject the content script on content-scripts.test and the user script on user-scripts.test, and then we only ever inject one of them.
split into separate .html files
// request header, return. The user agent may choose to skip this step andI'll leave it for blink owners to decide, but this hasn't _yet_ been accepted into the fetch spec.
I'd put it as a non-quoted comment below.
- Devlin Cronin
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Devlin Cronin (Gerrit)
Devlin Cronin voted and added 4 comments![]( "Open in Gerrit")
Votes added by Devlin Cronin
| Code-Review | +1 |
4 comments
// Confirm if headers that are forbidden are allowed on a POST request.(based on feature state)
// custom header when it normally calculates the Origin for POST.nit: throughout: I don't think I'd call this a "custom header", per se -- rather, it's a set value for the Origin header. Custom headers usually refer to non-specified headers, like MyHeaderThatICreated.
chrome.test.succeed();nit: if we use chrome.test.succeed(), we should wrap this in a runTests() -- even if it just has one test in it. That will also ensure we send a failure if there's an exception thrown.
Related details
- Justin Lulejian
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Lulejian (Gerrit)
Justin Lulejian voted and added 4 comments![]( "Open in Gerrit")
Votes added by Justin Lulejian
| Auto-Submit | +1 |
4 comments
Hi Dave! Could you review the third_party/blink/.* changes please?
Hi Kenichi! Would you be able to review the services/network/.* changes please? If not could you add the most appropriate reviewer?
For reference, here is the W3C proposal for this: https://docs.google.com/document/d/1R3ndiV4iMu2QaqbbkFbP9GO27oKi8gXmmfrSU4HUMj0/edit?usp=sharing
// Confirm if headers that are forbidden are allowed on a POST request.Justin Lulejian(based on feature state)
Done here and elsewhere.
// custom header when it normally calculates the Origin for POST.nit: throughout: I don't think I'd call this a "custom header", per se -- rather, it's a set value for the Origin header. Custom headers usually refer to non-specified headers, like MyHeaderThatICreated.
Done
nit: if we use chrome.test.succeed(), we should wrap this in a runTests() -- even if it just has one test in it. That will also ensure we send a failure if there's an exception thrown.
Decided to switch to chrome.test.sendMessage() to make it more explicit that the test assertions aren't happening here, this is just driving the test and the C++ side of the test is doing the assertions.
Related details
- Dave Tapuska
- Kenichi Ishibashi
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kenichi Ishibashi (Gerrit)
Kenichi Ishibashi added 1 comment![]( "Open in Gerrit")
toyoshim@: Could you take a look?
I suppose this is fine, but I want to double check with toyoshim@.
Related details
- Dave Tapuska
- Justin Lulejian
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Takashi Toyoshima (Gerrit)
Takashi Toyoshima added 4 comments![]( "Open in Gerrit")
Change and direction looks good.
Just one request on code is a file location to follow the code structure and layring.
Also another nit is a request to add a clear comment about the secure design.
if (should_include_origin_header()) {Can you have a short comment on this to clarify why this is safe. e.g
```
// If the Origin header is given, check if the initiator has a permission to
// override unsafe headers for the target URL. This Allowlist is given from
// a trustworthy process per factory, and safe to trust as a secondary security
// check here in the network service.
```
bool has_custom_origin_header_with_bypass =nit: const bool
bool ShouldAllowUnsafeHeaders(If you call this only inside the network service, this doesn't need to be in the public/cpp. Probably, //services/network/cors/cors_util.cc would be a better place.
This public/cpp is used to place a C++ common code that may be also used in the browser process or blink/platform layer.
Related details
- Dave Tapuska
- Justin Lulejian
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Lulejian (Gerrit)
Justin Lulejian voted and added 4 comments![]( "Open in Gerrit")
Votes added by Justin Lulejian
| Auto-Submit | +1 |
4 comments
With Takashi added as reviewer I'll move Dave to CC to reduce reviewer load on this commit.
Can you have a short comment on this to clarify why this is safe. e.g
```
// If the Origin header is given, check if the initiator has a permission to
// override unsafe headers for the target URL. This Allowlist is given from
// a trustworthy process per factory, and safe to trust as a secondary security
// check here in the network service.
```
Done
bool has_custom_origin_header_with_bypass =Justin Lulejiannit: const bool
Done
If you call this only inside the network service, this doesn't need to be in the public/cpp. Probably, //services/network/cors/cors_util.cc would be a better place.
This public/cpp is used to place a C++ common code that may be also used in the browser process or blink/platform layer.
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| 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")
Looks like gerrit was over-eager in re-adding me -- mind pinging when this is ready for a re-stamp?