Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
nickolasdeluca opened this issue Jul 12, 2023 · 10 comments

Comments

@nickolasdeluca
Copy link

nickolasdeluca commented Jul 12, 2023

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
Copy link
Owner

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
Copy link
Author

What do you mean by "special case"?

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

@entronad
Copy link
Owner

entronad commented Jul 12, 2023

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
Copy link

lisonge commented Jul 12, 2023

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
Copy link
Owner

@lisonge
Thanks, your approach is the best way.

@nickolasdeluca
Please try v2.0.2

@nickolasdeluca
Copy link
Author

@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
Copy link
Owner

entronad commented Jul 12, 2023

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
Copy link
Author

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
Copy link
Owner

entronad commented Jul 13, 2023

@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
Copy link
Author

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants