Sec-ant / barcode-detector

A Barcode Detection API polyfill that uses ZXing-C++ WebAssembly under the hood.
https://www.npmjs.com/package/barcode-detector/v/latest
MIT License
100 stars 7 forks source link

`detect` method not working inside iframe #110

Closed jp06 closed 1 month ago

jp06 commented 1 month ago

If you try to use it inside an iframe, you will get the following error:

Uncaught (in promise) TypeError: Failed to execute 'detect' on 'BarcodeDetector': The provided value is not of type '(Blob or HTMLCanvasElement or HTMLImageElement or HTMLVideoElement or ImageBitmap or ImageData or OffscreenCanvas or SVGImageElement or VideoFrame)'.

The code uses instanceof for checking the type of node in the detect argument, but it can cause a faulty checking when it inside an iframe when used like in this polyfill.

Context (just learned about this today): https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/instanceof#instanceof_and_multiple_realms

The suggested way of using instanceof for checking node in MDN works. I tried this in the debugger:

o instanceof HTMLVideoElement // false
o instanceof o.ownerDocument.defaultView.HTMLVideoElement // true
Sec-ant commented 1 month ago

Thank you for raising this issue, TIL.

I followed your suggestions and created a patch in #111, could you please try the preview package to see if it works in your use case?

Technically, we should also consider checking other instances such as Blob, ImageData, ImageBitmap, etc. However, unlike DOM elements, for which we can reliably obtain their realms using the ownerDocument.defaultView API, there doesn't seem to be a straightforward way to do the same for these types. That said, compared to DOM elements, there are hardly any scenarios where it would be necessary to retrieve iframe-related elements for these other types. Therefore, I think it's safe to assume that non-DOM element types passed to the detect function are always initialized within the current realm.

jp06 commented 1 month ago

Hi @Sec-ant, thank you for the quick response!

I haven't tried it yet but I am already using patch-package for an updated HTMLVideoElement check and it seems to work and am able to scan with it.

However, (maybe off-topic for this particular issue) I just realized that the polyfill is being used which I think shouldn't be in the first place as the browser I am using that moment actually has BarcodeDetector. Could also be related to it being inside an iframe?

Sec-ant commented 1 month ago

However, (maybe off-topic for this particular issue) I just realized that the polyfill is being used which I think shouldn't be in the first place as the browser I am using that moment actually has BarcodeDetector. Could also be related to it being inside an iframe?

Unfortunately I don't have a device that supports the API natively. The polyfill is loaded when globalThis.BarcodeDetector is undefined as defined here. When the code is executed from an iframe, globalThis should also point to the window object of that iframe. So there shouldn't be an issue.

You can check the output of console.log(BarcodeDetector);. If it is polyfilled, the function/class body will be (partially) printed and you should be able to jump to the source, otherwise it's the native implementation.

jp06 commented 1 month ago

I think I just misunderstood how this works:

import { BarcodeDetector } from "barcode-detector";

I'm not sure why I was thinking it will export the native BarcodeDetector if it exists instead of the polyfill. I'll probably just use the side effect import.

Thank you!

jp06 commented 1 month ago

Technically, we should also consider checking other instances such as Blob, ImageData, ImageBitmap, etc. However, unlike DOM elements, for which we can reliably obtain their realms using the ownerDocument.defaultView API, there doesn't seem to be a straightforward way to do the same for these types. That said, compared to DOM elements, there are hardly any scenarios where it would be necessary to retrieve iframe-related elements for these other types. Therefore, I think it's safe to assume that non-DOM element types passed to the detect function are always initialized within the current realm.

Hi @Sec-ant, yeah I actually got bitten by this 😆. I changed my code earlier to input an ImageData instead of an HTMLVideoElement as I have to do some transformation to the provided image and got the same sort of error.

Sec-ant commented 1 month ago

Hi @Sec-ant, yeah I actually got bitten by this 😆. I changed my code earlier to input an ImageData instead of an HTMLVideoElement as I have to do some transformation to the provided image and got the same sort of error.

Does this work?

const currentImageData = new ImageData(foreignImageData.data, foreignImageData.width, foreignImageData.height);

Or if you have any good idea to check the type of ImageData from a foreign global window, I'd be glad to fix it.

jp06 commented 1 month ago

I just assumed this is the same issue but I actually haven't tried it yet. I'm guessing it would be fixed if these instanceof checks are also updated to check for the realm's instance of those elements. I just slept before trying this, will only be able to confirm later.

https://github.com/Sec-ant/barcode-detector/blob/1748a8d2ad4d1e202a2d92c8b24fedfbdf160997/src/utils.ts#L158-L200

jp06 commented 1 month ago

You beat me right to it and already opened a PR 😂. I think your PR would fix it. Thank you!

Sec-ant commented 1 month ago

I opened #118 to use Object.prototype.toString.call(image) === "[object ImageData]" as a fallback. It's a less strict check, but will solve problems. I learnt it from https://github.com/isw-kudos/ckeditor-pastebase64/pull/1/files.

jp06 commented 1 month ago

I installed the new version and can confirm this is fixed. Thank you!