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

Default QRcode.js library issues #93

Closed
pkarjala opened this issue Aug 29, 2024 · 11 comments
Closed

Default QRcode.js library issues #93

pkarjala opened this issue Aug 29, 2024 · 11 comments
Milestone

Comments

@pkarjala
Copy link

pkarjala commented Aug 29, 2024

When installing devise-otp into a fresh Rails 7.1 project using sprockets for asset compilation and importmaps, the default included QRCode.js library does not properly operate and generate QR codes.

Relevant Gems:

  • sprockets 4.2.1
  • sprockets-rails 3.4.2
  • importmap-rails 2.0.1
  • devise 4.9.4
  • devise-otp 0.7.1

application.js file:

// Configure your import map in config/importmap.rb. Read more: https://github.com/rails/importmap-rails
// Bootstrap dependencies for Sprockets
//= require popper
//= require bootstrap-sprockets
// Devise-otp dependencies
//= require devise-otp

import "jquery";
import "select2";
import Rails from "@rails/ujs";

Rails.start()

window.addEventListener('DOMContentLoaded', () => {
  $(".select2").select2();
})

When set up using the instructions in the default readme, the assets compile without issue. When logging in and viewing the otp/token/edit page, the QR code does not render.

Inspecting the page where the QR code would be rendered displays the following JS console errors:

Uncaught ReferenceError: QRCode is not defined
    at edit:79:15

Uncaught TypeError: Cannot read properties of undefined (reading '_android')
    at application-1c9eb959d4e521bbab99cec2bb53e7fb4bd94091d1906d7b80bc0dd645840c15.js:5189:12
    at application-1c9eb959d4e521bbab99cec2bb53e7fb4bd94091d1906d7b80bc0dd645840c15.js:5364:4
    at application-1c9eb959d4e521bbab99cec2bb53e7fb4bd94091d1906d7b80bc0dd645840c15.js:5515:3

The first error points to the script tag generated in the devise-otp edit view where the QR code is supposed to be rendered, as follows (with sensitive data obfuscated):

//<![CDATA[

              new QRCode("qrcode", {
                text: "otpauth://totp/MY-APPLICATION:user%40example.com?secret=SOMELONGSECRETTEXTSTRING&issuer=MY-APPLICATION",
                width: 256,
                height: 256,
                colorDark : "#000000",
                colorLight : "#ffffff",
                correctLevel : QRCode.CorrectLevel.H
              });
            
//]]>

The second set of errors refer to the qrcode.js file itself, referring to this specific line reference:

https://github.com/wmlele/devise-otp/blob/master/app/assets/javascripts/qrcode.js#L283

This is a known unfixed issue in the qrcode.js library: davidshimjs/qrcodejs#292

As a result of these errors, the qrcode.js library is unable to render the QR code for the 2FA authentication.


There are a few possible solutions I can see to this issue.

  1. Fix the devise-otp gem's qrcode.js library so that it operates under Rails 7.1

This may require fixes to the qrcode.js library itself, or changes to the way that the asset is referenced in using Importmaps or other Rails 7.1 tooling. I don't think it's an asset compilation issue at this time, but I'm also not 100% certain. There is also a https://www.npmjs.com/package/qrcodejs2-fixes package which repairs the _android issue listed above.

  1. Replace the qrcode.js library with a different browser-side rendering library for QR codes in JS.

https://github.com/davidshimjs/qrcodejs has not seen any commits for 9 years and appears unmaintained, so it may not be desirable to continue to use it. It may be better to use a drop-in replacement, such as the suggested jquery-qrcode library in the https://github.com/wmlele/devise-otp/blob/master/docs/QR_CODES.md document (with the caveat that the user now must use jquery).

3. Remove the qrcode.js library and use the already required dependency rotp to generate QR codes

The suggestion at #90 (comment) notes that this is an option.
This would resolve any js dependencies entirely by removing them, and could simplify the gem. The caveat is that it now would need to render the qrcode server-side and serve it to the browser, which has different implications, and may not be desirable by everyone.

This was mistaken; rotp cannot generate QR codes, just the URIs that you can feed into a QR code generator.

  1. Replace the qrcode.js library with a server-side QR code rendering library in a different third party gem.

Pick a completely different gem to handle the client or browser side QR code rendering.


I'll continue to poke at this bug and report back with any additional findings. I suspect I'm going to override the current otp_authenticator_token_image call with something else.

@pkarjala
Copy link
Author

https://github.com/whomwah/rqrcode works as a gem for generating QR codes.

@pkarjala
Copy link
Author

https://github.com/kazuhikoarase/qrcode-generator is a more up-to-date js library for generating QR codes.

@strzibny
Copy link
Collaborator

strzibny commented Sep 1, 2024

Yes we should replace it but hard to say what's the good alternative here. Generating it server side could also be an interesting option.

@pkarjala
Copy link
Author

pkarjala commented Sep 3, 2024

I would recommend removing the JS dependency entirely and going with the rqrcode solution of generating it server-side, while leaving some hooks or overrides available for someone to be able to drop in their own preferred replacement.

Currently https://github.com/devise-two-factor/devise-two-factor uses rqrcode for generating the QR codes.

@strzibny
Copy link
Collaborator

strzibny commented Sep 4, 2024

Yes I like the server-side option a lot.

@strzibny strzibny added this to the 1.0 milestone Sep 4, 2024
@strouptl
Copy link
Contributor

@pkarjala, that sounds awesome! Do you want to take a shot at this, or did you want someone else to look into it?

@pkarjala
Copy link
Author

I'm willing to but my time is a bit crunched at the moment. My hope is to come back to a fork I made and implement a number of suggestions to turn into pull requests.

@strouptl
Copy link
Contributor

@pkarjala, if you are tied up, then I'll take a quick shot at it. I think it is an excellent option, and very worthwhile to get us up to speed with importmaps.

Let me know if you want me to hold off for any reason.

@pkarjala
Copy link
Author

pkarjala commented Sep 20, 2024

Please go ahead and work on it!

My only input is that I would forego using JS entirely and just have it render the QR code server side using rqrcode. Maybe provide an override option for folks who would prefer to use JS, but just bake the default functionality into the gem using rqrcode.

Here's my current implementation to jump off of:

### otp_rqrcode_image_generator
# Generates a QR code PNG image based on a URL using the rqrcode gem
#
# @param otp_url  A string with the URL of the OTP code
# @return         A string encoded in base64 of a PNG or a blank string if no URL was provided.
def otp_rqrcode_image_generator(otp_url)
  return '' if otp_url.blank?

  return RQRCode::QRCode.new(otp_url).as_png(resize_exactly_to: 300).to_data_url
end

@strouptl
Copy link
Contributor

Done. This eliminates all JavaScript and Sprockets dependencies.

FYI, I went with the SVG option, as that eliminates additional dependencies within rqrcode (vs. the png route). I realized some styling was missing, though, so I have added some default CSS to reapply a max-width, along with some centering), and updated the README.

@strzibny, let me know your thoughts!

@strzibny
Copy link
Collaborator

Merged. I think we simply can suggest the styling directly also in README since it might be easier for people to copy&paste.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants