phantom-render-stream icon indicating copy to clipboard operation
phantom-render-stream copied to clipboard

wish: document implications of re-using phantom processes

Open markstos opened this issue 11 years ago • 5 comments

If one request sets a cookie, and a future request re-uses the same phantom pool member, doesn't that mean the future request would be using the cookies from the previous request? If that's the case, the security implications of re-using browser processes should be documented.

markstos avatar May 25 '14 13:05 markstos

When using the --debug=true option, I noticed this:

 2014-05-26T00:20:44 [DEBUG] CookieJar - Created but will not store cookies (use option '--cookies-file=<filename>' to enable persisten cookie storage)

It's still not clear whether this means that cookie's won't be stored from request-to-request, or whether it's only communicating that they won't be stored beyond when the Phantom process closes.

markstos avatar May 26 '14 00:05 markstos

Related, but what if I wish to pass a specific cookie to the renderer? I wrote my own version of this utility, but am looking at this repo b/c the spawn/stream and cached phantom instance will hopefully make the performance a bit more tolerable. Right now, I pass the cookie string into my phantom script and do this:

// args passed cookie domain + value
var args = require('system').args;

phantom.addCookie({
  'domain': args[1],
  'name': 'connect.sid'
  'value': args[2]
})

This is dynamic, b/c each time I spawn it, I want it to re-use the same cookie session as the original request for rendering a pdf. If I persist this to a file and use --cookies-file=cookies.txt, then as @markstos points out, that is a security leak. If you're pooling phantom members, how can we be sure they'll pick up the right cookie file?

davisford avatar Oct 16 '14 04:10 davisford

I forked and added experimental support for passing cookies through b/c I have an immediate need for this, and wanted to try it out. If there is any interest from the maintainers, I can try to submit a PR -- with a test.

The security implications of such could be somewhat mitigated by setting the expiry time to something very low (e.g. 30 sec).

davisford avatar Oct 16 '14 16:10 davisford

Since setting the cookies is optional, I'm in favor of this addition. (I'm a co-user, not a maintainer). I reviewed the pull request. My only addition would more documentation to explain the feature and security implications. As I understand, an alternative way to set cookies is to point Phantomjs at ".js" file which calls the addCookie method

markstos avatar Oct 16 '14 17:10 markstos

The alternative way to pass cookies to PhantomJS is to indicate it via the command line arg: --cookies-file=/path/to/cookies.txt. If you specify that, it will write them to a file. If the file exists, it will set them on any initial requests.

You should be able to pass them through this way already, as it seems phantom-render-stream just concats the args it receives onto the command line.

However, this way (cookies.txt) is not ideal for me. The file is binary encoded in some kind of homegrown C++ serialization CookieJar, so it is not readable or editable without PhantomJS. In my app, I pass a request from client to server to generate a PDF of a particular URL which the user has already authenticated for, so I have the cookie in hand, and I can pass it into phantom-render-stream through the opts but I cannot via cookies.txt -- unless I pre-create it using another script to walk through the login.

davisford avatar Oct 16 '14 20:10 davisford