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

chore(remix-dev): update proxy-agent dependency #6862

Closed
wants to merge 3 commits into from

Conversation

roughee
Copy link
Contributor

@roughee roughee commented Jul 18, 2023

Fixes #6833

@remix-cla-bot
Copy link
Contributor

remix-cla-bot bot commented Jul 18, 2023

Hi @roughee,

Welcome, and thank you for contributing to Remix!

Before we consider your pull request, we ask that you sign our Contributor License Agreement (CLA). We require this only once.

You may review the CLA and sign it by adding your name to contributors.yml.

Once the CLA is signed, the CLA Signed label will be added to the pull request.

If you have already signed the CLA and received this response in error, or if you have any questions, please contact us at hello@remix.run.

Thanks!

- The Remix team

@changeset-bot
Copy link

changeset-bot bot commented Jul 18, 2023

⚠️ No Changeset found

Latest commit: b923517

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@remix-cla-bot
Copy link
Contributor

remix-cla-bot bot commented Jul 18, 2023

Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳

@brophdawg11 brophdawg11 added the dependencies Pull requests that update a dependency file label Jul 18, 2023
@lukehsiao
Copy link

This fixes a critical security vuln for everyone that uses Remix. Do we have any estimates when this will release once it merges?

@rickbatka
Copy link

Any maintainers want to weigh in on this? npm audit is now reporting critical vulnerabilities for everyone using Remix.

FWIW, I tried using a "resolutions" override in my Remix app to force proxy-agent to 6.3.0, and it breaks remix build:

  "resolutions": {
    "proxy-agent" : "^6.3.0",
}

Then ran:

yarn install
yarn build

Here is the output from remix build:

/workspaces/platform/web/node_modules/@remix-run/dev/dist/cli/create.js:67
const defaultAgent = new ProxyAgent__default["default"]();
                     ^

TypeError: ProxyAgent__default.default is not a constructor
    at Object.<anonymous> (/workspaces/platform/web/node_modules/@remix-run/dev/dist/cli/create.js:67:22)
    at Module._compile (node:internal/modules/cjs/loader:1255:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1309:10)
    at Module.load (node:internal/modules/cjs/loader:1113:32)
    at Module._load (node:internal/modules/cjs/loader:960:12)
    at Module.require (node:internal/modules/cjs/loader:1137:19)
    at require (node:internal/modules/helpers:121:18)
    at Object.<anonymous> (/workspaces/platform/web/node_modules/@remix-run/dev/dist/cli/commands.js:56:16)
    at Module._compile (node:internal/modules/cjs/loader:1255:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1309:10)
    at Module.load (node:internal/modules/cjs/loader:1113:32)
    at Module._load (node:internal/modules/cjs/loader:960:12)
    at Module.require (node:internal/modules/cjs/loader:1137:19)
    at require (node:internal/modules/helpers:121:18)
    at Object.<anonymous> (/workspaces/platform/web/node_modules/@remix-run/dev/dist/cli/run.js:22:16)
    at Module._compile (node:internal/modules/cjs/loader:1255:14)

Node.js v20.2.0
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

Rebuilding...
ERROR: "dev:remix" exited with 1.
error Command failed with exit code 1.

I worry that this PR might not go far enough to fix the issue - it might clear the vulnerability warning, while also breaking the remix compiler.

But I'm just taking a guess.

@roughee
Copy link
Contributor Author

roughee commented Jul 19, 2023

@rickbatka

you are welcome to try yourself on the remix repo it builds just fine.

git checkout dev
yarn install
npx playwright install
yarn build
yarn test

image

then

cd packages
cd remix-dev
yarn upgrade proxy-agent@^6.3.0
cd ..
cd ..
yarn install
yarn build
yarn test

image

@roughee
Copy link
Contributor Author

roughee commented Jul 20, 2023

@machour fixed the import it should resolve the CI build.

@machour machour linked an issue Jul 20, 2023 that may be closed by this pull request
1 task
@MichaelDeBoey MichaelDeBoey changed the title upgrade proxy agent to 6.3.0 chore(remix-dev): update proxy-agent dependency Jul 20, 2023
@machour
Copy link
Collaborator

machour commented Jul 21, 2023

@roughee Thank you so much for your work on this PR, but the security issue seems to have been fixed as part as a larger PR (#6887)

@brophdawg11 Do you think we will cut 1.19.1 to address the issue, or will this have to wait for the incoming major bump?

I leave it up to you to close this PR.

@averypelle
Copy link

averypelle commented Jul 21, 2023

@machour @brophdawg11 I would advocate in favor of a patch release containing just this change.

If the larger PR is part of a breaking change, many codebases will be exposed to this vulnerability for a much longer time, i.e. until they figure out how to migrate to the latest major version.

Importantly, vercel remains exposed to this critical security vulnerability via its dependency on this project vercel/vercel#10222 . A patch version would allow for a much quicker resolution for that repo and the many codebases that use it.

@rickbatka
Copy link

@machour @brophdawg11 I would advocate in favor of a patch release containing just this change.

If the larger PR is part of a breaking change, many codebases will be exposed to this vulnerability for a much longer time, i.e. until they figure out how to migrate to the latest major version.

Importantly, vercel remains exposed to this critical security vulnerability via its dependency on this project vercel/vercel#10222 . A patch version would allow for a much quicker resolution for that repo and the many codebases that use it.

I agree, my codebase is one of those affected (we are on 1.18). This really should go out as a patch release for all supported versions of Remix. The vulnerability is quite serious (maintainer pulled his package and made various statements online).

@quinn
Copy link

quinn commented Jul 26, 2023

I think this is an appropriate patch release on the 1.19 series given the critical vulnerability. I'd like to get this updated and clear my security alerts :)

@brophdawg11
Copy link
Contributor

We've got another fix we'd like to put out in a 1.19.2 so I cherry-picked these into #7027 to include as well

@brophdawg11 brophdawg11 closed this Aug 1, 2023
@brophdawg11
Copy link
Contributor

Also cross-posting this comment for visibility. This is not a vulnerability with your runtime app. This is something only used by the create-remix CLI tool, which is currently part of @remix-run/dev which is why you're seeing the npm warning. Your production application is not using the create-remix CLI, and thus is not impacted. It's just a symptom of the design of npm audit (tl;dr;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed dependencies Pull requests that update a dependency file package:dev
Projects
None yet
Development

Successfully merging this pull request may close these issues.

@remix-run/dev critical security vulnerabilities from dependencies
9 participants