@kaavee315draftchecks…fix/decrypt-json-validation → master2 files · +82 −0updated 5d agoAdds runtime schema validation to decryptConnectedAccountPayloadWithEncryptionKey in apps/apollo/src/lib/connected_accounts/utils/securityUtils.ts. After securityProvider.decryptJson<ConnectionData>() succeeds, the decrypted value is now passed through ConnectionDataSchema.safeParse(). On a schema mismatch the function returns Err(ConnectedAccountError.InternalServerError); on success it returns Ok(parsed.data). The goal was to stop a corrupted / tampered / mis-keyed ciphertext that decrypts to valid-but-wrong JSON from silently propagating as ConnectionData.
A test block was added to securityUtils.test.ts exercising the path through the public getDecryptedConnectedAccountPayload helper.
REVIEW NOTE (kept as DRAFT — do not merge): this change as written has behavioral regressions on a hot read path and a failing test. See "Open concerns" below. Left as-is for human decision because the fix requires abandoning the fail-closed intent, which is a design call.
Legacy-format rejection (critical). getDecryptedConnectedAccountPayload previously returned the raw decrypted payload for ALL successfully-decrypted data, including old-format (non-{authScheme,val}) connections. Callers depend on this: parseConnectionDataAndValidate is intentionally lenient (logs + returns Ok even when safeParse fails), isConnectionDataInNewFormat gates legacy handling, and patch_connected_account.ts returns a tailored "uses legacy data format" BadRequest only after a successful decrypt. With this change, old-format connections now return Err (500) at the decrypt layer, so decryptConnectionData (tool-execution auth path) fails outright and the legacy user-facing messages are never reached.
Top-level field stripping (critical). ConnectionDataSchema is z.object({ authScheme, val }) with NO top-level .catchall, so parsed.data drops any top-level key outside {authScheme,val}. Returning Ok(parsed.data) instead of Ok(decryptedPayload.val) silently removes the top-level refresh_token that getAuthorizationParams.ts (the "EXTREMELY TEMPORARY FIX, VERY JANK" block) reads at connectionDataDecrypted.val.refresh_token. Verified against zod 3: a valid payload with a top-level refresh_token parses successfully but loses that field.
Failing test (major). The "malformed payload" test asserts result.val.message matches /Error parsing decrypted connected account data/, but the inner-encrypted ciphertext takes the legacy-fallback branch in getDecryptedConnectedAccountPayload, so the top-level message is actually Error decrypting connected account data (the parse error is nested in baseError.encryptionKeyLayerError). This assertion would fail in CI.
Suggested direction: validate for observability only (log a warning on \!safeParse.success) and always return the original Ok(decryptedPayload.val) so no field is stripped and no successfully-decrypted payload is rejected; or scope the reject to a verified-safe set of statuses after confirming no legacy-format rows remain in prod.
node_modules is not installed in this worktree so local typecheck/tests were not run.{authScheme:'oauth2', val:{status:'active',access_token}, refresh_token} payload passes safeParse but the parsed result drops the top-level refresh_token, and that an old-format payload (no authScheme/val) fails safeParse.getDecryptedConnectedAccountPayload (decryptConnectionData, getConnectionStatus, isConnectionInNewFormat, patch_connected_account, revoke, triggers, link, reinitiate) to confirm the legacy-passthrough contract this change breaks.🤖 Generated with Claude Code