Skip to content

Conversation

@alexanderadam
Copy link
Contributor

@alexanderadam alexanderadam commented Oct 21, 2025

I have no idea whether this was somehow intentionally done like this or whether that's actually something that intentionally failing or maybe something else.

However, I just thought that I could do this quickly and check what you're thinking instead of a long discussion. 😉

PS: I'm looking for a new adventure in case anybody is looking to hire or work with a Ruby/Rails/Crystal dev

@grape-bot
Copy link

1 Warning
⚠️ Unless you're refactoring existing code or improving documentation, please update CHANGELOG.md.
1 Message
📖 We really appreciate pull requests that demonstrate issues, even without a fix. That said, the next step is to try and fix the failing tests!

Here's an example of a CHANGELOG.md entry:

* [#2613](https://github.com/ruby-grape/grape/pull/2613): Sendfile test fix - [@alexanderadam](https://github.com/alexanderadam).

Generated by 🚫 Danger

@dblock
Copy link
Member

dblock commented Oct 21, 2025

This definitely passed at some point, we should understand why the behavior changed. Are we getting a newer version of Rack now that has a code change? I'd like to avoid masking a real change accidentally - would you mind bisecting this down to a specific change in rack before we merge the fix?

@alexanderadam
Copy link
Contributor Author

I think that this was related to this vulnerability and the attached patches from Samuel Williams.

For the 3.2 release this was this patch:

To enable x-accel-redirect, you must configure the middleware explicitly:
use Rack::Sendfile, "x-accel-redirect"
For security reasons, the x-sendfile-type header from requests is ignored.
The sendfile variation must be set via the middleware constructor.

@dblock
Copy link
Member

dblock commented Oct 21, 2025

Perfect, thanks @alexanderadam and @ioquatix.

@dblock dblock merged commit 37d013b into ruby-grape:master Oct 21, 2025
52 checks passed
@alexanderadam alexanderadam deleted the fix/sendfile_test_fix branch October 23, 2025 22:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants