Skip to content

Post-hoc fixes for #20: ProfileCard href safety, missing tests, dead tooltip assertion - #22

Merged
TheGreatAxios merged 3 commits into
mainfrom
cl-6028-posthoc-fixes
Aug 14, 2026
Merged

Post-hoc fixes for #20: ProfileCard href safety, missing tests, dead tooltip assertion#22
TheGreatAxios merged 3 commits into
mainfrom
cl-6028-posthoc-fixes

Conversation

@TheGreatAxios

@TheGreatAxios TheGreatAxios commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

A post-hoc review of #20 (merged to `main` at 1d638e5) found three issues, all fixed here:

  1. ProfileCard XSS via href (verified exploitable) — `sharedChannels[].href` and `pinnedSkills[].href` rendered straight into ``. React blocks bare `javascript:` but `data:`/`file:` pass through unchanged. Now routed through `toSafeHref` (`src/lib/url.js`), the same guard already used by `artifact-body`, `embed-body`, and `research-body`: safe `http(s)` hrefs pass through, anything else falls back to plain text.
  2. ProfileCard had zero tests. Added `tests/ui/profile-card.test.tsx`: unsafe-scheme hrefs render as plain text (not a live link), safe hrefs pass through unchanged, action-button click wiring, and the conditional header/status/subtitle/footer/empty-section branches.
  3. tooltip.test.tsx's Escape-dismissal assertion was dead code — it lived inside `if (describedBy !== null)`, which never ran because Radix nulls `aria-describedby` immediately on Escape, so the test asserted nothing about dismissal. Replaced with an unconditional assertion that `aria-describedby` is cleared and the tooltip content element is removed from the DOM.

Branched from `main` (1d638e5), not from any diverged commit.

Test plan

  • `bun run typecheck` — clean
  • `bun run lint` — clean
  • `bun run test` — 146 pass, 0 fail (up from 133)
  • `node scripts/dep-guard.mjs` — clean
  • `bun run build` (incl. contrast-test) — all light/dark pairs pass
  • `git diff --stat main` scoped to exactly the three fix items — no unrelated files
@TheGreatAxios
Add ProfileCard href-safety and coverage tests
02c10a7 
Covers the CL-6028 post-hoc review findings: unsafe schemes (data:,
file:, javascript:) on sharedChannels/pinnedSkills hrefs must never
reach a live <a href>, safe http(s) hrefs must pass through unchanged,
and the component's action-button wiring and conditional
header/status/footer branches previously had no coverage at all.
@TheGreatAxios
ProfileCard: route hrefs through toSafeHref before rendering
ab5c3cf 
sharedChannels[].href and pinnedSkills[].href rendered straight into
<a href> — React blocks bare javascript: but passes data: and file:
through unchanged, so an untrusted channel/skill name (agent- or
directory-supplied) could carry a live script or local-file link.
Matches the toSafeHref pattern already used by artifact-body,
embed-body, and research-body: safe http(s) hrefs pass through
unchanged, anything else falls back to plain text rather than a
disabled-looking but still-live link.
@TheGreatAxios
tooltip.test.tsx: assert Escape actually dismisses the tooltip
4a4a3e8 
The Escape-dismissal test's final assertion was inside
'if (describedBy !== null)', which never ran — Radix nulls
aria-describedby immediately on Escape, so the guard always skipped
the assertion and the test proved nothing about dismissal beyond the
open-state check above it. Now asserts unconditionally that
aria-describedby is cleared and the tooltip content element is gone
from the document.
@TheGreatAxios
TheGreatAxios merged commit 0627774 into main Aug 14, 2026
@TheGreatAxios
TheGreatAxios deleted the cl-6028-posthoc-fixes branch August 14, 2026 19:09
TheGreatAxios added a commit that referenced this pull request Aug 14, 2026
@TheGreatAxios
Merge main (0627774) into cl-5879-shell-mock-v2-react-ui: brings in t…
60598ae 
…he ProfileCard href-safety fix (#22)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

1 participant

@TheGreatAxios