Skip to content

fix(security): prevent fail-open auth in provision/webhooks and add ticket replay protection #14

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.

Sign up for GitHub

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

Merged
JOY (JOY) merged 1 commit into from
Aug 27, 2026
Merged

fix(security): prevent fail-open auth in provision/webhooks and add ticket replay protection #14

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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: 1 addition & 5 deletions apps/backend/src/api/routes/dos-org-sync.controller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,11 +32,7 @@ export class DosOrgSyncWebhookController {
process.env.DOS_SYNC_WEBHOOK_SECRET ||
process.env.DOS_WEBHOOK_SECRET ||
process.env.JWT_SECRET;
if (!secret) {
return true;
}

if (!signatureHeader) {
if (!secret || !signatureHeader) {
return false;
}

Expand Down
46 changes: 37 additions & 9 deletions apps/backend/src/api/routes/provision.controller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {
} from '@nestjs/common';
import { ApiTags } from '@nestjs/swagger';
import { Request, Response } from 'express';
import { timingSafeEqual } from 'crypto';

@gemini-code-assist gemini-code-assist Bot Aug 27, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Import createHash from the crypto module to support timing-safe comparison of secrets of arbitrary lengths.

Suggested change
import { timingSafeEqual } from 'crypto';
import { timingSafeEqual, createHash } from 'crypto';

import { OrganizationService } from '@gitroom/nestjs-libraries/database/prisma/organizations/organization.service';
import { UsersService } from '@gitroom/nestjs-libraries/database/prisma/users/users.service';
import { AuthService } from '@gitroom/backend/services/auth/auth.service';
Expand All @@ -19,6 +20,7 @@ import { getCookieUrlFromDomain } from '@gitroom/helpers/subdomain/subdomain.man
import { ProvisionUserDto } from '@gitroom/nestjs-libraries/dtos/provision/provision-user.dto';
import { ConsumeTicketDto } from '@gitroom/nestjs-libraries/dtos/provision/consume-ticket.dto';
import { makeId } from '@gitroom/nestjs-libraries/services/make.is';
import { ioRedis } from '@gitroom/nestjs-libraries/redis/redis.service';
import { Provider } from '@prisma/client';

@ApiTags('Provisioning')
Expand All @@ -35,19 +37,24 @@ export class ProvisionController {
process.env.PROVISIONING_SECRET_KEY ||
process.env.DOS_PROVISIONING_SECRET ||
process.env.DOS_SYNC_WEBHOOK_SECRET ||
process.env.INTERNAL_API_KEY ||
process.env.JWT_SECRET;
process.env.INTERNAL_API_KEY;

if (!secret) {
return true;
if (!secret || !authHeader) {
return false;
}

if (!authHeader) {
const token = authHeader.replace(/^Bearer\s+/i, '').trim();
if (!token) {
return false;
}

const token = authHeader.replace(/^Bearer\s+/i, '').trim();
return token === secret;
const expected = Buffer.from(secret);
const provided = Buffer.from(token);
if (expected.length !== provided.length) {
return false;
}

return timingSafeEqual(provided, expected);
Comment on lines +51 to +57

@gemini-code-assist gemini-code-assist Bot Aug 27, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-medium medium

The current implementation of timingSafeEqual performs an early return if the lengths of the expected and provided tokens do not match. This can leak the length of the secret key via timing analysis.

To prevent this, hash both the expected and provided tokens using a fixed-length hashing function (like SHA-256) before performing the constant-time comparison.

    const expected = createHash('sha256').update(secret).digest();
    const provided = createHash('sha256').update(token).digest();
    return timingSafeEqual(provided, expected);

}

@Post('/provision')
Expand Down Expand Up @@ -137,14 +144,24 @@ export class ProvisionController {
targetOrg = userOrgs[0] || { id: orgId || makeId(10), name: effectiveOrgName };
}

// 3. Issue one-time login ticket (valid for 5 minutes)
// 3. Issue one-time login ticket (single-use, valid for 5 minutes)
const ticketId = makeId(32);
const ticket = AuthChecker.signJWT({
jti: ticketId,
userId: user.id,
orgId: targetOrg.id,
type: 'one_time_ticket',
exp: Math.floor(Date.now() / 1000) + 300,
});

// Store in Redis with 300s TTL for single-use / replay protection
await ioRedis.set(
`ticket:${ticketId}`,
JSON.stringify({ userId: user.id, orgId: targetOrg.id }),
'EX',
300
);

const loginUrl = `${process.env.FRONTEND_URL}/auth/ticket?ticket=${ticket}`;

return {
Expand Down Expand Up @@ -180,10 +197,21 @@ export class ProvisionController {
throw new HttpException('Invalid or expired ticket', HttpStatus.BAD_REQUEST);
}

if (payload?.type !== 'one_time_ticket' || !payload?.userId) {
if (payload?.type !== 'one_time_ticket' || !payload?.userId || !payload?.jti) {
throw new HttpException('Invalid ticket type', HttpStatus.BAD_REQUEST);
}

// Atomic consume & replay protection: verify ticket exists in Redis then delete immediately
const ticketKey = `ticket:${payload.jti}`;
const storedTicket = await ioRedis.get(ticketKey);
if (!storedTicket) {
throw new HttpException(
'Ticket has already been used or has expired',
HttpStatus.BAD_REQUEST
);
}
await ioRedis.del(ticketKey);
Comment on lines +206 to +213

@gemini-code-assist gemini-code-assist Bot Aug 27, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-high high

The current implementation of the one-time ticket consumption is not atomic and is vulnerable to a race condition (Time-of-Check to Time-of-Use). If multiple concurrent requests are made with the same ticket, both could pass the ioRedis.get check before either executes ioRedis.del, allowing the ticket to be reused.

Since the storedTicket value is not actually used in the rest of the method (the user ID is retrieved from the verified JWT payload), you can achieve true atomicity by using ioRedis.del directly and checking if the deleted count is greater than 0.

Suggested change
const storedTicket = await ioRedis.get(ticketKey);
if (!storedTicket) {
throw new HttpException(
'Ticket has already been used or has expired',
HttpStatus.BAD_REQUEST
);
}
await ioRedis.del(ticketKey);
const deleted = await ioRedis.del(ticketKey);
if (deleted === 0) {
throw new HttpException(
'Ticket has already been used or has expired',
HttpStatus.BAD_REQUEST
);
}


const user = await this._userService.getUserById(payload.userId);
if (!user || !user.activated) {
throw new HttpException('User not found or inactive', HttpStatus.NOT_FOUND);
Expand Down
9 changes: 9 additions & 0 deletions apps/backend/src/api/routes/root.controller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,13 @@ export class RootController {
getRoot(): string {
return 'App is running!';
}

@Get('/health')
getHealth() {
return {
status: 'ok',
timestamp: new Date().toISOString(),
service: 'crove-post',
};
}
Comment on lines +9 to +16

@gemini-code-assist gemini-code-assist Bot Aug 27, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The health check endpoint returns a hardcoded service name 'crove-post'. This appears to be a copy-paste artifact from another project (e.g., Crove). It should be updated to reflect the correct service name (e.g., 'postiz').

Suggested change
@Get('/health')
getHealth() {
return {
status: 'ok',
timestamp: new Date().toISOString(),
service: 'crove-post',
};
}
@Get('/health')
getHealth() {
return {
status: 'ok',
timestamp: new Date().toISOString(),
service: 'postiz',
};
}

}
Loading