entronad / crypto-es

A cryptography algorithms library
Other
272 stars 30 forks source link

ReferenceError: window is not defined when importing CryptoES with the newer 2.0.0 version #34

Closed nickolasdeluca closed 1 year ago

nickolasdeluca commented 1 year ago

Whenever I import CryptoES and rebuild my API it throws this error:

const crypto = globalThis?.crypto || global?.crypto || window?.crypto || self?.crypto || frames?.[0]?.crypto;
                                                       ^
ReferenceError: window is not defined

Using crypto-es version 2.0.0. Project is a backend API using Fastify, pure JS, no TypeScript involved.

Just tested with previous 1.2.7 version and this error isn't thrown.

Can provide more details if needed, just let me know what you need.

entronad commented 1 year ago

Commonly speaking his should not happen, because a js env should at least has one of globalThis/global/window and has crypto in it.

But in case of special cases like yours, I wrapped this assignment in try/catch(newly published in v2.0.1). This will rollback it to insecure Math.random.

nickolasdeluca commented 1 year ago

What do you mean by "special case"?

Is there something I can do to qualify not to resort back to math.random?

entronad commented 1 year ago

In a standard node or browser env, There should be a globalThis.crypto, so this line will successfully assign it to const crypto with out any error. For example you execute node debug.js in this repo.

But if your env doesn't has any of these variable (like in raw ReactNative), secure random won't assign successfully, Math.random will be used.

So run a console.log(globalThis.crypto) in your own code, make sure your env provides it if you don't want Math.random.

lisonge commented 1 year ago

in chrome70, globalThis is missing, but window.crypto.getRandomValues is still existent

if you use try{}catch{} to wrap your code, you will get void 0 instead of window.crypto

you should use typeof

const crypto =
  (typeof globalThis != 'undefined' ? globalThis : void 0)?.crypto ||
  (typeof global != 'undefined' ? global : void 0)?.crypto ||
  (typeof window != 'undefined' ? window : void 0)?.crypto ||
  (typeof self != 'undefined' ? self : void 0)?.crypto ||
  (typeof frames != 'undefined' ? frames : void 0)?.[0]?.crypto;
entronad commented 1 year ago

@lisonge Thanks, your approach is the best way.

@nickolasdeluca Please try v2.0.2

nickolasdeluca commented 1 year ago

@lisonge Thanks, your approach is the best way.

@nickolasdeluca Please try v2.0.2

Just tried 2.0.2 and it works, but is defaulting back to Math.Random(), as you said.

I'm curious, why didn't this happen with 1.2.7. Also, I migrated from crypto-js to crypto-es when I migrated my project from commonjs to es modules, this issue didn't exist when I was using crypto-js... can you explain why?

Just trying to understand what happened to maybe try to find an answer, because I feel that since Math.Random is not cryptographically safe, It's not safe for me to keep using it...

entronad commented 1 year ago

Because in CryptoES 1.2.7, there is only Math.random, the secure random is added in 2.0.0 recently (after I thought globalThis is popular enough)

Secure random is added to CryptoJS 4.0.0 in 2020, before that it's also only with Math.random. Now in it if you don't have global crypto it will error without rolling back.

The logic of assignment is same in same in CryptoJS and CryptoES, so it shouldn't that you can get a global crypto variable in CryptoJS but ont in CryptoES. If it really happens, please debug with break points to find out where is the problem and share with us.

nickolasdeluca commented 1 year ago

I just debugged the core.js file of the crypto-js package and this is the code that created the crypto object:

// Native crypto import via require (NodeJS)
if (!crypto && typeof require === 'function') {
    try {
        crypto = require('crypto');
    } catch (err) {}
}

I have the 4.1.1 version of the crypto-js package installed. This is the code I wrote to debug reach achieve this: The require for the crypto variable:

const crypto = require("crypto-js");

And my test route:

fastify.get(
  "/test",
  {
    schema: {
      tags: ["test"],
      response: {
        200: {
          type: "object",
          properties: {
            message: { type: "string" },
          },
        },
      },
    },
  },
  async (request, reply) => {
    let aes = crypto.AES.encrypt("Test", configuration.serverCryptoKey);

    reply.send({
      message: aes,
    });
  }
);

Was this what you were looking for?

entronad commented 1 year ago

@nickolasdeluca

globalThis is added to nodejs in v19.

require('crypto') is a CommonJS way to use nodejs' own crypto module. This is used before ES6 standard.

We didn't adopt require('crypto') because we aim CryptoES related only to the language, not to any special implementing env or native module, not even nodejs. As ES language spec has published globalThis, it's envs' duty to implement it, we should not indicate how it is done for any special env.

It's also simple to solve your problem, that is to use a polyfill of globalThis with crypto before importing CryptoES, I suggest this one: https://github.com/zloirock/core-js#ecmascript-globalthis

nickolasdeluca commented 1 year ago

@nickolasdeluca

globalThis is added to nodejs in v19.

require('crypto') is a CommonJS way to use nodejs' own crypto module. This is used before ES6 standard.

We didn't adopt require('crypto') because we aim CryptoES related only to the language, not to any special implementing env or native module, not even nodejs. As ES language spec has published globalThis, it's envs' duty to implement it, we should not indicate how it is done for any special env.

It's also simple to solve your problem, that is to use a polyfill of globalThis with crypto before importing CryptoES, I suggest this one: https://github.com/zloirock/core-js#ecmascript-globalthis

I don't like the idea to depend on yet another package to polyfill the global env variable. I'll keep using v1.2.7 until node 20 turns LTS, but thanks for the help so far.