Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

actix-files: Properly handle newlines in file names #3235

Merged
merged 3 commits into from
Jan 6, 2024

Conversation

svenstaro
Copy link
Contributor

@svenstaro svenstaro commented Jan 5, 2024

PR Type

Bug Fix

PR Checklist

  • Tests for the changes have been added / updated.
  • Documentation comments have been added / updated.
  • A changelog entry has been made for the appropriate packages.
  • Format code with the latest stable rustfmt.
  • (Team) Label with affected crates and semver status.

Overview

This used to result in the issue I reported here: #3234
This shouldn't break anything as so far it was just a 500 error. Afterwards, it should be fixed. On Windows where newlines are illegal (I think), this shouldn't even get there.

Closes #3234.

@svenstaro svenstaro force-pushed the actix-files-newline branch 2 times, most recently from 8cc549f to 9284200 Compare January 5, 2024 17:58
Copy link
Member

@robjtede robjtede left a comment

Choose a reason for hiding this comment

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

nice one, thanks 👍🏻

@robjtede robjtede added this pull request to the merge queue Jan 6, 2024
@robjtede robjtede removed this pull request from the merge queue due to a manual request Jan 6, 2024
@robjtede robjtede merged commit febba78 into actix:master Jan 6, 2024
12 checks passed
@robjtede
Copy link
Member

robjtede commented Jan 6, 2024

released in v0.6.4

@Dialga
Copy link
Contributor

Dialga commented Jan 7, 2024

Thanks @svenstaro, that fixes my specific issue in the latest miniserve but I've just tried testing other whitespace characters such as:

echo hi > $'\r\n'rnl.txt
echo hi > $'\v\n'vnl.txt
echo hi > $'\f\t'fnl.txt

These still result in a server error:

500 Internal Server Error
failed to parse header value

There may be more cases, I haven't tried the whole ascii table.
Let's hope no one uses those file names!

@svenstaro svenstaro deleted the actix-files-newline branch January 7, 2024 03:24
@svenstaro
Copy link
Contributor Author

svenstaro commented Jan 7, 2024

I was unaware that these are also legal white spaces for common filesystems! Maybe @Dialga you could go ahead and amend my fix to contain other legal white spaces as well? Should be super straight forward to take my work and build on that by adding a few replace().

@Dialga
Copy link
Contributor

Dialga commented Jan 7, 2024

Hi I'm a noob with rust, but I'll take a shot!

@svenstaro
Copy link
Contributor Author

Hi I'm a noob with rust, but I'll take a shot!

Just check what I did and then do more of that. Don't forget the test case!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-files project: actix-files B-semver-patch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

actix-files: Can't handle files with newlines in them
3 participants