Skip to content
This repository has been archived by the owner on Dec 14, 2018. It is now read-only.

@Html.Id() implementation does not usefully sanitize return value, breaking JavaScript / CSS selectors #704

Closed
dougbu opened this issue Jun 25, 2014 · 4 comments
Assignees
Milestone

Comments

@dougbu
Copy link
Member

dougbu commented Jun 25, 2014

We generate "sanitized" identifiers in most HTML helpers, substituting '_' for invalid characters. This is done for id attributes in input helpers, the for attribute in a label generated element, and so on. However it's not done for @Html.Id(), @Html.IdFor(), or @Html.IdForModel(). The current implementation of those methods match their @Html.Name() (and so on) equivalents.

Side note: Only whitespace characters are considered invalid now. But MVC 5.2 considers all but alphanumerics and a few special characters invalid. The limited number of invalid characters reduces the impact of this bug but there's still an impact. May also want to double-check what should be invalid and whether there's a reason to break compatibility with MVC 5.2 here.

@dougbu
Copy link
Member Author

dougbu commented Jul 24, 2014

Actually two separate issues here:

  1. @Html.Id() does not call TagBuilder.CreateSanitizedId() on the returned value and therefore does not replace invalid characters, no matter what the current HTML prefix or expression name may be. That is, currently @Html.Id() and @Html.Name() never return different values.
  2. TagBuilder.CreateSanitizedId() is used correctly when adding a default "id" attribute to an HTML element. But it does not escape dots or other characters, confusing even simple jQuery use of CSS validators.

Bottom line the @Html.Id() value currently returned will often contain characters matching [][.#*, >+~=|^$:], all of which could be confused with CSS selector separators. This not only breaks compatibility with MVC 5.2 but greatly reduces the utility of this method. Cleaning up just whitespace doesn't make the default "id" attribute values much more useful.

@dougbu
Copy link
Member Author

dougbu commented Aug 13, 2014

Currently @Html.Id() returns same value as @Html.Name(). As mentioned above, this value can include characters that can be confused with CSS selector separators. That breaks JavaScript we and our users may write.

@dougbu dougbu changed the title @Html.Id() implementation should not return same value as @Html.Name() @Html.Id() implementation does not usefully sanitize return value, breaking JavaScript / CSS selectors Aug 13, 2014
@dougbu
Copy link
Member Author

dougbu commented Sep 26, 2014

As part of correcting the second part of this bug, should address issue discussed in MVC 5.2 bug 2124. In particular should handle case where first character is invalid or at least the narrower case where first character is an open square bracket.

@danroth27 danroth27 added this to the 6.0.0-beta1 milestone Oct 15, 2014
dougbu added a commit that referenced this issue Oct 17, 2014
- #704 part 1 of 2
- doesn't help tag builders unless done in `DefaultHtmlGenerator`
dougbu added a commit that referenced this issue Oct 17, 2014
- #704 part 2 of 2
- change `@Html.Id()` to sanitize return value; was identical to `@Html.Name()`

Copied `TagBuilder.CreateSanitizedId()` and `TagBuilder.Html401IdUtil` from MVC 5.2
- except this `CreateSanitizedId()` returns a valid identifier if first `char` is not a letter
 - e.g. "[0].Name"

nits:
- expand variable names, use lots of `var`, put `public` members first
- add doc comments for `CreateSanitizedId()`

Note users will be able to apply different sanitization once we fix #1188.
@dougbu
Copy link
Member Author

dougbu commented Oct 17, 2014

Fixed in commits f9e44ff and dd5da33

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants