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

NestJS does not propagate Passport's errors #177

Closed
adrian-golawski opened this issue Jan 17, 2020 · 3 comments
Closed

NestJS does not propagate Passport's errors #177

adrian-golawski opened this issue Jan 17, 2020 · 3 comments

Comments

@adrian-golawski
Copy link

adrian-golawski commented Jan 17, 2020

I'm submitting a...


[ ] Regression 
[X] Bug report
[X] Feature request
[ ] Documentation issue or request
[ ] Support request => Please do not submit support request here, instead post your question on Stack Overflow.

Current behavior

Errors generated by Passport strategies are all substituted with

{
    "statusCode": 401,
    "error": "Unauthorized"
}

Expected behavior

Errors generated by Passport strategies are propagated and displayed to logs with proper Status Codes.
I.E. https://github.com/jaredhanson/passport-local/blob/2bf3939ca369e08a47a28585c2ccfb3cecffeb9c/lib/strategy.js#L75

if (!username || !password) {
    # usage of passport.Strategy.fail(challenge: any, status: number): void;
    return this.fail({ message: options.badRequestMessage || 'Missing credentials' }, 400);
}

should be returned as:

{
    "statusCode": 400,
    "error": "Missing credentials"
}

Minimal reproduction of the problem with instructions

  1. Use standard setup of passport-local example from Nest Documentation: https://docs.nestjs.com/techniques/authentication#implementing-passport-strategies
  2. Request auth/login endpoint with incorrect user credentials. You can achieve it by:
    a) Sending request with incorrect Content-Type: i.e. Content-Type: text/plain
    b) Sending JSON with invalid fieldNames: i.e. { "brokenusernamefield": "test", "password": "test"}

What is the motivation / use case for changing the behavior?

Passport strategies provide useful information about Missing Credentials, invalid format of the request, etc. Supressing all this information with 401: Unauthorized cause problem in debugging sessions. They should be at least logged as errors caught by ExceptionHandler.

Environment


Nest version: 6.10.2
Nest passport version: 6.1.1

 
For Tooling issues:
- Node version: 10.15.3  
- Platform:  Mac 

Others:

@Enflow-io
Copy link

+1

@hlj
Copy link

hlj commented Feb 11, 2020

I found the problem in code:

handleRequest(err, user, info, context): TUser {
if (err || !user) {
throw err || new UnauthorizedException();
}
return user;
}

This maybe changed to :

handleRequest(err, user, info, context): TUser {
      if (!user) {
        throw err || info || new UnauthorizedException();
      }
      return user;
    }

because Passport called as callback(null, false, failures[0].challenge, failures[0].status)
here is:
https://github.com/jaredhanson/passport/blob/42ff63c60ae55f466d21332306e9112295c0535e/lib/middleware/authenticate.js#L104

@kamilmysliwiec
Copy link
Member

We have accidentally removed the section in the docs on this feature. I've just added it back https://docs.nestjs.com/techniques/authentication#extending-guards

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

No branches or pull requests

4 participants