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

Accept: * is treated as unrecognized content type. #1009

Closed
NatalieWolfe opened this issue Feb 11, 2016 · 10 comments
Closed

Accept: * is treated as unrecognized content type. #1009

NatalieWolfe opened this issue Feb 11, 2016 · 10 comments

Comments

@NatalieWolfe
Copy link
Contributor

This is the non-critical part of #1008. Basically, the * content type is treated as not matching any formatters and thus an empty response is sent. I would expect it to be treated the same as */* in that any formatter should be accepted.

accept.js

var restify = require('restify');
var server = restify.createServer();

server.get('/', function(req, res, next){
    res.send({message: 'foo'});
    next();
});

server.listen(8080);

test

$ time curl http://localhost:8080/ -H 'Accept: *'

real    0m0.013s
user    0m0.003s
sys     0m0.003s
$ time curl http://localhost:8080/ -H 'Accept: */*'
{"message":"foo"}
real    0m0.012s
user    0m0.003s
sys     0m0.003s
@lukealbao
Copy link
Contributor

I don't think "" is a valid media-range. According to the spec, you need a type/subtype tuple. Maybe you're confusing it with accept-charset or accept-encoding, which do specify "" as wildcard values?

I could be wrong.

In any case, this logic is handled by the negotiator module, so if you want to pursue this, you'll probably have to do it there.

@NatalieWolfe
Copy link
Contributor Author

You are correct that according to the spec this is an invalid value for this header. However, I encountered this value in the wild, not out of my own folly. Our payment processor sends notifications to client servers when something happens (payment fails, subscription renews, whatever) and they have Accept: * as one of the headers in their request. In combination with bug #937 this was causing our server to puke. While simply saying "it isn't in the spec" is true, it is not useful.

I will take this to negotiator and see what they say. If they accept the bug (pun intended) then I'll close this one.

@lukealbao
Copy link
Contributor

Sorry – I didn't mean to imply any folly. I just mean that with the fix to 937 you can treat the "*" like you treat any unknown accept header. You can opt to "screw you, client!", already, without having to define new semantics for the negotiator.

@NatalieWolfe
Copy link
Contributor Author

And I didn't mean to imply that you did, just wanted to make it explicitly clear that the value of the header was out of my control.

Your suggestion of handling at the end application means every single user of this package must implement their own special logic for * or face a lurking, subtle bug in their server. Since there is no ambiguity around the meaning of that header value, it makes far more sense for some higher package, be it restify or negotiator, to handle it once and for all, fixing it for every person who uses their package.

Between the two, I think it makes more sense for restify to do it as it already performs these kinds of user-friendly actions, such as bundling 3rd-party modules, having several default formatters, and automatically formatting errors.

@DonutEspresso
Copy link
Member

I'm not convinced this is a common enough use case to be fixed in restify core, but, it should be relatively easy to fix the accept type as needed. Something like:

server.pre(function fixAccept(req, res, next) {
    if (req.headers['accept'] === '*') {
        req.headers['accept'] = '*/*';
    }
    return next();
});

Would that work for your use case?

@NatalieWolfe
Copy link
Contributor Author

That does, but it is exactly what @lukealbao recommended.

While it is not an every-day occurrence, it can still happen in a production setting and when it does it is not entirely obvious what the problem might be. I think adopting a laissez faire approach here is an unfortunate idea when it could be squashed completely with minimal effort within restify itself.

@retrohacker
Copy link
Member

retrohacker commented Apr 29, 2017

Heya @NatalieWolfe, thanks for the discussion on this so far!

I'm leaning towards this becoming a section in a doc somewhere (i.e. ### Handling Accept: *) as opposed to a code change since restify tries to stick as closely to spec as possible. It sounds like the payment processor is deviating from spec here and, while frustrating, the behaviour of restify is consistent with the specification in not handling their wildcard. As opposed to opting everyone into handling this case, manually dealing with the few cases where it is a problem seems to be ideal.

@retrohacker
Copy link
Member

retrohacker commented May 18, 2017

Labeling this as a feature request for documentation 😄 happy to revisit the decision to not deviate from the specification.

@stale
Copy link

stale bot commented Apr 10, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the Stale label Apr 10, 2018
@stale
Copy link

stale bot commented Apr 24, 2018

This issue has been automatically closed as stale because it has not had recent activity.

@stale stale bot closed this as completed Apr 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants