Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

req.accepts: */* wildcard fix #1020

Merged
merged 1 commit into from

5 participants

@papandreou

Look for */* anywhere in the Accept header instead of only supporting it when it's the exact value of the header.

IE8 sends:

Accept: image/gif, image/jpeg, image/pjpeg, image/pjpeg, application/x-shockwave-flash, application/x-ms-application, application/x-ms-xbap, application/vnd.ms-xpsdocument, application/xaml+xml, */*

for which req.accepts('html') returned false.

This bug doesn't exist on the master branch.

@papandreou papandreou req.accepts: */* wildcard fix.
Look for '*/*' anywhere in the Accept header instead of only supporting it when it's the exact value of the header.

IE8 sends:

    Accept: image/gif, image/jpeg, image/pjpeg, image/pjpeg, application/x-shockwave-flash, application/x-ms-application, application/x-ms-xbap, application/vnd.ms-xpsdocument, application/xaml+xml, */*

for which req.accepts('html') returned false.
c92c10d
@tj tj merged commit 1515d53 into strongloop:2.x
@yagudaev

In version 2.5.9 (latest tagged version you get with npm install) req.accept('json') will always return true due to this fix. I saw some changes on the master, but I haven't tested it yet. I downgraded to 2.5.4 for now.

We are using req.accept('json') in order to determine if it is an ajax request or a user who actually loads the page (serving html). There should be a way to determine if a mime type was explicitly sent by the client or implicitly sent. One approach would be to add a flag, 'implicit' set to false by default to preserve behavior from 2.5.4 and below but also allow to check for the scenario described above.

@yagudaev yagudaev commented on the diff
lib/request.js
@@ -135,8 +135,8 @@ req.accepts = function(type){
// normalize extensions ".json" -> "json"
if (type && '.' == type[0]) type = type.substr(1);
- // when Accept does not exist, or is '*/*' return true
- if (!accept || '*/*' == accept) {
+ // when Accept does not exist, or contains '*/*' return true
+ if (!accept || ~accept.indexOf('*/*')) {

will cause it to always return true since most browsers send this by default with every request.

@tj
tj added a note

that's as per RFC. If the agent is giving you / then it's fine with anything

@tj
tj added a note

in 3x you can also now do req.accepts(['json', 'html']) which returns the acceptable type (if any) which is probably more what you're looking for, this variant will not return a random value even with /, that's up to the user to default

@tj
tj added a note

not necessarily explicitly, for example if Accept contained "text/" it'll still match, but if there is not a match (excluding */) it'll return undefined. At least that's how it is right now, but I dont think that's necessarily "correct". We're probably better off to return the first value in that case

@tj
tj added a note

oh no sorry, it does return the first for "/"

I would expect to be able to use accept to do that. So we have this in our code:

if (req.accetps("json")) {
     res.contentType('json');
     // ... some code...
     res.send(200, jsonObj);
} else {
      res.contentType('html');
}

But since it always returns true it always goes to the if branch, which is not what I would expect.

@tj
tj added a note

You can do that, but "/" means they'll accept anything, if they wouldn't accept anything they wouldn't send that. In 3.x there's also:

res.format({
  json: function(){
    res.send(obj);
  },

  html: function(){
    res.send(html)
  }
})

which will respond with 406 not acceptable if it really doesn't match

@defunctzombie Collaborator

I ran into a similar issue with 2.5.9 and the */* "fix". The problem is that most browsers send */* at the end of the accepts header as a catch all. What really begins to matter is the order of the mime types in the accept header. Personally I think it is wrong to handle the / case in the framework. It is much easier for a user to decide what their default catch all case should be rather than all accepts methods returning true when */* is present.

I view the req.accepts(<some type here>) code as a way to see if that particular mime type is explicitly accepted and not implicitly. Personally, I would be in favor of reverting this change. Handling the */* case is better done at the app level.

@yagudaev
yagudaev added a note

yes I agree, I find it confusing. I am not entirely sure what the use case of the req.accepts() would be if it always returns true (or at least for 99% of clients). I understand that from the framework point it matters, but APIs are designed for user code not system code.

There can be an internal interface to check implicit accept, or an optional flag in the interface called implicit set to false by default.

@tj
tj added a note

Express is also not at fault though when it comes to clients sending */*, if they say they accept anything, then they better accept anything. if you want to decide the default just make it the first value

@defunctzombie Collaborator

I agree that express is not at fault. I think this is more of an issue regarding api usability/flexibility versus blame or a particular absolute "correct" way to do it. The / is easy to do without any framework support. My argument came from the fact that browsers will add the / as a fallback but often have the real mime types first.

If req.accepts('html') return true for application/json; */* then it makes it harder to write code that will handle the various cases. However, if it returns false unless the html mime type is present, then I find the resulting code clearer

if (req.accepts('html') {
    // case when text/html is present
}
else if (req.accepts('json')) {
    // case when application/json is present
}
else {
    // default case for */*
}

The above doesn't work when / causes everything to return true.

@tj
tj added a note

req.accepts() with a single string should arguably be removed, since it can't work with Accept's natural or qvalue ordering, which was why res.format() was introduced, but req.accepts(array) is what it uses internally

@bunkat
bunkat added a note

Please revert this change. req.accepts was functional the way it was before and was an easy way to determine if a content type was explicitly passed. Now it's just useless as it always returns true regardless of whether I pass in a single value or an array.

@tj
tj added a note

let's open a new issue to discuss this it's way too cluttered in here

@defunctzombie Collaborator

wtf?

@tj
tj added a note

hahahah what the

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@mstuart mstuart referenced this pull request in krakenjs/generator-kraken
Closed

Fix controller generator so it's compatible with IE8/9 out-of-the-box. #61

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Feb 22, 2012
  1. @papandreou

    req.accepts: */* wildcard fix.

    papandreou authored
    Look for '*/*' anywhere in the Accept header instead of only supporting it when it's the exact value of the header.
    
    IE8 sends:
    
        Accept: image/gif, image/jpeg, image/pjpeg, image/pjpeg, application/x-shockwave-flash, application/x-ms-application, application/x-ms-xbap, application/vnd.ms-xpsdocument, application/xaml+xml, */*
    
    for which req.accepts('html') returned false.
This page is out of date. Refresh to see the latest.
Showing with 2 additions and 2 deletions.
  1. +2 −2 lib/request.js
View
4 lib/request.js
@@ -135,8 +135,8 @@ req.accepts = function(type){
// normalize extensions ".json" -> "json"
if (type && '.' == type[0]) type = type.substr(1);
- // when Accept does not exist, or is '*/*' return true
- if (!accept || '*/*' == accept) {
+ // when Accept does not exist, or contains '*/*' return true
+ if (!accept || ~accept.indexOf('*/*')) {

will cause it to always return true since most browsers send this by default with every request.

@tj
tj added a note

that's as per RFC. If the agent is giving you / then it's fine with anything

@tj
tj added a note

in 3x you can also now do req.accepts(['json', 'html']) which returns the acceptable type (if any) which is probably more what you're looking for, this variant will not return a random value even with /, that's up to the user to default

@tj
tj added a note

not necessarily explicitly, for example if Accept contained "text/" it'll still match, but if there is not a match (excluding */) it'll return undefined. At least that's how it is right now, but I dont think that's necessarily "correct". We're probably better off to return the first value in that case

@tj
tj added a note

oh no sorry, it does return the first for "/"

I would expect to be able to use accept to do that. So we have this in our code:

if (req.accetps("json")) {
     res.contentType('json');
     // ... some code...
     res.send(200, jsonObj);
} else {
      res.contentType('html');
}

But since it always returns true it always goes to the if branch, which is not what I would expect.

@tj
tj added a note

You can do that, but "/" means they'll accept anything, if they wouldn't accept anything they wouldn't send that. In 3.x there's also:

res.format({
  json: function(){
    res.send(obj);
  },

  html: function(){
    res.send(html)
  }
})

which will respond with 406 not acceptable if it really doesn't match

@defunctzombie Collaborator

I ran into a similar issue with 2.5.9 and the */* "fix". The problem is that most browsers send */* at the end of the accepts header as a catch all. What really begins to matter is the order of the mime types in the accept header. Personally I think it is wrong to handle the / case in the framework. It is much easier for a user to decide what their default catch all case should be rather than all accepts methods returning true when */* is present.

I view the req.accepts(<some type here>) code as a way to see if that particular mime type is explicitly accepted and not implicitly. Personally, I would be in favor of reverting this change. Handling the */* case is better done at the app level.

@yagudaev
yagudaev added a note

yes I agree, I find it confusing. I am not entirely sure what the use case of the req.accepts() would be if it always returns true (or at least for 99% of clients). I understand that from the framework point it matters, but APIs are designed for user code not system code.

There can be an internal interface to check implicit accept, or an optional flag in the interface called implicit set to false by default.

@tj
tj added a note

Express is also not at fault though when it comes to clients sending */*, if they say they accept anything, then they better accept anything. if you want to decide the default just make it the first value

@defunctzombie Collaborator

I agree that express is not at fault. I think this is more of an issue regarding api usability/flexibility versus blame or a particular absolute "correct" way to do it. The / is easy to do without any framework support. My argument came from the fact that browsers will add the / as a fallback but often have the real mime types first.

If req.accepts('html') return true for application/json; */* then it makes it harder to write code that will handle the various cases. However, if it returns false unless the html mime type is present, then I find the resulting code clearer

if (req.accepts('html') {
    // case when text/html is present
}
else if (req.accepts('json')) {
    // case when application/json is present
}
else {
    // default case for */*
}

The above doesn't work when / causes everything to return true.

@tj
tj added a note

req.accepts() with a single string should arguably be removed, since it can't work with Accept's natural or qvalue ordering, which was why res.format() was introduced, but req.accepts(array) is what it uses internally

@bunkat
bunkat added a note

Please revert this change. req.accepts was functional the way it was before and was an easy way to determine if a content type was explicitly passed. Now it's just useless as it always returns true regardless of whether I pass in a single value or an array.

@tj
tj added a note

let's open a new issue to discuss this it's way too cluttered in here

@defunctzombie Collaborator

wtf?

@tj
tj added a note

hahahah what the

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
return true;
} else if (type) {
// allow "html" vs "text/html" etc
Something went wrong with that request. Please try again.