gesinn-it / QRLite

Lightweight MediaWiki extension to render QR codes.
GNU General Public License v3.0
3 stars 4 forks source link

Switch to external qrcode library and don't write to the filesystem #7

Closed samwilson closed 1 year ago

samwilson commented 4 years ago

Remove the phpqrcode library and replace it with endroid/qr-code, loading the new one with Composer.

This removes the need to write the qrcode files to the filesytem, which simplifies the code and removes an attack vector.

samwilson commented 4 years ago

This may need some discussion! :-) This PR is just me suggesting an idea.

One thing I'm not sure about is how to translate the old library's idea of the size parameter to the new library's. I've added a factor of 30, but this is arbitrary. It sort of looks about right to me, but maybe others have different ideas?

My main motivation for this is to not have to give the web server user write access to the filesystem. It also looks like the phpqrcode library is abandoned, so it's good to use something that's actively maintained.

samwilson commented 4 years ago

This also fixes a bug with syntax such as {{#qrlite:foo"bar;>foo<bar}}

samwilson commented 4 years ago

@gesinn-it @Fannon do either of you have a moment to review this? Thanks!

Fannon commented 4 years ago

Hi @samwilson

thanks for the PR! Currently I don't have a setup to really test this extension, but in general: The changes you propose definately make sense.

I see two potentials issues:

samwilson commented 1 year ago

@Fannon sorry for my very slow reply! This slipped through the net.

I think the dependency on Composer is acceptable these days. There are lots of extensions that install dependencies like this, and Composer has become a required part of MediaWiki.

Great point about the error handling, I've changed the patch to leave in the Exception catching. There's also the error that happens when the composer library isn't installed, so I've also added a check for that.

samwilson commented 1 year ago

Oops, I also need to upgrade endroid/qr-code to 4.6. Doing that now.

samwilson commented 1 year ago

Okay, this is ready for review again now. Thanks!

samwilson commented 1 year ago

I spoke too soon: #8 needs to be merged before this one.

gesinn-it-gea commented 1 year ago

Thanks for your PR (and sorry for the late feedback)! We'll have a look and come back to you. Not writing to the file system and using a maintained lib are definitely good things!

samwilson commented 1 year ago

Thanks! :-)

I've also suggested adding QRLite to MWStake/nonwmf-extensions, so that it appears in https://codesearch.wmcloud.org and future deprecations etc can be found and fixed.

gesinn-it-gea commented 1 year ago

@samwilson ... sounds good. We are enhancing the CI this week a little bit so that QRLite is in a good shape again.