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

Fix successRedirect to not override cookie path #243

Closed
combsco opened this Issue Jul 17, 2016 · 9 comments

Comments

Projects
None yet
5 participants
@combsco
Copy link

combsco commented Jul 17, 2016

If cookie parameters are provided they should be priority...

src/middleware/express.js
const cookieOptions = Object.assign({}, options.cookie, { path: options.successRedirect });

config.js - If I provide cookie parameters I cannot set the 'path'

      "cookie": {
        "name": "feathers-jwt",
        "domain": "lvh.me",
        "path": "/"
      }

Why?

Well if your API is at api.example.com and your frontend is at customer.example.com. You set the successRedirect URL to https://customer.example.com/login/success. Well that sets the cookie path to "https://customer.example.com/login/success" instead of "/login/success".

I'm assuming successRedirect was more geared/focused for those who's API/Frontend are coupled more tightly.

@combsco

This comment has been minimized.

Copy link
Author

combsco commented Jul 17, 2016

Why not just have an option to put the JWT in a query string on its way back to the successRedirect.

@marshallswain

This comment has been minimized.

Copy link
Member

marshallswain commented Jul 18, 2016

With a query string it would then be visible at the termination endpoints even with SSL turned on.

@combsco

This comment has been minimized.

Copy link
Author

combsco commented Jul 18, 2016

It being visible is fine... since if you're doing it properly someone is verifying the signature of that JWT somewhere. Cause even with SSL on or off... a client can just go into developer tools and yank if from localStorage and decode it if they want.

Which is why you aren't supposed to put confidential data in a JWT unless you encrypt the JWT.

@daffl

This comment has been minimized.

Copy link
Member

daffl commented Jul 18, 2016

It's quite a difference. Putting it into the query string means everybody on the way of the request can intercept your token even if you are using SSL.

For an attacker to get the token from localStorage they first have to inject some malicious script into your app which then figures out where and how to get the token. If a developer wants to grab their own token from localStorage by all means, they will only be able to make requests they are allowed to make with it anyway.

@marshallswain

This comment has been minimized.

Copy link
Member

marshallswain commented Jul 18, 2016

Yep, Even if your connection is SSL secured, other people connected to the same router, or at your ISP, or anywhere along the route between you and the server, will see the JWT token in the URL of the request. Depending on where along the route the attacker is snooping, they could potentially end up with every user's JWT token, so there would be no possibility of private data on your server. Redacted, this is not the case. The only concern is for the security of the termination endpoints of the SSL connection, which would be the browser and either the server or a proxy.

@ekryski

This comment has been minimized.

Copy link
Member

ekryski commented Jul 19, 2016

@combsco: @marshallswain and @daffl are right. Using the query string is insecure but I also have a fix for your issue coming as well 😄 . The new version of auth will allow you to set whatever cookie options you want (including your path).

@ekryski ekryski modified the milestone: 0.8 Aug 9, 2016

@josx

This comment has been minimized.

Copy link

josx commented Aug 17, 2016

i am using 0.7.9, and need to redirect cookies to another subdomain (using oauth with facebook).
Works pretty well with localhost, but not with other domains.
Something to do with cookie params? any clue? (frontend is just a custom domain).

 app.get('/auth/success', function(req, res){
     res.cookie('feathers-jwt', req.cookies['feathers-jwt'];
     res.redirect(app.get('frontend'));
 });
@josx

This comment has been minimized.

Copy link

josx commented Aug 17, 2016

Just for the record I resolved with 0.7.9 (without client redirection)

With conf

 {
   "host": "yourBackendUrl",
   "port": "PORT",
   "frontend": "http://yourFrontEndUrl",
   "mongodb": "MONGO_URL",
   "public": "../public/",
   "auth": {
     "cookie": {
       "secure": false,
       "domain": "yourDomain",
       "httpOnly": false,
       "maxAge": 300000
     },
     "shouldSetupSuccessRoute": false,
     "token": {
       "secret": "FEATHERS_AUTH_SECRET"
     },
     "local": {},
     "facebook": {
       "clientID": "yourClientId",
       "clientSecret": "yourClientSecret",
       "permissions": {
         "scope": ["public_profile", "email"]
       },
       "profileFields": ["id", "displayName", "email", "name", "cover", "picture"]
     }
   }
 }
app.get('/auth/success', function(req, res){
     res.cookie('feathers-jwt', req.cookies['feathers-jwt'], app.get('auth').cookie);
     res.redirect(app.get('frontend'));
});

@ekryski ekryski modified the milestones: 0.8, 1.0 Nov 21, 2016

@ekryski

This comment has been minimized.

Copy link
Member

ekryski commented Dec 30, 2016

I believe this is now fixed with auth v1.x. There is no success redirect by default anymore.

Instead if you need that functionality you specify it in your config file at the root of auth or pass it explicitly when calling authenticate on either your hook or express middleware.

Please check out the migration guide on how to upgrade. If that doesn't work feel free to re-open the issue and I will revisit this.

@ekryski ekryski closed this Dec 30, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.