Repository navigation
fix(security): prevent XSS, reverse tabnabbing, and zip bomb attacks - #52
Conversation
Add DOMPurify sanitization to strip dangerous HTML/JavaScript from rendered markdown. This prevents multiple XSS attack vectors: - Script tag injection: <script>alert()</script> - Event handler injection: onerror, onclick, onload, etc. - JavaScript protocol links: javascript: URLs in raw HTML - Markdown links refuse javascript: by default (markdown-it security) Add comprehensive security tests covering all attack vectors. Both main render path and error fallback path are now sanitized. Fixes: XSS vulnerability in HTML rendering (Severity: High)
…h conventional commit naming
- Add rel="noopener noreferrer" to external links via DOMPurify hook to prevent reverse tabnabbing attacks - Implement streaming decompression with size limits (2MB max) to prevent zip bomb attacks in document sharing - Add comprehensive security tests for Mermaid XSS vectors and link rel attributes - Replace gunzipSync with streaming Gunzip for memory-safe decompression
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded@alexanderop has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 54 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
WalkthroughAdds documentation for Claude commands and Vue composable testing; integrates DOMPurify sanitization into markdown rendering (including noopener handling); implements streaming gzip decompression with a 2MB decompressed-size limit for shared URLs; and adds tests validating sanitization and size-limited import behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Renderer as Markdown Renderer
participant DOMPurify as DOMPurify
participant Output as App Output
User->>Renderer: Submit raw Markdown
Renderer->>Renderer: Convert Markdown -> HTML
Renderer->>DOMPurify: Sanitize HTML (after-sanitize hook)
Note over DOMPurify: Strip scripts/unsafe attrs<br/>Add rel="noopener noreferrer" for external links
DOMPurify->>Renderer: Return cleaned HTML
Renderer->>Output: Render sanitized HTML
sequenceDiagram
participant Browser
participant Parser as Share URL Parser
participant Gunzip as safeGunzip
participant Validator as Size Validator
participant App as Application
Browser->>Parser: `#share`=<base64+gzip>
Parser->>Gunzip: Stream and decompress
Gunzip->>Validator: Emit chunks / track size
alt Decompressed size > 2MB
Validator->>App: Error — import rejected (too large)
else Size within limit
Validator->>App: Return decompressed JSON -> parse & import
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/modules/markdown-preview/composables/useMarkdown.ts (2)
2-27: DOMPurify hook for reverse tabnabbing is correct but could be slightly more robustThe
afterSanitizeAttributeshook that addsrel="noopener noreferrer"to externalhttp/httpslinks is sound and idempotent (it merges with existingrelvalues and avoids duplicates). Two small robustness nits you might consider:
- Normalize
tagNamewhen checking anchors, e.g.node.tagName.toUpperCase() === 'A', to be resilient across environments.- Optionally normalize
hrefwhen checking protocol (href.toLowerCase().startsWith('http://')/https://) in case user-supplied HTML uses uppercased schemes.Not required for correctness in typical browser/jsdom cases, but would harden the hook against odd markup.
If you’d like, please double-check the
afterSanitizeAttributeshook semantics in the currentisomorphic-dompurify/DOMPurify docs to confirm this usage matches recommended patterns.
40-58: Sanitization is correctly applied on both primary and fallback render pathsThe main path now sanitizes rendered HTML before adding test IDs, and the fallback path sanitizes basic markdown-it output as well. This closes the XSS gap even when the primary renderer fails.
One minor consistency idea: if you ever rely on GitHub alert
data-testidattributes in fallback scenarios, you might also runaddDataTestIdToAlertson the fallback HTML; currently only the primary path does so.If you update fallback behavior, please ensure it still aligns with how
createMarkdownRendererformats GitHub alerts so tests remain consistent.src/modules/share/composables/useDocumentShare.ts (1)
11-79: safeGunzip correctly enforces a decompressed size cap, with minor internal cleanups possibleThe new
safeGunziphelper cleanly enforces a ~2 MiB decompressed size limit and avoids accumulating unbounded data in memory, which is a solid mitigation for zip‑bomb style inputs. A couple of small points you might consider:
Constructor callback redundancy
You both pass a callback to
new Gunzip((chunk) => { ... })and then immediately assigngunzip.ondata = (chunk) => { ... }. Per fflate’s typings, the constructor callback is equivalent to settingondata, so the initial callback becomes unused once you overwrite it. You can simplify to:const gunzip = new Gunzip() gunzip.ondata = (chunk) => { if (aborted) return totalSize += chunk.length if (totalSize > MAX_DECOMPRESSED_SIZE) { aborted = true return } chunks.push(chunk) }Stronger early‑abort semantics (optional)
Currently, when
totalSizeexceeds the limit, you setaborted = trueand stop accumulating chunks, butgunzip.pushwill still continue processing the remaining compressed data. If you want to truly stop decompression work once the limit is exceeded, you could insteadthrowfrom insideondatawhen the limit is hit and rely on the surroundingtryCatchto capture that as an error. That would cap both memory use and decompression CPU for extreme zip‑bomb inputs.The existing implementation is functionally correct and already prevents unbounded decompressed storage; these changes would just tighten and simplify the implementation.
Please double-check against the current
fflatestreaming docs that throwing insideGunzip.ondatais the recommended way to abort decompression early before adopting that pattern.src/modules/share/composables/useDocumentShare.test.ts (1)
20-57: Test logic is correct and properly validates zip bomb protection.The test correctly creates an oversized document (2.1MB > 2MB limit), compresses it, and verifies rejection with an appropriate error message. The use of dynamic imports ensures proper test isolation.
Consider adding boundary tests for the exact 2MB limit.
To more thoroughly validate the size enforcement, consider adding tests for:
- Exactly 2,097,152 bytes (2MB)
- 2,097,151 bytes (1 byte under)
- 2,097,153 bytes (1 byte over)
This would help catch off-by-one errors in the size limit check.
Example:
it('should reject documents at exactly 2MB', async () => { const EXACTLY_TWO_MB = 2 * 1024 * 1024 // 2,097,152 bytes const doc = { id: 'test', title: 'Test', content: 'a'.repeat(EXACTLY_TWO_MB - 100), // Account for JSON overhead createdAt: Date.now(), sharedAt: Date.now(), } // ... rest of test })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
.claude/commands/branch.md(1 hunks).claude/skills/vue-test-composable/references/examples.md(1 hunks).claude/skills/vue-test-composable/references/helpers.md(1 hunks).claude/skills/vue-test-composable/skill.md(1 hunks)package.json(1 hunks)src/modules/markdown-preview/composables/useMarkdown.test.ts(1 hunks)src/modules/markdown-preview/composables/useMarkdown.ts(3 hunks)src/modules/share/composables/useDocumentShare.test.ts(1 hunks)src/modules/share/composables/useDocumentShare.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
src/modules/share/composables/useDocumentShare.ts (1)
src/shared/utils/result.ts (4)
Result(6-8)tryCatch(66-75)Err(20-22)Ok(13-15)
src/modules/markdown-preview/composables/useMarkdown.test.ts (1)
src/modules/markdown-preview/composables/useMarkdown.ts (1)
useMarkdown(29-70)
src/modules/markdown-preview/composables/useMarkdown.ts (1)
src/modules/markdown-preview/utils/markdown.ts (1)
addDataTestIdToAlerts(50-56)
src/modules/share/composables/useDocumentShare.test.ts (1)
src/modules/share/composables/useDocumentShare.ts (1)
useDocumentShare(97-356)
🪛 LanguageTool
.claude/commands/branch.md
[style] ~20-~20: Using many exclamation marks might seem excessive (in this case: 3 exclamation marks for a text that’s 812 characters long)
Context: ...t </current_branch> <recent_branches> !git branch --sort=-committerdate | head...
(EN_EXCESSIVE_EXCLAMATION)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: E2E Tests (sharing)
- GitHub Check: E2E Tests (color)
- GitHub Check: E2E Tests (editing)
- GitHub Check: E2E Tests (accessibility)
- GitHub Check: E2E Tests (theme)
- GitHub Check: E2E Tests (document-persistence)
- GitHub Check: E2E Tests (documents)
- GitHub Check: E2E Tests (smoke.test)
🔇 Additional comments (7)
.claude/commands/branch.md (1)
1-57: Branch command doc looks clear and consistentFlow, git commands, and branch naming rules are coherent; no issues from a tooling or usability perspective.
.claude/skills/vue-test-composable/skill.md (1)
1-38: Vue composable testing guide reads solidThe three-path breakdown (independent / lifecycle / injection) and references to helpers/examples are clear and actionable; no issues spotted.
src/modules/markdown-preview/composables/useMarkdown.test.ts (1)
1-142: Security test coverage for markdown rendering looks strongThe suite exercises reverse tabnabbing, script/event-handler stripping, javascript: URLs, and Mermaid-specific XSS vectors, while still validating safe HTML passthrough. This provides good regression coverage for the new sanitization behavior.
src/modules/share/composables/useDocumentShare.ts (1)
207-263: parseShareUrl integration with safeGunzip and error messaging looks correctThe share URL parsing flow now:
- Validates
#share=prefix and presence of encoded data.- Base64‑decodes into a
Uint8Array.- Uses
safeGunzipand surfaces either its error message (including “Document is too large to process safely.”) or proceeds to JSON decode + schema validation.This keeps the existing user‑facing error semantics while adding the decompressed‑size guard, and cleanly reuses the common
Result/tryCatchpattern already used elsewhere in the module.If you later tweak
MAX_DECOMPRESSED_SIZE, it’s worth re‑running the new zip‑bomb tests to ensure the thresholds and messages still line up with expectations.package.json (1)
50-79: DOM sanitization dependency safe; verify transitive DOMPurify version
isomorphic-dompurify@^2.32.0itself has no known security advisories. However, multiple security advisories exist for the upstream DOMPurify project, including nesting-based mXSS (CVE-2024-47875), prototype-pollution tampering (CVE-2024-48910), and depth-check bypass (CVE-2024-45801).Check which DOMPurify version is actually installed when you depend on isomorphic-dompurify@^2.32.0 (inspect node_modules or your lockfile) and ensure it resolves to patched releases (DOMPurify 2.5.4 / 3.1.3 for the mXSS and depth issues, and 2.4.2 for the prototype-pollution issue).
src/modules/share/composables/useDocumentShare.test.ts (2)
1-18: LGTM! Excellent test isolation setup.The test suite properly isolates state with
vi.resetModules()and restoreswindow.locationin cleanup. This ensures each test gets a fresh module instance with its own mocked environment.
59-93: LGTM! Test correctly validates the success path.The test properly verifies that small, valid documents are accepted and parsed correctly. The assertions check all critical aspects: non-null result, no error, and correct content extraction.
Together with the first test, these provide good coverage of the security requirement to reject oversized documents while accepting valid ones.
…iled instructions
The constructor callback was immediately overwritten by the ondata assignment, making it dead code. Simplified to only use ondata.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
.claude/commands/pr.md (1)
84-90: Add language specifier to the YAML frontmatter code block.Line 86 lacks a language identifier on the opening fence. Since this block documents YAML frontmatter, specify
yaml:-``` +```yaml --- description: <imperative verb phrase, e.g., "Run tests and fix failures"> argument-hint: <show expected args, use [] for optional, <> for required> allowed-tools: <see category defaults below> model: <haiku for routine operations, omit for complex reasoning> --- -``` +```yaml.claude/commands/create-command.md (3)
188-192: Add language specifier to the markdown example code block.Line 191 lacks a language identifier. Since this block contains markdown template content, specify
markdown:**Linear (sequential steps):** -```markdown +```markdown # <Title> ## Context
285-289: Add language specifier to the markdown example code block.Line 288 lacks a language identifier. Since this block contains markdown template content, specify
markdown:**Complex (full protocol):** -```markdown +```markdown # <Title> ## Context
383-387: Add language specifier to the markdown example code block.Line 386 lacks a language identifier for the example output block. Specify
markdown:For `/create-command deploy "Deploy to staging or production environment"`: -```markdown +```markdown --- description: Deploy to staging or production environment argument-hint: <environment> [version]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.claude/commands/create-command.md(1 hunks).claude/commands/pr.md(1 hunks)src/modules/share/composables/useDocumentShare.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-22T10:05:40.365Z
Learnt from: alexanderop
Repo: alexanderop/MarkVim PR: 51
File: .claude/commands/lint.md:12-12
Timestamp: 2025-11-22T10:05:40.365Z
Learning: In `.claude/commands/` files (Claude Code slash command templates), the syntax `!\`command\`` is intentional and valid. The `!` prefix executes the shell command and interpolates its output into the template. This is documented Claude Code behavior and should not be flagged as malformed syntax.
Applied to files:
.claude/commands/create-command.md
🧬 Code graph analysis (1)
src/modules/share/composables/useDocumentShare.ts (1)
src/shared/utils/result.ts (4)
Result(6-8)tryCatch(66-75)Err(20-22)Ok(13-15)
🪛 LanguageTool
.claude/commands/pr.md
[style] ~35-~35: Using many exclamation marks might seem excessive (in this case: 7 exclamation marks for a text that’s 1807 characters long)
Context: ...e-only </files_changed> <existing_pr> !gh pr view --json number,title,state,ur...
(EN_EXCESSIVE_EXCLAMATION)
.claude/commands/create-command.md
[uncategorized] ~25-~25: The official name of this software platform is spelled with a capital “H”.
Context: ... merge) | model: haiku, git tools | | github | GitHub operations (PR, issues, CI) |...
(GITHUB)
[uncategorized] ~79-~79: The official name of this software platform is spelled with a capital “H”.
Context: ...sh(git branch), Bash(git checkout)| |github|Bash(gh *), Bash(git *)| |quali...
(GITHUB)
[uncategorized] ~86-~86: The official name of this software platform is spelled with a capital “H”.
Context: ...- Use model: haiku for: git-workflow, github, quality (simple), utility (simple) - O...
(GITHUB)
🪛 markdownlint-cli2 (0.18.1)
.claude/commands/pr.md
86-86: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
.claude/commands/create-command.md
191-191: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
288-288: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
386-386: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: E2E Tests (color)
- GitHub Check: E2E Tests (sharing)
- GitHub Check: E2E Tests (document-persistence)
- GitHub Check: E2E Tests (theme)
- GitHub Check: E2E Tests (documents)
- GitHub Check: E2E Tests (accessibility)
- GitHub Check: E2E Tests (smoke.test)
- GitHub Check: E2E Tests (editing)
🔇 Additional comments (6)
.claude/commands/pr.md (1)
1-121: Documentation file is scaffolding infrastructure, not security-related code.This file documents PR creation workflows and is part of the project's tooling documentation. It doesn't implement or validate the security hardening changes (XSS prevention, reverse tabnabbing, zip bomb protection) mentioned in the PR objectives. The documentation itself is well-structured and clear.
.claude/commands/create-command.md (1)
1-386: Documentation file is scaffolding infrastructure, not security-related code.This file provides templates and patterns for creating new Claude slash commands and is part of the project's tooling infrastructure. It does not implement or validate the security hardening changes (XSS prevention via DOMPurify, reverse tabnabbing protection, or zip bomb defense) mentioned in the PR objectives.
src/modules/share/composables/useDocumentShare.ts (4)
3-3: LGTM! Type and module imports are correct.The Result type import and Gunzip addition to fflate imports properly support the new streaming decompression implementation.
Also applies to: 6-6
11-13: LGTM! Security constants are well-defined.The 2MB decompression limit provides effective zip bomb protection, and using 1024 bytes per KB for binary calculations is correct. The distinction from BYTES_TO_KB_DIVISOR (1000, line 105) for user-facing display is intentional and appropriate.
217-224: LGTM! Clean integration of safe decompression.The Result-based error handling is properly integrated, with appropriate error messaging and early return on failure. The replacement of direct gunzipSync with streaming safeGunzip successfully implements zip bomb protection at the entry point.
20-67: The code is correctly implemented for the synchronous Gunzip API.fflate's synchronous Gunzip throws errors directly from push(), so the existing
tryCatchwrapper aroundgunzip.push()is the correct approach for error handling. The suggestedonerrorhandler is unnecessary for this synchronous API variant.The streaming decompression with incremental size checking and Result-based error handling is well-structured and effectively prevents zip bomb attacks.
- Close code block properly before Lifecycle Dependent Example section - Add proper ## headings for Lifecycle and Injection Dependent examples - Add ## heading for useInjectedSetup section in helpers.md - Fix inline code formatting for inject() reference
Summary
This PR implements comprehensive security hardening for MarkVim, addressing three critical attack vectors: XSS injection in markdown rendering, reverse tabnabbing via external links, and zip bomb attacks in document sharing.
Changes
XSS Prevention: Add DOMPurify sanitization to markdown preview and error fallback paths
Reverse Tabnabbing Protection: Add
rel="noopener noreferrer"to external linksZip Bomb Protection: Implement streaming decompression with size limits
gunzipSyncwith streamingGunzipfor memory-safe decompressionTesting: Add comprehensive security tests
Test Plan
Summary by CodeRabbit
Security & Stability
Tests
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.