Repository navigation
Conversation
The ReScript comment view rendered marked output without sanitizing it, unlike the React and server component views. Pass it through sanitize-html with the same defaults the React view uses, and cover it with a system spec. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Capybara skips hidden elements by default, so the script check could never fail, and the handler check depended on timing. Check the DOM directly. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
No description provided. |
🚀 Quick Review App CommandsWelcome! Here are the commands you can use in this PR:
|
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Sanitize markup before rendering client/app/bundles/comments/rescript/bindings/SanitizeHtml.res, client/app/bundles/comments/rescript/CommentList/Comment/Comment.res, spec/rescript/rescript_spec.rb |
A ReScript binding exposes sanitize-html. Comment.make sanitizes generated markup before assigning it to innerHTML. The browser test checks Markdown rendering and the absence of image and script elements. |
Estimated code review effort: 2 (Simple) | ~8 minutes
Change: Bug fix
Suggested reviewers: yassa-tamer
Merge Risk: 
11c58
-
No actionable merge-blocking risk was identified.
Pre-merge checks: 
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main change: sanitizing comment markdown on the ReScript page. Docstring Coverage No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Docstring Coverage
-
Explanation
No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 0 functions across 1 files. (2 skipped: 2 unsupported.)
-
Finishing Touches
-
Generate docstrings
-
- Commit to this branch
- Create a new PR
Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Local Adversarial ReviewReviewed revision: Outcome: No open findings; closed and optional findings are in history. Coverage: openai/codex: Read-only source lookup permitted; actual inspection coverage is UNKNOWN. Test execution is prohibited. · anthropic/claude: Read-only file lookup in an export of the reviewed commit; actual inspection coverage is UNKNOWN. Git history and test execution are unavailable. Usage and attribution in the PR description. Reviewed commits:
Findings, assessments, and execution metadataFinding details
Execution metadata
Prompt: Shaka default · criteria Commit ead1e8f · openai/codex, anthropic/claude · 2 findingsCommit: Review coverage: Coverage excerpts may be incomplete or misidentified; inspect the original reports.
Unverified report excerpt · openai/codex: Repository criteria: supplied from `af62fed35be931c86dc695afda480f7dd7c34e21:AGENTS.md`. Inspected all three changed files, the unchanged React sanitizer usage, ReScript caller and Marked binding, compiler configuration, dependencies, route, and browser-test setup at the pinned HEAD. Tests run: none; candidate-code execution was prohibited. Compilation and before/after browser behavior remain unverified. The new spec should fail when sanitization is reverted because the injected image remains and can execute its handler. It covers bold formatting and this payload; unsafe URL schemes and broader markdown preservation are untested.
Unverified report excerpt · anthropic/claude: UNKNOWN; inspect the original report for complete coverage limits. Round 1 · ead1e8f · openai/codex · effort UNKNOWN · 0 findingsCoverageRepository criteria: supplied from Inspected all three changed files, the unchanged React sanitizer usage, ReScript caller and Marked binding, compiler configuration, dependencies, route, and browser-test setup at the pinned HEAD. Tests run: none; candidate-code execution was prohibited. Compilation and before/after browser behavior remain unverified. The new spec should fail when sanitization is reverted because the injected image remains and can execute its handler. It covers bold formatting and this payload; unsafe URL schemes and broader markdown preservation are untested. Findingsno findings The sanitizer matches the React view’s default-options contract. No gate weakening, permission widening, credentials, or unnecessary implementation complexity was identified. REVIEWED ead1e8f BY openai/codex EFFORT UNKNOWN FINDINGS 0 Round 2 · ead1e8f · anthropic/claude · effort UNKNOWN · 2 findingsCoverage
Repository criteria: supplied (af62fed:AGENTS.md). It holds build and workflow rules only; none of them bear on this diff. Correctness: The fix itself looks right. Contract drift: none. Security: this tightens a gate and adds no credentials. Simplicity: the one-line binding is the minimum needed; there's nothing to remove. Findings
REVIEWED ead1e8f BY anthropic/claude EFFORT UNKNOWN FINDINGS 2 Collated as: Commit 11c58d2 · openai/codex, anthropic/claude · 0 findingsCommit: Review coverage: Coverage excerpts may be incomplete or misidentified; inspect the original reports.
Unverified report excerpt · openai/codex: Repository criteria: supplied from `af62fed35be931c86dc695afda480f7dd7c34e21:AGENTS.md`. Inspected the exact diff, pinned ReScript rendering and fetch callers, React sanitizer usage, dependency/build configuration, Rails endpoint/view, and Capybara setup. Tests run: none; candidate-code execution was prohibited. Compilation and before/after browser behavior remain unverified. Observation: F1 is resolved by inspection: the spec waits for rendered bold text, then checks for images and scripts including hidden elements. Reverting sanitization should leave those elements and fail the assertions. F2 is also addressed by `visible: :all`. The regression covers initial comment loading; unsafe URL schemes, attributes on allowed tags, and live updates are not explicitly tested.
Unverified report excerpt · anthropic/claude: UNKNOWN; inspect the original report for complete coverage limits. Round 3 · 11c58d2 · openai/codex · effort UNKNOWN · 0 findingsCoverageRepository criteria: supplied from Inspected the exact diff, pinned ReScript rendering and fetch callers, React sanitizer usage, dependency/build configuration, Rails endpoint/view, and Capybara setup. Tests run: none; candidate-code execution was prohibited. Compilation and before/after browser behavior remain unverified. Observation: F1 is resolved by inspection: the spec waits for rendered bold text, then checks for images and scripts including hidden elements. Reverting sanitization should leave those elements and fail the assertions. F2 is also addressed by The regression covers initial comment loading; unsafe URL schemes, attributes on allowed tags, and live updates are not explicitly tested. Findingsno findings REVIEWED 11c58d2 BY openai/codex EFFORT UNKNOWN FINDINGS 0 Round 4 · 11c58d2 · anthropic/claude · effort UNKNOWN · 0 findingsCoverage
Repository criteria: supplied (af62fed:AGENTS.md). Nothing in those criteria applies to this change beyond the general build and test commands. FindingsPrior rounds
Fresh review
no findings REVIEWED 11c58d2 BY anthropic/claude EFFORT UNKNOWN FINDINGS 0 REVIEWED 11c58d2 BY anthropic/claude EFFORT UNKNOWN FINDINGS 0 |
|
Review: LGTM. The ReScript page now sanitizes the Minor, non-blocking:
I didn't run the specs or build the ReScript code. |
 [Medium impact] The PR appears safe to merge. SummaryThe ReScript comment view now sanitizes Markdown output before inserting HTML.
Reviews (1) · Last reviewed commit: "Count hidden elements in the ReScript sa..." · Reviewed by Greptile |
justin808
left a comment
justin808
left a comment
There was a problem hiding this comment.
🤖 Claude Code · Anthropic · claude-opus-5-5 · medium
Code Walkthrough
Comments on /rescript now show sanitized markdown, so HTML in a comment can no longer run script in a visitor's browser. The new binding exposes the sanitize-html package that was already a dependency, and the view applies it before dangerouslySetInnerHTML with the same defaults as Comment.jsx. The spec fails with the fix reverted. This head passed smoke / Demo fleet smoke, claude-review, CodeRabbit, and Greptile Review. Rollback is a revert, which restores the vulnerability.
Walkthrough for commit 11c58d2. This is a COMMENT, not an approval.
| it "keeps markdown formatting and removes unsafe HTML" do | ||
| expect(page).to have_css(".js-comment-text strong", text: "bold") | ||
| expect(page).to have_no_css(".js-comment-text img", visible: :all) | ||
| expect(page).to have_no_css(".js-comment-text script", visible: :all) |
There was a problem hiding this comment.
🤖 Claude Code · Anthropic · claude-opus-5-5 · medium
Capybara skips hidden elements by default, and a script element is never visible, so these checks need visible: :all to be able to fail.
|
🤖 Codex · OpenAI · gpt-6.1-sol (configured) · medium Checkpoint for
|
|
No description provided. |
✅ Review App DeletedReview app for PR #877 is deleted |
🤖 Claude Code · Anthropic · claude-opus-5-5 · medium - AI spend: $2.91
Recommendation: Merge · Code Walkthrough · Code Review · Checkpoint
Summary
Comments on the
/rescriptpage can no longer inject scripts or event handlers, because their markdown is now sanitized before display. This matches the React and server component views, and normal markdown formatting still renders.Intent
Why: The ReScript comment view rendered
markedoutput directly, so any comment could run script in another visitor's browser.Task: Claude prompt
Root cause:
Comment.respassedMarked.markedoutput straight todangerouslySetInnerHTML; the React and server component views sanitize first, and this one never did.Safe because:
sanitize-htmlis already a dependency, and only this one page's rendering changes.Human review: 2 of 5. Skim
client/app/bundles/comments/rescript/CommentList/Comment/Comment.res,client/app/bundles/comments/rescript/bindings/SanitizeHtml.res,spec/rescript/rescript_spec.rb(changes code or configuration).Scope: Add a
sanitize-htmlbinding, use it inComment.reswith the same defaults asComment.jsx, and add a system spec. (3 files, +19 / −1 lines)Files: Tests 1 · Other 2
Tests:
spec/rescript/rescript_spec.rbOther:
client/app/bundles/comments/rescript/CommentList/Comment/Comment.resclient/app/bundles/comments/rescript/bindings/SanitizeHtml.resVerification
Validation passed; local review completed at 11c58d2 (evidence).
Evidence and limitations
Validation and review:
validatepassed (exit 0); local review by openai/codex, anthropic/claude (attestation) at11c58d2.spec/rescript/rescript_spec.rbfails with the fix reverted and passes with it, in headless Chrome.validatepassed: RuboCop, ESLint, 77 RSpec examples, 16 Jest tests.Not verified: Comments arriving over Action Cable have no test; they render through the same
Comment.res. The live demo was not tested before or after this fix.Scope of the search
A search of
client/andapp/fordangerouslySetInnerHTML,marked,html_safe, andrawfound no other unsanitized comment rendering. Stored comments are unchanged; they are sanitized when displayed.Shaka metrics
Estimated token costs: $2.91 · ready in 14m
Details
Usage and cost
— not reported; + some usage not reported, so the amount is a minimum.
Understand the columns and accounting rules.
How each report was measured and priced
gpt‑6.1‑sol review ×2
Rate card: installed Shaka.
Standard Codex credit and OpenAI API-equivalent rates, verified 2026-09-29.
Sources: Codex credit rates, GPT-6.1 Sol, and prompt-cache accounting.
Local adversarial reviewer usage: included below. External reviewer/tool-model usage: UNKNOWN.
Native usage is PARTIAL. Scope: all turns in selected sources.
claude‑opus‑5‑5 review ×2
Rate card: installed Shaka.
Claude CLI recorded estimated USD for an independent call; not an invoice.
Local adversarial reviewer usage: included below. External reviewer/tool-model usage: UNKNOWN.
Native usage is PARTIAL. Scope: all turns in selected sources.
claude‑opus‑5‑5 implementation
Rate card: installed Shaka.
Anthropic API list prices, verified 2026-09-23.
Sources: Anthropic pricing.
Local adversarial reviewer usage: UNKNOWN. External reviewer/tool-model usage: UNKNOWN.
Native usage is PARTIAL. Scope: responses at or after 2026-11-02T08:00:00Z (whole-second sources include the cutoff second).
Rate card: installed Shaka.
Anthropic API list prices, verified 2026-09-23.
Sources: Anthropic pricing.
Local adversarial reviewer usage: UNKNOWN. External reviewer/tool-model usage: UNKNOWN.
Native usage is PARTIAL. 18 responses. Scope: responses at or after 2026-11-02T08:00:00Z (whole-second sources include the cutoff second).
Timings · 53m between checkpoints
This head: Prompt to CI 11m · CI to ready 3m · Ready to merged pending
Totals: 53m between checkpoints
Times are America/Los_Angeles wall-clock time. A wait runs from when an attention label goes on until it comes off or the pull request merges or closes.
Before first recorded validation: 3m 7s · Task start (agent-reported): 2026-11-02 00:06 PST. Validation started: 2026-11-02 00:09 PST. This interval overlaps the stretches above.
Measured steps: Validation 4m 23s (4 runs) · Local review 3m 21s (4 runs) · Product checkpoint 44s (1 run)
Models: openai; anthropic; openai · gpt-6.1-sol · medium · Machines: m5
Each measured step is a helper run recorded on the machine that ran it. A step logged on a machine that never updated this description is missing, so the totals can be partial. A step can overlap the stretches above.
Understand the timings.
Execution details
66aa5d5Shaka settings used
Recorded checks match this PR’s current commit. This repository’s settings do not require review.
Only the listed runs are covered. Selecting settings does not show that a check ran.
Current settings: Repository settings from commit
af62fed; Shaka installation: version 1.0.0, installed revision (Shaka skill version under Execution details)af62fedaf62fedaf62fedSkills used
No skill use recorded; coverage is UNKNOWN.
Agent-reported use is the agent’s account. The agent selects Corroborated when a record confirms invocation or loading. Shaka does not check that record or instruction compliance. Completeness is not verified.
WIP Details
Collection: alias available; host available; tag available; thread available.
PR prepared with Shaka.
Summary by CodeRabbit