Generate QR codes internally

[mallorydxw]

These commits generate QR codes via PHP instead of via the Google Chart API. This stops sending sensitive data to a third party.

Resolves: https://github.com/WordPress/two-factor/issues/42

Note that this uses the endroid/qr-code composer library, so to test this please run composer install after checking this branch out.

I suspect there's a standard WordPress way of including dependencies but I'm not sure what it is. I can update the PR if you let me know.

I would suggest storing the otpauth:// URI in the database instead of allowing passing arbitrary strings to the QR code generator. Just in case. But I haven't done that in this PR. Let me know if that's something you'd want to see before approving this, or if it ought to be raised as a separate PR.

Thanks.

[kasparsd]

Thanks for the pull request @mallorydxw! This is a much needed feature!

Looks like the https://github.com/endroid/qr-code library is licensed under MIT so it should be compatible with GPL2+ as far as I understand.

The only issue is with the potential namespace conflicts. Bundling this library with the two-factor plugin means that it would trigger a PHP error if the same library is used either by another plugin or theme. This gets worse with the additional dependencies required by the library itself.

Is there a way we could fork just the core functionality required for generating the most basic QR code and make it use the core WP image manipulation APIs?

[mallorydxw]

Thanks for the direction, @kasparsd!

The only issue is with the potential namespace conflicts. Bundling this library with the two-factor plugin means that it would trigger a PHP error if the same library is used either by another plugin or theme. This gets worse with the additional dependencies required by the library itself.

I noticed the endroid/qr-code library was using another library called BaconQrCode so we're now using the BaconQrCode library directly.

I've copied the BaconQrCode library to the includes/ directory, and I've changed the root namespace from BaconQrCode to TwoFactor\BaconQrCode to prevent collisions. I've added a small autoloader to two-factor.php so that there's no need to manually include a dozen or so different .php files.

and make it use the core WP image manipulation APIs?

I couldn't see how to use those APIs to create images, rather than merely resize or crop images. But I did notice that we could drop the requirement on the GD extension by generating SVG images instead of PNGs.

Looks like the https://github.com/endroid/qr-code library is licensed under MIT so it should be compatible with GPL2+ as far as I understand.

The BaconQrCode library is BSD 2 clause so I believe that's compatible too. The code is thankfully already commented with licence declarations in each and every .php file.

[DanielRuf]

The only issue is with the potential namespace conflicts. Bundling this library with the two-factor plugin means that it would trigger a PHP error if the same library is used either by another plugin or theme. This gets worse with the additional dependencies required by the library itself.

Namespaces should be scoped to prevent such well-known issues. https://github.com/humbug/php-scoper https://github.com/coenjacobs/mozart https://github.com/TypistTech/imposter-plugin

[mallorydxw]

Thanks @DanielRuf that's useful to know.

Anyway, I think this PR may be ready to be merged. The namespace has been scoped. It doesn't rely on gd or imagick extensions.

Please let me know if there's any further improvements I need to make before you're ready to merge it.

[jeffpaul]

@kasparsd any thoughts on a review of this PR and whether it's ready for consideration in v0.8.0?

[walterebert]

The generated QR code was not accepted in the OTP clients I tested. $otpauth_uri is double encoded. Removing urlencode() seems to solve the problem.

I also have a suggestion. The QR code is a SVG image. This could be directly embedded into the HTML. This would remove the need for the AJAX request.

[joshbetz]

Fixed the issues flagged by @walterebert here: https://github.com/WordPress/two-factor/tree/dxw-feature/generate-qr-internally.

@kasparsd Do you think we should commit the library directly here or add to composer?

[jeffpaul]

Per yesterday's bug scrub, @georgestephanis is in favor of being able to generate QR codes internally but we're going to punt this to a future release (ahead of a core merge proposal) so the 0.8.0 focus can narrow on the U2F deprecation.

[iandunn]

IMO, it'd be best to use a JavaScript library, so that we're not running a bunch of 3rd party code on the server unnecessarily. The library mentioned in #42 seems like a good one, but there may be a few others in npm that are lean, popular, and well-tested.

If an SVG is used, it'd be best to put it in a CSS background-image property, rather than an <img> tag. Keeping it out of the DOM prevents any chance of malicious JavaScript being injected.

Installing via npm instead of forking would give us npm/GitHub notifications if there's a security vuln discovered in it in the future.

Those changes would help with the eventual merge to Core, since the standards there for 3rd party libraries are very high.