fix(mail): validate UID + reject header injection; fix IPv6 CSP host#955
Conversation
… CSP host Addresses gitar findings on the mail app (now on master): - IMAP: validate the client-supplied message UID is a bare numeric UID before passing it to conn.uid(FETCH, ...), closing an IMAP command-injection gap. - SMTP: reject CR/LF/NUL in to/cc/subject before assigning them to MIME headers, preventing header injection (hidden Bcc, spoofed headers). - CSP: _strip_port no longer corrupts a bracketed IPv6 host. Validation errors surface as 400 (MailValidationError) instead of a 502. Adds tests/test_mail_security.py (19 cases).
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughIntroduces ChangesMail Security Hardening & IPv6 Strip-Port Fix
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| msg["From"] = cfg.email_address or cfg.username | ||
| msg["To"] = to | ||
| msg["To"] = _validate_header(to, "to") | ||
| if cc: | ||
| msg["Cc"] = cc | ||
| msg["Subject"] = subject | ||
| msg["Cc"] = _validate_header(cc, "cc") | ||
| msg["Subject"] = _validate_header(subject, "subject") |
There was a problem hiding this comment.
💡 Security: From header not validated for CRLF injection
_build_outgoing now validates To, Cc, and Subject via _validate_header, but the From header (cfg.email_address or cfg.username, mail_client.py:340) is still assigned without validation. These values originate from client input at account-creation time (AccountCreate.email_address/username in routes/mail.py). If a CR/LF/NUL ever slips into the stored account record, it would reach the MIME From header and reopen the same header-injection vector this PR closes for the other fields.
This is lower severity than the addressed cases because the values are account-config rather than per-request, and the line is outside the modified region — but applying _validate_header here (or validating at account creation) would close the gap consistently.
Validate the From header value alongside To/Cc/Subject.:
msg["From"] = _validate_header(cfg.email_address or cfg.username, "from")
msg["To"] = _validate_header(to, "to")
Was this helpful? React with 👍 / 👎
Code Review 👍 Approved with suggestions 0 resolved / 1 findingsHardens mail processing by validating IMAP UIDs, blocking header injection in SMTP fields, and correcting IPv6 bracket parsing. Ensure the 💡 Security: From header not validated for CRLF injection📄 tinyagentos/mail_client.py:340-344
This is lower severity than the addressed cases because the values are account-config rather than per-request, and the line is outside the modified region — but applying Validate the From header value alongside To/Cc/Subject.🤖 Prompt for agentsOptionsAuto-apply is off → Gitar will not commit updates to this branch. Comment with these commands to change:
Was this helpful? React with 👍 / 👎 | Gitar |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| @@ -306,10 +338,10 @@ def _build_outgoing( | |||
| ) -> MIMEMultipart: | |||
| msg = MIMEMultipart() | |||
| msg["From"] = cfg.email_address or cfg.username | |||
There was a problem hiding this comment.
WARNING: From header is not validated for CRLF/NUL injection
cfg.email_address and cfg.username are account configuration values supplied by the user and later used in MIME/SMTP commands. Since To, Cc, and Subject are now validated, leaving From unchecked leaves the same header-injection class available if a stored account value contains CR/LF/NUL.
| msg["From"] = cfg.email_address or cfg.username | |
| msg["From"] = _validate_header(cfg.email_address or cfg.username, "from") |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| # bracket; for a normal "host:port" just drop the trailing port. | ||
| if host.startswith("["): | ||
| end = host.find("]") | ||
| return host[: end + 1] if end != -1 else host |
There was a problem hiding this comment.
WARNING: Malformed bracketed Host values are accepted after port stripping
_strip_port("[::1]:6969:extra") returns "[::1]" because it stops at the first ] and ignores the trailing suffix. That malformed Host then passes _SAFE_HOST_RE, so CSP frame-src may be generated from a syntactically invalid Host header. Require no trailing characters after the closing bracket, or validate the port suffix, before returning the bracketed host.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 3 Issues Found | Recommendation: Address before merge Overview
Fix these issues in Kilo Cloud Issue Details (click to expand)WARNING
Other Observations (not in diff)Issues found in unchanged code that cannot receive inline comments:
Files Reviewed (4 files)
Reviewed by nex-n2-pro:free · 1,066,858 tokens |
1 participant
Fix-forward for the gitar findings on the merged promotion #952 (this code is on master).
Findings addressed
_get_message_blockingpassed the client-supplieduidpath param straight intoconn.uid("FETCH", uid, ...). A non-numeric value could carry extra IMAP tokens. Now validated as a bare numeric UID (_validate_uid); the ids we hand out fromlist_messagesare always numeric UIDs, so nothing legitimate is rejected._build_outgoingassigned client values directly to MIME headers. CR/LF/NUL are now rejected (_validate_header), preventing injected headers (hidden Bcc, spoofed From)._strip_portcorrupts bracketed IPv6 hosts — fixed to keep everything up to the closing bracket. (Latent:_SAFE_HOST_REalready rejects colons, so this never reached the CSP output, but the helper is now correct.)Validation failures now return 400 (
MailValidationError, a base of the existingMailFolderError) instead of a generic 502.Tests
tests/test_mail_security.py— 19 cases covering numeric-UID acceptance, UID injection rejection, header CRLF rejection, and IPv6-safe port stripping. All green (pytest tests/test_mail_security.py19 passed).Scope: mail_client.py, routes/mail.py, middleware/security_headers.py, one new test file.
Summary by CodeRabbit
Tests
Bug Fixes