mgomes / api_auth

HMAC authentication for Rails and HTTP Clients
MIT License
480 stars 147 forks source link

Don't use #blank? #114

Closed packrat386 closed 8 years ago

packrat386 commented 8 years ago

The #blank? method is actually defined in activesupport which isn't a runtime dependency of the gem. This is only ever going to be a string or nil, so this check works the same way.

awendt commented 8 years ago

@packrat386 If it's only ever going to be string or nil, why not use .nil? ?

packrat386 commented 8 years ago

I haven't tested extensively, but I wanted to account for the fact that the string returned by that match group could be empty, which would also justify defaulting to SHA1.

packrat386 commented 8 years ago

@awendt after looking into the regex it seems that checking nil? is sufficient. Also there seems to be a bug in the regex that checks the digest where it will accept SHA with it's number repeated any number of times.

irb(main):001:0> AUTH_HEADER_PATTERN = /APIAuth(?:-HMAC-(MD[245]|SHA(?:1|224|256|384|512)*))? ([^:]+):(.+)$/
=> /APIAuth(?:-HMAC-(MD[245]|SHA(?:1|224|256|384|512)*))? ([^:]+):(.+)$/
irb(main):002:0> AUTH_HEADER_PATTERN.match("APIAuth-HMAC-SHA256256256256 some:data")
=> #<MatchData "APIAuth-HMAC-SHA256256256256 some:data" 1:"SHA256256256256" 2:"some" 3:"data">

Is this behaviour intended, and if not would you prefer I fix that here, or in a separate PR?

kjg commented 8 years ago

@packrat386 That behaviour in the regex is not intended, but as far as I can tell also shouldn't pose any major issues. I'd certainly accept a PR that alters that behaviour.

kjg commented 8 years ago

@packrat386 thanks for finding this .blank? and changing it to .nil?

fwininger commented 8 years ago

:+1:

daneb commented 8 years ago

When will be a gem release that includes this fix?