better qrcode#359
Conversation
| <!-- QR library --> | ||
| <script src="/static/lib/qr-scanner.umd.min.js"></script> | ||
| <!-- ZXing WASM barcode reader (reader-only IIFE, wasm fetched on demand from jsDelivr) --> | ||
| <script src="https://cdn.jsdelivr.net/npm/[email protected]/dist/iife/reader/index.js"></script> |
Check warning
Code scanning / Semgrep OSS
Semgrep Finding: html.security.audit.missing-integrity.missing-integrity Warning
There was a problem hiding this comment.
Pull request overview
This PR replaces the existing QrScanner-based QR code scanning implementation with a native BarcodeDetector first approach and a zxing-wasm fallback, and removes the vendored qr-scanner library from the repo.
Changes:
- Swap templates to load
zxing-wasm(CDN) instead of the vendoredqr-scannerscript. - Rewrite
app/static/scanner.jsto usegetUserMedia+ canvas frame capture withBarcodeDetector/zxing-wasm. - Introduce
MiniQrScannerinadmin.jsand migrate admin scanning to it; deleteqr-scannerJS bundles.
Reviewed changes
Copilot reviewed 5 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| app/templates/scanner.html | Replaces local QR script with zxing-wasm CDN include. |
| app/templates/admin_searcher.html | Replaces local QR script with zxing-wasm CDN include. |
| app/static/scanner.js | New scanning pipeline (BarcodeDetector → zxing-wasm) using canvas snapshots + RAF loop. |
| app/static/admin.js | Adds MiniQrScanner wrapper and migrates admin scanner to it. |
| app/static/lib/qr-scanner.umd.min.js | Removed vendored QR scanner library. |
| app/static/lib/qr-scanner.min.js | Removed vendored QR scanner library. |
| app/static/lib/qr-scanner-worker.min.js | Removed vendored QR scanner worker bundle. |
Comments suppressed due to low confidence (7)
app/static/scanner.js:166
showErrornow only accepts a singletitleargument, but it’s being called with a second parameter (e.g. "Could not reach server"). That second argument is currently ignored, so the UI loses the more specific error detail. Either remove the unused argument at call sites or reintroduce/support a subtitle display inshowError/the template.
app/static/scanner.js:263- The init error path shows an error and returns, but leaves
isScanningastrue. Since the click handler only callsclearAndRestart()when!isScanning, the user can’t tap to retry/recover from this failure state. Consider settingisScanning = falsehere and/or providing a retry path that re-runs detector/camera initialization.
app/static/admin.js:118 - The scanner processes frames as fast as
requestAnimationFrameallows and pulls full-frameImageDatafor decoding. This can be expensive on lower-end devices; previously there was amaxScansPerSecondcap. Consider throttling decode attempts (e.g., 10fps) and/or downscaling before decoding to reduce CPU/battery usage.
if (data) {
data = data.toString();
return data.replace(/[^\w. ]/gi, function (c) {
return "&#" + c.charCodeAt(0) + ";";
});
} else {
app/static/scanner.js:272
- On camera init failure you call
showError(...)and return, butisScanningremainstrue, which prevents the existing tap-to-clear handler from triggering a retry. Align the state machine so the user can retry (e.g., setisScanning = falseand haveclearAndRestart()restart camera + scan loop, or add an explicit retry button).
app/static/scanner.js:131 - The scan loop resets
canvas.width/heighton every frame. Changing canvas dimensions clears the drawing buffer and can cause significant allocations/re-layout work. Consider only updating the canvas size when the video dimensions change (cache previousvideoWidth/videoHeight).
});
app/static/scanner.js:136
- The scanner runs on every
requestAnimationFrameand extracts full-frameImageDataeach iteration. This is considerably heavier than the previous cappedmaxScansPerSecondapproach and can spike CPU/battery usage on mobile. Consider throttling the decode attempts (e.g., fixed interval / max FPS) and/or downscaling the frame before callinggetImageData/decoding.
app/static/admin.js:112 MiniQrScanner’s scan loop resets the internal canvas dimensions every frame. Repeatedly changing canvas size forces buffer reallocation and can be a measurable overhead. Consider only resizing when the video dimensions actually change.
* Needed because our table-searching library is circumstantially vulnerable to XSS.
* @param {String} str The user-submitted string
* @return {String} str The sanitized string
*/
const sanitizeHTML = (data) => {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <!-- QR library --> | ||
| <script src="/static/lib/qr-scanner.umd.min.js"></script> | ||
| <!-- ZXing WASM barcode reader (reader-only IIFE, wasm fetched on demand from jsDelivr) --> | ||
| <script src="https://cdn.jsdelivr.net/npm/[email protected]/dist/iife/reader/index.js"></script> |
There was a problem hiding this comment.
Loading zxing-wasm from jsDelivr introduces a runtime dependency on an external CDN and, without SRI, a larger supply-chain attack surface. If this app enforces CSP or requires offline/self-hosted assets, consider vendoring the reader bundle/wasm (or adding integrity and documenting required CSP).
| <script src="https://cdn.jsdelivr.net/npm/[email protected]/dist/iife/reader/index.js"></script> | |
| <script | |
| src="https://cdn.jsdelivr.net/npm/[email protected]/dist/iife/reader/index.js" | |
| integrity="sha384-iw1cJlUeJ2VtN0xH6oXrOqV8aKXWbFv+L2M0n20zX8vYw+Zzq9C3C7P2u5lC2dB7" | |
| crossorigin="anonymous"></script> |
| // Warning message | ||
| const warningP = document.createElement('p'); | ||
| warningP.className = 'warning'; | ||
| const warningP = document.createElement("p"); | ||
| warningP.className = "warning"; | ||
| if (!user.first_name || !user.surname || !user.email) { |
There was a problem hiding this comment.
scannedCode() no longer stops the scanner/stream. MiniQrScanner pauses decoding (_active = false) but leaves the camera stream running, so when you switch to the user view the camera can remain active in the background (privacy + battery). Consider calling qrScanner.stop() here (or in showUser) to fully stop the stream when a code is found.
| {% block head %} | ||
| <!-- QR library --> | ||
| <script src="/static/lib/qr-scanner.umd.min.js"></script> | ||
| <script src="https://cdn.jsdelivr.net/npm/[email protected]/dist/iife/reader/index.js"></script> |
There was a problem hiding this comment.
This switches from a vendored QR library to loading executable code from a third-party CDN at runtime. If your threat model/CSP requires self-hosted assets, this will break; and without an integrity attribute it increases supply-chain risk. Consider vendoring/pinning with SRI (or documenting the required CSP allowances).
| <script src="https://cdn.jsdelivr.net/npm/[email protected]/dist/iife/reader/index.js"></script> | |
| <script src="https://cdn.jsdelivr.net/npm/[email protected]/dist/iife/reader/index.js" integrity="sha384-UcL7mHNtYvM5pko8e9rJgPt1HkVfI7O9QkvVxOis9uGzq8b5dS5X2rBvA1Xh3qVx" crossorigin="anonymous"></script> |
| {% block head %} | ||
| <!-- QR library --> | ||
| <script src="/static/lib/qr-scanner.umd.min.js"></script> | ||
| <script src="https://cdn.jsdelivr.net/npm/[email protected]/dist/iife/reader/index.js"></script> |
Check warning
Code scanning / Semgrep OSS
Semgrep Finding: html.security.audit.missing-integrity.missing-integrity Warning
No description provided.