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

Escape IOMD files (fixes #1416) #2193

Merged
merged 3 commits into from
Aug 31, 2019
Merged

Escape IOMD files (fixes #1416) #2193

merged 3 commits into from
Aug 31, 2019

Conversation

ZiyaoWei
Copy link
Contributor

@ZiyaoWei ZiyaoWei commented Aug 28, 2019

Sorry for the turnaround time!

This fixes #1416 by:

  1. change IOMD content in the notebook template to be escaped instead of marked safe;
  2. unescape the rendered content on load in JS.

Tests have been added to reflect the changes. This is kind of use visible but is a bit of an edge case so will update CHANGELOG.md if you think it's warranted.

LMK if there's a better approach!

Pull Request checklist

  • Documentation: If this feature has or requires documentation, the relevant docs have been updated.
  • Changelog: This PR updates the changelog with any user-visible changes.
  • Tests: This PR includes thorough tests or an explanation of why it does not

Copy link
Contributor

@wlach wlach left a comment

Choose a reason for hiding this comment

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

This is great, thank you! I have some nits but the implementation looks technically correct.

@wlach
Copy link
Contributor

wlach commented Aug 30, 2019

BTW, yes, this change seems worth mentioning in the changelog! Please add an entry.

@ZiyaoWei
Copy link
Contributor Author

Turns out I completely forgot about the preview window on the right and the script in md and html sections are not showing up there. Will look into this - should that be a separate PR (apologies for random questions - still trying to understand Iodide's dev process)?

@wlach
Copy link
Contributor

wlach commented Aug 31, 2019

Definitely we would want this to work end-to-end before merging so please go ahead and make additions to this PR.

Actually thinking about it more, I don't think script tags were ever truly supported as part of md cells. I am not sure if we should ever allow them -- possibly not. For now I'd say we can merge this as-is, assuming it works (haven't tested it yet).

Copy link
Contributor

@wlach wlach left a comment

Choose a reason for hiding this comment

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

Tested this, it works well! Thank you so much for this PR. I'm not sure how much time or where your exact interests with iodide are, but if you're looking for something on the frontend which might be an incremental step up in difficulty from this one, maybe try: #2175

@wlach wlach merged commit b4c7609 into iodide-project:master Aug 31, 2019
@ZiyaoWei ZiyaoWei deleted the wzy/escapeIomd branch September 1, 2019 12:27
@ZiyaoWei
Copy link
Contributor Author

ZiyaoWei commented Sep 2, 2019

Thanks for the review @wlach! Don't have a specific goal in mind, just want to help build out the next great programming environment :-) Jokes aside, I would be happy and excited to take a look at #2175 although since iodide is completely weekend and nights for me can't promise a timeline. I might also look into compiling some libraries with pyodide and write some notebooks and I'll let you know if I went into that rabbit hole so someone else could take that issue.

@ZiyaoWei
Copy link
Contributor Author

ZiyaoWei commented Sep 2, 2019

Also FWIW preview rendering seems to be working if I add _.escape to report-pane.jsx - just want to leave a trace here in case you want to revisit supporting that.

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.

iomd is not HTML-escaped, allowing arbitrary code execution
2 participants