[FedCM] Better parsing for Set-Login headers in IdentityUrlLoaderThrottle [chromium/src : main]
Christian Biesinger (Gerrit)
Christian Biesinger added 3 comments![]( "Open in Gerrit")
net::structured_headers::ParseItem(*header_value);Christian BiesingerSo, in general, parsing untrusted data (like random websites' headers) is only allowed in a sandboxed process. This code can run in either the browser process or a renderer process and only the latter is sandboxed.
It may be easiest to always use this method for parsing:
https://source.chromium.org/chromium/chromium/src/+/main:services/data_decoder/public/cpp/data_decoder.h;l=108?q=ParseStructuredH&ss=chromium
Acknowledged
ParseSetLoginHeaderCallback CreateParseCallback() {why can't this use content::GetSetLoginHeaderInProcessParser?
base::OnceCallback<void(std::optional<std::string> token)> callback)>;IMO this should be something like `std::optional<net::structured_headers::ParameterizedItem>` for two reasons:
- otherwise, the two parsing functions have to have identical code to get the token out of the Item, and
- if we ever extend this to make use of more parameters in this header it will be easier if the throttle class has the full item available
Related details
- Nicolás Peña
- Ryan Qin
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. |
Ryan Qin (Gerrit)
Ryan Qin added 2 comments![]( "Open in Gerrit")
ParseSetLoginHeaderCallback CreateParseCallback() {why can't this use content::GetSetLoginHeaderInProcessParser?
we can, done:)
base::OnceCallback<void(std::optional<std::string> token)> callback)>;IMO this should be something like `std::optional<net::structured_headers::ParameterizedItem>` for two reasons:
- otherwise, the two parsing functions have to have identical code to get the token out of the Item, and
- if we ever extend this to make use of more parameters in this header it will be easier if the throttle class has the full item available
- Christian Biesinger
- Nicolás Peña
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Christian Biesinger (Gerrit)
Christian Biesinger added 2 comments![]( "Open in Gerrit")
if (parse_set_login_header_cb_) {I don't think this is ever null, can this be a CHECK() instead?
}Please fix this WARNING reported by autoreview issue finding: In the browser process, GetSetLoginHeaderDataDecoderParser() returns an asynchronous parser. Since HandleResponseOrRedirect does not defer the response (by setting *defer = true), the URLLoader will continue processing the response immediately.
If the response is small (e.g., a redirect or an empty body), the URLLoader and this throttle may be destroyed before the DataDecoder returns the result. This would cause the sign-in status update to be dropped, creating a race condition that makes the FedCM sign-in status unreliable.
Consider passing the 'defer' pointer to HandleResponseOrRedirect, setting it to true if a parser is active, and calling delegate_->Resume() in OnHeaderParsed.
Related details
- Nicolás Peña
- Ryan Qin
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. |
Ryan Qin (Gerrit)
Ryan Qin added 2 comments![]( "Open in Gerrit")
I don't think this is ever null, can this be a CHECK() instead?
Done
Please fix this WARNING reported by autoreview issue finding: In the browser process, GetSetLoginHeaderDataDecoderParser() returns an asynchronous parser. Since HandleResponseOrRedirect does not defer the response (by setting *defer = true), the URLLoader will continue processing the response immediately.
If the response is small (e.g., a redirect or an empty body), the URLLoader and this throttle may be destroyed before the DataDecoder returns the result. This would cause the sign-in status update to be dropped, creating a race condition that makes the FedCM sign-in status unreliable.
Consider passing the 'defer' pointer to HandleResponseOrRedirect, setting it to true if a parser is active, and calling delegate_->Resume() in OnHeaderParsed.
- Christian Biesinger
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Arthur Sonzogni (Gerrit)
Arthur Sonzogni added 1 comment![]( "Open in Gerrit")
Message
**[Early Review]** This is an automated early review generated by an LLM. It is intended to help you catch obvious issues early and **potentially save a round of code review**.
If you find any suggestion irrelevant, please feel free to *ignore* or *close* it.
_I am going to take a look immediately._
Please see suggestions below.
1 comment
CHECK(parse_set_login_header_cb_);**[Early Review]**
If `parse_set_login_header_cb_` runs synchronously, `OnHeaderParsed` will be called immediately. This sets `deferred_` to `false` and calls `delegate_->Resume()` before `HandleResponseOrRedirect` returns. Subsequently, `HandleResponseOrRedirect` sets `*defer = false`. This has two issues: 1) It calls `delegate_->Resume()` when the loader is not actually deferred (since `*defer` is returned as `false`), which can cause mismatched state in `ThrottlingURLLoader`. 2) It unconditionally overwrites `*defer` to `false`, potentially overriding a `true` value set by a prior throttle. You should track synchronous execution to avoid calling `Resume()` prematurely, and restore the original `defer` value rather than blindly setting it to `false`.
Related details
- Alex Ilin
- Alex Moshchuk
- Arthur Sonzogni
- Christian Biesinger
- Kouhei Ueno
- Peter Beverloo
- Ryan Qin
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. |
Christian Biesinger (Gerrit)
Christian Biesinger added 1 comment![]( "Open in Gerrit")
deferred_ = true;Thinking through this is making my head hurt
I think the desired behavior is:
- If OnHeaderParsed is called synchronously, set defer to false
- Otherwise, set defer to true
- OnHeaderParsed should only call Resume() if it was called asynchronously
So maybe we need a member variable `is_inside_handleresponse_` that is set to true before we call parse_set_login_header_cb_.Run and set to false afterwards (we can use AutoReset for this), and OnHeaderParsed checks for that member variable when deciding whether to call Resume
And similarly we can have a member variable is_header_parsed_ that OnHeaderParsed will set to true
I think this will auto fix Arthur's LLM-generated comment.
wdyt?
Related details
- Alex Ilin
- Alex Moshchuk
- Arthur Sonzogni
Ryan Qin (Gerrit)
Ryan Qin added 3 comments![]( "Open in Gerrit")
**[Early Review]**
If `parse_set_login_header_cb_` runs synchronously, `OnHeaderParsed` will be called immediately. This sets `deferred_` to `false` and calls `delegate_->Resume()` before `HandleResponseOrRedirect` returns. Subsequently, `HandleResponseOrRedirect` sets `*defer = false`. This has two issues: 1) It calls `delegate_->Resume()` when the loader is not actually deferred (since `*defer` is returned as `false`), which can cause mismatched state in `ThrottlingURLLoader`. 2) It unconditionally overwrites `*defer` to `false`, potentially overriding a `true` value set by a prior throttle. You should track synchronous execution to avoid calling `Resume()` prematurely, and restore the original `defer` value rather than blindly setting it to `false`.
Done
Thinking through this is making my head hurt
I think the desired behavior is:
- If OnHeaderParsed is called synchronously, set defer to false
- Otherwise, set defer to true
- OnHeaderParsed should only call Resume() if it was called asynchronously
So maybe we need a member variable `is_inside_handleresponse_` that is set to true before we call parse_set_login_header_cb_.Run and set to false afterwards (we can use AutoReset for this), and OnHeaderParsed checks for that member variable when deciding whether to call Resume
And similarly we can have a member variable is_header_parsed_ that OnHeaderParsed will set to true
I think this will auto fix Arthur's LLM-generated comment.
wdyt?
Done
Thinking through this is making my head hurt
I think the desired behavior is:
- If OnHeaderParsed is called synchronously, set defer to false
- Otherwise, set defer to true
- OnHeaderParsed should only call Resume() if it was called asynchronously
So maybe we need a member variable `is_inside_handleresponse_` that is set to true before we call parse_set_login_header_cb_.Run and set to false afterwards (we can use AutoReset for this), and OnHeaderParsed checks for that member variable when deciding whether to call Resume
And similarly we can have a member variable is_header_parsed_ that OnHeaderParsed will set to true
I think this will auto fix Arthur's LLM-generated comment.
wdyt?
- Christian Biesinger
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Christian Biesinger (Gerrit)
Christian Biesinger voted and added 2 comments![]( "Open in Gerrit")
Votes added by Christian Biesinger
| Code-Review | +1 |
2 comments
if (!is_inside_handler_response_ && deferred_) {is this necessary? I thought deferred_ would always be true here if is_inside_handler_response_ is false
Related details
- Ryan Qin
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. |
Ryan Qin (Gerrit)
Ryan Qin added 1 comment![]( "Open in Gerrit")
if (!is_inside_handler_response_ && deferred_) {is this necessary? I thought deferred_ would always be true here if is_inside_handler_response_ is false
- Christian Biesinger
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Christian Biesinger (Gerrit)
Christian Biesinger voted Code-Review+1![]( "Open in Gerrit")
- Ryan Qin
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |