Skip to content

๐Ÿ›ก๏ธ Sentinel: [HIGH] ์ด๋ฉ”์ผ ํ—ค๋”์˜ CRLF ์ธ์ ์…˜ ์ทจ์•ฝ์  ์ˆ˜์ • - #1314

Merged
seonghobae merged 4 commits into
developfrom
sentinel-fix-crlf-14919358368017569235
Aug 11, 2026
Merged

๐Ÿ›ก๏ธ Sentinel: [HIGH] ์ด๋ฉ”์ผ ํ—ค๋”์˜ CRLF ์ธ์ ์…˜ ์ทจ์•ฝ์  ์ˆ˜์ •#1314
seonghobae merged 4 commits into
developfrom
sentinel-fix-crlf-14919358368017569235

Conversation

@seonghobae

@seonghobae seonghobae commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Security defect

SendEmailRequest did not apply one explicit CR/LF rejection rule consistently across all user-controlled SMTP header fields. The API must fail before dispatch when to, subject, in_reply_to, or references contains an isolated CR or LF.

Root-cause fix

  • run a mode="before" field validator across all four fields;
  • reject both chr(13) and chr(10);
  • exercise eight route-level cases (four fields ร— two newline characters);
  • assert HTTP 422 and that send_email is never called;
  • keep the security decision record aligned with the implemented field set.

CodeRabbit identified the missing to alignment on predecessor head e7fd1b994f7be7c43494aca6cf92b4fc64285dfc. Commits 6059aa56ca7c3a5ea8b1dedf5ad4d22ec82be886 and 0b41c9677d33a5cfe73ad9062c9124b8ee8fa90b added the recipient boundary and tests; all three review threads are resolved and CodeRabbit approved the exact current head.

Exact-head evidence

For 0b41c9677d33a5cfe73ad9062c9124b8ee8fa90b against live develop@f781701f108cacae08bf1b0749b62ddb8b8385fd:

  • Bandit 31468086850: success;
  • Dependency Review 31468086882: success;
  • SAST Semgrep 31468086887: success;
  • Security Scan 31468086893: success;
  • Application CI 31468086892: backend and frontend jobs success;
  • container build 31468087100: still in progress.

Pending, predecessor, or status-only evidence is not merge approval. Merge only after the unchanged head satisfies the remaining container, repository policy, and independent review gates.

@google-labs-jules

Copy link
Copy Markdown
Contributor

๐Ÿ‘‹ Jules, reporting for duty! I'm here to lend a hand with this pull request.

When you start a review, I'll add a ๐Ÿ‘€ emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down.

I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job!

For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with @jules. You can find this option in the Pull Request section of your global Jules UI settings. You can always switch back!

New to Jules? Learn more at jules.google/docs.


For security, I will only act on instructions from the user who triggered this task.

@coderabbitai

coderabbitai Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

๐Ÿ“ Walkthrough

Walkthrough

The email request model now rejects CR/LF characters in subject, in_reply_to, and references. Parameterized tests cover all injection cases and confirm that sending is blocked. Security documentation records the validation requirement.

Changes

Email header validation

Layer / File(s) Summary
CRLF validation
backend/api/emails.py
SendEmailRequest validates subject, in_reply_to, and references and rejects carriage-return or line-feed characters.
Validation coverage and security record
backend/tests/test_emails_api.py, .jules/sentinel.md
Parameterized tests cover CR and LF injection in each validated field. The security finding documents the required early validation.

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

Suggested reviewers: codex

๐Ÿšฅ Pre-merge checks | โœ… 4
โœ… Passed checks (4 passed)
Check name Status Explanation
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.
Description Check โœ… Passed Check skipped - CodeRabbitโ€™s high-level summary is enabled.
Title check โœ… Passed The title clearly identifies the CRLF injection vulnerability fix in email headers, which matches the main change.
โœจ Finishing Touches
๐Ÿ“ Generate docstrings
  • Create stacked PR
  • Commit on current branch
๐Ÿงช Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch sentinel-fix-crlf-14919358368017569235

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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 3

๐Ÿค– 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 @.jules/sentinel.md:
- Around line 132-135: Update the prevention record in the CRLF injection
documentation to match the implementation: either extend the SendEmailRequest
validator and API tests to cover the to field alongside subject, in_reply_to,
and references, or revise the record to list only the fields currently
validated. Keep the documented guarantee consistent with the actual model and
test coverage.

In `@backend/api/emails.py`:
- Around line 696-703: Extend reject_crlf to validate the to field, then add CR
and LF API cases in backend/tests/test_emails_api.py lines 1822-1848 matching
the existing 422 and no-dispatch assertions. Keep the four-field prevention rule
in .jules/sentinel.md lines 132-135, since implementation and tests must enforce
all four headers.

In `@backend/tests/test_emails_api.py`:
- Around line 1822-1848: Extend the parameterization in
test_send_email_endpoint_rejects_header_injection to include both CR and LF
newline cases for the to field. Preserve the existing expected 422 response and
mock_send_email.assert_not_called() assertions so the API boundary is explicitly
covered.
๐Ÿช„ Autofix

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: 4b96bfad-8e70-4080-bb7d-26143d82d4ae

๐Ÿ“ฅ Commits

Reviewing files that changed from the base of the PR and between f781701 and e7fd1b9.

๐Ÿ“’ Files selected for processing (3)
  • .jules/sentinel.md
  • backend/api/emails.py
  • backend/tests/test_emails_api.py

Comment thread .jules/sentinel.md
Comment thread backend/api/emails.py Outdated
Comment thread backend/tests/test_emails_api.py
@github-actions

github-actions Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

PR governance metadata gate update for 0b41c9677d33a5cfe73ad9062c9124b8ee8fa90b: no current blocking failures remain.

PR governance metadata gate is ready; all current-head requirements passed.

@seonghobae
seonghobae merged commit c400250 into develop Aug 11, 2026
44 checks passed
@seonghobae
seonghobae deleted the sentinel-fix-crlf-14919358368017569235 branch August 11, 2026 07:52
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.

1 participant