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

Webmozart assertions #1132

Merged
merged 6 commits into from Feb 14, 2020
Merged

Webmozart assertions #1132

merged 6 commits into from Feb 14, 2020

Conversation

@tvdijen
Copy link
Member

@tvdijen tvdijen commented May 29, 2019

Start using Webmozart for assertions, like we do in all externalized modules and the saml2-library already.

@tvdijen
Copy link
Member Author

@tvdijen tvdijen commented May 29, 2019

What's left using old assert()s are two deprecated classes and some old php-style templates. Not worth the effort of migrating.
Since this could potentially be breaking BC, it is targeted for 2.0

@tvdijen tvdijen added this to the 2.0 milestone May 29, 2019
@jornane
Copy link
Member

@jornane jornane commented Jun 1, 2019

This means that the wishes from the administrator by means of assert_options or assert.-settings in php.ini no longer will be respected.

If assertions must be done using a library instead of calling the assert language construct of PHP, it would be preferred to use a library that uses the PHP internals instead of inventing its own way of doing things. At the very least it should respect the PHP settings.

lib/SimpleSAML/Auth/Simple.php Outdated Show resolved Hide resolved
@tvdijen
Copy link
Member Author

@tvdijen tvdijen commented Jun 1, 2019

Hi @jornane ! We are aware of this and do this on purpose.
It was discussed here: simplesamlphp/saml2#90

@ Outdated Show resolved Hide resolved
@jornane
Copy link
Member

@jornane jornane commented Jun 3, 2019

Thanks @tvdijen ! I've read through simplesamlphp/saml2#90 but I think maybe I'm missing something?

Starting in PHP 7.2 the old behavior will be deprecated.

This is about using a string as the assertion, where PHP has to run eval() on that string. SimpleSAMLphp doesn't do that so it doesn't apply to us.

The webmozart project uses __callStatic, which breaks static code analyses, making mistakes like ba0b826 go unnoticed. At the same time its yet another dependency to replace a one-liner with another one-liner. I already mentioned respecting the administrators wishes with regard to php.ini settings for assertions.

At the end of the day, this is a bikeshed issue, so if this has already been decided, I won't stand in the way. However I do want to point out that the original reasoning for this change (deprecation) has been based on a misunderstanding. The current way SimpleSAMLphp does things is not currently, or about to be, deprecated.

@tvdijen

This comment has been hidden.

@tvdijen tvdijen force-pushed the webmozart branch 4 times, most recently from d8df0b5 to 2242033 Jul 12, 2019
@tvdijen tvdijen force-pushed the webmozart branch from 2242033 to c068c51 Jul 23, 2019
@tvdijen tvdijen force-pushed the webmozart branch 3 times, most recently from 7028b3e to 4bc6c20 Aug 10, 2019
@jaimeperez
Copy link
Member

@jaimeperez jaimeperez commented Aug 12, 2019

The main reason for having something like webmozart/assert is that we need to check method args and logic conditions. We haven't been using assert() (only) for development purposes, but also as sanity checks. With that in mind, keeping using assert() is not fine because you can (and probably should) turn it off in production. Given that this is security-related software granting access to resources, an error somewhere could lead to a security incident that could be exploited, and that is something we shouldn't allow to happen. If there is a bug in the software, and that can potentially lead to a security issue, it's our responsibility to avoid that, not the user's to configure the web server to stop on a failed assert().

@fkooman
Copy link

@fkooman fkooman commented Aug 12, 2019

Using assert (any kind of assert) to do input validation is not ideal, whether it is the built in assert or a library doing "assert".

Many of the "asserts" here (in PR) seem to be about testing whether or not something is an array. Since PHP 5.1 the array typehint is supported in PHP, but it doesn't seem to be used.

Furtermore, a nicer approach would be to (fully) document a function's parameters using docblock and run static code analysis tools like Psalm, Phan or PHPstan to help you make sure a function taking a string can never get an
int. Then you do not need the "assert" any longer. This works especially well for internal functions that never receive "user" input. In addition, this prepares you for a migration to PHP 7 where you can use e.g. php-cs-fixer to use your docblock and convert it to "real" PHP static types.

Of course, you can still keep "assert", but that should really be for testing for exceptional cases where (static) type hinting does not (sufficiently) work, e.g. missing array keys. Although, there are some solutions for that as well in the static code analysis tools.

Also, as this intended for 2.0, wouldn't it be possible to require PHP >= 7.2 (RHEL/CentOS 8) for 2.0? :-)

@tvdijen
Copy link
Member Author

