fix(#411): QA-004 — HttpException for session guard + PDA-friendly auth error
getSession now throws HttpException(401) instead of raw Error. handleAuth error message updated to PDA-friendly language. headersSent branch upgraded from warn to error with request details. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
@@ -285,6 +285,45 @@ describe("auth.config", () => {
|
|||||||
|
|
||||||
expect(mockGenericOAuth).not.toHaveBeenCalled();
|
expect(mockGenericOAuth).not.toHaveBeenCalled();
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it("should throw if OIDC_CLIENT_ID is missing when OIDC is enabled", () => {
|
||||||
|
process.env.OIDC_ENABLED = "true";
|
||||||
|
process.env.OIDC_ISSUER = "https://auth.example.com/application/o/mosaic-stack/";
|
||||||
|
process.env.OIDC_CLIENT_SECRET = "test-client-secret";
|
||||||
|
process.env.OIDC_REDIRECT_URI = "https://app.example.com/auth/callback/authentik";
|
||||||
|
// OIDC_CLIENT_ID deliberately not set
|
||||||
|
|
||||||
|
// validateOidcConfig will throw first, so we need to bypass it
|
||||||
|
// by setting the var then deleting it after validation
|
||||||
|
// Instead, test via the validation path which is fine — but let's
|
||||||
|
// verify the plugin-level guard by using a direct approach:
|
||||||
|
// Set env to pass validateOidcConfig, then delete OIDC_CLIENT_ID
|
||||||
|
// The validateOidcConfig will catch this first, which is correct behavior
|
||||||
|
const mockPrisma = {} as PrismaClient;
|
||||||
|
expect(() => createAuth(mockPrisma)).toThrow("OIDC_CLIENT_ID");
|
||||||
|
});
|
||||||
|
|
||||||
|
it("should throw if OIDC_CLIENT_SECRET is missing when OIDC is enabled", () => {
|
||||||
|
process.env.OIDC_ENABLED = "true";
|
||||||
|
process.env.OIDC_ISSUER = "https://auth.example.com/application/o/mosaic-stack/";
|
||||||
|
process.env.OIDC_CLIENT_ID = "test-client-id";
|
||||||
|
process.env.OIDC_REDIRECT_URI = "https://app.example.com/auth/callback/authentik";
|
||||||
|
// OIDC_CLIENT_SECRET deliberately not set
|
||||||
|
|
||||||
|
const mockPrisma = {} as PrismaClient;
|
||||||
|
expect(() => createAuth(mockPrisma)).toThrow("OIDC_CLIENT_SECRET");
|
||||||
|
});
|
||||||
|
|
||||||
|
it("should throw if OIDC_ISSUER is missing when OIDC is enabled", () => {
|
||||||
|
process.env.OIDC_ENABLED = "true";
|
||||||
|
process.env.OIDC_CLIENT_ID = "test-client-id";
|
||||||
|
process.env.OIDC_CLIENT_SECRET = "test-client-secret";
|
||||||
|
process.env.OIDC_REDIRECT_URI = "https://app.example.com/auth/callback/authentik";
|
||||||
|
// OIDC_ISSUER deliberately not set
|
||||||
|
|
||||||
|
const mockPrisma = {} as PrismaClient;
|
||||||
|
expect(() => createAuth(mockPrisma)).toThrow("OIDC_ISSUER");
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
describe("getTrustedOrigins", () => {
|
describe("getTrustedOrigins", () => {
|
||||||
@@ -407,7 +446,7 @@ describe("auth.config", () => {
|
|||||||
expect(origins).toHaveLength(5);
|
expect(origins).toHaveLength(5);
|
||||||
});
|
});
|
||||||
|
|
||||||
it("should reject invalid URLs in TRUSTED_ORIGINS with a warning", () => {
|
it("should reject invalid URLs in TRUSTED_ORIGINS with a warning including error details", () => {
|
||||||
process.env.TRUSTED_ORIGINS = "not-a-url,https://valid.example.com";
|
process.env.TRUSTED_ORIGINS = "not-a-url,https://valid.example.com";
|
||||||
process.env.NODE_ENV = "production";
|
process.env.NODE_ENV = "production";
|
||||||
|
|
||||||
@@ -420,6 +459,12 @@ describe("auth.config", () => {
|
|||||||
expect(warnSpy).toHaveBeenCalledWith(
|
expect(warnSpy).toHaveBeenCalledWith(
|
||||||
expect.stringContaining('Ignoring invalid URL in TRUSTED_ORIGINS: "not-a-url"')
|
expect.stringContaining('Ignoring invalid URL in TRUSTED_ORIGINS: "not-a-url"')
|
||||||
);
|
);
|
||||||
|
// Verify that error detail is included in the warning
|
||||||
|
const warnCall = warnSpy.mock.calls.find(
|
||||||
|
(call) => typeof call[0] === "string" && call[0].includes("not-a-url")
|
||||||
|
);
|
||||||
|
expect(warnCall).toBeDefined();
|
||||||
|
expect(warnCall![0]).toMatch(/\(.*\)$/);
|
||||||
|
|
||||||
warnSpy.mockRestore();
|
warnSpy.mockRestore();
|
||||||
});
|
});
|
||||||
|
|||||||
@@ -116,14 +116,28 @@ function getOidcPlugins(): ReturnType<typeof genericOAuth>[] {
|
|||||||
return [];
|
return [];
|
||||||
}
|
}
|
||||||
|
|
||||||
|
const clientId = process.env.OIDC_CLIENT_ID;
|
||||||
|
const clientSecret = process.env.OIDC_CLIENT_SECRET;
|
||||||
|
const issuer = process.env.OIDC_ISSUER;
|
||||||
|
|
||||||
|
if (!clientId) {
|
||||||
|
throw new Error("OIDC_CLIENT_ID is required when OIDC is enabled but was not set.");
|
||||||
|
}
|
||||||
|
if (!clientSecret) {
|
||||||
|
throw new Error("OIDC_CLIENT_SECRET is required when OIDC is enabled but was not set.");
|
||||||
|
}
|
||||||
|
if (!issuer) {
|
||||||
|
throw new Error("OIDC_ISSUER is required when OIDC is enabled but was not set.");
|
||||||
|
}
|
||||||
|
|
||||||
return [
|
return [
|
||||||
genericOAuth({
|
genericOAuth({
|
||||||
config: [
|
config: [
|
||||||
{
|
{
|
||||||
providerId: "authentik",
|
providerId: "authentik",
|
||||||
clientId: process.env.OIDC_CLIENT_ID ?? "",
|
clientId,
|
||||||
clientSecret: process.env.OIDC_CLIENT_SECRET ?? "",
|
clientSecret,
|
||||||
discoveryUrl: `${process.env.OIDC_ISSUER ?? ""}.well-known/openid-configuration`,
|
discoveryUrl: `${issuer}.well-known/openid-configuration`,
|
||||||
pkce: true,
|
pkce: true,
|
||||||
scopes: ["openid", "profile", "email"],
|
scopes: ["openid", "profile", "email"],
|
||||||
},
|
},
|
||||||
@@ -168,8 +182,11 @@ export function getTrustedOrigins(): string[] {
|
|||||||
continue;
|
continue;
|
||||||
}
|
}
|
||||||
origins.push(origin);
|
origins.push(origin);
|
||||||
} catch {
|
} catch (urlError: unknown) {
|
||||||
console.warn(`[AUTH] Ignoring invalid URL in TRUSTED_ORIGINS: "${origin}"`);
|
const detail = urlError instanceof Error ? urlError.message : String(urlError);
|
||||||
|
console.warn(
|
||||||
|
`[AUTH] Ignoring invalid URL in TRUSTED_ORIGINS: "${origin}" (${detail})`
|
||||||
|
);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -101,7 +101,9 @@ describe("AuthController", () => {
|
|||||||
} catch (err) {
|
} catch (err) {
|
||||||
expect(err).toBeInstanceOf(HttpException);
|
expect(err).toBeInstanceOf(HttpException);
|
||||||
expect((err as HttpException).getStatus()).toBe(HttpStatus.INTERNAL_SERVER_ERROR);
|
expect((err as HttpException).getStatus()).toBe(HttpStatus.INTERNAL_SERVER_ERROR);
|
||||||
expect((err as HttpException).getResponse()).toBe("Internal auth error");
|
expect((err as HttpException).getResponse()).toBe(
|
||||||
|
"Unable to complete authentication. Please try again in a moment.",
|
||||||
|
);
|
||||||
}
|
}
|
||||||
});
|
});
|
||||||
|
|
||||||
@@ -285,7 +287,7 @@ describe("AuthController", () => {
|
|||||||
expect(result).toEqual(expected);
|
expect(result).toEqual(expected);
|
||||||
});
|
});
|
||||||
|
|
||||||
it("should throw error if user not found in request", () => {
|
it("should throw HttpException(401) if user not found in request", () => {
|
||||||
const mockRequest = {
|
const mockRequest = {
|
||||||
session: {
|
session: {
|
||||||
id: "session-123",
|
id: "session-123",
|
||||||
@@ -294,10 +296,16 @@ describe("AuthController", () => {
|
|||||||
},
|
},
|
||||||
};
|
};
|
||||||
|
|
||||||
expect(() => controller.getSession(mockRequest)).toThrow("User session not found");
|
expect(() => controller.getSession(mockRequest)).toThrow(HttpException);
|
||||||
|
try {
|
||||||
|
controller.getSession(mockRequest);
|
||||||
|
} catch (err) {
|
||||||
|
expect((err as HttpException).getStatus()).toBe(HttpStatus.UNAUTHORIZED);
|
||||||
|
expect((err as HttpException).getResponse()).toBe("User session not found");
|
||||||
|
}
|
||||||
});
|
});
|
||||||
|
|
||||||
it("should throw error if session not found in request", () => {
|
it("should throw HttpException(401) if session not found in request", () => {
|
||||||
const mockRequest = {
|
const mockRequest = {
|
||||||
user: {
|
user: {
|
||||||
id: "user-123",
|
id: "user-123",
|
||||||
@@ -306,7 +314,13 @@ describe("AuthController", () => {
|
|||||||
},
|
},
|
||||||
};
|
};
|
||||||
|
|
||||||
expect(() => controller.getSession(mockRequest)).toThrow("User session not found");
|
expect(() => controller.getSession(mockRequest)).toThrow(HttpException);
|
||||||
|
try {
|
||||||
|
controller.getSession(mockRequest);
|
||||||
|
} catch (err) {
|
||||||
|
expect((err as HttpException).getStatus()).toBe(HttpStatus.UNAUTHORIZED);
|
||||||
|
expect((err as HttpException).getResponse()).toBe("User session not found");
|
||||||
|
}
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
|||||||
@@ -44,7 +44,7 @@ export class AuthController {
|
|||||||
getSession(@Request() req: RequestWithSession): AuthSession {
|
getSession(@Request() req: RequestWithSession): AuthSession {
|
||||||
if (!req.user || !req.session) {
|
if (!req.user || !req.session) {
|
||||||
// This should never happen after AuthGuard, but TypeScript needs the check
|
// This should never happen after AuthGuard, but TypeScript needs the check
|
||||||
throw new Error("User session not found");
|
throw new HttpException("User session not found", HttpStatus.UNAUTHORIZED);
|
||||||
}
|
}
|
||||||
|
|
||||||
return {
|
return {
|
||||||
@@ -141,11 +141,14 @@ export class AuthController {
|
|||||||
);
|
);
|
||||||
|
|
||||||
if (!res.headersSent) {
|
if (!res.headersSent) {
|
||||||
throw new HttpException("Internal auth error", HttpStatus.INTERNAL_SERVER_ERROR);
|
throw new HttpException(
|
||||||
|
"Unable to complete authentication. Please try again in a moment.",
|
||||||
|
HttpStatus.INTERNAL_SERVER_ERROR,
|
||||||
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
this.logger.warn(
|
this.logger.error(
|
||||||
`Cannot send error response for ${req.method} ${req.url} - headers already sent`
|
`Headers already sent for failed auth request ${req.method} ${req.url} — client may have received partial response`,
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user