Repository navigation
Add way to control HTML escaping in HTML writer - #4423
Conversation
6a8e292 to
5fb234b
Compare
|
👍, needs changelog entry. As for cleanup we could do that, but it would require making this feature dependent on optional third-party dependencies (though I'm not against that). You could use BeautifulSoup to parse the contents of HTML column cells and strip out bad content. But there's a lot of stuff you need to strip out--not just As for OTS, mozilla has a package called bleach that's comprehensive: lxml also has a On one hand, a table containing sneakily crafted javascript is hardly the easiest vector to get an astronomer to run arbitrary code. I would just email them a FORTRAN file and ask them to compile and run it :) That said, I don't think we should leave this alone. Personally, I think the BeautifulSoup + whitelist approach seems the simplest. |
|
Or bleach--the only reason I mentioned BS is that we already have it as an optional dependency. But bleach is a pure-python package and should be fairly trivial to require as well. |
|
Hmm, looking at the docs for bleach, another nice thing about it is it will try (using the same algorithms as browsers--as it's based on html5lib) to clean up poorly formatted HTML (unclosed tags, etc.) |
taldcroft
changed the title
|
@embray - bleach was a snap to install and use, good call. I've pushed up a commit that uses |
|
So now this begs the inevitable question of having the same control in Anyway, for now I think this PR is a useful step forward. |
|
Dang, forgot to add this optional dependency for testing. |
There was a problem hiding this comment.
I feel inclined to make this a requirement to use the feature at all. If it sweetens the deal though we could include bleach in install_requires so that it will almost always be installed along with Astropy anyways (I see no reason not to add more install dependencies these days, especially if they're pure Python....)
There was a problem hiding this comment.
I would guess that install_requires applies to pip install but not conda? Anyway, I can see your point and will make this happen (i.e. require bleach if raw_html_cols is not empty / None).
There was a problem hiding this comment.
I have no idea how it applies to conda.
There was a problem hiding this comment.
Continuum (I think) maintains an internal recipe for astropy so I'd let them know if you change the dependency. conda skeleton (which is what I typically use to generate a recipe if I can) looks at install_requires.
There was a problem hiding this comment.
It seems like putting bleach in install_requires would then require continuum to make and host a bleach package in their standard library. I think of the setuptools install_requires as translating to the conda package run requirements. Is that correct?
There was a problem hiding this comment.
So we probably should not blithely add install_requires dependencies even if they trivially install in the pip / virtualenvs world.
There was a problem hiding this comment.
I can ask them about it -- they might be willing to add bleach if we need it.
|
@embray - see new commits. |
There was a problem hiding this comment.
I find this kind of confusing... I think it would make more sense if xml_escaping just always requires bleach and raises an ImportError if this context manager is used without it installed. Then in the HTML reader's __init__ immediately raise an exception if raw_html_cols is given without bleach installed. That way it becomes immediately obvious that that specific option cannot be used, rather than failing with an exception during writing (which doesn't make it as immediately clear that this is due to the use of raw_html_cols).
There was a problem hiding this comment.
That strategy forces any user of xml_escaping to essentially pre-check whether there will be ever be raw HTML. That might be inconvenient if the code doesn't have available prior knowledge of this information in order to raise an exception at a convenient point. I think this strategy leads to code like:
# Pre-check (somehow...)
if check_if_user_wants_raw_html_anywhere(...):
try:
import bleach
except ImportError:
raise ImportError('you need bleach if you want raw HTML')
# Later on, possibly many layers deep in the code
user_wants_raw_html_here = ...
if user_wants_raw_html_here:
# Allow raw HTML (disable brute force HTML escaping) but use bleach.clean()
# to sanitize dangerous tags
with w.xml_escaping(False, raw_html_clean_kwargs):
w.start('td')
w.data(some_data)
w.end(indent=False)
else:
# Use brute-force escaping, but user might not have bleach installed,
# cannot use w.xml_escaping(True, ..)
w.start('td')
w.data(el.strip())
w.end(indent=False)
There was a problem hiding this comment.
Would it be less confusing if the context manager were named allow_raw_xml()?
There was a problem hiding this comment.
The way I'm seeing it, it's just an internal utility (doesn't even have a docstring) so it doesn't matter if it doesn't work if its dependencies aren't met.
It just seems strange to me to have a function that takes a flag that basic means "this should work" or "this shouldn't work".
There was a problem hiding this comment.
Good point that it should have a docstring.
so it doesn't matter if it doesn't work if its dependencies aren't met.
By "doesn't work", do you mean raises an exception? That would be a problem in the current use case.
It just seems strange to me to have a function that takes a flag that basic means "this should work" or "this shouldn't work".
I'm not sure what you mean by "shouldn't work". The flag means:
True: Use the default fast brute-force sanitizing inXMLWriterby replacing a certain set of characters. No XML will survive.False: Use nuanced (slower) sanitizing by actually parsing and cleaning the XML with bleach. Whitelisted XML will survive. We've agreed that if the user wants any whitelisted XML via this option that they must have bleach installed.
There was a problem hiding this comment.
I see. I didn't get that before. Maybe then that option should just be explicitly called use_bleach or something :)
There was a problem hiding this comment.
Or def select_xml_cleaning_method(method='all', **kwargs). Then method could be whitelist in which case it would bleach, or method='all' wherein it uses the default brute-force way.
There was a problem hiding this comment.
To make things even more clear, the default could be escape_xml_characters, with the other option being allow_whitelisted_tags.
|
This functionality was requested again today by @hcferguson , in addition to the original request in #4006, for a completely separate project. Any chance, we can finalize the discussions here, rebase, and get this merged relatively soon? |
f830b9b to
87befc5
Compare
|
OK, rebased, passing tests. I tried to address concerns from @embray. I'm not sure this is perfect but I think this is a workable solution. |
|
I used the "squash and merge" option here. Somehow it added me to the commit authors (see the commit history on master). If this is not desirable, we should indeed avoid this feature as @eteq suggested and ask the PR author to manually squash before merging. FYI. |
|
Hmm. I thought it was only a problem if the PR needs to be backported, which is not the case for this one, right? I apologize for any inconvenience caused. |
|
@pllim - it might be ok here, but problem is that sometimes we change what's a backport and what isn't, and when it should be backported. And now the changelog does not reflect the PR the commit originally came from. So it makes life a fair bit harder for the release manager. It also might have subtle side effects regarding some of our contributor statistics. While it's arguable whether the commits should be squashed in some cases, we definitely do always want a merge commit no matter what, even if before the merge you squash the commits by hand. So for now we just shouldn't use the squash-and-merge in the core at all - lesson learned! |
|
I'm sorry... |
|
Just a misunderstanding, no worries, @pllim! |
@eteq - I think someone with owner rights can disable it all together. |
|
Also... once you have done it, that "squash away" button becomes the default when you look at other PRs. Be warned! |
|
Good idea, @bsipocz ! Thanks, @astrofrog ! |
|
Hi there @taldcroft 👋 - thanks for the pull request! I'm just a friendly 🤖 that checks for issues related to the changelog and making sure that this pull request is milestoned and labelled correctly. This is mainly intended for the maintainers, so if you are not a maintainer you can ignore this, and a maintainer will let you know if any action is required on your part 😃. Everything looks good from my point of view! 👍 If there are any issues with this message, please report them here |
7 participants
Per discussion in #4006, this PR adds the
raw_html_colsparameter to thehtmldictinput of theio.asciiHTML writer. One can supply a single column name or a list of columns.This does not have any understanding of tags like
<script>etc that might be disallowed from being passed through as raw HTML. Note that one has to specifically opt-in for each column, so this isn't something where one could read a FITS table from someone, display it naively, and have something bad happen. But I appreciate taking security seriously. @embray? Is there an OTS way to detect and trap certain XML tags?