feat(session): support password rotation via Record<string, string>#1335
Draft
productdevbook wants to merge 1 commit intomainfrom
Draft
feat(session): support password rotation via Record<string, string>#1335productdevbook wants to merge 1 commit intomainfrom
productdevbook wants to merge 1 commit intomainfrom
Conversation
SessionConfig.password now accepts Record<string, string> for password
rotation. The first key is used for sealing new sessions, all keys
are tried for unsealing (via iron-crypto's existing PasswordHash support).
Usage:
useSession(event, {
password: {
default: "old-password-at-least-32-chars...",
new: "new-password-at-least-32-chars...",
},
});
Sessions sealed with a plain string password use passwordId "default",
so the old password should be keyed as "default" for backward compat.
Closes #1050
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughSession password configuration is extended to support password rotation by allowing the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/utils/session.ts (1)
32-38: Documentation could clarify key ordering semantics.The phrase "first key" relies on JavaScript's object property insertion order, which may not be immediately obvious to all users. Consider adding a brief note about this behavior.
📝 Suggested documentation improvement
/** * Private key used to encrypt session tokens. * * For password rotation, pass a record of `{ id: password }` pairs. - * The first key is used for sealing new sessions, all keys are tried for unsealing. + * The first key (by insertion order) is used for sealing new sessions; + * all keys are tried for unsealing based on the embedded password ID. */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/session.ts` around lines 32 - 38, The documentation for the password field in session.ts is ambiguous about which key is considered the "first key" when a record is provided; clarify that JavaScript object insertion order determines which key is used for sealing new sessions and that consumers should insert the preferred sealing key first (or use an ordered structure if they need explicit ordering). Update the comment on the password: string | Record<string, string> field to note that insertion order of object keys (as per ECMAScript spec) decides the first key used for sealing and that all keys will be attempted for unsealing.test/session.test.ts (1)
115-155: Test covers backward compatibility but not full rotation behavior.The test correctly validates that sessions sealed with the old string password can be unsealed after adding a rotated password map. However, it doesn't verify that new sessions created with the rotated config are sealed using the new password (first key in the map).
Consider adding assertions that new sessions sealed with
rotatedConfigembed the correct password ID, ensuring the rotation actually advances the sealing password.💡 Suggested enhancement
// After line 154, add verification that new sessions use the first key: const newCreateRes = await t.fetch("/rotate/read", { method: "POST", // would need a POST handler headers: { Cookie: oldCookie }, }); // Verify the new session is sealed with id "default" (first key in rotatedConfig)Alternatively, consider reordering the map to
{ new: newPassword, default: oldPassword }to demonstrate that new sessions usenewPasswordwhile old sessions with passwordId "default" still unseal correctly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/session.test.ts` around lines 115 - 155, Add a new route that creates a session using the rotatedConfig (e.g., a POST "/rotate/new" handler that calls useSession(event, rotatedConfig) and writes/returns the session), then in the test perform a fetch to that route and assert the newly-set cookie/session is sealed with the rotatedConfig's first password key (i.e., check the Set-Cookie value or any session sealing metadata returned to contain the password id such as "new" or "default"); if you want the new sessions to use the literal new password, reorder rotatedConfig.password to put the "new" key first ({ new: newPassword, default: oldPassword }) before creating/verifying the new session.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/utils/session.ts`:
- Around line 208-213: The code builds sealPassword from config.password and can
produce {id: undefined, secret: undefined} when config.password is an empty
object, causing seal(session, sealPassword, ...) to fail; add validation before
constructing sealPassword to ensure config.password is either a non-empty string
or an object with at least one key/value (or use Object.entries to extract a
single [id, secret] pair) and throw or return a clear error if it's
empty/invalid, then only call seal(session, sealPassword, ...) when the
validated id and secret are defined.
---
Nitpick comments:
In `@src/utils/session.ts`:
- Around line 32-38: The documentation for the password field in session.ts is
ambiguous about which key is considered the "first key" when a record is
provided; clarify that JavaScript object insertion order determines which key is
used for sealing new sessions and that consumers should insert the preferred
sealing key first (or use an ordered structure if they need explicit ordering).
Update the comment on the password: string | Record<string, string> field to
note that insertion order of object keys (as per ECMAScript spec) decides the
first key used for sealing and that all keys will be attempted for unsealing.
In `@test/session.test.ts`:
- Around line 115-155: Add a new route that creates a session using the
rotatedConfig (e.g., a POST "/rotate/new" handler that calls useSession(event,
rotatedConfig) and writes/returns the session), then in the test perform a fetch
to that route and assert the newly-set cookie/session is sealed with the
rotatedConfig's first password key (i.e., check the Set-Cookie value or any
session sealing metadata returned to contain the password id such as "new" or
"default"); if you want the new sessions to use the literal new password,
reorder rotatedConfig.password to put the "new" key first ({ new: newPassword,
default: oldPassword }) before creating/verifying the new session.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5ca8fc78-91f4-4bd9-a1bf-2acd17c24a54
📒 Files selected for processing (2)
src/utils/session.tstest/session.test.ts
Comment on lines
+208
to
+213
| const sealPassword = | ||
| typeof config.password === "string" | ||
| ? config.password | ||
| : { id: Object.keys(config.password)[0], secret: Object.values(config.password)[0] }; | ||
|
|
||
| const sealed = await seal(session, sealPassword, { |
There was a problem hiding this comment.
Missing validation for empty password record.
If config.password is an empty object {}, this code will create { id: undefined, secret: undefined } and pass it to seal(), which will fail at runtime when trying to use an undefined password.
🛡️ Proposed fix to add validation
const sealPassword =
typeof config.password === "string"
? config.password
- : { id: Object.keys(config.password)[0], secret: Object.values(config.password)[0] };
+ : (() => {
+ const keys = Object.keys(config.password);
+ if (keys.length === 0) {
+ throw new Error("Password record cannot be empty");
+ }
+ return { id: keys[0], secret: config.password[keys[0]] };
+ })();Alternatively, use a simpler approach with Object.entries:
const sealPassword =
typeof config.password === "string"
? config.password
- : { id: Object.keys(config.password)[0], secret: Object.values(config.password)[0] };
+ : (() => {
+ const [[id, secret]] = Object.entries(config.password);
+ if (!id) throw new Error("Password record cannot be empty");
+ return { id, secret };
+ })();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const sealPassword = | |
| typeof config.password === "string" | |
| ? config.password | |
| : { id: Object.keys(config.password)[0], secret: Object.values(config.password)[0] }; | |
| const sealed = await seal(session, sealPassword, { | |
| const sealPassword = | |
| typeof config.password === "string" | |
| ? config.password | |
| : (() => { | |
| const keys = Object.keys(config.password); | |
| if (keys.length === 0) { | |
| throw new Error("Password record cannot be empty"); | |
| } | |
| return { id: keys[0], secret: config.password[keys[0]] }; | |
| })(); | |
| const sealed = await seal(session, sealPassword, { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/session.ts` around lines 208 - 213, The code builds sealPassword
from config.password and can produce {id: undefined, secret: undefined} when
config.password is an empty object, causing seal(session, sealPassword, ...) to
fail; add validation before constructing sealPassword to ensure config.password
is either a non-empty string or an object with at least one key/value (or use
Object.entries to extract a single [id, secret] pair) and throw or return a
clear error if it's empty/invalid, then only call seal(session, sealPassword,
...) when the validated id and secret are defined.
pi0
reviewed
Mar 15, 2026
| const sealPassword = | ||
| typeof config.password === "string" | ||
| ? config.password | ||
| : { id: Object.keys(config.password)[0], secret: Object.values(config.password)[0] }; |
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
2 participants
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.
Summary
Closes #1050
Enables session password rotation by allowing
SessionConfig.passwordto acceptRecord<string, string>in addition to a plain string.How it works
PasswordHash(Record of passwords keyed by ID) — it reads thepasswordIdfrom the sealed token and looks up the matching passwordstringpassword use passwordId"default"internally, so the old password should use"default"as its keyChanges
SessionConfig.password:string→string | Record<string, string>sealSession: extracts first key from record as{ id, secret }for sealingunsealSession: passes record directly tounseal()(already supported)Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit