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

Remove HtmlSanitizer once more - see #9803 #10000

Merged
merged 2 commits into from
Apr 6, 2021

Conversation

nul800sebastiaan
Copy link
Member

After some problems with inadvertent dependency updates I propose once again that we remove HtmlSanitizer for the same reasons as #9803

We didn't notice that the new version 5 of HtmlSanitizer would pull in not only a newer version of System.Text.Encoding.CodePages (4.1.1 where we were shipping with 4.0.2.0 before) but also added a dependency on System.Runtime.CompilerServices.Unsafe (v4.0.4.0) which people were already using with a higher version than got included in the Umbraco builds.

As detailed in #9803 we're really not using the sanitizer and if someone has the ability to add plugins to the Umbraco install then and svg is the least of your worries from a security standpoint.

Now... you're damned if you do and you're damned if you don't.. : removing the dependency to HtmlSanitizer also downgrades System.Text.Encoding.CodePages again and we don't want that because the same problems will start popping up that we had now where the version would change unexpectedly. I have tested with the very latest v4 version that is available (4.1.3) and dropping that into a site "just" works, as you would expect it to.
The version on NuGet is 4.7.1, with assembly version being 4.1.3. This is also the last ever v4 version Microsoft made so I suggest we ship with that for the remainder of v8.

@schrepel
Copy link

Can I ask if this would cause an error when upgrading from 8.11.1 to 8.11.2? I ran the NuGet update today, then got a YSOD about System.Text.Encoding.CodePages, and I see it's the only thing added to web.config that hadn't been there in 8.11.1. Reverted back to 8.11.1 and things are fine.

@nul800sebastiaan
Copy link
Member Author

@schrepel I'm not sure what your question is, we're removing HTMLSanitizer especially to not run in to cases like this again, for the addition of System.Text.Encoding.CodePages you will get your web.config transformed automatically and everything should "just" work. Unlike the current upgrade which was missing some bits (as you've noticed going from 8.11.1 to 8.11.2).

However if something was added to your web.config and it didn't work then could you please add:

  • What exactly was added
  • What error did you get exactly

@nul800sebastiaan nul800sebastiaan added release/8.13.0 status/dependency-update This change requires a dependency to be updated (Deploy, Forms, Courier) type/feature labels Apr 6, 2021
@nul800sebastiaan nul800sebastiaan merged commit 3358ab2 into v8/contrib Apr 6, 2021
@nul800sebastiaan nul800sebastiaan deleted the v8/feature/remove-htmlsanitizer branch April 6, 2021 08:08
@nul800sebastiaan
Copy link
Member Author

Marked as breaking because you might decide to remove it manually and your custom code that was built against it won't run any more.

NuGet is not going to remove this automatically though, so it won't be removed without manual intervention as far as I can tell.

@schrepel
Copy link

Can I ask if this would cause an error when upgrading from 8.11.1 to 8.11.2? I ran the NuGet update today, then got a YSOD about System.Text.Encoding.CodePages, and I see it's the only thing added to web.config that hadn't been there in 8.11.1. Reverted back to 8.11.1 and things are fine.

Did NuGet upgrade to 8.13.0 today, the YSOD didn't come back, so things seem to be working better.

@ronaldbarendse
Copy link
Contributor

I've had System.Text.Encoding.CodePages 5.0.0 installed on an Umbraco 8.12.2 project and received the following error when trying to update using NuGet:

Update-Package : Unable to resolve dependencies. 'System.Text.Encoding.CodePages 5.0.0' is not compatible with 'UmbracoCms.Web 8.13.0 constraint: System.Text.Encoding.CodePages (>= 4.7.1 && < 4.999999.0)'.

To fix this, you first need to downgrade this package to a compatible version and then update to Umbraco 8.13.0:

Install-Package System.Text.Encoding.CodePages -Version 4.7.1
Update-Package

After updating, you can also manually remove HtmlSantizer and any dependencies in this order:

Uninstall-Package HtmlSanitizer
Uninstall-Package AngleSharp.Css
Uninstall-Package AngleSharp

@hfloyd
Copy link
Contributor

hfloyd commented May 4, 2021

So, just to clarify (since my NuGets are all confused...)

Umbraco 8.13.0 (and projects using the UmbracoCms.Core & UmbracoCms.Web NuGets) should:

  • NOT have HtmlSanitizer installed any longer (unless custom code is using it)
  • NOT have AngleSharp.Css installed any longer
  • NOT have AngleSharp installed any longer
  • System.Text.Encoding.CodePages should be version 4.71 ???

@nul800sebastiaan
Copy link
Member Author

Sorry to hear @hfloyd :/

There is no need to remove HtmlSanitizer/AngleSharp but when you're on 8.13, you may uninstall them through NuGet as Ronald describes.
NuGet should have already updated your dependency on System.Text.Encoding.CodePages but if it didn't then yes, try to follow Ronald's steps here as well.

@hfloyd
Copy link
Contributor

hfloyd commented May 5, 2021

Thanks for the clarity, @nul800sebastiaan . It's something Ive noticed from time-to-time - sometimes when I pull an updated Cloud site, and it has been upgraded remotely, the .Web project in VS is showing as having been updated, and I need to "Consolidate" to get the related class projects in-line with the new version. Usually there isn't any issue - but sometimes if a dependency version has changed also, there can be a bit of confusion about which is the "correct" version.

It would be great if the ZIP releases for the CMS included a package.config file, just so I would have a place to check these things. (Also would be useful when I inherit a site and am trying to determine if the packages installed are part of Umbraco requirements, or something extra installed by the previous devs. 😉)

@nul800sebastiaan
Copy link
Member Author

I think I get that (zip file including the packages.config) but I don't think it would make a big difference. So the only thing at this point is that we updated the System.Text.Encoding.CodePages version, that is most likely not necessary but was done out of an abundance of caution.
The other updates wouldn't have made a difference since removing something from packages.config will not uninstall those packages, an uninstall always requires manual intervention.

And then let's just acknowledge that working with NuGet (or package managers in general) is a bit messy. There's a lot of promises but in the end.. it's a difficult problem to solve and I haven't seen package managers solve it very well yet, unfortunately. 🙈 (though NetCore's implementation so far seems a bit better).

@hfloyd
Copy link
Contributor

hfloyd commented May 5, 2021

I hear ya, I wasn't thinking the packages.config would actually DO anything - I just meant as part of the "release notes" so to speak. 😁
Sometimes if I have a site which is acting up, I will download a fresh ZIP of the version in question and do a file compare with the problematic site - just to see if a DLL or something got messed up.

@ronaldbarendse
Copy link
Contributor

ronaldbarendse commented May 8, 2021

@hfloyd You can also get a list of the dependencies (and their versions) from NuGet, although they are split between the main package, Core and Web:

The new SDK-style projects use the NuGet PackageReference format for storing the dependencies and also adds the option to generate a lock file to pin a dependency to a specific version (e.g. to a newer patch version): https://docs.microsoft.com/en-us/nuget/consume-packages/package-references-in-project-files#locking-dependencies. Having that enabled on your future projects would make sure you have a complete list of all dependencies and their exact versions in the packages.lock.json.

@hfloyd
Copy link
Contributor

hfloyd commented May 11, 2021

Thanks for the info, @ronaldbarendse

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category/breaking release/8.13.0 status/dependency-update This change requires a dependency to be updated (Deploy, Forms, Courier) type/feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants