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

Package cloud #181

Closed
wants to merge 4 commits into from
Closed

Package cloud #181

wants to merge 4 commits into from

Conversation

@richter-alex
Copy link

richter-alex commented Mar 24, 2020

Setting up for private hosting on package cloud

Enhance HTML escaping
@richter-alex richter-alex deleted the Shopify:package-cloud branch Mar 24, 2020
@richter-alex richter-alex restored the Shopify:package-cloud branch Mar 24, 2020
@richter-alex richter-alex deleted the Shopify:package-cloud branch Mar 24, 2020
@ljharb

This comment has been minimized.

Copy link
Collaborator

ljharb commented Mar 24, 2020

Is there a reason you need to fork this?

@ljharb ljharb added the invalid label Mar 24, 2020
@@ -6,6 +6,8 @@ const RIGHT = '-->';
const ENCODE = [
['&', '&'],
['>', '>'],
['<', '&lt;'],

This comment has been minimized.

Copy link
@ljharb

ljharb Mar 24, 2020

Collaborator

note that I’m pretty sure this isnt needed, it just increases the HTML size for no benefit.

@richter-alex

This comment has been minimized.

Copy link
Author

richter-alex commented Mar 24, 2020

Hey @ljharb!

We were under the impression that this project and hypernova-ruby weren't actively maintained due to the reduced contributions/interactions lately.

Let me give you some context:

We noticed a bug awhile ago where unescaped HTML characters were causing a JSON.parse error, breaking the JS on some of our views. The original attempt to rectify this was here: airbnb/hypernova-ruby#22

The issue is that certain browsers will take a prop like </script and try to complete the tag as described in the above pull request.

I've been looking into this one for the past little while and I guess that hypernova-ruby performs that escaping through the BlankRenderer when the hypernova server is unavailable right? So a more complete solution would be to implement more thorough escaping on hypernova and its client libraries to cover all scenarios.

tl;dr we feel we should be escaping these other characters for safety and to protect against the current breakages we're experiencing.

Looking again at the original PR I'm inclined to agree that JSON escaping would be more appropriate, but anxious to hear what you think.

@ljharb

This comment has been minimized.

Copy link
Collaborator

ljharb commented Mar 24, 2020

It seems like it would be a trivial change to just add escaping of < alone - would that address the use case?

If so, it'd be great to reopen this PR and repurpose it to achieve that, and we can do the same process on hypernova-ruby.

@richter-alex

This comment has been minimized.

Copy link
Author

richter-alex commented Mar 24, 2020

I agree, that should be trivial and would resolve the issue on our end. How do you feel about switching to JSON escaping instead as part of the implementation as outlined in airbnb/hypernova-ruby#22?

@ljharb

This comment has been minimized.

Copy link
Collaborator

ljharb commented Mar 24, 2020

I’d have to see what that involves.

For now, I’m going to move forward with #167

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.