Merge pull request #778 from hedgedoc/fix/secretLength

src/auth/auth.service.spec.ts

@@ -13,6 +13,7 @@ import { UsersModule } from '../users/users.module';
 import { Identity } from '../users/identity.entity';
 import { LoggerModule } from '../logger/logger.module';
 import { AuthToken } from './auth-token.entity';
+import { TokenNotValidError } from '../errors/errors';
 
 describe('AuthService', () => {
   let service: AuthService;
@@ -105,6 +106,16 @@ describe('AuthService', () => {
         .checkPassword(testPassword, hash)
         .then((result) => expect(result).toBeTruthy());
     });
+    it('fails, if secret is too short', async () => {
+      const secret = service.BufferToBase64Url(await service.randomString(54));
+      const hash = await service.hashPassword(secret);
+      service
+        .checkPassword(secret, hash)
+        .then((result) => expect(result).toBeTruthy());
+      service
+        .checkPassword(secret.substr(0, secret.length - 1), hash)
+        .then((result) => expect(result).toBeFalsy());
+    });
   });
 
   describe('getTokensByUsername', () => {
@@ -148,6 +159,11 @@ describe('AuthService', () => {
         authTokens: [authToken],
       });
     });
+    it('fails on too long token', () => {
+      expect(
+        service.validateToken(`${authToken.keyId}.${'a'.repeat(73)}`),
+      ).rejects.toBeInstanceOf(TokenNotValidError);
+    });
   });
 
   describe('removeToken', () => {

src/auth/auth.service.ts

@@ -4,7 +4,7 @@
  * SPDX-License-Identifier: AGPL-3.0-only
  */
 
-import { Injectable, UnauthorizedException } from '@nestjs/common';
+import { Injectable } from '@nestjs/common';
 import { UsersService } from '../users/users.service';
 import { User } from '../users/user.entity';
 import { AuthToken } from './auth-token.entity';
@@ -35,20 +35,21 @@ export class AuthService {
   }
 
   async validateToken(token: string): Promise<User> {
-    try {
-      const [keyId, secret] = token.split('.');
-      const accessToken = await this.getAuthTokenAndValidate(keyId, secret);
-      await this.setLastUsedToken(keyId);
-      return this.usersService.getUserByUsername(accessToken.user.userName);
-    } catch (error) {
-      if (
-        error instanceof NotInDBError ||
-        error instanceof TokenNotValidError
-      ) {
-        throw new UnauthorizedException(error.message);
-      }
-      throw error;
+    const [keyId, secret] = token.split('.');
+    if (!secret) {
+      throw new TokenNotValidError('Invalid AuthToken format');
     }
+    if (secret.length > 72) {
+      // Only the first 72 characters of the tokens are considered by bcrypt
+      // This should prevent strange corner cases
+      // At the very least it won't hurt us
+      throw new TokenNotValidError(
+        `AuthToken '${secret}' is too long the be a proper token`,
+      );
+    }
+    const accessToken = await this.getAuthTokenAndValidate(keyId, secret);
+    await this.setLastUsedToken(keyId);
+    return this.usersService.getUserByUsername(accessToken.user.userName);
   }
 
   async hashPassword(cleartext: string): Promise<string> {
@@ -92,7 +93,7 @@ export class AuthService {
         `User '${user.userName}' has already 200 tokens and can't have anymore`,
       );
     }
-    const secret = this.BufferToBase64Url(await this.randomString(64));
+    const secret = this.BufferToBase64Url(await this.randomString(54));
     const keyId = this.BufferToBase64Url(await this.randomString(8));
     const accessToken = await this.hashPassword(secret);
     let token;

src/auth/token.strategy.ts

@@ -6,9 +6,10 @@
 
 import { Strategy } from 'passport-http-bearer';
 import { PassportStrategy } from '@nestjs/passport';
-import { Injectable } from '@nestjs/common';
+import { Injectable, UnauthorizedException } from '@nestjs/common';
 import { AuthService } from './auth.service';
 import { User } from '../users/user.entity';
+import { NotInDBError, TokenNotValidError } from '../errors/errors';
 
 @Injectable()
 export class TokenStrategy extends PassportStrategy(Strategy, 'token') {
@@ -17,6 +18,16 @@ export class TokenStrategy extends PassportStrategy(Strategy, 'token') {
   }
 
   async validate(token: string): Promise<User> {
-    return this.authService.validateToken(token);
+    try {
+      return await this.authService.validateToken(token);
+    } catch (error) {
+      if (
+        error instanceof NotInDBError ||
+        error instanceof TokenNotValidError
+      ) {
+        throw new UnauthorizedException(error.message);
+      }
+      throw error;
+    }
   }
 }