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.

Sign up for GitHub

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

Jump to bottom

Fix script/style escaping in App::getTag() #1137

Merged
merged 3 commits into from
May 14, 2020
Merged

Fix script/style escaping in App::getTag() #1137

merged 3 commits into from
May 14, 2020

Conversation

mvorisek
Copy link
Member

@mvorisek mvorisek commented Apr 26, 2020 •
edited
Loading

Fix #1136

@georgehristov , does it solve #1110 ?

The important fact is that style and script elements can not have child elements by definition.

@mvorisek
Copy link
Member Author

Help wanted with tests.

It should test:
a) all tags except script/style are escaped fully
b) script/body has escaped closing tags and comments

@mvorisek mvorisek changed the title Fix css escaping in App::getTag() Fix script/style escaping in App::getTag() Apr 26, 2020
@mvorisek mvorisek marked this pull request as ready for review April 26, 2020 11:54
@mvorisek mvorisek mentioned this pull request Apr 26, 2020
Closed
@mvorisek
Copy link
Member Author

@georgehristov Any feedback on this?

@mvorisek mvorisek requested a review from romaninsh May 12, 2020 06:48
@mvorisek
Copy link
Member Author

mvorisek commented May 12, 2020 •
edited
Loading

Improves PR #1111. @romaninsh ok? @DarkSide666 ping

@georgehristov
Copy link
Collaborator

The only hesitation I have on this is that I believe we should avoid hardcoding tags in the method (like script and style)

@mvorisek
Copy link
Member Author

mvorisek commented May 12, 2020 •
edited
Loading

@georgehristov isn't it by html spec? ref: https://mathiasbynens.be/notes/etago

@georgehristov
Copy link
Collaborator

Yes, agree. If getTag is intended only for html tags (and not xml tags for instance) then we are fine as long as the html spec is covered (which appears to be the case).

@atk4 atk4 deleted a comment from codecov bot May 14, 2020
DarkSide666
DarkSide666 approved these changes May 14, 2020
View reviewed changes
Copy link
Member

@DarkSide666 DarkSide666 left a comment

Choose a reason for hiding this comment

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

LGTM

@DarkSide666 DarkSide666 merged commit f772903 into atk4:develop May 14, 2020
@mvorisek mvorisek deleted the fix_css_escaping branch May 14, 2020 13:55
@ibelar ibelar mentioned this pull request May 14, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DarkSide666 DarkSide666 DarkSide666 approved these changes

@georgehristov georgehristov Awaiting requested review from georgehristov

@abbadon1334 abbadon1334 Awaiting requested review from abbadon1334

@ibelar ibelar Awaiting requested review from ibelar

@romaninsh romaninsh Awaiting requested review from romaninsh

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Proper etago escaping in style/script
3 participants
@mvorisek @georgehristov @DarkSide666