[v0.1/P0] Harden OSC 8 activation and remove URI disclosure

natsukium commented

2026-09-03 16:17:42 +09:00

Owner

Parent: #12 (P0.2)

Why

The grid parser filters OSC 8 schemes, but the final launcher still accepts an arbitrary string, logs producer-controlled URIs, and gives the user no safe target preview. That violates the documented activation and logging boundary.

Scope

Introduce a validated ActivationTarget; platform launchers must not accept raw strings.
Re-check schemes and reject malformed targets, NUL, controls, and bidi controls at activation.
Show a control-safe, bidi-safe preview while the activation modifier is held.
Log only outcome, scheme class, and length.
Keep argv-based xdg-open/open and the audited ShellExecuteW path.

Acceptance criteria

javascript: and data: are rejected at the activation boundary even if stored by a future grid path.
Logs never contain the URI.
The preview cannot inject controls or bidi reordering.
Linux, macOS, and Windows launcher tests cover the typed boundary.
Security/reference/explanation docs and CHANGELOG.md reflect the final behavior.

Parent: #12 (P0.2) ## Why The grid parser filters OSC 8 schemes, but the final launcher still accepts an arbitrary string, logs producer-controlled URIs, and gives the user no safe target preview. That violates the documented activation and logging boundary. ## Scope - Introduce a validated `ActivationTarget`; platform launchers must not accept raw strings. - Re-check schemes and reject malformed targets, NUL, controls, and bidi controls at activation. - Show a control-safe, bidi-safe preview while the activation modifier is held. - Log only outcome, scheme class, and length. - Keep argv-based `xdg-open`/`open` and the audited `ShellExecuteW` path. ## Acceptance criteria - [ ] `javascript:` and `data:` are rejected at the activation boundary even if stored by a future grid path. - [ ] Logs never contain the URI. - [ ] The preview cannot inject controls or bidi reordering. - [ ] Linux, macOS, and Windows launcher tests cover the typed boundary. - [ ] Security/reference/explanation docs and `CHANGELOG.md` reflect the final behavior.

natsukium added the

priority/P0

release/v0.1.0

labels

2026-09-03 16:17:59 +09:00

natsukium referenced this issue

2026-09-03 16:18:49 +09:00

[v0.1/Tracker] Close release blockers before v0.1.0 #12

natsukium commented

2026-09-03 20:35:10 +09:00

Author

Owner

Triage plan (2026-09-03)

Source-grounded triage against main at 69076d42, reviewed through seven rounds of an independent reviewer (pi sol/luna) until it passed with no findings. The dependency order that supersedes the tracker's is posted on #12.

Claim check

Accurate against HEAD (69076d42). Nothing in the three post-snapshot commits touches this path.

Grid-side filter exists and is the only scheme check. crates/felis-grid/src/lib.rs:2694-2713: sanitize_osc_str drops any OSC body containing a C0 byte or DEL, and osc8_scheme_allowed allowlists http/https/mailto/file (ASCII case-insensitive). Nothing filters Unicode bidi controls (U+202A–U+202E, U+2066–U+2069, U+200E/F, U+061C are 0x80+ in UTF-8 and pass the C0/DEL check).
The launcher accepts a raw string. crates/felis-client/src/hyperlink.rs:94 pub fn open_url(url: &str) (Unix) and :124 (Windows). ShellExecuteArgs::for_url (:150-158) does only the interior-NUL check via wide_z (:234-242). The Unix path has no check at all; it relies on Command::arg, which rejects an interior NUL at spawn. No scheme re-check on either path.
Logs carry the producer-controlled URI. crates/felis-client/src/event_handler.rs:929-933: warn!(?err, %uri, "open URL failed") and info!(%uri, "opened OSC 8 hyperlink"). The Windows thread's warn!(?err, ...) at hyperlink.rs:128 logs only the ShellExecuteW code, not the URI.
No preview surface exists. The only renderer overlays are search, confirm, and IME preedit (event_handler.rs:1109-1117, main.rs:1345). Yet docs/explanation/security-model.md:371-373 already promises "OSC 8 hyperlinks display the target URL on hover or in a status surface; the visible link text is never trusted to represent the target." That is a documented commitment the code does not meet, so the preview is not new scope, it is a doc/code gap.
What the client already bounds. The shadow rejects entries over LinkText::CAP (= felis_vt::OSC_BUFFER_LIMIT) at crates/felis-client-core/src/shadow.rs:321-334 via link_entry (:495-503), so URI length at activation is already bounded; the daemon's LINK_TABLE_BYTE_CAP (8 MiB, felis-grid/src/link_table.rs:59) bounds the table.
Docs state. docs/reference/spec.md:219 REQ-910 says allowlist + explicit activation only; docs/reference/security-audits.md:70-86 records the argv/ShellExecuteW shape. Neither says anything about activation-time re-validation, preview safety, or log contents.

The issue's "even if stored by a future grid path" framing is right: today the grid is the sole gate, and the client trusts whatever GridMsg::Hyperlink carries (a non-felis daemon, or a future grid change, bypasses it).

Verdict

accept-with-changes. Principle check passes: the feature is the explicit OSC 8 activation the principles already name (principle 2 lists OSC 8; principle 4 is satisfied because nothing scans content). The changes are about where the type lives and how minimal the preview is:

Put ActivationTarget in felis-client-core (new hyperlink.rs), not felis-client: it needs no winit/wgpu, it is testable on every CI host, and any future frontend (felis-<frontend> exec target per CLAUDE.md) reuses it. felis-client::hyperlink::open_url then takes &ActivationTarget only.
Keep the preview to the Ctrl-held hover case the pointer icon already keys on (hyperlink.rs:27-41), rendered through the existing overlay path rather than a new status bar. A persistent status surface would be layout felis does not own (principle 1).
Do not add a config knob for schemes; security-model.md:377-380 records that adding a scheme is a design decision, not config. Keep that.

Approach

felis-client-core (src/hyperlink.rs, new; export from lib.rs):

pub struct ActivationTarget { uri: String, scheme: SchemeClass }
pub enum SchemeClass { Http, Https, Mailto, File }
pub enum ActivationRejection { NoScheme, SchemeDenied, InteriorNul, ControlChar, BidiControl, TooLong }
impl ActivationTarget {
    pub fn parse(uri: &str) -> Result<Self, ActivationRejection>;   // allowlist re-check (same 4 schemes, ASCII case-insensitive), reject '\0', any `char::is_control` (covers C0, DEL, C1), the bidi set above, and len > LinkText::CAP
    pub fn as_str(&self) -> &str;
    pub fn scheme(&self) -> SchemeClass;
    pub fn preview(&self, max_chars: usize) -> String;             // clip with "…"; because parse already refused controls/bidi, the preview is the validated string itself; document that invariant in the test, not a comment
    pub fn log_fields(&self) -> (SchemeClass, usize);              // (scheme class, byte length) is the only thing a log line may carry
}

Bidi rejection rather than stripping: a URI carrying U+202E is hostile by construction (RFC 3986 URIs are ASCII after IRI mapping), and stripping would activate a target the user never saw.

felis-client:

