Skip to content

Check iframe sandbox flags such as (allow-same-origin) in a case-insensitive way#1276

Merged
camelburrito merged 1 commit intoampproject:masterfrom
camelburrito:iframe
Jan 5, 2016
Merged

Check iframe sandbox flags such as (allow-same-origin) in a case-insensitive way#1276
camelburrito merged 1 commit intoampproject:masterfrom
camelburrito:iframe

Conversation

@camelburrito
Copy link
Copy Markdown
Contributor

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I know it is a bit hypocritical because of the usage of this.element, but I passed the sandbox as an argument to make this more of a pure function. No need for it to depend on some magiv instance var. Much easier to reason about then.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This regex test is weird, and it won't allow insensitive matches. I think it'd be better to normalize this.sandbox_ into a string, then use !(/\s*allow-same-origin\s*/i).test(this.sandbox_).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It is lower cased above, but that must be at least proven by a test and probably better to just redo locally here.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ah, I didn't see the #lowerCase. Still, the ' ' + this.sandbox_ + ' ' bothers me.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Its stolen from jQuery's hyper optimized polyfill for hasClass :)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@cramforce
Copy link
Copy Markdown
Member

Please add a test.

@camelburrito camelburrito force-pushed the iframe branch 3 times, most recently from e080b31 to ff1dc5a Compare January 4, 2016 23:15
@camelburrito
Copy link
Copy Markdown
Contributor Author

PTAL

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

But this also needs the toLowerCase treatment, no?

@camelburrito
Copy link
Copy Markdown
Contributor Author

PTAL - all i had to do was add the /i to the regex - tests already exist - just tweaked one test.

@cramforce
Copy link
Copy Markdown
Member

LGTM

camelburrito pushed a commit that referenced this pull request Jan 5, 2016
Check iframe sandbox flags such as (allow-same-origin) in a case-insensitive way
@camelburrito camelburrito merged commit d4ba2a5 into ampproject:master Jan 5, 2016
@camelburrito camelburrito deleted the iframe branch January 5, 2016 01:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants