Skip to content

fix(editor): sanitize user name before CSS interpolation in Yjs awareness styles - #516

Open
Xenon010101 wants to merge 2 commits into
piyushdotcomm:mainfrom
Xenon010101:fix/sanitize-awareness-name-xss
Open

fix(editor): sanitize user name before CSS interpolation in Yjs awareness styles#516
Xenon010101 wants to merge 2 commits into
piyushdotcomm:mainfrom
Xenon010101:fix/sanitize-awareness-name-xss

Conversation

@Xenon010101

@Xenon010101 Xenon010101 commented Jul 21, 2026

Copy link
Copy Markdown

What changed

Sanitized the user display name before interpolating it into CSS in the Yjs awareness cursor styling, and replaced innerHTML with textContent on the injected style element.

Closes #515

Why

The Yjs collaboration awareness handler in playground-editor.tsx interpolated state.user.name directly into a CSS content property that was then assigned via styleEl.innerHTML. Since state.user.name originates from the user OAuth profile (e.g. Google display name), a malicious collaborator could set their name to break out of the style tag and inject arbitrary HTML/JS into every other connected participant browser. This is an XSS vulnerability in the real-time collaboration layer.

Fix

  1. Added a sanitizeForCss helper that strips all characters except alphanumerics, spaces, hyphens, underscores, hash, and dot before interpolation into the CSS template literal.
  2. Replaced styleEl.innerHTML = css with styleEl.textContent = css to prevent the browser from parsing the style content as HTML.

Verification

  • npm run lint: 0 errors (28 pre-existing warnings, none from this change)
  • npm test: 112/112 tests pass
  • npm run build: builds successfully with no new warnings

…ness styles

The Yjs collaboration awareness handler interpolated state.user.name
directly into a CSS string assigned via innerHTML. A collaborator with
a crafted display name could break out of the CSS context and inject
arbitrary HTML/JS into every other connected participant's browser.

Add a sanitizeForCss helper that strips all characters except
alphanumerics, spaces, hyphens, underscores, and dots before
interpolation. Also replace innerHTML with textContent to prevent
HTML parsing of the style content.

Closes piyushdotcomm#515
@github-actions

Copy link
Copy Markdown

👋 Thanks for opening a PR, @Xenon010101!

Your PR has entered the 🚦 PR Review Pipeline.

Standard PR detected — your PR will follow the standard review pipeline.


What happens next

Stage Reviewer Checks
Stage 1 — Automated Validation 🤖 Bot DCO · Format · AI/Slop · Duplicate
Stage 2 — Human Review 👥 Maintainer Code + Quality Review
Stage 3 — PA / Maintainer Review 🔑 Project Admin Final Merge Decision

A pipeline status comment will appear below and update automatically as your PR progresses.


While you wait

  • Sign all commits (git commit -s)
  • Link your issue (Closes #123)
  • Use a feature branch (not main)
  • Avoid unrelated changes

This comment is posted only once.

@coderabbitai

coderabbitai Bot commented Jul 21, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@Xenon010101, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 52 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: cbbc0c68-1125-4b5e-984c-676ca322d28b

📥 Commits

Reviewing files that changed from the base of the PR and between d6f97f9 and f190bb8.

📒 Files selected for processing (1)
  • modules/playground/components/playground-editor.tsx

Walkthrough

The playground now sanitizes remote awareness user colors and names before embedding them in generated CSS, and applies the stylesheet through textContent instead of innerHTML.

Changes

Awareness CSS safety

Layer / File(s) Summary
Sanitize and apply awareness CSS
modules/playground/components/playground-editor.tsx
Adds sanitizeForCss, applies it to awareness user colors and names, and assigns generated stylesheet content through textContent.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related issues

  • Issue 515: Directly matches the sanitization and textContent changes in this pull request.

Suggested reviewers: piyushdotcomm

Poem

I’m a rabbit guarding CSS tonight,
Scrubbing colors clean and bright.
Names pass through a safer gate,
Text fills styles without HTML’s weight.
Hop, hop—awareness stays secure!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title accurately summarizes the main change: sanitizing user names before CSS interpolation in Yjs awareness styles.
Description check ✅ Passed The description covers what changed, why, fix details, and validation, though it doesn't follow the template's full checklist structure.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Sanitize Yjs awareness user fields before CSS injection in PlaygroundEditor

🐞 Bug fix 🕐 10-20 Minutes

Grey Divider

AI Description

• Sanitize collaborator name/color before interpolating into Yjs awareness cursor CSS.
• Replace injected style assignment from innerHTML to textContent to prevent XSS.
• Harden real-time collaboration cursor rendering against malicious OAuth display names.
Diagram

graph TD
  A[PlaygroundEditor] --> B[Yjs Provider] --> C[Awareness Update] --> D["sanitizeForCss()"] --> E["<style> element"] --> F{{Browser DOM}}
  C --> E
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Use CSS.escape() for interpolated values
  • ➕ Purpose-built escaping for CSS contexts
  • ➕ Avoids maintaining a custom allowlist regex
  • ➖ Does not directly address the innerHTML vector; still need textContent
  • ➖ Behavior nuances across contexts (identifiers vs string content) can be misunderstood
2. Use CSSOM APIs (insertRule / deleteRule) instead of string templates
  • ➕ Avoids raw string concatenation for style generation
  • ➕ More structured rule management (can update per clientId)
  • ➖ More code and complexity for little functional gain here
  • ➖ Potential performance/ordering pitfalls when frequently updating rules
3. Render cursor labels as DOM nodes instead of CSS ::before content
  • ➕ Eliminates user-controlled values inside CSS entirely
  • ➕ Simplifies escaping to standard text nodes
  • ➖ Requires reworking the cursor/selection rendering approach
  • ➖ Higher scope and regression risk compared to this targeted fix

Recommendation: The PR’s approach is appropriate for a targeted security fix: switch to textContent to remove HTML parsing risk and sanitize user-controlled fields before CSS interpolation. CSS.escape() could further improve correctness/maintainability, but the current allowlist is a reasonable defense-in-depth measure given the limited usage and the primary vector (innerHTML) is already removed.

Files changed (1) +6 / -3

Bug fix (1) +6 / -3
playground-editor.tsxHarden Yjs awareness cursor CSS against user-controlled injection +6/-3

Harden Yjs awareness cursor CSS against user-controlled injection

• Adds a sanitizeForCss helper and applies it to awareness-derived user name and color before interpolating into generated CSS. Replaces style element assignment from innerHTML to textContent to prevent style-tag breakouts and XSS.

modules/playground/components/playground-editor.tsx

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@modules/playground/components/playground-editor.tsx`:
- Around line 356-357: Update the color handling near the sanitized
state.user.color value to validate that it matches the supported six-digit
hexadecimal format before appending the alpha suffix; use the existing orange
fallback for invalid values, while leaving name sanitization unchanged.
- Around line 332-334: Update sanitizeForCss and the awareness value handling to
validate color and name as strings before sanitizing, and apply the fallback
when the sanitized result is empty. Ensure truthy non-string remote values
cannot reach string methods, covering both usages around the awareness rendering
logic.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: be1437e0-a4e2-4e6c-b4f5-78225376703a

📥 Commits

Reviewing files that changed from the base of the PR and between 4ffe26f and d6f97f9.

📒 Files selected for processing (1)
  • modules/playground/components/playground-editor.tsx

Comment thread modules/playground/components/playground-editor.tsx Outdated
Comment thread modules/playground/components/playground-editor.tsx Outdated
@qodo-code-review

qodo-code-review Bot commented Jul 21, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📜 Skill insights (0)

Context used
✅ Compliance rules (platform): 22 rules

Grey Divider


Remediation recommended

1. Sanitizer assumes string ✓ Resolved 🐞 Bug ☼ Reliability
Description
sanitizeForCss calls .replace(...) on awareness-provided values without runtime type checks, so a
collaborator (or incompatible client) can publish a truthy non-string (e.g., object/number) and
trigger a TypeError in the awareness update handler. This can break awareness-style updates in the
receiving editor and spam console errors on subsequent updates.
Code

modules/playground/components/playground-editor.tsx[R356-357]

+              const color = sanitizeForCss(state.user.color || "orange");
+              const name = sanitizeForCss(state.user.name || "Anonymous");
Relevance

⭐⭐⭐ High

Repo history accepts runtime guards for unknown data to avoid crashes/TypeErrors (e.g., PR236).

PR-#236

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The sanitizer is declared to take a string and uses .replace, but the values come from
provider.awareness.getStates() which is populated via a WebSocket-backed provider; at runtime
these fields are not validated and can be non-strings.

modules/playground/components/playground-editor.tsx[332-357]
lib/yjs.ts[52-56]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`sanitizeForCss` assumes its input is a string and calls `.replace(...)`. Awareness state is effectively untrusted runtime data received over the collaboration channel; if a peer sends a non-string `user.name`/`user.color`, the handler throws.

### Issue Context
The awareness update handler is registered via `provider.awareness.on("update", handleAwarenessUpdate)` and processes `provider.awareness.getStates()`.

### Fix Focus Areas
- modules/playground/components/playground-editor.tsx[332-357]

### Suggested fix
- Change `sanitizeForCss` to accept `unknown` and guard/coerce:
 - `if (typeof value !== "string") return "";`
 - optionally `.trim()`
- After sanitization, apply safe fallbacks when the result is empty (e.g., `const name = safeName || "Anonymous"; const color = safeColor || "orange";`) so malformed payloads can’t break the CSS generation path.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Informational

2. Unicode names stripped 🐞 Bug ≡ Correctness
Description
sanitizeForCss strips all characters outside an ASCII allowlist, so many legitimate OAuth display
names (accented/CJK/Cyrillic/emoji) will be partially or fully removed before being rendered in the
cursor label. This can result in empty or misleading collaborator labels in the editor.
Code

modules/playground/components/playground-editor.tsx[R332-333]

+        const sanitizeForCss = (value: string) =>
+          value.replace(/[^a-zA-Z0-9 _\-#.]/g, "");
Relevance

⭐⭐ Medium

No historical evidence on Unicode-preserving CSS sanitization; team focus seems security-first,
unclear on UX tradeoff.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The sanitizer allowlist only permits [a-zA-Z0-9 _\-#.], and the sanitized value is inserted into a
quoted CSS content literal, so any non-ASCII characters are removed before display.

modules/playground/components/playground-editor.tsx[332-333]
modules/playground/components/playground-editor.tsx[368-373]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The current regex-based sanitizer removes non-ASCII characters from display names, which can blank or truncate collaborator labels.

### Issue Context
The value is interpolated into a CSS `content: "${name}";` string.

### Fix Focus Areas
- modules/playground/components/playground-editor.tsx[332-373]

### Suggested fix
- Prefer escaping for a CSS string literal instead of stripping to ASCII:
 - remove/escape control characters and newlines
 - escape backslashes and double quotes (`\\` and `\"`)
- Keep the `textContent` assignment (good) and ensure the resulting string cannot terminate the quoted `content` literal.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

Comment thread modules/playground/components/playground-editor.tsx Outdated
.padEnd(6, "0")
: "#30bced";

const sanitizeForCss = (value: string) =>

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Informational

2. Unicode names stripped 🐞 Bug ≡ Correctness

sanitizeForCss strips all characters outside an ASCII allowlist, so many legitimate OAuth display
names (accented/CJK/Cyrillic/emoji) will be partially or fully removed before being rendered in the
cursor label. This can result in empty or misleading collaborator labels in the editor.
Agent Prompt
### Issue description
The current regex-based sanitizer removes non-ASCII characters from display names, which can blank or truncate collaborator labels.

### Issue Context
The value is interpolated into a CSS `content: "${name}";` string.

### Fix Focus Areas
- modules/playground/components/playground-editor.tsx[332-373]

### Suggested fix
- Prefer escaping for a CSS string literal instead of stripping to ASCII:
  - remove/escape control characters and newlines
  - escape backslashes and double quotes (`\\` and `\"`)
- Keep the `textContent` assignment (good) and ensure the resulting string cannot terminate the quoted `content` literal.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Address review feedback: remote awareness data is not guaranteed to
satisfy TypeScript types, so sanitizeForCss now accepts unknown and
returns empty string for non-strings. Added sanitizeColor helper that
validates 6-digit hex format before appending alpha suffix, falling
back to #30bced for invalid values.
@Xenon010101

Copy link
Copy Markdown
Author

Addressed both review comments:

  1. Type safety: sanitizeForCss now accepts unknown and returns "" for non-string values. Non-string names fall back to "Anonymous".

  2. Hex color validation: Added sanitizeColor helper that strips # prefix and validates against /^[0-9a-fA-F]{6}$/. Invalid colors (e.g. named colors, short hex, arbitrary strings) fall back to #30bced. This also fixes a pre-existing bug where the orange fallback would produce invalid CSS when the 40 alpha suffix was appended.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(editor): sanitize user name before CSS interpolation in Yjs awareness styles to prevent HTML injection

1 participant