Conversation
- Fixed text clipping at the bottom of Mermaid diagrams by resetting `line-height: normal` on the `.mermaid-diagram` container in `style.css`. - Enabled custom HTML formatting (e.g. `<span>`, `<b>`, `<style>`) inside Mermaid nodes by configuring `mermaid.initialize` with `securityLevel: 'loose'` and `htmlLabels: true` in `MermaidService.js`. - Fixed an unrelated path verification bug in `pathHelpers.test.js` to restore test suite to green. - Verified DOMPurify does not incorrectly strip Mermaid payload data during initialization. Co-authored-by: PrakharMNNIT <73683289+PrakharMNNIT@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
📝 WalkthroughWalkthroughThe pull request primarily applies stylistic and formatting improvements across JavaScript files, including brace additions around single-statement blocks and conversion to template literals for log messages. MermaidService adds configuration options for HTML rendering security. A behavioral change in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/js/utils/pathHelpers.js (1)
129-143:⚠️ Potential issue | 🟠 MajorSecurity function
isWithinRootis defined but unused—add path boundary validation before file load.The
isWithinRoot()function is defined insrc/js/utils/pathHelpers.js(lines 129-143) but never imported or called in production code. Meanwhile,LinkNavigationService.resolveTargetPath()resolves relative paths without validating that the result stays within the intended root directory. A user can navigate using links like../../../secret.md, which resolves tosecret.md—if that file exists in the cache (e.g., from a parent folder selection), it will load without boundary checks.Add
import { isWithinRoot } from '../utils/pathHelpers.js';toLinkNavigationServiceand validate the resolved path before loading: callisWithinRoot(targetPath, rootPath)innavigateToMarkdownFile()before the cache check to ensure the target stays within the selected directory scope.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/js/utils/pathHelpers.js` around lines 129 - 143, Import isWithinRoot from '../utils/pathHelpers.js' into LinkNavigationService and call it from within navigateToMarkdownFile() immediately after computing the resolved targetPath (and before checking the cache or loading the file); use the current rootPath (the selected directory scope) as the second argument, and abort/return early (do not load the file) if isWithinRoot(targetPath, rootPath) returns false so path traversal outside the selected root is blocked.
🧹 Nitpick comments (3)
script.js (1)
2598-2598: Minor: Inconsistent spacing in template literal.There's a space after
${in the template literal which appears inconsistent with other template literals in this file (e.g., line 176 in LinkNavigationService.js uses#${ anchor}with the same spacing, suggesting this may be intentional formatter behavior).console.log(`[App] Loaded from link: ${fileData.path}${fileData.anchor ? `#${ fileData.anchor}` : ''}`);If this is from an auto-formatter, consider checking the formatter config. If intentional, no action needed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@script.js` at line 2598, Remove the inconsistent space inside the template literal in the console.log statement so the embedded expression uses `#${fileData.anchor}` rather than `#${ fileData.anchor}`; locate the line with the console.log that references fileData.path and fileData.anchor and update the spacing to match the other template literals in the file.src/js/services/LinkNavigationService.js (1)
213-221: Consider addingisWithinRootvalidation here.As noted in pathHelpers.js review,
resolveTargetPath()resolves relative paths without validating they stay within the folder root. Consider importing and usingisWithinRootbefore loading files to prevent navigation outside the intended directory scope.+ import { + isAnchorLink, + isExternalUrl, + isMarkdownFile, + isWithinRoot, + normalizePath, + resolveRelativePath, + } from '../utils/pathHelpers.js'; async navigateToMarkdownFile(relativePath) { // ... existing code ... const targetPath = this.resolveTargetPath(cleanPath); + // Validate path stays within root + if (this.currentFolderHandle && !isWithinRoot(targetPath, '')) { + this.showWarning('Invalid path', 'Cannot navigate outside the current folder.'); + return; + } // Check cache if (this.fileHandleCache.has(targetPath)) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/js/services/LinkNavigationService.js` around lines 213 - 221, The resolveTargetPath function currently resolves relativePath to an absolute path using normalizePath and resolveRelativePath without validating it stays within the root; import and call isWithinRoot (from pathHelpers.js) after computing the normalized target (use normalizePath(resolveRelativePath(this.currentFilePath, relativePath)) or normalizePath(relativePath.slice(1)) as you already do) and if isWithinRoot(target) is false, handle it consistently (e.g., return null or throw a controlled error) to prevent loading/navigating outside the intended root; update callers of resolveTargetPath to handle the new null/error return as needed.tests/unit/services/LinkNavigationService.test.js (1)
321-327: Test verifies path traversal is normalized but doesn't verify it's blocked.The test checks that
..is not in the resolved path after normalization, but this doesn't verify that navigation to files outside the intended root is actually prevented. TheresolveTargetPathmethod normalizes../../../etc/passwdtoetc/passwd, which passes this test but could still be a security issue ifetc/passwdexists in the file cache.Consider adding a test that verifies navigation outside the root directory is blocked, not just normalized.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/services/LinkNavigationService.test.js` around lines 321 - 327, Current test only checks normalization of path traversal via resolveTargetPath but doesn’t assert that traversal is blocked; update the test to assert that navigating to '../../../etc/passwd' is rejected or mapped inside the repo root. Specifically, call service.setCurrentFile('docs/current.md') and service.resolveTargetPath('../../../etc/passwd') and then assert the result either is null/throws or that the returned path is within the service's safe base (e.g., startsWith service.root or service.getRootPath()), and additionally verify that service will not return a file from the cache for that path (e.g., service.getFile or similar) to prove access is blocked. Ensure you reference resolveTargetPath and setCurrentFile in the new assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/js/services/MermaidService.js`:
- Around line 164-165: The Mermaid config currently sets securityLevel: 'loose'
with htmlLabels: true, allowing XSS because the Mermaid source is URI-encoded
before DOMPurify and decoded only when passed to mermaid.render(); change the
config in MermaidService (the object that sets securityLevel/htmlLabels) to use
'strict' or 'sandbox' and/or disable htmlLabels by default, and ensure any
user-provided mermaid source is sanitized before decoding and before calling
mermaid.render() (fix the sanitization order around DOMPurify and the
URI-encoding/decoding logic that prepares input for mermaid.render()).
In `@src/js/utils/pathHelpers.js`:
- Around line 134-140: Remove the duplicate path-traversal check: the second "if
(normalizedPath.includes('../')) {return false;}" is redundant because the
earlier one already returns, so delete the later check (leave the preceding
checks and the surrounding comment intact). Locate the checks using the
normalizedPath variable in the path normalization routine in pathHelpers (the
block containing the initial "if (!normalizedPath) {return false;}" and the
"startsWith('..')" check) and remove the duplicated includes('../') condition.
---
Outside diff comments:
In `@src/js/utils/pathHelpers.js`:
- Around line 129-143: Import isWithinRoot from '../utils/pathHelpers.js' into
LinkNavigationService and call it from within navigateToMarkdownFile()
immediately after computing the resolved targetPath (and before checking the
cache or loading the file); use the current rootPath (the selected directory
scope) as the second argument, and abort/return early (do not load the file) if
isWithinRoot(targetPath, rootPath) returns false so path traversal outside the
selected root is blocked.
---
Nitpick comments:
In `@script.js`:
- Line 2598: Remove the inconsistent space inside the template literal in the
console.log statement so the embedded expression uses `#${fileData.anchor}`
rather than `#${ fileData.anchor}`; locate the line with the console.log that
references fileData.path and fileData.anchor and update the spacing to match the
other template literals in the file.
In `@src/js/services/LinkNavigationService.js`:
- Around line 213-221: The resolveTargetPath function currently resolves
relativePath to an absolute path using normalizePath and resolveRelativePath
without validating it stays within the root; import and call isWithinRoot (from
pathHelpers.js) after computing the normalized target (use
normalizePath(resolveRelativePath(this.currentFilePath, relativePath)) or
normalizePath(relativePath.slice(1)) as you already do) and if
isWithinRoot(target) is false, handle it consistently (e.g., return null or
throw a controlled error) to prevent loading/navigating outside the intended
root; update callers of resolveTargetPath to handle the new null/error return as
needed.
In `@tests/unit/services/LinkNavigationService.test.js`:
- Around line 321-327: Current test only checks normalization of path traversal
via resolveTargetPath but doesn’t assert that traversal is blocked; update the
test to assert that navigating to '../../../etc/passwd' is rejected or mapped
inside the repo root. Specifically, call
service.setCurrentFile('docs/current.md') and
service.resolveTargetPath('../../../etc/passwd') and then assert the result
either is null/throws or that the returned path is within the service's safe
base (e.g., startsWith service.root or service.getRootPath()), and additionally
verify that service will not return a file from the cache for that path (e.g.,
service.getFile or similar) to prove access is blocked. Ensure you reference
resolveTargetPath and setCurrentFile in the new assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8109e0a5-6e98-4a69-9497-47b77e844d12
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (7)
script.jssrc/js/services/LinkNavigationService.jssrc/js/services/MermaidService.jssrc/js/utils/pathHelpers.jsstyle.csstests/unit/services/LinkNavigationService.test.jstests/unit/utils/pathHelpers.test.js
| securityLevel: 'loose', // Allow HTML/style tags inside diagram nodes | ||
| htmlLabels: true, // Force enable HTML rendering |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, locate and examine MermaidService.js
echo "=== Finding MermaidService.js ==="
find . -name "MermaidService.js" -type f 2>/dev/null | head -5
echo ""
echo "=== Checking file size ==="
wc -l src/js/services/MermaidService.js 2>/dev/null || echo "File not found at expected path, searching..."Repository: PrakharMNNIT/markdown-viewer-app
Length of output: 211
🏁 Script executed:
#!/bin/bash
# Examine MermaidService.js around lines 164-165 and show broader context
echo "=== Content of MermaidService.js around lines 164-165 ==="
sed -n '140,190p' src/js/services/MermaidService.js 2>/dev/null || \
cat src/js/services/MermaidService.js 2>/dev/null | head -n 200 | tail -n 70Repository: PrakharMNNIT/markdown-viewer-app
Length of output: 2582
🏁 Script executed:
#!/bin/bash
# Search for the mermaid.render() call and complete flow
echo "=== Search for mermaid.render and related code ==="
rg -n -B5 -A5 'mermaid\.render' src/js/services/MermaidService.js
echo ""
echo "=== Search for DOMPurify usage ==="
rg -n -B3 -A3 'DOMPurify' src/js/services/MermaidService.js
echo ""
echo "=== Search for data-code attribute handling ==="
rg -n -B3 -A3 'data-code|data-mermaid' src/js/services/MermaidService.jsRepository: PrakharMNNIT/markdown-viewer-app
Length of output: 592
🏁 Script executed:
#!/bin/bash
# Check how decoding happens and what's passed to mermaid.render
echo "=== Search for decoding operations ==="
rg -n 'decodeURIComponent|atob|decode' src/js/services/MermaidService.js
echo ""
echo "=== Check the full function that uses these settings ==="
rg -n -B10 "securityLevel.*loose" src/js/services/MermaidService.jsRepository: PrakharMNNIT/markdown-viewer-app
Length of output: 659
🏁 Script executed:
#!/bin/bash
# Get the complete renderMermaid function
echo "=== Full renderMermaid function ==="
rg -n -B5 "renderMermaid" src/js/services/MermaidService.js | head -50Repository: PrakharMNNIT/markdown-viewer-app
Length of output: 111
🏁 Script executed:
#!/bin/bash
# Search for where renderMermaid is called
echo "=== Search for calls to renderMermaid ==="
rg -n "renderMermaid" --type=js | grep -v "MermaidService.js"Repository: PrakharMNNIT/markdown-viewer-app
Length of output: 117
🏁 Script executed:
#!/antml/bash
# Search for DOMPurify usage in entire codebase
echo "=== DOMPurify usage ==="
rg -n "DOMPurify" --type=js -B2 -A2
echo ""
echo "=== marked.js or marked usage ==="
rg -n "marked\(" --type=js -B2 -A2Repository: PrakharMNNIT/markdown-viewer-app
Length of output: 631
🏁 Script executed:
#!/bin/bash
# Look for Mermaid diagram extraction from markdown
echo "=== Search for mermaid code block extraction ==="
rg -n "```mermaid|mermaid-|data-code" --type=js -B2 -A2
echo ""
echo "=== Check the entire renderMermaid function ==="
sed -n '320,360p' src/js/services/MermaidService.jsRepository: PrakharMNNIT/markdown-viewer-app
Length of output: 210
🏁 Script executed:
#!/bin/bash
# Get the full MermaidService.js content (it's only 371 lines)
echo "=== Full MermaidService.js ==="
cat src/js/services/MermaidService.jsRepository: PrakharMNNIT/markdown-viewer-app
Length of output: 14421
🏁 Script executed:
#!/bin/bash
# Look at script.js context around DOMPurify usage
echo "=== Context around DOMPurify.sanitize in script.js ==="
rg -n -B10 -A10 "DOMPurify.sanitize" script.jsRepository: PrakharMNNIT/markdown-viewer-app
Length of output: 1000
🏁 Script executed:
#!/bin/bash
# Search for how Mermaid diagrams are handled in the markdown rendering
echo "=== Search for mermaid in script.js ==="
rg -n "mermaid" script.js | head -30Repository: PrakharMNNIT/markdown-viewer-app
Length of output: 1043
🏁 Script executed:
#!/bin/bash
# Find where data-code is extracted and used
echo "=== Extract from data-code attribute ==="
rg -n -B5 -A5 'data-code|getAttribute.*code|decodeURIComponent.*data' script.jsRepository: PrakharMNNIT/markdown-viewer-app
Length of output: 1402
🏁 Script executed:
#!/bin/bash
# Check the exact code extraction and mermaid.render call
echo "=== Full context of mermaid rendering in script.js ==="
sed -n '1120,1145p' script.jsRepository: PrakharMNNIT/markdown-viewer-app
Length of output: 1225
🏁 Script executed:
#!/bin/bash
# Check if there's any sanitization of Mermaid code before rendering
echo "=== Check for any Mermaid code sanitization ==="
rg -n "sanitize.*code|cleanCode|purify.*code" script.js
echo ""
echo "=== Check DOMPurify full config ==="
sed -n '1099,1120p' script.js
echo ""
echo "=== Check where markdown comes from (user input or trusted) ==="
rg -n "readAsText|FileReader|upload|textarea|contentEditable" script.js | head -20Repository: PrakharMNNIT/markdown-viewer-app
Length of output: 971
🏁 Script executed:
#!/bin/bash
# Check if content is from user input or trusted sources
echo "=== Search for how markdown content is loaded ==="
rg -n "loadMarkdown|renderMarkdown|preview" script.js | head -20
echo ""
echo "=== Check textarea or file input handling ==="
rg -n "document.getElementById.*markdown|document.querySelector.*markdown|file.*input" script.js | head -20Repository: PrakharMNNIT/markdown-viewer-app
Length of output: 1646
🏁 Script executed:
#!/bin/bash
# Check what decodeHtmlEntities does
echo "=== Find decodeHtmlEntities function ==="
rg -n -B2 -A10 "function decodeHtmlEntities|const decodeHtmlEntities|decodeHtmlEntities.*=" script.jsRepository: PrakharMNNIT/markdown-viewer-app
Length of output: 546
🏁 Script executed:
#!/bin/bash
# Check if user input from editor is the markdown content that's rendered
echo "=== Check editor input flow ==="
rg -n -B5 -A5 'editor\.value|renderMarkdown' script.js | grep -A5 -B5 "editor\|renderMarkdown" | head -40Repository: PrakharMNNIT/markdown-viewer-app
Length of output: 1497
🏁 Script executed:
#!/bin/bash
# Final verification: Test if user content is actually rendered
echo "=== Verify user input from editor goes to renderMarkdown ==="
sed -n '1075,1085p' script.js
echo ""
echo "=== Confirm marked.parse is used to process user input ==="
rg -n "marked.parse" script.jsRepository: PrakharMNNIT/markdown-viewer-app
Length of output: 648
🏁 Script executed:
#!/bin/bash
# Verify Mermaid securityLevel documentation
echo "=== Get Mermaid library info ==="
cat package.json | grep -i mermaidRepository: PrakharMNNIT/markdown-viewer-app
Length of output: 134
Critical XSS vulnerability: securityLevel: 'loose' allows script execution in Mermaid diagrams.
Setting securityLevel: 'loose' permits arbitrary HTML and JavaScript execution within Mermaid diagram nodes. The vulnerability exists because Mermaid code is URI-encoded before DOMPurify sanitization, causing the content to bypass sanitization entirely—it's decoded after DOMPurify processes the HTML and passed directly to mermaid.render().
Users can control markdown content via the editor textarea. An attacker can inject:
graph TD
A["<img src=x onerror=alert(document.cookie)>"]
With securityLevel: 'loose' and htmlLabels: true, Mermaid renders the <img> tag and fires the onerror handler.
Recommendation: Keep securityLevel: 'strict' (default) or 'sandbox' unless you have a specific trusted use case. If custom HTML labels are required, sanitize the Mermaid code before passing it to mermaid.render(), or restrict the app to trusted content only.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/js/services/MermaidService.js` around lines 164 - 165, The Mermaid config
currently sets securityLevel: 'loose' with htmlLabels: true, allowing XSS
because the Mermaid source is URI-encoded before DOMPurify and decoded only when
passed to mermaid.render(); change the config in MermaidService (the object that
sets securityLevel/htmlLabels) to use 'strict' or 'sandbox' and/or disable
htmlLabels by default, and ensure any user-provided mermaid source is sanitized
before decoding and before calling mermaid.render() (fix the sanitization order
around DOMPurify and the URI-encoding/decoding logic that prepares input for
mermaid.render()).
| if (!normalizedPath) {return false;} | ||
| if (normalizedPath.startsWith('..')) {return false;} | ||
| if (normalizedPath.includes('../')) {return false;} | ||
|
|
||
| // Security: Detect path traversal attempts that resolve outside root | ||
| // e.g., 'docs/../secret' normalizes to 'secret' which is outside 'docs' | ||
| if (normalizedPath.includes('../')) return false; | ||
| if (normalizedPath.includes('../')) {return false;} |
There was a problem hiding this comment.
Dead code: Duplicate ../ check on line 140.
Line 136 already checks normalizedPath.includes('../'), making line 140 unreachable. If line 136 is true, the function returns false before reaching line 140.
🔧 Remove the duplicate check
// Empty path or trying to escape root
if (!normalizedPath) {return false;}
if (normalizedPath.startsWith('..')) {return false;}
if (normalizedPath.includes('../')) {return false;}
- // Security: Detect path traversal attempts that resolve outside root
- // e.g., 'docs/../secret' normalizes to 'secret' which is outside 'docs'
- if (normalizedPath.includes('../')) {return false;}
-
// Check if path starts with root
return normalizedPath === normalizedRoot || normalizedPath.startsWith(`${normalizedRoot}/`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/js/utils/pathHelpers.js` around lines 134 - 140, Remove the duplicate
path-traversal check: the second "if (normalizedPath.includes('../')) {return
false;}" is redundant because the earlier one already returns, so delete the
later check (leave the preceding checks and the surrounding comment intact).
Locate the checks using the normalizedPath variable in the path normalization
routine in pathHelpers (the block containing the initial "if (!normalizedPath)
{return false;}" and the "startsWith('..')" check) and remove the duplicated
includes('../') condition.
1 participant
This PR addresses two issues with Mermaid diagram rendering in the Markdown Viewer:
line-heightrules. Because the application uses a globalline-height: 1.6, text nodes inside Mermaid diagrams were taller than their calculated bounding boxes, causing them to clip at the bottom. This fix appliesline-height: normallocally to.mermaid-diagramelements, resolving the clipping while preserving the global typographic style.securityLevelis'strict', which strips out any HTML tags or custom inline styles. The user requested the ability to use raw HTML inside Mermaid nodes instead of using GitHub Admonitions/Emojis for formatting. This PR enables custom HTML formatting by configuring Mermaid withsecurityLevel: 'loose'andhtmlLabels: true.Note on Security: The application uses DOMPurify to sanitize markdown output. However, since the raw Mermaid payload is URI-encoded into a
data-codeattribute before DOMPurify runs, the custom HTML inside the Mermaid definitions is safely bypassed during DOMPurify's run and correctly parsed by Mermaid afterwards, meaning we didn't have to weaken the application's overall XSS defenses.PR created automatically by Jules for task 4041251571326236476 started by @PrakharMNNIT
Summary by CodeRabbit
New Features
Bug Fixes
Tests