Add WebTransport headers and responseHeaders support [chromium/src : main]
Adam Rice (Gerrit)
Adam Rice added 1 comment![]( "Open in Gerrit")
Sorry for the delay. It looks like you have a merge conflict, can you rebase against the latest origin/main?
Related details
- Aran Donohue
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| 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")
- Aran Donohue
- Victor Vasiliev
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Adam Rice (Gerrit)
Adam Rice added 5 comments![]( "Open in Gerrit")
Very nice implementation. I will lgtm once the response headers are safely filtered.
It would be nice to have some web platform tests for this feature. Do you have time to add any?
const std::vector<mojom::HttpRequestHeaderKeyValuePairPtr>&If you receive `additional_headers` by value here, then you can steal the contents of the strings when constructing `params.additional_headers` and avoid some copies and allocations.
for (const auto& header : additional_headers) {
params.additional_headers.emplace_back(header->key, header->value);
}Nit: this is more efficient:
```suggestion
params.additional_headers = base::ToVector(
additional_headers,
[] (const auto& header) -> std::pair<std::string, std::string> {
return {header->key, header->value};
});
```
response_header_pairs.push_back(The unfiltered response headers must not be exposed to the render process. As described in 14.2. of "Main fetch" (https://fetch.spec.whatwg.org/#main-fetch), they must be filtered by the rules of a "basic filtered response", meaning removing any headers in the "forbidden response-header name" list (https://fetch.spec.whatwg.org/#forbidden-response-header-name). In effect this means you need to filter out "Set-Cookie" and "Set-Cookie2" here.
[RuntimeEnabled=WebTransportHeaders, SameObject]This use of `[SameObject]` is incorrect, because the value changes from `null` to a different value when connection happens. The standard is wrong here, but please remove it in this CL as it risks causing buggy behaviour.
Related details
- Aran Donohue
- Victor Vasiliev
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. |
Adam Rice (Gerrit)
Adam Rice added 1 comment![]( "Open in Gerrit")
}It would be good to check that no forbidden headers have been passed by the render process, and call `mojo::ReportBadMessage` if they have. This will provide protection against compromised render processes.
Victor Vasiliev (Gerrit)
Victor Vasiliev added 1 comment![]( "Open in Gerrit")
The //net/quic parts look good to me.
Related details
- Aran Donohue
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. |
Aran Donohue (Gerrit)
Aran Donohue added 6 comments![]( "Open in Gerrit")
Thank you for the review. My webkit PR for the same thing had some of the same issues, so the review was helpful there too. I think these changes cover what you asked for. If you meant were suggesting we modify upstream WPT tests, please let me know.
const std::vector<mojom::HttpRequestHeaderKeyValuePairPtr>&If you receive `additional_headers` by value here, then you can steal the contents of the strings when constructing `params.additional_headers` and avoid some copies and allocations.
Done
It would be good to check that no forbidden headers have been passed by the render process, and call `mojo::ReportBadMessage` if they have. This will provide protection against compromised render processes.
Done
for (const auto& header : additional_headers) {
params.additional_headers.emplace_back(header->key, header->value);
}Nit: this is more efficient:
```suggestion
params.additional_headers = base::ToVector(
additional_headers,
[] (const auto& header) -> std::pair<std::string, std::string> {
return {header->key, header->value};
});
```
Done
The unfiltered response headers must not be exposed to the render process. As described in 14.2. of "Main fetch" (https://fetch.spec.whatwg.org/#main-fetch), they must be filtered by the rules of a "basic filtered response", meaning removing any headers in the "forbidden response-header name" list (https://fetch.spec.whatwg.org/#forbidden-response-header-name). In effect this means you need to filter out "Set-Cookie" and "Set-Cookie2" here.
Done
This use of `[SameObject]` is incorrect, because the value changes from `null` to a different value when connection happens. The standard is wrong here, but please remove it in this CL as it risks causing buggy behaviour.
- Adam Rice
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| 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
| Code-Review | +1 |
2 comments
lgtm with nit. Thanks for the extra test coverage!
const char* ForbiddenWebTransportAdditionalHeaderReason(Nit: This should return `std::string_view` rather than `const char*`, for efficiency and memory safety.
Related details
- Aran Donohue
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. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
gwsq error while processing this CL: Error looking up group chromeos-commercial-r...@google.com: Streaming RPC error: status 6 error: ??? to (unknown) : APP_ERROR(13) com.google.net.rpc3.client.RpcClientException: <eye3 title='/RostersService.LookupMembership, DEADLINE_EXCEEDED'/> DEADLINE_EXCEEDED;groups_rosters/RostersService.LookupMembership;Server deadline (4.94639751625s) expired in Apps Framework.;StartTimeMs=1776706145049;unknown;Deadline(sec)=10.0;ResFormat=uncompressed;ServerTimeSec=4.9512268729999995;LogBytes=256;Non-FailFast;EndUserCredsRequested;EffSecLevel=none;DelegatedRole=corp-platforms-expn-server;ReqFormat=uncompressed;ReqID=90c7c8b057184a5c;GlobalID=0;Server=[2002:a05:6919:e604:b0:41d:c0f3:8544]:4004
Suppressed: com.google.common.labs.concurrent.LabsFutures$LabeledExecutionException: GraphFuture{key=@com.google.apps.framework.producers.PrivateVisibility(module=com.google.ccc.groups.rosters.services.emailsettingsservice.actions.lookupdeliverysettings.LookupDeliverySettingsBatchRequestProducerModule.class) com.google.ccc.groups.rosters.proto.LookupMembershipBatchResponse} failed: com.google.net.rpc3.client.RpcClientException: <eye3 title='/RostersService.LookupMembership, DEADLINE_EXCEEDED'/> DEADLINE_EXCEEDED;groups_rosters/RostersService.LookupMembership;Server deadline (4.94639751625s) expired in Apps Framework.;StartTimeMs=1776706145049;unknown;Deadline(sec)=10.0;ResFormat=uncompressed;ServerTimeSec=4.9512268729999995;LogBytes=256;Non-FailFast;EndUserCredsRequested;EffSecLevel=none;DelegatedRole=corp-platforms-expn-server;ReqFormat=uncompressed;ReqID=90c7c8b057184a5c;GlobalID=0;Server=[2002:a05:6919:e604:b0:41d:c0f3:8544]:4004
at com.google.apps.framework.producers.PresentImpl.get(PresentImpl.java:32)
at com.google.ccc.groups.rosters.services.emailsettingsservice.actions.lookupdeliverysettings.LookupDeliverySettingsBatchRequestProducerModule.getMembershipResponses(LookupDeliverySettingsBatchRequestProducerModule.java:190)
at com.google.ccc.groups.rosters.services.emailsettingsservice.actions.lookupdeliverysettings.LookupDeliverySettingsBatchRequestProducerModule.dispatchRequests(LookupDeliverySettingsBatchRequestProducerModule.java:56)
Suppressed: CriticalInputFailure: com.google.ccc.groups.rosters.services.emailsettingsservice.actions.lookupdeliverysettings.LookupDeliverySettingsBatchRequestProducerModule.gatherResults failed while trying to inject @com.google.apps.framework.producers.PrivateVisibility(module=com.google.ccc.groups.rosters.services.emailsettingsservice.actions.lookupdeliverysettings.LookupDeliverySettingsBatchRequestProducerModule.class) java.util.List<com.google.apps.framework.producers.Present<com.google.ccc.groups.rosters.proto.LookupDeliverySettingsBatchResponse$Response>>
Suppressed: com.google.common.labs.concurrent.LabsFutures$LabeledExecutionException: GraphFuture{key=@com.google.ccc.groups.rosters.services.emailsettingsservice.actions.lookupdeliverysettings.Annotations$LookupDeliverySettingsScope com.google.ccc.groups.rosters.proto.LookupDeliverySettingsBatchResponse} failed: com.google.net.rpc3.client.RpcClientException: <eye3 title='/RostersService.LookupMembership, DEADLINE_EXCEEDED'/> DEADLINE_EXCEEDED;groups_rosters/RostersService.LookupMembership;Server deadline (4.94639751625s) expired in Apps Framework.;StartTimeMs=1776706145049;unknown;Deadline(sec)=10.0;ResFormat=uncompressed;ServerTimeSec=4.9512268729999995;LogBytes=256;Non-FailFast;EndUserCredsRequested;EffSecLevel=none;DelegatedRole=corp-platforms-expn-server;ReqFormat=uncompressed;ReqID=90c7c8b057184a5c;GlobalID=0;Server=[2002:a05:6919:e604:b0:41d:c0f3:8544]:4004
com.google.net.rpc3.client.RpcClientException: <eye3 title='/RostersService.LookupMembership, DEADLINE_EXCEEDED'/> DEADLINE_EXCEEDED;groups_rosters/RostersService.LookupMembership;Server deadline (4.94639751625s) expired in Apps Framework.;StartTimeMs=1776706145049;unknown;Deadline(sec)=10.0;ResFormat=uncompressed;ServerTimeSec=4.9512268729999995;LogBytes=256;Non-FailFast;EndUserCredsRequested;EffSecLevel=none;DelegatedRole=corp-platforms-expn-server;ReqFormat=uncompressed;ReqID=90c7c8b057184a5c;GlobalID=0;Server=[2002:a05:6919:e604:b0:41d:c0f3:8544]:4004
Suppressed: com.google.common.labs.concurrent.LabsFutures$LabeledExecutionException: GraphFuture{key=@com.google.apps.framework.producers.PrivateVisibility(module=com.google.ccc.groups.rosters.services.emailsettingsservice.actions.lookupdeliverysettings.LookupDeliverySettingsBatchRequestProducerModule.class) com.google.ccc.groups.rosters.proto.LookupMembershipBatchResponse} failed: com.google.net.rpc3.client.RpcClientException: <eye3 title='/RostersService.LookupMembership, DEADLINE_EXCEEDED'/> DEADLINE_EXCEEDED;groups_rosters/RostersService.LookupMembership;Server deadline (4.94639751625s) expired in Apps Framework.;StartTimeMs=1776706145049;unknown;Deadline(sec)=10.0;ResFormat=uncompressed;ServerTimeSec=4.9512268729999995;LogBytes=256;Non-FailFast;EndUserCredsRequested;EffSecLevel=none;DelegatedRole=corp-platforms-expn-server;ReqFormat=uncompressed;ReqID=90c7c8b057184a5c;GlobalID=0;Server=[2002:a05:6919:e604:b0:41d:c0f3:8544]:4004
at com.google.apps.framework.producers.PresentImpl.get(PresentImpl.java:32)
at com.google.ccc.groups.rosters.services.emailsettingsservice.actions.lookupdeliverysettings.LookupDeliverySettingsBatchRequestProducerModule.getMembershipResponses(LookupDeliverySettingsBatchRequestProducerModule.java:190)
at com.google.ccc.groups.rosters.services.emailsettingsservice.actions.lookupdeliverysettings.LookupDeliverySettingsBatchRequestProducerModule.dispatchRequests(LookupDeliverySettingsBatchRequestProducerModule.java:56)
Suppressed: CriticalInputFailure: com.google.ccc.groups.rosters.services.emailsettingsservice.actions.lookupdeliverysettings.LookupDeliverySettingsBatchRequestProducerModule.gatherResults failed while trying to inject @com.google.apps.framework.producers.PrivateVisibility(module=com.google.ccc.groups.rosters.services.emailsettingsservice.actions.lookupdeliverysettings.LookupDeliverySettingsBatchRequestProducerModule.class) java.util.List<com.google.apps.framework.producers.Present<com.google.ccc.groups.rosters.proto.LookupDeliverySettingsBatchResponse$Response>>
Suppressed: com.google.common.labs.concurrent.LabsFutures$LabeledExecutionException: GraphFuture{key=@com.google.ccc.groups.rosters.services.emailsettingsservice.actions.lookupdeliverysettings.Annotations$LookupDeliverySettingsScope com.google.ccc.groups.rosters.proto.LookupDeliverySettingsBatchResponse} failed: com.google.net.rpc3.client.RpcClientException: <eye3 title='/RostersService.LookupMembership, DEADLINE_EXCEEDED'/> DEADLINE_EXCEEDED;groups_rosters/RostersService.LookupMembership;Server deadline (4.94639751625s) expired in Apps Framework.;StartTimeMs=1776706145049;unknown;Deadline(sec)=10.0;ResFormat=uncompressed;ServerTimeSec=4.9512268729999995;LogBytes=256;Non-FailFast;EndUserCredsRequested;EffSecLevel=none;DelegatedRole=corp-platforms-expn-server;ReqFormat=uncompressed;ReqID=90c7c8b057184a5c;GlobalID=0;Server=[2002:a05:6919:e604:b0:41d:c0f3:8544]:4004
Related details
- Aran Donohue
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. |
Aran Donohue (Gerrit)
Aran Donohue added 2 comments![]( "Open in Gerrit")
Rebased and addressed the nit.
It looks like this needs two Code Review votes since I'm not a maintainer - any chance you could suggest a second voting reviewer @ri...@chromium.org? It also looks like updating and rebasing discarded the previous vote.
Nit: This should return `std::string_view` rather than `const char*`, for efficiency and memory safety.
- Adam Rice
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aran Donohue (Gerrit)
Aran Donohue added 1 comment![]( "Open in Gerrit")
- Adam Rice
- Chromium IPC Reviews
- Kelvin Jiang
- Mason Freed
- Robert Flack
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
From googleclient/chrome/chromium_gwsq/ipc/config.gwsq:
IPC: tse...@chromium.org
📎 It looks like you’re making a possibly security-sensitive change! 📎 IPC security review isn’t a rubberstamp, so your friendly security reviewer will need a fair amount of context to review your CL effectively. Please review your CL description and code comments to make sure they provide context for someone unfamiliar with your project/area. Pay special attention to where data comes from and which processes it flows between (and their privilege levels). Feel free to point your security reviewer at design docs, bugs, or other links if you can’t reasonably make a self-contained CL description. (Also see https://cbea.ms/git-commit/).
IPC reviewer(s): tse...@chromium.org
Reviewer source(s):
tse...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
- Adam Rice
- Kelvin Jiang
- Mason Freed
- Robert Flack
- Tom Sepez
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| 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 |
Thank you for the review. My webkit PR for the same thing had some of the same issues, so the review was helpful there too. I think these changes cover what you asked for. If you meant were suggesting we modify upstream WPT tests, please let me know.
You can actually modify WPT tests directly with Chromium repository CLs, and the change will be automatically upstreamed if it lands here and passes tests. Or you can just work upstream. Anyway, it's purely optional.
Related details
- Aran Donohue
Robert Flack (Gerrit)
Robert Flack voted and added 1 comment![]( "Open in Gerrit")
Votes added by Robert Flack
| Code-Review | +1 |
1 comment
runtime_enabled_features.json5 and VirtualTestSuites lgtm
Related details
- Aran Donohue
- Kelvin Jiang
- Mason Freed
- Tom Sepez
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed added 1 comment![]( "Open in Gerrit")
Hi, I'm not sure what files you've added me to review - can you clarify?
Related details
- Aran Donohue
- Kelvin Jiang
- Tom Sepez
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. |
Aran Donohue (Gerrit)
Aran Donohue added 1 comment![]( "Open in Gerrit")
Hi, I'm not sure what files you've added me to review - can you clarify?
- Kelvin Jiang
- Tom Sepez
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kelvin Jiang (Gerrit)
Kelvin Jiang voted and added 1 comment![]( "Open in Gerrit")
Votes added by Kelvin Jiang
| Code-Review | +1 |
1 comment
- Aran Donohue
- Tom Sepez
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| 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")
- Aran Donohue
- Kelvin Jiang
- Robert Flack
- Tom Sepez
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Robert Flack (Gerrit)
Robert Flack voted Code-Review+1![]( "Open in Gerrit")
- Aran Donohue
- Kelvin Jiang
- Tom Sepez
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aran Donohue (Gerrit)
Aran Donohue added 1 comment![]( "Open in Gerrit")
I would love to make this happen, but not sure the best things I can do to help move it forward. I see @tse...@chromium.org is OOO, is there someone else who could take a look or is it better to wait?
Related details
- Chromium IPC Reviews
- Kelvin Jiang
- Tom Sepez
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
From googleclient/chrome/chromium_gwsq/ipc/config.gwsq:
IPC: ort...@chromium.org, tse...@chromium.org
📎 It looks like you’re making a possibly security-sensitive change! 📎 IPC security review isn’t a rubberstamp, so your friendly security reviewer will need a fair amount of context to review your CL effectively. Please review your CL description and code comments to make sure they provide context for someone unfamiliar with your project/area. Pay special attention to where data comes from and which processes it flows between (and their privilege levels). Feel free to point your security reviewer at design docs, bugs, or other links if you can’t reasonably make a self-contained CL description. (Also see https://cbea.ms/git-commit/).
IPC reviewer(s): ort...@chromium.org, tse...@chromium.org
Reviewer source(s):
ort...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
- Giovanni Ortuno Urquidi
- Kelvin Jiang
- Tom Sepez
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kelvin Jiang (Gerrit)
Kelvin Jiang voted and added 1 comment![]( "Open in Gerrit")
Votes added by Kelvin Jiang
| Code-Review | +1 |
1 comment
- Aran Donohue
- Giovanni Ortuno Urquidi
- Tom Sepez
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Giovanni Ortuno Urquidi (Gerrit)
Giovanni Ortuno Urquidi added 2 comments![]( "Open in Gerrit")
array<HttpRequestHeaderKeyValuePair> additional_headers,Any reason why we use HttpRequestHeaderKeyValuePair instead of [HttpRequestHeaders](https://source.chromium.org/chromium/chromium/src/+/main:services/network/public/mojom/http_request_headers.mojom;l=20;drc=cca43644a6b893c7bebe5f8a18e9ebe2c71fcdc3)? You could also create a WebTransportHttpRequestHeaders and add a typemap that validates it has the right headers.
array<HttpRawHeaderPair> response_header_pairs,Similar question here, why do we use the raw request headers. Above we use the typemapped HttpResponseHeaders.
Related details
- Aran Donohue
- Tom Sepez
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. |
Aran Donohue (Gerrit)
Aran Donohue added 2 comments![]( "Open in Gerrit")
array<HttpRequestHeaderKeyValuePair> additional_headers,Any reason why we use HttpRequestHeaderKeyValuePair instead of [HttpRequestHeaders](https://source.chromium.org/chromium/chromium/src/+/main:services/network/public/mojom/http_request_headers.mojom;l=20;drc=cca43644a6b893c7bebe5f8a18e9ebe2c71fcdc3)? You could also create a WebTransportHttpRequestHeaders and add a typemap that validates it has the right headers.
Hi @ort...@chromium.org, thanks for the review.
Yes, the reason is to avoid the typemap deserializer for HttpRequestHeaders, which lets us then delegate handling for the headers multimap behavior to Quiche.
I agree a WebTransportHttpRequestHeaders can work. Please let me know if you'd like me to implement WebTransportHttpRequestHeaders as part of this.
array<HttpRawHeaderPair> response_header_pairs,Similar question here, why do we use the raw request headers. Above we use the typemapped HttpResponseHeaders.
For this one, I should be able to get rid of it. We are using it to allow processing headers without touching the original, but I don't think the original is used anywhere where it strictly needs to remain untouched. A side effect will be this will allow extensions to modify headers. Updated patchset on the way
Related details
- Giovanni Ortuno Urquidi
- Tom Sepez
Aran Donohue (Gerrit)
Aran Donohue added 1 comment![]( "Open in Gerrit")
array<HttpRawHeaderPair> response_header_pairs,Aran DonohueSimilar question here, why do we use the raw request headers. Above we use the typemapped HttpResponseHeaders.
For this one, I should be able to get rid of it. We are using it to allow processing headers without touching the original, but I don't think the original is used anywhere where it strictly needs to remain untouched. A side effect will be this will allow extensions to modify headers. Updated patchset on the way
- Adam Rice
- Giovanni Ortuno Urquidi
- Kelvin Jiang
- Robert Flack
- Tom Sepez
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. |
Giovanni Ortuno Urquidi (Gerrit)
Giovanni Ortuno Urquidi added 1 comment![]( "Open in Gerrit")
array<HttpRequestHeaderKeyValuePair> additional_headers,Aran DonohueAny reason why we use HttpRequestHeaderKeyValuePair instead of [HttpRequestHeaders](https://source.chromium.org/chromium/chromium/src/+/main:services/network/public/mojom/http_request_headers.mojom;l=20;drc=cca43644a6b893c7bebe5f8a18e9ebe2c71fcdc3)? You could also create a WebTransportHttpRequestHeaders and add a typemap that validates it has the right headers.
Hi @ort...@chromium.org, thanks for the review.
Yes, the reason is to avoid the typemap deserializer for HttpRequestHeaders, which lets us then delegate handling for the headers multimap behavior to Quiche.
I agree a WebTransportHttpRequestHeaders can work. Please let me know if you'd like me to implement WebTransportHttpRequestHeaders as part of this.
What part of the deserialization is problematic? Looking at the [traits](https://source.chromium.org/chromium/chromium/src/+/main:services/network/public/cpp/http_request_headers_mojom_traits.cc;l=14), the code in ForbiddenWebTransportAdditionalHeaderReason does the same checks.
Related details
- Adam Rice
- Aran Donohue
- Kelvin Jiang
- Robert Flack
- Tom Sepez
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. |
Aran Donohue (Gerrit)
Aran Donohue added 1 comment![]( "Open in Gerrit")
array<HttpRequestHeaderKeyValuePair> additional_headers,Aran DonohueAny reason why we use HttpRequestHeaderKeyValuePair instead of [HttpRequestHeaders](https://source.chromium.org/chromium/chromium/src/+/main:services/network/public/mojom/http_request_headers.mojom;l=20;drc=cca43644a6b893c7bebe5f8a18e9ebe2c71fcdc3)? You could also create a WebTransportHttpRequestHeaders and add a typemap that validates it has the right headers.
Giovanni Ortuno UrquidiHi @ort...@chromium.org, thanks for the review.
Yes, the reason is to avoid the typemap deserializer for HttpRequestHeaders, which lets us then delegate handling for the headers multimap behavior to Quiche.
I agree a WebTransportHttpRequestHeaders can work. Please let me know if you'd like me to implement WebTransportHttpRequestHeaders as part of this.
What part of the deserialization is problematic? Looking at the [traits](https://source.chromium.org/chromium/chromium/src/+/main:services/network/public/cpp/http_request_headers_mojom_traits.cc;l=14), the code in ForbiddenWebTransportAdditionalHeaderReason does the same checks.
I could be mistaken, but I think HttpRequestHeaders treats the headers as a map not a multimap. It discards duplicate keys, keeping the last. WebTransport (by delegation to Fetch and http specs) allows duplicate keys.
Here is the relevant code:
Calls SetHeader for every pair: https://source.chromium.org/chromium/chromium/src/+/main:services/network/public/cpp/http_request_headers_mojom_traits.cc;l=46
Delegates to SetHeaderInternal: https://source.chromium.org/chromium/chromium/src/+/main:net/http/http_request_headers.cc;l=108
SetHeaderInternal uses FindHeader to check for existing header with that key: https://source.chromium.org/chromium/chromium/src/+/main:net/http/http_request_headers.cc;l=303
FindHeader returns iterator into the vector: https://source.chromium.org/chromium/chromium/src/+/main:net/http/http_request_headers.cc;l=283
It's a vector with no multiple value support in each entry: https://source.chromium.org/chromium/chromium/src/+/main:net/http/http_request_headers.h;l=45
If I'm reading it right, if we were to use HttpRequestHeaders, in the case of headers with duplicate keys later keys would overwrite earlier keys, which would violate the spec. By avoiding HttpRequestHeaders, we let things flow through until quiche handles the headers.
Please let me know if you'd like me to change how this behaves or how it's written.
Related details
- Adam Rice
- Giovanni Ortuno Urquidi
Giovanni Ortuno Urquidi (Gerrit)
Giovanni Ortuno Urquidi added 1 comment![]( "Open in Gerrit")
array<HttpRequestHeaderKeyValuePair> additional_headers,Aran DonohueAny reason why we use HttpRequestHeaderKeyValuePair instead of [HttpRequestHeaders](https://source.chromium.org/chromium/chromium/src/+/main:services/network/public/mojom/http_request_headers.mojom;l=20;drc=cca43644a6b893c7bebe5f8a18e9ebe2c71fcdc3)? You could also create a WebTransportHttpRequestHeaders and add a typemap that validates it has the right headers.
Giovanni Ortuno UrquidiHi @ort...@chromium.org, thanks for the review.
Yes, the reason is to avoid the typemap deserializer for HttpRequestHeaders, which lets us then delegate handling for the headers multimap behavior to Quiche.
I agree a WebTransportHttpRequestHeaders can work. Please let me know if you'd like me to implement WebTransportHttpRequestHeaders as part of this.
Aran DonohueWhat part of the deserialization is problematic? Looking at the [traits](https://source.chromium.org/chromium/chromium/src/+/main:services/network/public/cpp/http_request_headers_mojom_traits.cc;l=14), the code in ForbiddenWebTransportAdditionalHeaderReason does the same checks.
I could be mistaken, but I think HttpRequestHeaders treats the headers as a map not a multimap. It discards duplicate keys, keeping the last. WebTransport (by delegation to Fetch and http specs) allows duplicate keys.
Here is the relevant code:
Calls SetHeader for every pair: https://source.chromium.org/chromium/chromium/src/+/main:services/network/public/cpp/http_request_headers_mojom_traits.cc;l=46
Delegates to SetHeaderInternal: https://source.chromium.org/chromium/chromium/src/+/main:net/http/http_request_headers.cc;l=108
SetHeaderInternal uses FindHeader to check for existing header with that key: https://source.chromium.org/chromium/chromium/src/+/main:net/http/http_request_headers.cc;l=303
FindHeader returns iterator into the vector: https://source.chromium.org/chromium/chromium/src/+/main:net/http/http_request_headers.cc;l=283
It's a vector with no multiple value support in each entry: https://source.chromium.org/chromium/chromium/src/+/main:net/http/http_request_headers.h;l=45
If I'm reading it right, if we were to use HttpRequestHeaders, in the case of headers with duplicate keys later keys would overwrite earlier keys, which would violate the spec. By avoiding HttpRequestHeaders, we let things flow through until quiche handles the headers.
Please let me know if you'd like me to change how this behaves or how it's written.
Ah! I missed the case of multiple headers with the same key. In that case, can we add a WebTransportHttpRequestHeaders struct that does the validation?
Related details
- Adam Rice
- Aran Donohue