[DNR] Support append operation in modifyHeader rules for request headers [chromium/src : main]
Kelvin Jiang (Gerrit)
Attention is currently required from: Devlin Cronin.
Kelvin Jiang would like Devlin Cronin to review this change.
[DNR] Support append operation in modifyHeader rules for request headers
This CL allows modifyHeaders rules to append values onto request
headers. Since request headers do not really support multiple entries,
appends will add a delimiter and the specified value onto the end of a
header.
E.g. If a rule wishes to append "en-us" to the access-language header,
then: "access-language: en-ca" becomes "access-language: en-ca, en-us".
Headers that are eligible to be appended are standard HTTP headers
that can support multiple values. These headers, along with their
delimiters, are stored in an internal allowlist inside:
e/b/api/declarative_net_request/constants.h
Also of note: for request headers, WebRequest only tracks which
headers are removed, and which are modified by actions. DNR header
appends for request headers are treated as a modification, and
appended headers cannot be modified by webrequest actions.
Bug: 1355626
Change-Id: Ie5ef2bae40e7c3d1d04f284775d9a9c1044790ea
---
M chrome/browser/extensions/api/web_request/web_request_api_unittest.cc
M chrome/test/data/extensions/api_test/declarative_net_request/modify_headers/background.js
M extensions/browser/api/declarative_net_request/constants.cc
M extensions/browser/api/declarative_net_request/constants.h
M extensions/browser/api/declarative_net_request/indexed_rule.cc
M extensions/browser/api/declarative_net_request/indexed_rule_unittest.cc
M extensions/browser/api/declarative_net_request/test_utils.cc
M extensions/browser/api/declarative_net_request/utils.cc
M extensions/browser/api/web_request/web_request_api_helpers.cc
9 files changed, 295 insertions(+), 63 deletions(-)
To view, visit change 3922770. To unsubscribe, or for help writing mail filters, visit settings.
Kelvin Jiang (Gerrit)
Attention is currently required from: Devlin Cronin.
Patch set 8:Commit-Queue +1
Devlin Cronin (Gerrit)
Attention is currently required from: Kelvin Jiang.
14 comments:
Patchset:
Thanks, Kelvin! A few questions here
Thanks, Kelvin! A few questions here
File chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:
Patch Set #8, Line 973:
// as set operations and will conflict with WebRequest modifications.Conflict in which way? (expand the comment)
Patch Set #8, Line 1011:
TestMergeOnBeforeSendHeadersResponses_DeclarativeNetRequest) {This is a 170 line long unit test. That's pretty big. It also looks like many of these are testing distinct behaviors (e.g., multiple appends, appends between extensions, sets between extensions, etc). Would it make sense to split these into a few more tests? Even if it's more LOC overall, I think it would make it quite a bit more readable.
Patch Set #8, Line 1022:
action_1.request_headers_to_modify = {If we keep this monolithic test, add a comment about what these blocks are doing
{"connection", "dnr_action_1"},
{"forwarded", "dnr_action_1"},These two don't seem related to the "// Multiple append actions can apply to the same header." comment
File extensions/browser/api/declarative_net_request/constants.h:
Patch Set #8, Line 246:
// An allowlist of request headers that can be appended onto. This is in theComment where this list came from, and when it should be extended
File extensions/browser/api/declarative_net_request/constants.cc:
Patch Set #8, Line 75:
"Rule with id * specifies an invalid request header to be appended.";Can we indicate which request headers are supported? e.g. that only standardized headers are supported?
File extensions/browser/api/declarative_net_request/indexed_rule.cc:
const auto* it =
kDNRRequestHeaderAppendAllowList.find(header_info.header);
if (it == kDNRRequestHeaderAppendAllowList.end())if (!base::Contains(kDNRRequestHeaderAppendAllowList, header_info.header))
File extensions/browser/api/declarative_net_request/utils.cc:
Patch Set #8, Line 441:
ctbugcrbug
File extensions/browser/api/web_request/web_request_api_helpers.cc:
Patch Set #8, Line 336:
std::string GetDNRNewRequestHeaderValue(net::HttpRequestHeaders* headers,Add a function comment
since the contents of |header_actions| for a given
// header will always be one of:Why does this mean that checking the first is sufficient? (Also, it seems like we *should* support multiple appends from different extensions — why don't wee?)
(*header_actions)[header][0].ConflictsWithSubsequentAction(
header_actionisn't this the same as (*iter)[0].Conflicts...()? Though it might be cleaner to pull the underlying type from the iter into a temporary var
// Record only the first time a header is changed by a DNR action. Each
// header should only contribute one count into the histogram as the
// count represents the total number of headers that have been changed
// by DNR actions.Is this a behavior change from before?
To view, visit change 3922770. To unsubscribe, or for help writing mail filters, visit settings.
Kelvin Jiang (Gerrit)
Attention is currently required from: Devlin Cronin.
Patch set 9:Commit-Queue +1
12 comments:
File chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:
Patch Set #8, Line 973:
// as set operations and will conflict with WebRequest modifications.Conflict in which way? (expand the comment)
conflict: both can't happen as two "set" operations which imply overriding the value of a header value cannot both be applied semantically
here, set = "set header h with value v to <v><appended part>"
Patch Set #8, Line 1011:
TestMergeOnBeforeSendHeadersResponses_DeclarativeNetRequest) {
This is a 170 line long unit test. That's pretty big. […]
will do... gonna separate into which operation types will be applied first
Patch Set #8, Line 1022:
action_1.request_headers_to_modify = {If we keep this monolithic test, add a comment about what these blocks are doing
as mentioned above, gonna split these tests
{"connection", "dnr_action_1"},
{"forwarded", "dnr_action_1"},
These two don't seem related to the "// Multiple append actions can apply to the same header. […]
ack, gonna specify that this comment is only applicable for the accept header and that if a lower priority action of set/remove follows the append, then the second action will not be applied
File extensions/browser/api/declarative_net_request/constants.h:
Patch Set #8, Line 246:
// An allowlist of request headers that can be appended onto. This is in theComment where this list came from, and when it should be extended
Done
File extensions/browser/api/declarative_net_request/constants.cc:
Patch Set #8, Line 75:
"Rule with id * specifies an invalid request header to be appended.";
Can we indicate which request headers are supported? e.g. […]
Done
File extensions/browser/api/declarative_net_request/indexed_rule.cc:
const auto* it =
kDNRRequestHeaderAppendAllowList.find(header_info.header);
if (it == kDNRRequestHeaderAppendAllowList.end())
if (!base::Contains(kDNRRequestHeaderAppendAllowList, header_info. […]
done (I like how this can still fit in one line)
File extensions/browser/api/declarative_net_request/utils.cc:
Patch Set #8, Line 441:
ctbugcrbug
Done
File extensions/browser/api/web_request/web_request_api_helpers.cc:
Patch Set #8, Line 336:
std::string GetDNRNewRequestHeaderValue(net::HttpRequestHeaders* headers,Add a function comment
Done
since the contents of |header_actions| for a given
// header will always be one of:
Why does this mean that checking the first is sufficient? (Also, it seems like we *should* support […]
^we do support multiple appends from different extensions, see `TestMergeOnBeforeSendHeadersResponses_DeclarativeNetRequest` for the "accept" header (note the `extension_id` for the actions in that test are different)
(*header_actions)[header][0].ConflictsWithSubsequentAction(
header_action
isn't this the same as (*iter)[0].Conflicts... […]
done (changed to `iter->second[0].Conflicts...` )
// Record only the first time a header is changed by a DNR action. Each
// header should only contribute one count into the histogram as the
// count represents the total number of headers that have been changed
// by DNR actions.Is this a behavior change from before?
This is not a behavior change, because previously, when only set/remove were supported, only one of those actions can be applied on a header and other actions for that header are ignored. Here, we enforce that only the first applied action that changes the header will be allowed to be recorded on the UMA since multiple append actions can be applied on a single header.
However, since at most one set/remove action can be applied on a header, there should be no behavior changes wrt logging to UMAs.
I also added a short comment explaining why we check for size() == 1 since it wasn't immediately obvious when I took another look
To view, visit change 3922770. To unsubscribe, or for help writing mail filters, visit settings.
Devlin Cronin (Gerrit)
Attention is currently required from: Kelvin Jiang.
Patch set 9:Code-Review +1
9 comments:
Patchset:
Thanks, Kelvin! LGTM % nits.
File chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:
Patch Set #9, Line 1022:
test_casesnit: these aren't really test cases — they're expected headers for a single case. Maybe rename to `expected_headers`?
Patch Set #9, Line 1022:
std::vector<ExpectedHeader> test_cases) {nit: const &
Patch Set #9, Line 1025:
bool request_headers_modified;nit: initialize to a sane value
Patch Set #9, Line 1026:
std::set<std::string> ignore1, ignore2;nit1: I think chromium style still requires variables on separate lines
nit2: expand the name to make it clear which type we're ignoring : )nit: rm \n
File extensions/browser/api/declarative_net_request/constants.h:
Patch Set #9, Line 266:
{"trailer", ""},what does an empty delimeter mean?
File extensions/browser/api/web_request/web_request_api_helpers.cc:
since the contents of |header_actions| for a given
// header will always be one of:
Where / how is this enforced? (Add a comment)
Patch Set #9, Line 420:
(*header_actions)[header].push_back(header_action);nit: we already found this with the find() call above. Prefer something like:
auto& actions_for_header = iter->second;
And then also use this on line 438 below.
To view, visit change 3922770. To unsubscribe, or for help writing mail filters, visit settings.
Kelvin Jiang (Gerrit)
Patch set 10:Commit-Queue +2
8 comments:
File chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:
Patch Set #9, Line 1022:
test_cases
nit: these aren't really test cases — they're expected headers for a single case. […]
Done
Patch Set #9, Line 1022:
std::vector<ExpectedHeader> test_cases) {nit: const &
Done
Patch Set #9, Line 1025:
bool request_headers_modified;nit: initialize to a sane value
Done
Patch Set #9, Line 1026:
std::set<std::string> ignore1, ignore2;
nit1: I think chromium style still requires variables on separate lines […]
Done
nit: rm \n
Done
File extensions/browser/api/declarative_net_request/constants.h:
Patch Set #9, Line 266:
{"trailer", ""},what does an empty delimeter mean?
it means that for this header, values specified by rules will be appended directly onto the end of the existing header value
`<original value><appended part>`
File extensions/browser/api/web_request/web_request_api_helpers.cc:
since the contents of |header_actions| for a given
// header will always be one of:Where / how is this enforced? (Add a comment)
Done
Patch Set #9, Line 420:
(*header_actions)[header].push_back(header_action);
nit: we already found this with the find() call above. Prefer something like: […]
I don't think we did? the find() call can still return null, but calling the accessor `(*header_actions)[header]` will auto-create an entry of (header, empty vector) if no entry for the header exists iirc?
To view, visit change 3922770. To unsubscribe, or for help writing mail filters, visit settings.
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
9 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: extensions/browser/api/web_request/web_request_api_helpers.cc
Insertions: 11, Deletions: 6.
The diff is too large to show. Please review the diff.
```
```
The name of the file: chrome/browser/extensions/api/web_request/web_request_api_unittest.cc
Insertions: 24, Deletions: 22.
The diff is too large to show. Please review the diff.
```
[DNR] Support append operation in modifyHeader rules for request headers
This CL allows modifyHeaders rules to append values onto request
headers. Since request headers do not really support multiple entries,
appends will add a delimiter and the specified value onto the end of a
header.
E.g. If a rule wishes to append "en-us" to the access-language header,
then: "access-language: en-ca" becomes "access-language: en-ca, en-us".
Headers that are eligible to be appended are standard HTTP headers
that can support multiple values. These headers, along with their
delimiters, are stored in an internal allowlist inside:
e/b/api/declarative_net_request/constants.h
Also of note: for request headers, WebRequest only tracks which
headers are removed, and which are modified by actions. DNR header
appends for request headers are treated as a modification, and
appended headers cannot be modified by webrequest actions.
Bug: 1355626
Change-Id: Ie5ef2bae40e7c3d1d04f284775d9a9c1044790ea
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3922770
Reviewed-by: Devlin Cronin <rdevlin...@chromium.org>
Commit-Queue: Kelvin Jiang <kelvi...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1064697}
---
M chrome/browser/extensions/api/web_request/web_request_api_unittest.cc
M chrome/test/data/extensions/api_test/declarative_net_request/modify_headers/background.js
M extensions/browser/api/declarative_net_request/constants.cc
M extensions/browser/api/declarative_net_request/constants.h
M extensions/browser/api/declarative_net_request/indexed_rule.cc
M extensions/browser/api/declarative_net_request/indexed_rule_unittest.cc
M extensions/browser/api/declarative_net_request/test_utils.cc
M extensions/browser/api/declarative_net_request/utils.cc
M extensions/browser/api/web_request/web_request_api_helpers.cc
9 files changed, 401 insertions(+), 87 deletions(-)