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

Allow @ in X-Request-Id header #31815

Merged
merged 1 commit into from Jan 30, 2018

Conversation

Projects
None yet
4 participants
@composerinteralia
Member

composerinteralia commented Jan 28, 2018

It makes sense to be as strict as possible
with headers from the outside world, but I
believe the primary concern is CRLF injection.
Allowing @ to support Apache's mod_unique_id
(see #31644) seems OK to me, but I definitely
want to get confirmation from an expert.

@rails-bot

This comment has been minimized.

rails-bot commented Jan 28, 2018

r? @georgeclaghorn

(@rails-bot has picked a reviewer for you, use r? to override)

actionpack/test/dispatch/request_id_test.rb Outdated
@@ -8,7 +8,7 @@ class RequestIdTest < ActiveSupport::TestCase
end
test "ensure that only alphanumeric uurids are accepted" do
assert_equal "X-Hacked-HeaderStuff", stub_request("HTTP_X_REQUEST_ID" => "; X-Hacked-Header: Stuff").request_id
assert_equal "X-Hacked-HeaderStuff@", stub_request("HTTP_X_REQUEST_ID" => ";\r\n X-Hacked-Header: Stuff@").request_id

This comment has been minimized.

@matthewd

matthewd Jan 28, 2018

Member

Let's be a little more explicit about this, and add a new test that uses an actual Apache mod_request_id value, and labels it as such. Otherwise @ seems a pretty arbitrary extension to the alphabet needed to cover UUIDs.

@composerinteralia composerinteralia force-pushed the composerinteralia:make-request-id branch 3 times, most recently Jan 28, 2018

actionpack/test/dispatch/request_id_test.rb Outdated
@@ -8,7 +8,12 @@ class RequestIdTest < ActiveSupport::TestCase
end
test "ensure that only alphanumeric uurids are accepted" do
assert_equal "X-Hacked-HeaderStuff", stub_request("HTTP_X_REQUEST_ID" => "; X-Hacked-Header: Stuff").request_id
assert_equal "X-Hacked-HeaderStuff", stub_request("HTTP_X_REQUEST_ID" => ";\r\n X-Hacked-Header: Stuff").request_id

This comment has been minimized.

@georgeclaghorn

georgeclaghorn Jan 30, 2018

Member

Why did this test need to change?

This comment has been minimized.

@composerinteralia

composerinteralia Jan 30, 2018

Member

It doesn't, really. I will update the PR now.

@composerinteralia composerinteralia force-pushed the composerinteralia:make-request-id branch Jan 30, 2018

Allow @ in X-Request-Id header
It makes sense to be as strict as possible
with headers from the outside world,
but allowing @ to support Apache's mod_unique_id
(see #31644) seems OK to me

@composerinteralia composerinteralia force-pushed the composerinteralia:make-request-id branch to eea28f4 Jan 30, 2018

@georgeclaghorn georgeclaghorn merged commit 8ef0751 into rails:master Jan 30, 2018

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
codeclimate All good!
Details

@composerinteralia composerinteralia deleted the composerinteralia:make-request-id branch Jan 30, 2018

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