CyberDeck / devise-fido-u2f

A devise module to authenticate additionally with a FIDO U2F hardware token, e.g. a Yubico USB security key.
MIT License
35 stars 4 forks source link

Implements the u2f helper method in the gem #6

Closed zedtux closed 6 years ago

zedtux commented 6 years ago

@CyberDeck I followed the instructions from the README.md file but it didn't worked. For some reasons, the u2f helper, inserted in my ApplicationHelper module, wasn't found by the gem and was failing.

I started to investigate why the generate was adding this method, but I wasn't able to figure it out. I was thinking of the request.base_url which would have been wrong from the gem, so it was added within my application, but the request object stays as it is, no matter where is the controller, so I just added the method in the controllers and all is working fine.

Can you please tell me if I'm wrong somewhere? Otherwise, if you agree with this, I can create a concern in order to move the method out of the controllers and avoid duplications so that the code is cleaner.

Let me know what's best in your opinion.

coveralls commented 6 years ago

Coverage Status

Coverage remained the same at 100.0% when pulling 7823a46689831118d59afab49b24b3a731d18204 on Pharmony:tasks/moves-the-u2f-helper-to-the-gem into b05717d71253c82dd78856d7ca683b4acdd3a7c5 on CyberDeck:master.

coveralls commented 6 years ago

Coverage Status

Coverage remained the same at 100.0% when pulling 82a83187caa5d031678a82494f93d003a67105b3 on Pharmony:tasks/moves-the-u2f-helper-to-the-gem into b05717d71253c82dd78856d7ca683b4acdd3a7c5 on CyberDeck:master.

zedtux commented 6 years ago

I'm not sure about the Code Climate error raised ...

It is complaining about the file app/views/devise/fido_usf_registrations/new.html.erb at line 13 that the JSON hash isn't escaped, but actually it is the same thing for the line 12 and it didn't complained...

I'm not sure if this is a true negative or not.

The only thing I could think of is to use json_escape ... but I don't like what is written in the WARNING section at the end of the documentation ...

zedtux commented 6 years ago

I have installed brakeman 4.1.1 (like code climate seems to use) and ran locally and here is the output:

Loading scanner...
Processing application in /gem
Processing gems...
[Notice] Detected Rails 5 application
Processing configuration...
[Notice] Escaping HTML by default
Parsing files...
Processing initializers...
Processing libs...
Processing routes...
[Notice] No route information found
Processing templates...
Processing data flow in templates...
Processing models...
Processing controllers...
Processing data flow in controllers...
Indexing call sites...
Running checks in parallel...
 - CheckBasicAuth
 - CheckBasicAuthTimingAttack
 - CheckCrossSiteScripting
 - CheckContentTag
 - CheckCreateWith
 - CheckDefaultRoutes
 - CheckDeserialize
 - CheckDetailedExceptions
 - CheckDigestDoS
 - CheckDynamicFinders
 - CheckEscapeFunction
 - CheckEvaluation
 - CheckExecute
 - CheckFileAccess
 - CheckFileDisclosure
 - CheckFilterSkipping
 - CheckForgerySetting
 - CheckHeaderDoS
 - CheckI18nXSS
 - CheckJRubyXML
 - CheckJSONEncoding
 - CheckJSONParsing
 - CheckLinkTo
 - CheckLinkToHref
 - CheckMailTo
 - CheckMassAssignment
 - CheckMimeTypeDoS
 - CheckModelAttrAccessible
 - CheckModelAttributes
 - CheckModelSerialize
 - CheckNestedAttributes
 - CheckNestedAttributesBypass
 - CheckNumberToCurrency
 - CheckPermitAttributes
 - CheckQuoteTableName
 - CheckRedirect
 - CheckRegexDoS
 - CheckRender
 - CheckRenderDoS
 - CheckRenderInline
 - CheckResponseSplitting
 - CheckRouteDoS
 - CheckSafeBufferManipulation
 - CheckSanitizeMethods
 - CheckSelectTag
 - CheckSelectVulnerability
 - CheckSend
 - CheckSendFile
 - CheckSessionManipulation
 - CheckSessionSettings
 - CheckSimpleFormat
 - CheckSingleQuotes
 - CheckSkipBeforeFilter
 - CheckSQL
 - CheckSQLCVEs
 - CheckSSLVerify
 - CheckStripTags
 - CheckSymbolDoSCVE
 - CheckTranslateBug
 - CheckUnsafeReflection
 - CheckValidationRegex
 - CheckWithoutProtection
 - CheckXMLDoS
 - CheckYAMLParsing
Checks finished, collecting results...
Generating report...

== Brakeman Report ==

Application Path: /gem
Rails Version: 5.1.4
Brakeman Version: 4.1.1
Scan Date: 2017-12-29 08:10:23 +0000
Duration: 0.293468301 seconds
Checks Run: BasicAuth, BasicAuthTimingAttack, ContentTag, CreateWith, CrossSiteScripting, DefaultRoutes, Deserialize, DetailedExceptions, DigestDoS, DynamicFinders, EscapeFunction, Evaluation, Execute, FileAccess, FileDisclosure, FilterSkipping, ForgerySetting, HeaderDoS, I18nXSS, JRubyXML, JSONEncoding, JSONParsing, LinkTo, LinkToHref, MailTo, MassAssignment, MimeTypeDoS, ModelAttrAccessible, ModelAttributes, ModelSerialize, NestedAttributes, NestedAttributesBypass, NumberToCurrency, PermitAttributes, QuoteTableName, Redirect, RegexDoS, Render, RenderDoS, RenderInline, ResponseSplitting, RouteDoS, SQL, SQLCVEs, SSLVerify, SafeBufferManipulation, SanitizeMethods, SelectTag, SelectVulnerability, Send, SendFile, SessionManipulation, SessionSettings, SimpleFormat, SingleQuotes, SkipBeforeFilter, StripTags, SymbolDoSCVE, TranslateBug, UnsafeReflection, ValidationRegex, WithoutProtection, XMLDoS, YAMLParsing

== Overview ==

Controllers: 2
Models: 1
Templates: 6
Errors: 0
Security Warnings: 0

== Warning Types ==

No warnings found

So I don't know what's wrong on Code Climate side ...

CyberDeck commented 6 years ago

I am fine with your PR regarding the u2f helper function.

My original intention of moving it into the application helper and not into the gem itself was to support so called Multi-facet App IDs of U2F, cf. App ID explanation. However, by moving it into the gem only single-facet apps are supported, which is probably fine for most Rails apps...

Regarding the codeclimate issue: I have no clue while this line fails and not the other one.

zedtux commented 6 years ago

My original intention of moving it into the application helper and not into the gem itself was to support so called Multi-facet App IDs of U2F, cf. App ID explanation.

I see, that better explain it.

... which is probably fine for most Rails apps...

I agree, and when someone will come with that request, we can find a solution :)