Skip to content

fix: sanitize dangerouslySetInnerHTML with DOMPurify to prevent XSS#33

Merged
abhizipstack merged 1 commit intomainfrom
fix/xss-no-code-model
Apr 2, 2026
Merged

fix: sanitize dangerouslySetInnerHTML with DOMPurify to prevent XSS#33
abhizipstack merged 1 commit intomainfrom
fix/xss-no-code-model

Conversation

@abhizipstack
Copy link
Copy Markdown
Contributor

What

  • Sanitize dangerouslySetInnerHTML output in no-code-model log renderer using DOMPurify

Why

  • XSS vulnerability: dangerouslySetInnerHTML renders ANSI-converted HTML without sanitization
  • Malicious content in transformation logs could execute scripts in the browser
  • Only instance of dangerouslySetInnerHTML in the entire frontend codebase

How

  • Install dompurify dependency
  • Wrap ansiToHtml.toHtml(log) output with DOMPurify.sanitize() before rendering
  • Log display still works correctly — DOMPurify allows safe HTML (ANSI color spans) while stripping dangerous content

Can this PR break any existing features. If yes, please list possible items. If no, please explain why. (PS: Admins do not merge the PR without this section filled)

  • No — DOMPurify allows standard HTML tags used by ansi-to-html (span, font color) while only stripping script tags and event handlers. Log rendering remains unchanged visually.

Database Migrations

  • None

Env Config

  • None

Relevant Docs

Related Issues or PRs

  • N/A

Dependencies Versions

  • dompurify (latest)

Notes on Testing

  • Open a project in IDE → run a transformation → check logs tab renders correctly
  • ESLint passes with 0 errors

Screenshots

N/A

Checklist

I have read and understood the Contribution Guidelines.

Add DOMPurify sanitization to the ANSI log renderer in no-code-model.
The parseLog function converts ANSI escape codes to HTML via
ansi-to-html, then renders with dangerouslySetInnerHTML. Without
sanitization, malicious content in logs could execute scripts.

- Install dompurify dependency
- Wrap ansiToHtml.toHtml() output with DOMPurify.sanitize()
- Only instance of dangerouslySetInnerHTML in the entire frontend

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@greptile-apps
Copy link
Copy Markdown

greptile-apps bot commented Apr 2, 2026

Greptile Summary

This PR fixes a real XSS vulnerability by wrapping dangerouslySetInnerHTML output in no-code-model.jsx with DOMPurify.sanitize() before rendering ANSI-converted transformation logs. It is a minimal, well-targeted security patch with no functional regressions.

  • Security fix: parseLog now sanitizes the HTML output of ansi-to-html via DOMPurify.sanitize(), preventing injected script tags or event handlers from executing in the browser.
  • Correct dependency placement: dompurify ^3.3.3 is added to dependencies (not devDependencies), appropriate for a runtime browser library.
  • Scope confirmed: A codebase-wide search confirms this is the only use of dangerouslySetInnerHTML in the frontend, so the fix is complete.
  • Incidental lock-file churn: package-lock.json drops @testing-library/dom and typescript peer-dep entries; these were orphaned transitive peers that npm re-resolved during install and do not affect the build or tests.
  • Suggestion: Passing an explicit ALLOWED_TAGS/ALLOWED_ATTR config to DOMPurify.sanitize() would make the intent clearer and guard against future default-config drift (see inline comment).

Confidence Score: 5/5

  • Safe to merge — correctly patches the XSS vulnerability with no breaking changes; only remaining feedback is a non-blocking style suggestion.
  • All findings are P2 (style/hardening suggestions). The core security fix is correct: DOMPurify 3.x allows style attributes by default so ANSI colour spans are preserved, and the sanitizer strips dangerous content. No logic errors, no data-loss risk, no breaking API changes.
  • No files require special attention.

Important Files Changed

Filename Overview
frontend/src/ide/editor/no-code-model/no-code-model.jsx Wraps ansiToHtml.toHtml(log) with DOMPurify.sanitize() in parseLog before rendering via dangerouslySetInnerHTML; correctly addresses the XSS vulnerability.
frontend/package.json Adds dompurify ^3.3.3 as a production dependency, correctly placed in dependencies rather than devDependencies.
frontend/package-lock.json Adds dompurify 3.3.3 lock entry; incidentally removes @testing-library/dom and typescript peer-dep entries that npm re-resolved as no longer needed during install.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[logsInfo data from API] --> B["parseLog(el)"]
    B --> C["ansiToHtml.toHtml(log)\n(ANSI → raw HTML string)"]
    C --> D["DOMPurify.sanitize(html)\n✅ strips scripts & event handlers\n✅ preserves span/style for colors"]
    D --> E["dangerouslySetInnerHTML\n{ __html: sanitizedHtml }"]
    E --> F[Safe DOM render in browser]

    style D fill:#d4edda,stroke:#28a745,color:#000
    style C fill:#fff3cd,stroke:#ffc107,color:#000
Loading
Prompt To Fix All With AI
This is a comment left during a code review.
Path: frontend/src/ide/editor/no-code-model/no-code-model.jsx
Line: 362

Comment:
**Consider explicitly allowing `style` attribute in DOMPurify config**

`ansi-to-html` generates `<span style="color:...">` elements for ANSI color codes. DOMPurify 3.x does allow `style` in its default `ALLOWED_ATTR` list, so this works correctly today. However, the current call relies implicitly on that default. To make the intent explicit and guard against future DOMPurify configuration changes or version upgrades that might alter defaults, consider passing an explicit config:

```suggestion
  const parseLog = (log) =>
    DOMPurify.sanitize(ansiToHtml.toHtml(log), {
      ALLOWED_TAGS: ["span", "br"],
      ALLOWED_ATTR: ["style"],
    });
```

This ensures ANSI colour spans are always preserved while keeping the allowlist as tight as possible.

How can I resolve this? If you propose a fix, please make it concise.

Reviews (1): Last reviewed commit: "fix: sanitize dangerouslySetInnerHTML wi..." | Re-trigger Greptile

Copy link
Copy Markdown
Contributor

@wicky-zipstack wicky-zipstack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Copy Markdown
Contributor

@tahierhussain tahierhussain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@abhizipstack abhizipstack merged commit 8efcdbf into main Apr 2, 2026
1 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants