| Code-Review | +1 |
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Code-Review | +1 |
bool allow_whitespace);Are you going to add a use of this function with `allow_whitespace` set to false?
std::optional<HttpByteRange> HttpUtil::ParseFetchSingleRange(It's good to add a fuzzer when adding a new parser. This is very easy with the `FUZZ_TEST` macro: see https://chromium.googlesource.com/chromium/src/+/HEAD/testing/libfuzzer/getting_started.md
size_t pos = kBytesUnit.size();Nit: you can make a string parser more obviously correct by having a `std::string_view` variable that contains the parts of the string you haven't yet parsed, and removing chunks from the strong with the `remove_prefix()` method as you consume them. If you use high-level methods like `find_first_not_of()` you can avoid subscripting, and bounds checks can be replaced with `!remaining_input.empty()` checks. See https://source.chromium.org/chromium/chromium/src/+/main:net/third_party/uri_template/uri_template.cc;drc=362dbaf3c1048a93929fff003e784d537f3ebfbc;bpv=1;bpt=1;l=167?gsn=Expand&gs=KYTHE%3A%2F%2Fkythe%3A%2F%2Fchromium.googlesource.com%2Fcodesearch%2Fchromium%2Fsrc%2F%2Fmain%3Flang%3Dc%252B%252B%3Fpath%3Dnet%2Fthird_party%2Furi_template%2Furi_template.cc%23jarBFsW_57seV6_jeOna5zFFWAA5nzNowIAXLd5OK5Afor an example of this technique.
This parser is simple enough that you don't need to do this, unless you want to.
std::optional<HttpByteRange> candidate_range;
if (!range_start) {
candidate_range = HttpByteRange::Suffix(*range_end);
} else if (range_end) {
candidate_range = HttpByteRange::Bounded(*range_start, *range_end);
} else {
candidate_range = HttpByteRange::RightUnbounded(*range_start);
}
return candidate_range;Why not do:
```suggestion
if (!range_start) {
return HttpByteRange::Suffix(*range_end);
}
if (!range_end) {
return HttpByteRange::RightUnbounded(*range_start);
}
return HttpByteRange::Bounded(*range_start, *range_end);
```
?
"bytes=5-10 ",Also good to test that we reject multiple ranges, and that extra '=' or '-' are rejected:
```
"bytes=0-10,15-20",
"bytes==0-5",
"bytes=0--5",
"bytes="0-5-",
"bytes="-0-5",
```
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
bool allow_whitespace);Are you going to add a use of this function with `allow_whitespace` set to false?
I am planning to use allow_whitespace=false for the Fetch CORS-safelisted Range header check. The Fetch spec explicitly defines parsing a single range header with an allowWhitespace boolean ("To parse a single range header value from a byte sequence value and a boolean allowWhitespace, run these steps:"), so this parser is intended to be reused for that validation.
Today, the CORS Range check in services/network/public/cpp/cors/cors.cc is implemented on top of the lenient ParseRangeHeader, with additional validation for LWS/comma handling, single-range enforcement, and suffix-range exclusion. The strict parser is intended to replace this hand-rolled validation.
Since this affects observable CORS behavior, I'd prefer to make that migration in a dedicated follow-up CL with appropriate CORS/WPT coverage rather than expand the scope of this parser CL.
Spec links:
CORS-safelisted request-header (the range case, allowWhitespace = false):
https://fetch.spec.whatwg.org/#cors-safelisted-request-header
The shared parser this CL implements (parse a single range header value,
takes an allowWhitespace boolean):
https://fetch.spec.whatwg.org/#simple-range-header-value
std::optional<HttpByteRange> HttpUtil::ParseFetchSingleRange(It's good to add a fuzzer when adding a new parser. This is very easy with the `FUZZ_TEST` macro: see https://chromium.googlesource.com/chromium/src/+/HEAD/testing/libfuzzer/getting_started.md
Done. Added a `FUZZ_TEST` for `ParseFetchSingleRange` that fuzzes both the input string and the `allow_whitespace` flag, ensuring the parser never crashes. Also registered it in `net/BUILD.gn` under the `net_unittests` fuzztests list.
size_t pos = kBytesUnit.size();Nit: you can make a string parser more obviously correct by having a `std::string_view` variable that contains the parts of the string you haven't yet parsed, and removing chunks from the strong with the `remove_prefix()` method as you consume them. If you use high-level methods like `find_first_not_of()` you can avoid subscripting, and bounds checks can be replaced with `!remaining_input.empty()` checks. See https://source.chromium.org/chromium/chromium/src/+/main:net/third_party/uri_template/uri_template.cc;drc=362dbaf3c1048a93929fff003e784d537f3ebfbc;bpv=1;bpt=1;l=167?gsn=Expand&gs=KYTHE%3A%2F%2Fkythe%3A%2F%2Fchromium.googlesource.com%2Fcodesearch%2Fchromium%2Fsrc%2F%2Fmain%3Flang%3Dc%252B%252B%3Fpath%3Dnet%2Fthird_party%2Furi_template%2Furi_template.cc%23jarBFsW_57seV6_jeOna5zFFWAA5nzNowIAXLd5OK5Afor an example of this technique.
This parser is simple enough that you don't need to do this, unless you want to.
Thank you for the suggestion and for sharing the parsing pattern. I'll leave the implementation as-is for this CL, since, as you noted, the parser is simple enough that it doesn't require this approach. The current implementation keeps all accesses bounds-checked via the in_bounds() helper, and it is covered by both the unit tests and the new fuzzer, so I'd prefer to avoid refactoring the already-reviewed parsing logic for a stylistic change
std::optional<HttpByteRange> candidate_range;
if (!range_start) {
candidate_range = HttpByteRange::Suffix(*range_end);
} else if (range_end) {
candidate_range = HttpByteRange::Bounded(*range_start, *range_end);
} else {
candidate_range = HttpByteRange::RightUnbounded(*range_start);
}
return candidate_range;Why not do:
```suggestion
if (!range_start) {
return HttpByteRange::Suffix(*range_end);
}
if (!range_end) {
return HttpByteRange::RightUnbounded(*range_start);
}
return HttpByteRange::Bounded(*range_start, *range_end);
```
?
Good suggestion. Updated to use early returns and dropped the temporary `std::optional`.
"bytes=5-10 ",Also good to test that we reject multiple ranges, and that extra '=' or '-' are rejected:
```
"bytes=0-10,15-20",
"bytes==0-5",
"bytes=0--5",
"bytes="0-5-",
"bytes="-0-5",
```
Done. Added test cases for multiple ranges (`bytes=0-10,15-20`) and invalid extra `=`/`-` delimiters (`bytes==0-5`, `bytes=0--5`, `bytes=0-5-`, and `bytes=-0-5`).
| Code-Review | +1 |
third_party/blink/renderer/platform/runtime_enabled_features.json5 LGTM
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
I just noticed there's 500 people in CC? That doesn't seem right... did an agent do that?
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
I just noticed there's 500 people in CC? That doesn't seem right... did an agent do that?
Yes, Mike. That happened during a rebase. I noticed it afterward and tried to clean it up, but I couldn't find an option to remove all of the CCs at once.
I'll take extra care going forward to ensure it doesn't happen again.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
// Kill switch for Fetch-compliant Range header validation on blob: URLs.Sorry, I mistakenly assumed the cost was in Blink. Since the feature is not in Blink, it is wasteful (and an encapsulation violation) to define it here, and you should define i in storage/browser/blob/features.h instead.
// Kill switch for Fetch-compliant Range header validation on blob: URLs.Sorry, I mistakenly assumed the cost was in Blink. Since the feature is not in Blink, it is wasteful (and an encapsulation violation) to define it here, and you should define i in storage/browser/blob/features.h instead.
Done, I've updated it. Thanks for the clarification, and sorry for the oversight. I'm still getting familiar with the repository structure, and I'll make sure to pay closer attention to this in the future.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |