Fix a bug where markdown files fail to render in the diff view. #1202
Conversation
Motivation:
Prism raises an exception if a markdown file is specified in the commit
diff view. Because the markdown syntax is not imported from
`'prismjs/components/*`.
```
framework-0e8d27528ba61906.js:9 Error: The language "markdown" has no grammar.
at Object.highlight (1809-d263923ea021c688.js:8:3755)
at [[...path]]-3633ecc2aa697c21.js:1:2753
at Object.renderContent ([[...path]]-3633ecc2aa697c21.js:1:2844)
...
```
Modifications:
- Support more widely used languages by declaring `prismjs/components/<lang>`
- Fall back to the plantext mode if `Prism` raises an exception.
- Minor) Move Java files for test servers to `webapp/javaTest` for easier
discovery.
Result:
Markdown files are now correctly rendered in the diff view.
WalkthroughBuild config updated to add a Java test source set and a Gradle task to install Playwright; DiffView expanded Prism languages and added a try/catch fallback for highlighting; checkstyle suppressions extended to include Changes
Sequence Diagram(s)sequenceDiagram
participant UI as DiffView (UI)
participant Prism as Prism.highlighter
rect rgb(220,240,255)
UI->>Prism: request highlight(code, lang)
Prism-->>UI: highlighted HTML
end
rect rgb(255,230,230)
UI->>Prism: request highlight(code, lang)
Prism--x UI: throws error
UI->>UI: catch error -> render plain text fallback
end
sequenceDiagram
participant Gradle as Gradle
participant Npm as npm
rect rgb(230,255,230)
Gradle->>Npm: npmInstall (dependency)
Npm-->>Gradle: install node_modules
Gradle->>Npm: run playwright:install (install browsers)
Npm-->>Gradle: playwright binaries installed
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🔇 Additional comments (1)
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: 1
🧹 Nitpick comments (1)
webapp/src/dogma/common/components/DiffView.tsx (1)
58-61: Consider explicit return for clarity.While returning
undefinedis valid in React, explicitly returningnullis clearer and more idiomatic.function highlightSyntax(path: string, str: string): React.JSX.Element { if (!str) { - return; + return null; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
webapp/build.gradle(1 hunks)webapp/src/dogma/common/components/DiffView.tsx(2 hunks)
🧰 Additional context used
🪛 ast-grep (0.39.6)
webapp/src/dogma/common/components/DiffView.tsx
[warning] 72-72: Usage of dangerouslySetInnerHTML detected. This bypasses React's built-in XSS protection. Always sanitize HTML content using libraries like DOMPurify before injecting it into the DOM to prevent XSS attacks.
Context: dangerouslySetInnerHTML
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://reactjs.org/docs/dom-elements.html#dangerouslysetinnerhtml
- https://cwe.mitre.org/data/definitions/79.html
(react-unsafe-html-injection)
🪛 Biome (2.1.2)
webapp/src/dogma/common/components/DiffView.tsx
[error] 73-73: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
⏰ 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). (9)
- GitHub Check: build-windows-latest-jdk-21
- GitHub Check: lint
- GitHub Check: build-ubuntu-latest-jdk-17-coverage
- GitHub Check: build-macos-latest-jdk-21
- GitHub Check: flaky-tests
- GitHub Check: build-ubuntu-latest-jdk-17-min-java-11
- GitHub Check: build-ubuntu-latest-jdk-21-snapshot
- GitHub Check: build-ubuntu-latest-jdk-11
- GitHub Check: docker-build
🔇 Additional comments (3)
webapp/src/dogma/common/components/DiffView.tsx (1)
11-11: LGTM! Language imports address the reported issue.The addition of Prism language components for json5, markdown, python, sass, xml-doc, and docker directly resolves the "The language 'markdown' has no grammar" error mentioned in the PR motivation.
Also applies to: 15-19
webapp/build.gradle (2)
23-32: LGTM! Test source configuration is correct.The sourceSets configuration properly defines the relocated test sources at
javaTest/javaand test resources atjavaTest/resources, matching the PR's stated objective of improving discoverability.
34-37: LGTM! Playwright installation task is properly configured.The
installPlayWrighttask is correctly set up as anNpmTaskwith appropriate dependency onnpmInstalland proper arguments to execute the Playwright installation script.
| try { | ||
| return ( | ||
| <pre | ||
| style={{ display: 'inline' }} | ||
| dangerouslySetInnerHTML={{ __html: Prism.highlight(str, Prism.languages[language], language) }} | ||
| /> | ||
| ); | ||
| } catch (e) { | ||
| return <pre style={{ display: 'inline' }}>{str}</pre>; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Excellent error handling with fallback to plain text.
The try/catch wrapper properly addresses the Prism exception issue mentioned in the PR motivation. The fallback to plain text ensures the UI remains functional even when syntax highlighting fails.
Regarding the static analysis warnings about dangerouslySetInnerHTML: Prism is designed to safely escape HTML entities in the input before highlighting, making this usage safe. This is the standard pattern for integrating Prism with React.
Verify that the version of Prism in use (1.29.0) properly escapes HTML content:
🌐 Web query:
Does Prism.js version 1.29.0 escape HTML entities in the input when using Prism.highlight()?
💡 Result:
Short answer: No — Prism.highlight does not itself HTML-escape the raw input. It returns an HTML string with token markup; you must escape user-supplied code (or insert via textContent) before/when placing it into the DOM. [1][2]
Sources:
- Prism docs: Prism.highlight returns a highlighted HTML string (API expects you to handle safe insertion). [1]
- Security advisories explaining past/ongoing XSS/dom-clobber risks and that some plugins historically did not escape output (showing escaping is a caller/plugin responsibility). [2]
Now I need to search the codebase to understand the source of the str parameter in the DiffView component.
---
🏁 Script executed:
```shell
#!/bin/bash
# Find DiffView.tsx and get context around lines 69-78
fd DiffView.tsx --type f --exec cat {} \;
Length of output: 3892
Incorrect security assessment — XSS vulnerability exists with current implementation.
The original review comment incorrectly claims Prism escapes HTML entities. Web search and security advisories confirm the opposite: Prism.highlight() does NOT escape the input; it returns an HTML string for syntax highlighting markup only. This means the dangerouslySetInnerHTML at line 73 is potentially vulnerable to XSS if the str parameter contains untrusted or user-generated content.
Additionally, Prism.js version 1.29.0 contains a known DOM Clobbering XSS vulnerability (CVE-2024-53382).
The try/catch wrapper provides error handling for Prism exceptions but does NOT mitigate XSS attacks.
Required action:
- Verify that FileDto.content is guaranteed to come from a trusted, sanitized backend source
- If file content could be user-generated or untrusted, sanitize the Prism output using a library like DOMPurify before passing to
dangerouslySetInnerHTML - Consider alternatives such as using textContent with escaped output instead of dangerouslySetInnerHTML, or confirming backend-level content validation
🧰 Tools
🪛 ast-grep (0.39.6)
[warning] 72-72: Usage of dangerouslySetInnerHTML detected. This bypasses React's built-in XSS protection. Always sanitize HTML content using libraries like DOMPurify before injecting it into the DOM to prevent XSS attacks.
Context: dangerouslySetInnerHTML
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://reactjs.org/docs/dom-elements.html#dangerouslysetinnerhtml
- https://cwe.mitre.org/data/definitions/79.html
(react-unsafe-html-injection)
🪛 Biome (2.1.2)
[error] 73-73: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
🤖 Prompt for AI Agents
In webapp/src/dogma/common/components/DiffView.tsx around lines 69–78, the
current use of Prism.highlight() with dangerouslySetInnerHTML is unsafe because
Prism does not escape input and returns HTML (also note Prism v1.29.0 has a
DOM-clobbering XSS CVE); confirm whether FileDto.content is always
trusted/sanitized by the backend, and if it can be untrusted, sanitize the
highlighted HTML with a vetted sanitizer like DOMPurify before passing to
dangerouslySetInnerHTML (or avoid innerHTML entirely by using
textContent/escaped output for untrusted content); additionally, upgrade or pin
Prism to a non-vulnerable version to mitigate the known CVE.
3 participants
Motivation:
Prism raises an exception if a markdown file is specified in the commit diff view. Because the markdown syntax is not imported from
'prismjs/components/*.Modifications:
prismjs/components/<lang>Prismraises an exception.webapp/javaTestfor easier discovery.Result:
Markdown files are now correctly rendered in the diff view.
Summary by CodeRabbit
New Features
Bug Fixes
Chores