Skip to content

Django 1.6.9#2635

Merged
jgmize merged 2 commits intomasterfrom
django-1.6.9
Jan 12, 2015
Merged

Django 1.6.9#2635
jgmize merged 2 commits intomasterfrom
django-1.6.9

Conversation

@jgmize
Copy link
Contributor

@jgmize jgmize commented Jan 10, 2015

This supersedes #2634

@jgmize
Copy link
Contributor Author

jgmize commented Jan 10, 2015

I pushed this to demo4 for testing and successfully ran the mcom-tests suite against it. I think we're good to go with these changes, but we may want to do a little more manual testing against demo4 before merging to be sure.

Copy link
Contributor

Choose a reason for hiding this comment

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

Version in vendor too old?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, I got an error when trying to use it with the latest bleach.

@pmclanahan
Copy link
Contributor

Good to have better bleach and the bleach_tags helper, but probably not necessary for this. I'm fine w/ it though if it works and stabilizes the tests.

@jgmize
Copy link
Contributor Author

jgmize commented Jan 10, 2015

You're probably right about bleach being overkill for plain text emails, and in hindsight I probably shouldn't have bothered, but now that the work is done I don't think it hurts anything to have that extra bit of protection.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe bleach_tags should return a safe string so we don't need all these safe filters? Is bleach the thing converting ampersands?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about that, and then assumed that the explicit |safe was a good thing, but maybe doing that together with renaming to bleach_safe?

Yes, bleach.clean() converts ampersands and I didn't see a way to tell it not to, so I just convert them back.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good. Marking as safe is:

from jinja2 import Markup

def bleach_safe(text):
    return Markup(bleach.clean(text, tags=[], strip=True).replace('&', '&'))

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm happy to do this and resubmit if you're busy.

@pmclanahan
Copy link
Contributor

I think it'd be nice to mark the return of the new bleach_tags as safe (since it is) and remove all of the safe filters where it's used. It's not a requirement though.

r+wc

pmac and others added 2 commits January 12, 2015 12:48
Also fixes some tests that were failing after upgrade.
Code was relying on Django's strip_tags filter which has
changed and now ignores things that obviously aren't HTML tags

Updated to latest playdoh-lib which includes fixes for Django 1.6
in funfactory.
Add bleach and upgraded html5lib submodules
Replace all instances of strip_tags|safe with bleach_tags|safe
Fix tests
jgmize added a commit that referenced this pull request Jan 12, 2015
@jgmize jgmize merged commit b2e4496 into master Jan 12, 2015
@jgmize jgmize deleted the django-1.6.9 branch January 12, 2015 18:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants