[net] Allow data: URL redirects in manual redirect mode [chromium/src : main]
Adam Rice (Gerrit)
Adam Rice added 4 comments![]( "Open in Gerrit")
I'm not sure how much real-world impact this change will have. We may need to guard it behind a base::Feature,
The description should have more detail about the "why". If possible, refer to the relevant spec sections.
void set_is_fetch_redirect_mode_manual(bool is_manual) {We usually avoid talking about web platform concepts in //net. It would be better to name the variable and accessors after the behaviour that they modify.
if (request_->is_fetch_redirect_mode_manual() && location.is_valid()) {This doesn't actually guarantee we won't follow the redirect, only that someone has promised not to follow the redirect. We need some extra protection to make sure that the redirect really is not followed.
Related details
- Helmut Januschka
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. |
Helmut Januschka (Gerrit)
Helmut Januschka added 3 comments![]( "Open in Gerrit")
The description should have more detail about the "why". If possible, refer to the relevant spec sections.
Done
We usually avoid talking about web platform concepts in //net. It would be better to name the variable and accessors after the behaviour that they modify.
Done
if (request_->is_fetch_redirect_mode_manual() && location.is_valid()) {This doesn't actually guarantee we won't follow the redirect, only that someone has promised not to follow the redirect. We need some extra protection to make sure that the redirect really is not followed.
we will return an opaque-redirect so this why it could considered safe, updated comment
Related details
- Adam Rice
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
mmenke (Gerrit)
mmenke added 1 comment![]( "Open in Gerrit")
// navigated to - an opaque-redirect response will be returned instead.
if (request_->is_fetch_redirect_mode_manual() && location.is_valid()) {Does this make any difference? As far as I can tell, this method always returns true. `request_->context()->job_factory()` should always be non-null, and it looks like there are no IsSafeRedirectTarget() overloads that return null.
This safety checking is an artifact of when the network stack was in the browser process, and all URLs when through this code, as opposed to only HTTPS ones.
If this does change behavior, please add unit tests. If not, let's leave well enough alone in this CL (happy to review a CL that removes this method and JobFactory::IsSafeRedirectTarget(), if you want).
Related details
- Adam Rice
- Helmut Januschka
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. |
mmenke (Gerrit)
mmenke added 1 comment![]( "Open in Gerrit")
// navigated to - an opaque-redirect response will be returned instead.
if (request_->is_fetch_redirect_mode_manual() && location.is_valid()) {Does this make any difference? As far as I can tell, this method always returns true. `request_->context()->job_factory()` should always be non-null, and it looks like there are no IsSafeRedirectTarget() overloads that return null.
This safety checking is an artifact of when the network stack was in the browser process, and all URLs when through this code, as opposed to only HTTPS ones.
If this does change behavior, please add unit tests. If not, let's leave well enough alone in this CL (happy to review a CL that removes this method and JobFactory::IsSafeRedirectTarget(), if you want).
Actually, there's an overload on iOS for Chrome URLs where it returns false, though not sure that one gets us anything.
I do not think we want to modify the behavior for redirects from Chrome URLs on iOS, and the behavior everywhere else looks unchanged.
Helmut Januschka (Gerrit)
Helmut Januschka added 1 comment![]( "Open in Gerrit")
// navigated to - an opaque-redirect response will be returned instead.
if (request_->is_fetch_redirect_mode_manual() && location.is_valid()) {mmenkeDoes this make any difference? As far as I can tell, this method always returns true. `request_->context()->job_factory()` should always be non-null, and it looks like there are no IsSafeRedirectTarget() overloads that return null.
This safety checking is an artifact of when the network stack was in the browser process, and all URLs when through this code, as opposed to only HTTPS ones.
If this does change behavior, please add unit tests. If not, let's leave well enough alone in this CL (happy to review a CL that removes this method and JobFactory::IsSafeRedirectTarget(), if you want).
Actually, there's an overload on iOS for Chrome URLs where it returns false, though not sure that one gets us anything.
I do not think we want to modify the behavior for redirects from Chrome URLs on iOS, and the behavior everywhere else looks unchanged.
thanks the if was rendundant added it on the trial and error approach, it is in fact not needed, uploaded new PS that removes it! thank you
Related details
- Adam Rice
- mmenke
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
mmenke (Gerrit)
mmenke added 2 comments![]( "Open in Gerrit")
Note that I'll defer to Adam to actually sign off on the CL - I'm not familiar with blink code here, just net/ and services/network.
bool is_fetch_redirect_mode_manual_ = false;Changes here and to files in services/network/ should no longer be needed, either, I believe.
Related details
- Adam Rice
- Helmut Januschka
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. |
Helmut Januschka (Gerrit)
Helmut Januschka added 1 comment![]( "Open in Gerrit")
bool is_fetch_redirect_mode_manual_ = false;Changes here and to files in services/network/ should no longer be needed, either, I believe.
I tested removing the changes to url_request.h and services/network/, keeping only the blink change. Unfortunately the tests still fail - the redirect is blocked at the network stack level in CorsURLLoader before it reaches blink.
Related details
- Adam Rice
- mmenke
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 2 comments![]( "Open in Gerrit")
Sorry for the delayed response.
url_request.set_is_fetch_redirect_mode_manual(request.redirect_mode ==I can't find anything in this CL that reads the is_fetch_redirecto_mode_manual flag, so setting it shouldn't make any difference.
Related details
- Helmut Januschka
- mmenke
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. |