fix(security): complete #183 — XSS output encoding + legacy AI key purge#188
Merged
Conversation
Two residual issues from #183's merge, found in post-merge review: - toEmbedUrl() validated the host via new URL() but returned the raw input, so an attribute-injection payload on an allowed host (e.g. https://www.youtube.com/embed/abc" onload="alert(1)) passed validation and kept its literal double-quote, breaking out of the iframe src="..." attribute. Now returns parsed.href, which percent-encodes quotes, spaces, and brackets. - The switch to in-memory key storage removed the localStorage write but never purged keys that older builds had already stored, so existing users kept a plaintext ai_api_key indefinitely. Added a one-time purge on module load (not read into memory) and in clear(). Both fixes ship with regression tests confirmed to fail against the pre-fix code and pass against the fix. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
m-khan-97
requested review from
SHAURYAKSHARMA24,
Vishnu2707 and
vogonPrayas
as code owners
July 14, 2026 20:40
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Vishnu2707
approved these changes
Jul 15, 2026
Vishnu2707
left a comment
Vishnu2707
left a comment
Member
There was a problem hiding this comment.
Thanks for the fixes. LGTM!
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
2 participants
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.
Follow-up to #183, addressing two residual issues @ritiksah141 flagged in review after it merged. Both are real; neither warranted reverting #183, so this patches forward.
1. XSS —
toEmbedUrl()returned the raw input, not the validated URLwebsite/script.js's embed fallback correctly validated the host vianew URL(raw)against an allowlist, but then returnedraw— the unmodified input. On an allowed host, an attribute-injection payload still passes:hostnameiswww.youtube.com(passes the allowlist), but the literal"survives and breaks out of the iframesrc="..."attribute when interpolated. The earlier "rejected" tests only covered disallowed hosts, so this class was never exercised.Fix: return
parsed.hrefinstead ofraw. The WHATWG URL serializer percent-encodes quotes, spaces, and angle brackets, so the returned value is safe to interpolate. Legitimate embed URLs are unaffected (they contain no characters that get encoded).2. Legacy plaintext key never purged (#180 regression)
The in-memory-key switch removed the
localStorage.setItem('ai_api_key', ...)write but never removed keys older builds had already persisted. Any user who configured a key before that build keeps a plaintextai_api_keyin localStorage indefinitely — the exact leak #180 set out to close.Fix:
localStorage.removeItem('ai_api_key')once on module load (deliberately not read into memory) and again inclear()as defence-in-depth.Verification
devcode before the fix and pass after (not just added green).node website/test_toEmbedUrl.mjs— passes (2 new attribute-injection-on-allowed-host cases)node frontend/src/utils/aiApi.test.mjs— passes (3 new legacy-key migration cases)npm run lint— 0 errors (4 pre-existing warnings in untouched files)Note
This is the miss I flagged in the #183 post-merge review: my approval checked the host-validation gate but not the value flowing out of it. These tests close that specific gap. cc @ritiksah141 @Vishnu2707