jshttp / negotiator

An HTTP content negotiator for Node.js
MIT License
309 stars 33 forks source link

Fix behaviors when headers are not present #19

Closed dougwilson closed 10 years ago

dougwilson commented 10 years ago

Hi! I was using your module and I noticed we kept responding with 406 to a certain client. It turns out the client had a privacy proxy running and was not sending any Accept header with the request. I checked in RFC 2616 and apparently they have information on what should be done if the header is missing. I updated the different modules with the RFC 2616 recommendations and added tests for the missing header cases.

Since new Negotiator(req) will pass req.headers[name] down to the modules, a missing header would pass down undefined, so I added the support for undefined to all the modules (it makes sense anyway, because the acceptable media type is not defined :))

RFC 2616 14.1

If no Accept header field is present, then it is assumed that the client accepts all media types.

RFC 2616 14.2

If no Accept-Charset header is present, the default is that any character set is acceptable.

RFC 2616 14.3

If no Accept-Encoding field is present in a request, the server MAY assume that the client will accept any content coding. In this case, if "identity" is one of the available content-codings, then the server SHOULD use the "identity" content-coding, unless it has additional information that a different content-coding is meaningful to the client.

RFC 2616 14.4

If no Accept-Language header is present in the request, the server SHOULD assume that all languages are equally acceptable.

Right now using this module as new Negotiator(req) results in a violation of section 14.1 and 14.2 of RFC 2616. Sections 14.3 and 14.4 are nice to have (and 14.3 already occurs, I just added a test for it).

federomero commented 10 years ago

Great @dougwilson, thank you for taking the time to write a good description. The only doubt I have is how should we treat an empty string in the Accept header? Should we also consider that as */* or should we consider that the client doesn't accept any media type? What do you think? I'm leaning towards the second option but I'm not convinced

dougwilson commented 10 years ago

Should we also consider that as / or should we consider that the client doesn't accept any media type? What do you think?

I thought about that too, but those direct RFC quotes seem ambiguous. Let me see if I can find something stated that says if a header that exists but only contains LWS is considered as existing or not.

So far I just went conservative with the interpretation of "If no Accept-Encoding field is present in a request"

dougwilson commented 10 years ago

I found some discussion about it here: http://stackoverflow.com/questions/12130910/how-to-interpret-empty-http-accept-header

Whether you go the 406 route or the / route I think boils down to your preference between correctness and robustness in this case. It seems either is valid as far as the spec is concerned.

dougwilson commented 10 years ago

Yea, I've been looking all over and cannot find any definitive answer. One one hand, you can think that an empty Accept header really makes no sense and treat it as if the Accept header was not there. On the other, perhaps the client is saying nothing is acceptable, which really doesn't make any sense unless it just doesn't want a response body back for the request (I don't think there's any other real way for the client to request no body back).

I like to lean towards the specs (which really do seem to say that an empty Accept header means accept nothing), maybe at least for now. Right now the behavior is that '' and undefined mean nothing is acceptable and this will switch one of them instead of both (for now?).

federomero commented 10 years ago

Ok, let's just have undefined mean everything is acceptable and '' mean nothing is acceptable.

dougwilson commented 10 years ago

Ok, let's just have undefined mean everything is acceptable and '' mean nothing is acceptable.

I see you made a new commit that went ahead and changed '' to also mean everything is acceptable :)

federomero commented 10 years ago

Actually, I didn't mean to. I forgot to that '' evaluates to false in js. I'll revert that commit.