Adjust SAFE_URL_PATTERN regex for use with test method of regexes.
#33136
Conversation
The test method on regexes behaves different than the match method on strings in the presence of the global modifier. See here for an example where the same input returns true, then false: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/RegExp/test Adds escapes for slashes and a unit test for sanitizing the same template twice.
|
Thank you for catching this and for adding a test case too! That's a recent regression, and must also apply to v4-dev, so I'll backport it manually after this lands. I'll take a closer look later today. |
|
@nikonthethird your patch totally makes sense. But just to be safe, I don't suppose we could have any other issues? Not sure why the regex had the global flag specified in the first place... BTW I feel like we should report this to eslint-plugin-unicorn. It shouldn't have auto-fixed the regex since it had the global flag. Edit: just saw you reported it already, thank you! |
|
@XhmikosR, yes, the global flag should not have been on the regex from the start, I agree. The regex was copied from Angular 7 according to the source comment above it, and Angular still has the regex with the global flag on it. But they are using the Edit: And their source comment mentions the Closure library, they have updated the regex and removed the global flag it seems. Correction: They have not updated the regex, they never had the global flag on it, from the first publicly accessible commit! So it seems someone at the Angular team added it. As far as I understood the documentation of the |
|
Yeah, agreed. Thanks for the clarification and especially for the test case. I'll merge it and backport it to v4-dev in the next days. 4.6.0 isn't affected since I landed this after releasing it. |
XhmikosR
changed the title
SAFE_URL_PATTERN regex for use with test method of regexes.
|
Ah, it seems that we don't fully cover the sanitizer in the v4-dev branch. @nikonthethird could you have a look at backporting this manually and adding a test there too? |
|
@XhmikosR ok, I'm on it. |
Recently, the method used to check attributes in the sanitizer has been switched from
matchtotest.The
testmethod on regexes however behaves different than thematchmethod on strings in the presence of the global modifier (g).See here for an example where the same input returns true, then false.
This modifier causes issues when the same template is sanitized multiple times, which happens when hovering over tooltips containing an
<img src>tag for example.This PR also adds a unit test for sanitizing the same template twice.
This fixes #33124.