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.

Sign up for GitHub

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

Jump to bottom

dead code from instanceOf.ts is not tree shaked for browser due globalThis.process #3925

Closed
dimaMachina opened this issue Jun 23, 2023 · 4 comments
Closed

dead code from instanceOf.ts is not tree shaked for browser due globalThis.process #3925

dimaMachina opened this issue Jun 23, 2023 · 4 comments

Comments

@dimaMachina
Copy link

after #3923 and #3501 dead code from instanceOf is no longer tree shaked for browsers

@IvanGoncharov wrote

Bundlers incorrectly replace (probably RegExp) process.env.NODE_ENV with "development" resulting in:

(_globalThis$process = globalThis.process) === null ||
_globalThis$process === void 0
  ? void 0
  : _globalThis$"development";

but this happens only when using globalThis.process.env.NODE_ENV
This was referenced Jun 24, 2023
Closed
Closed
@oidualc oidualc mentioned this issue Jun 24, 2023
Closed
4 tasks
@wreality wreality mentioned this issue Jul 1, 2023
Merged
@bartektelec bartektelec mentioned this issue Jul 10, 2023
Closed
4 tasks
@vdineva
Copy link

vdineva commented Sep 28, 2023 •
edited
Loading

Any update on this issue? This is a blocker for us to upgrade to newer versions of graphql and there's an advisory for the version we're currently on: GHSA-9pv7-vfvm-6vr7

@radesai21
Copy link

Can we get an update on this? This is a security warning caused by this GHSA-9pv7-vfvm-6vr7

@mattcosta7 mattcosta7 mentioned this issue Nov 16, 2023
Closed
4 tasks
@JoviDeCroock JoviDeCroock mentioned this issue Feb 14, 2024
Merged
@JoviDeCroock
Copy link
Member

JoviDeCroock commented May 11, 2024 •
edited
Loading

For now you can use something like rollup-plugin-replace or the equivalent in your own bundler to do the following

{
  `globalThis.process`: JSON.stringify(true),
  `globalThis.process.env.NODE_ENV`: JSON.stringify("production"),
}

after #4022 you'll be able to look it up in the docs, see the files changed you can pick and choose your bundler.

saihaj pushed a commit that referenced this issue May 29, 2024
@JoviDeCroock
fix: remove globalThis check and align with what bundlers can accept (
0d12b06 
#4022)

As surfaced in
[Discord](https://discord.com/channels/625400653321076807/862957336082645006/1206980831915282532)
this currently is a breaking change in the 16.x.x release line which is
preventing folks from upgrading towards a security fix. This PR should
result in a patch release on the 16 release line.

This change was originally introduced to support CFW and browser
environments which should still be supported with the `typeof` check CC
@n1ru4l

This also adds a check whether `.env` is present as in the DOM using
`id="process"` defines that as a global which we don't want to access on
accident. as shown in #4017

Bundles also target `process.env.NODE_ENV` specifically which fails when
it replaces `globalThis.process.env.NODE_ENV` as this becomes
`globalThis."production"` which is invalid syntax.

Fixes #3978
Fixes #3918
Fixes #3928
Fixes #3758
Fixes #3934

This purposefully does not account for
#3925 as we can't address
this without breaking CF/plain browsers so the small byte-size increase
will be expected for bundled browser environments. As a middle ground we
did optimise the performance here. We can revisit this for v17.

Most bundlers will be able to tree-shake this with a little help, in
#4075 (comment)
you can find a conclusion with a repo where we discuss a few.

- Next.JS by default replaces
[`process.env.NODE_ENV`](https://github.com/vercel/next.js/blob/b0ab0fe85fe8c93792051b058e060724ff373cc2/packages/next/webpack.config.js#L182)
you can add `typeof process` linearly
- Vite allows you to specify
[`config.define`](https://vitejs.dev/config/shared-options.html#define)
- ESBuild by default will replace `process.env.NODE_ENV` but does not
support replacing `typeof process`
- Rollup has a plugin for this
https://www.npmjs.com/package/@rollup/plugin-replace

Supersedes #4021
Supersedes #4019
Supersedes #3927

> This now also adds a documentation page on how to remove all of these
@JoviDeCroock
Copy link
Member

Hey @dimaMachina #4022 is live in the latest 16.x.x line, does this solve your issue?

@dimaMachina dimaMachina closed this as not planned Won't fix, can't repro, duplicate, stale Jul 12, 2024
yaacovCR pushed a commit to yaacovCR/graphql-js that referenced this issue Aug 19, 2024
@JoviDeCroock @yaacovCR
fix: remove globalThis check and align with what bundlers can accept (
0f70178 
graphql#4022)

As surfaced in
[Discord](https://discord.com/channels/625400653321076807/862957336082645006/1206980831915282532)
this currently is a breaking change in the 16.x.x release line which is
preventing folks from upgrading towards a security fix. This PR should
result in a patch release on the 16 release line.

This change was originally introduced to support CFW and browser
environments which should still be supported with the `typeof` check CC
@n1ru4l

This also adds a check whether `.env` is present as in the DOM using
`id="process"` defines that as a global which we don't want to access on
accident. as shown in graphql#4017

Bundles also target `process.env.NODE_ENV` specifically which fails when
it replaces `globalThis.process.env.NODE_ENV` as this becomes
`globalThis."production"` which is invalid syntax.

Fixes graphql#3978
Fixes graphql#3918
Fixes graphql#3928
Fixes graphql#3758
Fixes graphql#3934

This purposefully does not account for
graphql#3925 as we can't address
this without breaking CF/plain browsers so the small byte-size increase
will be expected for bundled browser environments. As a middle ground we
did optimise the performance here. We can revisit this for v17.

Most bundlers will be able to tree-shake this with a little help, in
graphql#4075 (comment)
you can find a conclusion with a repo where we discuss a few.

- Next.JS by default replaces
[`process.env.NODE_ENV`](https://github.com/vercel/next.js/blob/b0ab0fe85fe8c93792051b058e060724ff373cc2/packages/next/webpack.config.js#L182)
you can add `typeof process` linearly
- Vite allows you to specify
[`config.define`](https://vitejs.dev/config/shared-options.html#define)
- ESBuild by default will replace `process.env.NODE_ENV` but does not
support replacing `typeof process`
- Rollup has a plugin for this
https://www.npmjs.com/package/@rollup/plugin-replace

Supersedes graphql#4021
Supersedes graphql#4019
Supersedes graphql#3927

> This now also adds a documentation page on how to remove all of these
yaacovCR pushed a commit to yaacovCR/graphql-js that referenced this issue Aug 19, 2024
@JoviDeCroock @yaacovCR
fix: remove globalThis check and align with what bundlers can accept (
4db691e 
graphql#4022)

As surfaced in
[Discord](https://discord.com/channels/625400653321076807/862957336082645006/1206980831915282532)
this currently is a breaking change in the 16.x.x release line which is
preventing folks from upgrading towards a security fix. This PR should
result in a patch release on the 16 release line.

This change was originally introduced to support CFW and browser
environments which should still be supported with the `typeof` check CC
@n1ru4l

This also adds a check whether `.env` is present as in the DOM using
`id="process"` defines that as a global which we don't want to access on
accident. as shown in graphql#4017

Bundles also target `process.env.NODE_ENV` specifically which fails when
it replaces `globalThis.process.env.NODE_ENV` as this becomes
`globalThis."production"` which is invalid syntax.

Fixes graphql#3978
Fixes graphql#3918
Fixes graphql#3928
Fixes graphql#3758
Fixes graphql#3934

This purposefully does not account for
graphql#3925 as we can't address
this without breaking CF/plain browsers so the small byte-size increase
will be expected for bundled browser environments. As a middle ground we
did optimise the performance here. We can revisit this for v17.

Most bundlers will be able to tree-shake this with a little help, in
graphql#4075 (comment)
you can find a conclusion with a repo where we discuss a few.

- Next.JS by default replaces
[`process.env.NODE_ENV`](https://github.com/vercel/next.js/blob/b0ab0fe85fe8c93792051b058e060724ff373cc2/packages/next/webpack.config.js#L182)
you can add `typeof process` linearly
- Vite allows you to specify
[`config.define`](https://vitejs.dev/config/shared-options.html#define)
- ESBuild by default will replace `process.env.NODE_ENV` but does not
support replacing `typeof process`
- Rollup has a plugin for this
https://www.npmjs.com/package/@rollup/plugin-replace

Supersedes graphql#4021
Supersedes graphql#4019
Supersedes graphql#3927

> This now also adds a documentation page on how to remove all of these
yaacovCR pushed a commit that referenced this issue Sep 6, 2024
@JoviDeCroock @yaacovCR
fix: remove globalThis check and align with what bundlers can accept (
787f799 
#4022)

As surfaced in
[Discord](https://discord.com/channels/625400653321076807/862957336082645006/1206980831915282532)
this currently is a breaking change in the 16.x.x release line which is
preventing folks from upgrading towards a security fix. This PR should
result in a patch release on the 16 release line.

This change was originally introduced to support CFW and browser
environments which should still be supported with the `typeof` check CC
@n1ru4l

This also adds a check whether `.env` is present as in the DOM using
`id="process"` defines that as a global which we don't want to access on
accident. as shown in #4017

Bundles also target `process.env.NODE_ENV` specifically which fails when
it replaces `globalThis.process.env.NODE_ENV` as this becomes
`globalThis."production"` which is invalid syntax.

Fixes #3978
Fixes #3918
Fixes #3928
Fixes #3758
Fixes #3934

This purposefully does not account for
#3925 as we can't address
this without breaking CF/plain browsers so the small byte-size increase
will be expected for bundled browser environments. As a middle ground we
did optimise the performance here. We can revisit this for v17.

Most bundlers will be able to tree-shake this with a little help, in
#4075 (comment)
you can find a conclusion with a repo where we discuss a few.

- Next.JS by default replaces
[`process.env.NODE_ENV`](https://github.com/vercel/next.js/blob/b0ab0fe85fe8c93792051b058e060724ff373cc2/packages/next/webpack.config.js#L182)
you can add `typeof process` linearly
- Vite allows you to specify
[`config.define`](https://vitejs.dev/config/shared-options.html#define)
- ESBuild by default will replace `process.env.NODE_ENV` but does not
support replacing `typeof process`
- Rollup has a plugin for this
https://www.npmjs.com/package/@rollup/plugin-replace

Supersedes #4021
Supersedes #4019
Supersedes #3927

> This now also adds a documentation page on how to remove all of these
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@vdineva @dimaMachina @JoviDeCroock @radesai21 and others