Skip to content

display() should escape by default#915

Merged
antocuni merged 3 commits into
mainfrom
antocuni/display-should-escape
Nov 3, 2022
Merged

display() should escape by default#915
antocuni merged 3 commits into
mainfrom
antocuni/display-should-escape

Conversation

@antocuni

@antocuni antocuni commented Nov 2, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

fpliger commented Nov 2, 2022

Copy link
Copy Markdown
Contributor

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

antocuni commented Nov 2, 2022

Copy link
Copy Markdown
Contributor Author

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

fpliger commented Nov 2, 2022

Copy link
Copy Markdown
Contributor

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

antocuni commented Nov 2, 2022

Copy link
Copy Markdown
Contributor Author

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

fpliger commented Nov 2, 2022

Copy link
Copy Markdown
Contributor

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

antocuni commented Nov 3, 2022

Copy link
Copy Markdown
Contributor Author

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.

3 participants

@antocuni @fpliger @tedpatrick