handle psalm false-positives #4062
Conversation
staabm
changed the title
$filename is escaped for shell use
staabm
changed the title
$filename is escaped for shell use
otherwise psalm doesn't realize the properly escaped text
redaxo/src/addons/structure/plugins/content/lib/article_content_base.php
Show resolved
Hide resolved
Co-authored-by: Gregor Harlan <mail@gh01.de>
|
hier gehts weiter sobald die ganzen PRs die heute enstanden sind gemerged sind, da sich diese auf die psalm errors auswirken |
| } elseif (('delete' == $function || 'download' == $function) && '.sql' != substr($impname, -4, 4) && '.tar.gz' != substr($impname, -7, 7)) { | ||
| $impname = ''; | ||
| } | ||
| /** @psalm-taint-escape text */ |
There was a problem hiding this comment.
@staabm - mind putting up a PR to psalm to move the text sinks into a file one? A generic one for read/write/delete is fine. - I can submit the documentation if that convinces you ;-)
Example on how to do so: vimeo/psalm#4604
Reason: This will break once I change it myself otherwise ;-)
Ref vimeo/psalm#4596
There was a problem hiding this comment.
@LukasReschke I had a quick look around the codebase regarding your suggestion.
I am not sure, I could deliver a solution right now, with the knowledge I have of the psalm codebase
There was a problem hiding this comment.
@staabm np :-) - It's just a bit of boilerplate shifting around, but it could indeed be significantly better documented :)
Did so at vimeo/psalm#4630
| @@ -43,6 +43,7 @@ | |||
| // ------------------------------ FUNC EXPORT | |||
|
|
|||
| $exportfilename = strtolower($exportfilename); | |||
| /** @psalm-taint-escape shell */ | |||
| $filename = preg_replace('@[^\.a-z0-9_\-]@', '', $exportfilename); | |||
There was a problem hiding this comment.
wirklich nur shell? Ich meine, ich hätte gestern gestern in den Fehlern gesehen, dass die Variable am Ende auch in fopen landet. Und das sollte hierdurch ja eigentlich auch sicher sein, oder?
There was a problem hiding this comment.
ich hab hier das "escaped" markiert, was psalm gemeckert hat.
ggf. hat sich das auch mit dem neuen release geändert
| @@ -686,8 +686,9 @@ protected function fireCallbacks(rex_sql $sqlFields) | |||
| $fieldType = $field->getValue('type_id'); | |||
| $fieldAttributes = $field->getValue('attributes'); | |||
| $fieldValue = self::getSaveValue($fieldName, $fieldType, $fieldAttributes); | |||
| $fieldId = (int) $field->getValue('id'); | |||
There was a problem hiding this comment.
ich bin am überlegen, ob wir in rex_sql eine getIntValue-Methode aufnehmen sollten, da man ja sehr oft ints abfragt.
There was a problem hiding this comment.
oder in getValue nen 2. parameter um den typ zu übergeben (analog rex_get() etc)
|
da wir jetzt eine taint baseline haben, würde ich hier mergen wollen - gezielt einzelne issues beheben kann man dann in separaten PRs weiter angehen. |
https://psalm.dev/docs/security_analysis/
https://psalm.dev/docs/security_analysis/avoiding_false_positives/
upstream issues: