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

[SignalR] Update some npm deps #41270

Merged
merged 4 commits into from
Jun 2, 2022
Merged

[SignalR] Update some npm deps #41270

merged 4 commits into from
Jun 2, 2022

Conversation

BrennanConroy
Copy link
Member

@BrennanConroy BrennanConroy added the area-signalr Includes: SignalR clients and servers label Apr 20, 2022
@BrennanConroy BrennanConroy requested a review from halter73 as a code owner April 20, 2022 00:36
@MatanBobi
Copy link

@BrennanConroy I believe that the priority for this one is higher now as version 1.x has a vulnerability.. Wdyt?

@BrennanConroy
Copy link
Member Author

When installing the SignalR npm package you will automatically use the latest minor/patch of the EventSource package as we reference it with ^. This means that customers won't be using the vulnerable version.

However, I will update this PR so we explicitly use a safer version.

@imMatt
Copy link

imMatt commented May 30, 2022

Is there a rough ETA on this ticket being completed? As its causing npm audit to fail on any project with SignalR installed.

@nimishagarwal
Copy link

Can we use resolution strategy as
"resolutions": {
"@microsoft/signalr/eventsource": "^2.0.2"
}

@MatanBobi
Copy link

Is there a rough ETA on this ticket being completed? As its causing npm audit to fail on any project with SignalR installed.

@imMatt
The problem is with the github advisory claiming that version 1.1.1 doesn't have the fix (when in fact it does contain the fix), a PR was already raised there. Once it will be merged, npm audit won't fail for this as long as you have 1.1.1 installed.

@Lukas-Kullmann
Copy link

Adding to what @nimishagarwal said, if you use npm (and not yarn), you can use the overrides field in your package.json file to change the package temporarily.

{
  "overrides": {
    "@microsoft/[email protected]": {
      "eventsource": "^2.0.2"
    }
  }
}

Don't forget to delete your package-lock.json file afterwards and run npm install. 😉

@swasun99
Copy link

swasun99 commented May 31, 2022

{
  "overrides": {
    "@microsoft/[email protected]": {
      "eventsource": "^2.0.2"
    }
  }
}

Tried this and the resolutions as well but it didnt update the eventsource version in package lock file but updated the other dependencies.

image

@halter73
Copy link
Member

halter73 commented Jun 1, 2022

##[error](node(0,0): error : (NETCORE_ENGINEERING_TELEMETRY=Test) Setting the NODE_TLS_REJECT_UNAUTHORIZED environment variable to '0' makes TLS connections and HTTPS requests insecure by disabling certificate verification.
 (Use `node --trace-warnings ...` to show where the warning was created)

I'm wondering if rerunning with #41938 will make a difference otherwise we might have to look into changing how we do this:

// Use NODE_TLS_REJECT_UNAUTHORIZED to allow our test cert to be used by the Node tests (NEVER use this environment variable outside of testing)
const p = exec(`"${process.execPath}" "${jestPath}" --config "${configPath}"`, { env: { SERVER_URL: `${httpsUrl};${httpUrl}`, NODE_TLS_REJECT_UNAUTHORIZED: 0 }, timeout: 200000, maxBuffer: 10 * 1024 * 1024 },

Maybe we could set { env: { SERVER_URL: "${httpsUrl};${httpUrl}", NODE_TLS_REJECT_UNAUTHORIZED: 0, NODE_OPTIONS: "--trace-warnings "} first to see if that gives any suggestions.

@halter73
Copy link
Member

halter73 commented Jun 1, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@Lukas-Kullmann
Copy link

@swasun99 If you use version 6.0.5 of @microsoft/signalr, you have to change the version constraint to 6.0.5 in the overrides definition or drop its veesion constraint all together.

{
  "overrides": {
    "@microsoft/signalr": {
      "eventsource": "^2.0.2"
    } 
  } 
} 

@swasun99
Copy link

swasun99 commented Jun 1, 2022

@swasun99 If you use version 6.0.5 of @microsoft/signalr, you have to change the version constraint to 6.0.5 in the overrides definition or drop its veesion constraint all together.

{
  "overrides": {
    "@microsoft/signalr": {
      "eventsource": "^2.0.2"
    } 
  } 
} 

I tried both ways by adding the version constraint as 6.0.5 and also by dropping it, same update in packagelock file as shown above. no change in event source version.

@BrennanConroy BrennanConroy merged commit 4aa4ec3 into main Jun 2, 2022
@BrennanConroy BrennanConroy deleted the brecon/npmup branch June 2, 2022 17:01
@ghost ghost added this to the 7.0-preview6 milestone Jun 2, 2022
@wtgodbe wtgodbe mentioned this pull request Jun 13, 2022
@JanHergenhan
Copy link

The error still shows up. Has this not been fixed with version 6.0.6?

@ghost
Copy link

ghost commented Jun 14, 2022

Hi @JanHergenhan. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

@BrennanConroy
Copy link
Member Author

6.0.6 will pull in eventsource 1.1.2 which does not have the vulnerability.

@JanHergenhan
Copy link

6.0.6 will pull in eventsource 1.1.2 which does not have the vulnerability.

npm audit fix fixed it. Thank you 👍

@ghost
Copy link

ghost commented Jun 14, 2022

Hi @JanHergenhan. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

@github-actions github-actions bot locked and limited conversation to collaborators Dec 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-signalr Includes: SignalR clients and servers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants