Skip to content

False Positive: Dynamic render path is not taking into account allow-listed values #1569

Closed
@agrobbin

Description

@agrobbin

Hi Brakeman folks! Love the library, but ran into a situation where Brakeman is reporting a warning that I think might be a false positive.

Background

Brakeman version: 5.0.0
Rails version: 6.0.3.5
Ruby version: 2.7.2

Link to Rails application code: Closed source, but reproduced below

False Positive

Full warning from Brakeman:

Confidence: Medium
Category: Dynamic Render Path
Check: Render
Message: Render path contains parameter value
Code: render(action => "admin/fields/#{(params[:field].presence_in(["foo"]) or raise(ActionController::BadRequest))}", {})
File: app/views/admin/fields/show.html.haml
Line: 1

Relevant code:

# controller
@field = params[:field].presence_in(%w[foo]) || raise(ActionController::BadRequest)

# template
= render "admin2/fields/#{@field}"

Why might this be a false positive?

Since we're allow-listing the fields coming in from query parameters via Object#presence_in, I don't believe this should trigger a warning.

Activity

agrobbin

agrobbin commented on Feb 16, 2021

@agrobbin
Author

Whoops, I screwed up when simplifying the test case, I've updated the relevant code!

presidentbeef

presidentbeef commented on Feb 17, 2021

@presidentbeef
Owner

Agree, and I think this is totally fixable. Thank you for reporting!

agrobbin

agrobbin commented on Feb 18, 2021

@agrobbin
Author

That's great @presidentbeef! I'm afraid I might be in over my head with trying to write a PR for this, but if you have any tips, I can give it a whirl.

fschwahn

fschwahn commented on Nov 29, 2022

@fschwahn

We are also regularly seeing false positives here.

added a commit that references this issue on Dec 3, 2022
cda83f8
added a commit that references this issue on Apr 30, 2024
0c64053
Repository owner locked and limited conversation to collaborators on May 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

      Development

      No branches or pull requests

        Participants

        @agrobbin@fschwahn@presidentbeef

        Issue actions

          False Positive: Dynamic render path is not taking into account allow-listed values · Issue #1569 · presidentbeef/brakeman