Repository navigation
feat(slack): add encrypted slack_oauth_credentials store - #5134
Merged
Merged
Conversation
Slack bot tokens are currently stored unencrypted in
platform_integrations.metadata.access_token. This adds an encrypted store
alongside it and dual-writes to both, so the change is reversible: nothing
reads the new store yet.
- New slack_oauth_credentials table, one row per Slack platform_integrations
row, cascading on delete. Refresh/expiry columns are included but inert
until Slack token rotation is enabled, so that step needs no migration.
- Scheme constant, AAD builder, and advisory-lock key in worker-utils so the
write path and a future refresh loop cannot drift.
- RSA keyed-envelope encryption via SLACK_CREDENTIAL_KEYSET_JSON. Web holds
the private half because the Slack webhook decrypts in-process; the mcp-gateway
keyset is the precedent. The AAD binds each ciphertext to its row, integration,
workspace, owner, secret kind, and credential_version, so ciphertext cannot be
relocated between rows or replayed after a version bump.
- writeSlackCredential re-encrypts under a bumped credential_version on every
write, serialized by a transaction-scoped advisory lock and fenced by a
version compare-and-set.
- Backfill script for existing plaintext tokens, dry-run by default and
idempotent (already-migrated rows are skipped, not re-encrypted).
A separate table rather than columns on platform_oauth_credentials: that row is
validated by a .strict() Zod schema over a whole-table select in
git-token-service, so adding a column there fails every GitLab refresh closed
to reconnect_required.
The dual-write is deliberately best-effort while the store is write-only, so an
unconfigured keyset cannot break Slack installs. It becomes required, and moves
into the same transaction as the platform_integrations write, when the read
paths move over.
granted_scopes stays null for now: the Chat SDK adapter returns only
{ botToken, botUserId, teamName } and discards the raw oauth.v2.access
response, so the real granted scopes are not observable on the install path.
…l write The backfill computed its already-migrated set per batch, before the per-integration write, while writeSlackCredential unconditionally replaces an existing row. A reinstall landing between the two wrote a fresh credential that the backfill then overwrote with the token from its pre-run metadata snapshot, which Slack has revoked by that point. Add createSlackCredentialIfAbsent, which re-checks existence inside the same advisory-locked transaction as the write, and use it from the backfill. The batch query stays as a pre-filter so already-migrated rows do not each open a transaction. writeSlackCredential keeps its replace semantics for the install path.
Validation only checked that a private key existed under the active key ID, not that it matched active.publicKeyPem. A mispaired keyset encrypts successfully and fails every subsequent decryption, while isSlackCredentialEncryptionConfigured still reports success, so the backfill would write ciphertext nothing can read and the mismatch would only surface at read time in a later step. Compare the SPKI of the declared active public key with the public half derived from the active private key so the misconfiguration fails at load time.
RSO
marked this pull request as ready for review
August 7, 2026 13:32
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Contributor
Code Review SummaryStatus: 1 Issue Found | Recommendation: Address before merge Executive SummaryWell-constructed encrypted credential store with solid AAD binding, advisory-lock serialization, and a reversible dual-write; the only finding is a missing Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (20 files)
Fix these issues in Kilo Cloud Reviewed by kimi-k3 · Input: 96.4K · Output: 20.5K · Cached: 1.6M Review guidance: REVIEW.md from base branch |
jeanduplessis
approved these changes
Aug 7, 2026
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
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.
Step 2 of the Slack bot token remediation. Slack bot tokens are currently stored unencrypted in
platform_integrations.metadata.access_token. This adds an encrypted store alongside it and dual-writes both.Nothing reads the new store yet. This is deliberately reversible — revert the commit and behaviour is unchanged.
Design decisions worth reviewing
Why a separate table instead of columns on
platform_oauth_credentials.GitLabOAuthCredentialRowSchema(packages/worker-utils/src/gitlab-credential.ts:106) is.strict(), andgitlab-oauth-credential-refresher.ts:138selects the whole table row. Adding any column toplatform_oauth_credentialstherefore fails every GitLab refresh closed toreconnect_required. That's a hard blocker, not a style preference.Why RSA keyed envelope with the private key in web. The plan called for AAD binding
credential_versioninto the ciphertext.encryptWithSymmetricKey(packages/encryption/src/encryption.ts:186) never callssetAADand has no parameter for it, and neither does BYOK'sencryptApiKey— onlyencryptWithPublicKeysupports AAD. So "symmetric like the Google/BYOK precedent" and "AAD-bound ciphertext" were not both available off the shelf.We chose the keyed envelope with web holding both halves. This inverts the intent of the GitLab/Bitbucket split (where the private key lives only in
git-token-service), but the Slack webhook is handled inapps/weband must decrypt to call Slack, so an RSA split would mean a service hop per Slack event.mcp-gatewayalready does exactly this (apps/web/src/lib/mcp-gateway/grant-service.ts:9). The threat being defended is a stolen database dump, not a compromise of web's environment — an attacker with web's env already hasSLACK_CLIENT_SECRET.What the AAD buys. Each ciphertext is bound to its row id, integration, Slack team, owner, secret kind, and
credential_version. Consequences, each covered by a test:writeSlackCredentialmust bumpcredential_versionon every write, under an advisory lock and a version compare-and-set.Why the dual-write is best-effort. The store is write-only, so an unconfigured or failing keyset must not break Slack installs. There is an explicit test for this, with a comment stating the expectation flips when reads move over. At that point the write becomes required and moves into the same transaction as the
platform_integrationswrite.Changes
packages/db/src/schema.ts,0208_melodic_matthew_murdock.sqlpackages/worker-utils/src/slack-credential.tsapps/web/src/lib/integrations/platforms/slack/credential-keyset.ts.../slack/credential-encryption.ts.../slack/credential-store.tsapps/web/src/lib/integrations/slack-service.ts:196-231apps/web/src/scripts/d2026-08-07_backfill-slack-oauth-credentials.tsThe migration is purely additive. Rotation bookkeeping columns are included but inert, so enabling Slack token rotation later needs no further migration.
The keyset validator rejects a config that can encrypt but not decrypt its own writes — otherwise that misconfiguration would be silent until the first read.
Verification
src/lib/integrations+src/lib/bot; 341 inworker-utilsweb,db,worker-utils;pnpm formatrunpnpm drizzle migratepackages/db/src/schema.test.ts: 40/41 pass; the failure is pre-existing (see below)Before merge
pnpm web:env set SLACK_CREDENTIAL_KEYSET_JSON— shape documented inENVIRONMENT.md:269. Until it's set the dual-write no-ops into Sentry, which is safe but means no tokens are actually being encrypted.Pre-existing issues found, not fixed here
packages/db/src/schema.test.tsnever runs.packages/dbhas notestscript, and although web's jesttestMatchlistspackages/db/src/**/*.test.ts, jest's defaultrootsisapps/web, so the entry is dead.packages/db/AGENTS.mdcites it as the schema migration-consistency test. Its stale reference to0204_brainy_baron_strucker.sql(renamed to0204_drop_cost_insight_tables.sql) is the proof it's unwatched — that's the 1 failing assertion above.installationProviderdoes not exist in@chat-adapter/slack@4.27.0. Zero references repo-wide and absent fromSlackAdapterConfigin the installed types. The follow-up step's plan to wire an installation provider needs re-verifying against real 4.36.0 types.The install path cannot observe the real OAuth response.
slackAdapter.handleOAuthCallbackreturns only{ botToken, botUserId, teamName }, discardingexpires_in,refresh_token,enterprise_id, and the granted scopes. Sogranted_scopesstays null and the existinggetMissingSlackScopesbug (it compares against our hardcoded wish-list, so it can never detect a real gap) is not yet fixed. Rotation support will need either an adapter bump or our ownoauth.v2.accessexchange.