Skip to content

fix: sanitize card names in deck error display to prevent HTML injection#181

Merged
arasaka-net[bot] merged 2 commits intomainfrom
arasaka/issue-168
Apr 12, 2026
Merged

fix: sanitize card names in deck error display to prevent HTML injection#181
arasaka-net[bot] merged 2 commits intomainfrom
arasaka/issue-168

Conversation

@arasaka-net
Copy link
Copy Markdown

@arasaka-net arasaka-net bot commented Apr 12, 2026

This change has been prepared for integration into the family's asset record.

Summary
Replaces innerHTML template-literal interpolation of user-supplied card names in createDeck with createElement / textContent DOM construction, eliminating the XSS vector. A regression test asserts that an <img> injection payload produces no live element and renders as literal text.

Changes

  • In src/scripts/deck.js, replaced errorElement.innerHTML template literal (which interpolated failed card names verbatim) with explicit createElement + textContent assignments for the heading <strong> and each <li> item.
  • In src/scripts/deck.test.js, added a regression test 'renders HTML characters in failed card names as escaped text, not live markup' that verifies an <img onerror> payload is not injected as a DOM element and appears only as literal text content.

Verification

  • No dependencies installed in CI environment; npm test and npm run lint could not be executed directly.
  • Manual inspection confirms no innerHTML usage in deck.js interpolates user-supplied data — the single remaining innerHTML assignment at line 43 is a fully static string.
  • PR reviewer confirmed: No actionable defects identified. No inline review comments or change requests were filed against PR fix: sanitize card names in deck error display to prevent HTML injection #180.

Assumptions

  • The review state COMMENTED with body No actionable defects identified constitutes acceptance; no code changes are required for this revision.
  • The static innerHTML at line 43 (deck card-back image template) is out of scope per the issue and is not a concern.

Closes #168

Arasaka Implementation Pipeline.

Replace innerHTML interpolation of user-supplied card names with safe DOM
construction (createElement + textContent) in the createDeck error path.
Adds a test verifying that a card name containing HTML markup renders
as escaped text in the .deck-parse-errors element, not as live DOM
nodes. A future reversion of item.textContent back to innerHTML would
now be caught by the test suite.
Copy link
Copy Markdown
Author

@arasaka-net arasaka-net bot left a comment

Choose a reason for hiding this comment

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

Review identified matters requiring attention.

The XSS fix is correctly implemented: innerHTML interpolation of user-supplied card names in createDeck is replaced with createElement/textContent, and the regression test for the injection payload is structurally sound. One low-severity test-coverage gap exists for the multi-item rendering path.

Findings

🔵 LOW — No test covers multiple failed card names producing multiple <li> elements

src/scripts/deck.test.js:128

The new for loop in deck.js lines 59–63 iterates over every entry in failed and appends a separate <li> for each. The only createDeck tests that exercise the error-display path use a single failing card ('1x Hedge Fund\n1x Unknown Card' at line 129, and the XSS payload at line 134). No test passes two distinct unrecognized names and asserts that querySelectorAll('li') within .deck-parse-errors returns exactly two items. A regression where the loop emits only the first entry or de-duplicates incorrectly would pass all current tests. Add a test: call createDeck('1x Ghost Runner\n1x Sneakdoor Beta', ...) and assert errors.querySelectorAll('li').length === 2 and that each li.textContent matches the expected name.

Residual Risk

  • Tests could not be executed in this environment (npm test unavailable); the regression test's runtime behavior in jsdom — specifically whether jsdom silently swallows or surfaces an onerror handler on a hypothetically-injected <img> — was not verified empirically.
  • The static innerHTML at deck.js line 43 references a hardcoded external image URL (encrypted-tbn0.gstatic.com). This is pre-existing and outside the diff, but represents a latent availability dependency not covered by any test.

Arasaka Code Quality Assurance Division.

@arasaka-net arasaka-net bot merged commit 27f3b12 into main Apr 12, 2026
61 checks passed
arasaka-net bot pushed a commit that referenced this pull request Apr 12, 2026
Adds a test asserting that two distinct unrecognized card names each
produce a separate <li> element in .deck-parse-errors, guarding against
regressions where the for-loop emits only the first entry or
de-duplicates incorrectly. Addresses code review feedback on PR #181.
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.

Sanitize card names in deck error display to prevent HTML injection

0 participants