fix(ngHref): allow numbers as input #16652
Conversation
src/ng/sanitizeUri.js
Outdated
| if (normalizedVal !== '' && !normalizedVal.match(regex)) { | ||
| return 'unsafe:' + normalizedVal; | ||
| } | ||
| return uri; | ||
| return uri.toString(); |
There was a problem hiding this comment.
For the record, this will allow more types than just number. (This might be a good thing, but it might also lead to unexpected results, instead of a fastfailure.)
If you want to explicitly only allow numbers and strings, you can have an explicit check (as we do in $anchorScroll for example).
There was a problem hiding this comment.
yes, I was also thinking about this. I've added this mainly because it's weird to sanitize a string value but return the original value. For ngHref and other interpolation it's actually not necessary, as the interpolation itself does the string conversion. So maybe we should move the toString to where the value is passed into the sanitize fn?
There was a problem hiding this comment.
@petebacondarwin what do you think about this? Should we stringify here or somewhere earlier in the chain? I have a hard time pinpointing where exactly this behavior changed. I guess it happened somewhere here: #16378
There was a problem hiding this comment.
I think we should do the stringificaton inside trustAs() where we call $$sanitizeUri(). E.g. https://github.com/petebacondarwin/angular.js/blob/e541a33ed73ca4135def5d90b1613e070a43baff/src/ng/sce.js#L443
There was a problem hiding this comment.
Sounds good to me! I've updated the implementation and added a new test for custom toString(), too for good measure.
src/ng/sanitizeUri.js
Outdated
| if (normalizedVal !== '' && !normalizedVal.match(regex)) { | ||
| return 'unsafe:' + normalizedVal; | ||
| } | ||
| return uri; | ||
| return uri.toString(); |
There was a problem hiding this comment.
I think we should do the stringificaton inside trustAs() where we call $$sanitizeUri(). E.g. https://github.com/petebacondarwin/angular.js/blob/e541a33ed73ca4135def5d90b1613e070a43baff/src/ng/sce.js#L443
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
What is the current behavior? (You can also link to an open issue here)
What is the new behavior (if this is a feature change)?
Does this PR introduce a breaking change?
Please check if the PR fulfills these requirements
Other information: