feat(gitlab): encrypt GitLab integration credentials at rest#4539
Conversation
Code Review SummaryStatus: 1 Issue Found | Recommendation: Address before merge Executive SummaryBranch history was rewritten since the last review pass (the prior review SHA is no longer an ancestor of HEAD), so this pass re-reviewed the full PR diff; the highest-risk new issue is an inconsistent unhandled-throw error path in Overview
Issue Details (click to expand)WARNING
Previously Reported Findings — Status
Files Reviewed (86 files)
Fix these issues in Kilo Cloud Previous Review Summaries (6 snapshots, latest commit 7554245)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 7554245)Status: No Issues Found | Recommendation: Merge Executive SummaryIncremental review since the last pass: the migration now stages the previously-flagged destructive Files Reviewed (7 files)
Previous review (commit 613ad3e)Status: No Issues Found | Recommendation: Merge Executive SummaryIncremental review since the last pass: the only change is removal of a stale Files Reviewed (3 files)
Previous review (commit 4d3a7de)Status: No Issues Found | Recommendation: Merge Executive SummaryIncremental review since the last pass: the only change is a test-only fixture cleanup removing a stale Files Reviewed (1 file)
Previous review (commit a8ba27f)Status: 2 Issues Found | Recommendation: Address before merge Executive SummaryHistory was rewritten since the last review (the PR is now a single squashed commit), so this is a fresh full review; the highest-risk new finding is non-concurrent index creation on the live Overview
Issue Details (click to expand)WARNING
Previously Flagged, Re-Verified (not new, not re-reported)
Files Reviewed (89 files)
Fix these issues in Kilo Cloud Previous review (commit 51de2ee)Status: No Issues Found | Recommendation: Merge Executive SummaryThis incremental review covers commit Files Reviewed (12 files)
Previous review (commit 49c7dbf)Status: 2 Issues Found | Recommendation: Address before merge Executive SummaryThe highest-risk finding is a weakened audit safety check in Overview
Issue Details (click to expand)WARNING
Files Reviewed (85 files)
Reviewed by claude-sonnet-5 · Input: 64 · Output: 21.6K · Cached: 2.2M Review guidance: REVIEW.md from base branch |
51de2ee to
a8ba27f
Compare
… harden token handling Squashes branch work onto the credential-encryption prerequisite: - feat(gitlab): encrypt GitLab integration credentials at rest - fix(gitlab): bind credential version into OAuth credential AAD - fix(bitbucket): normalize lastValidatedAt to ISO in workspace access token status The platform_access_token_credentials and platform_oauth_credentials tables duplicated platform and integration_type identity already owned by the parent platform_integrations row. These redundant columns are dropped entirely and credential identity is derived from the platform_integration_id foreign key. Authorization queries in git-token-service now join platform_integrations to enforce ownership rather than trusting the credential's own columns. Stop persisting or filtering on the dropped columns, join platform_integrations where platform membership is required, remove the now-tautological credential profile-mismatch audit checks, and drop the columns from the GitLab and Bitbucket credential row Zod schemas and their test fixtures. fix(credentials): drop stale owned_by_organization_id refs in user test Migration 0186 drops owned_by_organization_id from platform_access_token_credentials (ownership now flows via platform_integration_id), but the GitLab PAT authorizer cleanup test still inserted it, failing the CI typecheck. Remove the stale refs. fix(credentials): drop stale platform field from credential row test mocks Migration 0186 drops the platform column from platform_oauth_credentials (Bitbucket and GitLab). The new strict BitbucketOAuthCredentialRowSchema and GitLabOAuthCredentialRowSchema reject the extra key, so the services returned reconnect_required instead of available. Remove the stale field from credential-row mocks only (integration/parent rows keep platform). fix(db): stage credential column drops for a follow-up PR Migration 0186 dropped owned_by_organization_id, platform, and integration_type from platform_access_token_credentials and platform from platform_oauth_credentials in the same deploy as the code that stopped using them. origin/main still queries platform_oauth_credentials.platform in live Bitbucket authorization paths (Worker + web), so a rolling deploy risks crashing in-flight requests while the migration runs. Keep the columns nullable in the schema (matching the 0185 state already on origin/main) and regenerate 0186 so it only contains additive changes (new columns, indexes, checks, FK) plus DROP NOT NULL widenings. Stop using platform_oauth_credentials.platform everywhere in application code; the column stays in the DB unused until a follow-up PR drops it. Add the deprecated columns as nullable optional fields to the strict Zod credential row schemas so full-table selects that include these columns do not fail runtime validation before the drop.
7554245 to
665bd67
Compare
Summary
Migrates plaintext GitLab OAuth, personal access, and project access tokens out of
platform_integrationsmetadata into the existing encryptedplatform_oauth_credentials/platform_access_token_credentialstables already used by Bitbucket. Private-key decryption and OAuth refresh are centralized ingit-token-service, exposed to server-side web consumers through a minimal internal broker so the web app never handles raw key material.Key pieces:
provider_metadataJSONB,provider_base_url,oauth_client_secret_encrypted, resource-scoped partial unique indexes, andcredential_version > 0checks. No GitLab-specific table is added.@kilocode/worker-utils: Newgitlab-credentialmodule with GitLab-specific envelope schemes, versions, and AAD builders reusing the Bitbucket RSA/envelope helpers; new broker/audit internal-service token audiences.git-token-service: New credential broker, audit, crypto, store, service, OAuth refresher, and HTTP handlers for encrypted GitLab credential read/refresh/audit.audit/backfill/scrub) migration runner with ascripts/dbentrypoint. The GitLab service, adapter, OAuth callback, and downstream callers (code reviews, cloud agent, gastown git-credentials) are updated to source tokens through the broker.ENVIRONMENT.mddocuments the sharedBITBUCKET_OAUTH_CREDENTIAL_*key vars now used by both providers.The migration is staged: encrypted write paths are in place and a backfill migrates existing plaintext rows; legacy plaintext fallback is preserved until a scrub pass confirms all credentials are encrypted.
Verification
No manual testing performed — this is a backend credential-storage change with no user-facing surface. Confidence comes from automated coverage:
@kilocode/dbschema-consistency tests@kilocode/worker-utils(245 tests)git-token-service(474 tests)apps/websuites: gitlab-service, gitlab platform adapter/broker/encryption/migration/metadata, bitbucket cache, cloud-agent helpers, user soft-delete, code-review-status, code-reviews & organization routers (916 tests)pnpm typecheck --changes-only,pnpm lint,pnpm format:checkcleanVisual Changes
N/A
Reviewer Notes
scrubmode requires a passing private-key audit assertion before removing any plaintext fields; review thecredential-migrationaudit/backfill/scrub gating inapps/web/src/lib/integrations/platforms/gitlab/.NOT NULLcolumns were dropped to accommodate GitLab PAT/project tokens that lack OAuth-style fields; confirm no existing Bitbucket write path relies on the old constraints.BITBUCKET_OAUTH_CREDENTIAL_*RSA keypair now encrypts GitLab credentials — verify key provisioning matches the documented shared-key model.softDeleteUserwas updated for the new credential rows; seeapps/web/src/lib/user/index.ts.