Open kwikwag opened 7 months ago
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.:exclamation: 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 :warning: |
:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
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.
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?
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
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.
:+1:
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.
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.
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.
AFAIK there is no way to conditionally import a module synchronously.
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?
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.
We're discussing separate issues here:
@peculiar/webcrypto
). 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:
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
}
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).
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 👍
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 beawait
-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.