[5.8] Strip slashes for unique rule helper - #27940
Conversation
| { | ||
| $this->idColumn = str_replace(',', '', $idColumn ?? $model->getKeyName()); | ||
| $this->ignore = str_replace(',', '', $model->{$this->idColumn}); | ||
| $this->idColumn = addslashes($idColumn ?? $model->getKeyName()); |
There was a problem hiding this comment.
Why would addslashes be relevant for the column, which I would expect is never a user provided input?
There was a problem hiding this comment.
Hmm I wonder why a “user” would request anything with any special characters.... maybe because they are maliciously trying things to a site?
There was a problem hiding this comment.
I think mfn's point is the same as the one Taylor made in the blog post: the developer using Laravel should never expose the ability for their user to define the database column through an input.
Worst case scenario the user input should be decoupled from the database, which would never be vulnerable.
There was a problem hiding this comment.
this could have simple avoided by type-hinting the method
|
|
||
| $this->ignore = str_replace(',', '', $id); | ||
| $this->idColumn = str_replace(',', '', $idColumn ?? 'id'); | ||
| $this->ignore = addslashes($id); |
There was a problem hiding this comment.
Stupid q: is this escaping so the value can be literally used in a DB query, where no further escaping happens?
There was a problem hiding this comment.
Using addslashes() to escape a value for a DB query is quite dangerous.
There was a problem hiding this comment.
My thoughts, thus I'm asking. Not very familiar with the exact code, though.
But I know at least for Postgres it doesn't even work correctly.
There was a problem hiding this comment.
We aren't adding slashes for the DB query. We are adding slashes for the str_getcsv call in the validator. This has nothing to do with trying to escape data for a DB query.
To clarify, the vulnerability is that a user can construct a string that is sent through the str_getcsv call by the validator and uses a column of their choosing in the DB query. The value is bound as a PDO parameter. However, columns are not. Therefore, we need to ensure that the "id" portion of the unique validation rule is a single string and can't be manipulated with commas / quotes to create multiple values when parsed by str_getcsv.
There was a problem hiding this comment.
👍 thanks for clarification.
I think however that the code could use some comments as to why all these special handling.
There was a problem hiding this comment.
Because a vulnerability was reported. It was possible to perform a XSS using the ID
There was a problem hiding this comment.
I know this is old article but i guess it tells you why addslashes is not perfect solution here - http://shiflett.org/blog/2006/addslashes-versus-mysql-real-escape-string Not sure though how it's realted to what @taylorotwell wrote, i guess someone needs to check it out.
There was a problem hiding this comment.
Keep in mind that Taylor already clarified, this is not for escaping for the database but for another case.
Also, the code already changed in the meantime and the addslashes was moved to the __toString and only for the ignore and not the column.
|
A regression test would be cool. 😉 |
|
Good job guys! You saved probably a trillion dollars by acting so fast on this to many organizations who wouldn't even had known! If i was filthy rich I'd buy each of you |
12 participants
This removes the need to introduce a breaking change while prevents manipulating the generated rule string.