Skip to content

fix(HTTP): strip CRLF from DownloadResponse filename to prevent header injection - #10533

Closed
gr8man wants to merge 2 commits into
codeigniter4:developfrom
gr8man:fix/download-response-crlf
Closed

fix(HTTP): strip CRLF from DownloadResponse filename to prevent header injection#10533
gr8man wants to merge 2 commits into
codeigniter4:developfrom
gr8man:fix/download-response-crlf

Conversation

@gr8man

@gr8man gr8man commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Description
In DownloadResponse, filenames with carriage return (\r) or newline (\n) characters could cause RFC 7230 header validation errors or lead to HTTP header injection / response splitting.

This PR strips \r and \n characters from the filename when setting it on DownloadResponse.

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value (without duplication)
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

Comment thread system/HTTP/DownloadResponse.php Outdated
{
$filename = $this->filename;
$x = explode('.', $this->filename);
$filename = str_replace(["\r", "\n"], '', $this->filename);

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.

If we don’t use Reflection magic, then there’s no need to sanitize the file name — it’s already safe from the setter.

…r injection

Sanitize carriage return and newline characters from filename in DownloadResponse to prevent HTTP response splitting and invalid header values per RFC 7230.
@gr8man
gr8man force-pushed the fix/download-response-crlf branch from 5d6e5ff to 87ac5fa Compare September 9, 2026 18:34

@michalsn michalsn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks, but running the new tests against the unchanged develop source already throws an InvalidArgumentException before headers are sent. I also checked the constructor and setter cases separately - both are rejected. Without a clear justification for changing from rejection to silent sanitization, I'm not comfortable merging this.

Please report suspected security vulnerabilities privately through our security reporting process.

@gr8man gr8man closed this Sep 10, 2026
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.

3 participants