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

Skip signature check for fake ad type #10863

Merged
merged 2 commits into from Aug 10, 2017
Merged

Conversation

zhouyx
Copy link
Contributor

@zhouyx zhouyx commented Aug 9, 2017

fix the fake ad type, also our /a4a/ proxy

@taymonbeal
Copy link
Member

Can you tell me how we ensure that this can only occur when the ad creative is trustworthy?

@zhouyx
Copy link
Contributor Author

zhouyx commented Aug 10, 2017

is type=fake not enough? I can add another getMode().localDev check, would that be sufficient?

@taymonbeal
Copy link
Member

Yeah, I think the previous version of the code had such a check in an equivalent place.

@zhouyx
Copy link
Contributor Author

zhouyx commented Aug 10, 2017

Added extra getMode().localDev() check

@@ -662,6 +662,11 @@ export class AmpA4A extends AMP.BaseElement {
})
.then(status => {
this.protectedEmitLifecycleEvent_('adResponseValidateEnd');
if (getMode().localDev &&
Copy link
Member

Choose a reason for hiding this comment

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

I think you might want these to be nested instead of joined by &&, to ensure that the block is compiled out of production builds. But I could be wrong; I don't know much about how that compiler pass works. Maybe ask Erwin.

Copy link
Contributor Author

@zhouyx zhouyx Aug 10, 2017

Choose a reason for hiding this comment

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

@taymonbeal I tried compiled with gulp dist. The block is being excluded even not nested.

@zhouyx zhouyx merged commit d84bd1f into ampproject:master Aug 10, 2017
@zhouyx zhouyx deleted the fix-fake-a4a-ad branch August 10, 2017 01:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants