Skip to content

Fix StaticFileHandler to return 404 on file error #16025

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.

Sign up for GitHub

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

Jump to bottom
Draft
wants to merge 5 commits into
base: master
Choose a base branch
from
Draft

Fix StaticFileHandler to return 404 on file error #16025

wants to merge 5 commits into from
+37 −9

Conversation

straight-shoota
Copy link
Member

Resolves #15901

@straight-shoota straight-shoota self-assigned this Jul 23, 2025
@straight-shoota straight-shoota added kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:stdlib:networking labels Jul 23, 2025
@straight-shoota straight-shoota marked this pull request as draft July 23, 2025 18:15
@straight-shoota
Copy link
Member Author

I don't get why the new spec fails on Aarch64 CI and in the interpreter only. Aarch64 CI is the runs-on environment which uses a different base image than the hosted runners from GitHub. But interpreter specs should run in exactly the same environment as other jobs. 🤷
Even the guard to make sure the file isn't readable didn't help. The file has no permissions. Yet still, apparently the file handler can still read its contents. This is very weird.

At last, I added a File.read test in order to properly detect whether the file is actually readable. If anyone has any better idea, I'm happy for suggestions. Maybe we can introduce some other kind of error?

ysbaddaden
ysbaddaden reviewed Jul 25, 2025
View reviewed changes
spec/std/http/server/handlers/static_file_handler_spec.cr

# FIXME: Setting permissions does not work on all systems. Even the
# permissions recheck is not sufficient (see https://github.com/crystal-lang/crystal/pull/16025#issuecomment-3112225515).
pending! if File.info("forbidden.txt").permissions.owner_read? || (File.read("forbidden.txt") rescue nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

If it's only the interpreter, then maybe we can skip only when interpreted?

Copy link
Member Author

Choose a reason for hiding this comment

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

With the File.read check, it seems good now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ysbaddaden ysbaddaden ysbaddaden left review comments

@mverzilli mverzilli mverzilli approved these changes

Assignees

@straight-shoota straight-shoota

Labels
kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:stdlib:networking
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

HTTP::StaticFileHandler raises an exception when given a long filename
3 participants
@straight-shoota @ysbaddaden @mverzilli