[PWA] Restrict manifest icon URL schemes [chromium/src : main]
Daniel Murphy (Gerrit)
Daniel Murphy voted Auto-Submit+1![]( "Open in Gerrit")
| Auto-Submit | +1 |
Related details
- Adriana Ixba
- Chromium IPC Reviews
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Adriana Ixba (Gerrit)
Adriana Ixba voted Code-Review+1![]( "Open in Gerrit")
- Chromium IPC Reviews
- Daniel Murphy
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: na...@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): na...@chromium.org
Reviewer source(s):
na...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
- Daniel Murphy
- Nasko Oskov
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nasko Oskov (Gerrit)
Nasko Oskov voted and added 1 comment![]( "Open in Gerrit")
Votes added by Nasko Oskov
| Code-Review | +1 |
1 comment
Related details
- Adriana Ixba
- Daniel Murphy
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Murphy (Gerrit)
Daniel Murphy voted Auto-Submit+1![]( "Open in Gerrit")
- Adriana Ixba
- Nasko Oskov
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Murphy (Gerrit)
Daniel Murphy voted and added 1 comment![]( "Open in Gerrit")
Votes added by Daniel Murphy
| Auto-Submit | +1 |
| Commit-Queue | +1 |
1 comment
icon->setSrc(execution_context->CompleteURL(icon->src()));This change resolves relative icon URLs synchronously in the renderer before they are converted into the Mojo options struct.
Why this is safe:
Consistency: Mutating the original V8 options object immediately ensures that the TypeConverter copies full URLs. This prevents the strict Mojo traits validation from rejecting the message due to invalid URLs.
No-op in Icon Loader: The asynchronous BackgroundFetchIconLoader still resolves these URLs later, but since they are already full URLs, it is a safe no-op.
Migration & Database Analysis:
New Registrations: Correctly stores the full resolved URL in the database.
Old Registrations: Old registrations with relative URLs were stored as "" in the database. This is completely harmless because the browser process uses the cached icon bitmap, not the URL, to display the icon. So no migration is needed.
Related details
- Adriana Ixba
- Nasko Oskov
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. |
Adriana Ixba (Gerrit)
Adriana Ixba voted and added 1 comment![]( "Open in Gerrit")
Votes added by Adriana Ixba
| Code-Review | +1 |
1 comment
Related details
- Daniel Murphy
- Nasko Oskov
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. |
Nasko Oskov (Gerrit)
Nasko Oskov voted![]( "Open in Gerrit")
| Code-Review | +1 |
| Commit-Queue | +2 |
Related details
- Daniel Murphy
Daniel Murphy (Gerrit)
Daniel Murphy voted and added 1 comment![]( "Open in Gerrit")
Votes added by Daniel Murphy
| Auto-Submit | +1 |
1 comment
icon->setSrc(execution_context->CompleteURL(icon->src()));This change resolves relative icon URLs synchronously in the renderer before they are converted into the Mojo options struct.
Why this is safe:
Consistency: Mutating the original V8 options object immediately ensures that the TypeConverter copies full URLs. This prevents the strict Mojo traits validation from rejecting the message due to invalid URLs.
No-op in Icon Loader: The asynchronous BackgroundFetchIconLoader still resolves these URLs later, but since they are already full URLs, it is a safe no-op.Migration & Database Analysis:
New Registrations: Correctly stores the full resolved URL in the database.
Old Registrations: Old registrations with relative URLs were stored as "" in the database. This is completely harmless because the browser process uses the cached icon bitmap, not the URL, to display the icon. So no migration is needed.
- Nasko Oskov
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Murphy (Gerrit)
Daniel Murphy voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Daniel Murphy (Gerrit)
Daniel Murphy added 2 comments![]( "Open in Gerrit")
+peter for background fetch reviewer
icon->setSrc(execution_context->CompleteURL(icon->src()));Daniel MurphyThis change resolves relative icon URLs synchronously in the renderer before they are converted into the Mojo options struct.
Why this is safe:
Consistency: Mutating the original V8 options object immediately ensures that the TypeConverter copies full URLs. This prevents the strict Mojo traits validation from rejecting the message due to invalid URLs.
No-op in Icon Loader: The asynchronous BackgroundFetchIconLoader still resolves these URLs later, but since they are already full URLs, it is a safe no-op.Migration & Database Analysis:
New Registrations: Correctly stores the full resolved URL in the database.
Old Registrations: Old registrations with relative URLs were stored as "" in the database. This is completely harmless because the browser process uses the cached icon bitmap, not the URL, to display the icon. So no migration is needed.
Done
- Nasko Oskov
- Peter Beverloo
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. |
Peter Beverloo (Gerrit)
Peter Beverloo voted and added 1 comment![]( "Open in Gerrit")
Votes added by Peter Beverloo
| Code-Review | +1 |
1 comment
- Daniel Murphy
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. |
Daniel Murphy (Gerrit)
Daniel Murphy voted and added 1 comment![]( "Open in Gerrit")
Votes added by Daniel Murphy
| Commit-Queue | +2 |
1 comment
icon->setSrc(execution_context->CompleteURL(icon->src()));Daniel MurphyThis change resolves relative icon URLs synchronously in the renderer before they are converted into the Mojo options struct.
Why this is safe:
Consistency: Mutating the original V8 options object immediately ensures that the TypeConverter copies full URLs. This prevents the strict Mojo traits validation from rejecting the message due to invalid URLs.
No-op in Icon Loader: The asynchronous BackgroundFetchIconLoader still resolves these URLs later, but since they are already full URLs, it is a safe no-op.Migration & Database Analysis:
New Registrations: Correctly stores the full resolved URL in the database.
Old Registrations: Old registrations with relative URLs were stored as "" in the database. This is completely harmless because the browser process uses the cached icon bitmap, not the URL, to display the icon. So no migration is needed.
Daniel MurphyDone
re-opening, for background_fetch_manager.cc owner
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[PWA] Restrict manifest icon URL schemes
Restrict the allowed schemes for icons in the Web App Manifest to
`http`, `https`, and `data`, OR a the same scheme as the document.
This mitigates blind SSRF risks where a malicious manifest could point
icons to internal network targets.
In the parser, icons with invalid schemes are ignored with a console
error. In Mojo traits, deserialization fails if an invalid scheme is
sent, protecting against compromised renderers.
Also, in BackgroundFetchManager, resolve relative icon URLs
synchronously in the renderer before converting to Mojo types. This
ensures that the new strict Mojo traits validation checks pass for all
valid relative URLs, while correctly persisting fully-resolved URLs in
the metadata database.
- M content/browser/manifest/manifest_browsertest.cc
- M content/browser/manifest/manifest_manager_host.cc
- M content/common/background_fetch/background_fetch_mojom_traits_unittest.cc
- M third_party/blink/common/manifest/manifest_mojom_traits.cc
- M third_party/blink/common/manifest/manifest_mojom_traits_unittest.cc
- M third_party/blink/renderer/modules/background_fetch/background_fetch_manager.cc
- M third_party/blink/renderer/modules/manifest/manifest_parser.cc
- M third_party/blink/renderer/modules/manifest/manifest_parser_unittest.cc
Code-Review: +1 by Adriana Ixba, +1 by Peter Beverloo, +1 by Nasko Oskov
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |