feat: Add Email Header Injection vulnerability module (#651) - #655
Open
atharv01h wants to merge 1 commit into
Open
feat: Add Email Header Injection vulnerability module (#651)#655atharv01h wants to merge 1 commit into
atharv01h wants to merge 1 commit into
Conversation
Contributor
📝 WalkthroughWalkthroughAdds an Email Header Injection module: Spring Mail dependency, EmailService (unsafe + sanitized) with JavaMailSender implementation, a three-level unsafe-profile REST controller, SMTP configuration, tests for success/failure and CRLF behaviors, plus attack-vector and i18n entries. ChangesEmail Header Injection Vulnerability Module
Sequence DiagramsequenceDiagram
participant Client
participant EmailHeaderInjection
participant EmailService
participant JavaMailSender
Client->>EmailHeaderInjection: Level 1 (toEmail, subject, body)
EmailHeaderInjection->>EmailService: sendEmail(to, subject, body)
EmailService->>JavaMailSender: send(SimpleMailMessage)
Client->>EmailHeaderInjection: Level 2 (toEmail with \n, subject)
EmailHeaderInjection->>EmailHeaderInjection: reject if \n in toEmail/subject
Client->>EmailHeaderInjection: Level 2 (toEmail with \r only, subject)
EmailHeaderInjection->>EmailService: sendEmail(to, subject, body)
EmailService->>JavaMailSender: send(SimpleMailMessage with CRLF intact)
Client->>EmailHeaderInjection: Level 3 (toEmail, subject, body)
EmailHeaderInjection->>EmailService: sendEmailSanitized(to, subject, body)
EmailService->>EmailService: sanitizeHeader removes \r\n
EmailService->>JavaMailSender: send(SimpleMailMessage sanitized)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 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 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/main/resources/attackvectors/EmailHeaderInjectionPayload.properties (2)
5-8: ⚡ Quick winConsider adding a space in the CR-only bypass example.
Similar to Level 1, the example
subject=Test%0DCC:attacker@evil.comomits whitespace between%0Dand theCC:header. For consistency and SMTP compliance, consider:
subject=Test%0D%20CC:attacker@evil.com🤖 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 `@src/main/resources/attackvectors/EmailHeaderInjectionPayload.properties` around lines 5 - 8, The EMAIL_HEADER_INJECTION_PAYLOAD_LEVEL_2 example uses a CR-only bypass string "subject=Test%0DCC:attacker@evil.com" which omits a space and may mislead readers; update the example in the EMAIL_HEADER_INJECTION_PAYLOAD_LEVEL_2 property to include an explicit space after %0D (use %20 or a literal space) so it reads like "subject=Test%0D%20CC:attacker@evil.com" to match Level 1 formatting and SMTP expectations.
1-4: ⚡ Quick winConsider adding a space in the payload example for SMTP compliance.
The example payload
subject=Test%0D%0ACC:attacker@evil.comomits whitespace between the CRLF sequence and theCC:header. While some SMTP implementations may accept this, RFC 5322 recommends optional folding whitespace (FWS) after the colon. For educational clarity and to demonstrate a more realistic attack, consider:
subject=Test%0D%0A%20CC:attacker@evil.comwhere
%20represents the space character.🤖 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 `@src/main/resources/attackvectors/EmailHeaderInjectionPayload.properties` around lines 1 - 4, The EMAIL_HEADER_INJECTION_PAYLOAD_LEVEL_1 example payload omits the optional folding whitespace after the CRLF; update the example value for the payload (the string under EMAIL_HEADER_INJECTION_PAYLOAD_LEVEL_1) to include a space (%20) after %0D%0A so the example reads with CRLF+space before the CC header (e.g., change the payload example from subject=Test%0D%0ACC:attacker@evil.com to include %20 after %0D%0A) and optionally add a short note that %20 represents a space character.
🤖 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
`@src/main/java/org/sasanlabs/service/vulnerability/emailHeaderInjection/EmailHeaderInjection.java`:
- Around line 180-181: The successResponse in EmailHeaderInjection (method
returning "Email sent (sanitized) to: " + toEmail + " with subject: " + subject)
still reflects raw header inputs; change it to avoid echoing unsanitized user
data by either returning a neutral message like "Email sent successfully (secure
delivery)" or by echoing only a safely sanitized form of toEmail/subject
produced by the existing sanitization routine used in this class; update the
return in the method that calls successResponse to use the neutral/sanitized
value and ensure any referenced symbols (toEmail, subject, successResponse,
EmailHeaderInjection) are used accordingly.
In `@src/main/resources/application.properties`:
- Around line 64-65: The two mail properties are hardcoded to false; make them
configurable via placeholders so deployments can enable SMTP auth/STARTTLS
through env/config. Replace spring.mail.properties.mail.smtp.auth and
spring.mail.properties.mail.smtp.starttls.enable values with property
placeholders that default to false (e.g., use an env/config-backed placeholder
like MAIL_SMTP_AUTH and MAIL_SMTP_STARTTLS with default false) so the values for
spring.mail.properties.mail.smtp.auth and
spring.mail.properties.mail.smtp.starttls.enable can be overridden at runtime.
- Around line 60-66: Add SMTP socket timeouts to avoid request-thread hangs by
defining spring.mail.properties.mail.smtp.connectiontimeout,
spring.mail.properties.mail.smtp.timeout, and
spring.mail.properties.mail.smtp.writetimeout in application.properties (use
sensible millisecond defaults and allow overriding via environment variables),
e.g. add entries for those three keys with a default like 10000ms so the mail
client won't block indefinitely; ensure the keys match exactly
(spring.mail.properties.mail.smtp.connectiontimeout,
spring.mail.properties.mail.smtp.timeout,
spring.mail.properties.mail.smtp.writetimeout) so they are picked up by the mail
subsystem.
---
Nitpick comments:
In `@src/main/resources/attackvectors/EmailHeaderInjectionPayload.properties`:
- Around line 5-8: The EMAIL_HEADER_INJECTION_PAYLOAD_LEVEL_2 example uses a
CR-only bypass string "subject=Test%0DCC:attacker@evil.com" which omits a space
and may mislead readers; update the example in the
EMAIL_HEADER_INJECTION_PAYLOAD_LEVEL_2 property to include an explicit space
after %0D (use %20 or a literal space) so it reads like
"subject=Test%0D%20CC:attacker@evil.com" to match Level 1 formatting and SMTP
expectations.
- Around line 1-4: The EMAIL_HEADER_INJECTION_PAYLOAD_LEVEL_1 example payload
omits the optional folding whitespace after the CRLF; update the example value
for the payload (the string under EMAIL_HEADER_INJECTION_PAYLOAD_LEVEL_1) to
include a space (%20) after %0D%0A so the example reads with CRLF+space before
the CC header (e.g., change the payload example from
subject=Test%0D%0ACC:attacker@evil.com to include %20 after %0D%0A) and
optionally add a short note that %20 represents a space character.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b9d13ee6-9211-4538-9bdf-4c648dce5c53
📒 Files selected for processing (10)
build.gradlesrc/main/java/org/sasanlabs/service/vulnerability/emailHeaderInjection/EmailHeaderInjection.javasrc/main/java/org/sasanlabs/service/vulnerability/emailHeaderInjection/EmailService.javasrc/main/java/org/sasanlabs/service/vulnerability/emailHeaderInjection/EmailServiceImpl.javasrc/main/java/org/sasanlabs/vulnerability/types/VulnerabilityType.javasrc/main/resources/application.propertiessrc/main/resources/attackvectors/EmailHeaderInjectionPayload.propertiessrc/main/resources/i18n/messages.propertiessrc/main/resources/i18n/messages_en_US.propertiessrc/test/java/org/sasanlabs/service/vulnerability/emailHeaderInjection/EmailHeaderInjectionTest.java
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #655 +/- ##
============================================
+ Coverage 54.69% 55.32% +0.62%
- Complexity 663 678 +15
============================================
Files 91 93 +2
Lines 3587 3637 +50
Branches 397 397
============================================
+ Hits 1962 2012 +50
Misses 1448 1448
Partials 177 177 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
atharv01h
force-pushed
the
fix/651-email-header-injection
branch
from
June 10, 2026 14:24
71c14ac to
7656dc1
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main/resources/application.properties (1)
71-71: Consider sanitizing the From address for defense-in-depth.The
vulnerableapp.email.fromproperty is used directly inEmailServiceImplwithout CRLF sanitization, even in Level 3 (the secure variant). While this value comes from configuration rather than user input, a misconfiguredEMAIL_FROMenvironment variable containing CRLF characters would inject additional headers and bypass the Level 3 sanitization.For defense-in-depth in a security teaching context, consider applying the same
sanitizeHeader()logic to the From address inEmailServiceImpl.🤖 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 `@src/main/resources/application.properties` at line 71, The From address from configuration (property vulnerableapp.email.from / env EMAIL_FROM) is used directly in EmailServiceImpl and should be passed through the existing sanitizeHeader() function to prevent CRLF injection; update EmailServiceImpl (where the From address is read/assigned and used to build messages) to call sanitizeHeader(fromAddress) and use the sanitized result everywhere (including Level 3 secure flow) so that any misconfigured environment value cannot inject extra headers.
🤖 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.
Nitpick comments:
In `@src/main/resources/application.properties`:
- Line 71: The From address from configuration (property
vulnerableapp.email.from / env EMAIL_FROM) is used directly in EmailServiceImpl
and should be passed through the existing sanitizeHeader() function to prevent
CRLF injection; update EmailServiceImpl (where the From address is read/assigned
and used to build messages) to call sanitizeHeader(fromAddress) and use the
sanitized result everywhere (including Level 3 secure flow) so that any
misconfigured environment value cannot inject extra headers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 33cae785-1a9f-497f-bba1-7b26848c431b
📒 Files selected for processing (10)
build.gradlesrc/main/java/org/sasanlabs/service/vulnerability/emailHeaderInjection/EmailHeaderInjection.javasrc/main/java/org/sasanlabs/service/vulnerability/emailHeaderInjection/EmailService.javasrc/main/java/org/sasanlabs/service/vulnerability/emailHeaderInjection/EmailServiceImpl.javasrc/main/java/org/sasanlabs/vulnerability/types/VulnerabilityType.javasrc/main/resources/application.propertiessrc/main/resources/attackvectors/EmailHeaderInjectionPayload.propertiessrc/main/resources/i18n/messages.propertiessrc/main/resources/i18n/messages_en_US.propertiessrc/test/java/org/sasanlabs/service/vulnerability/emailHeaderInjection/EmailHeaderInjectionTest.java
✅ Files skipped from review due to trivial changes (2)
- src/main/resources/i18n/messages_en_US.properties
- src/main/resources/i18n/messages.properties
🚧 Files skipped from review as they are similar to previous changes (7)
- src/main/java/org/sasanlabs/vulnerability/types/VulnerabilityType.java
- src/main/resources/attackvectors/EmailHeaderInjectionPayload.properties
- src/main/java/org/sasanlabs/service/vulnerability/emailHeaderInjection/EmailService.java
- src/main/java/org/sasanlabs/service/vulnerability/emailHeaderInjection/EmailHeaderInjection.java
- build.gradle
- src/test/java/org/sasanlabs/service/vulnerability/emailHeaderInjection/EmailHeaderInjectionTest.java
- src/main/java/org/sasanlabs/service/vulnerability/emailHeaderInjection/EmailServiceImpl.java
Member
|
@atharv01h is this ticket assigned to you ? If not I would suggest please look at the assignee and then only raise the PR. |
Contributor
|
This PR has had no activity for 30 days, so it's been marked stale. If you're still working on it, push a commit or leave a comment and this label will clear. Otherwise it'll close automatically in 60 days — no hard feelings, and you (or anyone else) are welcome to reopen or resubmit against current main whenever you're ready. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR implements the Email Header Injection (CWE-93) vulnerability module as requested in #651. It introduces a modular email sending service alongside a new three-level vulnerability controller to educate users on SMTP/Email Header Injection vulnerabilities and CRLF sanitization techniques.
Details
spring-boot-starter-mailand basic SMTP configurations with environment variables.EmailServiceandEmailServiceImplusingJavaMailSender.\nbut misses\r, which acts as a separator in some SMTP server configurations (bypassable).\r,\n) stripping.Summary by CodeRabbit