Skip to content
This repository has been archived by the owner on May 30, 2023. It is now read-only.

Don't change the URL of the main execution context on phantom::exit #12720

Closed
wants to merge 1 commit into from

Conversation

vitallium
Copy link
Collaborator

We shouldn't change the URL of the main execution context. Because this will change security policy of our main frame which leads to the following message: Unsafe JavaScript attempt to access frame with URL about:blank from frame with URL file://bla.js. Domains, protocols and ports must match.

We can fix it by isolating main frame of PhantomJS execution context. We can destroy it separately.

This fixes: #12697 #12660

@vitallium vitallium changed the title We shouldn't change the URL of the main execution context. Don't change the URL of the main execution context on phantom::exit Nov 6, 2014
We shouldn't change the URL of the main execution context.
Because this will change security policy of our main frame which leads to the following message: `Unsafe JavaScript attempt to access frame with URL about:blank from frame with URL file://bla.js. Domains, protocols and ports must match.`

We can fix it by isolating main frame of PhantomJS execution context. We can destroy it separately.
@mickaelandrieu
Copy link
Contributor

👍 thank you

@GrahamCampbell
Copy link

👍

@JamesMGreene
Copy link
Collaborator

LGTM. 👍

@vitallium
Copy link
Collaborator Author

@ariya ping

@mickaelandrieu
Copy link
Contributor

@ariya also, can you publish new release with this? it's a very annoying issue :/

@ariya
Copy link
Owner

ariya commented Nov 14, 2014

@vitallium The URL change is part of the backport of the fix in #12431. I know it might not be needed on Windows, but maybe there was a reason it's needed in Linux? @milianw Do you still recall the reason for that?

@ariya
Copy link
Owner

ariya commented Nov 14, 2014

@mickaelandrieu It's unlike there will be 1.9.9 release. 1.9.8 was an exception due to SSLv3/POODLE. Doing a release is costly and we'd rather spend more time finishing PhantomJS 2.

Note that you can always build PhantomJS yourself. It doesn't take more than 30 minutes.

laurelnaiad added a commit to laurelnaiad/marklogic-samplestack that referenced this pull request Nov 15, 2014
laurelnaiad added a commit to laurelnaiad/marklogic-samplestack that referenced this pull request Nov 15, 2014
ariya/phantomjs#12720

Note: **npm package** phantomjs 1.9.11 is last to use phantomjs
binary 1.9.7.
@ariya
Copy link
Owner

ariya commented Nov 16, 2014

@vitallium I will land this anyway on 1.9 branch.

@vitallium
Copy link
Collaborator Author

@ariya I don't think this PR breaks Phantom::exit on Linux or OS X. I was wondering why we were adding the main (host) page to the others. I think it's a bit odd.

@ariya
Copy link
Owner

ariya commented Nov 16, 2014

@vitallium I think it's historical baggage.
Anyway, it's landed now. Thank you!

@notedit
Copy link

notedit commented Dec 2, 2014

is this merged into master ?

@alexserver
Copy link

so this PR was closed, but was this merged to master ? I'm still having the problem with version 1.9.16

@tedtate
Copy link

tedtate commented Mar 13, 2015

@alexserver I have the same issue. 1.9.16 is the version of the node wrapper for Phantom (https://github.com/Medium/phantomjs) which currently uses the last release of the 1.9 branch of Phantom. Unfortunately, the latest release (1.9.8) doesn't include this fix and it seems like there aren't any plans to do another release on the 1.9 branch.

If you use 1.9.11 of the node wrapper it should fix the issue.

@alexserver
Copy link

@tedtate Oh I understand, it is confusing to see this between the node wrapper and the phantomjs binary. thanks man.

captn3m0 added a commit to sdslabs/phoenix that referenced this pull request Mar 27, 2015
@vitallium vitallium deleted the fix-unsafe-js-access branch August 23, 2015 18:00
@khebbie
Copy link

khebbie commented Apr 23, 2016

Hello!

You have a new message, please read http://find-it-usa.com/boil.php?mx

klaus@hebsgaard.dk

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

Successfully merging this pull request may close these issues.

None yet

9 participants