Fix/browser support

[kwikwag]

Allows one to use fido2-lib in the browser.

This was theoretically possible also beforehand, as the groundwork was there, but browser packagers made a jumble because of the conflicts with the 'crypto' global - so adding an explicit check and doing dynamic imports was the way to go.

Since the imports are made dynamic, it means they're promised, and thus a module-level variable ready was introduced which should be await-ed before any invocation of the WebCrypto API.

I assumed any use of WebCrypto is promised anyway, so waiting for ready is only relevant in the async APIs in the main module - but I haven't checked this and I advise someone with deeper familiarity with the package review this.
Also, I have not tested with Node engine version <16.

[codecov-commenter]

Codecov Report

Attention: 14 lines in your changes are missing coverage. Please review.

Comparison is base (50ee8da) 92.85% compared to head (a73a814) 92.72%.
Report is 1 commits behind head on master.

❗ Current head a73a814 differs from pull request most recent head 7fb925e. Consider uploading reports for the commit 7fb925e to get more accurate results

Files	Patch %	Lines
lib/toolbox.js	69.56%	14 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #150      +/-   ##
==========================================
- Coverage   92.85%   92.72%   -0.13%     
==========================================
  Files          16       16              
  Lines        6019     6049      +30     
==========================================
+ Hits         5589     5609      +20     
- Misses        430      440      +10     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[wparad]

Node16 isn't LTS anymore, wouldn't it make sense to roll the minimum dependent version and then the fallback to built in subtle crypto can always be done (or laded synchronously from node) without worrying about all the async added complexity or third party library imports?

The reason I say this is, it sucks to have this complexity added to the library for 99% of implementations, if anything the hack should be added for only nodejs 16, if nodejs 16 really needs to still be supported, a separate composition root should be created with the nodejs16 hacks added in there before wrapping the more commonly used main composition root. This would keep everything clean.

[kwikwag]

Node16 isn't LTS anymore, wouldn't it make sense to roll the minimum dependent version...

I'm not a maintainer, but I tend to agree that it makes sense to advance to Node 16. In general, keep in mind that while things do fall out of maintenance, it can break legacy systems. It really depends on the tradeoff and here it's quite minute.

... fallback to built in subtle crypto can always be done (or laded synchronously from node) without worrying about all the async added complexity or third party library imports?

If I could have made the crypto import optional and synchronous at the same time I would. But I am unaware of such an option. Perhaps there is a way to include a shim or polyfill of some sort, for the case of browser-targeted bundlers?

[wparad]

LTS is node 18, so I'm actually suggesting to advance there. This won't break anything because you'll update the required engine in the package.json and this package would bump the major version.

Re the browser crypto: https://developer.mozilla.org/en-US/docs/Web/API/Crypto/subtle, and here is an example of using it in practice: https://github.com/Authress-Engineering/openapi-explorer/blob/release/2.1/src/templates/security-scheme-template.js#L203

[kwikwag]

LTS is node 18, so I'm actually suggesting to advance there. This won't break anything because you'll update the required engine in the package.json and this package would bump the major version.

👍

Re the browser crypto: https://developer.mozilla.org/en-US/docs/Web/API/Crypto/subtle, and here is an example of using it in practice: https://github.com/Authress-Engineering/openapi-explorer/blob/release/2.1/src/templates/security-scheme-template.js#L203

I was talking about Node.js crypto. Browser WebCrypto is available before running. The async behavior is due to dynamic imports in the isNode() case.

[kwikwag]

LTS is node 18, so I'm actually suggesting to advance there. This won't break anything because you'll update the required engine in the package.json and this package would bump the major version.

Another thought: while upping the engine version means old users don't get updates, in a security context, using a maintained version is imperative so maybe it's better to force users to use a maintained engine.

[JamesCullum]

EOL software is not in scope anymore, that's why I changed the test to LTS 18 & 20 already.

Do you know if we even need the polyfill anymore? If browsers, Deno and Node support native crypto, do we actually still need that dependency?

If not, we could get rid of the async parts again and just differentiate between those two cases.

[kwikwag]

AFAIK there is no way to conditionally import a module synchronously.

[wparad]

let crypto;
if (isNodeJs) {
  crypto = require('crypto');
} else {
  crypto = (window.crypto || window.msCrypto).subtle;
}

Where of course this doesn't work because the interfaces aren't the same, which means you might want to create a shim wrapper or find a package that actually handles this.

BUT, can I recommend that we actually split the package and handle the "front end" separately from the "back end". In 99% of implementations, I can't imagine needing both parts, and most of the logic is fairly isolated right, i.e. both sides use some standard properties. Having both in one place, adds additional complexity where it actually isn't valuable, right?

[JamesCullum]

I believe that the used APIs might be similiar enough to at least give it a shot. It's not really important to make it synchronous, I was more focussed about whether we can remove polyfill.

There isn't really any frontend stuff in the package at the moment apart from using native crypto of the JS engine (labelled "browser"), so I'm not sure if we would really gain from such a split.

[kwikwag]

We're discussing separate issues here:

1. Whether or not a polyfill is needed. I think this is beside the point. It's each to include polyfill as a fallback as long as it's not painful for the framework. A polyfill can be required for older browsers, runtimes, when crypto isn't available for some weird reason. One can rely on the consumer to provide the polyfill (e.g. in a browser-targeted bundler, one can specify an implementation for "crypto") or the polyfill can be provided by the package (how it's done until now with @peculiar/webcrypto).
2. How an implementation of WebCrypto is selected. The above example by @wparad won't work AFAIK - the ES module shouldn't allow require(). The addition of msCrypto there I think it also beside the point. The thing is that right now the situation is that the Node.js crypto package and @peculiar/webcrypto package are both loaded regardless of whether WebCrypto exists in the global scope (self for browsers), and this can cause errors on a browser. This makes sense as this library was written with server-side in mind; nonetheless it's useful for PWAs that want to interact with passkeys.

What this pull request addresses is the second issue.

Since submitting this pull request I discovered the following:

1. In my use case (an Angular project) there is a simple way to ignore the packages from being included by the bundler, which is adding the following to package.json. This is a rather simple workaround which I am personally willing to live with and means there is no need to modify fido2-lib.

"browser": {
    "@peculiar/webcrypto": false,
    "crypto": false
}

2. ES modules allow top-level async calls, which means we can use a conditional import with the following code. However, this is a rather modern feature which is still being rolled in, and in my specific use-case, it actually breaks the bundler unless special configuration is added, so I'm faced with a manual configuration change every which way.

// Import webcrypto
let webcrypto = undefined;
if ((typeof self !== "undefined") && ("crypto" in self)) {
	// Always use crypto if available natively (browser / Deno)
	webcrypto = self.crypto;
}
const usePolyfill = async () => {
	const module = await import("@peculiar/webcrypto");
	webcrypto = new module.Crypto();
};
const useNative = async () => {
	// Always use node webcrypto if available ( Node >= 16.0 )
	// This also allows bundlers to resolve crypto with a custom polyfill
	const module = await import("crypto");
	webcrypto = module.webcrypto;
};

if (webcrypto === undefined) {
	try {
		webcrypto = await useNative();
	}
	catch (err) {
		console.warn("Native WebCrypto unavailable (older browser or runtime) - using polyfill", err);
		webcrypto = await usePolyfill();
	}
}
if (webcrypto === undefined) {
	throw new Error("Failed to initialize WebCrypto");
}

I therefore suggest either accepting the pull request as-is, which means forgetting about dealing with the polyfill and top-level awaits; or putting the pull request on hold, and users who find this on the web can consider using a bundler-based workaround (which I assume should work for most users, but who knows).

[JamesCullum]

Hey @kwikwag thanks a lot for your detailed explanation and further research - this definitely helped, especially with you providing a quick fix!

I like the solution and approach, and agree that it would make sense. However if this feature is so new that it breaks bundlers, I would freeze this PR for now and revisit it in the future, just as you proposed. Let me know when we feel confident that the support is there 👍