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

fix(ngSanitize) - fixed the problem with relative links #4736

Closed
wants to merge 2 commits into from
Closed

fix(ngSanitize) - fixed the problem with relative links #4736

wants to merge 2 commits into from

Conversation

janbaer
Copy link

@janbaer janbaer commented Oct 31, 2013

fixed the problem that relative links was removed by the htmlParser function

Closes #3748

fixed the problem that relative links was removed by the htmlParser function

Closes #3748
@caitp
Copy link
Contributor

caitp commented Nov 14, 2013

is there a reason why we can't use $sce.getTrustedResourceUrl() for href/src? --- Having a single whitelist/blacklist seems like a sensible approach to this

@brandonaaskov
Copy link

That's awesome, thank you. I started playing around with this and made a fix in the regex in linky.js. Doesn't $sanitize run ng-bind-html through linky? I might just not be understanding why they're separated here.

@petebacondarwin
Copy link
Contributor

@caitp - that sounds like a good idea. I don't think the sanitize parser has had much attention since before $sce was created.

@petebacondarwin
Copy link
Contributor

@janbaer - have you signed the CLA?

@janbaer
Copy link
Author

janbaer commented Nov 16, 2013

Yes I did it!

@ghost ghost assigned tbosch Nov 21, 2013
@tbosch
Copy link
Contributor

tbosch commented Nov 21, 2013

Hi,
$sce.getTrustedResourceUrl() is used for checking urls from which templates are allowed to be loaded. In contrast, $sanitize checks the content of those templates (or any other html) that has been loaded. So $sanitize should allow external links, but $sce.getTrustedResourceUrl() should not.

However, there is another duplication which we should remove and which will also fix this bug:
Right now $compile also sanitizes links for images and links. It's very similar to $resource and it even provides the regex that should be used as a configurable provider property (see https://github.com/angular/angular.js/blob/master/src/ng/compile.js#L718-L730).

The idea would be make $sanitize depend on $compile, read out the regex from there and use almost the same code snippet from $compile (including the call to urlResolve). By this, $sanitize would always be in sync to the sanitization that we don in $compile.

@janbaer Would you like to update your pull request (including the tests) accordingly? For the unit tests, you can have a look at the unit tests for the link sanitizing in $compile: https://github.com/angular/angular.js/blob/master/test/ng/compileSpec.js#L3836-L4190
Otherwise I could also create a new PR...

* angular/master: (140 commits)
  fix(ngAnimate): use a fallback CSS property that doesn't break existing styles
  refactor($sce): Use $sniffer instead of $document for feature detection.
  fix(ngClass): ensure that ngClass only adds/removes the changed classes
  fix($animate): ensure the DOM operation isn't run twice
  fix(ngInclude): allow ngInclude to load scripts when jQuery is included
  docs($log): the documented default log behavior was incorrect
  docs(ngAnimate): fixed two small typos
  docs(booleanAtts): explain the motivation for boolean attributes
  docs(tutorial/step-2): correct the link to jasmine docs
  docs(ngRepeat): fix typo
  docs(ngPluralize): Fix missing space before parentheses
  docs(guide/directive): use `hideDialog` handler in example
  docs(guide/ie): fix typo
  docs(guide/providers): remove extra closing parenthesis in example
  docs(guide/migration): fix typo
  docs(guide/compiler): fix typo in isolate scope def
  docs(api): example for $provide.value() uses $provide.value()
  docs(ngRoute): make it easier to find the example
  docs(tutorial): minimum node.js version is 0.10 (Windows too)
  docs(guide/migration): fix typo
  ...
@janbaer
Copy link
Author

janbaer commented Nov 21, 2013

@tbosch Ok, I can try it. But "and use almost the same code snippet from $compile" sounds like code duplication. Maybe we should think about a common UrlValidator that has the regex in it self...

@tbosch
Copy link
Contributor

tbosch commented Nov 21, 2013

Actually, you don't have to repeat the tests:
If you just add a test that tests that $sanitize uses the whitelist regex from $compile then you don't need to test the regex itself, as that is already tests in $compile.
I think that's the easiest way to do it...

@janbaer
Copy link
Author

janbaer commented Nov 23, 2013

@tbosch I'm sorry I've tried but I think this change is more complicated then I've (and maybe you) thought. The first is, that both regexp are totally different.

URI_REGEXP = /^((ftp|https?)://|mailto:|tel:|#|/)/i
aHrefSanitizationWhitelist = /^\s*(https?|ftp|mailto|tel|file):/

I don't mean the different naming conventions. I've just tried to replace the regexp in sanitize.js and all tests for isUri was failed. Ok I can try to change the regex so that it will run. But than I had a problem with the tests in compileSpec.js.

The other problem is that the regexp will not be exported from the $compile module.

I don't think that it's a good a idea to have a dependency to another module to use just a common regexp. I think it would be better to accept the small change that I've made so that we can use it in the next version (1.2.3) and then thinking about how the $sanitize module can use more functionality from $compile as a regexp.

But it's not my decision, maybe I'm wrong...

@tbosch
Copy link
Contributor

tbosch commented Nov 25, 2013

Hi @janbaer,
the trick is to not only replace the regexp, but also call urlResolve before, so the url gets an absolute url before we do the regex test. That's the same as $compile does.

To get this into our next release on Wednesday, I created an own PR: #5137

Thanks for your help and reporting this!
Tobias

@janbaer
Copy link
Author

janbaer commented Nov 26, 2013

Hi @tbosch,

thanks for your PR! When it's merged and released, I can reuse the official release of #angular.js in my project! 👍

@brandonaaskov
Copy link

Woot! Thank you! 👍

@tbosch
Copy link
Contributor

tbosch commented Nov 26, 2013

Closing this as the other PR is already in master via commit 3335234,

@tbosch tbosch closed this Nov 26, 2013
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

angular-sanitize.js remove internal links
5 participants