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

feat(core): Prevent session hijacking #9057

Merged
merged 13 commits into from
Apr 9, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion cypress/support/commands.ts
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,11 @@ Cypress.Commands.add('signinAsOwner', () => {
});

Cypress.Commands.add('signout', () => {
cy.request('POST', `${BACKEND_BASE_URL}/rest/logout`);
cy.request({
method: 'POST',
url: `${BACKEND_BASE_URL}/rest/logout`,
headers: { 'browser-id': localStorage.getItem('n8n-browserId') }
});
cy.getCookie(N8N_AUTH_COOKIE).should('not.exist');
});

Expand Down
9 changes: 8 additions & 1 deletion packages/cli/src/Server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ import {
TEMPLATES_DIR,
} from '@/constants';
import { CredentialsController } from '@/credentials/credentials.controller';
import type { CurlHelper } from '@/requests';
import type { APIRequest, CurlHelper } from '@/requests';
import { registerController } from '@/decorators';
import { AuthController } from '@/controllers/auth.controller';
import { BinaryDataController } from '@/controllers/binaryData.controller';
Expand Down Expand Up @@ -235,6 +235,13 @@ export class Server extends AbstractServer {
frontendService.settings.publicApi.latestVersion = apiLatestVersion;
}
}

// Extract BrowserId from headers
this.app.use((req: APIRequest, _, next) => {
req.browserId = req.headers['browser-id'] as string;
next();
});

// Parse cookies for easier access
this.app.use(cookieParser());

Expand Down
35 changes: 24 additions & 11 deletions packages/cli/src/auth/auth.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ interface AuthJwtPayload {
id: string;
/** This hash is derived from email and bcrypt of password */
hash: string;
/** This is a client generated unique string to prevent session hijacking */
browserId?: string;
}

interface IssuedJWT extends AuthJwtPayload {
Expand All @@ -31,6 +33,8 @@ interface PasswordResetToken {
hash: string;
}

const pushEndpoint = `/${config.get('endpoints.rest')}/push`;

@Service()
export class AuthService {
constructor(
Expand All @@ -48,7 +52,7 @@ export class AuthService {
const token = req.cookies[AUTH_COOKIE_NAME];
if (token) {
try {
req.user = await this.resolveJwt(token, res);
req.user = await this.resolveJwt(token, req, res);
} catch (error) {
if (error instanceof JsonWebTokenError || error instanceof AuthError) {
this.clearCookie(res);
Expand All @@ -66,7 +70,8 @@ export class AuthService {
res.clearCookie(AUTH_COOKIE_NAME);
}

issueCookie(res: Response, user: User) {
issueCookie(res: Response, user: User, browserId?: string) {
// TODO: move this check to the login endpoint in AuthController
// If the instance has exceeded its user quota, prevent non-owners from logging in
const isWithinUsersLimit = this.license.isWithinUsersLimit();
if (
Expand All @@ -77,7 +82,7 @@ export class AuthService {
throw new UnauthorizedError(RESPONSE_ERROR_MESSAGES.USERS_QUOTA_REACHED);
}

const token = this.issueJWT(user);
const token = this.issueJWT(user, browserId);
res.cookie(AUTH_COOKIE_NAME, token, {
maxAge: this.jwtExpiration * Time.seconds.toMilliseconds,
httpOnly: true,
Expand All @@ -86,17 +91,18 @@ export class AuthService {
});
}

issueJWT(user: User) {
issueJWT(user: User, browserId?: string) {
const payload: AuthJwtPayload = {
id: user.id,
hash: this.createJWTHash(user),
browserId: browserId && this.hash(browserId),
};
return this.jwtService.sign(payload, {
expiresIn: this.jwtExpiration,
});
}

async resolveJwt(token: string, res: Response): Promise<User> {
async resolveJwt(token: string, req: AuthenticatedRequest, res: Response): Promise<User> {
const jwtPayload: IssuedJWT = this.jwtService.verify(token, {
algorithms: ['HS256'],
});
Expand All @@ -112,14 +118,20 @@ export class AuthService {
// or, If the user has been deactivated (i.e. LDAP users)
user.disabled ||
// or, If the email or password has been updated
jwtPayload.hash !== this.createJWTHash(user)
jwtPayload.hash !== this.createJWTHash(user) ||
// If the token was issued for another browser session
// NOTE: we need to exclude push endpoint from this check because we can't send custom header on websocket requests
// TODO: Implement a custom handshake for push, to avoid having to send any data on querystring or headers
(req.baseUrl !== pushEndpoint &&
jwtPayload.browserId &&
(!req.browserId || jwtPayload.browserId !== this.hash(req.browserId)))
) {
throw new AuthError('Unauthorized');
}

if (jwtPayload.exp * 1000 - Date.now() < this.jwtRefreshTimeout) {
this.logger.debug('JWT about to expire. Will be refreshed');
this.issueCookie(res, user);
this.issueCookie(res, user, jwtPayload.browserId);
}

return user;
Expand Down Expand Up @@ -175,10 +187,11 @@ export class AuthService {
}

createJWTHash({ email, password }: User) {
const hash = createHash('sha256')
.update(email + ':' + password)
.digest('base64');
return hash.substring(0, 10);
return this.hash(email + ':' + password).substring(0, 10);
}

private hash(input: string) {
return createHash('sha256').update(input).digest('base64');
}

/** How many **milliseconds** before expiration should a JWT be renewed */
Expand Down
2 changes: 1 addition & 1 deletion packages/cli/src/controllers/auth.controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ export class AuthController {
}
}

this.authService.issueCookie(res, user);
this.authService.issueCookie(res, user, req.browserId);
void this.internalHooks.onUserLoginSuccess({
user,
authenticationMethod: usedAuthenticationMethod,
Expand Down
2 changes: 1 addition & 1 deletion packages/cli/src/controllers/invitation.controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,7 @@ export class InvitationController {

const updatedUser = await this.userRepository.save(invitee, { transaction: false });

this.authService.issueCookie(res, updatedUser);
this.authService.issueCookie(res, updatedUser, req.browserId);

void this.internalHooks.onUserSignup(updatedUser, {
user_type: 'email',
Expand Down
4 changes: 2 additions & 2 deletions packages/cli/src/controllers/me.controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ export class MeController {

this.logger.info('User updated successfully', { userId });

this.authService.issueCookie(res, user);
this.authService.issueCookie(res, user, req.browserId);

const updatedKeys = Object.keys(payload);
void this.internalHooks.onUserUpdate({
Expand Down Expand Up @@ -138,7 +138,7 @@ export class MeController {
const updatedUser = await this.userRepository.save(user, { transaction: false });
this.logger.info('Password updated successfully', { userId: user.id });

this.authService.issueCookie(res, updatedUser);
this.authService.issueCookie(res, updatedUser, req.browserId);

void this.internalHooks.onUserUpdate({
user: updatedUser,
Expand Down
2 changes: 1 addition & 1 deletion packages/cli/src/controllers/owner.controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ export class OwnerController {

this.logger.debug('Setting isInstanceOwnerSetUp updated successfully');

this.authService.issueCookie(res, owner);
this.authService.issueCookie(res, owner, req.browserId);

void this.internalHooks.onInstanceOwnerSetup({ user_id: owner.id });

Expand Down
2 changes: 1 addition & 1 deletion packages/cli/src/controllers/passwordReset.controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,7 @@ export class PasswordResetController {

this.logger.info('User password updated successfully', { userId: user.id });

this.authService.issueCookie(res, user);
this.authService.issueCookie(res, user, req.browserId);

void this.internalHooks.onUserUpdate({
user,
Expand Down
2 changes: 1 addition & 1 deletion packages/cli/src/middlewares/cors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ export const corsMiddleware: RequestHandler = (req, res, next) => {
res.header('Access-Control-Allow-Methods', 'GET, POST, OPTIONS, PUT, PATCH, DELETE');
res.header(
'Access-Control-Allow-Headers',
'Origin, X-Requested-With, Content-Type, Accept, push-ref',
'Origin, X-Requested-With, Content-Type, Accept, push-ref, browser-id',
);
}

Expand Down
18 changes: 13 additions & 5 deletions packages/cli/src/requests.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,22 +50,30 @@ export class UserRoleChangePayload {
newRoleName: AssignableRole;
}

export type APIRequest<
RouteParams = {},
ResponseBody = {},
RequestBody = {},
RequestQuery = {},
> = express.Request<RouteParams, ResponseBody, RequestBody, RequestQuery> & {
browserId?: string;
};

export type AuthlessRequest<
RouteParams = {},
ResponseBody = {},
RequestBody = {},
RequestQuery = {},
> = express.Request<RouteParams, ResponseBody, RequestBody, RequestQuery>;
> = APIRequest<RouteParams, ResponseBody, RequestBody, RequestQuery> & {
user: never;
};

export type AuthenticatedRequest<
RouteParams = {},
ResponseBody = {},
RequestBody = {},
RequestQuery = {},
> = Omit<
express.Request<RouteParams, ResponseBody, RequestBody, RequestQuery>,
'user' | 'cookies'
> & {
> = Omit<APIRequest<RouteParams, ResponseBody, RequestBody, RequestQuery>, 'user' | 'cookies'> & {
user: User;
cookies: Record<string, string | undefined>;
};
Expand Down
2 changes: 1 addition & 1 deletion packages/cli/src/sso/saml/routes/saml.controller.ee.ts
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ export class SamlController {
});
// Only sign in user if SAML is enabled, otherwise treat as test connection
if (isSamlLicensedAndEnabled()) {
this.authService.issueCookie(res, loginResult.authenticatedUser);
this.authService.issueCookie(res, loginResult.authenticatedUser, req.browserId);
if (loginResult.onboardingRequired) {
return res.redirect(this.urlService.getInstanceBaseUrl() + SamlUrls.samlOnboarding);
} else {
Expand Down
Loading
Loading