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

lower the baseline version for system.text.encodings.web #49671

Merged
merged 5 commits into from
Mar 16, 2021
Merged

lower the baseline version for system.text.encodings.web #49671

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
adc7cfb
lower the baseline version
Anipik Mar 15, 2021
2b6c9a9
pin ref assembly version to 5.0.0.0
Anipik Mar 16, 2021
a28f0fc
adding condition for netfx
Anipik Mar 16, 2021
8934eb9
Update src/libraries/pkg/baseline/packageIndex.json
Anipik Mar 16, 2021
9fc65d7
correct the condition
Anipik Mar 16, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/libraries/System.Text.Encodings.Web/ref/System.Text.Encodings.Web.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
<TargetFrameworks>$(NetCoreAppCurrent);netstandard2.0;net461</TargetFrameworks>
<ExcludeCurrentNetCoreAppFromPackage>true</ExcludeCurrentNetCoreAppFromPackage>
<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
<AssemblyVersion>5.0.0.0</AssemblyVersion>
Anipik marked this conversation as resolved.
Show resolved Hide resolved
<!-- Only CLS-compliant members can be abstract -->
<NoWarn>$(NoWarn);CS3011</NoWarn>
<Nullable>enable</Nullable>
Expand Down
2 changes: 1 addition & 1 deletion src/libraries/pkg/baseline/packageIndex.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6455,7 +6455,7 @@
"5.0.0",
"5.0.1"
],
"BaselineVersion": "5.0.1",
"BaselineVersion": "5.0.0",
Copy link
Member

Choose a reason for hiding this comment

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

Curious if we want to do this or not.
This would mean that JSON wouldn't bring the security fix made to STEW. What's your thinking @GrabYourPitchforks?
On the plus side, it would mean that ASP.NET doesn't see that new version from STEW (though they might be able to intentionally downgrade). Is it possible to have ASP.NET test with and without the baseline?

Copy link
Member

Choose a reason for hiding this comment

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

It doesn't sound right to downgrade the baseline and miss the security fix when STEW is transitively referenced.

Copy link
Member

Choose a reason for hiding this comment

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

We discussed with ASP.NET folks over teams, let's undo the change to the baseline: keep it at 5.0.1 here. They'll make some customizations to their build to avoid consuming the versioned S.T.E.W.

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"BaselineVersion": "5.0.0",
"BaselineVersion": "5.0.1",

Copy link
Member

Choose a reason for hiding this comment

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

This would mean that JSON wouldn't bring the security fix made to STEW.

My main concern would be somebody adding a package ref to System.Text.Json, then to S.T.E.W, and as a result missing the fix because I believe NuGet will pull in the existing transitive package reference.

"InboxOn": {
"netcoreapp3.0": "4.0.4.0",
"net5.0": "5.0.0.0"
Expand Down