fix(security): critical auth fixes from audit - #190
Open
BillyOutlast wants to merge 2 commits into
Open
Conversation
added 2 commits
July 28, 2026 00:58
… length, OIDC redirect CRITICAL-1: session cookie missing httpOnly/secure/sameSite attributes CRITICAL-2: rate limiting disabled in nuxt.config CRITICAL-3: password maxLength validation missing CRITICAL-4: OIDC redirect URL validation missing Includes execFileSync hardening (shell-injection prevention) and session module refactoring (cache/db/filter/memory). Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai> Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)
Reviewer's GuideImplements multiple critical security improvements across authentication, sessions, Nuxt security config, and runtime environment, including secure session cookies with fixation protection, OIDC redirect validation, rate limiting, password length bounds, and running the container as non-root, plus refactors for session filtering and JSON path walking and adds targeted unit tests. Sequence diagram for secure session signin with fixation protectionsequenceDiagram
actor User
participant SigninEndpoint as signin_simple_post
participant SessionHandler
participant SessionProvider
User->>SigninEndpoint: POST /api/v1/auth/signin/simple
SigninEndpoint->>SessionHandler: signin(h3, userId, options)
activate SessionHandler
SessionHandler->>SessionHandler: getSessionToken(h3)
Note right of SessionHandler: oldToken may exist
SessionHandler->>SessionHandler: createSessionCookie(h3, expiresAt)
SessionHandler->>SessionProvider: removeSession(oldToken)
SessionHandler-->>SigninEndpoint: SessionWithToken
deactivate SessionHandler
SigninEndpoint-->>User: 200 OK (session cookie set)
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 8 issues, and left some high level feedback:
- The
secureflag for the session cookie is derived directly fromgetRequestURL(h3).protocol; in common reverse-proxy setups this will often behttpeven when the external scheme ishttps, so consider basing this on a trusted config (e.g.,X-Forwarded-Proto/Nuxt runtime config) to avoid accidentally issuing non-secure cookies in production. - Enabling
strictTransportSecuritywithincludeSubdomains: trueat the Nuxt security layer may have unintended effects in non-HTTPS or mixed environments (e.g., staging, custom subdomains), so it may be worth tying this to an explicit production/HTTPS-only configuration flag.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `secure` flag for the session cookie is derived directly from `getRequestURL(h3).protocol`; in common reverse-proxy setups this will often be `http` even when the external scheme is `https`, so consider basing this on a trusted config (e.g., `X-Forwarded-Proto`/Nuxt runtime config) to avoid accidentally issuing non-secure cookies in production.
- Enabling `strictTransportSecurity` with `includeSubdomains: true` at the Nuxt security layer may have unintended effects in non-HTTPS or mixed environments (e.g., staging, custom subdomains), so it may be worth tying this to an explicit production/HTTPS-only configuration flag.
## Individual Comments
### Comment 1
<location path="server/nuxt.config.ts" line_range="33-39" />
<code_context>
const commitHash =
process.env.BUILD_GIT_REF ??
- execSync("git rev-parse --short HEAD").toString().trim();
+ execFileSync("git", ["rev-parse", "--short", "HEAD"], {
+ encoding: "utf-8",
+ env: {
+ ...process.env,
+ PATH: process.env.PATH ?? "/usr/local/bin:/usr/bin:/bin",
+ },
+ }).trim(); // NOSONAR:typescript:S4036 - execFileSync doesn't use shell; PATH explicitly sanitized
console.log(`Drop ${dropVersion} #${commitHash}`);
</code_context>
<issue_to_address>
**nitpick:** Environment PATH is not actually sanitized here despite the comment, which may be misleading for future reviewers.
The NOSONAR note claims PATH is "explicitly sanitized", but here we simply reuse `process.env.PATH` when present. While the behavior is fine, the comment overstates the safety guarantees. Consider rephrasing to emphasize that `execFileSync` avoids a shell and that a safe fallback PATH is used only when `process.env.PATH` is unset, rather than implying additional sanitization.
</issue_to_address>
### Comment 2
<location path="server/nuxt.config.ts" line_range="275-277" />
<code_context>
],
},
- strictTransportSecurity: false,
+ strictTransportSecurity: { maxAge: 31536000, includeSubdomains: true },
},
- rateLimiter: false,
</code_context>
<issue_to_address>
**🚨 suggestion (security):** Strict-Transport-Security with includeSubdomains may be risky in non-production environments.
This HSTS config is good for production, but risky if shared with non-HTTPS or ephemeral environments on the same domain/subdomains. Consider restricting it to production only (e.g. `NODE_ENV === "production"`) or making it configurable so HSTS is only enabled on intended public domains.
```suggestion
},
strictTransportSecurity:
process.env.NODE_ENV === "production"
? { maxAge: 31536000, includeSubdomains: true }
: false,
},
```
</issue_to_address>
### Comment 3
<location path="server/server/internal/session/index.ts" line_range="274-279" />
<code_context>
- // trying to guess a session id (jwts let us sign + encrypt stuff in a std way)
- setCookie(h3, dropTokenCookieName, token, { expires: expiresAt });
+ // PENDING(sonar): consider switching to JWTs for session tokens - deferred, significant refactoring needed
+ setCookie(h3, dropTokenCookieName, token, {
+ expires: expiresAt,
+ httpOnly: true,
+ secure: getRequestURL(h3).protocol === "https:",
+ sameSite: "lax",
+ path: "/",
+ });
return token;
</code_context>
<issue_to_address>
**🚨 suggestion (security):** Relying solely on getRequestURL(h3).protocol for the `secure` flag may be inaccurate behind proxies.
Because the flag is set from `getRequestURL(h3).protocol === "https:"`, a proxied setup with TLS terminated at a load balancer may still show `http:` to the app, resulting in cookies without `secure` even when the client uses HTTPS. If this handler can run behind such infrastructure, consider also checking a trusted indicator of HTTPS (e.g. `X-Forwarded-Proto`, a trusted proxy setting, or a config/env flag) when deciding whether to set `secure`.
Suggested implementation:
```typescript
}
*/
// Determine whether the original client connection is HTTPS.
// This implementation considers both the direct protocol and a trusted "x-forwarded-proto" header,
// which is commonly set by reverse proxies / load balancers when terminating TLS.
private isSecureRequest(h3: H3Event): boolean {
const urlSecure = getRequestURL(h3).protocol === "https:";
const forwardedProtoHeader = h3.node.req.headers["x-forwarded-proto"];
const forwardedProto =
typeof forwardedProtoHeader === "string"
? forwardedProtoHeader.split(",")[0]?.trim().toLowerCase()
: undefined;
const proxySecure = forwardedProto === "https";
return urlSecure || proxySecure;
}
private createSessionCookie(h3: H3Event, expiresAt: Date) {
```
```typescript
setCookie(h3, dropTokenCookieName, token, {
expires: expiresAt,
httpOnly: true,
secure: this.isSecureRequest(h3),
sameSite: "lax",
path: "/",
});
```
</issue_to_address>
### Comment 4
<location path="server/server/api/v1/auth/oidc/callback.get.ts" line_range="68-77" />
<code_context>
await userStatsManager.cacheUserSessions();
if (result.options.redirect) {
+ const requestOrigin = getRequestURL(h3).origin;
+ let redirectUrl: URL;
+ try {
+ redirectUrl = new URL(result.options.redirect, requestOrigin);
+ } catch {
+ throw createError({
+ statusCode: 400,
+ message: "Invalid redirect URL",
+ });
+ }
+ if (redirectUrl.origin !== requestOrigin) {
+ throw createError({
+ statusCode: 400,
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Use the validated URL object for the redirect to avoid subtle discrepancies with the original string.
Since you validate via the `redirectUrl` object but then pass the original string to `sendRedirect`, you risk discrepancies between what was validated and what is sent. Please use `redirectUrl.toString()` (or `.href`) in the redirect so the exact validated (and normalized) URL is used.
</issue_to_address>
### Comment 5
<location path="Dockerfile" line_range="105-106" />
<code_context>
# Nuxt's port
ENV PORT=4000
+# Run as non-root user for security
+USER node
+
CMD ["sh", "/app/startup/launch.sh"]
</code_context>
<issue_to_address>
**issue (bug_risk):** Switching to USER node may require additional file permission adjustments in the image.
Running as `node` is a solid hardening step, but it can fail at runtime if files or directories (like `/app`, `startup/launch.sh`, or writable cache/log paths) remain root-owned without appropriate permissions for the `node` user. Please verify ownership/permissions across build and deployment environments and add explicit `chown`/`chmod` steps where needed to ensure compatibility with `USER node`.
</issue_to_address>
### Comment 6
<location path="server/test/unit/security/oidc-redirect.test.ts" line_range="42-51" />
<code_context>
+ it("allows same-origin redirect", async () => {
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests for additional redirect edge cases (absolute same-origin URL, invalid redirect string, and no-redirect scenario).
The suite already covers relative same-origin, cross-origin, and localhost redirects. To fully exercise the new validation logic in `oidc/callback.get.ts`, please also add tests for:
- An absolute same-origin redirect (e.g. `https://drop.example.com/dashboard`) to confirm it’s accepted.
- A malformed redirect (e.g. `"::not-a-url"`) to verify the `new URL(...)` path throws and `createError` is called with a 400.
- A missing `options.redirect` to confirm `sendRedirect` is not invoked and the handler completes normally.
Suggested implementation:
```typescript
});
it("allows same-origin redirect", async () => {
vi.mocked(sessionHandler.signin).mockResolvedValue("signin");
vi.mocked(authManager.getAuthProviders).mockReturnValue({
Simple: false,
OpenID: {
authorize: vi.fn().mockResolvedValue({
user: { id: "user-1" },
options: { redirect: "/dashboard" },
claims: {},
}),
} as unknown as never,
});
const event = createEvent(); // use the same helper / factory used in the other tests
await handler(event); // call the same handler under test used elsewhere in this file
expect(sendRedirect).toHaveBeenCalledWith(event, "/dashboard");
});
it("allows absolute same-origin redirect", async () => {
vi.mocked(sessionHandler.signin).mockResolvedValue("signin");
vi.mocked(authManager.getAuthProviders).mockReturnValue({
Simple: false,
OpenID: {
authorize: vi.fn().mockResolvedValue({
user: { id: "user-1" },
options: {
redirect: "https://drop.example.com/dashboard",
},
claims: {},
}),
} as unknown as never,
});
const event = createEvent();
await handler(event);
expect(sendRedirect).toHaveBeenCalledWith(
event,
"https://drop.example.com/dashboard",
);
});
it("rejects malformed redirect URLs with a 400 error", async () => {
vi.mocked(sessionHandler.signin).mockResolvedValue("signin");
vi.mocked(authManager.getAuthProviders).mockReturnValue({
Simple: false,
OpenID: {
authorize: vi.fn().mockResolvedValue({
user: { id: "user-1" },
options: {
redirect: "::not-a-url",
},
claims: {},
}),
} as unknown as never,
});
const event = createEvent();
await expect(handler(event)).rejects.toEqual(
createError({
statusCode: 400,
statusMessage: expect.stringContaining("Invalid redirect URL"),
}),
);
expect(sendRedirect).not.toHaveBeenCalled();
});
it("handles missing redirect without calling sendRedirect", async () => {
vi.mocked(sessionHandler.signin).mockResolvedValue("signin");
vi.mocked(authManager.getAuthProviders).mockReturnValue({
Simple: false,
OpenID: {
authorize: vi.fn().mockResolvedValue({
user: { id: "user-1" },
options: {},
claims: {},
}),
} as unknown as never,
});
const event = createEvent();
const result = await handler(event);
expect(sendRedirect).not.toHaveBeenCalled();
// Optionally, assert on the default behavior if the handler returns something specific
expect(result).toBeDefined();
```
You will need to align a few identifiers with the rest of the file:
1. Replace `handler(event)` with the actual function under test (e.g. `oidcCallbackHandler(event)` or whatever is used in the other tests in this file).
2. Replace `createEvent()` with the real helper / factory used to build the `H3Event` (e.g. `createEvent({ path: "/api/oidc/callback" })` or similar). If there is no helper, construct the event exactly as done in the existing tests.
3. Ensure `sendRedirect` and `createError` are the same mocked imports used elsewhere in this test suite. If the file asserts errors differently (e.g. using `toThrowError` instead of comparing to `createError(...)`), adjust the `await expect(handler(event)).rejects...` assertion to match the existing convention.
4. Update the `"Invalid redirect URL"` substring to match the exact error message thrown in `oidc/callback.get.ts` if it differs (or loosen the assertion to `expect.anything()` if the message is not important).
</issue_to_address>
### Comment 7
<location path="server/test/unit/security/session-cookie.test.ts" line_range="26-33" />
<code_context>
+// eslint-disable-next-line import/first
+import sessionHandler from "../../../server/internal/session";
+
+describe("Session Cookie Security Attributes", () => {
+ beforeEach(() => {
+ vi.clearAllMocks();
+ vi.stubGlobal("setCookie", vi.fn());
+ vi.stubGlobal("getRequestURL", vi.fn());
+ vi.stubGlobal("createError", (opts: unknown) => {
+ throw opts;
+ });
+ vi.stubGlobal("deleteCookie", vi.fn());
+ });
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test that explicitly verifies session fixation prevention (old session token is invalidated).
The updated `SessionHandler.signin` correctly issues a new cookie and calls `removeSession(oldToken)`, but the tests don’t verify this behavior.
Given the existing mocked DB session provider, please add a test that:
- Configures `getSession` / `removeSession` for an `old-token`.
- Calls `sessionHandler.signin` with a request containing `"drop-token=old-token"`.
- Asserts `removeSession` is called with `"old-token"` and `setCookie` is invoked with a token that is defined and not equal to `"old-token"`.
This ensures the session fixation mitigation is covered by tests and protects against regressions.
Suggested implementation:
```typescript
vi.stubGlobal("createError", (opts: unknown) => {
throw opts;
});
vi.stubGlobal("deleteCookie", vi.fn());
});
it("prevents session fixation by invalidating old session tokens", async () => {
const oldToken = "old-token";
// Ensure a valid URL is returned for the request (signin relies on this)
vi.mocked(getRequestURL).mockReturnValue(
new URL("https://drop.example.com"),
);
// Spy on the session handler's session methods
const getSessionSpy = vi
.spyOn(sessionHandler as any, "getSession")
.mockResolvedValue({
token: oldToken,
userId: "user-1",
});
const removeSessionSpy = vi
.spyOn(sessionHandler as any, "removeSession")
.mockResolvedValue(undefined);
// Call signin with a request containing drop-token=old-token
await sessionHandler.signin(
{
headers: {
cookie: `drop-token=${oldToken}`,
},
} as any,
{} as any,
);
// The old session should have been looked up and then invalidated
expect(getSessionSpy).toHaveBeenCalledWith(oldToken);
expect(removeSessionSpy).toHaveBeenCalledWith(oldToken);
// A new cookie should be set with a new token value
expect(setCookie).toHaveBeenCalled();
const lastCall =
vi.mocked(setCookie as any).mock.calls[
vi.mocked(setCookie as any).mock.calls.length - 1
];
const [cookieName, cookieValue] = lastCall ?? [];
expect(cookieName).toBe("drop-token");
expect(cookieValue).toBeDefined();
expect(cookieValue).not.toBe(oldToken);
});
it("sets secure flag for HTTPS requests", async () => {
```
If `sessionHandler` does not expose `getSession` / `removeSession` as methods (e.g. they live in a separate provider module), adjust the spies accordingly:
1. Import the actual session provider module used by `SessionHandler.signin` (for example, `import * as sessionProvider from "../../../server/internal/session-provider";`).
2. Replace `vi.spyOn(sessionHandler as any, "getSession")` / `"removeSession"` with spies on that provider:
- `const getSessionSpy = vi.spyOn(sessionProvider, "getSession")...`
- `const removeSessionSpy = vi.spyOn(sessionProvider, "removeSession")...`
3. Ensure the cookie name (`"drop-token"`) in the test matches the real session cookie name if it differs in your implementation.
</issue_to_address>
### Comment 8
<location path="server/test/unit/security/oidc-redirect.test.ts" line_range="23-32" />
<code_context>
+ },
+}));
+
+describe("OIDC Redirect Validation", () => {
+ beforeEach(() => {
+ vi.clearAllMocks();
+ vi.stubGlobal("setHeader", vi.fn());
+ vi.stubGlobal("sendRedirect", vi.fn());
+ vi.stubGlobal("getQuery", vi.fn());
+ vi.stubGlobal("getRequestURL", vi.fn());
+ vi.stubGlobal("createError", (opts: unknown) => {
+ throw opts;
+ });
+
+ vi.mocked(authManager.getAuthProviders).mockReturnValue({
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding at least a smoke test that the Nuxt security rate limiter and request size limiter are enabled with the expected configuration.
These controls are configured in `nuxt.config.ts`, but nothing verifies they remain enabled with the expected settings. Since they are critical security protections, having at least one automated check would reduce the risk of accidental misconfiguration.
For example, you could:
- Add a small config/unit test that imports the Nuxt config and asserts the `security.rateLimiter` and `security.requestSizeLimiter` options match the expected structure/values.
- Or add a basic integration/e2e test that (a) sends enough requests to trigger the rate limiter and (b) sends an oversized payload, asserting both are rejected as expected.
Even the lightweight config test would provide a useful safety net against future regressions.
Suggested implementation:
```typescript
describe("Nuxt security configuration", () => {
it("enables Nuxt security rateLimiter and requestSizeLimiter with expected structure", () => {
const security = (nuxtConfig as any).security;
expect(security).toBeDefined();
expect(security?.rateLimiter).toBeDefined();
expect(security?.requestSizeLimiter).toBeDefined();
// Smoke-test the basic structure of the rate limiter configuration
expect(security?.rateLimiter).toMatchObject({
// Adjust these keys/expectations to match your actual nuxt.config.ts
tokensPerInterval: expect.any(Number),
interval: expect.any(String),
});
// Smoke-test the basic structure of the request size limiter configuration
expect(security?.requestSizeLimiter).toEqual(
expect.objectContaining({
// Adjust these keys/expectations to match your actual nuxt.config.ts
maxRequestSizeInBytes: expect.any(Number),
maxUploadFileRequestInBytes: expect.any(Number),
}),
);
});
});
describe("OIDC Redirect Validation", () => {
```
`).
Here are the edits:
<file_operations>
<file_operation operation="edit" file_path="server/test/unit/security/oidc-redirect.test.ts">
<<<<<<< SEARCH
describe("OIDC Redirect Validation", () => {
=======
describe("Nuxt security configuration", () => {
it("enables Nuxt security rateLimiter and requestSizeLimiter with expected structure", () => {
const security = (nuxtConfig as any).security;
expect(security).toBeDefined();
expect(security?.rateLimiter).toBeDefined();
expect(security?.requestSizeLimiter).toBeDefined();
// Smoke-test the basic structure of the rate limiter configuration
expect(security?.rateLimiter).toMatchObject({
// Adjust these keys/expectations to match your actual nuxt.config.ts
tokensPerInterval: expect.any(Number),
interval: expect.any(String),
});
// Smoke-test the basic structure of the request size limiter configuration
expect(security?.requestSizeLimiter).toEqual(
expect.objectContaining({
// Adjust these keys/expectations to match your actual nuxt.config.ts
maxRequestSizeInBytes: expect.any(Number),
maxUploadFileRequestInBytes: expect.any(Number),
}),
);
});
});
describe("OIDC Redirect Validation", () => {
>>>>>>> REPLACE
</file_operation>
</file_operations>
<additional_changes>
1. At the top of `server/test/unit/security/oidc-redirect.test.ts`, add an import for your Nuxt config (path may need adjustment depending on your repo layout):
```ts
import nuxtConfig from "../../../../nuxt.config";
```
2. Adjust the expectations in the new `describe("Nuxt security configuration", ...)` block to match the actual shape and keys of `security.rateLimiter` and `security.requestSizeLimiter` in your `nuxt.config.ts`.
- If you use different keys (e.g. `tokensPerInterval`/`interval` vs `tokens`/`windowMs`, or `maxRequestSize` vs `maxRequestSizeInBytes`), update the `expect.objectContaining` / `toMatchObject` calls accordingly.
- If the config includes an `enabled` flag or similar, you may want to assert `enabled: true` as well.
3. If your test environment does not have global `expect` from Vitest, add `import { expect } from "vitest";` at the top with the other imports.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+33
to
+39
| execFileSync("git", ["rev-parse", "--short", "HEAD"], { | ||
| encoding: "utf-8", | ||
| env: { | ||
| ...process.env, | ||
| PATH: process.env.PATH ?? "/usr/local/bin:/usr/bin:/bin", | ||
| }, | ||
| }).trim(); // NOSONAR:typescript:S4036 - execFileSync doesn't use shell; PATH explicitly sanitized |
There was a problem hiding this comment.
nitpick: Environment PATH is not actually sanitized here despite the comment, which may be misleading for future reviewers.
The NOSONAR note claims PATH is "explicitly sanitized", but here we simply reuse process.env.PATH when present. While the behavior is fine, the comment overstates the safety guarantees. Consider rephrasing to emphasize that execFileSync avoids a shell and that a safe fallback PATH is used only when process.env.PATH is unset, rather than implying additional sanitization.
Comment on lines
275
to
277
| }, | ||
| strictTransportSecurity: false, | ||
| strictTransportSecurity: { maxAge: 31536000, includeSubdomains: true }, | ||
| }, |
There was a problem hiding this comment.
🚨 suggestion (security): Strict-Transport-Security with includeSubdomains may be risky in non-production environments.
This HSTS config is good for production, but risky if shared with non-HTTPS or ephemeral environments on the same domain/subdomains. Consider restricting it to production only (e.g. NODE_ENV === "production") or making it configurable so HSTS is only enabled on intended public domains.
Suggested change
| }, | |
| strictTransportSecurity: false, | |
| strictTransportSecurity: { maxAge: 31536000, includeSubdomains: true }, | |
| }, | |
| }, | |
| strictTransportSecurity: | |
| process.env.NODE_ENV === "production" | |
| ? { maxAge: 31536000, includeSubdomains: true } | |
| : false, | |
| }, |
Comment on lines
+274
to
+279
| setCookie(h3, dropTokenCookieName, token, { | ||
| expires: expiresAt, | ||
| httpOnly: true, | ||
| secure: getRequestURL(h3).protocol === "https:", | ||
| sameSite: "lax", | ||
| path: "/", |
There was a problem hiding this comment.
🚨 suggestion (security): Relying solely on getRequestURL(h3).protocol for the secure flag may be inaccurate behind proxies.
Because the flag is set from getRequestURL(h3).protocol === "https:", a proxied setup with TLS terminated at a load balancer may still show http: to the app, resulting in cookies without secure even when the client uses HTTPS. If this handler can run behind such infrastructure, consider also checking a trusted indicator of HTTPS (e.g. X-Forwarded-Proto, a trusted proxy setting, or a config/env flag) when deciding whether to set secure.
Suggested implementation:
}
*/
// Determine whether the original client connection is HTTPS.
// This implementation considers both the direct protocol and a trusted "x-forwarded-proto" header,
// which is commonly set by reverse proxies / load balancers when terminating TLS.
private isSecureRequest(h3: H3Event): boolean {
const urlSecure = getRequestURL(h3).protocol === "https:";
const forwardedProtoHeader = h3.node.req.headers["x-forwarded-proto"];
const forwardedProto =
typeof forwardedProtoHeader === "string"
? forwardedProtoHeader.split(",")[0]?.trim().toLowerCase()
: undefined;
const proxySecure = forwardedProto === "https";
return urlSecure || proxySecure;
}
private createSessionCookie(h3: H3Event, expiresAt: Date) { setCookie(h3, dropTokenCookieName, token, {
expires: expiresAt,
httpOnly: true,
secure: this.isSecureRequest(h3),
sameSite: "lax",
path: "/",
});
Comment on lines
+68
to
+77
| const requestOrigin = getRequestURL(h3).origin; | ||
| let redirectUrl: URL; | ||
| try { | ||
| redirectUrl = new URL(result.options.redirect, requestOrigin); | ||
| } catch { | ||
| throw createError({ | ||
| statusCode: 400, | ||
| message: "Invalid redirect URL", | ||
| }); | ||
| } |
There was a problem hiding this comment.
suggestion (bug_risk): Use the validated URL object for the redirect to avoid subtle discrepancies with the original string.
Since you validate via the redirectUrl object but then pass the original string to sendRedirect, you risk discrepancies between what was validated and what is sent. Please use redirectUrl.toString() (or .href) in the redirect so the exact validated (and normalized) URL is used.
Comment on lines
+105
to
+106
| # Run as non-root user for security | ||
| USER node |
There was a problem hiding this comment.
issue (bug_risk): Switching to USER node may require additional file permission adjustments in the image.
Running as node is a solid hardening step, but it can fail at runtime if files or directories (like /app, startup/launch.sh, or writable cache/log paths) remain root-owned without appropriate permissions for the node user. Please verify ownership/permissions across build and deployment environments and add explicit chown/chmod steps where needed to ensure compatibility with USER node.
Comment on lines
+42
to
+51
| it("allows same-origin redirect", async () => { | ||
| vi.mocked(sessionHandler.signin).mockResolvedValue("signin"); | ||
| vi.mocked(authManager.getAuthProviders).mockReturnValue({ | ||
| Simple: false, | ||
| OpenID: { | ||
| authorize: vi.fn().mockResolvedValue({ | ||
| user: { id: "user-1" }, | ||
| options: { redirect: "/dashboard" }, | ||
| claims: {}, | ||
| }), |
There was a problem hiding this comment.
suggestion (testing): Add tests for additional redirect edge cases (absolute same-origin URL, invalid redirect string, and no-redirect scenario).
The suite already covers relative same-origin, cross-origin, and localhost redirects. To fully exercise the new validation logic in oidc/callback.get.ts, please also add tests for:
- An absolute same-origin redirect (e.g.
https://drop.example.com/dashboard) to confirm it’s accepted. - A malformed redirect (e.g.
"::not-a-url") to verify thenew URL(...)path throws andcreateErroris called with a 400. - A missing
options.redirectto confirmsendRedirectis not invoked and the handler completes normally.
Suggested implementation:
});
it("allows same-origin redirect", async () => {
vi.mocked(sessionHandler.signin).mockResolvedValue("signin");
vi.mocked(authManager.getAuthProviders).mockReturnValue({
Simple: false,
OpenID: {
authorize: vi.fn().mockResolvedValue({
user: { id: "user-1" },
options: { redirect: "/dashboard" },
claims: {},
}),
} as unknown as never,
});
const event = createEvent(); // use the same helper / factory used in the other tests
await handler(event); // call the same handler under test used elsewhere in this file
expect(sendRedirect).toHaveBeenCalledWith(event, "/dashboard");
});
it("allows absolute same-origin redirect", async () => {
vi.mocked(sessionHandler.signin).mockResolvedValue("signin");
vi.mocked(authManager.getAuthProviders).mockReturnValue({
Simple: false,
OpenID: {
authorize: vi.fn().mockResolvedValue({
user: { id: "user-1" },
options: {
redirect: "https://drop.example.com/dashboard",
},
claims: {},
}),
} as unknown as never,
});
const event = createEvent();
await handler(event);
expect(sendRedirect).toHaveBeenCalledWith(
event,
"https://drop.example.com/dashboard",
);
});
it("rejects malformed redirect URLs with a 400 error", async () => {
vi.mocked(sessionHandler.signin).mockResolvedValue("signin");
vi.mocked(authManager.getAuthProviders).mockReturnValue({
Simple: false,
OpenID: {
authorize: vi.fn().mockResolvedValue({
user: { id: "user-1" },
options: {
redirect: "::not-a-url",
},
claims: {},
}),
} as unknown as never,
});
const event = createEvent();
await expect(handler(event)).rejects.toEqual(
createError({
statusCode: 400,
statusMessage: expect.stringContaining("Invalid redirect URL"),
}),
);
expect(sendRedirect).not.toHaveBeenCalled();
});
it("handles missing redirect without calling sendRedirect", async () => {
vi.mocked(sessionHandler.signin).mockResolvedValue("signin");
vi.mocked(authManager.getAuthProviders).mockReturnValue({
Simple: false,
OpenID: {
authorize: vi.fn().mockResolvedValue({
user: { id: "user-1" },
options: {},
claims: {},
}),
} as unknown as never,
});
const event = createEvent();
const result = await handler(event);
expect(sendRedirect).not.toHaveBeenCalled();
// Optionally, assert on the default behavior if the handler returns something specific
expect(result).toBeDefined();You will need to align a few identifiers with the rest of the file:
- Replace
handler(event)with the actual function under test (e.g.oidcCallbackHandler(event)or whatever is used in the other tests in this file). - Replace
createEvent()with the real helper / factory used to build theH3Event(e.g.createEvent({ path: "/api/oidc/callback" })or similar). If there is no helper, construct the event exactly as done in the existing tests. - Ensure
sendRedirectandcreateErrorare the same mocked imports used elsewhere in this test suite. If the file asserts errors differently (e.g. usingtoThrowErrorinstead of comparing tocreateError(...)), adjust theawait expect(handler(event)).rejects...assertion to match the existing convention. - Update the
"Invalid redirect URL"substring to match the exact error message thrown inoidc/callback.get.tsif it differs (or loosen the assertion toexpect.anything()if the message is not important).
Comment on lines
+26
to
+33
| describe("Session Cookie Security Attributes", () => { | ||
| beforeEach(() => { | ||
| vi.clearAllMocks(); | ||
| vi.stubGlobal("setCookie", vi.fn()); | ||
| vi.stubGlobal("getRequestURL", vi.fn()); | ||
| vi.stubGlobal("createError", (opts: unknown) => { | ||
| throw opts; | ||
| }); |
There was a problem hiding this comment.
suggestion (testing): Add a test that explicitly verifies session fixation prevention (old session token is invalidated).
The updated SessionHandler.signin correctly issues a new cookie and calls removeSession(oldToken), but the tests don’t verify this behavior.
Given the existing mocked DB session provider, please add a test that:
- Configures
getSession/removeSessionfor anold-token. - Calls
sessionHandler.signinwith a request containing"drop-token=old-token". - Asserts
removeSessionis called with"old-token"andsetCookieis invoked with a token that is defined and not equal to"old-token".
This ensures the session fixation mitigation is covered by tests and protects against regressions.
Suggested implementation:
vi.stubGlobal("createError", (opts: unknown) => {
throw opts;
});
vi.stubGlobal("deleteCookie", vi.fn());
});
it("prevents session fixation by invalidating old session tokens", async () => {
const oldToken = "old-token";
// Ensure a valid URL is returned for the request (signin relies on this)
vi.mocked(getRequestURL).mockReturnValue(
new URL("https://drop.example.com"),
);
// Spy on the session handler's session methods
const getSessionSpy = vi
.spyOn(sessionHandler as any, "getSession")
.mockResolvedValue({
token: oldToken,
userId: "user-1",
});
const removeSessionSpy = vi
.spyOn(sessionHandler as any, "removeSession")
.mockResolvedValue(undefined);
// Call signin with a request containing drop-token=old-token
await sessionHandler.signin(
{
headers: {
cookie: `drop-token=${oldToken}`,
},
} as any,
{} as any,
);
// The old session should have been looked up and then invalidated
expect(getSessionSpy).toHaveBeenCalledWith(oldToken);
expect(removeSessionSpy).toHaveBeenCalledWith(oldToken);
// A new cookie should be set with a new token value
expect(setCookie).toHaveBeenCalled();
const lastCall =
vi.mocked(setCookie as any).mock.calls[
vi.mocked(setCookie as any).mock.calls.length - 1
];
const [cookieName, cookieValue] = lastCall ?? [];
expect(cookieName).toBe("drop-token");
expect(cookieValue).toBeDefined();
expect(cookieValue).not.toBe(oldToken);
});
it("sets secure flag for HTTPS requests", async () => {If sessionHandler does not expose getSession / removeSession as methods (e.g. they live in a separate provider module), adjust the spies accordingly:
- Import the actual session provider module used by
SessionHandler.signin(for example,import * as sessionProvider from "../../../server/internal/session-provider";). - Replace
vi.spyOn(sessionHandler as any, "getSession")/"removeSession"with spies on that provider:const getSessionSpy = vi.spyOn(sessionProvider, "getSession")...const removeSessionSpy = vi.spyOn(sessionProvider, "removeSession")...
- Ensure the cookie name (
"drop-token") in the test matches the real session cookie name if it differs in your implementation.
Comment on lines
+23
to
+32
| describe("OIDC Redirect Validation", () => { | ||
| beforeEach(() => { | ||
| vi.clearAllMocks(); | ||
| vi.stubGlobal("setHeader", vi.fn()); | ||
| vi.stubGlobal("sendRedirect", vi.fn()); | ||
| vi.stubGlobal("getQuery", vi.fn()); | ||
| vi.stubGlobal("getRequestURL", vi.fn()); | ||
| vi.stubGlobal("createError", (opts: unknown) => { | ||
| throw opts; | ||
| }); |
There was a problem hiding this comment.
suggestion (testing): Consider adding at least a smoke test that the Nuxt security rate limiter and request size limiter are enabled with the expected configuration.
These controls are configured in nuxt.config.ts, but nothing verifies they remain enabled with the expected settings. Since they are critical security protections, having at least one automated check would reduce the risk of accidental misconfiguration.
For example, you could:
- Add a small config/unit test that imports the Nuxt config and asserts the
security.rateLimiterandsecurity.requestSizeLimiteroptions match the expected structure/values. - Or add a basic integration/e2e test that (a) sends enough requests to trigger the rate limiter and (b) sends an oversized payload, asserting both are rejected as expected.
Even the lightweight config test would provide a useful safety net against future regressions.
Suggested implementation:
describe("Nuxt security configuration", () => {
it("enables Nuxt security rateLimiter and requestSizeLimiter with expected structure", () => {
const security = (nuxtConfig as any).security;
expect(security).toBeDefined();
expect(security?.rateLimiter).toBeDefined();
expect(security?.requestSizeLimiter).toBeDefined();
// Smoke-test the basic structure of the rate limiter configuration
expect(security?.rateLimiter).toMatchObject({
// Adjust these keys/expectations to match your actual nuxt.config.ts
tokensPerInterval: expect.any(Number),
interval: expect.any(String),
});
// Smoke-test the basic structure of the request size limiter configuration
expect(security?.requestSizeLimiter).toEqual(
expect.objectContaining({
// Adjust these keys/expectations to match your actual nuxt.config.ts
maxRequestSizeInBytes: expect.any(Number),
maxUploadFileRequestInBytes: expect.any(Number),
}),
);
});
});
describe("OIDC Redirect Validation", () => {`).
Here are the edits:
<file_operations>
<file_operation operation="edit" file_path="server/test/unit/security/oidc-redirect.test.ts">
<<<<<<< SEARCH
describe("OIDC Redirect Validation", () => {
describe("Nuxt security configuration", () => {
it("enables Nuxt security rateLimiter and requestSizeLimiter with expected structure", () => {
const security = (nuxtConfig as any).security;
expect(security).toBeDefined();
expect(security?.rateLimiter).toBeDefined();
expect(security?.requestSizeLimiter).toBeDefined();
// Smoke-test the basic structure of the rate limiter configuration
expect(security?.rateLimiter).toMatchObject({
// Adjust these keys/expectations to match your actual nuxt.config.ts
tokensPerInterval: expect.any(Number),
interval: expect.any(String),
});
// Smoke-test the basic structure of the request size limiter configuration
expect(security?.requestSizeLimiter).toEqual(
expect.objectContaining({
// Adjust these keys/expectations to match your actual nuxt.config.ts
maxRequestSizeInBytes: expect.any(Number),
maxUploadFileRequestInBytes: expect.any(Number),
}),
);
});
});
describe("OIDC Redirect Validation", () => {
REPLACE
</file_operation>
</file_operations>
<additional_changes>
- At the top of
server/test/unit/security/oidc-redirect.test.ts, add an import for your Nuxt config (path may need adjustment depending on your repo layout):
import nuxtConfig from "../../../../nuxt.config";-
Adjust the expectations in the new
describe("Nuxt security configuration", ...)block to match the actual shape and keys ofsecurity.rateLimiterandsecurity.requestSizeLimiterin yournuxt.config.ts.- If you use different keys (e.g.
tokensPerInterval/intervalvstokens/windowMs, ormaxRequestSizevsmaxRequestSizeInBytes), update theexpect.objectContaining/toMatchObjectcalls accordingly. - If the config includes an
enabledflag or similar, you may want to assertenabled: trueas well.
- If you use different keys (e.g.
-
If your test environment does not have global
expectfrom Vitest, addimport { expect } from "vitest";at the top with the other imports.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Critical Security Fixes
Fixes for critical findings from security audit.
Session Cookie Security
httpOnly: true,secure: true,sameSite: 'lax',path: '/'to session cookieserver/server/internal/session/index.tsRate Limiting
{ tokensPerInterval: 30, interval: 60000 }server/nuxt.config.tsPassword Length Limit
maxLength: 128to password schema to prevent DoSserver/server/api/v1/auth/signin/simple.post.tsOIDC Redirect Validation
server/server/api/v1/auth/oidc/callback.get.tsDockerfile Security
USER nodedirective to run as non-rootDockerfileVerification
pnpm --filter drop typecheckpassespnpm --filter drop lintpassespnpm --filter drop testpassesRelated
Closes #TBD
Summary by Sourcery
Harden authentication and session handling based on security audit findings, improve HTTP and container security defaults, and add regression tests for critical security behaviors.
New Features:
Bug Fixes:
Enhancements:
Build:
Deployment:
Tests: