Skip to content

fix: resolve 4 bugs in Editron - #577

Closed
saurabhhhcodes wants to merge 1 commit into
piyushdotcomm:mainfrom
saurabhhhcodes:fix/Editron-76801
Closed

fix: resolve 4 bugs in Editron#577
saurabhhhcodes wants to merge 1 commit into
piyushdotcomm:mainfrom
saurabhhhcodes:fix/Editron-76801

Conversation

@saurabhhhcodes

@saurabhhhcodes saurabhhhcodes commented Aug 3, 2026

Copy link
Copy Markdown

Description

This PR fixes real bugs found in the codebase:

  • Replaced innerHTML assignment with textContent: prevents HTML injection / XSS and is faster since it does not parse markup.
  • Replaced innerHTML assignment with textContent: prevents HTML injection / XSS and is faster since it does not parse markup.
  • Replaced innerHTML assignment with textContent: prevents HTML injection / XSS and is faster since it does not parse markup.
  • Replaced innerHTML assignment with textContent: prevents HTML injection / XSS and is faster since it does not parse markup.

Type of Change

  • Bug fix (non-breaking change fixing an issue)

How Has This Been Tested?

  • Local manual testing

Checklist

  • My code follows the style guidelines
  • I have performed a self-review

Related Issue

Ref: #576

Summary by CodeRabbit

  • Bug Fixes
    • Improved security when displaying fetched data, counter values, and dynamically generated styles by using safer text-based rendering.
    • Preserved existing display behavior while preventing unintended HTML interpretation.

@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown

👋 Thanks for opening a PR, @saurabhhhcodes!

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.

@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown

🔁 Possible Duplicate PR Detected

Hi @saurabhhhcodes, a PR with a very similar title already exists:

➡️ #575

Please check if your changes overlap with the existing PR.

This is just a heads-up. The PR will not be auto-closed.

@coderabbitai

coderabbitai Bot commented Aug 3, 2026

Copy link
Copy Markdown

Review Change Stack

Walkthrough

The changes replace four innerHTML assignments with textContent assignments in JSON output, tutorial counters, the default counter template, and playground awareness-style injection.

Changes

Text content assignments

Layer / File(s) Summary
Replace HTML assignments with text content
editron-starters/json-server/index.js, editron-starters/tutorialkit/.../counter.js, editron-starters/tutorialkit/src/templates/default/counter.js, modules/playground/components/playground-editor.tsx
JSON output, counter displays, and generated awareness CSS now use textContent instead of innerHTML.

Estimated code review effort: 1 (Trivial) | ~5 minutes

Possibly related PRs

Suggested reviewers: piyushdotcomm

Poem

A rabbit swaps the markup stream,
For text that keeps the output clean.
Counters hop and styles take flight,
Four assignments now write just right.
textContent leads the scene.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the primary change as fixing four bugs in Editron.
Description check ✅ Passed The description explains the four fixes, their security rationale, testing method, change type, and related issue.
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.
✨ 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.

Warning

⚠️ This pull request shows signs of AI-generated slop (ai_padded_prose). It has been flagged by CodeRabbit slop detection and should be reviewed carefully.

@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Replace innerHTML with textContent to prevent XSS in Editron starters

🐞 Bug fix 🕐 10-20 Minutes

Grey Divider

AI Description

• Replace innerHTML writes with textContent to avoid HTML injection and parsing overhead.
• Harden starter/tutorial UI rendering and playground CSS injection against unsafe markup.
• Keep behavior identical while ensuring rendered output is treated as plain text.
Diagram

graph TD
  A["json-server starter"] -->|"textContent"| B("DOM: #output")
  C["tutorial/template counter"] -->|"textContent"| D("DOM: counter")
  E["Yjs awareness"] --> F["PlaygroundEditor"] -->|"textContent"| G("DOM: <style>")
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Sanitize then keep innerHTML
  • ➕ Preserves ability to render intentional markup when needed (e.g., formatted output).
  • ➖ More dependencies/maintenance (e.g., DOMPurify) and still riskier than avoiding HTML interpretation entirely.
2. Use CSSOM APIs for dynamic style updates
  • ➕ Avoids string concatenation; can add/remove rules via CSSStyleSheet.insertRule for finer control.
  • ➖ More code complexity; harder to diff/debug generated styling; less aligned with current simple string-based approach.

Recommendation: The PR’s approach is the best fit here: the rendered values (JSON, counter text, generated CSS) are not intended to be treated as HTML, so textContent is the safest and simplest fix. Consider CSSOM only if you later need incremental rule updates or rule-level toggling for awareness styling.

Files changed (4) +4 / -4

Bug fix (4) +4 / -4
index.jsRender fetched JSON via textContent instead of innerHTML +1/-1

Render fetched JSON via textContent instead of innerHTML

• Switches the JSON output assignment from 'innerHTML' to 'textContent' when rendering 'JSON.stringify(...)'. This prevents accidental HTML interpretation if the payload contains markup-like text.

editron-starters/json-server/index.js

counter.jsUse textContent for tutorial counter label +1/-1

Use textContent for tutorial counter label

• Updates the counter example to write the 'count is X' string via 'textContent' instead of 'innerHTML'. Keeps behavior the same while removing HTML parsing/injection risk.

editron-starters/tutorialkit/src/content/tutorial/1-basics/1-introduction/1-welcome/_files/counter.js

counter.jsUse textContent for default template counter label +1/-1

Use textContent for default template counter label

• Updates the default counter template to set the counter label with 'textContent'. Avoids interpreting user-controlled values as markup while preserving the click-to-increment behavior.

editron-starters/tutorialkit/src/templates/default/counter.js

playground-editor.tsxInject awareness CSS via textContent on the style element +1/-1

Inject awareness CSS via textContent on the style element

• Changes the dynamic '<style>' population from 'innerHTML' to 'textContent' when generating CSS for remote selections. This avoids HTML parsing and reduces the risk of markup injection via display names or other interpolated values.

modules/playground/components/playground-editor.tsx

@qodo-code-review

Copy link
Copy Markdown

Code Review by Qodo

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

Context used
✅ Compliance rules (platform): 22 rules

Grey Divider


Informational

1. playground-editor.tsx outside /src 📘 Rule violation ⌂ Architecture
Description
This PR modifies modules/playground/components/playground-editor.tsx, which is application source
code not located under the top-level /src directory. This violates the required project layout and
makes it harder to enforce consistent tooling and structure across the codebase.
Code

modules/playground/components/playground-editor.tsx[391]

+          styleEl.textContent = css;
Relevance

● Weak

Repo routinely modifies modules/... outside /src (e.g., PlaygroundEditor changes merged in PR
#184, #164).

PR-#184
PR-#164

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
PR Compliance ID 599992 requires application source code to be placed under /src. The modified
file path modules/playground/components/playground-editor.tsx (see cited line) is outside /src,
so the change set violates the rule.

Rule 599992: Place all application source code under the /src directory
modules/playground/components/playground-editor.tsx[391-391]

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 PR changes a source file located outside the required `/src` directory, violating the project layout compliance requirement.

## Issue Context
The repository’s application source code is expected to live under `/src/**`. This PR modifies `modules/playground/components/playground-editor.tsx`, which is outside `/src`.

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

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


Grey Divider

To customize comments, go to the Qodo configuration screen, or learn more in the docs.

Qodo Logo

@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: 1

🤖 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`:
- Line 391: Validate state.user.color against an approved color allowlist or hex
format before generating css, and CSS-escape state.user.name before
interpolating it into the stylesheet. Update the CSS construction immediately
before styleEl.textContent in the playground editor, preserving the existing
style injection flow while preventing user-controlled values from adding CSS
rules.
🪄 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: 35dda861-bac6-4b46-bbca-114a7e46581c

📥 Commits

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

📒 Files selected for processing (4)
  • editron-starters/json-server/index.js
  • editron-starters/tutorialkit/src/content/tutorial/1-basics/1-introduction/1-welcome/_files/counter.js
  • editron-starters/tutorialkit/src/templates/default/counter.js
  • modules/playground/components/playground-editor.tsx

}
}
styleEl.innerHTML = css;
styleEl.textContent = css;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Validate awareness values before generating CSS.

textContent prevents HTML parsing, but it does not escape CSS. If a collaborator can control state.user.name or state.user.color, these values can break the generated CSS and inject additional rules. Validate color against an allowlist or hex format, and apply CSS-string escaping to name before interpolation.

🤖 Prompt for 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.

In `@modules/playground/components/playground-editor.tsx` at line 391, Validate
state.user.color against an approved color allowlist or hex format before
generating css, and CSS-escape state.user.name before interpolating it into the
stylesheet. Update the CSS construction immediately before styleEl.textContent
in the playground editor, preserving the existing style injection flow while
preventing user-controlled values from adding CSS rules.

@coderabbitai coderabbitai Bot mentioned this pull request Aug 4, 2026
4 tasks
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.

2 participants