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

WIP: remove unsafe script #201

Closed
wants to merge 4 commits into from
Closed

WIP: remove unsafe script #201

wants to merge 4 commits into from

Conversation

shashi
Copy link
Member

@shashi shashi commented Sep 13, 2018

I'm having the following problem here:

screenshot from 2018-09-13 17-51-29
@piever this situation arises with any simple @manipulate like

screenshot from 2018-09-13 17-56-57

@manipulate for i=1:10
   i
end

I'm didn't dig deep into why, but I feel like it probably shouldn't?

closes #200 when done

@SimonDanisch
Copy link
Member

unsafe-script seemed like it definitely makes problems

"""<unsafe-script style='display:none'>
WebIO.mount(this.previousSibling,""")
# NOTE: do NOT add space between </div> and <unsafe-script>
write(io, escapeHTML(sprint(s->jsexpr(s, x))))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why don't we need escapeHTML any more?

Copy link
Member Author

Choose a reason for hiding this comment

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

things in a <script> tag get special treatment.. for example, & is actually JS syntax but &amp; is not and will cause a parse error 🙃

I think I figured out how to fix the issue here.

@piever
Copy link
Collaborator

piever commented Sep 14, 2018

It'd be great to get rid of unsafe-script. I'm not too sure I understand what's going on with this PR, but I think the initial behavior is problematic and seems like some issue with escaping?

function encode_scripts(htmlstr::String)
# sometimes a </script> can be inside a string inside a string inside a string...
# we need to go deeper....
replace(htmlstr, r"</(_*)script>" => x->replace(x, "/"=>"/_"))
Copy link
Member Author

Choose a reason for hiding this comment

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

@JobJob this hack was superb. But not ugly enough. 🤣

Copy link
Member

Choose a reason for hiding this comment

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

Lol, what's with the _*?

Copy link
Member Author

Choose a reason for hiding this comment

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

So it will match anything like </script>, </_script>, </__script> and add another underscore...

There's sometimes code that will call setInnerHtml which will in turn call setInnerHtml etc, so you need to escape just enough.

@shashi
Copy link
Member Author

shashi commented Sep 18, 2018

Ok my failing example works now, but this doesn't work for the initial output...

@manipulate for i=1:10
    i
    @manipulate for j=1:10
        i,j
    end
end

It might have to do with some code I edited in WebIO.render of Observable. But my edits look correct to me (there was some kind of double rendering going on before -- the nested observable value was both printed and serialized with the scope).

@piever
Copy link
Collaborator

piever commented Sep 18, 2018

As a general comment, it's probably important to add extensive comments in the code explaining how the whole mechanism works, esp. wrt escaping and the various hacks, otherwise it's going to be an adventure the next time it needs to be updated!

@shashi shashi closed this Nov 2, 2018
@shashi shashi deleted the s/no-unsafe-script branch November 2, 2018 23:15
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.

Get rid of unsafe-script
4 participants