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

TaintedExtract should not be thrown for array values (only keys) #11263

Open
gharlan opened this issue Feb 7, 2025 · 2 comments
Open

TaintedExtract should not be thrown for array values (only keys) #11263

gharlan opened this issue Feb 7, 2025 · 2 comments

Comments

@gharlan
Copy link
Contributor

gharlan commented Feb 7, 2025

extract(['foo' => $_GET['foo']]);

This throws a TaintedExtract error. In my opinion TaintedExtract should only look at the array keys (used as variable names). The given code is the same as:

$foo = $_GET['foo'];

which is safe.

While this would be dangerous:

extract([$_GET['foo'] => 'foo']);

https://psalm.dev/r/6620d37bc4

  • TaintedExtract for line 3 is correct
  • TaintedExtract for line 5 is wrong in my opinion. There is nothing wrong in this line. If the variable $foo is used in any other taint sink, it is reported anyway (see line 6).
  • So TaintedHtml and TaintedTextWithQuotes for line 6 are also correct
Copy link

I found these snippets:

https://psalm.dev/r/6620d37bc4
<?php // --taint-analysis

extract([$_GET['foo'] => 'foo']);

extract(['foo' => $_GET['foo']]);
echo $foo;
Psalm output (using commit 7b4ab31):

ERROR: TaintedHtml - 6:6 - Detected tainted HTML

ERROR: TaintedTextWithQuotes - 6:6 - Detected tainted text with possible quotes

ERROR: TaintedExtract - 3:9 - Detected tainted extract

ERROR: TaintedExtract - 5:9 - Detected tainted extract

@orklah
Copy link
Collaborator

orklah commented Feb 8, 2025

I guess if every key for an array is known and the array is sealed (meaning no other keys could be in the array) it should be safe to not emit a TaintedExtract as long as the taint is transmitted to every new variable created by extract

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants