Encrypt Redis-backed auth profile credentials#126
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d6c9b0cf64
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return { | ||
| value, | ||
| needsMigration: this.encryptionAvailable, | ||
| }; |
There was a problem hiding this comment.
Avoid migrating undecryptable legacy credentials
When a value matches the legacy encrypted shape but decrypt(...) fails (for example after key rotation or temporary key misconfiguration), the fallback path returns needsMigration: true, so getSettings will rewrite that ciphertext as if it were plaintext. That irreversibly destroys the original token material and forces re-authentication even if the correct old key is restored later.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| try { | ||
| await this.set(key, settings); |
There was a problem hiding this comment.
Guard migration write against concurrent settings updates
The migration path performs an unconditional SET of the full settings blob during a read. If a user updates settings after the initial redis.get but before this write, the migration write can overwrite that newer state with the stale snapshot that was just read, causing lost updates during the first read-driven migration window.
Useful? React with 👍 / 👎.
Summary
authProfiles[*].credentialandauthProfiles[*].metadata.refreshTokenbefore persisting agent settings to Redisenc:v1:) to prevent double-encryptionValidation
Fixes #117