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

[API Proposal]: Add System.Web.IHtmlString and System.Web.HtmlString to System.Web.HttpUtility #83477

Closed
twsouthwick opened this issue Mar 15, 2023 · 17 comments · Fixed by #85673
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Net tenet-compatibility Incompatibility with previous versions or .NET Framework
Milestone

Comments

@twsouthwick
Copy link
Member

twsouthwick commented Mar 15, 2023

Edit by @antonfirsov: Made class HtmlString optional.

Background and motivation

We have created https://github.com/dotnet/systemweb-adapters to provide APIs people used from System.Web backed by ASP.NET Core types to enable quicker migration to ASP.NET Core. Many of the APIs are ones we can provide there. However, System.Web.IHtmlString was commonly used in ASP.NET Framework and expected HttpUtility.HtmlEncode(object) to special case it (see ReferenceSource).

This proposal is to add this type and the default implementation of it back and update HttpUtility.HtmlEncode to special case it so that code that is being migrated from ASP.NET Framework will continue to work without having to rewrite it (or find unexpected bugs at some point).

API Proposal

namespace System.Web;

public interface IHtmlString
{
    string ToHtmlString();
}

// Consider
//public class HtmlString : IHtmlString
//{
//    public HtmlString(string value);
//    public string ToHtmlString();
//}

API Usage

using System;
using System.Web;

var str = "<hello />";
var htmlString = new HtmlString(str);

Console.WriteLine(HttpUtility.HtmlEncode(str));
Console.WriteLine(HttpUtility.HtmlEncode(htmlString));

Outputs the following:

&lt;hello /&gt;
<hello />

Alternative Designs

No response

Risks

No response

@twsouthwick twsouthwick added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Mar 15, 2023
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Mar 15, 2023
@ghost
Copy link

ghost commented Mar 15, 2023

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

Issue Details

Background and motivation

We have created https://github.com/dotnet/systemweb-adapters to provide APIs people used from System.Web backed by ASP.NET Core types to enable quicker migration to ASP.NET Core. Many of the APIs are ones we can provide there. However, System.Web.IHtmlString was commonly used in ASP.NET Code and expected HttpUtility.HtmlEncode(object) to special case it (see ReferenceSource).

This proposal is to add this type and the default implementaiton of it back and update HttpUtility.HtmlEncode to special case it so that code that is being migrated from ASP.NET Framework will continue to work without having to rewrite it (or find unexpected bugs at some point).

API Proposal

namespace System.Web;

public interface IHtmlString
{
    string ToHtmlString();
}

public class HtmlString : IHtmlString
{
    public HtmlString(string value);

    public string ToHtmlString();

    public override string ToString();
}

API Usage

using System;
using System.Web;

var str = "<hello />";
var htmlString = new HtmlString(str);

Console.WriteLine(HttpUtility.HtmlEncode(str));
Console.WriteLine(HttpUtility.HtmlEncode(htmlString));

Outputs the following:

&lt;hello /&gt;
<hello />

Alternative Designs

No response

Risks

No response

Author: twsouthwick
Assignees: -
Labels:

api-suggestion, area-System.Net, untriaged

Milestone: -

@antonfirsov antonfirsov added the tenet-compatibility Incompatibility with previous versions or .NET Framework label Mar 31, 2023
@joperezr
Copy link
Member

cc: @karelz do you know who the right person to look into this is?

@danroth27
Copy link
Member

@terrajobst Thoughts on this proposal? This is to facilitate ASP.NET to ASP.NET Core migration.

@stephentoub
Copy link
Member

Porting the interface (and the extra line or two in HttpUtility to recognize it) seems reasonable.

Is the class necessary though? ASP.NET already has Microsoft.AspNetCore.Html.HtmlString... can it just implement IHtmlString if added?

@CZEMacLeod
Copy link

@stephentoub @twsouthwick I think the class just makes porting code slightly easier, the interface and the realignment of the HttpUtility code is the more important part.
I think if the class isn't included in the runtime, it could be added to the systemweb adapters project instead and would be almost as easy to use for porting code.
It certainly might be useful, again from a porting and using the incremental approach, for Microsoft.AspNetCore.Html.HtmlString to also implement IHtmlString or provide some other mechanism to enable casting from the legacy class/interface to the new class/interface, but that is outside this scope and can be addressed by the issue already open in dotnet/systemweb-adapters#205 or opened in the aspnetcore project once this (blocking) part is in place.

@MihaZupan
Copy link
Member

Do we need the string ToHtmlString() method at all, or can this be a marker interface, where HttpUtility just hands you back the ToString()?

@twsouthwick
Copy link
Member Author

Do we need the string ToHtmlString() method at all, or can this be a marker interface, where HttpUtility just hands you back the ToString()?

Yes - it's a .NET Framework API we'd like to enable for compat reasons to simplify migration. If we don't have that method, the interface would be different and wouldn't work for compat

Is the class necessary though? ASP.NET already has Microsoft.AspNetCore.Html.HtmlString... can it just implement IHtmlString if added?

It's not as necessary as the interface. As @CZEMacLeod mentioned, that's something we could put in the System.Web Adapters project or people could probably define it themselves. The goal of adding this is purely to simplify the challenges people have with migration without requiring them to bring in a whole new stack (ASP.NET Core) to potentially low levels of their project and then figure out how to keep support framework and core. This is the only API we've been trying to enable for users that has requirements in existing BCL types - everything else is just in the Microsoft.AspNetCore.SystemWebAdapters package, which has no other dependencies and easy to add at lower levels for migration.

@antonfirsov antonfirsov added this to the 8.0.0 milestone Apr 28, 2023
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Apr 28, 2023
@antonfirsov antonfirsov added the api-ready-for-review API is ready for review, it is NOT ready for implementation label Apr 28, 2023
@antonfirsov antonfirsov self-assigned this Apr 28, 2023
@antonfirsov
Copy link
Member

Considering the interest and the low implementation cost, I added this to 8.0 and marked it ready-for-review. @dotnet/ncl let me know if there are any objections.

@stephentoub
Copy link
Member

stephentoub commented Apr 28, 2023

@stephentoub @twsouthwick I think the class just makes porting code slightly easier, the interface and the realignment of the HttpUtility code is the more important part.

It's not as necessary as the interface. As @CZEMacLeod mentioned, that's something we could put in the System.Web Adapters project or people could probably define it themselves.

My concern is simply there's now already something called HtmlString in .NET. This would introduce a second one with the same name, in which case it could actually make porting harder if someone ends up with usings for System.Web and Microsoft.AspNetCore.Html and compilation fails because of ambiguities. No additional references are required to use this type, as it's part of the ASP.NET shared framework, it already provides the exact same ctor (new HtmlString(someString) will "just work"), so if we simply make the IHtmlString implement the interface, the only porting impact is someone without a using Microsoft.AspNetCore.Html; would need to add such a using.

If this isn't an issue, I don't personally have a problem adding the additional tiny type. Though if there's already a compat library that's used to enable other such porting-related things (and it sounds like there is), I'd prefer if this type is just added there if needed.

@twsouthwick
Copy link
Member Author

@stephentoub I'm fine going forward with that. Sounds like to enable the scenarios we care about, we want to:

  • Add System.Web.IHtmlString and support into System.Web.HttpUtility to dotnet/runtime
  • If needed, add System.Web.HtmlString to dotnet/systemweb-adapters
  • Explore implementing System.Web.IHtmlString on Microsoft.AspNetCore.Html.HtmlString

/cc @Tratcher @mjrousos

@stephentoub stephentoub removed the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Apr 28, 2023
@stephentoub
Copy link
Member

@Tratcher
Copy link
Member

@GrabYourPitchforks @Eilon who might have more context on this.

@GrabYourPitchforks
Copy link
Member

GrabYourPitchforks commented May 2, 2023

@GrabYourPitchforks @Eilon who might have more context on this.

I don't have context on this specific request. But I can speak to the backstory of this type.

HtmlString was invented to facilitate a migration away from WebForms's old <%= ... %> syntax in favor of everybody using <%: ... %>. At the time, XSS in WebForms due to misuse of <%= ... %> was a huge problem, and we didn't have much success in getting people to write <%= HtmlEncode(...) %>. At the same time, we were also iterating on MVC's HTML helpers, which used a syntax like <%= Html.TextBox(...) %>.

We basically set a goal for ourselves that we wanted to tell people to stop using <%= ... %> entirely and to prefer <%: ... %> everywhere instead, which would significantly lower the risk of XSS attack. But then writing <%: Html.TextBox(...) %> would emit &lt;input type=&quot;text&quot; ... /> instead of <input type="text" ... />. So we invented the HtmlString type in .NET Framework 4.0 and plumbed it through System.Web's HtmlEncode routine, which would make calls like <%: Html.TextBox(...) %> not double-encode and would help facilitate devs committing <%: ... %> syntax to muscle memory.

Later, when Razor came along, the @... syntax was created as a more "modern" equivalent of <%: ... %>. Razor did not introduce a <%= ... %> equivalent since it was deemed too dangerous.

The type we had always intended people to use within aspnet Full Framework was the concrete HtmlString type. The IHtmlString type was only exposed out of technical necessity, because MVC at the time targeted .NET Framework 3.5 and could not take a direct dependency on .NET Framework 4.0's new HtmlString type. The interface was an implementation detail which allowed the MVC runtime to bridge this gap and perform dynamic typing and type generation.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label May 2, 2023
@terrajobst
Copy link
Member

terrajobst commented May 2, 2023

Video

  • This interface would need to ship in-box and some existing type will need to implement the interface
    • @halter73 will investigate which type(s) this would be
    • HttpUtility.HtmlEncode(object) will need to be updated
namespace System.Web;

public interface IHtmlString
{
    string ToHtmlString();
}

@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels May 2, 2023
@twsouthwick
Copy link
Member Author

@halter73 Are default interface implementations used in public ASP.NET Core APIs? If so, the following would enable all implementations of IHtmlContent to get a default implementation of behavior (which can be overridden by others, i.e. HtmlString for a better implementation if needed):

namespace Microsoft.AspNetCore.Html;

public interface IHtmlContent
+    : System.Web.IHtmlString
{
    void WriteTo(TextWriter writer, HtmlEncoder encoder);

+   string System.Web.IHtmlString.ToHtmlString()
+   {
+    using var writer = new StringWriter();
+    WriteTo(writer, HtmlEncoder.Default);
+    return writer.ToString();
+  }
}

@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label May 3, 2023
@twsouthwick
Copy link
Member Author

@stephentoub is this only in-box for .NET 8? Or is there a package that supports .NET Standard for this assembly? I don't see anything obvious on NuGet, but thought I'd check.

If not, then I'll look into adding an interface of IHtmlString to dotnet/systemweb-adapters and type forward to the inbox version for .NET Framework and .NET 8+.

@stephentoub
Copy link
Member

is this only in-box for .NET 8?

Correct. We don't ship a separate package with HttpUtility.

@dotnet dotnet locked as resolved and limited conversation to collaborators Jun 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Net tenet-compatibility Incompatibility with previous versions or .NET Framework
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants