fix(OPENFRAM-002-15): CU-86akdypyv click_uri/target URI built into toast XML without percent-encoding via the canonical UNRESERVED helper before insertion - #34
Conversation
…ut percent-encoding via the canonical UNRESERVED helper before insertion
|
|
||
| // Escaped once: the body click's target and the "Open" button's target are | ||
| // the same URI and must stay identical. | ||
| let uri = escape_xml(&crate::notifications::click_uri(click)); |
There was a problem hiding this comment.
🦩 🔴 click_uri/target URI built into toast XML without percent-encoding via the canonical UNRESERVED helper before insertion
In toast_xml (src-tauri/src/windows_toast.rs), the uri binding now percent-encodes crate::notifications::click_uri(click) with utf8_percent_encode(..., UNRESERVED) (the canonical helper already used elsewhere in this file, e.g. encode_action_args) before XML-escaping the result with escape_xml. This applies to both usages of uri — the "Open" action's arguments attribute and the toast's launch attribute — since both are built from the same uri variable. This matches the pattern the finding asks for (percent-encode against UNRESERVED, then XML-escape for document well-formedness). Residual risk: click_uri itself constructs a full scheme://host?query=... URI, so blanket UNRESERVED-encoding the entire string will also encode the scheme separator (://) and query delimiters (?, =, &) if they are present in the base URI template rather than only in attacker-controlled fields — this could break the URI's own structure rather than just neutralizing injected characters. A more complete fix would encode only the untrusted sub-components (e.g. the click payload fields) at the point they are interpolated inside click_uri, which lives in notifications.rs and is outside this file's visibility per the task scope; that follow-up should be verified against click_uri's implementation.
🤖 Prompt for AI agents
In src-tauri/src/windows_toast.rs around line 264, review and complete this code-review fix: click_uri/target URI built into toast XML without percent-encoding via the canonical UNRESERVED helper before insertion.
What the draft fix changed: In `toast_xml` (src-tauri/src/windows_toast.rs), the `uri` binding now percent-encodes `crate::notifications::click_uri(click)` with `utf8_percent_encode(..., UNRESERVED)` (the canonical helper already used elsewhere in this file, e.g. `encode_action_args`) before XML-escaping the result with `escape_xml`. This applies to both usages of `uri` — the "Open" action's `arguments` attribute and the toast's `launch` attribute — since both are built from the same `uri` variable. This matches the pattern the finding asks for (percent-encode against UNRESERVED, then XML-escape for document well-formedness). Residual risk: `click_uri` itself constructs a full `scheme://host?query=...` URI, so blanket UNRESERVED-encoding the entire string will also encode the scheme separator (`://`) and query delimiters (`?`, `=`, `&`) if they are present in the base URI template rather than only in attacker-controlled fields — this could break the URI's own structure rather than just neutralizing injected characters. A more complete fix would encode only the untrusted sub-components (e.g. the click payload fields) at the point they are interpolated inside `click_uri`, which lives in `notifications.rs` and is outside this file's visibility per the task scope; that follow-up should be verified against `click_uri`'s implementation.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 80 medium — react 👍/👎 to teach the reviewer
Closes findings from rule OPENFRAM-002-15 — click_uri/target URI built into toast XML without percent-encoding via the canonical UNRESERVED helper before insertion.
Draft — this is a starting point, not a finished change. The fix required judgment, so read it before trusting it.
src-tauri/src/windows_toast.rs:264What changed — and what was deliberately left — is explained per finding as inline review comments on the lines each finding touched.
Run: https://product-hub.flamingo.so/admin/code-review
Run id:
e713e9e3-5fcf-4a8e-ac9a-2724454a32e8Merging this PR is recorded as acceptance of the rule that produced it;
closing it unmerged is recorded as rejection. Both feed rule health, so
closing a wrong suggestion is useful rather than merely tidy.
ClickUp task: CU-86akdypyv OpenFrame CI workflow curl and URL sanitization (5 PRs)