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

Add sandbox attribute to omid_v1_present iframe #31

Closed
wants to merge 1 commit into from

Conversation

bjoberg
Copy link

@bjoberg bjoberg commented Jun 20, 2023

Resolves #30

@nlehrer
Copy link

nlehrer commented Jun 20, 2023

Thanks, Brett. What we will do is merge this into the upstream repo and then it'll end up here after the next release. I'll let you know once it is merged and then we can close this PR. If you want to contribute more to the upstream repo you can join the commit group (email [email protected] for more info).

@bjoberg
Copy link
Author

bjoberg commented Jun 20, 2023

Sounds good @nlehrer, thanks for clarifying. Is there a release schedule I can follow?

@nlehrer
Copy link

nlehrer commented Jun 20, 2023

There is a release about once every two weeks. We haven't published an official release schedule, but I can ask about that possibility. We're planning to do a release sometime this week.

@bjoberg
Copy link
Author

bjoberg commented Jun 22, 2023

@nlehrer has then been merged into the upstream repo and should I close this PR?

@@ -66,6 +67,7 @@ exports.appendPresenceIframe_ = function(globalObject) {
iframe.id = exports.OMID_PRESENT_FRAME_NAME;
iframe.name = exports.OMID_PRESENT_FRAME_NAME;
iframe.style.display = 'none';
iframe.sandbox = undefined;
Copy link

Choose a reason for hiding this comment

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

FYI setting this to undefined causes the browser to complain; setting it to empty string seems to be the right way to set it to just "sandbox".

Copy link
Author

Choose a reason for hiding this comment

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

Oh interesting... Thanks for the follow up. Want me to make this change, or will you do it in the other repo?

Copy link

Choose a reason for hiding this comment

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

I did it in the other repo PR. Thanks.

@nlehrer
Copy link

nlehrer commented Jun 22, 2023

It's going to be merged today or tomorrow (will keep you updated). I'm closing this one meanwhile. Thanks!

@nlehrer nlehrer closed this Jun 22, 2023
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.

omid_v1_present iframe missing sandbox attribute
2 participants