Skip to content

Conversation

eerhardt
Copy link
Member

@eerhardt eerhardt commented Apr 6, 2023

Related: #84389.

@eerhardt eerhardt requested a review from krwq April 6, 2023 16:38
@ghost ghost assigned eerhardt Apr 6, 2023
@ghost
Copy link

ghost commented Apr 6, 2023

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghost
Copy link

ghost commented Apr 6, 2023

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

Issue Details

null

Author: eerhardt
Assignees: -
Labels:

area-System.Xml

Milestone: -

@eerhardt
Copy link
Member Author

eerhardt commented Apr 6, 2023

@vitek-karas @agocke @jkotas @MichalStrehovsky -

This is a really unfortunate case that I'm not sure how to handle. It started with marking XslCompiledTransform as RequiresDynamicCode. That meant that System.Security.Cryptography.Xml needed to be marked as RequiresDynamicCode in the off-chance that XSLTs are used in the XML payload. Which then infected Microsoft.Extensions.Configuration.Xml in the off-chance that the XML files were using EncryptedXml. At each level up the chances that the caller is using XSLTs are smaller and smaller.

The good thing is that Microsoft.Extensions.Configuration.Xml isn't used in many mainline scenarios, but it still concerns me having to mark these APIs as unsafe, when 95+% of callers aren't going to have problems. It might make people numb to these warnings if they are for rarely used capabilities.

Do we have a strategy on when warnings can be suppressed in cases like this? Or is it a strict "it is guaranteed to work, or will warn" policy?

@jkotas
Copy link
Member

jkotas commented Apr 6, 2023

The usual strategy we have developed for cases like this is to introduce a config switch and default it to false when PublishTrimmed / PublishAot is on. I think Microsoft.Extensions.Configuration.Xml would be the most meaningful place for such config switch.

@eerhardt
Copy link
Member Author

eerhardt commented Apr 7, 2023

Thanks. For this case I'll wait for customer feedback before adding a feature switch in this area. The same APIs are getting annotated with RequiresUnreferencedCode in #84468. The switch will have to disable using EncryptXml altogether. It may not be worth adding the switch if no one is using Xml Configuration in trimmed/AOT'd apps, or if they are satisfied with suppressing the warning. At least they will see up front that using encrypted XML isn't guaranteed to work.

@eerhardt eerhardt merged commit 5117e1f into dotnet:main Apr 12, 2023
@eerhardt eerhardt deleted the AnnotateXslt branch April 12, 2023 13:13
akoeplinger added a commit to akoeplinger/runtime that referenced this pull request Apr 14, 2023
…y.Xml on iOS/tvOS

They started failing after dotnet#84426 because we now throw an exception when xslt is used.
That exception gets hidden in CryptoHelpers.CreateFromName() though since you only see "Unknown transform has been encountered" so it is hard to figure out what happened. Added a more specific error message that mentions the algorithm name.
@ghost ghost locked as resolved and limited conversation to collaborators May 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants