feat($sanitize): Added support for the sftp protocol in $$ SanitizeUriProvider and linky filter #16102
Conversation
… SanitizeUriProvider Added support for the sftp protocol in $$ SanitizeUriProvider and linky filter
|
I do not understand why the build is falling |
|
Hi, the build error is unrelated to your PR. We are looking into it. |
|
@Narretz, Tell me please, where are the rules for adding to the documentation about the fixes. |
|
@XFree the docs are in the code files themselves. However, it looks like the default whitelist is not documented. The docs for linky are here: https://github.com/angular/angular.js/blob/master/src/ngSanitize/filter/linky.js Btw, the scope for this should be |
XFree
changed the title
XFree
changed the title
|
Could you please add |
|
@Narretz, |
|
@XFree the docs are created from the comments around the functions, e.g. angular.js/src/ngSanitize/filter/linky.js Lines 8 to 10 in ecc09a4 |
|
@Narretz , Oh. Thank you. |
|
Is there any browser with built-in support of sftp?... That looks safe, and so I'm okay with merging, but I think that might be better suited as an additional config knob that gets set in the application itself. SFTP matches the idea of fetching stuff at the end of a link, so I think it's reasonable. But if we allow sftp, then you could push that logic to ssh://, magnet://, onion:// and a more which would be surprising for devs who didn't explicitly allow those. In all of those cases, I'd rather see something like linkyProvider.allowProtocols("sftp://") / $compileProvider.allowProtocols("sftp://") in the config phase of the application: this way, you don't change the default, and if you need to use these protocols, then two lines are enough to enable them app-wide. |
|
@rjamet , In this case, when this need to do this for https, but this is beyond the scope of this PR. |
src/ng/sanitizeUri.js
Outdated
| var aHrefSanitizationWhitelist = /^\s*(https?|ftp|mailto|tel|file):/, | ||
| imgSrcSanitizationWhitelist = /^\s*((https?|ftp|file|blob):|data:image\/)/; | ||
| var aHrefSanitizationWhitelist = /^\s*(https?|s?ftp|mailto|tel|file):/, | ||
| imgSrcSanitizationWhitelist = /^\s*((https?|s?ftp|file|blob):|data:image\/)/; |
There was a problem hiding this comment.
Oops, I missed this, sorry: is there a reason you're updating imgSrc too? I can see how navigational links could work, but if browsers don't support the protocol, images and such wouldn't work at all (Chrome seems to complain with Failed to load resource: net::ERR_UNKNOWN_URL_SCHEME at least).
|
LGTM security-wise, I'll let the Angular team decide for the rest. |
|
I do not understand what the problems are with CI? |
|
|
|
When will Pull request be merged? =) |
…protocol in links Add support for the sftp protocol in the linky filter and the "aHrefSanitizationWhitelist" that is used by $sanitize and can be configured in the $compileProvider. Closes #16102
… SanitizeUriProvider
Added support for the sftp protocol in $$ SanitizeUriProvider and linky filter
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
feature
What is the current behavior? (You can also link to an open issue here)
sftp not suported
What is the new behavior (if this is a feature change)?
sftp suported
Does this PR introduce a breaking change?
no
Please check if the PR fulfills these requirements
Other information: