Fix Cloudflare webhook guard validation logic (#17780)
## Summary - Restructures the `CloudflareSecretMatchGuard` to properly validate the `cf-webhook-auth` header before performing the comparison — checks for missing, empty, or mismatched-length values first - Removes `@ts-expect-error` workarounds with explicit typed header access - Expands test coverage with additional edge cases (missing header, wrong value, empty string, length mismatch) ## Test plan - [x] Unit tests pass (6 tests covering matching, missing config, missing header, wrong value, empty string, length mismatch) Made with [Cursor](https://cursor.com) --------- Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com>
This commit is contained in:
co-authored by
Cursor
claude[bot] <41898282+claude[bot]@users.noreply.github.com>
parent
497230a052
commit
bade5289b6
+25
-15
@@ -4,6 +4,7 @@ import {
|
||||
type CanActivate,
|
||||
type ExecutionContext,
|
||||
Injectable,
|
||||
InternalServerErrorException,
|
||||
} from '@nestjs/common';
|
||||
|
||||
import { timingSafeEqual } from 'crypto';
|
||||
@@ -15,24 +16,33 @@ export class CloudflareSecretMatchGuard implements CanActivate {
|
||||
constructor(private readonly twentyConfigService: TwentyConfigService) {}
|
||||
|
||||
canActivate(context: ExecutionContext): boolean {
|
||||
try {
|
||||
const request = context.switchToHttp().getRequest<Request>();
|
||||
const cloudflareWebhookSecret = this.twentyConfigService.get(
|
||||
'CLOUDFLARE_WEBHOOK_SECRET',
|
||||
);
|
||||
|
||||
const cloudflareWebhookSecret = this.twentyConfigService.get(
|
||||
'CLOUDFLARE_WEBHOOK_SECRET',
|
||||
if (!cloudflareWebhookSecret) {
|
||||
throw new InternalServerErrorException(
|
||||
'CLOUDFLARE_WEBHOOK_SECRET is not configured',
|
||||
);
|
||||
}
|
||||
|
||||
if (
|
||||
!cloudflareWebhookSecret ||
|
||||
(cloudflareWebhookSecret &&
|
||||
// @ts-expect-error legacy noImplicitAny
|
||||
(typeof request.headers['cf-webhook-auth'] === 'string' ||
|
||||
timingSafeEqual(
|
||||
// @ts-expect-error legacy noImplicitAny
|
||||
Buffer.from(request.headers['cf-webhook-auth']),
|
||||
Buffer.from(cloudflareWebhookSecret),
|
||||
)))
|
||||
) {
|
||||
try {
|
||||
const request = context.switchToHttp().getRequest();
|
||||
|
||||
const headerValue = request.headers['cf-webhook-auth'];
|
||||
|
||||
if (typeof headerValue !== 'string' || headerValue.length === 0) {
|
||||
return false;
|
||||
}
|
||||
|
||||
const headerBuffer = Buffer.from(headerValue);
|
||||
const secretBuffer = Buffer.from(cloudflareWebhookSecret);
|
||||
|
||||
if (headerBuffer.length !== secretBuffer.length) {
|
||||
return false;
|
||||
}
|
||||
|
||||
if (timingSafeEqual(headerBuffer, secretBuffer)) {
|
||||
return true;
|
||||
}
|
||||
|
||||
|
||||
+51
-32
@@ -1,10 +1,15 @@
|
||||
import { type ExecutionContext } from '@nestjs/common';
|
||||
|
||||
import * as crypto from 'crypto';
|
||||
|
||||
import { type TwentyConfigService } from 'src/engine/core-modules/twenty-config/twenty-config.service';
|
||||
import { CloudflareSecretMatchGuard } from 'src/engine/core-modules/cloudflare/guards/cloudflare-secret.guard';
|
||||
|
||||
const buildMockContext = (headers: Record<string, unknown>) =>
|
||||
({
|
||||
switchToHttp: () => ({
|
||||
getRequest: () => ({ headers }),
|
||||
}),
|
||||
}) as unknown as ExecutionContext;
|
||||
|
||||
describe('CloudflareSecretMatchGuard.canActivate', () => {
|
||||
let guard: CloudflareSecretMatchGuard;
|
||||
let twentyConfigService: TwentyConfigService;
|
||||
@@ -17,48 +22,62 @@ describe('CloudflareSecretMatchGuard.canActivate', () => {
|
||||
});
|
||||
|
||||
it('should return true when the webhook secret matches', () => {
|
||||
const mockRequest = { headers: { 'cf-webhook-auth': 'valid-secret' } };
|
||||
|
||||
jest.spyOn(twentyConfigService, 'get').mockReturnValue('valid-secret');
|
||||
|
||||
const mockContext = {
|
||||
switchToHttp: () => ({
|
||||
getRequest: () => mockRequest,
|
||||
}),
|
||||
} as unknown as ExecutionContext;
|
||||
const context = buildMockContext({
|
||||
'cf-webhook-auth': 'valid-secret',
|
||||
});
|
||||
|
||||
jest.spyOn(crypto, 'timingSafeEqual').mockReturnValue(true);
|
||||
|
||||
expect(guard.canActivate(mockContext)).toBe(true);
|
||||
expect(guard.canActivate(context)).toBe(true);
|
||||
});
|
||||
|
||||
it('should return true when env is not set', () => {
|
||||
const mockRequest = { headers: { 'cf-webhook-auth': 'valid-secret' } };
|
||||
|
||||
it('should throw InternalServerErrorException when env is not set', () => {
|
||||
jest.spyOn(twentyConfigService, 'get').mockReturnValue(undefined);
|
||||
|
||||
const mockContext = {
|
||||
switchToHttp: () => ({
|
||||
getRequest: () => mockRequest,
|
||||
}),
|
||||
} as unknown as ExecutionContext;
|
||||
const context = buildMockContext({
|
||||
'cf-webhook-auth': 'any-value',
|
||||
});
|
||||
|
||||
jest.spyOn(crypto, 'timingSafeEqual').mockReturnValue(true);
|
||||
|
||||
expect(guard.canActivate(mockContext)).toBe(true);
|
||||
expect(() => guard.canActivate(context)).toThrow(
|
||||
'CLOUDFLARE_WEBHOOK_SECRET is not configured',
|
||||
);
|
||||
});
|
||||
|
||||
it('should return false if an error occurs', () => {
|
||||
const mockRequest = { headers: {} };
|
||||
|
||||
it('should return false when the header is missing', () => {
|
||||
jest.spyOn(twentyConfigService, 'get').mockReturnValue('valid-secret');
|
||||
|
||||
const mockContext = {
|
||||
switchToHttp: () => ({
|
||||
getRequest: () => mockRequest,
|
||||
}),
|
||||
} as unknown as ExecutionContext;
|
||||
const context = buildMockContext({});
|
||||
|
||||
expect(guard.canActivate(mockContext)).toBe(false);
|
||||
expect(guard.canActivate(context)).toBe(false);
|
||||
});
|
||||
|
||||
it('should return false when the header value is wrong', () => {
|
||||
jest.spyOn(twentyConfigService, 'get').mockReturnValue('valid-secret');
|
||||
|
||||
const context = buildMockContext({
|
||||
'cf-webhook-auth': 'wrong-secret',
|
||||
});
|
||||
|
||||
expect(guard.canActivate(context)).toBe(false);
|
||||
});
|
||||
|
||||
it('should return false when the header is an empty string', () => {
|
||||
jest.spyOn(twentyConfigService, 'get').mockReturnValue('valid-secret');
|
||||
|
||||
const context = buildMockContext({
|
||||
'cf-webhook-auth': '',
|
||||
});
|
||||
|
||||
expect(guard.canActivate(context)).toBe(false);
|
||||
});
|
||||
|
||||
it('should return false when header length differs from secret', () => {
|
||||
jest.spyOn(twentyConfigService, 'get').mockReturnValue('short');
|
||||
|
||||
const context = buildMockContext({
|
||||
'cf-webhook-auth': 'much-longer-value',
|
||||
});
|
||||
|
||||
expect(guard.canActivate(context)).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user