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

Bang variants of last, first and similar fail with false positives. #1723

Closed
powersurge360 opened this issue Jul 18, 2022 · 1 comment · Fixed by #1732
Closed

Bang variants of last, first and similar fail with false positives. #1723

powersurge360 opened this issue Jul 18, 2022 · 1 comment · Fixed by #1732

Comments

@powersurge360
Copy link

Background

Brakeman version: 5.2.3
Rails version: 7.0.3
Ruby version: 3.1.2

Link to Rails application code: link

False Positive

Full warning from Brakeman:

Loading scanner...
Processing application in /Users/powersurge360/projects/trivia_app
Processing gems...                    
[Notice] Detected Rails 7 application
Processing configuration...           
[Notice] Escaping HTML by default
Parsing files...                      
Detecting file types...               
Processing initializers...            
Processing libs...                    
Processing routes...                  
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
 - CheckCookieSerialization
 - CheckCreateWith
 - CheckCSRFTokenForgeryCVE
 - CheckDefaultRoutes
 - CheckDeserialize
 - CheckDetailedExceptions
 - CheckDigestDoS
 - CheckDynamicFinders
 - CheckEOLRails
 - CheckEOLRuby
 - CheckEscapeFunction
 - CheckEvaluation
 - CheckExecute
 - CheckFileAccess
 - CheckFileDisclosure
 - CheckFilterSkipping
 - CheckForgerySetting
 - CheckHeaderDoS
 - CheckI18nXSS
 - CheckJRubyXML
 - CheckJSONEncoding
 - CheckJSONEntityEscape
 - CheckJSONParsing
 - CheckLinkTo
 - CheckLinkToHref
 - CheckMailTo
 - CheckMassAssignment
 - CheckMimeTypeDoS
 - CheckModelAttrAccessible
 - CheckModelAttributes
 - CheckModelSerialize
 - CheckNestedAttributes
 - CheckNestedAttributesBypass
 - CheckNumberToCurrency
 - CheckPageCachingCVE
 - CheckPermitAttributes
 - CheckQuoteTableName
 - CheckRedirect
 - CheckRegexDoS
 - CheckRender
 - CheckRenderDoS
 - CheckRenderInline
 - CheckResponseSplitting
 - CheckRouteDoS
 - CheckSafeBufferManipulation
 - CheckSanitizeMethods
 - CheckSelectTag
 - CheckSelectVulnerability
 - CheckSend
 - CheckSendFile
 - CheckSessionManipulation
 - CheckSessionSettings
 - CheckSimpleFormat
 - CheckSingleQuotes
 - CheckSkipBeforeFilter
 - CheckSprocketsPathTraversal
 - CheckSQL
 - CheckSQLCVEs
 - CheckSSLVerify
 - CheckStripTags
 - CheckSymbolDoSCVE
 - CheckTemplateInjection
 - CheckTranslateBug
 - CheckUnsafeReflection
 - CheckUnsafeReflectionMethods
 - CheckValidationRegex
 - CheckVerbConfusion
 - CheckWithoutProtection
 - CheckXMLDoS
 - CheckYAMLParsing
Checks finished, collecting results...
Generating report...

== Brakeman Report ==

Application Path: /Users/powersurge360/projects/trivia_app
Rails Version: 7.0.3
Brakeman Version: 5.2.3
Scan Date: 2022-07-17 23:47:47 -0400
Duration: 0.267614 seconds
Checks Run: BasicAuth, BasicAuthTimingAttack, CSRFTokenForgeryCVE, ContentTag, CookieSerialization, CreateWith, CrossSiteScripting, DefaultRoutes, Deserialize, DetailedExceptions, DigestDoS, DynamicFinders, EOLRails, EOLRuby, EscapeFunction, Evaluation, Execute, FileAccess, FileDisclosure, FilterSkipping, ForgerySetting, HeaderDoS, I18nXSS, JRubyXML, JSONEncoding, JSONEntityEscape, JSONParsing, LinkTo, LinkToHref, MailTo, MassAssignment, MimeTypeDoS, ModelAttrAccessible, ModelAttributes, ModelSerialize, NestedAttributes, NestedAttributesBypass, NumberToCurrency, PageCachingCVE, PermitAttributes, QuoteTableName, Redirect, RegexDoS, Render, RenderDoS, RenderInline, ResponseSplitting, RouteDoS, SQL, SQLCVEs, SSLVerify, SafeBufferManipulation, SanitizeMethods, SelectTag, SelectVulnerability, Send, SendFile, SessionManipulation, SessionSettings, SimpleFormat, SingleQuotes, SkipBeforeFilter, SprocketsPathTraversal, StripTags, SymbolDoSCVE, TemplateInjection, TranslateBug, UnsafeReflection, UnsafeReflectionMethods, ValidationRegex, VerbConfusion, WithoutProtection, XMLDoS, YAMLParsing

== Overview ==

Controllers: 3
Models: 6
Templates: 14
Errors: 0
Security Warnings: 1

== Warning Types ==

Redirect: 1

== Warnings ==

Confidence: High
Category: Redirect
Check: Redirect
Message: Possible unprotected redirect
Code: redirect_to(Game.last!)
File: app/controllers/games_controller.rb
Line: 37

Relevant code:

@game = Game.latest_round(channel: params[:channel]).last!

Why might this be a false positive?

Mostly equivalent to last, except it throws a record not found exception when no matching record is found. It seems to be a false positive on all of the ! variants of the methods, like second! or first!. last doesn't have the issue, but also requires that I get my application to 404 some other way.

Note that in my code I explicitly pass the path game_path to avoid the brakeman errors, but I would prefer not to.

@powersurge360
Copy link
Author

Also, mentioning here because I was poking around, rails 7 introduces sole and find_sole_by which seem like they should also be whitelisted.

presidentbeef added a commit that referenced this issue Sep 23, 2022
presidentbeef added a commit that referenced this issue Sep 23, 2022
Repository owner locked and limited conversation to collaborators May 9, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant