Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

fix($sanitize): Use same whitelist mechanism as $compile does. #5137

Closed
wants to merge 1 commit into from

Conversation

tbosch
Copy link
Contributor

@tbosch tbosch commented Nov 25, 2013

$sanitize now uses the same mechanism as $compile to validate uris.
By this, the validation in $sanitize is now more general and can be
configured using $compileProvider#imgSrcSanitizationWhitelist and
$compileProvider#aHrefSanitizationWhitelist.

This commit also refactors the linky filter to use $sanitize as a
service instead of directly referencing private functions of $sanitize.

Fixes #3748.

@mary-poppins
Copy link

Thanks for the PR!

  • Contributor signed CLA now or in the past
    • If you just signed, leave a comment here with your real name
  • PR's commit messages follow the commit message format

If you need to make changes to your pull request, you can update the commit with git commit --amend.
Then, update the pull request with git push -f.

Thanks again for your help!

@IgorMinar
Copy link
Contributor

the commit message should read: fix($sanitize): use same whitelist mechanism as $compile does (lowercase u and now period at the end)

@@ -500,7 +500,8 @@ function $CompileProvider($provide) {
COMMENT_DIRECTIVE_REGEXP = /^\s*directive\:\s*([\d\w\-_]+)\s+(.*)$/,
CLASS_DIRECTIVE_REGEXP = /(([\d\w\-_]+)(?:\:([^;]+))?;?)/,
aHrefSanitizationWhitelist = /^\s*(https?|ftp|mailto|tel|file):/,
imgSrcSanitizationWhitelist = /^\s*(https?|ftp|file):|data:image\//;
imgSrcSanitizationWhitelist = /^\s*(https?|ftp|file):|data:image\//,
selfProvider = this;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this name is not very good because it makes it sounds like the variable is provider of self, but it's really just $compileProvider so I would call it like that.

@IgorMinar
Copy link
Contributor

I'm not too crazy about how the state is shared via $compileProvider but right now I don't have a better suggestion. Can you think of another way?

@tbosch
Copy link
Contributor Author

tbosch commented Nov 26, 2013

The whitelist regex of $compileProvider can be configured and might be different for every injector.
So we can't keep it in an internal place without exposing it as a provider / service.
I put it on the provider and not on $compile service as otherwise there is a circular dependency: $compile -> $sce -> $sanitize -> $compile

The only other way I can think of is creating a new service whose only purpose is to validate uris. But that would be a new public API, although we could prefix it with $$.

E.g. $$uriSanitizer

  • provider has the two properties from $compile
  • creates a function that is able to sanitize uris.

That would be the nicest way of doing it...

@tbosch
Copy link
Contributor Author

tbosch commented Nov 26, 2013

Either way we introduce a new public api:

  1. extending $compileProvider with a new function
  2. creating a new $$uriSanitizer service

I would vote for 2)

@IgorMinar
Copy link
Contributor

I like 2 better

@tbosch
Copy link
Contributor Author

tbosch commented Nov 26, 2013

Introduced the new $$sanitizeUri service. Could you review again?

return uri;
};
};
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing blank line at the end of the file

@IgorMinar
Copy link
Contributor

lgtm. next time please split this into two separate commits ($$sanitizeUri + linky) to make it easier to review

@tbosch
Copy link
Contributor Author

tbosch commented Nov 26, 2013

Will do. Thanks!

`$sanitize` now uses the same mechanism as `$compile` to validate uris.
By this, the validation in `$sanitize` is more general and can be
configured in the same way as the one in `$compile`.

Changes
- Creates the new private service `$$sanitizeUri`.
- Moves related specs from `compileSpec.js` into `sanitizeUriSpec.js`.
- Refactors the `linky` filter to be less dependent on `$sanitize`
  internal functions.

Fixes angular#3748.
@tbosch
Copy link
Contributor Author

tbosch commented Nov 26, 2013

Landed in master as 3335234

@tbosch tbosch closed this Nov 26, 2013
@tbosch tbosch deleted the sanitize branch November 26, 2013 22:30
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

angular-sanitize.js remove internal links
3 participants