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

Downgrade STJ to 8.0.4 #109818

Merged
merged 2 commits into from
Nov 14, 2024
Merged

Downgrade STJ to 8.0.4 #109818

merged 2 commits into from
Nov 14, 2024

Conversation

ViktorHofer
Copy link
Member

@ViktorHofer ViktorHofer commented Nov 14, 2024

We can't yet rely on a STJ/8.0.5 package as the desktop msbuild that is used in our CI images doesn't have binding redirects for it yet.

  • Suppress the src/tasks vulnerability warning as STJ isn't used at runtime for msbuild tasks.
  • Suppress the ones about HostModel's usage. We want to use a live STJ instead but that needs a separate PR (more work).

We can't yet rely on a STJ/8.0.5 package as desktop msbuild doesn't have binding redirects for it yet.

- Suppress the src/tasks vulnerability warning as STJ isn't used at runtime for msbuild tasks.
- Suppress the ones about HostModel's usage. We want to use a live STJ instead but that needs a separate PR (more work).
@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Nov 14, 2024
@ViktorHofer
Copy link
Member Author

cc @am11

Copy link
Contributor

Tagging subscribers to this area: @dotnet/runtime-infrastructure
See info in area-owners.md if you want to be subscribed.

@am11
Copy link
Member

am11 commented Nov 14, 2024

Is there any ETA when VS will add that redirect? Community members were concerned about a month ago dotnet/diagnostics#4988 (comment). Perhaps the next time deprecation should be issued after VS has the redirects or perhaps it's the indication that we need better handling for these kind of CVE mitigations.

@am11 am11 removed the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Nov 14, 2024
@ViktorHofer
Copy link
Member Author

Is there any ETA when VS will add that redirect?

The .NET Framework msbuild that ships inside VS already has the app.config redirect and that's part of the VS 17.12 stable release (which released two days ago). But our CI images also lag behind a couple weeks, usually a month.

Perhaps the next time deprecation should be issued after VS has the redirects or perhaps it's the indication that we need better handling for these kind of CVE mitigations.

We need to have a better mechanism here, agreed. Ie the System.Text.Json packageref under src/tasks should never be flagged as it's only use as a contract and not as a runtime implementation (if it's used as such then that's a bug in those msbuild tasks). We have one promising feature planned with the NuGet team called "Supplied by framework" which will help for .NETCoreApp case but not for .NET Framework as STJ isn't inbox there.

There are different ways to tackle this:

  • MSBuild is considering building a targeting pack that would give you those references so that a PackageRef to STJ for contract-only purposes isn't necessary.
  • MSBuild might consider adding a .NETCoreApp task host so that we can move many of our .NET Framework tasks to .NETCoreApp only.
  • The above two only help with MSBuild but the same applies to source generators or any other plugin model as well. So we need a better mechanism to represent these "contract-only" needs so that vulnerable implementation assemblies that aren't used at runtime don't even get downloaded.

This is top of mind for many of us...

@am11
Copy link
Member

am11 commented Nov 14, 2024

Thanks for the details.

MSBuild might consider adding a .NETCoreApp task host so that we can move many of our .NET Framework tasks to .NETCoreApp only.

This sounds like a best solution, just upvoted dotnet/msbuild#4834 😉

@ViktorHofer
Copy link
Member Author

ViktorHofer commented Nov 14, 2024

/ba-g don't wait on the long running wasm legs... unblock dotnet/sdk broken builds

@ViktorHofer ViktorHofer merged commit 31e28ad into main Nov 14, 2024
152 of 156 checks passed
@ViktorHofer ViktorHofer deleted the STJ805Downgrade branch November 14, 2024 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants