Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
✨ PromoteIQ : add #20694
✨ PromoteIQ : add #20694
Changes from 8 commits
0a86224
92b072f
b9d643f
c29f100
74e445d
9f20d56
e1deb48
4cd9a7a
87e92a8
5c40e96
c682e32
cc42fdb
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh also, @aisleypay do you think this is possibly a security issue by allowing the user to embed / run their own JS through an attribute, that is then just run?
cc @lannka @cramforce
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Each of our clients have their own CDN. To enable our clients who want to use PromoteIQ on AMP-enabled pages, they have to pass that specific CDN, which includes client specific PromoteIQ logic.
The
sfcallback
is for client specific rendering. Each client can render an ad in any way they please. Our clients take our responses and render the responses in their desired format. In order for PromoteIQ to work on AMP enabled pages, the ability to allow clients to still use their custom rendering logic is key.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aisleypay Ah, thank you for the reason! That makes sense, but it definitely opens up a lot of security / Developer experience on the AMP side of things. Thus, I think it would be good to get a response from @lannka and/or @cramforce
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interesting, so you don't mind if the client is manipulating the ads or even fake clicks?
anyway, I don't see a security issue to AMP pages as we are rendering it in an iframe sandbox. It's more ad network's choice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lannka Can you clarify what you mean by 'manipulating the ads'?
As far as fake clicks go, we work closely with our clients on inventory implementation - QA and constant monitoring. Impression/click capture and monitoring is part of the PromoteIQ integration process and for the lifetime of our relationship with the client. When we see suspicious behavior we work with our clients to resolve it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got you. I guess you are probably doing more reservation ads, so you don't need to worry about pub modifying the ad contents to have click fraud.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lannka @torch2424 Indeed! Do either of you have any further questions or concerns per this thread?