search

Yubico / php-u2flib-server

(OBSOLETE) U2F library in PHP
https://developers.yubico.com/php-u2flib-server/
BSD 2-Clause "Simplified" License
289 stars 68 forks source link

Split of U2F.php into a Class per File #44

Closed paul999 closed 8 years ago

paul999 commented 8 years ago

This PR splits of each classes into their own file. Fixes #43.

StormTide commented 8 years ago

This would reintroduce #2 (Composer RCE MITM by reference).

klali commented 8 years ago

Is there any reason to not do require on the dependant classes in U2F.php ? thus avoiding the dependency on composer and it's autoloader (and changing depending software).

paul999 commented 8 years ago

composer is the standard for using dependency management within the rest of the php world. If anyone want to properly use your libary with composer you need to make use of their provided services, like autoloading. Nearly all large PHP projects use composer for their dependency management. Having it as dependency is not a bad thing. You should not manually require files. That is the task for the autoloader.

StormTide commented 8 years ago

Composer is not a PHP standard, its not even PHP-FIG. It is subject to a critical remote code execution exploit that, while acknowledged, has gone unpatched for years. Adding this PR will reintroduce #2.

The PHP-FIG standard for autoloading is defined in PSR-4 http://www.php-fig.org/psr/psr-4/

When Composer resolves the remote code execution vulnerability, this PR could be re-evaluated, but until then it would make the U2F library actually reduce a web application's security rather than add to it as a 2nd factor auth layer is intended to do.

StormTide commented 8 years ago

Would suggest the path forward on this one would be to refactor to a vendor/subnamespace/class format eg Yubico/U2F/Server.php or similar along the PSR-4 convention, I've opened an issue.

paul999 commented 8 years ago

I never said it was a PHP standard, nor I mentioned php-fig. Composer provides a psr compitable autoloader, nothing else.

However, if this is the official opinion on this PR, I won't be using this libary due to missing proper composer intergration and not following commonly used (And supported) dependency managers.

maxnet commented 8 years ago

If anyone want to properly use your libary with composer you need to make use of their provided services, like autoloading.

Why? In what way are the users of the library affected, if the library did not use auto-loading internally?

We are talking about a mere 50 lines worth of helper classes here that are included in the main file. Think the overhead auto-loading adds is more than always parsing those lines.