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

jsdom v23: atob (and btoa) in Window.js are recursive and always fail #3628

Closed
DoodleBobBuffPants opened this issue Nov 28, 2023 · 9 comments

Comments

@DoodleBobBuffPants
Copy link

Basic info:

  • Node.js version: 21.0.0
  • jsdom version: 23.0.0

Minimal reproduction case

See my comment on the offending PR: #3625 (comment)

Raising this issue mainly to make sure the comment gets looked at as the PR is already merged

Ultimately, it looks atob and btoa are now recursive and always fail with a RangeError: Maximum call stack size exceeded when called. Verified by extracting the two functions and running them

jordanshatford added a commit to jordanshatford/clip-queue that referenced this issue Nov 28, 2023
see: jsdom/jsdom#3628

Signed-off-by: Jordan Shatford <jordanshatford@live.com>
jordanshatford added a commit to jordanshatford/clip-queue that referenced this issue Nov 28, 2023
see: jsdom/jsdom#3628

Signed-off-by: Jordan Shatford <jordanshatford@live.com>
@BacLuc
Copy link

BacLuc commented Nov 28, 2023

I am also having this issue here:
ecamp/ecamp3#4168
In the build here: https://github.com/ecamp/ecamp3/actions/runs/7020186673/job/19099326775?pr=4168

This change solved the issue locally for me: BacLuc@1173cfb

@domenic
Copy link
Member

domenic commented Nov 29, 2023

Sorry about this. I can't believe we had no test coverage that would catch this; I assumed the refactoring I did would not cause an issue.

I'll work on this and #3627 as soon as I can, but finding time in my nights has been difficult and it might take until the weekend.

@frontendphil
Copy link

It is always the one test that's missing. The fact that you need a free night to work on this is terrible! Thanks for putting so much work into a lib that fuels so many test runners.

@DoodleBobBuffPants
Copy link
Author

What @frontendphil said - I'm sure there are many who have been bitten by that missing test, but there are also many who appreciate your efforts, and at the end of the day we've learned something and acquired another story to tell 🙂

@domenic
Copy link
Member

domenic commented Nov 30, 2023

I'm unable to reproduce this issue. Test case:

"use strict";
const { JSDOM } = require("jsdom");

const dom1 = new JSDOM();
console.log(dom1.window.btoa("Hello"));
console.log(dom1.window.atob("SGVsbG8="));

const dom2 = new JSDOM(``, { runScripts: "outside-only" });
console.log(dom2.window.btoa("Hello"));
console.log(dom2.window.atob("SGVsbG8="));

const dom3 = new JSDOM(``, { runScripts: "dangerously" });
console.log(dom3.window.btoa("Hello"));
console.log(dom3.window.atob("SGVsbG8="));

It seems like this must be caused by something else in the software stack that people are using, so it's best to debug it in those tools that are built on top of jsdom. Whatever they are doing is messing with how name resolution works inside the definition of our Window class, I guess.

I'll close this until someone is able to come up with a reproduction case that only involves jsdom, and not any layers on top.

@domenic domenic closed this as completed Nov 30, 2023
@sebamarynissen
Copy link
Contributor

sebamarynissen commented Dec 9, 2023

@domenic I'm having the same issue as well, and I'm pretty sure it's related to using jsdom-global, or at least code that's patching stuff on Node's globalThis. That's because the code in Window.js

this.atob = function (str) {
  try {
    return atob(str);
  } catch (e) {  }
};

is equivalent to

this.atob = function (str) {
  try {
    return globalThis.atob(str);
  } catch (e) {  }
};

so when atob is patched onto the global object, you end with an infinite loop. The fact that the error is rethrown with DOMException.create() hides the fact that it's actually a RangeError: Maximum call stack size exceeded.

Minimum reproduction case is

const { JSDOM } = require('jsdom');
const dom = new JSDOM('');
globalThis.atob = dom.window.atob;
globalThis.btoa = dom.window.btoa;

console.log(dom.window.btoa('Hello'));
console.log(dom.window.atob('SGVsbG8='));

I think this needs to be fixed on the side of jsdom though. It's as easy as adding

const { atob, btoa } = require("node:buffer");

at the top of the file. This ensures both atob and btoa are always the Node implementations and not whatever is present on the global object at runtime.

Not sure about the proper way to write a test case for this though as we want to avoid polluting global for all other tests obviously.

I will file a PR to fix this.

sebamarynissen added a commit to sebamarynissen/jsdom that referenced this issue Dec 9, 2023
@sebamarynissen
Copy link
Contributor

sebamarynissen commented Dec 9, 2023

Just wanted to add the workaround I used in my project, perhaps that it can be useful for someone.

I'm using jsdom to test a Vue.js project. I was relying on jsdom-global to setup the browser environment for my tests, which was indeed patching atob on the global object. I simply threw it out and replaced it with the snippet below that manually sets up the browser environment and only patches what's absolutely needed for my tests to run instead of just blindly overriding stuff that Node already implements. Feels way better, especially as Node supports a lot of browser apis natively nowadays.

import { JSDOM } from 'jsdom';
const dom = new JSDOM('', {
  url: 'https://www.example.com',
});

const keys = [
  'document',
  'navigator',
  'window',
  'SVGElement',
  'Element',
];
Object.assign(globalThis, Object.fromEntries(
  keys.map(key => [key, dom.window[key]]),
));

sebamarynissen added a commit to sebamarynissen/jsdom that referenced this issue Dec 9, 2023
Instead of borrowing atob and btoa from `globalThis` which can still go wrong if the functions were overriden there as well, it should now be fixed by always explicitly requiring atob and btoa from `node:buffer`
@domenic
Copy link
Member

domenic commented Dec 11, 2023

@domenic I'm having the same issue as well, and I'm pretty sure it's related to using jsdom-global, or at least code that's patching stuff on Node's globalThis.

This makes sense. Note that jsdom-global is a third-party package, and is explicitly doing what you are advised not to do when using jsdom, so it is not supported and we will not accept pull requests to work around its problems. If you would like to fix this in jsdom-global, please do so by sending pull requests to that repository, not to this one.

@sebamarynissen
Copy link
Contributor

@domenic I understand, and totally agree. I just wanted to point out that it's not only jsdom-global that's doing it, but also vitest (see https://github.com/vitest-dev/vitest/blob/main/packages/vitest/src/integrations/env/jsdom.ts#L140), although I think they restore the original global after running the test so that it doesn't pollute the global outside the test.

Nevertheless, I still think there's an argument to add

const { atob, btoa } = require("node:buffer");

in the Window.js file as I did in #3637 regardless of all of this, as I personally think it's better to not rely on Node.js globals, but rather explicitly import them if possible.

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

Successfully merging a pull request may close this issue.

5 participants