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

Wali/nuget package vulnerability fix #2068

Merged
merged 4 commits into from
Jul 27, 2023

Conversation

waliMSFT
Copy link
Contributor

updated the version of the nuget package System.Runtime.Uri to 4.3.2

@waliMSFT waliMSFT requested a review from a team as a code owner July 17, 2023 16:33
@@ -18,7 +18,9 @@
<PackageReference Include="Microsoft.VisualStudio.Azure.Containers.Tools.Targets" Version="1.17.0" />
<PackageReference Include="Newtonsoft.Json" Version="13.0.2" />
<PackageReference Include="RunProcessAsTask" Version="1.2.4" />
<PackageReference Include="runtime.unix.System.Private.Uri" Version="4.3.2" />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you mind providing some context as to why this runtime.unix.System.Private.Uri package was also added as a package reference throughout these .csproj files?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding a screenshot or a description of the vulnerability we are solving would be nice.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please have a look on the comment: #2068 (comment)

@@ -57,7 +57,9 @@
<PackageReference Include="Microsoft.CSharp" Version="4.7.0" />
<PackageReference Include="Microsoft.Extensions.Http" Version="7.0.0" />
<PackageReference Include="Newtonsoft.Json" Version="13.0.2" />
<PackageReference Include="runtime.unix.System.Private.Uri" Version="4.3.2" />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I'm not sure if we need to include these package references in Detector.csproj and Detector.Test.csproj since they already reference the Common.csproj project, so these package references should be propagated downstream to these projects, but it's worth double-checking this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please have a look on the comment: #2068 (comment)

@waliMSFT
Copy link
Contributor Author

waliMSFT commented Jul 18, 2023

After discussing with Dan, I have updated the dependency in the projects. Our partners showed concern on the System.Private.Uri version 4.3.0. Xunit has a inner dependency on that package, so updated the System.Private.Uri to 4.3.2 in the Common project as other projects have dependency on it.

@daniv-msft
Copy link
Contributor

After discussing with Dan, I have updated the dependency in the projects. Our partners showed concern on the System.Private.Uri version 4.3.0. Xunit has a inner dependency on that package, so updated the System.Private.Uri to 4.3.2 in the Common project as other projects have dependency on it.

Could you please clarify why the other projects apart from Detector don't show the vulnerability? Given that here we need to take a direct dependency on a package we don't need directly, I'd rather make sure we minimize the impact on our project to where it is absolutely needed.

@cormacpayne
Copy link
Contributor

@daniv-msft @waliMSFT -- to follow-up on the above comment: it may be worth introducing packages.lock.json files for our projects to better understand the dependencies we're taking and where each one is coming from (i.e., direct vs. transitive). This doc outlines the changes we'd make in each .csproj file if we wanted to generate it.

@waliMSFT
Copy link
Contributor Author

waliMSFT commented Jul 20, 2023

After discussing with Dan, I have updated the dependency in the projects. Our partners showed concern on the System.Private.Uri version 4.3.0. Xunit has a inner dependency on that package, so updated the System.Private.Uri to 4.3.2 in the Common project as other projects have dependency on it.

Could you please clarify why the other projects apart from Detector don't show the vulnerability? Given that here we need to take a direct dependency on a package we don't need directly, I'd rather make sure we minimize the impact on our project to where it is absolutely needed.

@daniv-msft Among all the projects we have inside Oryx/src directory, only the Detector project uses the Xunit package. As few projects have dependency on Detector project, so the root dependency on the Xunit always comes from the Detector project.
In addition, there are projects do not have dependency on the Detector project. for example: BuildServer, BuildScriptGenerator.Common, etc. So, they don't possess any vulnerability.

@daniv-msft
Copy link
Contributor

After discussing with Dan, I have updated the dependency in the projects. Our partners showed concern on the System.Private.Uri version 4.3.0. Xunit has a inner dependency on that package, so updated the System.Private.Uri to 4.3.2 in the Common project as other projects have dependency on it.

Could you please clarify why the other projects apart from Detector don't show the vulnerability? Given that here we need to take a direct dependency on a package we don't need directly, I'd rather make sure we minimize the impact on our project to where it is absolutely needed.

@daniv-msft Among all the projects we have inside Oryx/src directory, only the Detector project uses the Xunit package. As few projects have dependency on Detector project, so the root dependency on the Xunit always comes from the Detector project. In addition, there are projects do not have dependency on the Detector project. for example: BuildServer, BuildScriptGenerator.Common, etc. So, they don't possess any vulnerability.

Thanks for clarifying! In that case, please consider moving the System.Private.Uri nuget reference to Detector.csproj only so that we keep the workaround we need to create here as minimal as possible.

@waliMSFT
Copy link
Contributor Author

Thanks for clarifying! In that case, please consider moving the System.Private.Uri nuget reference to Detector.csproj only so that we keep the workaround we need to create here as minimal as possible.
@daniv-msft I have made the change as you said. I agree with you to make the change as minimum as possible. However, I was wondering the vulnerability on the system.private.uri version 4.3.0 will be there if we import the xUnit package in any other project. It will create the same vulnerability. As all the projects use the Common project, previously I thought to add the updated system.private.uri version 4.3.2 there.

@waliMSFT waliMSFT merged commit e001adb into main Jul 27, 2023
1 check passed
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 this pull request may close these issues.

4 participants