Skip to content

Commit

Permalink
Remove guards for launched 'inline-styles' experiment (#17187)
Browse files Browse the repository at this point in the history 
* Remove 'inline-styles' experiment.

* Remove from test-fixed-layer.js.

* Remove unused imports.

* Fix lint.

* Fix tests.
  • Loading branch information
William Chou authored Aug 2, 2018
1 parent 97ef0d6 commit 38e6a3b
Show file tree
Hide file tree
Showing 7 changed files with 21 additions and 95 deletions.
2 changes: 1 addition & 1 deletion extensions/amp-mustache/0.1/test/test-amp-mustache.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ describe('amp-mustache 0.1', () => {
value: /*eslint no-script-url: 0*/ 'javascript:alert();',
});
expect(result./*OK*/innerHTML).to.equal(
'value = <a data-="" target="_top">abc</a>');
'value = <a data-="" style="color:red;" target="_top">abc</a>');
});
});

Expand Down
7 changes: 1 addition & 6 deletions src/purifier.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ import {
} from './url';
import {dict} from './utils/object';
import {filterSplice} from './utils/array';
import {isExperimentOn} from './experiments';
import {parseSrcset} from './srcset';
import {startsWith} from './string';
import {urls} from './config';
Expand Down Expand Up @@ -231,7 +230,6 @@ const PURIFY_CONFIG = {
export function purifyHtml(dirty) {
const config = Object.assign({}, PURIFY_CONFIG, {
'ADD_ATTR': WHITELISTED_ATTRS,
'FORBID_ATTR': isExperimentOn(self, 'inline-styles') ? [] : ['style'],
'FORBID_TAGS': Object.keys(BLACKLISTED_TAGS),
// Avoid reparenting of some elements to document head e.g. <script>.
'FORCE_BODY': true,
Expand Down Expand Up @@ -480,10 +478,7 @@ export function isValidAttr(tagName, attrName, attrValue, opt_purify = false) {

// Inline styles are not allowed.
if (attrName == 'style') {
if (isExperimentOn(self, 'inline-styles')) {
return !INVALID_INLINE_STYLE_REGEX.test(attrValue);
}
return false;
return !INVALID_INLINE_STYLE_REGEX.test(attrValue);
}

// Don't allow CSS class names with internal AMP prefix.
Expand Down
16 changes: 5 additions & 11 deletions src/service/fixed-layer.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ import {
} from '../style';
import {dev, user} from '../log';
import {endsWith} from '../string';
import {isExperimentOn} from '../experiments';

const TAG = 'FixedLayer';

Expand Down Expand Up @@ -470,13 +469,11 @@ export class FixedLayer {
* @private
*/
warnAboutInlineStylesIfNecessary_(element) {
if (isExperimentOn(this.ampdoc.win, 'inline-styles')) {
if (element.hasAttribute('style')
&& (element.style.top || element.style.bottom)) {
user().error(TAG, 'Inline styles with `top`, `bottom` and other ' +
'CSS rules are not supported yet for fixed or sticky elements ' +
'(#14186). Unexpected behavior may occur.', element);
}
if (element.hasAttribute('style')
&& (element.style.top || element.style.bottom)) {
user().error(TAG, 'Inline styles with `top`, `bottom` and other ' +
'CSS rules are not supported yet for fixed or sticky elements ' +
'(#14186). Unexpected behavior may occur.', element);
}
}

Expand Down Expand Up @@ -874,9 +871,6 @@ class TransferLayerShadow {
* @param {!Document} doc
*/
constructor(doc) {
/** @private @const {!Document} */
this.doc_ = doc;

/** @private @const {!Element} */
this.layer_ = doc.createElement('div');
this.layer_.id = 'i-amphtml-fixed-layer';
Expand Down
7 changes: 0 additions & 7 deletions test/functional/test-fixed-layer.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ import {AmpDocSingle} from '../../src/service/ampdoc-impl';
import {FixedLayer} from '../../src/service/fixed-layer';
import {endsWith} from '../../src/string';
import {installPlatformService} from '../../src/service/platform-impl';
import {toggleExperiment} from '../../src/experiments';
import {user} from '../../src/log';


Expand Down Expand Up @@ -1031,9 +1030,6 @@ describes.sandboxed('FixedLayer', {}, () => {
});

it('should user error when inline styles may be overriden', () => {
toggleExperiment(ampdoc.win, 'inline-styles', true,
/* opt_transientExperiment */ true);

// Set both attribute and property since element1 is a fake element.
element1.setAttribute('style', 'bottom: 10px');
element1.style.bottom = '10px';
Expand Down Expand Up @@ -1311,9 +1307,6 @@ describes.sandboxed('FixedLayer', {}, () => {
});

it('should user error when inline styles may be overriden', () => {
toggleExperiment(ampdoc.win, 'inline-styles', true,
/* opt_transientExperiment */ true);

// Set both attribute and property since element1 is a fake element.
element1.setAttribute('style', 'bottom: 10px');
element1.style.bottom = '10px';
Expand Down
54 changes: 14 additions & 40 deletions test/functional/test-purifier.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ import {
rewriteAttributeValue,
rewriteAttributesForElement,
} from '../../src/purifier';
import {toggleExperiment} from '../../src/experiments';

/**
* Helper that serializes output of purifyHtml() to string.
Expand Down Expand Up @@ -181,7 +180,6 @@ function runSanitizerTests() {
it('should NOT output security-sensitive markup', () => {
expect(purify('a<script>b</script>c')).to.be.equal('ac');
expect(purify('a<script>b<img>d</script>c')).to.be.equal('ac');
expect(purify('a<style>b</style>c')).to.be.equal('ac');
expect(purify('a<img>c')).to.be.equal('ac');
expect(purify('a<iframe></iframe>c')).to.be.equal('ac');
expect(purify('a<frame></frame>c')).to.be.equal('ac');
Expand Down Expand Up @@ -306,26 +304,20 @@ function runSanitizerTests() {
});

it('should NOT output security-sensitive attributes', () => {
allowConsoleError(() => {
expect(purify('a<a onclick="alert">b</a>')).to.be.equal(
'a<a>b</a>');
expect(purify('a<a style="color: red;">b</a>')).to.be.equal(
'a<a>b</a>');
expect(purify('a<a STYLE="color: red;">b</a>')).to.be.equal(
'a<a>b</a>');
expect(purify('a<a href="javascript:alert">b</a>')).to.be.equal(
'a<a target="_top">b</a>');
expect(purify('a<a href="JAVASCRIPT:alert">b</a>')).to.be.equal(
'a<a target="_top">b</a>');
expect(purify('a<a href="vbscript:alert">b</a>')).to.be.equal(
'a<a target="_top">b</a>');
expect(purify('a<a href="VBSCRIPT:alert">b</a>')).to.be.equal(
'a<a target="_top">b</a>');
expect(purify('a<a href="data:alert">b</a>')).to.be.equal(
'a<a target="_top">b</a>');
expect(purify('a<a href="DATA:alert">b</a>')).to.be.equal(
'a<a target="_top">b</a>');
});
expect(purify('a<a onclick="alert">b</a>')).to.be.equal(
'a<a>b</a>');
expect(purify('a<a href="javascript:alert">b</a>')).to.be.equal(
'a<a target="_top">b</a>');
expect(purify('a<a href="JAVASCRIPT:alert">b</a>')).to.be.equal(
'a<a target="_top">b</a>');
expect(purify('a<a href="vbscript:alert">b</a>')).to.be.equal(
'a<a target="_top">b</a>');
expect(purify('a<a href="VBSCRIPT:alert">b</a>')).to.be.equal(
'a<a target="_top">b</a>');
expect(purify('a<a href="data:alert">b</a>')).to.be.equal(
'a<a target="_top">b</a>');
expect(purify('a<a href="DATA:alert">b</a>')).to.be.equal(
'a<a target="_top">b</a>');
});

it('should NOT output blacklisted values for class attributes', () => {
Expand Down Expand Up @@ -433,30 +425,12 @@ function runSanitizerTests() {
.to.be.equal('ac');
});

it('should output style attributes if inline styles enabled', () => {
toggleExperiment(self, 'inline-styles', true,
/* opt_transientExperiment */ true);
expect(purifyTagsForTripleMustache(
'<b style="color: red">abc</b>'))
.to.be.equal('<b style="color: red">abc</b>');
});

it('should compensate for broken markup', () => {
expect(purifyTagsForTripleMustache('<b>a<i>b')).to.be.equal(
'<b>a<i>b</i></b>');
});

describe('should sanitize `style` attribute', () => {
beforeEach(() => {
toggleExperiment(self, 'inline-styles', true,
/* opt_transientExperiment */ true);
});

afterEach(() => {
toggleExperiment(self, 'inline-styles', false,
/* opt_transientExperiment */ true);
});

it('should allow valid styles',() => {
expect(purify('<div style="color:blue">Test</div>'))
.to.equal('<div style="color:blue">Test</div>');
Expand Down
24 changes: 0 additions & 24 deletions test/functional/test-sanitizer.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ import {
sanitizeHtml,
sanitizeTagsForTripleMustache,
} from '../../src/sanitizer';
import {toggleExperiment} from '../../src/experiments';

describe('Caja-based', () => {
runSanitizerTests();
Expand Down Expand Up @@ -112,7 +111,6 @@ function runSanitizerTests() {
it('should NOT output security-sensitive markup', () => {
expect(sanitizeHtml('a<script>b</script>c')).to.be.equal('ac');
expect(sanitizeHtml('a<script>b<img>d</script>c')).to.be.equal('ac');
expect(sanitizeHtml('a<style>b</style>c')).to.be.equal('ac');
expect(sanitizeHtml('a<img>c')).to.be.equal('ac');
expect(sanitizeHtml('a<iframe></iframe>c')).to.be.equal('ac');
expect(sanitizeHtml('a<frame></frame>c')).to.be.equal('ac');
Expand Down Expand Up @@ -240,10 +238,6 @@ function runSanitizerTests() {
allowConsoleError(() => {
expect(sanitizeHtml('a<a onclick="alert">b</a>')).to.be.equal(
'a<a>b</a>');
expect(sanitizeHtml('a<a style="color: red;">b</a>')).to.be.equal(
'a<a>b</a>');
expect(sanitizeHtml('a<a STYLE="color: red;">b</a>')).to.be.equal(
'a<a>b</a>');
expect(sanitizeHtml('a<a href="javascript:alert">b</a>')).to.be.equal(
'a<a target="_top">b</a>');
expect(sanitizeHtml('a<a href="JAVASCRIPT:alert">b</a>')).to.be.equal(
Expand Down Expand Up @@ -355,30 +349,12 @@ function runSanitizerTests() {
.to.be.equal('ac');
});

it('should output style attributes if inline styles enabled', () => {
toggleExperiment(self, 'inline-styles', true,
/* opt_transientExperiment */ true);
expect(sanitizeTagsForTripleMustache(
'<b style="color: red">abc</b>'))
.to.be.equal('<b style="color: red">abc</b>');
});

it('should compensate for broken markup', () => {
expect(sanitizeTagsForTripleMustache('<b>a<i>b')).to.be.equal(
'<b>a<i>b</i></b>');
});

describe('should sanitize `style` attribute', () => {
beforeEach(() => {
toggleExperiment(self, 'inline-styles', true,
/* opt_transientExperiment */ true);
});

afterEach(() => {
toggleExperiment(self, 'inline-styles', false,
/* opt_transientExperiment */ true);
});

it('should allow valid styles',() => {
expect(sanitizeHtml('<div style="color:blue">Test</div>'))
.to.equal('<div style="color:blue">Test</div>');
Expand Down
6 changes: 0 additions & 6 deletions tools/experiments/experiments.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -241,12 +241,6 @@ const EXPERIMENTS = [
spec: 'https://github.com/ampproject/amphtml/issues/16465',
cleanupIssue: 'https://github.com/ampproject/amphtml/issues/16466',
},
{
id: 'inline-styles',
name: 'Enables the usage of inline styles for non fixed elements',
spec: 'https://github.com/ampproject/amphtml/issues/11881',
cleanupIssue: 'https://github.com/ampproject/amphtml/issues/13595',
},
{
id: 'url-replacement-v2',
name: 'new parsing engine for url variables',
Expand Down

0 comments on commit 38e6a3b

Please sign in to comment.