Skip to content

display() should escape by default #915

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.

Sign up for GitHub

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

Jump to bottom
Merged
antocuni merged 3 commits into from
Nov 3, 2022
Merged

display() should escape by default #915

antocuni merged 3 commits into from
Nov 3, 2022

Conversation

@antocuni
Copy link
Contributor

@antocuni antocuni commented Nov 2, 2022 •
edited
Loading

Before this PR, display(some_str) and display(some_obj) used to put the result directly in the .innerHTML of the output element, without escaping.
A notable effect is that display(some_module) didn't display anything, because e.g. <module sys (built-in)> was interpreted as a tag by the HTML parser.

This PR fixes it:

  • display(some_str) escapes the string by default. This is almost always what you want
  • display(some_obj) calls repr(obj) and escapes the result. Again, it's a very sensible default
  • if you want to inject some raw HTML in the output, you can use the new HTML class: display(HTML("<p>hello</p>")).

I don't remember whether we wrote it down anywhere, but I'm pretty sure that this is what we decided in Austin when we talked in real life with @fpliger and @philippjfr .

Behavior on main:

image

Behavior with this PR:
image

Related issues:

EDIT: I also had to delete workflows/dashboard.yaml because it was failing and we are not using it anyway

@fpliger
Copy link
Contributor

fpliger commented Nov 2, 2022

I don't remember whether we wrote it down anywhere, but I'm pretty sure that this is what we decided in Austin when we talked in real life with @fpliger and @philippjfr .

Yeah, I think so... (with a few corners)

if you want to inject some raw HTML in the output, you can use the new HTML class: display(HTML("

hello

")).

Why a class and not a method?

@antocuni
Copy link
Contributor Author

antocuni commented Nov 2, 2022

if you want to inject some raw HTML in the output, you can use the new HTML class: display(HTML("hello")).

Why a class and not a method?

Because the HTML class implements the _repr_html_ method and it works out of the box:

pyscript/pyscriptjs/src/python/pyscript.py

Lines 49 to 58 in e04cf3c

class HTML:
"""
Wrap a string so that display() can render it as plain HTML
"""
def __init__(self, html):
self._html = html
def _repr_html_(self):
return self._html

I think that all the code around format_mime & co. needs a heavy refactoring/rethinking.
It might be that with some refactoring we can turn it into a function, but the user-facing API of display() and display(HTML(...)) will remain the same.

@fpliger
Copy link
Contributor

fpliger commented Nov 2, 2022

Because the HTML class implements the repr_html method and it works out of the box:
pyscript/pyscriptjs/src/python/pyscript.py

Lines 49 to 58 in e04cf3c

class HTML:
"""
Wrap a string so that display() can render it as plain HTML
"""

 def __init__(self, html): 
     self._html = html 

 def _repr_html_(self): 
     return self._html

I think that all the code around format_mime & co. needs a heavy refactoring/rethinking.
It might be that with some refactoring we can turn it into a function, but the user-facing API of display() and display(HTML(...)) will remain the same.

Yeah, agreed, we talked about that already (when chatting with @philippjfr) a few months ago when we were in Austin... we will probably need to revisit the implementation of that and [try to] keep the user-facing API unchanged..

It may be me (and may even being super pedantic).. don't really love the all caps HTML compared to html.

@antocuni
Copy link
Contributor Author

antocuni commented Nov 2, 2022

It may be me (and may even being super pedantic).. don't really love the all caps HTML compared to html.

I think that the official name of the language is HTML (uppercase) ;).
But also, in Python there is also the html module, so we cannot use that.

@fpliger
Copy link
Contributor

fpliger commented Nov 2, 2022

I think that the official name of the language is HTML (uppercase) ;).
But also, in Python there is also the html module, so we cannot use that.

Alt Text

@antocuni
Copy link
Contributor Author

antocuni commented Nov 3, 2022

Can someone approve this please?

@antocuni antocuni merged commit 6611915 into main Nov 3, 2022
@antocuni antocuni deleted the antocuni/display-should-escape branch November 3, 2022 08:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@marimeireles marimeireles Awaiting requested review from marimeireles

1 more reviewer

@tedpatrick tedpatrick tedpatrick approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Printing is susceptible to cross-site scripting vulnerabilities by default [BUG] print() doesn't output HTML tags.

4 participants

@antocuni @fpliger @tedpatrick