Report hardening: escaping, config validation, modal dialog (MED-001/011 + 5 low)#42
Merged
Conversation
readTemplate did a synchronous fs.readFileSync on every call, and it's called once per comparison from generateReport/generateHtmlDiff — so report.html and html-diff.html were re-read from disk for every slug (hundreds of redundant blocking reads where two would do, plus filesystem contention now that compare is parallelized). Cache the contents at module level; templates don't change during a run. Fixes LOW-005. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Path keys and viewport names from reglance.json become the
`${key}-${viewport}` slug that is joined onto the output directory to form
write paths for screenshots, snapshots, diffs, and reports. They were never
validated, so a key like "../../x" could redirect a write outside the
.reglance/ tree.
Validate both at config load against a safe-slug pattern (letters, numbers,
dashes, dots, underscores; no separators or "..") with a clear message, so an
unsafe value fails fast instead of escaping the output sandbox. This also
closes the config-derived source for the report-escaping work.
Fixes LOW-003.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
diffColor/diffColorAlt come from reglance.json and were spread over the
defaults with no validation. A non-array value (e.g. null) made
generateIndex's `diffColor.join(',')` throw a TypeError at the very end of an
otherwise-successful compare run, and a crafted array would be interpolated
into the report's CSS.
Validate both as [r, g, b] triples of 0–255 integers in loadConfig, failing
fast with a clear message that points at the config instead of crashing deep
in report generation. The validated integers also remove the injection vector
for the {diffColor} interpolation.
Fixes LOW-004.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
generateIndex built the table rows and the inline window.diffData script by
string interpolation with no escaping on config/site-derived values
(report.url, name, viewport name). A value with HTML metacharacters injected
markup into the report, and JSON.stringify doesn't neutralize </script>, so a
value containing it could close the inline script and execute following
markup on the report's file:// origin. (It also broke the report on any URL
containing a plain & or <.)
Escape every interpolated value with the existing escapeHtml helper (rows,
title/data attributes, viewport <option>s, the {name} token), and serialize
diffData through a jsonForScript helper that escapes <, U+2028, and U+2029.
Per the D-001 decision configs are treated as untrusted, so this is a real
fix as well as a robustness one.
Fixes MED-011.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
generateReport and generateHtmlDiff substituted {name}, {urlKey}, and
{viewportName} (all config-derived) into report.html and html-diff.html
without escaping — landing in <title>, <h1>, and <p> contexts. Same gap as
MED-011, on the secondary per-comparison pages. Escape them at substitution
time with escapeHtml; numeric tokens stay coerced. generateHtmlDiff is now
exercised directly by a unit test.
Fixes LOW-001.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
"View Diff" opens the modal but was an <a href="#" onclick="...; return false;"> — an in-page action dressed as a link. It announced to AT as a link pointing nowhere (next to the real "View Report"/"View HTML Diff" links) and jumped to top if the handler ever failed to return false. Render it as a <button class="link-button"> styled like the surrounding links, and pass `this` so the modal can return focus to it on close (used by MED-001). Fixes LOW-010. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The image diff viewer was a plain div toggled via style.display: no role/aria-modal/name, focus never moved in or was trapped, Tab leaked to the table behind the overlay, focus wasn't returned to the trigger on close, and the modal title read currentDiff.property — a field diffData never sets — so every title rendered "undefined". - Add role="dialog", aria-modal="true", aria-labelledby="modalTitle"; control visibility with the hidden attribute (single source of truth) via a .modal[hidden] rule instead of a display string. - On open, move focus to the close button and remember the trigger (passed as `this` from the View Diff button); trap Tab among the dialog's buttons; Escape/close returns focus to the trigger. Handlers gate on isOpen(). - Fix the title to use currentDiff.name; set a descriptive per-diff image alt; make the counter aria-live and toggle aria-busy on the body while loading. Fixes MED-001. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add coverage for the output-generation layer that was previously untested: - escapeHtml against all five metacharacters (and the &-first ordering), exported for direct testing. - generateIndex severity-class selection at the exact threshold boundaries (0.1 and 1). - Strengthen the compareSlug test to assert the report and html-diff files are actually written (and named correctly), not just that diffPercentage is returned. These join the escaping/dialog/a11y assertions added alongside the fixes in this branch. Coverage of the browser-only modal interactions and the Playwright capture path remains out of reach without a DOM/browser harness; the pure generators and the retry/exit policy are covered. Addresses MED-012. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes 8 findings — report XSS-hardening, config validation, the modal dialog, and a perf/test cleanup — as 8 atomic commits. Verified with
npm test(83 tests) +npm run lint(0 errors).Per your triage: MED-009, MED-010 (issue #20), and LOW-014 are intentionally not addressed here.
Medium
{name}), and serializewindow.diffDatathrough a helper that neutralizes</script>and U+2028/U+2029. Closes the stored-injection class (and the report no longer breaks on a&/<in a URL). Per the D-001 "untrusted config" decision.role="dialog"+aria-modal+aria-labelledby, visibility via thehiddenattribute (single source of truth), focus moved in on open, Tab trapped within the dialog, focus returned to the trigger on close, handlers gated onisOpen(). Also fixes the title that rendered "undefined" (used a fielddiffDatanever set), adds a descriptive per-diff image alt, a live counter, andaria-busywhile loading.Low
{name}/{urlKey}/{viewportName}in the per-comparison and html-diff templates (same gap as MED-011, secondary pages)...at config load (they become output filenames — path-traversal-on-write).pixelmatchOptions.diffColor/diffColorAltas[r,g,b]0–255 ints at load, so a bad value fails fast with a clear message instead of aTypeErrorat the end of a compare run (and removes the CSS-injection vector).report.html/html-diff.htmlfrom disk per comparison).<button>(passing its trigger for MED-001's focus return) instead ofhref="#".MED-012 — test coverage
Added a
report.test.mjssuite over the previously-untested output layer:escapeHtml(all five metacharacters),generateHtmlDiff,generateIndex(escaping, the</script>sink, severity-class boundaries at 0.1/1, dialog/sortable/empty-state/label markup), and strengthenedcompareSlugto assert the report/html-diff files are written. The browser-only modal interactions and the Playwright capture path remain out of reach without a DOM/browser harness — noted in the commit. (Accepted as partial per your note.)Notes
🤖 Generated with Claude Code