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

HttpUtility.JavaScriptStringEncode single quote behavior does not match documentation #100248

Open
1 task done
ogjkoch opened this issue Mar 18, 2024 · 8 comments
Open
1 task done
Labels
area-System.Net documentation Documentation bug or enhancement, does not impact product or test code help wanted [up-for-grabs] Good issue for external contributors
Milestone

Comments

@ogjkoch
Copy link

ogjkoch commented Mar 18, 2024

Is there an existing issue for this?

  • I have searched the existing issues

Describe the bug

The documentation for HttpUtility.JavaScriptStringEncode states that the single quote character (') will be escaped as \'.

However, it gets escaped as \u0027.

Double quote is escaped as the documentation describes.

Expected Behavior

Expected that HttpUtility.JavaScriptStringEncode functions as documented.

Steps To Reproduce

Pass a string containing a single quote character (') to HttpUtility.JavaScriptStringEncode.

Here is a .NET fiddle displaying this behavior.

Exceptions (if any)

No response

.NET Version

8.0

Anything else?

No response

@amcasey
Copy link
Member

amcasey commented Mar 25, 2024

Single quote is listed here, but not here. Seems like an oversight, but this file hasn't changed in years, other than to pick up perf and language improvements.

@amcasey amcasey transferred this issue from dotnet/aspnetcore Mar 25, 2024
Copy link
Contributor

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

@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Mar 25, 2024
@MihaZupan
Copy link
Member

I think the behavior here is correct, \' isn't a valid escape sequence.
The docs appear to be wrong here.

@MihaZupan MihaZupan added the documentation Documentation bug or enhancement, does not impact product or test code label Apr 9, 2024
@MihaZupan MihaZupan added this to the Future milestone Apr 9, 2024
@MihaZupan MihaZupan added help wanted [up-for-grabs] Good issue for external contributors and removed untriaged New issue has not been triaged by the area owner labels Apr 9, 2024
@amcasey
Copy link
Member

amcasey commented Apr 9, 2024

I think the behavior here is correct, \' isn't a valid escape sequence. The docs appear to be wrong here.

Do you have more details on this behavior? When I type "\'" in the browser console, it outputs "'". Is that a non-compliant implementation? Naively, I would have guessed it was allowed simply because JS allows '-delimited strings.

Or maybe I've misunderstood entirely and we're talking about C# string literals?

Edit: I think this is the relevant grammar production in the ecmascript spec.

@MihaZupan
Copy link
Member

Ah, looks like I was looking at JSON, which defines the same set except for '.
Assuming the Unicode escape sequence \u0027 would be treated the same way as \', do you think updating the docs here would be sufficient?
It seems plausible that people are using this API for JSON string escaping, and \' would break those.

@amcasey
Copy link
Member

amcasey commented Apr 11, 2024

It seems plausible that people are using this API for JSON string escaping, and ' would break those.

While that does seem plausible, the code already escapes '. Changing that seems likely to break people using the type to escape JS strings. It may, however, be worth putting a remark in the docs about having to consider \' if you're escaping for JSON.

@MihaZupan
Copy link
Member

Right, I'm suggesting we don't change the behavior here and update the docs instead since over-escaping is fine in JSON (and should be in JS strings as well?).

@amcasey
Copy link
Member

amcasey commented Apr 11, 2024

My turn to get mixed up. 😆 I forgot what the current state was.

Yes, I think I'd agree that a doc update makes more sense. I kind of wish we had escaped single quote "properly" in the first place, but that ship has sailed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.Net documentation Documentation bug or enhancement, does not impact product or test code help wanted [up-for-grabs] Good issue for external contributors
Projects
None yet
Development

No branches or pull requests

3 participants