diff --git a/.claude/skills/resolving-issues/SKILL.md b/.claude/skills/resolving-issues/SKILL.md index e2accc2c..a6d16629 100644 --- a/.claude/skills/resolving-issues/SKILL.md +++ b/.claude/skills/resolving-issues/SKILL.md @@ -88,6 +88,7 @@ Fresh agent per issue, scoped to one issue + master branch ref. Steps: - `cargo clippy --all-targets -- -D warnings` — scope to touched crate(s) for speed; workspace-wide if changes ripple - `cargo test ` — ditto on scope - `cargo check --target wasm32-unknown-unknown ` if dual-target lib crate touched + - `cargo clippy --target wasm32-unknown-unknown --all-targets -- -D warnings` if dual-target lib crate touched — wasm-only lints (e.g. unnecessary `as u32` on `js_sys::Date::get_month()` which already returns `u32` on wasm32, or `js_sys`-only call paths under `#[cfg(target_arch = "wasm32")]`) only fire on the wasm target. Native clippy misses them, and CI's `cargo check --target wasm32` catches compile errors but not lint warnings — the wasm-clippy gate is the only path that closes that gap locally - `just check` if available + scope is wide enough to warrant the full sweep Apply `superpowers:verification-before-completion` — confirm command output before claiming done. diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 01e0f6a2..22b3c2fd 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -23,6 +23,18 @@ jobs: components: rustfmt - run: cargo fmt --check + lint: + name: ESLint (e2e/) + runs-on: ubuntu-latest + timeout-minutes: 10 + steps: + - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + - uses: actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e # v6.4.0 + with: + node-version: '20' + - run: npm ci + - run: npm run lint + clippy: name: Clippy runs-on: ubuntu-latest diff --git a/crates/web/src/app.rs b/crates/web/src/app.rs index c915ddcb..0fdddd48 100644 --- a/crates/web/src/app.rs +++ b/crates/web/src/app.rs @@ -1176,10 +1176,15 @@ pub fn App() -> impl IntoView { queue_open_rail.set(false); }); let on_pinned_jump = Callback::new(move |msg_id: String| { - js_sys::eval(&format!( - "document.getElementById('msg-{}')?.scrollIntoView({{behavior:'smooth',block:'center'}})", - msg_id.replace('\'', "") - )).ok(); + if let Some(elem) = web_sys::window() + .and_then(|w| w.document()) + .and_then(|d| d.get_element_by_id(&format!("msg-{msg_id}"))) + { + let opts = web_sys::ScrollIntoViewOptions::new(); + opts.set_behavior(web_sys::ScrollBehavior::Smooth); + opts.set_block(web_sys::ScrollLogicalPosition::Center); + elem.scroll_into_view_with_scroll_into_view_options(&opts); + } write.ui.set_show_pinned.set(false); }); view! { diff --git a/crates/web/tests/browser.rs b/crates/web/tests/browser.rs index 2a284376..2a6a92b6 100644 --- a/crates/web/tests/browser.rs +++ b/crates/web/tests/browser.rs @@ -12508,6 +12508,102 @@ mod stun_config { } } +#[cfg(test)] +mod pinned_jump_safe_scroll { + //! Regression tests for the pinned-message jump callback in + //! `crates/web/src/app.rs` (`on_pinned_jump`). The callback used to + //! interpolate `msg_id` into a `js_sys::eval(format!(...))` string with + //! only single-quote stripping as sanitization; it now uses the safe + //! DOM API: `document.getElementById(&format!("msg-{msg_id}"))` plus + //! `Element::scroll_into_view_with_scroll_into_view_options`. + //! + //! These tests exercise the same DOM-lookup pattern the callback now + //! relies on. The callback body is inline (no extracted helper), so we + //! verify the underlying API contract directly: real-looking IDs hit, + //! missing IDs miss cleanly, and adversarial IDs (quotes, backslashes, + //! brackets, newlines) are treated as literal element IDs that simply + //! match nothing — no DOM injection is possible. + //! + //! Refs: https://github.com/intendednull/willow/issues/425 + //! + //! Note: this test does not assert that scrolling visually happened + //! (jsdom-style headless harnesses don't lay out content). The + //! original `eval` path also silently swallowed errors via `.ok()`, + //! so the property under test is "no panic + correct lookup result", + //! identical for both implementations. + use wasm_bindgen_test::*; + + /// Mimic the lookup path used by `on_pinned_jump` in + /// `crates/web/src/app.rs`: `window → document → get_element_by_id` + /// with the same `msg-{msg_id}` formatting. Returns the matched + /// element if any. + fn lookup_msg_element(msg_id: &str) -> Option { + web_sys::window() + .and_then(|w| w.document()) + .and_then(|d| d.get_element_by_id(&format!("msg-{msg_id}"))) + } + + /// Append a `
` to the body and return the id used. + /// Caller is responsible for unique ids per test to avoid cross-test + /// pollution in the shared document. + fn mount_msg_div(id: &str) { + let doc = web_sys::window().expect("window").document().expect("doc"); + let div = doc.create_element("div").expect("create_element"); + div.set_id(&format!("msg-{id}")); + doc.body() + .expect("body") + .append_child(&div) + .expect("append"); + } + + #[wasm_bindgen_test] + fn lookup_finds_existing_hex_id() { + // EventHash::to_string() is a hex digest; verify a realistic id + // round-trips through the safe lookup the callback now uses. + let id = "deadbeefcafef00d1234567890abcdef"; + mount_msg_div(id); + + let el = lookup_msg_element(id).expect("element with msg- id must be found"); + assert_eq!(el.id(), format!("msg-{id}")); + } + + #[wasm_bindgen_test] + fn lookup_returns_none_for_missing_id() { + // Property the old `eval(...).ok()` chain provided implicitly via + // optional chaining and silent error swallowing: a missing id is + // a no-op, never a panic. The safe API mirrors this with `None`. + let id = "nonexistent_id_does_not_match_any_element_in_the_dom"; + assert!( + lookup_msg_element(id).is_none(), + "missing id must return None, not panic" + ); + } + + #[wasm_bindgen_test] + fn lookup_treats_adversarial_id_as_literal_no_injection() { + // Cases the old `replace('\'', "")` band-aid did not cover: + // double-quote, backslash, newline, brackets, parens. With the + // safe DOM API, these become part of the literal id passed to + // `getElementById`, which simply finds no match — no JS context + // exists for them to break out of, so injection is impossible. + let adversarial_ids = [ + "abc\"';alert(1)//", + "id with spaces and 'quotes' and \"double\"", + "id\\with\\backslashes", + "id\nwith\nnewlines", + "id);scrollIntoView({behavior:'smooth'});//", + "", + ]; + + for bad in adversarial_ids { + assert!( + lookup_msg_element(bad).is_none(), + "adversarial id {bad:?} must not match any element", + ); + } + } +} + // ── data-state lifecycle (PR-3 §`data-state` attribute pattern) ───────────── // // Three failure modes from the spec: diff --git a/package.json b/package.json index 13868953..591e7c05 100644 --- a/package.json +++ b/package.json @@ -4,7 +4,8 @@ "description": "", "main": "index.js", "scripts": { - "test": "echo \"Error: no test specified\" && exit 1" + "test": "echo \"Error: no test specified\" && exit 1", + "lint": "eslint e2e/" }, "repository": { "type": "git",