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

feat(ngSanitize): add $sanitizeExt factory #6252

Closed
wants to merge 1 commit into from

Conversation

bradyisom
Copy link
Contributor

Browser: Chrome
Component: misc core
Regression: no

Add $sanitizeExt factory to allow extenstion of the $sanitize element and
attribute whitelists. This allows for unsupported, standard elements and
attributes, as well as custom elements and attributes.

Closes #5900

@mary-poppins
Copy link

Thanks for the PR! Please check the items below to help us merge this faster. See the contributing docs for more information.

  • Uses the issue template (#6252)

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!

@bradyisom
Copy link
Contributor Author

I don't understand the errors in the Travis CI build. It looks completely unrelated to the code changes. Is there a way to run the build again to see if it was an issue with the build environment?

@caitp
Copy link
Contributor

caitp commented Feb 14, 2014

Don't worry about the CI builds right now, we're getting a lot of flakiness from the SL browsers right now which is causing failures, https://travis-ci.org/angular/angular.js/builds/18846911 doesn't look like a huge problem

@OliverJAsh
Copy link

Nice work on raising the PR!

Just wondered why you went for methods to extend the whitelist, instead of allowing the whitelist to be completely overridden as per your previous suggestion? I would prefer it if we allowed the consumer to override the whitelist because they might need to remove a default item.

@bradyisom
Copy link
Contributor Author

Having one method that overrides the entire whitelist seemed too bulky, as it would require passing in the entire list of tags/attributes. These methods also update more than one internal list, so you'd have to supply a method for each list.

If you want to have the ability to remove tags/attributes, I'd suggest just adding "remove" methods instead.

@OliverJAsh
Copy link

I’m wary of having methods to add/remove, because if we change the default whitelist in a future update, it might negate any work the consumer has done to change the whitelist. I’m not sure this is a problem in all honesty, just wondering if it could make for some painful upgrades for users.

@bradyisom
Copy link
Contributor Author

Any updates on this one? It doesn't look like it's been assigned or assigned to a milestone. I'd like to have this feature in Angular so that I can use it in my product instead of having to recreate the ngSanitize module on my own.

@lefos987 lefos987 added this to the 1.3.0 milestone Apr 24, 2014
Add $sanitizeExt factory to allow extenstion of the $sanitize element and
attribute whitelists. This allows for unsupported, standard elements and
attributes, as well as custom elements and attributes.

Closes angular#5900
@bradyisom
Copy link
Contributor Author

I've tried making small tweaks and committing several times to get the build to succeed once, but I'm getting all sorts of random errors from NPM install failing to Selenium timing out. Obviously, none of those are specific to the changes in this PR.

@OliverJAsh
Copy link

I still think this needs a way of removing elements, preferably as per my suggestion: #6252 (comment)

@OliverJAsh
Copy link

Whilst this remains not configurable, I am implementing my own sanitizer using html-janitor:

sbscribe.filter('unsafe', function ($sce) {
    return function (val) {
        return $sce.trustAsHtml(val);
    };
});

// FIXME: Actually use `ngSanitize`
// https://github.com/angular/angular.js/issues/5900
// https://github.com/angular/angular.js/issues/6218
// https://github.com/angular/angular.js/pull/6252
// TODO: Not exhaustive?
var janitor = new HtmlJanitor({
    // Whilelist
    tags: {
        p: {},
        code: {},
        pre: {},
        // TODO: Map b => strong
        strong: {},
        b: {},
        // TODO: Map em => i
        em: {},
        i: {},
        // TODO: Map strike => del
        strike: {},
        del: {},
        a: { href: true },
        ul: {},
        ol: {},
        li: {},
        blockquote: {},
        h1: {},
        h2: {},
        h3: {},
        h4: {},
        h5: {},
        h6: {},
        sub: {},
        sup: {}
    }
});

sbscribe.filter('sanitize', function (unsafeFilter) {
    return function (val) {
        return unsafeFilter(janitor.clean(val));
    };
});

@petebacondarwin
Copy link
Contributor

This PR is shortly to become outdated with @IgorMinar's PR #12524 lands. In any case we should be configuring the sanitizer via its provider.

I am going to close this PR.

If you still want to be able to whitelist specific elements, which we would not recommend unless you really know what you are doing, then please put together another PR once #12524 lands.

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.

Enhancing ngSanitize whitelists
9 participants