Fix security issues found in review - #20
Merged
Merged
Conversation
- Add URL scheme allowlist (safeUrl) to block javascript:/data: XSS - Escape unescaped year interpolations - Add SRI hashes + crossorigin to CDN scripts - Add CSP and hardening headers to SWA config - Validate slugs against path traversal in generator Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Azure Static Web Apps: Your stage site is ready! Visit it here: https://blue-rock-0e6a0831e-20.westus2.7.azurestaticapps.net |
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.
Security review of the application surfaced five issues; this PR fixes all of them.
Findings & fixes
1. DOM-XSS via
javascript:/data:URLs (Medium/High) —escapeAttronly quote-escaped URL attributes, never validated the scheme. Content is LLM-generated from untrusted GitHub/Wikipedia sources, so ajavascript:URL could become stored XSS. Added asafeUrl()helper (strips control-char/whitespace obfuscation, allowlistshttp/https/mailto+ relative/hash links) and wrapped all six URL sinks (image_url×2,wikipedia_url,github_url×2, summary link).2. Unescaped
yearinterpolation (Low) — wrapped the three raw${...year}sinks inescapeHtml()(the YAML parser can return a non-numeric string).3. CDN scripts without SRI (Medium, supply chain) — added
integrity(sha384) +crossoriginto all four jsdelivr highlight.js scripts.4. Missing security headers (Low) — added
Content-Security-Policy,X-Content-Type-Options: nosniff, andX-Frame-Options: DENYtostaticwebapp.config.json.5. Generator path traversal (Low) —
Checkpointer.path_fornow validates slugs against^[a-z0-9][a-z0-9-]*$and asserts the resolved path stays under the output root.Verification
node --checkonapp.js, JSON parse of the SWA config, and slug-compliance check all pass.safeUrl()unit-tested againstjavascript:,java\tscript:, leading-space,data:,vbscript:(all blocked) and valid http/hash/relative URLs (all pass).Notes
'unsafe-inline'inscript-srcbecause the SPA uses inlineonclick=handlers, so CSP alone wouldn't blockjavascript:URIs — that's why Add assembly code stepper/simulator #1 fixes it at the source. Refactoring handlers toaddEventListenerlater would allow dropping'unsafe-inline'.🤖 Generated with Claude Code