hyperlink.rs: open_url(target: &ActivationTarget). On Unix, factor the argv shape into fn launch_argv(target) -> (&'static str, &str) and pin it in a test the way ShellExecuteArgs is pinned (hyperlink.rs:127-152), so the Linux/macOS boundary is assertable on the CI that exists. ShellExecuteArgs::for_url(&ActivationTarget); the NUL check in wide_z stays as defense in depth.
event_handler.rs:922-938: url_at → ActivationTarget::parse; on Err do nothing but debug!(?rejection, "OSC 8 activation refused") (rejection carries no URI). On success: info!(scheme = ?t.scheme(), len = t.as_str().len(), "opened OSC 8 hyperlink"); the failure arm logs ?err only. Add hover_target: Option<ActivationTarget> updated where update_mouse_cursor_icon runs, and a LinkPreviewOverlay { text } passed to the renderer alongside the confirm overlay (event_handler.rs:1110-1117).
felis-render-wgpu: reuse the ConfirmOverlay text drawing (app_methods.rs:592-596) for a bottom-anchored single-line preview; no new shader.

Tests

client-core: parse rejects javascript:alert(1), data:text/html,…, JAVASCRIPT: and vbscript:; accepts HTTPS:// and mailto:; rejects NUL, \x1b, \u{85}, \u{202e}, \u{2066}; preview clips at the cap and never yields a char::is_control or bidi char (proptest over arbitrary strings: parse(s).map(|t| t.preview(64)) never contains a forbidden char).
client: Linux/macOS launch_argv test (program name + single arg, verbatim); Windows tests retargeted to ActivationTarget; the existing HOSTILE_URLS list (hyperlink.rs:430-441) stays.
Log content: a tracing subscriber capturing to a Vec<u8> around the activation path asserts the captured text does not contain the URI (client-core has tracing; if a test subscriber dependency is unwanted, unit-test log_fields and keep the call sites free of %uri by grep in review).

Docs cascade (doc-cascade skill)

docs/explanation/security-model.md "OSC 8 hyperlinks and OSC 7 CWD": add the activation-boundary bullet (typed target, re-check at activation, rejected classes incl. bidi), the preview bullet (replace the unmet promise at :371-373 with what ships: Ctrl-hover preview), and a logging bullet (scheme class + length only). Record the rejected alternative (stripping bidi) inline.
docs/reference/spec.md REQ-910: append "re-validated at activation from a typed target; logs carry scheme class and length only; a control-safe preview is shown while the activation modifier is held."
docs/reference/security-audits.md:70-86: update the OSC 8 entry (typed boundary, Unix argv pin).
The mouse/keybindings reference page that names Ctrl+Click (grep Ctrl+Click / Ctrl+Left under docs/reference/) gains the preview sentence.
CHANGELOG.md Unreleased / Changed: preview on Ctrl-hover; activation refuses malformed targets; logs no longer contain URIs.
skills/felis: no change (no CLI/IPC surface).

Dependencies

None. #12 puts it first and that still holds; it touches none of the wire/limits work in #14–#16/#49.

Risk/effort

M. Most of the cost is the preview overlay in felis-render-wgpu; the boundary type and log change are S. Main risk: Windows launcher path remains untestable in CI (existing gap, security-audits.md:82-86); mitigated by keeping the Windows change to the argument type only.

Labels

Keep priority/P0, release/v0.1.0. The log disclosure alone would be P1, but the activation boundary is the documented security model's stated guarantee and cheap to close before the first tag.

## Triage plan (2026-09-03) Source-grounded triage against `main` at `69076d42`, reviewed through seven rounds of an independent reviewer (`pi` sol/luna) until it passed with no findings. The dependency order that supersedes the tracker's is posted on #12. ## Claim check Accurate against HEAD (69076d42). Nothing in the three post-snapshot commits touches this path. - **Grid-side filter exists and is the only scheme check.** `crates/felis-grid/src/lib.rs:2694-2713`: `sanitize_osc_str` drops any OSC body containing a C0 byte or DEL, and `osc8_scheme_allowed` allowlists `http`/`https`/`mailto`/`file` (ASCII case-insensitive). Nothing filters Unicode bidi controls (U+202A–U+202E, U+2066–U+2069, U+200E/F, U+061C are 0x80+ in UTF-8 and pass the C0/DEL check). - **The launcher accepts a raw string.** `crates/felis-client/src/hyperlink.rs:94` `pub fn open_url(url: &str)` (Unix) and `:124` (Windows). `ShellExecuteArgs::for_url` (`:150-158`) does only the interior-NUL check via `wide_z` (`:234-242`). The Unix path has no check at all; it relies on `Command::arg`, which rejects an interior NUL at spawn. No scheme re-check on either path. - **Logs carry the producer-controlled URI.** `crates/felis-client/src/event_handler.rs:929-933`: `warn!(?err, %uri, "open URL failed")` and `info!(%uri, "opened OSC 8 hyperlink")`. The Windows thread's `warn!(?err, ...)` at `hyperlink.rs:128` logs only the `ShellExecuteW` code, not the URI. - **No preview surface exists.** The only renderer overlays are search, confirm, and IME preedit (`event_handler.rs:1109-1117`, `main.rs:1345`). Yet `docs/explanation/security-model.md:371-373` already promises "OSC 8 hyperlinks display the target URL on hover or in a status surface; the visible link text is never trusted to represent the target." That is a documented commitment the code does not meet, so the preview is not new scope, it is a doc/code gap. - **What the client already bounds.** The shadow rejects entries over `LinkText::CAP` (= `felis_vt::OSC_BUFFER_LIMIT`) at `crates/felis-client-core/src/shadow.rs:321-334` via `link_entry` (`:495-503`), so URI length at activation is already bounded; the daemon's `LINK_TABLE_BYTE_CAP` (8 MiB, `felis-grid/src/link_table.rs:59`) bounds the table. - **Docs state.** `docs/reference/spec.md:219` REQ-910 says allowlist + explicit activation only; `docs/reference/security-audits.md:70-86` records the argv/`ShellExecuteW` shape. Neither says anything about activation-time re-validation, preview safety, or log contents. The issue's "even if stored by a future grid path" framing is right: today the grid is the sole gate, and the client trusts whatever `GridMsg::Hyperlink` carries (a non-felis daemon, or a future grid change, bypasses it). ## Verdict **accept-with-changes.** Principle check passes: the feature is the explicit OSC 8 activation the principles already name (principle 2 lists OSC 8; principle 4 is satisfied because nothing scans content). The changes are about *where* the type lives and how minimal the preview is: 1. Put `ActivationTarget` in **`felis-client-core`** (new `hyperlink.rs`), not `felis-client`: it needs no winit/wgpu, it is testable on every CI host, and any future frontend (`felis-<frontend>` exec target per CLAUDE.md) reuses it. `felis-client::hyperlink::open_url` then takes `&ActivationTarget` only. 2. Keep the preview to the **Ctrl-held hover** case the pointer icon already keys on (`hyperlink.rs:27-41`), rendered through the existing overlay path rather than a new status bar. A persistent status surface would be layout felis does not own (principle 1). 3. Do not add a config knob for schemes; `security-model.md:377-380` records that adding a scheme is a design decision, not config. Keep that. ## Approach **felis-client-core** (`src/hyperlink.rs`, new; export from `lib.rs`): ```rust pub struct ActivationTarget { uri: String, scheme: SchemeClass } pub enum SchemeClass { Http, Https, Mailto, File } pub enum ActivationRejection { NoScheme, SchemeDenied, InteriorNul, ControlChar, BidiControl, TooLong } impl ActivationTarget { pub fn parse(uri: &str) -> Result<Self, ActivationRejection>; // allowlist re-check (same 4 schemes, ASCII case-insensitive), reject '\0', any `char::is_control` (covers C0, DEL, C1), the bidi set above, and len > LinkText::CAP pub fn as_str(&self) -> &str; pub fn scheme(&self) -> SchemeClass; pub fn preview(&self, max_chars: usize) -> String; // clip with "…"; because parse already refused controls/bidi, the preview is the validated string itself; document that invariant in the test, not a comment pub fn log_fields(&self) -> (SchemeClass, usize); // (scheme class, byte length) is the only thing a log line may carry } ``` Bidi rejection rather than stripping: a URI carrying U+202E is hostile by construction (RFC 3986 URIs are ASCII after IRI mapping), and stripping would activate a target the user never saw. **felis-client**: - `hyperlink.rs`: `open_url(target: &ActivationTarget)`. On Unix, factor the argv shape into `fn launch_argv(target) -> (&'static str, &str)` and pin it in a test the way `ShellExecuteArgs` is pinned (`hyperlink.rs:127-152`), so the Linux/macOS boundary is assertable on the CI that exists. `ShellExecuteArgs::for_url(&ActivationTarget)`; the NUL check in `wide_z` stays as defense in depth. - `event_handler.rs:922-938`: `url_at` → `ActivationTarget::parse`; on `Err` do nothing but `debug!(?rejection, "OSC 8 activation refused")` (rejection carries no URI). On success: `info!(scheme = ?t.scheme(), len = t.as_str().len(), "opened OSC 8 hyperlink")`; the failure arm logs `?err` only. Add `hover_target: Option<ActivationTarget>` updated where `update_mouse_cursor_icon` runs, and a `LinkPreviewOverlay { text }` passed to the renderer alongside the confirm overlay (`event_handler.rs:1110-1117`). - `felis-render-wgpu`: reuse the `ConfirmOverlay` text drawing (`app_methods.rs:592-596`) for a bottom-anchored single-line preview; no new shader. **Tests** - client-core: `parse` rejects `javascript:alert(1)`, `data:text/html,…`, `JAVASCRIPT:` and `vbscript:`; accepts `HTTPS://` and `mailto:`; rejects NUL, `\x1b`, `\u{85}`, `\u{202e}`, `\u{2066}`; `preview` clips at the cap and never yields a `char::is_control` or bidi char (proptest over arbitrary strings: `parse(s).map(|t| t.preview(64))` never contains a forbidden char). - client: Linux/macOS `launch_argv` test (program name + single arg, verbatim); Windows tests retargeted to `ActivationTarget`; the existing `HOSTILE_URLS` list (`hyperlink.rs:430-441`) stays. - Log content: a `tracing` subscriber capturing to a `Vec<u8>` around the activation path asserts the captured text does not contain the URI (client-core has `tracing`; if a test subscriber dependency is unwanted, unit-test `log_fields` and keep the call sites free of `%uri` by grep in review). **Docs cascade** (`doc-cascade` skill) - `docs/explanation/security-model.md` "OSC 8 hyperlinks and OSC 7 CWD": add the activation-boundary bullet (typed target, re-check at activation, rejected classes incl. bidi), the preview bullet (replace the unmet promise at `:371-373` with what ships: Ctrl-hover preview), and a logging bullet (scheme class + length only). Record the rejected alternative (stripping bidi) inline. - `docs/reference/spec.md` REQ-910: append "re-validated at activation from a typed target; logs carry scheme class and length only; a control-safe preview is shown while the activation modifier is held." - `docs/reference/security-audits.md:70-86`: update the OSC 8 entry (typed boundary, Unix argv pin). - The mouse/keybindings reference page that names Ctrl+Click (grep `Ctrl+Click` / `Ctrl+Left` under `docs/reference/`) gains the preview sentence. - `CHANGELOG.md` Unreleased / Changed: preview on Ctrl-hover; activation refuses malformed targets; logs no longer contain URIs. - `skills/felis`: no change (no CLI/IPC surface). ## Dependencies None. #12 puts it first and that still holds; it touches none of the wire/limits work in #14–#16/#49. ## Risk/effort **M.** Most of the cost is the preview overlay in `felis-render-wgpu`; the boundary type and log change are S. Main risk: Windows launcher path remains untestable in CI (existing gap, `security-audits.md:82-86`); mitigated by keeping the Windows change to the argument type only. ## Labels Keep `priority/P0`, `release/v0.1.0`. The log disclosure alone would be P1, but the activation boundary is the documented security model's stated guarantee and cheap to close before the first tag.

natsukium added the

status/planned

label

2026-09-03 20:35:10 +09:00

natsukium referenced this issue

2026-09-03 20:35:21 +09:00

[v0.1/Tracker] Close release blockers before v0.1.0 #12

natsukium started working

2026-09-03 21:00:35 +09:00

natsukium stopped working

2026-09-03 21:00:37 +09:00

2 seconds