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

Request to limit scope of functions in validator.js #13026

Closed
kienstra opened this issue Jan 25, 2018 · 5 comments
Closed

Request to limit scope of functions in validator.js #13026

kienstra opened this issue Jan 25, 2018 · 5 comments

Comments

@kienstra
Copy link

kienstra commented Jan 25, 2018

What's the issue?

It seems that functions in validator.js are escaping into the global namespace.

How do we reproduce the issue?

  1. Navigate to an AMP page like this, with #development=1 appended to the URL.
  2. Open the browser's console, and enter wp.
  3. Expected: wp is not defined
  4. Actual: wp is a function:
ƒ wp(a){rp.call(this);this.name=a;this.value=[];this.b=!1;this.o=34}

It looks like this script leaks functions into the global namespace:

https://cdn.ampproject.org/v0/validator.js

There are other functions leaked, like aa.

Maybe that script is intended for development only, as it seems that it's not requested if #development=1 isn't appended. But it would really help us with this issue on the AMP WordPress plugin:

ampproject/amp-wp#843

We're working on validating AMP in the WordPress editor.

What browsers are affected?

Chrome 63.0.3239.132

Which AMP version is affected?

1516337355291

Thanks a lot for your help 😄

/cc @amedina, @Gregable, @pbakaus

@Gregable
Copy link
Member

I think we can get a fix out for this fairly quickly. @powdercloud has a prototype, but we need to get it through a release. Thanks for the report.

@kienstra
Copy link
Author

Thanks a lot, @Gregable! That would really help.

@ampprojectbot
Copy link
Member

This is a high priority issue but it hasn't been updated in awhile. @Gregable Do you have any updates?

@powdercloud
Copy link
Contributor

Yeah this is done. I just confirmed by looking at https://cdn.ampproject.org/v0/validator.js and verifying that the whole thing is wrapped into a javascript scope with the usual pattern. Please reopen / complain if you see further issues.

@kienstra
Copy link
Author

Thank You

Hi @powdercloud and @Gregable,
Thanks a lot for fixing this. As you mentioned, validator.js is wrapped in a function, and functions like wp aren't leaked.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants