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

Set response headers based on Rack version #2355

Merged
merged 12 commits into from
Oct 24, 2023
10 changes: 10 additions & 0 deletions .github/workflows/test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,16 @@ jobs:
if: ${{ matrix.gemfile == 'multi_xml' }}
run: bundle exec rspec spec/integration/multi_xml

- name: Run tests (spec/integration/rack/v2)
# rack_2_0.gemfile is equals to Gemfile
if: ${{ matrix.gemfile == 'rack_2_0' }}
run: bundle exec rspec spec/integration/rack/v2

- name: Run tests (spec/integration/rack/v3)
# rack_2_0.gemfile is equals to Gemfile
if: ${{ matrix.gemfile == 'rack_3_0' }}
run: bundle exec rspec spec/integration/rack/v3

- name: Coveralls
uses: coverallsapp/github-action@master
with:
Expand Down
2 changes: 2 additions & 0 deletions .rubocop_todo.yml
Original file line number Diff line number Diff line change
Expand Up @@ -272,6 +272,8 @@ RSpec/FilePath:
- 'spec/integration/eager_load/eager_load_spec.rb'
- 'spec/integration/multi_json/json_spec.rb'
- 'spec/integration/multi_xml/xml_spec.rb'
- 'spec/integration/rack/v2/headers_spec.rb'
- 'spec/integration/rack/v3/headers_spec.rb'

# Offense count: 12
# Configuration parameters: Max.
Expand Down
5 changes: 3 additions & 2 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
### 1.8.1 (Next)
### 1.9.0 (Next)

#### Features

* [#2353](https://github.com/ruby-grape/grape/pull/2353): Added Rails 7.1 support - [@ericproulx](https://github.com/ericproulx).
* [#2355](https://github.com/ruby-grape/grape/pull/2355): Set response headers based on Rack version - [@schinery](https://github.com/schinery).
* [#2360](https://github.com/ruby-grape/grape/pull/2360): Reduce gem size by removing specs - [@ericproulx](https://github.com/ericproulx).
* Your contribution here.

#### Fixes

* Your contribution here.
Expand Down
7 changes: 4 additions & 3 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@ content negotiation, versioning and much more.

## Stable Release

You're reading the documentation for the next release of Grape, which should be **1.8.1**.
You're reading the documentation for the next release of Grape, which should be **1.9.0**.
Please read [UPGRADING](UPGRADING.md) when upgrading from a previous version.
The current stable release is [1.8.0](https://github.com/ruby-grape/grape/blob/v1.8.0/README.md).

Expand Down Expand Up @@ -2130,8 +2130,9 @@ curl -H "secret_PassWord: swordfish" ...

The header name will have been normalized for you.

- In the `header` helper names will be coerced into a capitalized kebab case.
- In the `env` collection they appear in all uppercase, in snake case, and prefixed with 'HTTP_'.
- In the `header` helper names will be coerced into a downcased kebab case as `secret-password` if using Rack 3.
- In the `header` helper names will be coerced into a capitalized kebab case as `Secret-PassWord` if using Rack < 3.
- In the `env` collection they appear in all uppercase, in snake case, and prefixed with 'HTTP_' as `HTTP_SECRET_PASSWORD`

The header name will have been normalized per HTTP standards defined in [RFC2616 Section 4.2](https://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html#sec4.2) regardless of what is being sent by a client.

Expand Down
29 changes: 29 additions & 0 deletions UPGRADING.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,35 @@
Upgrading Grape
===============

### Upgrading to >= 1.9.0

#### Headers

As per [rack/rack#1592](https://github.com/rack/rack/issues/1592) Rack 3.0 is enforcing the HTTP/2 semantics, and thus treats all headers as lowercase. Starting with Grape 1.9.0, headers will be cased based on what version of Rack you are using.
Copy link
Contributor

Choose a reason for hiding this comment

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

Rack 3.0 is enforcing the HTTP/2 semantics, and thus treats all headers as lowercase. Starting with Grape 1.9.0, headers will be cased based on what version of Rack you are using.

Rack 3 is following the HTTP/2+ semantics which require header names to be lower case. To avoid compatibility issues, starting with Grape 1.9.0, headers will be cased based on what version of Rack you are using.


Given this request:

```shell
curl -H "Content-Type: application/json" -H "Secret-Password: foo" ...
```

If you are using Rack 3 in your application then the headers will be set to:

```ruby
{ "content-type" => "application/json", "secret-password" => "foo"}
```

This means if you are checking for header values in your application, you would need to change your code to use downcased keys.

```ruby
get do
# This would use headers['Secret-Password'] in Rack < 3
error!('Unauthorized', 401) unless headers['secret-password'] == 'swordfish'
end
```

See [#2355](https://github.com/ruby-grape/grape/pull/2355) for more information.

dblock marked this conversation as resolved.
Show resolved Hide resolved
### Upgrading to >= 1.7.0

#### Exceptions renaming
Expand Down
4 changes: 4 additions & 0 deletions lib/grape.rb
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,10 @@ def self.deprecator
@deprecator ||= ActiveSupport::Deprecation.new('2.0', 'Grape')
end

def self.rack3?
Gem::Version.new(::Rack.release) >= Gem::Version.new('3')
Copy link
Contributor

Choose a reason for hiding this comment

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

If you are just trying to detect whether to use lower case headers or not, I suggest a slightly different approach:

LOWER_CASE_HEADERS = Rack::CONTENT_TYPE == "content-type"

You could then use that directly or in a method.

end

eager_autoload do
autoload :API
autoload :Endpoint
Expand Down
8 changes: 4 additions & 4 deletions lib/grape/dsl/inside_route.rb
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@ def redirect(url, permanent: false, body: nil, **_options)
status 302
body_message ||= "This resource has been moved temporarily to #{url}."
end
header 'Location', url
header Grape::Http::Headers::LOCATION, url
content_type 'text/plain'
body body_message
end
Expand Down Expand Up @@ -328,9 +328,9 @@ def sendfile(value = nil)
def stream(value = nil)
return if value.nil? && @stream.nil?

header 'Content-Length', nil
header 'Transfer-Encoding', nil
header 'Cache-Control', 'no-cache' # Skips ETag generation (reading the response up front)
header Grape::Http::Headers::CONTENT_LENGTH, nil
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use Rack::CONTENT_LENGTH if that suits your use case.

header Grape::Http::Headers::TRANSFER_ENCODING, nil
header Grape::Http::Headers::CACHE_CONTROL, 'no-cache' # Skips ETag generation (reading the response up front)
if value.is_a?(String)
file_body = Grape::ServeStream::FileBody.new(value)
@stream = Grape::ServeStream::StreamResponse.new(file_body)
Expand Down
2 changes: 1 addition & 1 deletion lib/grape/endpoint.rb
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,7 @@ def run
if (allowed_methods = env[Grape::Env::GRAPE_ALLOWED_METHODS])
raise Grape::Exceptions::MethodNotAllowed.new(header.merge('Allow' => allowed_methods)) unless options?

header 'Allow', allowed_methods
header Grape::Http::Headers::ALLOW, allowed_methods
response_object = ''
status 204
else
Expand Down
20 changes: 18 additions & 2 deletions lib/grape/http/headers.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,24 @@ module Headers
PATH_INFO = 'PATH_INFO'
REQUEST_METHOD = 'REQUEST_METHOD'
QUERY_STRING = 'QUERY_STRING'
CONTENT_TYPE = 'Content-Type'

if Grape.rack3?
ALLOW = 'allow'
CACHE_CONTROL = 'cache-control'
CONTENT_LENGTH = 'content-length'
CONTENT_TYPE = 'content-type'
LOCATION = 'location'
TRANSFER_ENCODING = 'transfer-encoding'
X_CASCADE = 'x-cascade'
else
ALLOW = 'Allow'
CACHE_CONTROL = 'Cache-Control'
CONTENT_LENGTH = 'Content-Length'
CONTENT_TYPE = 'Content-Type'
LOCATION = 'Location'
TRANSFER_ENCODING = 'Transfer-Encoding'
X_CASCADE = 'X-Cascade'
end

GET = 'GET'
POST = 'POST'
Expand All @@ -24,7 +41,6 @@ module Headers
SUPPORTED_METHODS_WITHOUT_OPTIONS = Grape::Util::LazyObject.new { [GET, POST, PUT, PATCH, DELETE, HEAD].freeze }

HTTP_ACCEPT_VERSION = 'HTTP_ACCEPT_VERSION'
X_CASCADE = 'X-Cascade'
HTTP_TRANSFER_ENCODING = 'HTTP_TRANSFER_ENCODING'
HTTP_ACCEPT = 'HTTP_ACCEPT'

Expand Down
10 changes: 8 additions & 2 deletions lib/grape/request.rb
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,14 @@ def build_headers
end
end

def transform_header(header)
-header[5..].split('_').each(&:capitalize!).join('-')
if Grape.rack3?
def transform_header(header)
-header[5..].tr('_', '-').downcase
end
else
def transform_header(header)
-header[5..].split('_').map(&:capitalize).join('-')
end
end
end
end
2 changes: 1 addition & 1 deletion lib/grape/version.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,5 +2,5 @@

module Grape
# The current version of Grape.
VERSION = '1.8.1'
VERSION = '1.9.0'
end
12 changes: 9 additions & 3 deletions spec/grape/api/custom_validations_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -162,13 +162,19 @@ def validate(request)
return unless request.params.key? @attrs.first
# check if admin flag is set to true
return unless @option

# check if user is admin or not
# as an example get a token from request and check if it's admin or not
raise Grape::Exceptions::Validation.new(params: @attrs, message: 'Can not set Admin only field.') unless request.headers['X-Access-Token'] == 'admin'
raise Grape::Exceptions::Validation.new(params: @attrs, message: 'Can not set Admin only field.') unless request.headers[access_header] == 'admin'
Copy link
Member

Choose a reason for hiding this comment

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

Could use x_access_token_header here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, tried that, but got...

     NameError:
       undefined local variable or method `x_access_token_header' for #<#<Class:0x0000ffff8f8f9b18>:0x0000ffff928d97e8 @attrs=[:admin_field], @option=true, @required=false, @scope=#<Grape::Validations::ParamsScope:0x0000ffff8f8f96b8 @element=nil, @element_renamed=nil, @parent=nil, @api=#<Class:0x0000ffff8f8f9898>, @optional=false, @type=Hash, @group=nil, @dependent_on=nil, @declared_params=nil, @index=nil>, @fail_fast=false, @allow_blank=nil>

which was annoying, as don't like the defining it twice either.

end

def access_header
Grape.rack3? ? 'x-access-token' : 'X-Access-Token'
end
end
end
let(:app) { Rack::Builder.new(subject) }
let(:x_access_token_header) { Grape.rack3? ? 'x-access-token' : 'X-Access-Token' }

before do
described_class.register_validator('admin', admin_validator)
Expand Down Expand Up @@ -197,14 +203,14 @@ def validate(request)
end

it 'does not fail when we send admin fields and we are admin' do
header 'X-Access-Token', 'admin'
header x_access_token_header, 'admin'
get '/', admin_field: 'tester', non_admin_field: 'toaster', admin_false_field: 'test'
expect(last_response.status).to eq 200
expect(last_response.body).to eq 'bacon'
end

it 'fails when we send admin fields and we are not admin' do
header 'X-Access-Token', 'user'
header x_access_token_header, 'user'
get '/', admin_field: 'tester', non_admin_field: 'toaster', admin_false_field: 'test'
expect(last_response.status).to eq 400
expect(last_response.body).to include 'Can not set Admin only field.'
Expand Down
Loading