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

Enhance HTML escaping #1

Merged
merged 2 commits into from Mar 24, 2020
Merged

Enhance HTML escaping #1

merged 2 commits into from Mar 24, 2020

Conversation

richter-alex
Copy link
Member

@richter-alex richter-alex commented Mar 18, 2020

Enhances HTML escaping by accounting for < and /. This helps to further avoid cases where these HTML characters inputted into Hypernova are not escaped, leading to downstream breakages in places like JSON.parse().

npm test comes back green.

This is the first step toward packaging these changes into a new version for private release on package cloud.

@richter-alex richter-alex changed the title Escape closing tag chevrons Enhance HTML escaping Mar 18, 2020
@richter-alex richter-alex marked this pull request as ready for review March 18, 2020 20:03
Copy link

@JackMc JackMc left a comment

Choose a reason for hiding this comment

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

This looks good to me.

One comment: would it be worth rewriting

hypernova/src/index.js

Lines 46 to 49 in a29f81b

function toScript(attrs, data) {
const dataAttributes = Object.keys(attrs).map(name => makeValidDataAttribute(name, attrs[name]));
return `<script type="application/json" ${dataAttributes.join(' ')}>${LEFT}${encode(data)}${RIGHT}</script>`; // eslint-disable-line max-len
}
in terms of DOM APIs instead of string interpolation? It's generally a bad pattern to construct HTML using string interpolation. Maybe in another PR:)

@richter-alex
Copy link
Member Author

I agree, we can roll that into the next release as a quick win through another PR before putting it up.

Copy link

@jamesism jamesism left a comment

Choose a reason for hiding this comment

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

code LGTM

@richter-alex richter-alex merged commit d38d725 into master Mar 24, 2020
@richter-alex richter-alex deleted the better-html-escaping branch March 24, 2020 02:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants