Audit sweep: security, correctness, perf, cleanup#20
Open
pulsedemon wants to merge 26 commits into
Open
Conversation
The committed .env only contained a public Sentry DSN (DSNs are designed to ship to the client), so this isn't a credential leak — but the pattern invites future contributors to put real secrets there. Amplify already rebuilds .env from CI env vars at build time, so removing the tracked file has no deployment impact.
updateSavedMixesList previously interpolated mix.name straight into innerHTML. mix.name flows from localStorage and is therefore attacker- controlled by anything with same-origin write access (DevTools, browser extensions, another exploit on the origin). A planted name like <img src=x onerror=...> would execute on every render of the lab. Switch to createElement + textContent for the label and event-listener binding for the buttons. data-id attributes removed since the closure- captured mix already supplies the id. Adds a regression test that stores a hostile name and asserts no script execution and that the markup is rendered as literal text.
viruses/lab/index.html wrote URL params directly into iframe.src with no validation, so values like ?primary=../../foo would resolve to arbitrary same-origin paths inside the inner iframes. Adds an inline ALLOWED_VIRUSES set (kept in sync with components/Playlist.ts) and shows an "invalid mix" fallback when params don't validate. The lab page is a static HTML with an inline module script and can't import Playlist directly. A drift test parses the inline allowlist and asserts equality with Playlist.viruses so CI catches divergence.
Adds Referrer-Policy, X-Content-Type-Options, X-Frame-Options. Deliberately not adding CSP in this commit — the site's heavy iframe + inline-script + GA + Google Fonts mix needs a careful policy and a misconfigured CSP would break everything. Filed as a follow-up.
requestFullscreen / exitFullscreen are spec'd to require their `this` to be the element or document respectively. The previous `(obj[method] as () => ...)()` invocation called them as free functions, which works in some browsers but fails per spec and has been observed to silently no-op in stricter engines. Switch to `.call(obj)` and add a test that inspects mock.contexts[0] to prove the binding is preserved.
parseInt("auto") returns NaN, so on the first drag of an element
that never had top/left explicitly set, offsetX/offsetY became NaN
and the element snapped to "NaNpx" — which browsers treat as 0,
visually teleporting it to (0,0) before normal drag math resumed.
Coalesce to 0 with `|| 0` so the first drag honors the element's
on-screen starting point.
randomIntBetween(min, max) was implemented as exclusive of max (Math.floor(rand * (max - min)) + min), which contradicts both the function's name and the docstring at every call site. The randomization interval comment was "2-12s" but the function actually returned 2..11s. Switch to inclusive on both ends and add tests that hit every bound across 1000 samples to catch any future drift. Visual impact at call sites: rotation interval now actually reaches 12s, and ErrorUI font sizes now actually reach 100px.
document.onkeydown / document.onkeyup property assignment clobbers any other handler on the same property. addEventListener composes, which leaves room for future handlers (or third-party scripts that ad-hoc attach) without silently breaking each other.
Playlist.current() previously warned and reset currentIndex to 0 without confirming the playlist itself was valid. If both the index *and* the playlist were stale, this would return undefined-flavored garbage on the next read. Regenerate the playlist on out-of-bounds so current() always returns a valid item. (Audit also flagged a perceived duplicate-insertion bug in setCurrentVirus; on re-read it's actually fine — the splice path is only reached when indexOf returns -1, so a duplicate can't appear. No change there.)
viruses/error-like-text and viruses/static-uzumaki are not registered in Playlist.viruses and have no external references. They were still shipping in the build because Vite globs viruses/*/index.html. static-uzumaki lacks an index.html entry, so it was never even buildable on its own — purely dead SCSS+TS.
teleportMenu compared menu.style.inset to a list of formatted shorthand strings. Browsers normalize shorthand-inset serialization differently (Chrome vs Safari), so the indexOf could miss and the function might pick the same corner it was already in — defeating the whole "teleport elsewhere" idea. Track the current corner by index. Pick uniformly from the other three on each call. Same behavior on first call, predictable behavior thereafter.
Replaces .sort(() => 0.5 - Math.random()) with the project's
Fisher-Yates shuffle (utils/misc.ts). Sort-based shuffles are
biased and aren't even sortable comparators; for a 15-char title
the visual effect is unchanged but the implementation is now correct.
shuffleTitle now returns { interval, stop } where stop both clears
the timer AND restores the original document.title. main.ts calls
stop() in beforeunload so the title doesn't get persisted to
browser history in a scrambled state.
The overlay previously constructed its own Playlist instance, which re-read localStorage and re-shuffled. This both wasted work and meant mixes saved in the lab could be invisible to the overlay until a full page reload (depending on which instance was queried). Pass the live Playlist via options. floating-buttons now also calls playlist.loadSavedMixes() before opening so freshly-saved mixes are guaranteed to appear. Adds a regression test that mutates testPlaylist.savedMixes after playlist construction and asserts the new mix renders in the overlay's thumbnail grid.
The gallery rendered 15+ virus iframes simultaneously, each booting its own three.js/rapier scene. On mobile or cold caches this thrashes hard. Switch iframes to data-src and assign real src only when the thumbnail enters the overlay's viewport (with a 100px root margin so near-offscreen items still pre-warm). Falls back to immediate src assignment when IntersectionObserver is unavailable (e.g., some test environments). Test simulates an IO callback and asserts iframes have no src until intersection, then get their data-src promoted to src.
The crane-game virus loads ~2MB of rapier3d WASM and can easily exceed 5s on cold cache or slow networks, producing a stream of spurious "safety timeout" Sentry warnings. 12s still catches genuinely-stuck loads while accommodating the slowest virus in the playlist on realistic mobile conditions.
startRandomization() already clears loadRandomInterval as its first step, so the preceding clearInterval call is dead.
- Delete window.TVStaticLoading global (no consumer; verified via grep). Stops leaking the class onto the window object. - Drop unused TVStaticLoading import from main.ts. - Wire VirusLab.cleanup() into VirusLoader.toggleLab close path so the well-named teardown runs (event listeners + iframes get tidied before the container is removed). Also updates VirusLoader tests to use the bumped 12s safety timeout (carried over from the prior commit's runtime change).
Empty file, tracked, redundant alongside tsconfig.json.
include: ["."] pulled dist/, coverage/, .superpowers/, node_modules/ into the typecheck universe until excludes kicked in. Switch to an explicit allowlist of source dirs (.ts only) and add dist + coverage to exclude. Also fixes a typecheck error that surfaced once dist/ wasn't being scanned: the fullscreen test's `delete doc.requestFullscreen` was flagged because TS sees the lib property as required. Restore via assignment instead.
vite.config.ts and vitest.config.ts each had their own inline copy of a handlebars .hbs loader. The two had already drifted — the Vite copy escaped <, >, / and the line/paragraph separators; the Vitest copy only escaped the separators. Extracting both to build-utils/handlebars-loader.ts means they stay in sync and the safer escape set is used everywhere.
Three test cases used (lab as any).saveMix() / .deleteMix() with disable-comments for half a dozen lint rules. Replace with an internals() helper that double-casts through unknown to a typed interface — same runtime behavior, cleaner test code, no eslint-disable comments. Intersection with VirusLab itself collapses to never because TS can't widen private to public, hence the double-cast.
Amplify pins node 20 and CI runs node 22, but local dev had no version gate. Add engines.node: ">=20" so devs on older node get a clear warning at install time.
Centralizes LOAD_SAFETY_TIMEOUT_MS, MIN_LOAD_ANIMATION_MS,
NAVIGATION_LOCK_MS, RANDOMIZATION_{MIN,MAX}_S, FLASH_INTERVAL_MS,
FLASH_STOP_DELAY_MS. Wires VirusLoader and Flash through them.
Other magic numbers live inside individual virus animations and are
out of scope for this PR — those tend to be aesthetic tuning rather
than control-flow timing.
Loading/Skipping/Reloading logs and the TVStaticLoading watermark "fortune" log shipped to production, polluting users' devtools every 2-12s. Wrap each in `if (import.meta.env.DEV)` so they remain useful during development but stay quiet in the bundled site. Keeps the ASCII-art banner in main.ts — that's intentional branding.
README now mentions the /viruses/<name> -> /?virus=<name> dev-server redirect and the lab allowlist that mirrors Playlist.viruses. The "add a new virus" steps now include updating the lab allowlist.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
CodeQL flagged the lazy-load assignment `iframe.src = src` (where
`src` came from `iframe.getAttribute('data-src')`) as "DOM text
reinterpreted as HTML." Even though the data-src values originate
from a hardcoded `Playlist.viruses` array, the static analyzer
can't prove that, and the indirection through a DOM attribute
means a future change (or DOM tampering) could pivot to a
`javascript:` URL or a same-origin path-traversal.
Add a strict regex check (`/^\/viruses\/[a-z][a-z0-9-]*\/$/`) and
drop anything that doesn't match before assigning. Removes the
attribute either way so we never re-attempt a tampered value.
Add a regression test that poisons `playlist.viruses` and asserts
only the well-formed entry gets a src.
There was a problem hiding this comment.
Pull request overview
This PR performs a broad audit sweep across the repo, focused on hardening client-side security (XSS / iframe navigation), fixing a few correctness issues (fullscreen binding, random bounds, drag NaN), improving runtime performance (lazy iframe thumbnails), and cleaning up build/test plumbing (shared Handlebars loader, centralized constants, tighter TS config, removed orphaned viruses).
Changes:
- Security hardening: DOM-XSS fix in VirusLab saved mixes; lab iframe allowlist validation; baseline security headers; stop tracking
.env. - Correctness fixes: inclusive
randomIntBetween, draggable NaN guard, fullscreenthisbinding, playlist regeneration on OOB, safer event wiring. - Performance/cleanup: lazy-load thumbnail iframes via
IntersectionObserver, shared Handlebars loader, centralized constants, removed unused/orphan code.
Reviewed changes
Copilot reviewed 35 out of 38 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
vitest.config.ts |
Use shared Handlebars loader plugin. |
vite.config.ts |
Use shared Handlebars loader plugin. |
build-utils/handlebars-loader.ts |
New shared .hbs loader implementation. |
viruses/lab/index.html |
Add allowlist + param validation for iframe src. |
viruses/lab/__tests__/lab-allowlist.test.ts |
Drift test enforcing allowlist sync with Playlist. |
components/VirusLab.ts |
Replace innerHTML mix list rendering with DOM APIs. |
components/__tests__/VirusLab.test.ts |
Add XSS regression + typed internals helper. |
components/VirusThumbnailOverlay.ts |
Require live Playlist; lazy-load thumbnail iframes. |
components/__tests__/VirusThumbnailOverlay.test.ts |
Tests for live playlist + lazy-load + safe path. |
ui/floating-buttons.ts |
Pass live playlist into thumbnail overlay; refresh mixes. |
ui/menu.ts |
Robust menu teleporting; Fisher–Yates title shuffle handle. |
ui/__tests__/menu.test.ts |
Update tests for shuffleTitle handle + stop restore. |
ui/fullscreen.ts |
Fix fullscreen method invocation this binding. |
ui/__tests__/fullscreen.test.ts |
Add assertion that this is correct for fullscreen call. |
main.ts |
Switch to addEventListener; title shuffle stop on unload; remove unused global. |
utils/random.ts |
Make randomIntBetween inclusive of max. |
utils/__tests__/random.test.ts |
Add tests for inclusive bounds + min===max. |
utils/misc.ts |
Draggable NaN guard; expose shuffle used by UI. |
utils/__tests__/misc.test.ts |
Add draggable regression test for auto positioning. |
components/VirusLoader.ts |
Centralize constants; gate logs; wire lab cleanup; adjust timeouts. |
components/__tests__/VirusLoader.test.ts |
Update safety-timeout expectations for new timeout. |
components/flash/flash.ts |
Use centralized flash timing constants. |
components/constants.ts |
New shared constants for loader/flash/randomization. |
components/TVStaticLoading.ts |
Gate console logging behind DEV. |
components/Playlist.ts |
Regenerate playlist on OOB currentIndex; minor cleanup. |
components/__tests__/Playlist.test.ts |
Update test to assert regeneration behavior. |
customHttp.yml |
Add baseline security headers. |
tsconfig.json |
Tighten includes/excludes for TS checking. |
package.json |
Declare Node engine >=20. |
README.md |
Document routing redirect and lab allowlist rules. |
.gitignore |
Ignore .env. |
.env |
Remove tracked .env file. |
jsconfig.json |
Remove empty JS config. |
viruses/error-like-text/index.html |
Delete orphan virus (not in playlist). |
viruses/error-like-text/error-like-text.ts |
Delete orphan virus implementation. |
viruses/error-like-text/error-like-text.scss |
Delete orphan virus styles. |
viruses/static-uzumaki/static-uzumaki.ts |
Delete orphan virus implementation. |
viruses/static-uzumaki/static-uzumaki.scss |
Delete orphan virus styles. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+5
to
6
| /** Returns a random integer in [min, max] (inclusive of both bounds). */ | ||
| export function randomIntBetween(min: number, max: number): number { |
| const deleteButton = document.createElement('button'); | ||
| deleteButton.className = 'delete-mix'; | ||
| deleteButton.textContent = 'Delete'; | ||
| deleteButton.addEventListener('click', () => this.deleteMix(mix.id!)); |
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
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.
Summary
Comprehensive repo audit followed by 25 commits across 4 categories. Stakes are low (personal art project) but this PR ships real security fixes, fixes a few bugs that have been quietly wrong, and tightens build/test plumbing.
Security
VirusLab.updateSavedMixesList—mix.namefrom localStorage was interpolated intoinnerHTML. Switched tocreateElement+textContent. Regression test included.viruses/lab/index.htmlnow validates?primary=/?secondary=against an allowlist before assigningiframe.src. Path-traversal payloads like../../foono longer reach the iframe. Drift test enforces sync withPlaylist.viruses..envuntracked — committed.envonly held a public Sentry DSN, but the pattern is wrong. Added to.gitignoreand removed from tracking. Amplify rebuilds it from CI env at deploy time.customHttp.yml:Referrer-Policy,X-Content-Type-Options,X-Frame-Options. (CSP intentionally deferred — the iframe + inline-script mix needs careful policy work.)Correctness
thisbinding —obj[method]()lostthisforrequestFullscreen/exitFullscreen. Switched to.call(obj)and added amock.contexts[0]assertion test.draggableNaN guard — first drag with computedtop: autono longer teleports the element to (0, 0).randomIntBetweennow inclusive ofmax— was off-by-one. Rotation interval now actually reaches 12s, matching docstring. Test asserts both bounds appear across 1000 samples.Playlist.current()regenerates the playlist on out-of-bounds rather than silently resettingcurrentIndex.document.onkeydown->addEventListener— property assignment was clobbering global handlers.teleportMenutracks position by index, not fragile string-match onstyle.insetshorthand.shuffleTitlenow uses proper Fisher-Yates and returns a{ interval, stop }handle;stop()restores the original title onbeforeunload.viruses/error-like-textandviruses/static-uzumakiweren't inPlaylist.virusesbut still shipped to prod.Performance
IntersectionObserver(with fallback for environments without IO). Previously booted ~15 three.js/rapier scenes on overlay open.Playlistinstead of constructing its own. Saved mixes now appear without a reload.safetyTimeoutbumped 5s -> 12s to stop spurious Sentry warnings oncrane-gamecold WASM loads.Cleanup
jsconfig.json(empty) deleted;tsconfig.jsonincludetightened.build-utils/handlebars-loader.ts(was duplicated betweenvite.config.tsandvitest.config.ts, with already-drifting escape logic).components/constants.ts.console.logs gated behindimport.meta.env.DEV.as anycasts replaced with a typedinternals()helper.engines.node: ">=20"added topackage.json.window.TVStaticLoadingglobal and unused imports removed.VirusLab.cleanup()is now actually wired into the lab-close path.Test plan
yarn lint— cleanyarn format:check— cleannpx tsc --noEmit— cleanyarn test— 221 tests passing across 17 files (incl. new XSS, lab allowlist drift, fullscreen this-binding, draggable NaN, randomIntBetween bounds, lazy-load IO, live-playlist regression tests). Coverage thresholds still met.yarn build— builds cleanyarn preview:localStorage.savedVirusMixes, open lab, confirm no script execution/viruses/lab/?primary=..%2F&secondary=sphere, confirm "invalid mix" fallbacksrcuntil visibleOut of scope (filed as follow-ups)
prefers-reduced-motionhonored inside virus animations