π‘οΈ Sentinel: [CRITICAL/HIGH] Fix μ΄λ©μΌ CRLF μΈμ μ λ° μ΄μ€ νμ₯μ μ°ν μ·¨μ½μ - #1164
Conversation
μ΄ μ»€λ°μ backend/api/emails.py λͺ¨λμ μ·¨μ½μ μ μμ ν©λλ€. 1. `field_validator`λ₯Ό μΆκ°νμ¬ μ΄λ©μΌ ν€λ νλμ μμ€ λ¬Έμλ₯Ό μ°¨λ¨νμ¬ CRLF μΈμ μ μ λ°©μ§ν©λλ€. 2. `import_email_files`μ νμΌ μ λ‘λ κ²μ¦μ κ°μ νμ¬ νμΌ μ΄λ¦μ λΆν νκ³ μ΄μ€ νμ₯μ μ°νλ₯Ό νμ§ν©λλ€.
|
π 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 New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
π WalkthroughWalkthroughThe PR strengthens email header and imported filename validation against CRLF injection and double-extension bypasses, documents the security fixes, and updates selected frontend dependency versions. ChangesEmail and upload security validation
Frontend dependency updates
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: π₯ Pre-merge checks | β 4β Passed checks (4 passed)
β¨ Finishing Touchesπ Generate docstrings
π§ͺ Generate unit tests (beta)
Comment |
|
PR governance metadata gate is not ready for
|
There was a problem hiding this comment.
Pull request overview
Hardens the email API surface against SMTP CRLF/header injection and file-upload double-extension bypasses, reducing risk of unauthorized email actions and malicious upload acceptance.
Changes:
- Added a Pydantic
@field_validator(mode="before")to reject CR/LF characters in header-like request fields for/api/emails/send. - Reworked
/api/emails/import-filesfilename validation to require an allowed final extension and reject embedded dangerous extensions. - Documented the vulnerability and prevention pattern in the Sentinel security log.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| backend/api/emails.py | Adds CRLF rejection for email header fields and strengthens upload filename extension validation. |
| .jules/sentinel.md | Records the vulnerability/learning/prevention notes (but currently duplicates an existing entry). |
Comments suppressed due to low confidence (1)
backend/api/emails.py:589
- Filename validation splits on '.' but doesn't normalize individual segments. A filename like
malware.exe .eml(space before the dot) will produce the segmentexewhich won't match.exe, potentially allowing a double-extension bypass on platforms/tools that trim/normalize whitespace in filenames. Strip each segment (or otherwise normalize) before checking allowed/dangerous extensions.
if len(segments) < 2 or ("." + segments[-1]) not in allowed_extensions:
raise HTTPException(status_code=400, detail="invalid_file_type")
dangerous_extensions = {".exe", ".sh", ".bat", ".cmd", ".msi", ".vbs", ".scr", ".pif", ".dll", ".com"}
if any(("." + seg) in dangerous_extensions for seg in segments[:-1]):
π‘ Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| allowed_extensions = {".eml", ".zip", ".mbox"} | ||
| segments = normalized_filename.split(".") | ||
| if len(segments) < 2 or ("." + segments[-1]) not in allowed_extensions: | ||
| raise HTTPException(status_code=400, detail="invalid_file_type") |
| @field_validator("to", "subject", "in_reply_to", "references", mode="before") | ||
| @classmethod | ||
| def reject_crlf(cls, v: str | None) -> str | None: | ||
| if v is None: | ||
| return v | ||
| if not isinstance(v, str): | ||
| return v | ||
| if chr(10) in v or chr(13) in v: | ||
| raise ValueError("Email header fields must not contain newlines") | ||
| return v |
| ## 2024-06-25 - [Fix Email SMTP CRLF Injection & Double Extension Upload] | ||
| **Vulnerability:** Attackers could inject arbitrary SMTP commands (e.g. MAIL FROM) using CRLF (\r\n) sequences in email subjects or recipients because `^[^\r\n]*$` validation in Pydantic wasn't catching all edge cases correctly. Attackers could also bypass file upload validations by providing double extensions (e.g., `malicious.exe.eml`). | ||
| **Learning:** Pydantic regex patterns might fall short for strict network protocol inputs like SMTP headers if improperly formulated or bypassed. Simple `.endswith()` checks for file uploads fail to prevent embedded dangerous extensions. | ||
| **Prevention:** Always use `@field_validator` with explicit `mode="before"` string matching for `chr(10)` and `chr(13)` across all user-controlled email header fields (to, subject, in_reply_to, references). Always tokenize uploaded filenames via `.split(".")` and reject if any segment matches a known dangerous extension (e.g., `.exe`, `.sh`). |
μ΄ μ»€λ°μ Trivy νμΌμμ€ν μ€μΊμμ λ°κ²¬λ μ·¨μ½μ (CVE)μ μμ νκΈ° μν΄ νλ‘ νΈμλ ν¨ν€μ§λ₯Ό μ λ°μ΄νΈν©λλ€. 1. `next` ν¨ν€μ§λ₯Ό CVEκ° ν¨μΉλ 16.2.12 λ²μ μΌλ‘ μ λ°μ΄νΈν©λλ€. 2. `sharp` ν¨ν€μ§λ₯Ό 0.35.3 λ²μ μΌλ‘ μ λ°μ΄νΈνμ¬ κ΄λ ¨λ μ·¨μ½μ μ ν΄κ²°ν©λλ€. 3. `postcss`λ₯Ό μ λ°μ΄νΈλ μμ‘΄μ±μ λ§μΆμ΄ 8.5.23 λ²μ μΌλ‘ μ€μ ν©λλ€.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 117-120: Remove the duplicate 2024-06-25 sentinel entry from
.jules/sentinel.md, preserving a single authoritative heading and its guidance.
Keep the remaining entry unchanged.
In `@backend/api/emails.py`:
- Around line 584-589: Normalize each segment in the filename validation flow
before comparing against dangerous_extensions, including trimming whitespace or
rejecting whitespace-padded segments so values like βexe β cannot bypass the
check. Update the logic around normalized_filename and the dangerous_extensions
check, and add a regression test covering a filename such as malware.exe .eml.
In `@frontend/package.json`:
- Line 46: Update the PostCSS version constraints in package.json overrides,
package.json resolutions, and the frontend/.pnpmfile.cjs configuration from the
older ranges to 8.5.23, then regenerate the lockfile so all direct and
transitive resolutions align with the patched release.
πͺ 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: 0e82f096-dec3-4cb3-a511-26e875a8c41f
β Files ignored due to path filters (1)
frontend/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
π Files selected for processing (3)
.jules/sentinel.mdbackend/api/emails.pyfrontend/package.json
| ## 2024-06-25 - [Fix Email SMTP CRLF Injection & Double Extension Upload] | ||
| **Vulnerability:** Attackers could inject arbitrary SMTP commands (e.g. MAIL FROM) using CRLF (\r\n) sequences in email subjects or recipients because `^[^\r\n]*$` validation in Pydantic wasn't catching all edge cases correctly. Attackers could also bypass file upload validations by providing double extensions (e.g., `malicious.exe.eml`). | ||
| **Learning:** Pydantic regex patterns might fall short for strict network protocol inputs like SMTP headers if improperly formulated or bypassed. Simple `.endswith()` checks for file uploads fail to prevent embedded dangerous extensions. | ||
| **Prevention:** Always use `@field_validator` with explicit `mode="before"` string matching for `chr(10)` and `chr(13)` across all user-controlled email header fields (to, subject, in_reply_to, references). Always tokenize uploaded filenames via `.split(".")` and reject if any segment matches a known dangerous extension (e.g., `.exe`, `.sh`). |
There was a problem hiding this comment.
π Maintainability & Code Quality | π‘ Minor | β‘ Quick win
Remove the duplicate sentinel entry.
This section duplicates lines 94-97 verbatim, producing duplicate headings and redundant guidance. Keep one authoritative entry.
π§° Tools
πͺ markdownlint-cli2 (0.23.0)
[warning] 117-117: Multiple headings with the same content
(MD024, no-duplicate-heading)
π€ 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 @.jules/sentinel.md around lines 117 - 120, Remove the duplicate 2024-06-25
sentinel entry from .jules/sentinel.md, preserving a single authoritative
heading and its guidance. Keep the remaining entry unchanged.
Source: Linters/SAST tools
| segments = normalized_filename.split(".") | ||
| if len(segments) < 2 or ("." + segments[-1]) not in allowed_extensions: | ||
| raise HTTPException(status_code=400, detail="invalid_file_type") | ||
|
|
||
| dangerous_extensions = {".exe", ".sh", ".bat", ".cmd", ".msi", ".vbs", ".scr", ".pif", ".dll", ".com"} | ||
| if any(("." + seg) in dangerous_extensions for seg in segments[:-1]): |
There was a problem hiding this comment.
π Security & Privacy | π Major | β‘ Quick win
Normalize each filename segment before checking dangerous extensions.
strip() only trims the whole filename, so malware.exe .eml produces an earlier segment of exe and bypasses the .exe check. Normalize or reject whitespace-padded segments and add a regression test.
Proposed fix
- segments = normalized_filename.split(".")
+ segments = [segment.strip() for segment in normalized_filename.split(".")]π Committable suggestion
βΌοΈ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| segments = normalized_filename.split(".") | |
| if len(segments) < 2 or ("." + segments[-1]) not in allowed_extensions: | |
| raise HTTPException(status_code=400, detail="invalid_file_type") | |
| dangerous_extensions = {".exe", ".sh", ".bat", ".cmd", ".msi", ".vbs", ".scr", ".pif", ".dll", ".com"} | |
| if any(("." + seg) in dangerous_extensions for seg in segments[:-1]): | |
| segments = [segment.strip() for segment in normalized_filename.split(".")] | |
| if len(segments) < 2 or ("." + segments[-1]) not in allowed_extensions: | |
| raise HTTPException(status_code=400, detail="invalid_file_type") | |
| dangerous_extensions = {".exe", ".sh", ".bat", ".cmd", ".msi", ".vbs", ".scr", ".pif", ".dll", ".com"} | |
| if any(("." + seg) in dangerous_extensions for seg in segments[:-1]): |
π€ 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 `@backend/api/emails.py` around lines 584 - 589, Normalize each segment in the
filename validation flow before comparing against dangerous_extensions,
including trimming whitespace or rejecting whitespace-padded segments so values
like βexe β cannot bypass the check. Update the logic around normalized_filename
and the dangerous_extensions check, and add a regression test covering a
filename such as malware.exe .eml.
| "fast-check": "^4.9.0", | ||
| "jsdom": "^29.1.0", | ||
| "postcss": "^8.5.16", | ||
| "postcss": "^8.5.23", |
There was a problem hiding this comment.
π Security & Privacy | π Major | β‘ Quick win
𧩠Analysis chain
π Script executed:
cd frontend
rg -n '"postcss"|postcss:' package.json .pnpmfile.cjs
rg -n 'postcss@|version: 8\.5\.' pnpm-lock.yaml
pnpm list postcss --depth InfinityRepository: ContextualWisdomLab/naruon
Length of output: 1239
π Script executed:
cd frontend
printf '\n== package.json ==\n'
cat -n package.json | sed -n '40,65p'
printf '\n== .pnpmfile.cjs ==\n'
cat -n .pnpmfile.cjs | sed -n '1,120p'
printf '\n== pnpm-lock postcss hits ==\n'
rg -n 'postcss@8\.5\.|version: 8\.5\.|postcss:' pnpm-lock.yamlRepository: ContextualWisdomLab/naruon
Length of output: 1545
Align all PostCSS constraints with 8.5.23
frontend/package.json already updates the direct dependency, but overrides, resolutions, and frontend/.pnpmfile.cjs still force ^8.5.16/^8.5.15. Those lower ranges can leave transitive installs below the patched release, so bump all three to 8.5.23 and refresh the lockfile.
π€ 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 `@frontend/package.json` at line 46, Update the PostCSS version constraints in
package.json overrides, package.json resolutions, and the frontend/.pnpmfile.cjs
configuration from the older ranges to 8.5.23, then regenerate the lockfile so
all direct and transitive resolutions align with the patched release.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.
Files not reviewed (1)
- frontend/pnpm-lock.yaml: Generated file
Comments suppressed due to low confidence (2)
backend/api/emails.py:589
- This introduces new security behavior (rejecting double extensions like
malware.exe.eml), but there is no regression test coverage for the new filename validation. Please add API tests that assertimport-filesrejects embedded dangerous extensions and accepts valid.eml/.zip/.mboxnames.
if any(("." + seg) in dangerous_extensions for seg in segments[:-1]):
.jules/sentinel.md:120
- This Sentinel entry is duplicated: the same "2024-06-25 - [Fix Email SMTP CRLF Injection & Double Extension Upload]" section already exists earlier in the file. Please remove the duplicate block to keep the log unambiguous.
## 2024-06-25 - [Fix Email SMTP CRLF Injection & Double Extension Upload]
**Vulnerability:** Attackers could inject arbitrary SMTP commands (e.g. MAIL FROM) using CRLF (\r\n) sequences in email subjects or recipients because `^[^\r\n]*$` validation in Pydantic wasn't catching all edge cases correctly. Attackers could also bypass file upload validations by providing double extensions (e.g., `malicious.exe.eml`).
**Learning:** Pydantic regex patterns might fall short for strict network protocol inputs like SMTP headers if improperly formulated or bypassed. Simple `.endswith()` checks for file uploads fail to prevent embedded dangerous extensions.
**Prevention:** Always use `@field_validator` with explicit `mode="before"` string matching for `chr(10)` and `chr(13)` across all user-controlled email header fields (to, subject, in_reply_to, references). Always tokenize uploaded filenames via `.split(".")` and reject if any segment matches a known dangerous extension (e.g., `.exe`, `.sh`).
| raise HTTPException(status_code=400, detail="invalid_file_type") | ||
|
|
||
| allowed_extensions = {".eml", ".zip", ".mbox"} | ||
| segments = normalized_filename.split(".") |
|
Superseded by #1206 on current develop (document organization_id scope and/or SMTP CRLF header rejection reimplemented with tests). |
π¨ Severity: HIGH
π‘ Vulnerability:
to,subjectλ±)μ CRLF(\r\n) μ£Όμ μ΄ κ°λ₯νμ¬ μμμ SMTP λͺ λ Ήμ΄(μ: MAIL FROM)κ° μ€νλ μ μμμ΅λλ€ (CRLF Injection)..endswith()μ μμ‘΄νμ¬ μ€κ°μ μ μ± νμ₯μκ° ν¬ν¨λ μ΄μ€ νμ₯μ νμΌ(μ:malware.exe.eml)μ ν΅κ³Όμν¬ μ μμμ΅λλ€.π― Impact: 곡격μκ° κΆν μλ μ΄λ©μΌ μ μ‘(μ€νΈ λ±)μ μννκ±°λ, μμ€ν μ μ μ± μ€ν¬λ¦½νΈ/μ€ν νμΌμ μ λ‘λν μ μμμ΅λλ€.
π§ Fix:
backend/api/emails.pyμSendEmailRequestλͺ¨λΈμ@field_validator(mode="before")λ₯Ό μΆκ°νμ¬ μ΄λ©μΌ ν€λ κ΄λ ¨ νλμ μμ€ λ¬Έμ(CRLF)κ° ν¬ν¨λ κ²½μ° λͺ μμ μΌλ‘ κ±°λΆνλλ‘ μμ νμ΅λλ€.import_email_files)μμ νμΌ μ΄λ¦μ.μΌλ‘ λΆν νμ¬ λͺ¨λ μΈκ·Έλ¨ΌνΈλ₯Ό κ²μ¬νκ³ μ μ± νμ₯μ(μ:.exe,.sh)κ° ν¬ν¨λ κ²½μ° μ λ‘λλ₯Ό μ°¨λ¨νλλ‘ μμ νμ΅λλ€.β Verification:
cd backend && uv run pytest tests/test_emails_api.pyλ₯Ό μ€ννμ¬ ν΄λΉ μλν¬μΈνΈμ ν μ€νΈκ° λͺ¨λ ν΅κ³Όλ¨μ νμΈνμ΅λλ€.PR created automatically by Jules for task 8488562200955951224 started by @seonghobae
Summary by CodeRabbit