Skip to content

refactor(page): wire the page's buttons in script instead of inline onclick attributes - #147

Merged
mbradley merged 3 commits into
mainfrom
fix/138-no-inline-handlers
Oct 15, 2026
Merged

mbradley merged 3 commits into
mainfrom
fix/138-no-inline-handlers

Conversation

@mbradley

@mbradley mbradley commented Oct 15, 2026 •

Copy link
Copy Markdown
Member

Summary

  • The verifier page's four buttons that used inline onclick attributes (Check, Load my links, and the remove dialog's Cancel and Remove) now get their click handlers from the page's script. Each calls the same function as before.
  • The handlers are attached before the page's start-up code, so an error during start-up can't leave the buttons without handlers. The Check button also gets type="button", like the other three.
  • A new test fails if either page (/ or /verify/...) gains an inline event-handler attribute or a javascript: URL.

Motivation

Related Issue

Testing

  • npm run test:once: 52 files, 1654 tests passed. npx tsc --noEmit: clean.
  • A new test runs the page's real wiring code against stand-in buttons and checks that each click reaches its own function. It fails if a listener line is removed or commented out, if the wiring lines don't parse, or if two handlers are swapped. Other new tests fail if an inline onclick returns, if the wiring moves after the start-up code, or if an onclick hides behind a quoted >.
  • In Chromium and WebKit on wrangler dev: each of the four buttons, by mouse click, Enter and Space, behaved the same as on main, with no console errors. Firefox was not run.
  • Manual check after merge: on the live page, run a check with "Look up someone"'s Check button. Then sign in with an account that has linked accounts, open "Manage verified links", click "Load my links", click "Remove" on one row, then "Cancel" in the dialog.

API / Behavior Notes

  • None. The page behaves the same.

Sensitive Information Check

  • Confirmed: no partner, customer or brand names, or other sensitive external identities.

…nclick attributes

A Content-Security-Policy with a script nonce blocks inline event-handler
attributes. The four buttons (Load my links, Cancel, Remove verification,
Check) now get click listeners from the page's existing script, calling the
same functions as before. A test fails if either page carries an on*
attribute or a javascript: URL.
…dler test

The button listeners now come before the start-up block, so an error there
cannot leave the buttons dead (the inline handlers used to survive that). A
test pins the ordering. The wiring check no longer depends on quote style or
a trailing semicolon, and the markup check handles a '>' inside an attribute
value.
The wiring tests matched source text, so a commented-out or never-run
listener passed. They now run the page's wiring lines against stand-in
buttons and check each click reaches its own function. The Check button also
gets type="button" like the others; the page has no form, so nothing changes.
@mbradley
mbradley requested review from a team, dcadenas and rabble and removed request for a team October 15, 2026 17:39
@divine-roborev

Copy link
Copy Markdown

roborev: Combined Review (00414ef)

Verdict: No findings at or above high severity.


Reviewers: 2 done | Synthesis: opencode | Total: 3m38s

@dcadenas dcadenas left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The four buttons keep their existing actions, with listeners registered before page startup.
No actionable findings at 00414ef.

Checks

  • npm run typecheck: passed.
  • npm run test:once: 52 files and 1,654 tests passed. The interactive test command was not run separately.
  • CI on the reviewed commit passed, including the executed Typecheck, Test, and semantic-title validation steps. CLA and roborev also passed.
  • The regression tests cover the emitted button wiring and both pages' markup, but do not run a browser or the remote-signer flow.

Local checks used Node 26.8.1; CI used Node 24.
I did not rerun browser checks; the description reports Chromium and WebKit testing, with Firefox untested.
This is preparation for #138, not the Content-Security-Policy rollout itself.

@mbradley
mbradley merged commit aec5ee7 into main Oct 15, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants