Return 401 when the auth header doesn't actually contain a token

johnbrett / hapi-auth-bearer-token

Simple Bearer authentication scheme plugin for hapi, accepts token by Header, Cookie or Query parameter.

MIT License

218 stars 46 forks source link

Return 401 when the auth header doesn't actually contain a token #62

Closed fermaem closed 8 years ago

johnbrett commented 8 years ago

Hi @fermaem, thanks for the contribution! Sincere apologies for delay in responding! What happens currently here, a 400 bad request if headers don't supply the token?

fermaem commented 8 years ago

What happens currently here, a 400 bad request if headers don't supply the token?

@johnbrett The validateFunc func is either called with an empty token (when auth contains Bearer with trailing spaces) or a null one (when auth only contains Bearer). Basically, the behavior is undefined and depends on the callback implementation.

I've opted for a 401 rather than a 400 in order to comply with https://tools.ietf.org/html/rfc7235#section-3.1 regarding the usage of a token against a protected resource.

fermaem commented 8 years ago

@johnbrett Do you need any additional information regarding this PR?

johnbrett commented 8 years ago

No and sincere apology!! swamped with work. I want to check this behaviour with other auth schemes, which I haven't gotten around to just yet. Will try do this tomorrow!!

fermaem commented 8 years ago

Will try do this tomorrow!!

:heart_eyes:

johnbrett commented 8 years ago

Hi @fermaem, as promised, looked into this today :) So I found something strange - when I add your test, without the code change, the test still passes.

I also noticed that the code change doesn't get covered by your test, so we don't have 100% code coverage.

Can you:

try submit a failing test (if you can't, maybe you were getting a 400 from somewhere else?)
if you submit a failing test and a fix, can you make sure the code coverage afterwards is 100%? I can help you get this if you need, but can't merge a PR without it :)

Also, I'm going to update the npm test command so that it fails if coverage is less than 100, to match my other modules and those of the hapi ecosystem.

If you've any questions let me know, I'll help any way I can!

johnbrett commented 8 years ago

Closing due to inactivity. Feel free to reopen if you have further questions.

fermaem commented 8 years ago

@johnbrett I'm not done with this PR yet. Just currently missing some time to work on it.

Could you please reopen it (as I'm lacking the rights to do so)?

johnbrett commented 8 years ago

Apologies @fermaem, looking forward to seeing the work on it! Let me know if there's anything I can help with.

johnbrett commented 8 years ago

Hey @fermaem, any updates on this? Can I leave it closed until you want to continue a PR, I will reopen if comment or make any commits, otherwise I can't get "inbox zero" of my github dashboard :)

fermaem commented 8 years ago

Hey @fermaem, any updates on this? Can I leave it closed until you want to continue a PR, I will reopen if comment or make any commits, otherwise I can't get "inbox zero" of my github dashboard :)

So sorry for the delay. Let's close it until I can get out of $DAYJOB swamp.

johnbrett commented 8 years ago

No need for apologies - no open source project I run has time constraints on what people do in their spare time :)