fix: force admin password reset

[jescalada]

Testing agentic PR review flow

[github-actions]

Hi @jescalada! Thanks for the PR.

I notice a couple of things that need attention:

Description clarity: The current description "Testing agentic PR review flow" doesn't explain what this change actually does or why it's needed. Could you please provide a clearer description of what the admin password reset fix addresses?

Issue link: According to the contributing guidelines, PRs should link to an existing issue. Could you either link this PR to an existing issue using "Fixes #N", "Closes #N", or "Resolves #N", or create a new issue describing the problem this fixes?

Looking forward to your updates!

[github-actions]

Automated Security Review

Summary

Two security issues found: a session fixation vulnerability after password change, and a path-based access control bypass via path traversal or query string manipulation.

Findings

src/service/routes/auth.ts

Path-based access control bypass
The middleware uses req.path to check against PASSWORD_CHANGE_ALLOWED_PATHS. Depending on the Express router mount point and how the application is configured, req.path may include or exclude trailing slashes, query strings, or encoded characters inconsistently. More critically, any route not in the allowlist is blocked, but the allowlist itself is broad — /config and /openidconnect are permitted for users who must change their password. If those endpoints perform sensitive operations, a user in a forced-reset state can still reach them. Review whether every path in the allowlist is safe to expose to a user with a compromised/default credential.

+const PASSWORD_CHANGE_ALLOWED_PATHS = new Set([
+  '/',
+  '/config',
+  '/login',
+  '/logout',
+  '/profile',
+  '/change-password',
+  '/openidconnect',
+  '/openidconnect/callback',
+]);
+
+router.use((req: Request, res: Response, next: NextFunction) => {
+  if (!mustChangePassword(req.user)) {
+    return next();
+  }
+
+  if (PASSWORD_CHANGE_ALLOWED_PATHS.has(req.path)) {
+    return next();
+  }

Recommended fix: Audit each allowed path to confirm it exposes no sensitive data or actions to a user whose identity has not yet been fully verified (i.e., they are still using the default/compromised password). At minimum, remove /config and /profile from the allowlist unless those routes are read-only and contain no sensitive information. Consider using a stricter default-deny list containing only /login, /logout, /change-password, and the OIDC callback.

---

src/service/routes/auth.ts

Session fixation after password change
After a successful password change, req.logout is called and the connect.sid cookie is cleared. However, the response (res.status(200).send(...)) is sent regardless of whether req.logout has completed, because the logout callback's completion is not awaited before sending the response. If req.logout fails or has not yet invalidated the server-side session by the time the response is sent, the old session ID remains valid. An attacker who obtained the session token before the password change (e.g., via session hijacking) could continue using it.

+    req.logout?.((err: unknown) => {
+      if (err) return next(err);
+    });
+    res.clearCookie('connect.sid');
     (req.user as User).mustChangePassword = false;
 
     res.status(200).send({ message: 'Password updated successfully' }).end();

Recommended fix: Move the response inside the req.logout callback so it is only sent after the session has been fully invalidated, and regenerate the session before issuing a new one:

req.logout?.((err: unknown) => {
  if (err) return next(err);
  res.clearCookie('connect.sid');
  res.status(200).send({ message: 'Password updated successfully' }).end();
});

This ensures the old session is destroyed before the client is told the operation succeeded, preventing reuse of the pre-change session token.

---

Disclaimer: This review is AI-generated. Please validate the findings before fixing.

[github-actions]

Thanks for the contribution!

The description "Testing agentic PR review flow" does not explain what this PR actually changes or why. Could you update it to describe the problem being fixed (e.g., what the forced admin password reset addresses, what the root cause was, and how the fix works)?

Please also link this PR to a relevant issue. If one does not exist yet, consider opening one first. From CONTRIBUTING.md:

"Check for existing issues: Search open issues before starting work. If none exists, create one describing the change."
"Push & open a PR: Target the main branch. Fill in the PR template and link the relevant issue."

Adding a Fixes #N or Closes #N line in the PR description will connect the issue automatically.

[jescalada]

/security-review

[github-actions]

Thanks for the contribution, jescalada!

The PR description "Testing agentic PR review flow" does not explain what the change does or why it is needed. Could you update it to describe what the forced admin password reset fixes, what the root cause was, and how the change addresses it?

Please also link this PR to a relevant issue. If one does not exist yet, consider opening one before merging. From CONTRIBUTING.md:

"Check for existing issues: Search open issues before starting work. If none exists, create one describing the change."
"Push & open a PR: Target the main branch. Fill in the PR template and link the relevant issue."

Adding a Fixes #N or Closes #N line in the PR description will connect the issue automatically.