Mark parameters in bundled extensions as sensitive

[TimWolla]

No description provided.

[TimWolla]

Anyone has an idea why this fails the one job on AppVeyor?

========DIFF========
001+ Fatal error: Uncaught Error: Call to undefined function zend_test_sensitive_parameter() in C:\projects\php-src\ext\zend_test\tests\sensitive_parameter.php:2
001- Fatal error: Uncaught Error: Dummy in %ssensitive_parameter.php:2
     Stack trace:
003- #0 %ssensitive_parameter.php(2): zend_test_sensitive_parameter(Object(SensitiveParameterValue))
004- #1 {main}
003+ #0 {main}
       thrown in %ssensitive_parameter.php on line 2
========DONE========
FAIL Test that zend_mark_function_parameter_as_sensitive works. [C:\projects\php-src\ext\zend_test\tests\sensitive_parameter.phpt] 

The output matches the one of sensitive_parameter_disable_functions.phpt, it looks like as if disable_functions= "leaked" to an unrelated test.

[bwoebi]

May I suggest that we add the #[SensitiveParameter] in the stub files instead and generate it from there?
This seems like an unncessary disconnect between definition and setting the attribute.

Ideally by generally introducing attributes to stub file processing, without hard dependency on zend_ce_sensitive_parameter.

[TimWolla]

@bwoebi Apparently supporting full-blown attributes is hard:
https://chat.stackoverflow.com/transcript/11?m=54325171#54325171

And even just supported “sensitive parameters” from the stubs is non-trivial: https://chat.stackoverflow.com/transcript/message/54346688#54346688

I've intentionally requested Ilija's review, because he was looking into adding stub support.

[iluuu1994]

@TimWolla FWIW @kocsismate agrees that this should be in the stubs and he'll have a look when he has time.

[TimWolla]

Yeah, I agreed initially and I agree even more after creating this PR. I've marked the PR as a draft for the time being, until the stub support is ready.

[NattyNarwhal]

There are cases where only a subset of the string is sensitive (ODBC and Postgres connection strings). How would this be able to handle that?

[TimWolla]

There are cases where only a subset of the string is sensitive (ODBC and Postgres connection strings). How would this be able to handle that?

A parameter can only be marked as sensitive in full. If those connection strings include passwords, then they should likely be marked as sensitive (especially since a username or hostname might also be considered sensitive by some users).

[ramsey]

Looks like the stub support was merged.

[kocsismate]

Looks like the stub support was merged.

No, not yet :) But now that I merged my huge stub related PR, I can work on the stub support of attributes without having conflicts. I'll start it early June.

[ramsey]

Sounds good. Feature freeze for 8.2 is 19 July. ;-)

[TimWolla]

Squashed the fixup commits without modifying the base commit:

https://github.com/php/php-src/compare/77653397d905d027ff46c710f5dd0af6b157337e..5d1dfc73a8403abb7bfc6dda50c845f554fddeb1

Then rebased the squashed commits onto the latest master, while dropping the zend_test changes:

https://github.com/php/php-src/compare/5d1dfc73a8403abb7bfc6dda50c845f554fddeb1..6960dd1aff38ea0dcd6fa1dc49401f7c7256884e

[bukka]

The selected params look good to me.

[TimWolla]

Thanks for the reviews. Rebased on the latest master to fix the merge conflict. Once CI passes: How would this PR be appropriately merged? Is the "Rebase and merge" button on GitHub fine for this?

[kocsismate]

How would this PR be appropriately merged? Is the "Rebase and merge" button on GitHub fine for this?

I'd be fine with just squashing all the commits into one. But if you liked a more fine-grained commit history, then feel free to "rebase and merge".

[TimWolla]

But if you liked a more fine-grained commit history, then feel free to "rebase and merge".

Yes, I believe it is useful to have this split per extension, similarly to your other Stub changes for the constants.

Merged now, thanks again!