ignore `node:crypto` import with webpack in `@simplewebauthn/server` about simplewebauthn HOT 8 CLOSED

I believe I've identified a fix for this. The word webpack in the stacktrace kept jumping out at me. After some searching I was reminded about webpack's webpackIgnore magic comment 🤔

What I'm seeing on my end is that when I change this line in getWebCrypto()...

const _nodeCrypto = await import('node:crypto');

...to this...

const _nodeCrypto = await import(/* webpackIgnore: true */ 'node:crypto');

...then the Next.js code I posted above loads just fine with const runtime = 'edge':

I confirmed that the same code works when setting const runtime = 'nodejs' (the default according to here) so I can't see why this won't be a solution to an issue I ran into locally that sounds like the one that you described.

from simplewebauthn.

Hello @balazsorban44, thank you for starting a dialog here about this. Congratulations on your success with Auth.js. I'm honored you would consider leveraging my library to bring passkey auth to it 🚀

I feel the need to start off by saying that I wouldn't characterize how getWebCrypto() works as polyfilling anything. Historically "polyfilling" meant introducing additional code to fill in gaps when a native runtime does not fully implement the desired API. In contrast, getWebCrypto() attempts to leverage the same native JS API but from two possible locations.

NOTE: I simplified some of the code samples below to keep some Deno-specific code architecture out of the discussion.

It first tries to use globalThis.crypto when it's available, to try and leverage the fact that more modern ESM-first runtimes typically make WebCrypto available at this import path (which as we know includes Node LTS v20+):

  /**
   * Naively attempt to access Crypto as a global object, which popular alternative run-times
   * support.
   */
  const _globalThisCrypto = globalThis.crypto;

  if (_globalThisCrypto) {
    webCrypto = _globalThisCrypto;
    return webCrypto;
  }

If that fails then it tries to import('node:crypto') to try and support runtimes that are either an older version of Node (starting with Node v16) or have implemented some kind of Node compatibility:

  /**
   * `globalThis.crypto` isn't available, so attempt a Node import...
   */
  let _nodeCrypto;
  try {
    // dnt-shim-ignore
    _nodeCrypto = await import('node:crypto');
  } catch (_err) {
    /**
     * Intentionally declaring webcrypto as undefined because we're assuming the Node import
     * failed due to either:
     * - `import()` isn't supported
     * - `node:crypto` is unavailable.
     */
    _nodeCrypto = { webcrypto: undefined };
  }

  if (_nodeCrypto?.webcrypto) {
    webCrypto = _nodeCrypto.webcrypto as Crypto;
    return webCrypto;
  }

And if neither of these succeed then a MissingWebCrypto exception is thrown. For the record, SimpleWebAuthn makes no attempt to try and define its own webcrypto functionality if things get this far.

I say all of this upfront because...

3. users on Node.js 18 could be suggested to add globalThis.crypto ??= await import("node:crypto").webcrypto in an initialization phase of their code.
4. If the user uses this library via a framework, they might already not have to worry about this. In our case for example, Next.js polyfills (with method 3.) globalThis.crypto for us.

...based on my understanding of all the various runtimes and how they expose WebCrypto, the ultimate attempt to await import('node:crypto') can only happen if globalThis.crypto is undefined before this library is used. Might this be some kind of weird race condition with how Next.js ensures that globalThis.crypto is populated in its Edge runtime?

from simplewebauthn.

This causes an issue, not sure if it's the one you're talking about:

import { generateAuthenticationOptions } from '@simplewebauthn/server';

export const runtime = 'edge';

export default async function Home() {
  const options = await generateAuthenticationOptions();
  return (
    <>
      <pre>{JSON.stringify(options, null, 2)}</pre>
    </>
  );
}

@balazsorban44 I'll continue to investigate this, but when you can it would be helpful to get confirmation that this is the issue we're talking about.

from simplewebauthn.

#517 (comment) is a sufficient reproduction.

I believe webpack tries to eagerly evaluate the import, even if the code is not reached. If I put a console.log right before the import, it is indeed not executed. Trying to verify this with someone.

Webpack tries to compile the code, because it can't really know if something is unreachable at runtime or not.

In the meantime, if the fix for us would be to add /* webpackIgnore: true */ (which I also verified), that would be sufficient closure to this issue.

Technically, the more correct solution would be to have a Node.js specific export condition, but that's a stretch for supporting Node.js 18 only.

On polyfilling/not polyfilling. The terminology I used might be incorrect, and I saw that you are not using any third-party lib, but only trying to rely on the runtime-native implementation (kudos for that).

I guess my point in general was to try to avoid even that, and strictly stick with the standard Web APIs in the library, and make it up to the runtime environment to patch up inconsistencies like missing globalThis.crypto.

Your lib would be cleaner, and would put pressure on runtimes not following the standard Web APIs. (which as far as I can tell is only Node.js 18 at this point, so we are in a very good shape 🥳)

from simplewebauthn.

Node.js 16 is still supported, which is EOL. Maybe, just maybe, v10 could drop support for Node.js 18, and drop the whole getWebCrypto.ts and related test file. 🙂

Thanks for the reminder that I need to update the project's minimum Node runtime, you're right that I should drop at least v16 support if not v18 too. I've created #519 for myself in response ✌️

from simplewebauthn.

I've just published @simplewebauthn/[email protected] that should fix this issue. Thanks again for stopping by @balazsorban44 🙏

from simplewebauthn.

Something else that would help is at least a stacktrace, if not a basic list of repro steps I can use to recreate the issue on my end. I looked through nextauthjs/next-auth#9919 and it's not clear to me why the changes in that PR fixed whatever problem initiated the work that became that PR.

from simplewebauthn.

Another note. According to https://github.com/MasterKale/SimpleWebAuthn/tree/master/packages/server#simplewebauthnserver-

Node.js 16 is still supported, which is EOL. Maybe, just maybe, v10 could drop support for Node.js 18, and drop the whole getWebCrypto.ts and related test file. 🙂

from simplewebauthn.