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

Possible unprotected redirect for URL #1687

Open
QuentinBonnafet opened this issue Feb 23, 2022 · 2 comments
Open

Possible unprotected redirect for URL #1687

QuentinBonnafet opened this issue Feb 23, 2022 · 2 comments

Comments

@QuentinBonnafet
Copy link

Brakeman version: 5.2.1
Rails version: 6.1.4.6
Ruby version: 3.1.0

I am having this result for an url that allows user to download a document.

Confidence: High
Category: Redirect
Check: Redirect
Message: Possible unprotected redirect
Code: redirect_to(Classified.find(params[:id]).documents.find(params[:document_id]).asset.expiring_url(10))
File: ******_controller.rb

An issue has already been opened for this.
Should we still avoid the warning or is there any chance for the warning to be changed as here?

@presidentbeef
Copy link
Owner

Since expiring_url is from Paperclip (I assume) and isn't likely to be confused with an attacker-controllable value, I think it's fine to have Brakeman ignore expiring_url.

@benjaminysmall
Copy link

Similar situation here, if another data point is useful.

Brakeman 5.1.1
Rails 6.1.5.1
Ruby 3.0.2

Confidence: High
Category: Redirect
Check: Redirect
Message: Possible unprotected redirect
Code: redirect_to(Template.find_by(:id => params[:template_id]).file.url(:expires_in => Template::DOWNLOAD_LINK_EXPIRATION.in_seconds))
File: app/controllers/templates_controller.rb

Where .file.url is Shrine.

Easy enough to add to the ignore file, but if it's possible to detect this as safe (perhaps because the parameter in question is only being used as the input to an ActiveRecord query and not a bare part of the redirect? honestly not sure if that matters or would be detectable) that'd be a welcome change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants