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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃悰 Remove oldschool HTML attribrutes #7309

Merged
merged 1 commit into from Sep 1, 2016

Conversation

@AileenCGN
Copy link
Member

commented Sep 1, 2016

no issue

Uses allowedAttributes functionality of Sanitize HTML and whitelists attributes for certain tags, regarding AMP validation rules.

This PR fixes issues with inline style like border, bgcolor, align and so on.

@kirrg001

View changes

core/server/apps/amp/lib/helpers/amp_content.js Outdated
// @TODO: remove this, when Amperize supports HTML sanitizing
$('amp-iframe').removeAttr('webkitallowfullscreen');
$('amp-iframe').removeAttr('mozallowfullscreen');
$('video').children('track').remove();

This comment has been minimized.

Copy link
@kirrg001

kirrg001 Sep 1, 2016

Contributor

$('*').removeAttr('style'); is redundant now with the new logic or?

This comment has been minimized.

Copy link
@AileenCGN

AileenCGN Sep 1, 2016

Author Member

No, because of amp-*: ['*'] which would allow further amp-HTML tags and allow all remaining attributes. But style is one, that always has to be stripped out.

This comment has been minimized.

Copy link
@kirrg001

kirrg001 Sep 1, 2016

Contributor

if i remove the line, the amp_content_spec.js test is still green, that's why i wondered

This comment has been minimized.

Copy link
@AileenCGN

AileenCGN Sep 1, 2016

Author Member

I did not write a test for that specifically. Can do it quickly. It'a mostly a fallback.

This comment has been minimized.

Copy link
@kirrg001

kirrg001 Sep 1, 2016

Contributor

you can do as a separate PR, this is already green. so would like to merge now 馃憤

This comment has been minimized.

Copy link
@kirrg001

kirrg001 Sep 1, 2016

Contributor

its not green, didn't start even haha. ok then feel free to add here 馃憤

This comment has been minimized.

Copy link
@AileenCGN

AileenCGN Sep 1, 2016

Author Member

I was wondering already... Update comes in two minutes

This comment has been minimized.

Copy link
@AileenCGN

AileenCGN Sep 1, 2016

Author Member

Bummer 馃様
amp-* in allowed tags didn't work, therefore, amp-*: ['*'] in not necessary and makes also $('*').removeAttr('style') redundant. I changed it to only allow our supported or AMPs built in tags and will push these changes.

This comment has been minimized.

Copy link
@kirrg001

kirrg001 Sep 1, 2016

Contributor

supi 馃憤

馃悰 Remove oldschool HTML attribrutes
no issue

Uses `allowedAttributes` functionality of `Sanitize` HTML and whitelists attributes for certain tags, regarding
AMP validation rules.

This PR fixes issues with inline style like `border`, `bgcolor`, `align` and so on.

@AileenCGN AileenCGN force-pushed the AileenCGN:remove-oldschool-attribute-amp branch to 9cfacb7 Sep 1, 2016

@kirrg001 kirrg001 merged commit 1143631 into TryGhost:master Sep 1, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
mixonic added a commit to mixonic/Ghost that referenced this pull request Oct 28, 2016
馃悰 Remove oldschool HTML attribrutes (TryGhost#7309)
no issue

Uses `allowedAttributes` functionality of `Sanitize` HTML and whitelists attributes for certain tags, regarding
AMP validation rules.

This PR fixes issues with inline style like `border`, `bgcolor`, `align` and so on.
madfrog2047 added a commit to madfrog2047/Ghost that referenced this pull request Nov 20, 2016
馃悰 Remove oldschool HTML attribrutes (TryGhost#7309)
no issue

Uses `allowedAttributes` functionality of `Sanitize` HTML and whitelists attributes for certain tags, regarding
AMP validation rules.

This PR fixes issues with inline style like `border`, `bgcolor`, `align` and so on.

@AileenCGN AileenCGN deleted the AileenCGN:remove-oldschool-attribute-amp branch Oct 13, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can鈥檛 perform that action at this time.