test(style): Limit allowed attributes on <a> elements #4035

ExE-Boss · 2019-05-01T11:49:11Z

Resolves #4033

ExE-Boss

Here’s some facepalm worthy bugs this change detected on top of the restriction to only allowing the href attribute on <a> elements:

ExE-Boss · 2019-05-01T12:07:01Z

api/BatteryManager.json

@@ -35,7 +35,7 @@
                  "name": "dom.battery.enabled"
                }
              ],
-              "notes": "Disabled by default in Firefox 10, but can be enabled setting the preference <code>dom.battery.enabled</code> to <code>true</code>. Starting with Firefox 11, <code>mozBattery</code> is enabled by default. The Battery API is currently supported on Android, Windows, and Linux with UPower installed. Support for MacOS is available starting with Firefox 18. Firefox also provide support for the deprecated <a href='https://developer.mozilla.org/docs/Web/API/Navigator/battery' title='The battery read-only property returns a BatteryManager provides information about the system's battery charge level; provides also some new events you can handle to monitor the battery status. This implements the Battery Status API; see that documentation for additional details, a guide to using the API, and sample code.'><code>navigator.battery</code></a>."
+              "notes": "Disabled by default in Firefox 10, but can be enabled setting the preference <code>dom.battery.enabled</code> to <code>true</code>. Starting with Firefox 11, <code>mozBattery</code> is enabled by default. The Battery API is currently supported on Android, Windows, and Linux with UPower installed. Support for MacOS is available starting with Firefox 18. Firefox also provide support for the deprecated <a href='https://developer.mozilla.org/docs/Web/API/Navigator/battery'><code>navigator.battery</code></a>."


The title attribute here was actually unclosed:

Firefox also provide support for the deprecated <a href='https://developer.mozilla.org/docs/Web/API/Navigator/battery' title='The battery read-only property returns a BatteryManager provides information about the system's battery charge level; provides also some new events you can handle to monitor the battery status. This implements the Battery Status API; see that documentation for additional details, a guide to using the API, and sample code.'> <code>navigator.battery</code> </a>

ExE-Boss · 2019-05-01T12:08:04Z

api/CharacterData.json

@@ -65,7 +65,7 @@
            },
            "firefox": {
              "version_added": "25",
-              "notes": "Two properties, <code>nextElementSibling</code> and <code>previousElementSibling</code>, have been moved to the <a href='https://developer.mozilla.org/docs/Web/API/NonDocumentTypeChildNode><code>NonDocumentTypeChildNode</code></a> interface, also implemented by <code>CharacterData</code>."
+              "notes": "Two properties, <code>nextElementSibling</code> and <code>previousElementSibling</code>, have been moved to the <a href='https://developer.mozilla.org/docs/Web/API/NonDocumentTypeChildNode'><code>NonDocumentTypeChildNode</code></a> interface, also implemented by <code>CharacterData</code>."


This href attribute had an unclosed ', which broke the compatibility table: https://developer.mozilla.org/docs/Web/API/CharacterData#Browser_compatibility

ExE-Boss · 2019-05-01T12:10:38Z

css/properties/transition.json

@@ -47,7 +47,7 @@
                "notes": [
                  "Before Firefox 57, transitions do not work when transitioning from a <a href='https://developer.mozilla.org/docs/Web/CSS/text-shadow'><code>text-shadow</code></a> with a color specified to a <code>text-shadow</code> without a color specified (see <A href='https://bugzil.la/726550'>bug 726550</a>).",
                  "Before Firefox 57, cancelling a filling animation (for example, with <code>animation-fill-mode: forwards</code> set) can trigger a transition set on the same element, although only once (see <a href='https://bugzil.la/1192592'>bug 1192592</a> and <a href='https://bug1192592.bmoattachments.org/attachment.cgi?id=8843824'>these test cases</a> for more information).",
-                  "Before Firefox 57, the <a href='https://developer.mozilla.org/docs/Web/CSS/background-position'><code>background-position</code></a> property can't be transitioned between two values containing different numbers of <a href='https://developer.mozilla.org/docs/Web/CSS/position_value' t><code>&lt;position&gt;</code></a> values, for example <code>background-position: 10px 10px;</code> and <code>background-position: 20px 20px, 30px 30px;</code> (see <a href='https://bugzil.la/1390446'>bug 1390446</a>)."
+                  "Before Firefox 57, the <a href='https://developer.mozilla.org/docs/Web/CSS/background-position'><code>background-position</code></a> property can't be transitioned between two values containing different numbers of <a href='https://developer.mozilla.org/docs/Web/CSS/position_value'><code>&lt;position&gt;</code></a> values, for example <code>background-position: 10px 10px;</code> and <code>background-position: 20px 20px, 30px 30px;</code> (see <a href='https://bugzil.la/1390446'>bug 1390446</a>)."


These <a> elements had an extra empty t attribute for whatever reason.

ExE-Boss · 2019-05-01T12:12:15Z

test/test-style.js

+ * @param {number} index
+ * @return {[number, number] | [null, null]}
+ */
+function indexToPos(str, index) {


I imported this helper function from #3913.

queengooborg

LGTM! 👍

Elchi3 · 2019-05-03T09:37:26Z

So first of all, thanks for your quick PR on this topic, @ExE-Boss! I think you uncovered some problems that are definitely worth fixing.

In here, you're using some real advanced regex magic and I wonder if this is a good approach or whether we wouldn't be better off by using a HTML validation/sanitization library or so, especially since this isn't the first time this comes up: there are several problems due to us not doing proper validation of free form fields like the notes property. However, I feel that this PR is a bit like duct tape so only a subset of problems are addressed and we need more duct tape to address more issues. So, I want to take a step back and look at a few issues together so we can decide how we could solve them:

notes allow HTML, but we don't validate if it is well-formed HTML (Lint for broken HTML #3985)
notes allow all kinds of HTML, but we only want to allow <a> and <code> (maybe more) (Add documentation and linter for escaping HTML in the notes property #2799)
notes allow links, but we only want them to have href (Lint for unnnecessary attributes in markup (e.g. inside notes) #4033 and this PR)

I'm not sure if all this is still best kept in "test-style" or if we need larger weapons to do this properly (maybe "test-html" and the use of a library instead of our own complicated regexes).

What do you think about this topic?

ddbeck · 2019-05-10T18:36:05Z

In here, you're using some real advanced regex magic and I wonder if this is a good approach or whether we wouldn't be better off by using a HTML validation/sanitization library

FWIW, I used jsdom to check this sort of thing in short-descriptions https://github.com/mdn/short-descriptions/blob/master/test/content-rules.js#L21 It's another option worth considering.

queengooborg · 2019-05-22T08:18:13Z

Just to throw out additional information, I found sanitize-html which seems to allow the limitation of HTML elements and attributes on those elements. I would probably look at this less as a dependency and more as an example of what we're looking for as it performs the fixes and outputs clean HTML (rather than letting us know what the problems are), but we could totally do an original !== clean check or something.

ExE-Boss · 2019-06-27T17:37:40Z

Closing in favour of #4259.

ExE-Boss requested a review from Elchi3 as a code owner May 1, 2019 11:49

test(style): Limit allowed attributes on <a> elements

08521ec

ExE-Boss force-pushed the test/style/limit-allowed-attributes branch from 4accbdd to a8af691 Compare May 1, 2019 12:00

fix: Remove disallowed title attributes from <a> elements

a8af691

ExE-Boss commented May 1, 2019

View reviewed changes

queengooborg added the linter Issues or pull requests regarding the tests / linter of the JSON files. label May 2, 2019

queengooborg approved these changes May 2, 2019

View reviewed changes

queengooborg added the rebase-needed 🚧 label May 8, 2019

ExE-Boss mentioned this pull request May 21, 2019

fix: Correct broken HTML and remove title tags in links for notes #4195

Merged

ExE-Boss mentioned this pull request Jun 6, 2019

test(HTML): Apply whitelist to HTML in HTML accepting properties #4259

Closed

ExE-Boss closed this Jun 27, 2019

Elchi3 removed their request for review July 9, 2019 12:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test(style): Limit allowed attributes on <a> elements #4035

test(style): Limit allowed attributes on <a> elements #4035

ExE-Boss commented May 1, 2019

ExE-Boss left a comment •

edited

Loading

ExE-Boss May 1, 2019

ExE-Boss May 1, 2019

ExE-Boss May 1, 2019

ExE-Boss May 1, 2019

queengooborg left a comment

Elchi3 commented May 3, 2019

ddbeck commented May 10, 2019

queengooborg commented May 22, 2019

ExE-Boss commented Jun 27, 2019

test(style): Limit allowed attributes on <a> elements #4035

test(style): Limit allowed attributes on <a> elements #4035

Conversation

ExE-Boss commented May 1, 2019

ExE-Boss left a comment • edited Loading

Choose a reason for hiding this comment

ExE-Boss May 1, 2019

Choose a reason for hiding this comment

ExE-Boss May 1, 2019

Choose a reason for hiding this comment

ExE-Boss May 1, 2019

Choose a reason for hiding this comment

ExE-Boss May 1, 2019

Choose a reason for hiding this comment

queengooborg left a comment

Choose a reason for hiding this comment

Elchi3 commented May 3, 2019

ddbeck commented May 10, 2019

queengooborg commented May 22, 2019

ExE-Boss commented Jun 27, 2019

ExE-Boss left a comment •

edited

Loading