Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sanitize mdstrip #1351

Merged
merged 4 commits into from Jan 8, 2018
Merged

Sanitize mdstrip #1351

merged 4 commits into from Jan 8, 2018

Conversation

abulte
Copy link
Contributor

@abulte abulte commented Jan 8, 2018

Jinja do_striptags result is not safe to piped into a safe filter, for example when we output json_ld.

Not totally sure about the impact of this on the rest of the site, but some nasty things happening on a reuse do not happen anymore and the reuse stil displays fine.

@abulte abulte changed the title Use escape instead of striptags in mdstrip [WIP] Use escape instead of striptags in mdstrip Jan 8, 2018
@noirbizarre
Copy link
Contributor

Maybe simply use bleach like we do for markdown

@abulte
Copy link
Contributor Author

abulte commented Jan 8, 2018

Hum, actually we should not replace striptags but complement it.

@abulte abulte changed the title [WIP] Use escape instead of striptags in mdstrip [WIP] Sanitize mdstrip Jan 8, 2018
@abulte
Copy link
Contributor Author

abulte commented Jan 8, 2018

New version with bleach and keeping the striptags feature. I'm unsure about https://github.com/opendatateam/udata/pull/1351/files#diff-8eed20078dbe50136ec367e07dcf4ed1L106

@abulte abulte requested review from jphnoel and removed request for jphnoel January 8, 2018 11:32
@abulte abulte changed the title [WIP] Sanitize mdstrip Sanitize mdstrip Jan 8, 2018
@abulte abulte requested a review from a team January 8, 2018 11:41
@@ -100,6 +107,7 @@ def mdstrip(value, length=None, end='…'):
value = value.split(EXCERPT_TOKEN, 1)[0]
rendered = md(value)
text = do_striptags(rendered)
text = bleach_clean(text)
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems to be working. I'm just wondering whether this should after or before the do_striptags

Copy link
Contributor Author

@abulte abulte Jan 8, 2018

Choose a reason for hiding this comment

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

Somehow do_striptags would reintroduce the injection. I tested because it seemed more logical but...

Copy link
Contributor

Choose a reason for hiding this comment

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

Fine to me, I was just wondering the reasonning 👍

@@ -100,6 +107,7 @@ def mdstrip(value, length=None, end='…'):
value = value.split(EXCERPT_TOKEN, 1)[0]
rendered = md(value)
text = do_striptags(rendered)
text = bleach_clean(text)
Copy link
Contributor

Choose a reason for hiding this comment

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

Fine to me, I was just wondering the reasonning 👍

@abulte abulte merged commit f53ce8f into opendatateam:master Jan 8, 2018
@abulte abulte deleted the fix/escape-md-content branch January 8, 2018 14:56
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.

None yet

3 participants