@tvdijen tvdijen commented Aug 12, 2019

This PR was (at first) never targeted for 2.0, so all your points are valid and this PR exists merely as a reminder.
I'm working on a redo of the typehints-PR, where the source will be fully typehinted.. Then only a few assertions will be left, mostly checking for the existence of array keys. Hopefully we can branch of 1.18 soon and start working on the real deal..

@jaimeperez
Copy link
Member

@jaimeperez jaimeperez commented Aug 12, 2019

I agree that using any kind of assert is not ideal, but it's better than nothing. We have already started using type hints and static analysis with psalm. Tim has done an incredible job on that so far, and fixed a lot of issues. This includes having proper phpdoc blocks that can be used with automatic tools like php-cs-fixer.

Still, that's not enough. First, because typehints don't cover all cases. Due to PHP's flexibility, we have some places where an argument can have several types. You cannot typehint that. Second, because adding typehints breaks method signatures. And you cannot break method signatures without breaking backwards-compatibility.

Regarding the minimum version of PHP required, we shall see, although I'm not particularly optimistic. We still have plenty of users on PHP 5.x, unfortunately.

@tvdijen
Copy link
Member Author

@tvdijen tvdijen commented Aug 12, 2019

Regarding the minimum version of PHP required, we shall see, although I'm not particularly optimistic. We still have plenty of users on PHP 5.x, unfortunately.

We're gonna have to move forward at some point and the major distributions all have a possibility to support PHP 7+.. RHEL 8 comes with PHP7.2 by default, so for 2.0 this is the time to move if you ask me

@tvdijen tvdijen force-pushed the webmozart branch from 4bc6c20 to 1e57347 Aug 31, 2019
@tvdijen tvdijen force-pushed the webmozart branch 3 times, most recently from d0be85a to 4e83fd5 Sep 24, 2019
@tvdijen tvdijen force-pushed the webmozart branch 2 times, most recently from 6b79992 to a3f0a9b Oct 14, 2019
@codecov
Copy link

@codecov codecov bot commented Oct 14, 2019

Codecov Report

No coverage uploaded for pull request base (master@e7ff7c6). Click here to learn what that means.
The diff coverage is 38.01%.

Impacted file tree graph

@@           Coverage Diff            @@
##             master   #1132   +/-   ##
========================================
  Coverage          ?   36.3%           
  Complexity        ?    3692           
========================================
  Files             ?     136           
  Lines             ?   11426           
  Branches          ?       0           
========================================
  Hits              ?    4148           
  Misses            ?    7278           
  Partials          ?       0
Impacted Files Coverage Δ Complexity Δ
modules/core/lib/ACL.php 0% <0%> (ø) 51 <0> (?)
modules/saml/lib/Auth/Process/TransientNameID.php 0% <0%> (ø) 2 <0> (?)
modules/saml/lib/Auth/Process/AttributeNameID.php 0% <0%> (ø) 8 <0> (?)
...les/core/lib/Auth/Process/WarnShortSSOInterval.php 0% <0%> (ø) 5 <0> (?)
lib/SimpleSAML/XHTML/IdPDisco.php 0% <0%> (ø) 69 <0> (?)
modules/saml/lib/IdP/SQLNameID.php 0% <0%> (ø) 10 <0> (?)
lib/SimpleSAML/Error/NotFound.php 0% <0%> (ø) 4 <0> (?)
lib/SimpleSAML/SessionHandlerStore.php 0% <0%> (ø) 7 <0> (?)
.../SimpleSAML/Metadata/MetaDataStorageHandlerPdo.php 0% <0%> (ø) 7 <0> (?)
lib/SimpleSAML/Error/MetadataNotFound.php 0% <0%> (ø) 1 <0> (?)
... and 80 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e7ff7c6...c96df0d. Read the comment docs.

@tvdijen tvdijen added this to In progress in 2.0 release via automation Nov 22, 2019
@tvdijen tvdijen force-pushed the webmozart branch from 83bec2a to c96df0d Feb 14, 2020
@tvdijen tvdijen merged commit 0cf0f5b into master Feb 14, 2020
0 of 5 checks passed
0 of 5 checks passed
continuous-integration/travis-ci/push The Travis CI build failed
Details
Scrutinizer Running
Details
@appveyor
continuous-integration/appveyor/branch Waiting for AppVeyor build to complete
Details
@appveyor
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
2.0 release automation moved this from In progress to Done Feb 14, 2020
@tvdijen tvdijen deleted the webmozart branch Feb 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
2.0 release
  
Done
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants