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

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

jdziat
Copy link
Contributor

@jdziat jdziat commented Apr 28, 2016

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.

	updated the handle_auth method to allow for new Meteor versions.
@vsivsi
Copy link
Owner

vsivsi commented Apr 28, 2016

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
Copy link
Contributor Author

jdziat commented Apr 28, 2016

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
Copy link
Contributor Author

jdziat commented Apr 28, 2016

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
Copy link
Owner

vsivsi commented Apr 28, 2016

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
Copy link
Owner

vsivsi commented Apr 28, 2016

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
Copy link
Contributor Author

jdziat commented Apr 28, 2016

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
Copy link
Contributor Author

jdziat commented Apr 28, 2016

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
Copy link
Owner

vsivsi commented Apr 28, 2016

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
Copy link
Owner

vsivsi commented Apr 28, 2016

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
Copy link
Contributor Author

jdziat commented Apr 28, 2016

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
Copy link
Owner

vsivsi commented Apr 28, 2016

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
Copy link
Contributor Author

jdziat commented Apr 28, 2016

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
Copy link
Owner

vsivsi commented Apr 28, 2016

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
Copy link
Contributor Author

jdziat commented Apr 28, 2016

Strange I wonder what is different between the two setups

@vsivsi
Copy link
Owner

vsivsi commented Apr 28, 2016

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
Copy link
Contributor Author

jdziat commented Apr 28, 2016

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

@jdziat
Copy link
Contributor Author

jdziat commented Apr 28, 2016

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

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

@vsivsi
Copy link
Owner

vsivsi commented Apr 28, 2016

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
Copy link
Contributor Author

jdziat commented Apr 28, 2016

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
Copy link
Owner

vsivsi commented Apr 28, 2016

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

@jdziat
Copy link
Contributor Author

jdziat commented Apr 29, 2016

Yep, thanks for the help.

@jdziat jdziat closed this Apr 29, 2016
@jdziat jdziat deleted the InvalidAuthToken branch April 29, 2016 00:03
@edemaine
Copy link
Contributor

edemaine commented May 8, 2016

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.

edemaine added a commit to edemaine/coauthor that referenced this pull request May 8, 2016
@vsivsi
Copy link
Owner

vsivsi commented May 8, 2016

@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.

@vsivsi vsivsi changed the title modified: src/http_access_server.coffee X-Auth-Token cookie should be set for root path May 8, 2016
edemaine added a commit to edemaine/meteor-file-sample-app that referenced this pull request May 8, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants