Skip to content

Follow-ups to #373: analytics row a11y, delegated navigate test, compare.js onclick, loadLocalEsc #393

Closed

Description

@dborup

Non-blocking follow-ups from the round-3 re-review of #373 (#333, merged as 8a26a339). Details are in the review comment on #373.

  1. N6, a11y regression. public/analytics.js (~1105): the repeater <div> now has role="row", but its parent .repeater-list has no role. The row therefore lacks the required rowgroup/table/grid ancestor. Add the matching roles or drop role="row".
  2. N7, test gap. The delegated selector was broadened from tr[data-action="navigate"] to [data-action="navigate"] (analytics.js ~333). That is what makes the repeater row clickable, but no test pins it.
  3. N9, remaining inline onclick. public/compare.js (~529) keeps an inline onclick because compare.js has no delegated click handler for nav rows. Add a delegated handler and remove the inline handler, for consistency with fix(ui): escape untrusted node, observer and route values in HTML rendering #373.
  4. N10. test-xss-escape-sinks.js loadLocalEsc now asserts exactly one function esc( per file, but a differently declared second esc (for example const esc = …) is not caught.

The perf.js sinks (N8) are tracked separately as a security issue.

Activity

  1. dborup commented on Oct 13, 2026

    @dborup
    OwnerAuthor

    Related nits from the round-2 re-review of #394 (#392, merged). Details are in the review on #394.

    • N1: docs/api-spec.md documents the total slow-path bound as ≤257 runes; the real bound is 259 (128 + ellipsis per half, plus the join space).
    • N2: the fix(ui): escape perf dashboard values (#392) #394 PR description still describes round 1. For example, it says the numeric metrics are left as-is, but round 2 escapes them.
    • N3: scripts/check-xss-sinks.sh header: the claim that generic locals (path/q.path/k) cannot be added without a false-positive explosion did not hold empirically. Re-evaluate adding them.
    • N4: the DEL (0x7f) half of the control-character strip in normalizeSlowPath is unpinned.
    • N5 and N6: only 5 of ~30 newly escaped perf cards have a test, and escapeHtml(err.message) in perf.js (~374) has none.
  2. added a commit that references this issue on Oct 13, 2026
  3. dborup commented on Oct 13, 2026

    @dborup
    OwnerAuthor

    Fixed by #402 (merged as 4a5ec5e231dd97ed7a4392e50e5f409f4dc5fab0).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions