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

X-Cascade header Rack 3 issue #2354

Closed
schinery opened this issue Oct 15, 2023 · 7 comments
Closed

X-Cascade header Rack 3 issue #2354

schinery opened this issue Oct 15, 2023 · 7 comments

Comments

@schinery
Copy link
Contributor

schinery commented Oct 15, 2023

The X-Cascade header in Rack 3 applications appears to need to be lower case, and so it currently doesn't work as expected in Grape apps using Rack 3 using cascade: true.

The following spec, which passes in Rack 2, fails as the default response is returned from Grape instead.

it "raises ActionController::RoutingError" do
  expect { get "/api/foo" }.to raise_error(ActionController::RoutingError)
end

Manually updating my local Grape gem to use x-cascade results in the spec passing again.

@schinery schinery changed the title X-Cascade header issue Rails 7.1/Rack 3 issue X-Cascade header Rails 7.1/Rack 3 issue Oct 15, 2023
@schinery
Copy link
Contributor Author

Related to #2254

@schinery
Copy link
Contributor Author

This is what they are doing for Sidekiq Web:

    if Gem::Version.new(Rack::RELEASE) < Gem::Version.new("3")
      CONTENT_LANGUAGE = "Content-Language"
      CONTENT_SECURITY_POLICY = "Content-Security-Policy"
      LOCATION = "Location"
      X_CASCADE = "X-Cascade"
    else
      CONTENT_LANGUAGE = "content-language"
      CONTENT_SECURITY_POLICY = "content-security-policy"
      LOCATION = "location"
      X_CASCADE = "x-cascade"
    end

Would something similar in Grape Headers work?

@dblock
Copy link
Member

dblock commented Oct 16, 2023

It would probably work. Start by writing a Rails 6 and 7 spec?

@schinery
Copy link
Contributor Author

schinery commented Oct 16, 2023

Another option could be to make the X-Cascade and Content-Type (assuming this should also be changed) headers lowercase, as these should work fine in Rack < 3 apps.

Since HTTP/2 requires it and HTTP/1 works fine with it

rack/rack#1592 (comment)

@schinery
Copy link
Contributor Author

schinery commented Oct 16, 2023

@dblock I've thrown something together for both approaches, please let me know your thoughts and whether either of these is going in the right direction:

@dblock
Copy link
Member

dblock commented Oct 16, 2023

Thanks. Let's dig up why that was changed in Rack before deciding which way to go?

From rack/rack#1592

"I suggest with Rack 3.0 we take the opportunity to enforce the HTTP/2 semantics. It also simplifies the requirements around headers being a hash, to the point where I think it would be acceptable for Rack 3.0."

It feels like we should major-version increment grape if we're going to make a change like this, and lowercase all header names everywhere?

@schinery schinery changed the title X-Cascade header Rails 7.1/Rack 3 issue X-Cascade header Rack 3 issue Oct 23, 2023
@dblock
Copy link
Member

dblock commented Oct 24, 2023

Closed via #2355.

@dblock dblock closed this as completed Oct 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants