21
Root
40d

So I need to "fix" a false-positive security warning (mass-assignment of a foreign key). Do I "fix" it by...

A) Setting it manually and double-saving the object?
B) Rewriting the mass-assignment so the linter doesn't realize what I'm doing?

Both options suck.
But security is going to complain if I don't do it.

Guess what?
I'm not doing it.

SMD you ducks.

Comments
  • 3
    Linters are gross.
  • 3
    Let security complain and explain why you needed to do what you did.

    Then annotate the code with information for the linter so it ignores the one specific test in this case only.
  • 3
    @Oktokolo I looked into how, and for this particular one (Brakeman) apparently you can only ignore issues by adding the issue's id hash to an ignore file; I don't have access.
  • 2
    @Root
    But someone has access. Wait for the complaint and request the hash to be added.
  • 4
    @Oktokolo Yep, that's what I'm doing.
  • 1
    @Root I'm gonna say it like I would in real life... And I'd known you _personally_ for at least six month:

    Momma, what's that crazy bitch talking about? Explain it to me in simple words, cuz I have no fucking idea what you just talked about. Oo
  • 1
    drop table mic
    go #home
  • 2
    (and just to be sure... The linter is the crazy bitch and I really have no clue what the linter means. I read the message of Root and thought: Duh. A foreign key is in best case a primary key - which is in a relational database the holy grail you could use. So .... Wy the frigging fuck should I not use it for a mass assignment?)
  • 2
    @IntrusionCM Code is easier to understand, so here:

    Assume `params` equals the hash/dict `{column_a: value, column_b: value, foreign_key: value}`

    The linter considers this bad because it "mass-assigns" to a foreign key:
    `@model_object.update(params)`

    In this case, setting the foreign key only happens from an admin page, and the model is for a report generator; there is no danger here. An admin can specify any report id (the foreign key) they want, or `nil` to remove it. All values are okay; if it's an invalid value like "scrabble," validations will catch it and display a warning.

    Anyway, in order to fix the linter warning, I would need to either do...

    A) double-saving:
    ```
    @model_object.foreign_key = params.delete(:foreign_key)
    success = @model_object.save
    @model_object.update(params) if success
    ```
    This is bad because it both makes two trips to the DB and decreases readability.

    B) Re-implement the update() mass-assignment to confuse the linter:
    ```
    params.each do |column, value|
    # call `model_object.column=value` for each pair
    @model_object.send("#{column}=", value)
    end
    @model_object.save
    ```
    This looks scary (because of the `send()`), is confusing to read at a glance, and doesn't actually make things any safer -- possibly even less safe.

    So instead, I'm going to just leave the code as-is and tell security to get bent if they insist.
  • 1
    @Root Sounds sane to me.

    I'd consider this as a highly opiniated test... Kinda like MySQLs safe update mode:

    When you do not specify a key constraint in the WHERE clause, or provide a LIMIT clause, or both in a UPDATE or DELETE clause, it fails hard.

    The parameter is called "--i-am-a-dummy", too.

    And I think the same applies here.

    A sounds very dangerous to me. If an Auto increment is involved, the delete might lead to an excessive burn of IDs, which might end up in a roll up... Which usually opens the box of the pandora.
    Even if there is no Auto Inc involved, deleting rows "Just for the fun of it" (and possibly triggering cascades and constraint checkings on all involved tables) is really worse.

    B sounds even worse. Code wise it's "security by obscurity"… Performance wise even worse, due to looping (Bad Omen TM).

    In both cases, especially due to ORM, you'll burn a lot of performance... Since you invalidate the whole ORM state object by object, leading to reinitialisation / update of the lifecycle inside the ORM.

    So... The linter might have a good Intention, but it should be a "You might" not "You ARE" doing something wrong here in my opinion.
Add Comment