Security hardening: password-hash upgrade path at the Workers PBKDF2 cap, secret-store AAD/versioning, prod rate-limiter fail-closed, audit fixes, SECURITY.md - #1382
Conversation
…ersioning, audit fixes, SECURITY.md Co-Authored-By: Kent C. Dodds <me@kentcdodds.com>
…e retirement criterion Co-Authored-By: Kent C. Dodds <me@kentcdodds.com>
Co-Authored-By: Kent C. Dodds <me@kentcdodds.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
|
🔎 Preview deployed: https://kody-pr-1382.kody-a99.workers.dev Worker: Mocks:
|
|
bugbot review |
|
@coderabbitai review |
Bugbot couldn't run — GitHub account mismatchThe GitHub account linked to your Cursor account does not match the PR author. Please ensure you're using the correct GitHub account, or run Bugbot from a team that covers this repository. |
✅ Action performedReview finished.
|
📝 WalkthroughWalkthroughThe PR adds context-bound encryption for user and platform secrets, transparent password-hash upgrades, production authentication-binding validation, security documentation, MCP retirement criteria, and a dependency override. ChangesSecurity hardening
MCP retirement policy
Dependency override
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant SecretService
participant Crypto as crypto.ts
User->>SecretService: Save or resolve secret
SecretService->>Crypto: Encrypt or decrypt with userSecretContext
Crypto-->>SecretService: Context-bound value
SecretService-->>User: Return result
sequenceDiagram
participant Admin
participant PlatformApps as platform-apps.ts
participant Crypto as crypto.ts
Admin->>PlatformApps: Rename OAuth app
PlatformApps->>Crypto: Decrypt with old slug context
Crypto-->>PlatformApps: Return client secret
PlatformApps->>Crypto: Encrypt with new slug context
Crypto-->>PlatformApps: Return rebound ciphertext
PlatformApps-->>Admin: Store renamed app
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
packages/worker/src/mcp/secrets/crypto.node.test.ts (1)
43-72: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider adding the platform OAuth context case.
The suite covers
userSecretContextbinding well. It does not coverplatformOauthAppContext. A test that encrypts underapp:oneand fails to decrypt underapp:twowould lock in the slug-binding invariant thatrenamePlatformOauthAppdepends on.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/worker/src/mcp/secrets/crypto.node.test.ts` around lines 43 - 72, Extend the v2 context-binding test around encryptSecretValue and decryptSecretValue to cover platformOauthAppContext: encrypt with app:one, then assert decryption with app:two rejects with the existing unable-to-decrypt error. Preserve the current user-context, tampering, and unknown-version assertions.packages/worker/src/mcp/secrets/crypto.ts (2)
113-128: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueConsider removing the default empty context.
encryptStringWithPurposeanddecryptStringWithPurposedefaultcontextto''. Call sites that forget to pass a context silently produce unbound ciphertext, and the compiler does not flag it. If every current caller has a natural identity to bind, makecontextrequired so new call sites must decide explicitly. If some callers are genuinely identity-free (for example, stateless cookie payloads), keep the default and document that choice at the declaration.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/worker/src/mcp/secrets/crypto.ts` around lines 113 - 128, Review all callers of encryptStringWithPurpose and decryptStringWithPurpose, then make context required if each caller has a natural identity to bind, preserving the existing context values. If any caller is intentionally identity-free, retain the default empty context and document that contract at both declarations.
94-109: 🔒 Security & Privacy | 🔵 Trivial | 🏗️ Heavy liftConfirm the plan to retire the legacy no-AAD branch.
The two-part branch decrypts without AAD, so a legacy ciphertext stays swappable between rows until it is rewritten. The comment at Line 13 states that legacy payloads upgrade "whenever the value is re-encrypted on write", but no read path re-encrypts. A stored secret that a user never re-saves keeps the weaker binding indefinitely.
Consider one of these follow-ups:
- Add an upgrade-on-read step that re-encrypts with the v2 format after a successful legacy decrypt.
- Run a one-time backfill, then delete the two-part branch.
Either option gives a concrete end date for the compatibility path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/worker/src/mcp/secrets/crypto.ts` around lines 94 - 109, Update the legacy two-part handling in the decryption flow to provide a concrete migration path: after successful decryption, re-encrypt the plaintext using the current AAD-bound format and persist the upgraded payload, or remove this compatibility branch after implementing an equivalent one-time backfill. Ensure the legacy payload is not left indefinitely without AAD binding.packages/worker/src/integrations/platform-apps.ts (1)
385-390: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse the existing row accessor instead of inline SQL.
getPlatformOauthAppRowBySlugalready returns the row includingclient_secret_encrypted; Line 494 uses it for exactly this column. Calling it here removes the inlineSELECTand keeps the table-column knowledge in one place.♻️ Proposed refactor
- const secretRow = await input.db - .prepare( - `SELECT client_secret_encrypted FROM platform_oauth_apps WHERE slug = ?`, - ) - .bind(slug) - .first<{ client_secret_encrypted: string | null }>() + const secretRow = await getPlatformOauthAppRowBySlug({ db: input.db, slug })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/worker/src/integrations/platform-apps.ts` around lines 385 - 390, Replace the inline SQL query in the surrounding function with the existing getPlatformOauthAppRowBySlug accessor, then read client_secret_encrypted from its returned row while preserving the current null handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/contributing/decisions/0005-mcp-dual-lane-stateless-migration.md`:
- Around line 64-66: Update the second retirement criterion in the dual-lane
migration decision so blank or unknown legacy client identities are not
classified as noise. Use a stable server-derived identity for such requests, or
explicitly make any blank/unknown bucket block retirement when it exceeds the
threshold; preserve the existing treatment of named clients and the 100-request
limit.
- Around line 61-64: Update criterion 1 in the migration decision to use a
denominator matching the metric’s scope: total authenticated, verified,
non-suspended, instrumented `/mcp` requests. Alternatively, add and reference a
metric covering every `/mcp` request; do not describe the existing scoped metric
as total route traffic.
- Around line 68-70: Update the retirement plan to add a D1 migration that drops
the mcp_agent_sessions table when the McpAgent lane is removed, and remove all
consumers of that table. Keep the owner check in place until the existing
session purge path is removed, then retire it with the purge path and MCP_OBJECT
changes.
In `@docs/contributing/security.md`:
- Around line 316-317: Update the security documentation around resolveSecret to
explicitly document that legacy unversioned AES-GCM ciphertext lacks owner
binding and may decrypt when copied to another user's row, including that
metadata-only writes preserve it. State that only v2 provides owner binding, or
direct implementation to reject legacy payloads unless they are owner-bound.
In `@packages/worker/src/password-upgrade.ts`:
- Around line 19-24: Update the password upgrade helper in
packages/worker/src/password-upgrade.ts:19-24 to perform an atomic conditional
update matching both userId and the original storedHash, and ignore zero-row
results. In packages/worker/src/app/handlers/auth.ts:506-515 and
packages/worker/src/oauth-handlers.ts:856-865, retain these call sites only
after the helper provides that conditional-write protection; no direct changes
are required there.
---
Nitpick comments:
In `@packages/worker/src/integrations/platform-apps.ts`:
- Around line 385-390: Replace the inline SQL query in the surrounding function
with the existing getPlatformOauthAppRowBySlug accessor, then read
client_secret_encrypted from its returned row while preserving the current null
handling.
In `@packages/worker/src/mcp/secrets/crypto.node.test.ts`:
- Around line 43-72: Extend the v2 context-binding test around
encryptSecretValue and decryptSecretValue to cover platformOauthAppContext:
encrypt with app:one, then assert decryption with app:two rejects with the
existing unable-to-decrypt error. Preserve the current user-context, tampering,
and unknown-version assertions.
In `@packages/worker/src/mcp/secrets/crypto.ts`:
- Around line 113-128: Review all callers of encryptStringWithPurpose and
decryptStringWithPurpose, then make context required if each caller has a
natural identity to bind, preserving the existing context values. If any caller
is intentionally identity-free, retain the default empty context and document
that contract at both declarations.
- Around line 94-109: Update the legacy two-part handling in the decryption flow
to provide a concrete migration path: after successful decryption, re-encrypt
the plaintext using the current AAD-bound format and persist the upgraded
payload, or remove this compatibility branch after implementing an equivalent
one-time backfill. Ensure the legacy payload is not left indefinitely without
AAD binding.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4b9202ef-c42c-4df6-b683-b72fe4771415
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (19)
SECURITY.mddocs/contributing/decisions/0005-mcp-dual-lane-stateless-migration.mddocs/contributing/security.mdpackage.jsonpackages/shared/src/password-hash.node.test.tspackages/shared/src/password-hash.tspackages/worker/src/app/env.tspackages/worker/src/app/handler.node.test.tspackages/worker/src/app/handlers/admin-platform-integrations.tspackages/worker/src/app/handlers/auth.tspackages/worker/src/integrations/platform-apps.node.test.tspackages/worker/src/integrations/platform-apps.tspackages/worker/src/mcp/capabilities/admin/admin-platform-oauth-app-save.tspackages/worker/src/mcp/secrets/crypto.node.test.tspackages/worker/src/mcp/secrets/crypto.tspackages/worker/src/mcp/secrets/service.tspackages/worker/src/oauth-handlers.tspackages/worker/src/password-upgrade.tspackages/worker/src/remote-connector/settings-service.ts
| Retire the legacy lane when **both** hold for the window: | ||
|
|
||
| 1. legacy-lane requests are **< 1% of total `/mcp` requests**, and | ||
| 2. **no single `client_name` accounts for more than 100 legacy requests** (a |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- decision file ---'
sed -n '1,120p' docs/contributing/decisions/0005-mcp-dual-lane-stateless-migration.md
printf '%s\n' '--- producer candidates ---'
fd -i 'protocol-metrics' .
printf '%s\n' '--- metric references ---'
rg -n -C 3 'protocol-metrics|Analytics Engine|legacy.?lane|client_name|mcp' packages/worker docs/contributing/decisions/0005-mcp-dual-lane-stateless-migration.md 2>/dev/null | head -n 300Repository: kentcdodds/kody
Length of output: 29749
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- protocol metrics outline ---'
ast-grep outline packages/worker/src/mcp/protocol-metrics.ts
printf '%s\n' '--- protocol metrics source ---'
cat -n packages/worker/src/mcp/protocol-metrics.ts
printf '%s\n' '--- protocol metrics tests ---'
cat -n packages/worker/src/mcp/protocol-metrics.node.test.ts
printf '%s\n' '--- route instrumentation and auth flow references ---'
rg -n -C 5 'recordMcp|MCP_PROTOCOL_EVENTS|writeDataPoint|isLegacyRequest|protocol.?metric|authenticate|authenticated' packages/worker/src/mcp packages/worker/src | head -n 500Repository: kentcdodds/kody
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- metric call sites ---'
rg -n -C 12 'recordMcpProtocolEvent|classifyMcpProtocolRequest' packages/worker/src --glob '!**/*.test.*'
printf '%s\n' '--- MCP route handlers ---'
rg -l 'handleMcpRequest|createMcpHandler|McpAgent' packages/worker/src/mcp packages/worker/src | head -n 80
printf '%s\n' '--- focused handler definitions ---'
rg -n -C 20 'function handleMcpRequest|export .*handleMcpRequest|async function handleMcpRequest|createMcpHandler' packages/worker/src/mcp packages/worker/src | head -n 300Repository: kentcdodds/kody
Length of output: 32366
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- authenticated request boundary ---'
sed -n '164,285p' packages/worker/src/mcp-auth.ts
printf '%s\n' '--- route dispatch to authentication ---'
rg -n -C 8 'handleMcpRequest\(' packages/worker/src/index.ts packages/worker/src/mcp/index.ts packages/worker/src
printf '%s\n' '--- exact dataset configuration ---'
sed -n '400,420p' packages/worker/wrangler.jsonc
sed -n '644,660p' packages/worker/wrangler.jsonc
printf '%s\n' '--- read-only source invariant probe ---'
python3 - <<'PY'
from pathlib import Path
source = Path("packages/worker/src/mcp-auth.ts").read_text()
metrics = Path("packages/worker/src/mcp/protocol-metrics.ts").read_text()
decision = Path("docs/contributing/decisions/0005-mcp-dual-lane-stateless-migration.md").read_text()
classify = source.index("const classification = await classifyMcpProtocolRequest(request)")
auth_boundary = source.index("const token = await", source.index("export async function handleMcpRequest"))
write = source.index("recordMcpProtocolEvent(env", classify)
lease = source.index("return await withAccountWriteLease", write)
query = decision[decision.index("SELECT blob1 AS lane"):decision.index("```", decision.index("SELECT blob1 AS lane"))]
print("metric_write_after_auth_boundary:", auth_boundary < classify)
print("metric_write_before_lease:", write < lease)
print("query_has_total_aggregate:", "SUM(_sample_interval)" in query and "GROUP BY" not in query)
print("query_groups_by_lane_and_client:", "GROUP BY lane, client_name" in query)
print("producer_counts_one_event:", "doubles: [1]" in metrics)
PYRepository: kentcdodds/kody
Length of output: 9533
🏁 Script executed:
#!/bin/bash
set -e
python3 - <<'PY'
from pathlib import Path
auth = Path("packages/worker/src/mcp-auth.ts").read_text()
metrics = Path("packages/worker/src/mcp/protocol-metrics.ts").read_text()
decision = Path("docs/contributing/decisions/0005-mcp-dual-lane-stateless-migration.md").read_text()
auth_start = auth.index("export async function handleMcpRequest")
token_check = auth.index("const tokenSummary = await helpers.unwrapToken", auth_start)
classification = auth.index("const classification = await classifyMcpProtocolRequest(request)", auth_start)
metric_write = auth.index("recordMcpProtocolEvent(env", classification)
query_start = decision.index("SELECT blob1 AS lane")
query_end = decision.index("```", query_start)
query = decision[query_start:query_end]
print("metrics_after_token_resolution:", token_check < classification)
print("metrics_after_authentication_checks:", "if (!authContext)" in auth[:classification] and "if (!authContext.emailVerified)" in auth[:classification] and "if (authContext.suspended)" in auth[:classification])
print("metric_write_after_classification:", classification < metric_write)
print("query_aggregates_recorded_events_only:", "FROM kody_mcp_protocol_events" in query)
print("query_groups_by_lane_and_client:", "GROUP BY lane, client_name" in query)
print("query_has_explicit_total_mcp_denominator:", "total" in query.lower() and "mcp" in query.lower())
print("producer_emits_one_weighted_event:", "doubles: [1]" in metrics and "writeDataPoint" in metrics)
PYRepository: kentcdodds/kody
Length of output: 442
Align the denominator with the recorded metric scope.
The metric records only authenticated, verified, non-suspended requests. The query therefore does not represent total /mcp route traffic. Define criterion 1 using total authenticated, instrumented /mcp requests, or add a metric for every /mcp request.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/contributing/decisions/0005-mcp-dual-lane-stateless-migration.md` around
lines 61 - 64, Update criterion 1 in the migration decision to use a denominator
matching the metric’s scope: total authenticated, verified, non-suspended,
instrumented `/mcp` requests. Alternatively, add and reference a metric covering
every `/mcp` request; do not describe the existing scoped metric as total route
traffic.
There was a problem hiding this comment.
Fixed in 034bf62 — the criterion now uses total instrumented /mcp requests as the denominator and explains why that population is the right one.
There was a problem hiding this comment.
Skipped: comment is from another GitHub bot.
| 2. **no single `client_name` accounts for more than 100 legacy requests** (a | ||
| named client above that is a real installation that would break, not noise; | ||
| unnamed/blank clients count as noise). |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Do not treat blank client identities as noise.
packages/worker/src/mcp/protocol-metrics.node.test.ts shows supported legacy requests with clientName: '', including header-only tools/call, http:GET, and http:DELETE requests. A real legacy client can therefore exceed 100 requests in the blank bucket while the two retirement criteria still pass. Use a stable server-derived identity, or make blank and unknown legacy traffic block retirement.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/contributing/decisions/0005-mcp-dual-lane-stateless-migration.md` around
lines 64 - 66, Update the second retirement criterion in the dual-lane migration
decision so blank or unknown legacy client identities are not classified as
noise. Use a stable server-derived identity for such requests, or explicitly
make any blank/unknown bucket block retirement when it exceeds the threshold;
preserve the existing treatment of named clients and the 100-request limit.
There was a problem hiding this comment.
Fixed in 034bf62 — the blank/unnamed client_name bucket now explicitly counts as a bucket (header-only legacy clients report no name), not noise.
There was a problem hiding this comment.
Skipped: comment is from another GitHub bot.
| When both hold, remove the `McpAgent` lane, the `mcp_agent_sessions` registry, | ||
| the `MCP_OBJECT` Durable Object, and the session purge path in one change. If a | ||
| later reading fails the criterion again before the removal ships, reset the |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- candidate files ---'
git ls-files 'docs/contributing/decisions/0005-mcp-dual-lane-stateless-migration.md' \
'packages/worker/src/mcp/session-registry.ts' \
'packages/worker/src/mcp/protocol-metrics.ts' \
'packages/worker/src/mcp/*' \
| sed -n '1,120p'
printf '%s\n' '--- decision excerpt ---'
sed -n '35,90p' docs/contributing/decisions/0005-mcp-dual-lane-stateless-migration.md
printf '%s\n' '--- session registry ---'
sed -n '1,130p' packages/worker/src/mcp/session-registry.ts
printf '%s\n' '--- protocol metrics ---'
sed -n '1,220p' packages/worker/src/mcp/protocol-metrics.tsRepository: kentcdodds/kody
Length of output: 17287
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- all legacy-session references ---'
rg -n -C 3 \
'mcp_agent_sessions|purgePersistedMcpAgentSession|listMcpAgentSessionsForUser|MCP_OBJECT|McpAgent' \
--glob '!node_modules' --glob '!dist' --glob '!build' \
packages docs migrations 2>/dev/null | sed -n '1,260p'
printf '%s\n' '--- relevant worker files ---'
fd -i -t f 'mcp|migration|wrangler' packages/worker | \
rg 'session|durable|migration|wrangler|mcp' | sed -n '1,220p'
printf '%s\n' '--- database schema references ---'
rg -n -C 4 'CREATE TABLE[^;]*mcp_agent_sessions|mcp_agent_sessions' \
--glob '*.sql' --glob '*.ts' --glob '*.md' . | sed -n '1,240p'Repository: kentcdodds/kody
Length of output: 41622
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- account deletion implementation ---'
ast-grep outline packages/worker/src/app/account-deletion.ts
rg -n -C 8 \
'listMcpAgentSessionsForUser|purgeForAccountDeletion|purgePersistedMcpAgentSession|mcp_agent_sessions|MCP_OBJECT' \
packages/worker/src/app/account-deletion.ts packages/worker/src/mcp packages/worker/src/index.ts packages/worker/src/mcp-auth.ts \
| sed -n '1,300p'
printf '%s\n' '--- MCP Durable Object implementation ---'
fd -i -t f 'index.ts' packages/worker/src/mcp --exec sh -c '
for f do
if rg -q "class MCP|purgeForAccountDeletion|McpAgent" "$f"; then
printf "%s\n" "--- $f ---"
ast-grep outline "$f"
rg -n -C 8 "class MCP|purgeForAccountDeletion|purgePersistedMcpAgentSession|McpAgent" "$f"
fi
done
' sh
printf '%s\n' '--- Durable Object migration/config declarations ---'
rg -n -C 8 \
'MCP|migrations|new_sqlite_classes|deleted_classes|Analytics Engine|MCP_PROTOCOL_EVENTS' \
packages/worker/wrangler.jsonc packages/worker/worker-configuration.d.ts packages/worker/src \
| sed -n '1,300p'Repository: kentcdodds/kody
Length of output: 42728
🌐 Web query:
Cloudflare Durable Objects delete existing object storage when a Durable Object class or namespace binding is removed, and recommended cleanup before migration
💡 Result:
Yes, deleting a Durable Object class permanently removes its namespace and all associated stored data [1]. To safely retire a Durable Object class, you must follow the migration process for declarative exports (or the legacy migration flow for older projects) [1][2]. Simply removing the binding from your configuration file is not sufficient and will generally prevent the deployment from succeeding if the class is still defined in your code or referenced in other ways [1][3]. Recommended process for retiring/deleting a Durable Object class: 1. Remove references: Ensure your Worker code no longer references the Durable Object class [1]. 2. Update configuration: In your Wrangler configuration file, change the class entry in the exports map to a deleted tombstone [1][4]. { "exports": { "YourClassName": { "type": "durable-object", "state": "deleted" } } } 3. Check for external dependencies: Cloudflare will prevent the deletion if other Workers in your account still bind to the namespace [1]. You must remove those bindings first [1]. 4. Redeploy: Apply the change by deploying your Worker [1]. Once the deployment is successful and the namespace is deleted, the tombstone becomes stale [1][5]. 5. Cleanup: After the namespace is deleted, the tombstone entry will appear in the removable_entries list during your deployment output [1]. You can then safely remove the class entry entirely from your exports map in your next configuration update [1][5]. Regarding data cleanup, while deleting the class wipes all data, you may want to explicitly call storage.deleteAll within your Durable Object code before retiring a class if you need to ensure complete deallocation or handle specific application-level data cleanup before the object ceases to exist [6][4]. Keep in mind that for SQLite-backed objects, deleteAll is atomic, whereas for legacy key-value backends, it may require retries if it hits time limits [7][8].
Citations:
- 1: https://developers.cloudflare.com/durable-objects/reference/durable-objects-migrations/
- 2: https://developers.cloudflare.com/durable-objects/reference/durable-object-class-migrations-legacy/
- 3: https://developers.cloudflare.com/durable-objects/observability/troubleshooting/
- 4: https://developers.cloudflare.com/durable-objects/best-practices/rules-of-durable-objects/
- 5: https://developers.cloudflare.com/durable-objects/reference/durable-objects-migrations/index.md
- 6: https://developers.cloudflare.com/durable-objects/best-practices/access-durable-objects-storage/
- 7: https://developers.cloudflare.com/durable-objects/api/sqlite-storage-api/
- 8: https://developers.cloudflare.com/durable-objects/api/legacy-kv-storage-api/
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- migration inventory ---'
git ls-files packages/worker/migrations | sort
for f in $(git ls-files packages/worker/migrations | sort); do
printf '%s\n' "--- $f ---"
rg -n -C 3 'mcp_agent_sessions|DROP TABLE|deleted_classes|MCP' "$f" || true
done
printf '%s\n' '--- deletion ordering and row cleanup ---'
sed -n '360,470p' packages/worker/src/app/account-deletion.ts
sed -n '788,910p' packages/worker/src/app/account-deletion.ts
sed -n '1035,1145p' packages/worker/src/app/account-deletion.ts
printf '%s\n' '--- all direct D1 table cleanup declarations ---'
rg -n -C 3 'DROP TABLE|DELETE FROM .*mcp_agent_sessions|mcp_agent_sessions' \
packages/worker/migrations packages/worker/src \
| sed -n '1,260p'Repository: kentcdodds/kody
Length of output: 35886
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
import re
migration_dir = Path("packages/worker/migrations")
migrations = sorted(migration_dir.glob("*.sql"))
registry_creators = []
registry_drops = []
for path in migrations:
text = path.read_text()
if re.search(r"\b(?:CREATE\s+TABLE|INSERT\s+INTO|DELETE\s+FROM|SELECT\b.*\bFROM)\s+mcp_agent_sessions\b", text, re.I | re.S):
registry_creators.append(path.name)
if re.search(r"\bDROP\s+TABLE\s+(?:IF\s+EXISTS\s+)?mcp_agent_sessions\b", text, re.I):
registry_drops.append(path.name)
consumers = []
for root in (Path("packages/worker/src"), Path("packages/worker/worker-configuration.d.ts")):
paths = [root] if root.is_file() else root.rglob("*")
for path in paths:
if path.is_file() and path.suffix in {".ts", ".tsx", ".js", ".jsonc"}:
text = path.read_text(errors="replace")
if "mcp_agent_sessions" in text:
consumers.append(str(path))
print({"migrations": [p.name for p in migrations]})
print({"registry_references_in_migrations": registry_creators})
print({"registry_drop_migrations": registry_drops})
print({"runtime_or_config_consumers": sorted(consumers)})
PYRepository: kentcdodds/kody
Length of output: 975
Drop mcp_agent_sessions as a D1 migration when retiring the lane.
mcp_agent_sessions is created by 0001-squashed-init.sql, and no later migration removes it. Deleting the MCP Durable Object class removes its object storage, but it does not remove D1 rows. Add a D1 migration to drop the table, remove its consumers, and keep the owner check until the existing purge path is removed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/contributing/decisions/0005-mcp-dual-lane-stateless-migration.md` around
lines 68 - 70, Update the retirement plan to add a D1 migration that drops the
mcp_agent_sessions table when the McpAgent lane is removed, and remove all
consumers of that table. Keep the owner check in place until the existing
session purge path is removed, then retire it with the purge path and MCP_OBJECT
changes.
Source: Coding guidelines
There was a problem hiding this comment.
Fixed in 034bf62 — the retirement list now includes a D1 migration dropping mcp_agent_sessions (removing the DO class doesn't delete the D1 rows).
There was a problem hiding this comment.
Skipped: comment is from another GitHub bot.
| another user's row fails to decrypt. Legacy unversioned ciphertexts still | ||
| decrypt and upgrade to `v2` whenever the value is re-encrypted on write. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 8 \
'legacy|v2\.|additionalData|decryptSecret|encryptSecret|re-?encrypt' \
packages/worker/src/mcp/secretsRepository: kentcdodds/kody
Length of output: 27831
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- crypto implementation ---'
cat -n packages/worker/src/mcp/secrets/crypto.ts | sed -n '70,115p'
printf '%s\n' '--- secret read and write paths ---'
cat -n packages/worker/src/mcp/secrets/service.ts | sed -n '600,750p'
printf '%s\n' '--- documentation ---'
cat -n docs/contributing/security.md | sed -n '300,325p'
printf '%s\n' '--- relevant ownership and lookup helpers ---'
rg -n -C 5 \
'getSecretEntry|bucketId|userId|owner|secret.*scope|scope.*secret' \
packages/worker/src/mcp/secrets packages/worker/src/mcp \
-g '*.ts' | head -n 300
printf '%s\n' '--- standalone no-AAD AES-GCM probe ---'
node - <<'JS'
const { webcrypto } = require('crypto');
const crypto = webcrypto;
(async () => {
const keyBytes = await crypto.subtle.digest(
'SHA-256',
new TextEncoder().encode('mcp-secret-store:primary-secret-store-key-at-least-32-chars!!')
);
const key = await crypto.subtle.importKey('raw', keyBytes, 'AES-GCM', false, ['encrypt', 'decrypt']);
const iv = crypto.getRandomValues(new Uint8Array(12));
const plaintext = new TextEncoder().encode('legacy-value');
const ciphertext = await crypto.subtle.encrypt({ name: 'AES-GCM', iv }, key, plaintext);
for (const aad of [undefined, new TextEncoder().encode('secret-store:user-a'), new TextEncoder().encode('secret-store:user-b')]) {
try {
const options = { name: 'AES-GCM', iv };
if (aad !== undefined) options.additionalData = aad;
const result = await crypto.subtle.decrypt(options, key, ciphertext);
console.log(aad === undefined ? 'no AAD: decrypts' : `AAD ${new TextDecoder().decode(aad)}: decrypts`, new TextDecoder().decode(result));
} catch {
console.log(aad === undefined ? 'no AAD: rejects' : `AAD ${new TextDecoder().decode(aad)}: rejects`);
}
}
})();
JSRepository: kentcdodds/kody
Length of output: 31343
Document the legacy ciphertext isolation gap.
If a legacy ciphertext is copied into another user's row, resolveSecret decrypts it because the legacy AES-GCM path uses no AAD. Metadata-only writes preserve the legacy ciphertext. State that only v2 provides owner binding, or reject legacy payloads unless they are owner-bound.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/contributing/security.md` around lines 316 - 317, Update the security
documentation around resolveSecret to explicitly document that legacy
unversioned AES-GCM ciphertext lacks owner binding and may decrypt when copied
to another user's row, including that metadata-only writes preserve it. State
that only v2 provides owner binding, or direct implementation to reject legacy
payloads unless they are owner-bound.
Source: Coding guidelines
There was a problem hiding this comment.
Fixed in 034bf62 — security.md now documents that only v2 provides owner binding, that legacy ciphertexts remain swap-decryptable until rewritten (metadata-only writes preserve them), and that the compatibility branch is deliberately upgrade-on-write and gets removed once no legacy ciphertexts remain.
There was a problem hiding this comment.
Skipped: comment is from another GitHub bot.
| if (!passwordHashNeedsUpgrade(storedHash)) return | ||
| const passwordHash = await createPasswordHash(verifiedPassword) | ||
| await db.update(usersTable, userId, { | ||
| password_hash: passwordHash, | ||
| updated_at: utcSqliteTimestamp(), | ||
| }) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Prevent a stale hash upgrade from overwriting a password change.
The helper verifies storedHash, then updates by userId only. If a password reset changes the hash between those operations, this update restores the old verified password. Use an atomic compare-and-swap that matches both the user ID and the original password_hash. Ignore a zero-row update.
packages/worker/src/password-upgrade.ts#L19-L24: Update only whenpassword_hashstill equalsstoredHash.packages/worker/src/app/handlers/auth.ts#L506-L515: Keep this call only after the helper has conditional-write protection.packages/worker/src/oauth-handlers.ts#L856-L865: Keep this call only after the helper has conditional-write protection.
📍 Affects 3 files
packages/worker/src/password-upgrade.ts#L19-L24(this comment)packages/worker/src/app/handlers/auth.ts#L506-L515packages/worker/src/oauth-handlers.ts#L856-L865
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/worker/src/password-upgrade.ts` around lines 19 - 24, Update the
password upgrade helper in packages/worker/src/password-upgrade.ts:19-24 to
perform an atomic conditional update matching both userId and the original
storedHash, and ignore zero-row results. In
packages/worker/src/app/handlers/auth.ts:506-515 and
packages/worker/src/oauth-handlers.ts:856-865, retain these call sites only
after the helper provides that conditional-write protection; no direct changes
are required there.
There was a problem hiding this comment.
Fixed in 034bf62 — the rehash write is now a compare-and-swap via db.updateMany with where: { id: userId, password_hash: storedHash }, so a concurrent password change can never be overwritten by a stale upgrade; a zero-row update is silently ignored.
There was a problem hiding this comment.
Skipped: comment is from another GitHub bot.
Co-Authored-By: Kent C. Dodds <me@kentcdodds.com>
Preview testing surfaced that every password-auth request 500ed: the production Workers runtime throws NotSupportedError for PBKDF2 above 100,000 iterations (local workerd does not enforce the cap, so unit tests and local e2e could not catch it). Generation and the accepted verification ceiling both sit at the runtime cap now, over-cap hashes are rejected cleanly before deriveBits, and a workers-pool test covers the hash paths in the Workers runtime. Co-Authored-By: Kent C. Dodds <me@kentcdodds.com>
devin-ai-integration
Bot
changed the title
Preview verificationManual testing against the preview deploy Round 1 (600k iterations, head 034bf62) — caught a release blocker. Every Round 2 (head f248a42) — all scenarios pass:
Secret created as Auth checks
Test recording preview: |
1 participant
Intent
First batch of security hardening from the full-repo review: put password hashing at the strongest setting the Workers runtime allows with a transparent upgrade path, bind secret-store ciphertexts to their owner identity, close the "production silently falls back to the D1 auth rate limiter" gap, remediate npm audit findings, and add a disclosure policy.
Summary
packages/shared/src/password-hash.ts): generation stays at PBKDF2-SHA256 100k iterations — preview testing proved Cloudflare's production runtime rejects PBKDF2 above 100,000 iterations (deriveBitsthrowsNotSupportedError; local workerd does not enforce the cap, so unit tests/local e2e can't catch it). The original 600k raise 500ed every auth request on the preview and was reverted. What this PR adds instead: verification rejects over-cap iteration counts cleanly before deriving,passwordHashNeedsUpgrade+upgradePasswordHashIfNeededtransparently rehash lower-strength hashes after successful login via a compare-and-swap (app/handlers/auth.ts,oauth-handlers.ts) so the setting can be raised without a migration if the platform cap lifts, a rehash failure never blocks a valid login, and a workers-pool test covers the hash paths in the Workers runtime.packages/worker/src/mcp/secrets/crypto.ts): newv2.<iv>.<ct>ciphertext format with AES-GCM AADkody.v2|<purpose>|<context>, where context isuser:<userId>for user secrets andapp:<slug>for platform OAuth client secrets. A ciphertext moved to another user's row no longer decrypts. Legacy two-part ciphertexts keep decrypting (no AAD) and upgrade to v2 on next write; unknown versions and tampering reject.renamePlatformOauthAppnow decrypts under the old slug and re-encrypts under the new slug so carried secrets stay decryptable after a rename.app/env.ts): env validation throws whenSENTRY_ENVIRONMENT === 'production'andAUTH_RATE_LIMITERis missing, so the D1 fallback can never silently become the production limiter. Local/preview/test/self-hosted keep the D1 fallback./mcpAND no legacy client bucket (including blank/unnamed) has >100 requests, via the weighted Analytics Engine query — mechanical readout instead of judgment. Retirement includes a D1 migration droppingmcp_agent_sessions.brace-expansion,undici, etc.). No Dependabot/Renovate added.SECURITY.md(private advisory reporting + email fallback);docs/contributing/security.mddocuments the CSRF posture (SameSite=Lax + JSON content types, no tokens) as an explicit invariant, the v2/AAD ciphertext format and its owner-binding boundary (only v2 binds; legacy upgrades on write), the password scheme and the platform iteration cap, and the production rate-limiter guard.Testing
POST /auth500ed at 600k onkody-pr-1382.kody-a99.workers.devwhile the baseline preview returned 401 for the same payload; fixed and covered by a newpassword-hash.workers.test.ts.npm run validate(2027 unit tests, 8 e2e, MCP e2e) passing locally.npm auditandnpm audit --omit=dev: 0 vulnerabilities.System changes
Touches authentication (login rehash machinery + over-cap rejection), secret storage (ciphertext format change, backward compatible), and env validation (new production fail-closed). No migrations; no data rewritten at rest — legacy ciphertexts/hashes upgrade lazily on write/login.
Link to Devin session: https://app.devin.ai/sessions/b5bf26745b254f289a40e061348af106
Requested by: @kentcdodds