vsivsi / meteor-file-collection

Extends Meteor Collections to handle file data using MongoDB gridFS.
http://atmospherejs.com/vsivsi/file-collection
Other
159 stars 37 forks source link

X-Auth-Token cookie should be set for root path #105

Closed jdziat closed 8 years ago

jdziat commented 8 years ago

Fix for Meteor 1.3.2.4 security when uploading a file.

Noticed that my newer Meteor build did not auth correctly using your examples. It doesn't appear to be passing the X-Auth-Token the way that you are expecting it.

updated the handle_auth method to allow for new Meteor versions.

vsivsi commented 8 years ago

Hi, thanks for the PR. Can you send a link to the change in Meteor that necessitates this change? File-collection uses its own cookie parser, and cookies come from the client-side of the app, not Meteor, so I don't immediately understand how/why this change is necessary. Any additional context you can supply will speed-up my evaluation and testing before being able to accept this change.

jdziat commented 8 years ago

I just noticed when using the example implementation for the login token it actually does not ever set the X-Auth-Token anywhere in the header. I could be missing something though.

jdziat commented 8 years ago

actually use

else if req.cookies?['meteor_login_token']?
            req.meteorUserId = lookup_userId_by_token req.cookies['meteor_login_token']

instead of

else if req.headers.cookie?
             req.meteorUserId = lookup_userId_by_token req.headers.cookie.split('=')[1]
vsivsi commented 8 years ago

By "example implementation" are you referring to: https://github.com/vsivsi/meteor-file-sample-app ?

If so, then yes, it uses cookies exclusively for authentication because that is the only way to easily get links in <img> tags and such working with authentication. The cookie is set here: https://github.com/vsivsi/meteor-file-sample-app/blob/master/sample.coffee#L70

Use of the X-Auth-Token HTTP header is typically reserved for programmatic access to files (using curl, explicit XHR GET requests in client code, etc), such as illustrated by the curl commands in this section: https://github.com/vsivsi/meteor-file-collection#http-request-behaviors

You only need the cookie or the header field, but not both, to successfully authenticate.

So I'm still unclear why your proposed change is necessary and what that has to do with Meteor 1.3.2.4.

To my knowledge Meteor itself doesn't use cookies, and so I don't know why anything would have changed given that the cookie exchange is between developer (your) client code and file-collection's own route handler that contains its own cookie parser.

For me to proceed with this, you need to provide a separate minimal sample application (perhaps based on the sample app, that clearly demonstrates the issue when using Meteor 1.3.2.4.

vsivsi commented 8 years ago

As for your suggestion to use meteor_login_token as the cookie instead of (or in addition to) X-Auth-Token, I don't understand why that is necessary or desirable.

jdziat commented 8 years ago

I apologize it is not a meteor 1.3 issue.

The below line is how you configure the X-Auth-Token Correct?

Deps.autorun(function () {
      // Sending userId prevents a race condition
      Meteor.subscribe('myData', Meteor.userId());
      // $.cookie() assumes use of "jquery-cookie" Atmosphere package.
      // You can use any other cookie package you may prefer...
      $.cookie('X-Auth-Token', Accounts._storedLoginToken());
    });

the X-Auth-Token never makes it to the server. Unless I'm missing something obvious, which is possible.

jdziat commented 8 years ago

It also appears that meteor_login_token is used across a variety of projects and is enabled in a meteor project using accounts by default. I'm looking at finding a reference for you now though.

vsivsi commented 8 years ago

Yes, that is how it is done in the sample app. I think the confusion here is that both the HTTP header and the cookie are named "X-Auth-Token", and they are equivalent to file-collection.

So you can define an HTTP header field named X-Auth-Token:

GET /gridfs/fs/38a14c8fef2d6cef53c70792 HTTP/1.1
Host: www.example.com
X-Auth-Token: 3pl5vbN_ZbKDJ1ko5JteO3ZSTrnQIl5g6fd8XW0U4NQ

OR

You can define a cookie (on the client) named X-Auth-Token:

GET /gridfs/fs/38a14c8fef2d6cef53c70792 HTTP/1.1
Host: www.example.com
Cookie: X-Auth-Token=3pl5vbN_ZbKDJ1ko5JteO3ZSTrnQIl5g6fd8XW0U4NQ
vsivsi commented 8 years ago

The only mention of "meteor_login_token" in the entire Meteor project on Github is somebody referring to using it in a comment to an issue. I've never encountered it before and it doesn't seem to be referenced at all in the Meteor source code or documentation.

https://github.com/meteor/meteor/search?utf8=%E2%9C%93&q=meteor_login_token

jdziat commented 8 years ago

I'm setting the X-Auth-Token using jquery cookie on the client side. However it doesn't seem to be getting passed over to the server side. When i log the actual request I'm receiving it always shows the meteor login token. I wonder if another package is causing this issue.

vsivsi commented 8 years ago

I just tested with the sample app and Meteor 1.3.2.4 and everything works fine with cookie authentication.

You should check your client-side request headers to make sure the cookie is being included in the request. This is what my (working) request looks like:

screen shot 2016-04-28 at 10 52 30 am

jdziat commented 8 years ago

Found the issue, I had to set the header in the actual resumable.js headers section via:

myFiles.resumable.opts.headers['X-Auth-Token'] = Accounts._storedLoginToken();

vsivsi commented 8 years ago

That works but shouldn't be necessary because the resumable.js XHR requests should pick-up the cookie you set with jQuery. That is how the sample app does it without any difficulty.

You do need to be careful to make sure that auth tokens set in cookies / header state (such as in resumable) stay up-to-date with the currently authenticated user, or you can leak credentials after a user logs out and/or another user authenticates. This is why the sample app sets the cookie in a Tracker.autoRun() callback function, so it will rerun if the (reactive) state of Meteor.userId changes.

jdziat commented 8 years ago

Strange I wonder what is different between the two setups

vsivsi commented 8 years ago

It definitely works. Here's an HTTP POST request generated by resumable in the sample app:

screen shot 2016-04-28 at 12 56 57 pm

jdziat commented 8 years ago

Yea, and it looks like the meteor-login-token was from the fast-render package.

jdziat commented 8 years ago

Needs to have the correct path setup or else resumable will not pick it up.

`$.cookie('x-auth-token', Accounts._storedLoginToken(), { path: '/' });``

vsivsi commented 8 years ago

Hmmm, the sample app uses jQuery to set the cookie and doesn't need to specify the path. I assume the root is the default... Also, cookie names are case sensitive (unlike HTTP request field names) so you're going to need to literally use "X-Auth-Token".

jdziat commented 8 years ago

It's one level above where it gets called from. In my app i have /asset/imports and when i don't specify a path it sets the path to /asset

vsivsi commented 8 years ago

Got it. Okay, well it seems like you have this sorted out then.

jdziat commented 8 years ago

Yep, thanks for the help.

edemaine commented 8 years ago

Wow, this is super useful! I've been having strange transient issues where users could not display images from uploaded files if they went to my app on a new device / after not accessing it for a long time. I figured it was a cookie issue, but couldn't track it down. Now I see it is likely a path issue, for people going directly to nonroot paths on my site. [https://github.com/carhartl/jquery-cookie says that "By default the path of the cookie is the path of the page where the cookie was created (standard browser behavior). If you want to make it available for instance across the entire domain use path: '/'."] Thanks for this suggestion!

Might I suggest modifying the sample app to include root: '/' in the $.cookie line? (if you haven't already) This isn't necessary for the sample app, because it doesn't have nonroot paths, but would be useful in many Meteor apps, I suspect, and would save future generations from this issue.

vsivsi commented 8 years ago

@edemaine thanks for the suggestion. Can you do a quick PR for that over there? I'm away from my dev box, but can easily review and merge from my phone.