fix(oidc): implement better http timeoutes, callbacks, and session lifetimes for oidc#9635
fix(oidc): implement better http timeoutes, callbacks, and session lifetimes for oidc#9635perfectra1n wants to merge 3 commits intomainfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a configurable timeout for OAuth/OIDC requests and synchronizes OIDC session lifetimes with the application's session settings. It also enhances security by implementing session regeneration upon successful OIDC login and improves error logging. Feedback suggests explicitly saving the session after regeneration to ensure state persistence before redirection.
| req.session.regenerate((err) => { | ||
| if (err) { | ||
| log.error(`Failed to regenerate session on OIDC login: ${err}`); | ||
| return reject(err); | ||
| } | ||
| req.session.loggedIn = true; | ||
| req.session.lastAuthState = { | ||
| totpEnabled: false, | ||
| ssoEnabled: true | ||
| }; | ||
| resolve(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
When using req.session.regenerate in an asynchronous context like afterCallback, it is a best practice to explicitly call req.session.save() after modifying the new session's properties. This ensures that the session state is persisted to the store before the middleware proceeds to redirect the user, preventing potential race conditions where the redirect happens before the session is saved.
await new Promise<void>((resolve, reject) => {
req.session.regenerate((err) => {
if (err) {
log.error(`Failed to regenerate session on OIDC login: ${err}`);
return reject(err);
}
req.session.loggedIn = true;
req.session.lastAuthState = {
totpEnabled: false,
ssoEnabled: true
};
req.session.save((saveErr) => {
if (saveErr) {
log.error(`Failed to save session after OIDC regeneration: ${saveErr}`);
return reject(saveErr);
}
resolve();
});
});
});|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request enhances OIDC configuration by adding support for custom HTTP timeouts and scopes, while also improving security through session regeneration upon login and aligning OIDC session lifetimes with application defaults. Additionally, logging for authentication failures has been improved. A suggestion was made to specify the radix in parseInt() when parsing the timeout value to ensure consistent behavior.
| iniGetter: () => getIniSection("MultiFactorAuthentication")?.oauthHttpTimeout, | ||
| defaultValue: 30000, | ||
| transformer: (value: unknown) => { | ||
| const parsed = parseInt(String(value)); |
There was a problem hiding this comment.
It is a best practice to always specify the radix (base) when using parseInt() to avoid any ambiguity in how the string is parsed, especially in older environments or with strings that might have leading zeros.
| const parsed = parseInt(String(value)); | |
| const parsed = parseInt(String(value), 10); |
oauthHttpTimeouttoconfig.ini/ env vars inconfig.ts, wired intogenerateOAuthConfig()inopen_id.ts. Fixes the problem of having to login twice caused by cold-start IdP responses exceeding the library's 5s default.cookieMaxAge. Added asession: { rolling: true, rollingDuration, absoluteDuration }block to the OIDC config - both bounded byconfig.Session.cookieMaxAge. Fixes the problem where the library's silent 24h-rolling / 7d-absolute defaults were the weak link inmin(trilium.sid, appSession).req.session.regenerate(...)around theloggedIn = trueassignment inafterCallback. Brings the OIDC path to parity with the password path (login.ts:133) and closes the session-fixation gap.console.log("user invalid!")withlog.error(...)and the barecatch {}inisTokenValidwith a logged `catch (err). Makes future OIDC misbehavior actually debuggable.