Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Validate header names according to RFC 2616 #399

Merged
merged 1 commit into from

5 participants

@novemberborn

Header name validation rules are too strict. RFC 2616 allows any ASCII character, with a few exceptions. See http://pretty-rfc.herokuapp.com/RFC2616#basic.rules, or:

message-header = field-name ":" [ field-value ]
field-name     = token

token          = 1*<any CHAR except CTLs or separators>
CHAR           = <any US-ASCII character (octets 0 - 127)>
separators     = "(" | ")" | "<" | ">" | "@"
                     | "," | ";" | ":" | "\" | <">
                     | "/" | "[" | "]" | "?" | "="
                     | "{" | "}" | SP | HT
CTL            = <any US-ASCII control character
                 (octets 0 - 31) and DEL (127)>
SP             = <US-ASCII SP, space (32)>
HT             = <US-ASCII HT, horizontal-tab (9)>

This commit loosens the restrictions in Rack::Lint.

@travisbot

This pull request fails (merged 30a8314e into edc8b92).

@novemberborn

I don't think the fail comes from my commit, if I'm reading Travis output correctly it's

NameError: uninitialized constant Bacon::Context::Timeout
115 test/spec_utils.rb:88:in `block (2 levels) in <top (required)>': Rack::Utils - should not hang on escaping long strings that end in % (http://redmine.ruby-lang.org/issues/5149)
116 test/spec_utils.rb:81:in `block in <top (required)>'
117 test/spec_utils.rb:5:in `<top (required)>'
@raggi
Owner

Yeah, travis seemed to be freaking out for other reasons...

@raggi raggi was assigned
@bensymonds

+1

Any comment from Rack maintainers?

@raggi
Owner

I will take some time to look over the RFC and PR asap.

@raggi
Owner

So RFC 2616 only specifies rules for the field values. In section 4.2 it references RFC 822 3.2 for the field name format.

The field name format specified by 822.3.2 is as follows:

field-name  =  1*<any CHAR, excluding CTLs, SPACE, and ":">

This is much more open than even the new rule set.

@raggi
Owner

we must not allow '.' as it's used for rack internal names

@bensymonds

Hey, thanks for responding.

Regarding the RFC 822 thing - RFC 2616 section 4.2 seems to reference section 3.1, not 3.2, of RFC 822. Section 3.1 just has some information about how the headers are separated from the body - nothing specific about the header format. Given RFC 2616 section 4.2 does specify header format, I think this is the correct spec to follow.

Regarding the missing '#' - I've added it.

Regarding '.' - why does the fact it's used for rack internal names mean it can't be allowed?

@raggi
Owner

Externally created dotted headers could cause unpredictable (read: exploitable) behavior in applications

@raggi
Owner

Sorry the . comment is totally invalid for this context. It only applies to incoming headers in env.

I think we've removed more than we should from the removed lines, specifically the items around newlines. Newlines are certainly not allowed in field names in HTTP.

We also should not allow :, and I'd like that to remain explicit as well.

I would say we're safe to remove the - and _ termination rules, and the last rule, and replace that with a single validation that is essentially: 1*<any CHAR, excluding CTLs, SPACE, and ":">

@bensymonds

Ok added a new commit with following changes (once everyone is happy I can squash into a single, tidy commit):

  • re-added the check for : and \n (including the bit in spec_lint.rb)
  • updated the SPEC (this was missing from the original commit I think)

How does it look now?

@raggi
Owner

Nearly there, but I'd really like to get it close to the RFC and still keep Lint descriptive. I'll look into this for the next release.

@novemberborn novemberborn Validate header names according to RFC 2616
Header name validation rules are too strict. RFC 2616 allows any ASCII character save from 0-32, 127, "(", ")", "<", ">", "@", ",", ";", ":", "\", <">, "/", "[", "]", "?", "=" and "{", "}".
facb3a6
@bensymonds

Cool ok well will leave it with you guys then. Just shout if you need anything from this end. Cheers.

@raggi
Owner

Can you check this against RFC 7320, please.

@spastorino spastorino merged commit 34b981e into rack:master
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Feb 8, 2013
  1. @novemberborn

    Validate header names according to RFC 2616

    novemberborn authored Ben Symonds committed
    Header name validation rules are too strict. RFC 2616 allows any ASCII character save from 0-32, 127, "(", ")", "<", ">", "@", ",", ";", ":", "\", <">, "/", "[", "]", "?", "=" and "{", "}".
This page is out of date. Refresh to see the latest.
Showing with 5 additions and 17 deletions.
  1. +1 −3 SPEC
  2. +2 −5 lib/rack/lint.rb
  3. +2 −9 test/spec_lint.rb
View
4 SPEC
@@ -202,9 +202,7 @@ server, and must not be sent back to the client.
The header keys must be Strings.
The header must not contain a +Status+ key,
contain keys with <tt>:</tt> or newlines in their name,
-contain keys names that end in <tt>-</tt> or <tt>_</tt>,
-but only contain keys that consist of
-letters, digits, <tt>_</tt> or <tt>-</tt> and start with a letter.
+but only contain keys that match the token rule according to RFC 2616.
The values of the header must be Strings,
consisting of lines (for multiple header values, e.g. multiple
<tt>Set-Cookie</tt> values) seperated by "\n".
View
7 lib/rack/lint.rb
@@ -574,11 +574,8 @@ def check_headers(header)
assert("header must not contain Status") { key.downcase != "status" }
## contain keys with <tt>:</tt> or newlines in their name,
assert("header names must not contain : or \\n") { key !~ /[:\n]/ }
- ## contain keys names that end in <tt>-</tt> or <tt>_</tt>,
- assert("header names must not end in - or _") { key !~ /[-_]\z/ }
- ## but only contain keys that consist of
- ## letters, digits, <tt>_</tt> or <tt>-</tt> and start with a letter.
- assert("invalid header name: #{key}") { key =~ /\A[a-zA-Z][a-zA-Z0-9_-]*\z/ }
+ ## The header must match the token rule according to RFC 2616
+ assert("invalid header name: #{key}") { key =~ /\A[\!#\$%&'\*\+-.0-9A-Z\^_`a-z\|~]+\z/ }
## The values of the header must be Strings,
assert("a header value must be a String, but the value of " +
View
11 test/spec_lint.rb
@@ -191,17 +191,10 @@ def result.name
lambda {
Rack::Lint.new(lambda { |env|
- [200, {"Content-" => "text/plain"}, []]
+ [200, {"([{<quark>}])?" => "text/plain"}, []]
}).call(env({}))
}.should.raise(Rack::Lint::LintError).
- message.should.match(/must not end/)
-
- lambda {
- Rack::Lint.new(lambda { |env|
- [200, {"..%%quark%%.." => "text/plain"}, []]
- }).call(env({}))
- }.should.raise(Rack::Lint::LintError).
- message.should.equal("invalid header name: ..%%quark%%..")
+ message.should.equal("invalid header name: ([{<quark>}])?")
lambda {
Rack::Lint.new(lambda { |env|
Something went wrong with that request. Please try again.