dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.38k stars 4.75k forks source link

Remove unused code in Xslt that parses Scripts in xsls as the Transform operation is not supported in Core. #50308

Open joperezr opened 3 years ago

joperezr commented 3 years ago

There are two main parts to performing an xsl transform. First we compile the xsl stylesheet and emit some IL with the instructions of what needs to happen during the transformation, and the second part is actually performing the transformation of a passed in Xml using those compiled IL instructions. Stylesheets may have Scripts inside which could be using different languages to define functions to be performed with the input.

When porting both of our Xslt engines (XslTransform and XslCompileTransform) from .NET Framework to core, we added a lot of code that performs the first part of the transformation (compilation into IL instructions) for Scripts, but we decided to not support Scripts in Core so when you try to then perform the transformation we will throw PlatformNotSupported exception. This means that running XsltCompileTransform.Load(someStyleSheetWithScripts) will not throw (since this only does the compiling) but when doing XsltCompileTransform.Transform(someInputXml) you will actually get the exception.

This issue is to track all of those code blocks we have for compiling Scripts, given that we effectively don't support them so we should throw PNSE whenever somebody tries to Load a stylesheet containing Scripts.

ghost commented 3 years ago

Tagging subscribers to this area: @buyaa-n, @krwq See info in area-owners.md if you want to be subscribed.

Issue Details
There are two main parts to performing an xsl transform. First we compile the xsl stylesheet and emit some IL with the instructions of what needs to happen during the transformation, and the second part is actually performing the transformation of a passed in Xml using those compiled IL instructions. Stylesheets may have Scripts inside which could be using different languages to define functions to be performed with the input. When porting both of our Xslt engines (XslTransform and XslCompileTransform) from .NET Framework to core, we added a lot of code that performs the first part of the transformation (compilation into IL instructions) for Scripts, but we decided to not support Scripts in Core so when you try to then perform the transformation we will throw PlatformNotSupported exception. This means that running `XsltCompileTransform.Load(someStyleSheetWithScripts)` will not throw (since this only does the compiling) but when doing `XsltCompileTransform.Transform(someInputXml)` you will actually get the exception. This issue is to track all of those code blocks we have for compiling Scripts, given that we effectively don't support them so we should throw PNSE whenever somebody tries to Load a stylesheet containing Scripts.
Author: joperezr
Assignees: -
Labels: `area-System.Xml`, `enhancement`
Milestone: 6.0.0
ghost commented 3 years ago

Tagging subscribers to 'size-reduction': @eerhardt, @SamMonoRT, @marek-safar See info in area-owners.md if you want to be subscribed.

Issue Details
There are two main parts to performing an xsl transform. First we compile the xsl stylesheet and emit some IL with the instructions of what needs to happen during the transformation, and the second part is actually performing the transformation of a passed in Xml using those compiled IL instructions. Stylesheets may have Scripts inside which could be using different languages to define functions to be performed with the input. When porting both of our Xslt engines (XslTransform and XslCompileTransform) from .NET Framework to core, we added a lot of code that performs the first part of the transformation (compilation into IL instructions) for Scripts, but we decided to not support Scripts in Core so when you try to then perform the transformation we will throw PlatformNotSupported exception. This means that running `XsltCompileTransform.Load(someStyleSheetWithScripts)` will not throw (since this only does the compiling) but when doing `XsltCompileTransform.Transform(someInputXml)` you will actually get the exception. This issue is to track all of those code blocks we have for compiling Scripts, given that we effectively don't support them so we should throw PNSE whenever somebody tries to Load a stylesheet containing Scripts.
Author: joperezr
Assignees: -
Labels: `area-System.Xml`, `enhancement`, `size-reduction`
Milestone: 6.0.0
milkshakeuk commented 3 years ago

@joperezr

Why was it decided to remove support for script blocks in .net core and above? Is there a possibility it will ever make its way back into newer versions of dotnet? We recently (finally) ported a dotnet framework project to .net5 and the platform not support exception meant we couldn't port across to .net5 (we have a lot of xslt files which use scripts.

joperezr commented 3 years ago

I am not familiar as to why we only partially added support for Scripts but didn't actually support the Transofrm side of it. Perhaps @krwq knows more here?

Is there a possibility it will ever make its way back into newer versions of dotnet?

I think the answer here will e very dependent on what @krwq can share as of why it was removed, but if it was just an issue with not having enough bandwidth to port that, then I wouldn't see a reason why not to add it in the future.

krwq commented 3 years ago

@milkshakeuk I assume you mean C# script blocks? If so it was removed solely because of security concerns - you can execute arbitrary code in such scripts. Also System.Xml should have as little dependencies as possible and this was adding really weird dependency in the project. It's better idea to manipulate the doc using i.e. XDocument directly in your code rather than C# script inside of an XSLT. I'm personally opposed to ever adding it back for the same reason we're trying to get rid of BinaryFormatter - it is a security trap and regardless how well we document it's unsafe API for untrusted inputs people still do it.

@GrabYourPitchforks thoughts?

milkshakeuk commented 3 years ago

@krwq in our case its javascript blocks.

<msxsl:script language="javascript" implements-prefix="local">

We inherited these, I guess it makes sense from a security perspective but we haven't the time to convert all these javascript functions to use extension objects instead, so for now we have spun up another app which targets .net framework 4.8 just so we make as little changes as possible to the XSLT's.

krwq commented 3 years ago

I can't remember how javascript got executed in XSLT but I think having javascript interpreter inside of XML is not a good idea (especially in terms of size of the library - relatively very few people use it but everyone has to pay the size price)

GrabYourPitchforks commented 3 years ago

Agree with @krwq - it's a security trap. But I want to say another reason the functionality got yanked is that it depends on CodeDom, which has a somewhat tenuous relationship with .NET Core and beyond and is considered a legacy component. If this functionality ever comes back as an optional package (and I wouldn't expect it to do so given its security concerns) I'd expect it to be based on Roslyn instead of CodeDom. This would bring with it a whole new set of dependencies, but at least it would be more future-proof.

milkshakeuk commented 3 years ago

@krwq @GrabYourPitchforks thanks for the updates.

It would be useful to know the reasons why on the Microsoft documentation page rather than its just not supported.

krwq commented 3 years ago

@milkshakeuk please open a ticket in dotnet/docs to edit https://github.com/dotnet/docs/blob/main/docs/standard/data/xml/script-blocks-using-msxsl-script.md