feat: poc migrating from prefixed-api-key #3841
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughIntroduces key migration support: new POST /v2/keys.migrateKeys endpoint, OpenAPI schemas, handlers, and tests; adds on-demand migration to verifyKey; updates key services to hash inputs and support migrations; extends DB schema (key_migrations table, pending_migration_id column) with generated queries; adds prefixed API key utility; minor docs and build tweaks. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant API as API Gateway
participant Handler as migrateKeys Handler
participant Cache as ApiCache (LiveApiByID)
participant DB as DB (keys, identities, roles, perms, key_migrations)
participant Audit as Audit Logs
Client->>API: POST /v2/keys.migrateKeys {migrationId, apiId, keys[]}
API->>Handler: Dispatch request
Handler->>Cache: SWR(apiId)
alt API not found
Handler-->>API: 404 Not Found (api)
else API found
Handler->>DB: FindKeyMigrationByID(migrationId, workspace)
alt Migration not found
Handler-->>API: 404 Not Found (migration)
else Migration found
Handler->>DB: FindKeysByHash(hashes)
Handler->>DB: FindIdentitiesByExternalId(externalIds)
Handler->>DB: Insert missing identities/roles/perms (bulk)
Handler->>DB: InsertKeys (bulk, with pending_migration_id)
Handler->>Audit: Queue audit entries
Handler-->>API: 200 {migrated[], failed[]}
end
end
sequenceDiagram
autonumber
participant Client
participant API as API Gateway
participant Verify as verifyKey Handler
participant Keys as Key Service
participant DB as DB
participant Cache as Key Cache
Client->>API: POST /v2/keys.verifyKey {key, migrationId?}
API->>Verify: Dispatch
Verify->>Keys: Get(sha256(key))
alt Key not found AND migrationId provided
Verify->>Keys: GetMigrated(raw key, migrationId)
Keys->>DB: FindKeyMigrationByID(migrationId)
alt not found
Keys-->>Verify: NotFound
else found
Keys->>DB: FindLiveKeyByHash(hash(part_from_key))
alt PendingMigrationID matches
Keys->>DB: UpdateKeyHashAndMigration(new hash, clear pending_migration_id, start, updated_at_m)
Keys->>Cache: Invalidate(old hash)
end
Keys-->>Verify: KeyVerifier
end
end
Verify-->>Client: 200 Verification result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
✨ Finishing Touches🧪 Generate unit tests
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 |
…at_poc_migrating_from_prefixed-api-key
…s://github.com/unkeyed/unkey into 08-24-feat_poc_migrating_from_prefixed-api-key
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 93
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
go/Makefile (1)
29-38: Brace expansion is non‑POSIX; loop may fail under /bin/sh (dash).Make’s default SHELL is /bin/sh; {1..10} isn’t portable (fails on dash). Use seq or a while loop.
Apply one of the following:
- @for i in {1..10}; do \ + @for i in $(seq 1 10); do \ echo "Migration attempt $$i..."; \ if docker compose -f ../deployment/docker-compose.yaml run --rm clickhouse_migrator; then \ echo "Migrations completed successfully!"; \ break; \ else \ echo "Migration failed, retrying in 5 seconds..."; \ sleep 5; \ fi; \ doneor strictly POSIX without seq:
- @for i in {1..10}; do \ + @i=1; while [ $$i -le 10 ]; do \ + echo "Migration attempt $$i..."; \ + i=$$((i+1)); \ - echo "Migration attempt $$i..."; \ if docker compose -f ../deployment/docker-compose.yaml run --rm clickhouse_migrator; then \ echo "Migrations completed successfully!"; \ - break; \ + break; \ else \ echo "Migration failed, retrying in 5 seconds..."; \ sleep 5; \ fi; \ - done + donego/apps/api/openapi/spec/paths/v2/keys/verifyKey/V2KeysVerifyKeyRequestBody.yaml (1)
71-76: Align migrationId constraints with DB and other specs; avoid support email in schemaMax length here (256) diverges from migrateKeys (255) and key_migrations.id (255). Also, prefer docs link over email in schema.
Apply:
migrationId: type: string - maxLength: 256 - description: Migrate keys on demand from your previous system. Reach out for migration support at [email protected] + minLength: 3 + maxLength: 255 + pattern: '^[A-Za-z0-9_:-]+$' + description: Migrate keys on demand from your previous system. See product docs for details. example: "m_1234abcd"Optional: add a dedicated example (outside this hunk) showing verification with migrationId.
go/apps/api/openapi/spec/paths/v2/keys/migrateKeys/V2KeysMigrateKeysRequestBody.yaml (1)
25-33: Ensurekeysexample satisfiesminItems: 1The
keysproperty is defined withminItems: 1, but the current “basic” example uses an empty array, which will fail OpenAPI example validation.• File:
go/apps/api/openapi/spec/paths/v2/keys/migrateKeys/V2KeysMigrateKeysRequestBody.yaml(examples.basic.value.keys)
• Fix: include at least oneV2KeysMigrateKeyDataobject in thekeysarrayProposed diff:
value: migrationId: resend apiId: api_123456789 - keys: [] + keys: + – <validKeyExampleHere>Replace
<validKeyExampleHere>with a realV2KeysMigrateKeyDataexample (e.g. includingkeyId,hash, etc.) to satisfy the schema’s requirements.go/pkg/db/bulk_key_insert.sql.go (1)
30-31: Preallocate allArgs to reduce allocations.Minor perf nit for large batches.
Apply:
- var allArgs []any + allArgs := make([]any, 0, len(args)*16) // 16 placeholders per row (owner_id is NULL literal)go/apps/api/routes/v2_keys_create_key/handler.go (1)
151-156: Guard against missing KeyAuth configuration.Two gotchas:
- api.KeyAuth.StoreEncryptedKeys may be false just because KeyAuth is missing.
- InsertKey uses api.KeyAuthID.String without checking Valid; can insert empty keyring_id.
Add a preflight check after loading api to fail early if KeyAuth is absent.
Apply near API fetch:
@@ - api, hit, err := h.ApiCache.SWR(ctx, req.ApiId, func(ctx context.Context) (db.FindLiveApiByIDRow, error) { + api, hit, err := h.ApiCache.SWR(ctx, req.ApiId, func(ctx context.Context) (db.FindLiveApiByIDRow, error) { return db.Query.FindLiveApiByID(ctx, h.DB.RO(), req.ApiId) }, caches.DefaultFindFirstOp) @@ if hit == cache.Null { return fault.New("api not found", fault.Code(codes.Data.Api.NotFound.URN()), fault.Internal("api not found"), fault.Public("The requested API does not exist or has been deleted."), ) } + // Ensure API has KeyAuth configured + if !api.KeyAuthID.Valid { + return fault.New("api not configured for key auth", + fault.Code(codes.App.Precondition.PreconditionFailed.URN()), + fault.Internal("missing key_auth for api"), + fault.Public("This API is not configured for key-based authentication."), + ) + }go/apps/api/openapi/spec/paths/v2/keys/migrateKeys/V2KeysMigrateKeyData.yaml (1)
129-134: Complete the examples.basicKey.value block (currently empty).Empty example blocks break bundling/generators.
basicKey: summary: Basic key description: Migrate this basic key - value: + value: + hash: "sha256_abc123def456" + name: "Payment Service Production Key" + externalId: "user_1234abcd" + enabled: truego/apps/api/openapi/openapi-generated.yaml (1)
5144-5145: Typo in whoami description: “existance” → “existence”.User-facing docs.
- If your rootkey lacks permissions but the key exists, we may return a 404 status here to prevent leaking the existance of a key to unauthorized clients. + If your root key lacks permissions but the key exists, we may return a 404 status here to prevent leaking the existence of a key to unauthorized clients.Note: update the source spec and regenerate.
♻️ Duplicate comments (4)
internal/db/src/schema/keys.ts (1)
161-171: Prefer varchar over enum for algorithm; make id NOT NULL; keep lengths consistentPast feedback asked for a raw string to avoid schema changes per algorithm. Also mark id as notNull for the composite PK.
Apply:
export const keyMigrations = mysqlTable( "key_migrations", { - id: varchar("id", { length: 255 }), + id: varchar("id", { length: 255 }).notNull(), workspaceId: varchar("workspace_id", { length: 256 }).notNull(), - algorithm: mysqlEnum("algorithm", ["github.com/seamapi/prefixed-api-key"]), + algorithm: varchar("algorithm", { length: 255 }).notNull(), }, (table) => ({ idWorkspacePk: primaryKey({ columns: [table.id, table.workspaceId] }), }), );go/internal/services/keys/get_migrated.go (3)
32-36: Scope migration lookup to the tenant workspace (ForWorkspace).Ensure WorkspaceID here matches the root key’s forWorkspaceId to prevent cross-tenant migration visibility.
#!/bin/bash # Inspect zen.Session to confirm AuthorizedWorkspaceID aligns with forWorkspaceId in this context rg -nP --type=go 'type\s+\*?Session|AuthorizedWorkspaceID\(\)' go/ -C3
37-43: API contract for missing migration.If product expects a 400 when migration ID doesn’t exist, propagate an error instead of StatusNotFound.
Point me to the verify route mapping so I can align status handling.
24-26: Rename span to reflect action.Use a more specific, stable trace name, e.g., "keys.verify.migrate".
- ctx, span := tracing.Start(ctx, "keys.GetMigrated") + ctx, span := tracing.Start(ctx, "keys.verify.migrate")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (12)
go/gen/proto/ctrl/v1/build.pb.gois excluded by!**/*.pb.go,!**/gen/**go/gen/proto/ctrl/v1/openapi.pb.gois excluded by!**/*.pb.go,!**/gen/**go/gen/proto/ctrl/v1/routing.pb.gois excluded by!**/*.pb.go,!**/gen/**go/gen/proto/ctrl/v1/service.pb.gois excluded by!**/*.pb.go,!**/gen/**go/gen/proto/ctrl/v1/version.pb.gois excluded by!**/*.pb.go,!**/gen/**go/gen/proto/deploy/assetmanagerd/v1/asset.pb.gois excluded by!**/*.pb.go,!**/gen/**go/gen/proto/deploy/billaged/v1/billing.pb.gois excluded by!**/*.pb.go,!**/gen/**go/gen/proto/deploy/builderd/v1/builder.pb.gois excluded by!**/*.pb.go,!**/gen/**go/gen/proto/metal/vmprovisioner/v1/vmprovisioner.pb.gois excluded by!**/*.pb.go,!**/gen/**go/gen/proto/vault/v1/object.pb.gois excluded by!**/*.pb.go,!**/gen/**go/gen/proto/vault/v1/service.pb.gois excluded by!**/*.pb.go,!**/gen/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (61)
apps/docs/errors/unkey/data/migration_not_found.mdx(1 hunks)go/Makefile(1 hunks)go/apps/api/openapi/config.yaml(0 hunks)go/apps/api/openapi/gen.go(3 hunks)go/apps/api/openapi/openapi-generated.yaml(5 hunks)go/apps/api/openapi/openapi-split.yaml(1 hunks)go/apps/api/openapi/spec/paths/v2/keys/createKey/V2KeysCreateKeyRequestBody.yaml(1 hunks)go/apps/api/openapi/spec/paths/v2/keys/migrateKeys/V2KeysMigrateKeyData.yaml(1 hunks)go/apps/api/openapi/spec/paths/v2/keys/migrateKeys/V2KeysMigrateKeysMigration.yaml(1 hunks)go/apps/api/openapi/spec/paths/v2/keys/migrateKeys/V2KeysMigrateKeysRequestBody.yaml(1 hunks)go/apps/api/openapi/spec/paths/v2/keys/migrateKeys/V2KeysMigrateKeysResponseBody.yaml(1 hunks)go/apps/api/openapi/spec/paths/v2/keys/migrateKeys/V2KeysMigrateKeysResponseData.yaml(1 hunks)go/apps/api/openapi/spec/paths/v2/keys/migrateKeys/index.yaml(1 hunks)go/apps/api/openapi/spec/paths/v2/keys/verifyKey/V2KeysVerifyKeyRequestBody.yaml(1 hunks)go/apps/api/routes/register.go(2 hunks)go/apps/api/routes/v2_keys_create_key/handler.go(6 hunks)go/apps/api/routes/v2_keys_migrate_keys/200_test.go(1 hunks)go/apps/api/routes/v2_keys_migrate_keys/400_test.go(1 hunks)go/apps/api/routes/v2_keys_migrate_keys/401_test.go(1 hunks)go/apps/api/routes/v2_keys_migrate_keys/403_test.go(1 hunks)go/apps/api/routes/v2_keys_migrate_keys/404_test.go(1 hunks)go/apps/api/routes/v2_keys_migrate_keys/handler.go(1 hunks)go/apps/api/routes/v2_keys_verify_key/handler.go(2 hunks)go/apps/api/routes/v2_keys_verify_key/migration_test.go(1 hunks)go/apps/api/routes/v2_keys_verify_key/resend_demo_test.go(1 hunks)go/internal/services/keys/get.go(2 hunks)go/internal/services/keys/get_migrated.go(1 hunks)go/internal/services/keys/interface.go(1 hunks)go/pkg/codes/constants_gen.go(1 hunks)go/pkg/codes/unkey_data.go(2 hunks)go/pkg/db/bulk_key_insert.sql.go(3 hunks)go/pkg/db/bulk_key_migration_insert.sql.go(1 hunks)go/pkg/db/identity_find_many_by_external_id.sql_generated.go(1 hunks)go/pkg/db/key_find_by_id.sql_generated.go(2 hunks)go/pkg/db/key_find_for_verification.sql_generated.go(4 hunks)go/pkg/db/key_find_live_by_hash.sql_generated.go(4 hunks)go/pkg/db/key_find_live_by_id.sql_generated.go(4 hunks)go/pkg/db/key_find_many_by_hash.sql_generated.go(1 hunks)go/pkg/db/key_insert.sql_generated.go(5 hunks)go/pkg/db/key_list_by_key_auth_id.sql_generated.go(3 hunks)go/pkg/db/key_list_live_by_auth_id.sql_generated.go(4 hunks)go/pkg/db/key_migration_find_by_id.sql_generated.go(1 hunks)go/pkg/db/key_migration_insert.sql_generated.go(1 hunks)go/pkg/db/key_update_hash_and_migration.sql_generated.go(1 hunks)go/pkg/db/models_generated.go(3 hunks)go/pkg/db/querier_bulk_generated.go(1 hunks)go/pkg/db/querier_generated.go(13 hunks)go/pkg/db/queries/identity_find_many_by_external_id.sql(1 hunks)go/pkg/db/queries/key_find_for_verification.sql(1 hunks)go/pkg/db/queries/key_find_many_by_hash.sql(1 hunks)go/pkg/db/queries/key_insert.sql(2 hunks)go/pkg/db/queries/key_migration_find_by_id.sql(1 hunks)go/pkg/db/queries/key_migration_insert.sql(1 hunks)go/pkg/db/queries/key_update_hash_and_migration.sql(1 hunks)go/pkg/db/schema.sql(3 hunks)go/pkg/hash/manual/main.go(1 hunks)go/pkg/prefixedapikey/LICENSE(1 hunks)go/pkg/prefixedapikey/prefixedapikey.go(1 hunks)go/pkg/prefixedapikey/prefixedapikey_test.go(1 hunks)go/pkg/zen/middleware_openapi_validation.go(1 hunks)internal/db/src/schema/keys.ts(3 hunks)
💤 Files with no reviewable changes (1)
- go/apps/api/openapi/config.yaml
🧰 Additional context used
🧠 Learnings (8)
📚 Learning: 2025-08-04T07:44:39.438Z
Learnt from: CR
PR: unkeyed/unkey#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-04T07:44:39.438Z
Learning: Applies to **/*_test.go : Use table-driven tests in Go
Applied to files:
go/Makefile
📚 Learning: 2025-08-04T07:44:39.438Z
Learnt from: CR
PR: unkeyed/unkey#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-04T07:44:39.438Z
Learning: Applies to **/*_test.go : Organize Go integration tests with real dependencies
Applied to files:
go/Makefile
📚 Learning: 2025-08-14T16:25:48.167Z
Learnt from: Flo4604
PR: unkeyed/unkey#3785
File: go/apps/api/routes/v2_keys_reroll_key/401_test.go:52-61
Timestamp: 2025-08-14T16:25:48.167Z
Learning: User Flo4604 requested creation of a GitHub issue to track converting all test files to use table-driven test patterns as a broader codebase improvement, following the suggestion made during review of go/apps/api/routes/v2_keys_reroll_key/401_test.go.
Applied to files:
go/apps/api/routes/v2_keys_migrate_keys/400_test.gogo/apps/api/routes/v2_keys_verify_key/migration_test.gogo/apps/api/routes/v2_keys_migrate_keys/403_test.gogo/apps/api/routes/v2_keys_migrate_keys/200_test.gogo/apps/api/routes/v2_keys_migrate_keys/404_test.gogo/apps/api/routes/v2_keys_migrate_keys/401_test.gogo/apps/api/routes/v2_keys_verify_key/resend_demo_test.go
📚 Learning: 2025-07-17T14:24:20.403Z
Learnt from: Flo4604
PR: unkeyed/unkey#3631
File: go/pkg/db/bulk_keyring_insert.sql.go:23-25
Timestamp: 2025-07-17T14:24:20.403Z
Learning: In go/pkg/db/bulk_keyring_insert.sql.go and similar bulk insert generated files, hardcoded zero values for fields like size_approx and size_last_updated_at are intentional and reflect the original SQL query structure, not missing parameters.
Applied to files:
go/pkg/db/key_migration_insert.sql_generated.gogo/pkg/db/queries/key_migration_insert.sqlgo/pkg/db/key_update_hash_and_migration.sql_generated.gogo/pkg/db/bulk_key_migration_insert.sql.gogo/pkg/db/queries/key_insert.sqlgo/pkg/db/queries/key_update_hash_and_migration.sqlgo/pkg/db/key_insert.sql_generated.gogo/pkg/db/bulk_key_insert.sql.go
📚 Learning: 2025-08-21T12:37:40.996Z
Learnt from: Flo4604
PR: unkeyed/unkey#3821
File: apps/dashboard/lib/trpc/routers/key/updateRootKeyPermissions.ts:74-74
Timestamp: 2025-08-21T12:37:40.996Z
Learning: Root keys in Unkey have two workspace fields: `workspaceId` (always set to env().UNKEY_WORKSPACE_ID for the Unkey workspace that owns the key) and `forWorkspaceId` (set to ctx.workspace.id for the user's workspace that the key is for). When querying root keys, the system filters by forWorkspaceId to get keys for the current user's workspace, but the returned rootKey.workspaceId is always the Unkey workspace ID.
Applied to files:
go/internal/services/keys/get_migrated.go
📚 Learning: 2025-04-08T09:34:24.576Z
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#2872
File: apps/dashboard/lib/trpc/routers/ratelimit/createNamespace.ts:36-39
Timestamp: 2025-04-08T09:34:24.576Z
Learning: In the Unkey dashboard, when making database queries involving workspaces, use `ctx.workspace.id` directly instead of fetching the workspace separately for better performance and security.
Applied to files:
go/internal/services/keys/get_migrated.go
📚 Learning: 2025-04-08T09:34:24.576Z
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#2872
File: apps/dashboard/lib/trpc/routers/ratelimit/createNamespace.ts:36-39
Timestamp: 2025-04-08T09:34:24.576Z
Learning: When querying or updating namespaces in the Unkey dashboard, always scope the operations to the current workspace using `eq(table.workspaceId, ctx.workspace.id)` to prevent cross-workspace access.
Applied to files:
go/internal/services/keys/get_migrated.go
📚 Learning: 2025-08-21T15:54:45.198Z
Learnt from: chronark
PR: unkeyed/unkey#3825
File: go/internal/services/usagelimiter/limit.go:38-0
Timestamp: 2025-08-21T15:54:45.198Z
Learning: In go/internal/services/usagelimiter/limit.go, the UpdateKeyCreditsDecrement operation cannot be safely wrapped with db.WithRetry due to the lack of idempotency mechanisms in the current tech stack. Retrying this non-idempotent write operation risks double-charging users if the first attempt commits but the client sees a transient error.
Applied to files:
go/apps/api/routes/v2_keys_create_key/handler.go
🪛 Gitleaks (8.27.2)
go/pkg/prefixedapikey/prefixedapikey_test.go
40-40: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
91-91: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
362-362: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
apps/docs/errors/unkey/data/migration_not_found.mdx
42-44: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
go/apps/api/routes/v2_keys_verify_key/resend_demo_test.go
77-77: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
78-78: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
87-87: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
91-91: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 LanguageTool
apps/docs/errors/unkey/data/migration_not_found.mdx
[grammar] ~6-~6: Use correct spacing
Context: ...-- err:unkey:data:api_not_found json Example { "meta": { "requestId": "req_2c9a0jf23l4k567" }, "error": { "detail": "The requested Migration could not be found", "status": 404, "title": "Not Found", "type": "https://unkey.com/docs/api-reference/errors-v2/unkey/data/migration_not_found" } } ## What Happened? This error occurs when y...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~22-~22: Use correct spacing
Context: ..._not_found" } } ``` ## What Happened? This error occurs when you're trying to ...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~24-~24: Use correct spacing
Context: ... that doesn't exist in the Unkey system. Before attempting to migrate keys, please contact [email protected] for assistance. Common scenarios that trigger this error...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~31-~31: Use correct spacing
Context: ...ommon scenarios that trigger this error: - Using an incorrect API ID in your reques...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~33-~33: There might be a mistake here.
Context: ...ing an incorrect API ID in your requests - Referencing an API that has been deleted...
(QB_NEW_EN)
[grammar] ~34-~34: There might be a mistake here.
Context: ...Referencing an API that has been deleted - Attempting to access an API in a workspa...
(QB_NEW_EN)
[grammar] ~35-~35: There might be a mistake here.
Context: ... in a workspace you don't have access to - Typos in API names when using name-based...
(QB_NEW_EN)
[grammar] ~36-~36: Use correct spacing
Context: ... API names when using name-based lookups Here's an example of a request that woul...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~38-~38: Use correct spacing
Context: ...a request that would trigger this error: bash # Attempting to create a key for a non-existent API curl -X POST https://api.unkey.com/v2/keys.migrateKeys \ -H "Content-Type: application/json" \ -H "Authorization: Bearer unkey_YOUR_API_KEY" \ -d '{ "apiId": "your_api_id", "migrationId": "nonexistent_migration_id", "keys": [{}] }' ## How To Fix Before attempting t...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~52-~52: Use correct spacing
Context: ... "keys": [{}] }' ``` ## How To Fix Before attempting to migrate keys, please contact [email protected] for assistance.
(QB_NEW_EN_OTHER_ERROR_IDS_5)
🪛 golangci-lint (2.2.2)
go/internal/services/keys/get_migrated.go
64-64: unnecessary conversion
(unconvert)
96-96: unnecessary conversion
(unconvert)
🪛 YAMLlint (1.37.1)
go/apps/api/openapi/spec/paths/v2/keys/migrateKeys/V2KeysMigrateKeysMigration.yaml
[error] 17-17: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (50)
go/pkg/hash/manual/main.go (1)
13-25: Confirm these tokens are non-sensitive test fixtures.Public repo: ensure values are synthetic and not real customer data. If sourced from tests, reference them; otherwise, rotate and annotate.
go/pkg/zen/middleware_openapi_validation.go (1)
27-34: No functional changes; middleware still validates and short-circuits on 4xx as expected.Whitespace-only; behavior unchanged. Good to keep the early return on invalid requests.
go/pkg/prefixedapikey/LICENSE (1)
1-22: Confirm third‑party license provenance and attribution.MIT license text attributes copyright to “Seam (c) 2022”. Please confirm:
- Source repository/commit for the imported code.
- That upstream license headers are preserved where required in source files.
- Whether a third_party_licenses/NOTICE entry is needed for redistribution.
go/Makefile (1)
49-49: Good addition: format after codegen.Running go fmt after buf/go generate prevents noisy diffs from generated models. LGTM.
go/apps/api/openapi/spec/paths/v2/keys/createKey/V2KeysCreateKeyRequestBody.yaml (1)
27-27: Increase to 255 looks consistent with migration specs.Non‑breaking relaxation. Verify DB column and server‑side validators also allow 255 to avoid 400s from backend.
go/apps/api/routes/register.go (2)
43-43: Import added for v2_keys_migrate_keys is used.Import is referenced below; no action needed.
379-390: v2/keys.migrateKeys: confirm request payload size limitsThe handler’s permission gating is fully covered by the existing 401/403 tests in v2_keys_migrate_keys (see 403_test.go and 401_test.go), so root-key permissions are enforced. However, the default middleware chain (withTracing, withMetrics, withLogging, withPanicRecovery, withErrorHandling, withValidation) does not explicitly cap the request body size. Please verify that your server’s global or per-route configuration enforces an adequate payload limit for bulk key migrations, or consider adding a size-limit middleware (e.g. WithBodyLimit) for this endpoint.
Key locations to review:
- go/apps/api/routes/register.go (lines 379–390): migration route registration using defaultMiddlewares
- Default middleware definition (lines 68–75): ensure it includes or is shadowed by a body-size limiter
- Handler implementation in go/apps/api/routes/v2_keys_migrate_keys/handler.go: confirm no missing manual size checks where large payloads are processed
go/pkg/db/key_find_live_by_hash.sql_generated.go (1)
15-16: Generated change looks consistent.pending_migration_id is selected, mapped (sql.NullString), and scanned in the correct order. No manual edits needed.
Also applies to: 128-129, 146-147, 260-261
go/pkg/db/key_list_by_key_auth_id.sql_generated.go (1)
15-16: Generated: pending_migration_id propagated and scan order matches SELECT.Looks good and consistent with other queries.
Also applies to: 52-53, 107-108
go/pkg/db/key_list_live_by_auth_id.sql_generated.go (4)
15-16: SELECT adds pending_migration_id in the correct position.Column order remains aligned with scan order (field appears right after environment). Looks good.
137-147: Row type maps pending_migration_id as sql.NullString (correct).Type and db tag are appropriate; downstream code can feature-detect migrations via Valid. No further changes needed.
152-153: Comment stays consistent with query.Doc block includes pending_migration_id; avoid drift with future regen.
281-282: Scan list matches SELECT column order.The added &i.PendingMigrationID aligns with the new SELECT column. Safe.
go/pkg/db/key_insert.sql_generated.go (3)
30-32: Include pending_migration_id in INSERT; ensure schema defaults and indexing.Confirm the DB migration added keys.pending_migration_id NULL default and consider indexing if it’s queried directly during migrations.
Would you like a migration index recommendation based on your common read paths?
112-129: Arg order and added PendingMigrationID are consistent with VALUES.Placeholder count matches; owner_id intentionally null; final arg maps to pending_migration_id. Good.
53-70: Fix Param Name Mismatch: KeyringID → KeyAuthIDThe generated Go types still expose a
KeyringIDfield (withdb:"keyring_id") even though the underlying SQL column is namedkey_auth_id. This mismatch will break all call sites that rely on the correct parameter name. Rather than hand-editing generated code, update your SQL to usesqlc.arg(key_auth_id)and then re-generate.Impacted generated files (confirmed via direct field searches):
- go/pkg/db/key_insert.sql_generated.go (around line 114):
// ... in ExecContext call arg.KeyringID,- go/pkg/db/bulk_key_insert.sql.go (around line 33):
// ... when assembling arguments allArgs = append(allArgs, arg.KeyringID)Action items:
- In your SQL source file, replace the parameter definition to reference
key_auth_id(e.g.,sqlc.arg(key_auth_id)).- Re-run the
sqlc generatestep to update all affected generated files.- Remove any temporary hand-edits to generated code.
⛔ Skipped due to learnings
Learnt from: Flo4604 PR: unkeyed/unkey#3631 File: go/pkg/db/bulk_keyring_insert.sql.go:23-25 Timestamp: 2025-07-17T14:24:20.403Z Learning: In go/pkg/db/bulk_keyring_insert.sql.go and similar bulk insert generated files, hardcoded zero values for fields like size_approx and size_last_updated_at are intentional and reflect the original SQL query structure, not missing parameters.go/apps/api/routes/v2_keys_verify_key/handler.go (2)
18-18: Good: avoid passing raw key by hashing first.Importing and using hash.Sha256(req.Key) reduces exposure of secrets at rest and in logs.
65-77: Migration fallback is correct; clarify emit lifecycle to avoid losing the first emitter.If Get returns an emitter (metrics/audit), reassigning emit on fallback drops the first one. If it must be called even for “not found”, preserve it or document it. One safe pattern is to keep a single emitVerify that always corresponds to the chosen key.
- key, emit, err := h.Keys.Get(ctx, s, hash.Sha256(req.Key)) + key, emit, err := h.Keys.Get(ctx, s, hash.Sha256(req.Key)) if err != nil { return err } - if key.Status == keys.StatusNotFound && req.MigrationId != nil { - key, emit, err = h.Keys.GetMigrated(ctx, s, req.Key, ptr.SafeDeref(req.MigrationId)) + emitVerify := emit + if key.Status == keys.StatusNotFound && req.MigrationId != nil { + migratedKey, migratedEmit, err := h.Keys.GetMigrated(ctx, s, req.Key, ptr.SafeDeref(req.MigrationId)) if err != nil { return err } + key = migratedKey + emitVerify = migratedEmit } @@ - emit() + emitVerify()Additionally, please confirm that Keys.GetMigrated never logs the raw key and that any telemetry clearly flags “migration_fallback_used” without including secret material.
go/apps/api/routes/v2_keys_migrate_keys/403_test.go (1)
85-92: Ensure request body uses minimally valid values to avoid 400 before auth check.If schema validation precedes auth, a malformed key hash/migrationId could yield 400 instead of 403. Consider fixed, simple placeholders that pass validation (e.g., “deadbeef”).
Do you want me to adjust the request payloads across 401/403 suites to guarantee the permission path is what’s being tested?
go/pkg/db/queries/key_insert.sql (1)
18-19: No empty‐string assignments for PendingMigrationID detectedI ran the ripgrep search across the Go codebase for any direct
""orsql.NullString{String: "", Valid: true}usages againstPendingMigrationIDand found none. The onlysql.NullStringinstantiation (inapps/api/routes/v2_keys_migrate_keys/handler.go:175) pertains to aNamefield, notPendingMigrationID. This confirms that callers aren’t inadvertently passing empty strings forPendingMigrationID. You can consider this concern addressed.go/pkg/codes/unkey_data.go (1)
23-28: New dataMigration category: docs and naming are clear.LGTM.
go/pkg/db/key_find_many_by_hash.sql_generated.go (2)
25-56: LGTM: IN-slice expansion and parameterization look correct.Generated code is safe wrt SQL injection and handles empty slices via IN (NULL).
25-35: Optional: preallocate, chunk large inputs, and confirm index on keys(hash)Summary of suggestions:
- Preallocate slices to reduce allocations:
queryParams := make([]interface{}, 0, len(hashes))- Later, when scanning results:
items := make([]FindKeysByHashRow, 0, len(hashes))- If callers may pass thousands of hashes, split
hashesinto smaller chunks (e.g. 500–1000 per query) to avoid exceeding parameter or packet size limits.- Ensure there’s an index on
keys(hash)so this query doesn’t trigger a full table scan. I ran:but didn’t find any matching migration. Please verify that your schema includes something like:rg -n -C2 -iP 'create\s+index.*\bkeys\b.*\bhash\b' --type=sqlIf it’s missing, add it to keep lookups by hash fast.CREATE INDEX idx_keys_hash ON keys(hash);go/pkg/prefixedapikey/prefixedapikey_test.go (1)
15-22: Consider t.Parallel() for subtests if the harness is concurrency-safe.This can cut test time substantially; verify harness/thread-safety first.
Also applies to: 24-33, 75-83, 131-139, 158-166, 216-226, 328-334
go/pkg/db/key_find_by_id.sql_generated.go (2)
13-20: LGTM: added pending_migration_id is wired and scan order matches SELECT.Also applies to: 48-49
21-51: PendingMigrationID field is correctly included in the Key modelThe generated
Keystruct ingo/pkg/db/models_generated.godefines:
PendingMigrationID sql.NullStringtagged asdb:"pending_migration_id", matching the usage in all SQL‐generated methods (includingFindKeyByID).
No further changes are required.go/apps/api/routes/v2_keys_migrate_keys/404_test.go (1)
126-139: Potential 400 vs 404 mismatch for minimal API ID.If IDs must match a strict pattern (e.g., api_xxx), "api" may yield 400 (validation), not 404. Prefer a syntactically valid but nonexistent ID.
- // Test with minimum valid API ID length (3 chars as per validation) - minimalApiID := "api" + // Use a syntactically valid but nonexistent API ID + minimalApiID := uid.New(uid.APIPrefix)go/internal/services/keys/get.go (2)
34-34: LGTM: hash the root key before lookup.Passing hash.Sha256(rootKey) into Get avoids caching/logging raw secrets.
69-75: LGTM: cache/db lookups keyed by sha256Hash.Keyed SWR and the DB query now align with the new contract.
go/pkg/db/key_migration_find_by_id.sql_generated.go (1)
12-20: LGTM: generated query matches schema and parameters.No manual edits recommended.
Also applies to: 22-41
go/pkg/db/bulk_key_insert.sql.go (1)
11-13: Pending migration column wiring looks correct; placeholder/arg count matches.Column list (17) aligns with 16 placeholders + explicit NULL for owner_id; args order maps correctly to columns including PendingMigrationID.
Also applies to: 24-24, 30-48
go/pkg/db/key_migration_insert.sql_generated.go (1)
12-22: LGTM: single-row insert generation is standard.Matches schema and types.
go/apps/api/routes/v2_keys_verify_key/migration_test.go (1)
58-88: Test scenario is solid end-to-end.Covers migration seeding, migrate route, verify with/without migrationId, and DB state post-migration. Nice.
go/apps/api/routes/v2_keys_create_key/handler.go (5)
15-20: Imports for caches look good.No issues; prepares for SWR usage.
87-90: SWR cache usage: LGTM.Good fallback to DB via DefaultFindFirstOp.
92-104: Improved error mapping: LGTM.Clear public/internal messages and proper codes.
107-113: Null-hit handling is correct.Matches cache sentinel semantics; returns consistent 404.
322-322: Inline ratelimit ID generation: LGTM.Simplifies local flow.
go/pkg/db/models_generated.go (3)
371-411: Enum + NULL wrapper generation: LGTM.Matches SQL enum; Scanner/Valuer are correct.
766-791: Verify database migration for PendingMigrationIDAll generated Go models and query methods (e.g. FindKeyByID, FindLiveKeyByHash, KeyInsert/BulkInsertKey, UpdateKeyHashAndMigration, key listing/verifying functions) include the new pending_migration_id column. However, please double-check your SQL migration that adds keys.pending_migration_id to ensure:
- The column is created with the intended NULL/NOT NULL constraint (and default if applicable).
- Any existing schema-altering scripts (e.g., under your migrations/ directory) include this change.
- No other code or reports assume a different nullability or default value for this column.
806-811: No missing workspace scoping on key_migrationsAll SQLC-generated queries for the
key_migrationstable already include theworkspace_idcolumn in their WHERE clauses (e.g.FindKeyMigrationByIDfilters onworkspace_id) and the INSERTs explicitly setworkspace_id. There are no other SELECT/UPDATE/DELETE operations againstkey_migrationswithout workspace scoping. This comment can be resolved.go/pkg/db/key_update_hash_and_migration.sql_generated.go (1)
23-31: Param struct shape: LGTM.Nullable updated_at_m fits the column; API surface is clean.
go/internal/services/keys/get_migrated.go (1)
101-106: Cache invalidation: LGTM.Removing both old and new hashes covers both caches; safe if identical (noop).
go/pkg/db/identity_find_many_by_external_id.sql_generated.go (1)
34-42: IN-clause expansion handling: LGTM.NULL substitution for empty slices avoids syntax errors and returns no rows.
go/apps/api/routes/v2_keys_migrate_keys/200_test.go (1)
99-116: Assert pending migration flag persisted.Verifies core contract of bulk migration path.
key, err := db.Query.FindLiveKeyByID(ctx, h.DB.RO(), res.Body.Data.Migrated[0].KeyId) require.NoError(t, err) keydata := db.ToKeyData(key) + require.True(t, key.PendingMigrationID.Valid) + require.Equal(t, "unkeyed", key.PendingMigrationID.String)go/pkg/db/querier_generated.go (2)
1000-1020: LGTM: generated API extended safely.Additions to InsertKey (pending_migration_id) and UpdateKeyHashAndMigration are consistent and backward-compatible.
Also applies to: 1694-1702
322-325: No action needed:keystable already defines the recommended UNIQUE and INDEX constraintsThe schema in go/pkg/db/schema.sql already includes:
- CONSTRAINT
hash_idxUNIQUE(hash) (line 124), preventing hash duplicates- CREATE INDEX
pending_migration_id_idxONkeys(pending_migration_id) (line 430)These satisfy the recommendation for a unique/hash constraint and an index on pending_migration_id.
go/apps/api/openapi/gen.go (1)
1012-1045: LGTM: new types and VerifyKey request extension.Additions align with the route and handler; no issues spotted in the generated surface.
Also applies to: 2113-2115, 1397-1403
go/pkg/prefixedapikey/prefixedapikey.go (1)
166-177: LGTM: constant-time compare and input decoding are correct.go/apps/api/openapi/openapi-generated.yaml (1)
618-618: Increase name maxLength to 255 — LGTM.
go/apps/api/openapi/spec/paths/v2/keys/migrateKeys/V2KeysMigrateKeysResponseData.yaml
Show resolved
Hide resolved
go/apps/api/openapi/spec/paths/v2/keys/migrateKeys/V2KeysMigrateKeyData.yaml
Outdated
Show resolved
Hide resolved
go/apps/api/openapi/spec/paths/v2/keys/migrateKeys/V2KeysMigrateKeyData.yaml
Show resolved
Hide resolved
2 participants
What does this PR do?
THIS REQUIRES A DB MIGRATION
Add key migration support to the verify endpoint, allowing seamless migration of keys from external systems. This enables users to migrate their existing API keys to Unkey without disrupting their applications.
The PR adds a new
migrationIdparameter to the verify endpoint that, when provided, allows Unkey to verify keys using the algorithm from the previous system. After the first successful verification, the key is automatically rehashed using Unkey's algorithm for future verifications.Fixes #
Type of change
How should this be tested?
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit