fix(api): resolve CSRF guard ordering with global AuthGuard (#514)
All checks were successful
ci/woodpecker/push/api Pipeline was successful
All checks were successful
ci/woodpecker/push/api Pipeline was successful
Co-authored-by: Jason Woltje <jason@diversecanvas.com> Co-committed-by: Jason Woltje <jason@diversecanvas.com>
This commit was merged in pull request #514.
This commit is contained in:
@@ -174,17 +174,19 @@ describe("CsrfGuard", () => {
|
|||||||
});
|
});
|
||||||
|
|
||||||
describe("Session binding validation", () => {
|
describe("Session binding validation", () => {
|
||||||
it("should reject when user is not authenticated", () => {
|
it("should allow when user context is not yet available (global guard ordering)", () => {
|
||||||
|
// CsrfGuard runs as APP_GUARD before per-controller AuthGuard,
|
||||||
|
// so request.user may not be populated. Double-submit cookie match
|
||||||
|
// is sufficient protection in this case.
|
||||||
const token = generateValidToken("user-123");
|
const token = generateValidToken("user-123");
|
||||||
const context = createContext(
|
const context = createContext(
|
||||||
"POST",
|
"POST",
|
||||||
{ "csrf-token": token },
|
{ "csrf-token": token },
|
||||||
{ "x-csrf-token": token },
|
{ "x-csrf-token": token },
|
||||||
false
|
false
|
||||||
// No userId - unauthenticated
|
// No userId - AuthGuard hasn't run yet
|
||||||
);
|
);
|
||||||
expect(() => guard.canActivate(context)).toThrow(ForbiddenException);
|
expect(guard.canActivate(context)).toBe(true);
|
||||||
expect(() => guard.canActivate(context)).toThrow("CSRF validation requires authentication");
|
|
||||||
});
|
});
|
||||||
|
|
||||||
it("should reject token from different session", () => {
|
it("should reject token from different session", () => {
|
||||||
|
|||||||
@@ -89,20 +89,12 @@ export class CsrfGuard implements CanActivate {
|
|||||||
throw new ForbiddenException("CSRF token mismatch");
|
throw new ForbiddenException("CSRF token mismatch");
|
||||||
}
|
}
|
||||||
|
|
||||||
// Validate session binding via HMAC
|
// Validate session binding via HMAC when user context is available.
|
||||||
|
// CsrfGuard is a global guard (APP_GUARD) that runs before per-controller
|
||||||
|
// AuthGuard, so request.user may not be populated yet. In that case, the
|
||||||
|
// double-submit cookie match above is sufficient CSRF protection.
|
||||||
const userId = request.user?.id;
|
const userId = request.user?.id;
|
||||||
if (!userId) {
|
if (userId) {
|
||||||
this.logger.warn({
|
|
||||||
event: "CSRF_NO_USER_CONTEXT",
|
|
||||||
method: request.method,
|
|
||||||
path: request.path,
|
|
||||||
securityEvent: true,
|
|
||||||
timestamp: new Date().toISOString(),
|
|
||||||
});
|
|
||||||
|
|
||||||
throw new ForbiddenException("CSRF validation requires authentication");
|
|
||||||
}
|
|
||||||
|
|
||||||
if (!this.csrfService.validateToken(cookieToken, userId)) {
|
if (!this.csrfService.validateToken(cookieToken, userId)) {
|
||||||
this.logger.warn({
|
this.logger.warn({
|
||||||
event: "CSRF_SESSION_BINDING_INVALID",
|
event: "CSRF_SESSION_BINDING_INVALID",
|
||||||
@@ -114,6 +106,14 @@ export class CsrfGuard implements CanActivate {
|
|||||||
|
|
||||||
throw new ForbiddenException("CSRF token not bound to session");
|
throw new ForbiddenException("CSRF token not bound to session");
|
||||||
}
|
}
|
||||||
|
} else {
|
||||||
|
this.logger.debug({
|
||||||
|
event: "CSRF_SKIP_SESSION_BINDING",
|
||||||
|
method: request.method,
|
||||||
|
path: request.path,
|
||||||
|
reason: "User context not yet available (global guard runs before AuthGuard)",
|
||||||
|
});
|
||||||
|
}
|
||||||
|
|
||||||
return true;
|
return true;
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user