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

Accept ActiveStorage::Filename#sanitized and to_i as safe #1883

Open
bb opened this issue Nov 7, 2024 · 2 comments
Open

Accept ActiveStorage::Filename#sanitized and to_i as safe #1883

bb opened this issue Nov 7, 2024 · 2 comments

Comments

@bb
Copy link

bb commented Nov 7, 2024

Background

This is a follow-up to #337.

Brakeman version: 6.2.2
Rails version: 7.1.5
Ruby version: 3.3.4

Link to Rails application code: ?

False Positive

Full warning from Brakeman:

Confidence: Weak
Category: File Access
Check: SendFile
Message: Parameter value used in file name
Code: send_file("public/ogimages/#{ActiveStorage::Filename.new("post-#{MeiliSearchRails.client.index("posts").document(params[:id].to_i)["id"].to_i}").sanitized}.png", :type => "image/png", :disposition => "inline")
File: app/controllers/posts_controller.rb
Line: 64

Relevant code:

    file_local = ActiveStorage::Filename.new("alert-#{post_hash["id"].to_i}").sanitized
    file_name = "public/ogimages/#{file_local}.png"
    send_file file_name, type: "image/png", disposition: "inline"

Why might this be a false positive?

  • ActiveStorage::Filename.new(foo).sanitized is an official Rails way to sanitize a file name
  • to_i makes sure it's only numeric, so even without ActiveStorage sanitizing, a path traversal is not possible using only an integer number which can neither contain . nor / making a directory traversal impossible
@bb
Copy link
Author

bb commented Nov 7, 2024

I just found about #1375 so I guess this might be because I'm only calling sanitized on the local part and adding a static path outside of the sanitization?!

@presidentbeef
Copy link
Owner

Yes, see my comment on that PR 😄

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

2 participants