feat: copy encryption endpoints (vibed hard on this one) #3710
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📝 WalkthroughWalkthroughThis change introduces new internal API endpoints for vault encryption and decryption, including OpenAPI specifications, handler implementations, and comprehensive tests. It adds a static vault authentication token to configuration, updates route registration, and refactors internal chproxy route authentication to use a new centralized static token validation function. Supporting data models and OpenAPI schemas are included. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API_Server
participant VaultService
Client->>API_Server: POST /_internal/vault/encrypt (Authorization: Bearer TOKEN, JSON body)
API_Server->>API_Server: Validate static token
API_Server->>API_Server: Parse request body (keyring, data)
API_Server->>VaultService: Encrypt(keyring, data)
VaultService-->>API_Server: Encrypted data, keyId
API_Server-->>Client: 200 OK (JSON: encrypted, keyId)
Client->>API_Server: POST /_internal/vault/decrypt (Authorization: Bearer TOKEN, JSON body)
API_Server->>API_Server: Validate static token
API_Server->>API_Server: Parse request body (keyring, encrypted)
API_Server->>VaultService: Decrypt(keyring, encrypted)
VaultService-->>API_Server: Plaintext data
API_Server-->>Client: 200 OK (JSON: plaintext)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
chronark
requested review from
Flo4604,
MichaelUnkey,
imeyer,
mcstepp,
ogzhanolguncu and
perkinsjr
as code owners
|
Thank you for following the naming conventions for pull request titles! 🙏 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (30)
deployment/docker-compose.yaml(1 hunks)go/apps/api/config.go(1 hunks)go/apps/api/openapi/gen.go(2 hunks)go/apps/api/openapi/openapi-generated.yaml(2 hunks)go/apps/api/openapi/openapi-split.yaml(1 hunks)go/apps/api/openapi/spec/paths/_internal/chproxy/metrics/index.yaml(1 hunks)go/apps/api/openapi/spec/paths/_internal/chproxy/ratelimits/index.yaml(1 hunks)go/apps/api/openapi/spec/paths/_internal/chproxy/verifications/index.yaml(1 hunks)go/apps/api/openapi/spec/paths/_internal/vault/decrypt/InternalVaultDecryptRequestBody.yaml(1 hunks)go/apps/api/openapi/spec/paths/_internal/vault/decrypt/InternalVaultDecryptResponseBody.yaml(1 hunks)go/apps/api/openapi/spec/paths/_internal/vault/decrypt/index.yaml(1 hunks)go/apps/api/openapi/spec/paths/_internal/vault/encrypt/InternalVaultEncryptRequestBody.yaml(1 hunks)go/apps/api/openapi/spec/paths/_internal/vault/encrypt/InternalVaultEncryptResponseBody.yaml(1 hunks)go/apps/api/openapi/spec/paths/_internal/vault/encrypt/index.yaml(1 hunks)go/apps/api/routes/_internal_chproxy_metrics/handler.go(2 hunks)go/apps/api/routes/_internal_chproxy_ratelimits/handler.go(2 hunks)go/apps/api/routes/_internal_chproxy_verifications/handler.go(2 hunks)go/apps/api/routes/_internal_vault_decrypt/200_test.go(1 hunks)go/apps/api/routes/_internal_vault_decrypt/400_test.go(1 hunks)go/apps/api/routes/_internal_vault_decrypt/401_test.go(1 hunks)go/apps/api/routes/_internal_vault_decrypt/handler.go(1 hunks)go/apps/api/routes/_internal_vault_encrypt/200_test.go(1 hunks)go/apps/api/routes/_internal_vault_encrypt/400_test.go(1 hunks)go/apps/api/routes/_internal_vault_encrypt/401_test.go(1 hunks)go/apps/api/routes/_internal_vault_encrypt/handler.go(1 hunks)go/apps/api/routes/register.go(2 hunks)go/apps/api/routes/services.go(1 hunks)go/apps/api/run.go(1 hunks)go/pkg/zen/auth.go(2 hunks)go/pkg/zen/auth_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
Learnt from: Flo4604
PR: unkeyed/unkey#3647
File: go/apps/api/openapi/openapi-generated.yaml:3569-3575
Timestamp: 2025-07-22T18:09:41.800Z
Learning: In the Unkey codebase, using non-standard HTTP status code 529 for internal-only endpoints is acceptable and should not be flagged as an issue in future reviews.
Learnt from: Flo4604
PR: unkeyed/unkey#2955
File: go/apps/api/routes/v2_identities_create_identity/handler.go:162-202
Timestamp: 2025-03-19T09:25:59.751Z
Learning: In the Unkey codebase, input validation for API endpoints is primarily handled through OpenAPI schema validation, which occurs before requests reach the handler code. For example, in the identities.createIdentity endpoint, minimum values for ratelimit duration and limit are defined in the OpenAPI schema rather than duplicating these checks in the handler.
📚 Learning: applies to go/deploy/**/*.{env,sh,yaml,yml,json,conf,ini} : all environment variables must follow th...
Learnt from: CR
PR: unkeyed/unkey#0
File: go/deploy/CLAUDE.md:0-0
Timestamp: 2025-07-21T18:05:58.236Z
Learning: Applies to go/deploy/**/*.{env,sh,yaml,yml,json,conf,ini} : All environment variables MUST follow the format UNKEY_<SERVICE_NAME>_VARNAME.
Applied to files:
deployment/docker-compose.yaml
📚 Learning: the docker-compose.yaml file in deployment/ is specifically for development environments, not produc...
Learnt from: chronark
PR: unkeyed/unkey#3638
File: deployment/docker-compose.yaml:81-94
Timestamp: 2025-07-22T09:02:12.495Z
Learning: The docker-compose.yaml file in deployment/ is specifically for development environments, not production. Kafka and other service configurations in this file should be optimized for development convenience rather than production security/hardening.
Applied to files:
deployment/docker-compose.yaml
📚 Learning: in the go/cmd/cli/commands/deploy/ directory, ogzhanolguncu prefers to keep potentially temporary fe...
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#3564
File: go/cmd/cli/commands/deploy/flags.go:17-20
Timestamp: 2025-07-15T14:45:18.920Z
Learning: In the go/cmd/cli/commands/deploy/ directory, ogzhanolguncu prefers to keep potentially temporary features (like UNKEY_DOCKER_REGISTRY environment variable) undocumented in help text if they might be deleted in the future, to avoid documentation churn.
Applied to files:
deployment/docker-compose.yaml
📚 Learning: in go packages, variables defined in one file within a package (like `latencybuckets` and `constlabe...
Learnt from: Flo4604
PR: unkeyed/unkey#3606
File: go/pkg/prometheus/metrics/database.go:29-30
Timestamp: 2025-07-16T10:06:35.397Z
Learning: In Go packages, variables defined in one file within a package (like `latencyBuckets` and `constLabels` in go/pkg/prometheus/metrics/http.go) are accessible from other files in the same package without requiring imports. This is a common pattern for sharing configuration across multiple files within a package.
Applied to files:
go/apps/api/routes/_internal_chproxy_metrics/handler.gogo/apps/api/routes/_internal_chproxy_verifications/handler.gogo/apps/api/routes/_internal_chproxy_ratelimits/handler.go
📚 Learning: in the metrics package init function, panicking on initialization errors is acceptable since it occu...
Learnt from: chronark
PR: unkeyed/unkey#2901
File: go/pkg/otel/metrics/metrics.go:11-22
Timestamp: 2025-02-26T15:07:05.646Z
Learning: In the metrics package init function, panicking on initialization errors is acceptable since it occurs during startup and indicates a fundamental issue that should be addressed immediately rather than allowing the application to continue with incorrect metrics setup.
Applied to files:
go/apps/api/routes/_internal_chproxy_metrics/handler.go
📚 Learning: for debugging database replica usage in go/pkg/db/replica.go, it's acceptable to mark queryrowcontex...
Learnt from: Flo4604
PR: unkeyed/unkey#3606
File: go/pkg/db/replica.go:8-11
Timestamp: 2025-07-16T15:38:53.491Z
Learning: For debugging database replica usage in go/pkg/db/replica.go, it's acceptable to mark QueryRowContext operations as "success" even though SQL errors only surface during row.Scan() calls. The timing metrics are the primary concern for debugging replica performance patterns.
Applied to files:
go/apps/api/routes/_internal_chproxy_metrics/handler.gogo/apps/api/routes/_internal_chproxy_verifications/handler.gogo/apps/api/routes/_internal_chproxy_ratelimits/handler.go
📚 Learning: applies to go/deploy/**/*.{go,js,ts,tsx,py,sh,md,txt,json,yaml,yml,ini,env,conf,html,css,scss,xml,c,...
Learnt from: CR
PR: unkeyed/unkey#0
File: go/deploy/CLAUDE.md:0-0
Timestamp: 2025-07-21T18:05:58.236Z
Learning: Applies to go/deploy/**/*.{go,js,ts,tsx,py,sh,md,txt,json,yaml,yml,ini,env,conf,html,css,scss,xml,c,h,cpp,java,rb,rs,php,pl,sql} : Update relevant anchors when modifying associated code.
Applied to files:
go/apps/api/openapi/spec/paths/_internal/chproxy/metrics/index.yamlgo/apps/api/openapi/spec/paths/_internal/chproxy/ratelimits/index.yaml
📚 Learning: the `cloudflareratelimiter` type definition in `apps/api/src/pkg/env.ts` should not have its interfa...
Learnt from: chronark
PR: unkeyed/unkey#2544
File: apps/api/src/pkg/env.ts:4-6
Timestamp: 2024-10-23T12:05:31.121Z
Learning: The `cloudflareRatelimiter` type definition in `apps/api/src/pkg/env.ts` should not have its interface changed; it should keep the `limit` method returning `Promise<{ success: boolean }>` without additional error properties.
Applied to files:
go/apps/api/openapi/spec/paths/_internal/chproxy/ratelimits/index.yamlgo/apps/api/routes/_internal_chproxy_ratelimits/handler.go
📚 Learning: in the unkey codebase, openapi 3.1 is used, which allows sibling keys (such as `description`) alongs...
Learnt from: Flo4604
PR: unkeyed/unkey#3421
File: go/apps/api/openapi/openapi.yaml:196-200
Timestamp: 2025-07-03T05:58:10.699Z
Learning: In the Unkey codebase, OpenAPI 3.1 is used, which allows sibling keys (such as `description`) alongside `$ref` in schema objects. Do not flag this as an error in future reviews.
Applied to files:
go/apps/api/openapi/spec/paths/_internal/chproxy/ratelimits/index.yaml
📚 Learning: the v2 update identity endpoint (go/apps/api/routes/v2_identities_update_identity/handler.go) intent...
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#3661
File: go/apps/api/routes/v2_identities_update_identity/handler.go:115-119
Timestamp: 2025-07-28T11:47:43.144Z
Learning: The v2 update identity endpoint (go/apps/api/routes/v2_identities_update_identity/handler.go) intentionally uses `ExternalId` field instead of the unified `Identity` field used in other v2 identity endpoints. This is because the update endpoint needs to both find by externalId and potentially update the externalId value, making the specific field name more appropriate than the generic `Identity` field.
Applied to files:
go/apps/api/routes/_internal_chproxy_verifications/handler.go
🧬 Code Graph Analysis (5)
go/apps/api/routes/_internal_chproxy_metrics/handler.go (1)
go/pkg/zen/auth.go (1)
StaticAuth(60-74)
go/apps/api/routes/_internal_chproxy_verifications/handler.go (1)
go/pkg/zen/auth.go (1)
StaticAuth(60-74)
go/apps/api/routes/_internal_chproxy_ratelimits/handler.go (1)
go/pkg/zen/auth.go (1)
StaticAuth(60-74)
go/apps/api/routes/_internal_vault_decrypt/handler.go (5)
go/apps/api/routes/_internal_vault_encrypt/handler.go (1)
Handler(15-19)go/pkg/zen/auth.go (1)
StaticAuth(60-74)go/pkg/zen/request_util.go (1)
BindBody(10-22)go/apps/api/openapi/gen.go (2)
InternalVaultDecryptRequestBody(167-173)InternalVaultDecryptResponseBody(176-179)go/pkg/fault/wrap.go (2)
Wrap(25-67)Internal(75-89)
go/apps/api/routes/register.go (6)
go/apps/api/routes/_internal_chproxy_metrics/handler.go (1)
Handler(15-19)go/apps/api/routes/_internal_chproxy_ratelimits/handler.go (1)
Handler(15-19)go/apps/api/routes/_internal_chproxy_verifications/handler.go (1)
Handler(15-19)go/apps/api/routes/_internal_vault_encrypt/handler.go (1)
Handler(15-19)go/pkg/clickhouse/interface.go (1)
ClickHouse(38-41)apps/dashboard/lib/vault.ts (1)
Vault(20-76)
🪛 Checkov (3.2.334)
deployment/docker-compose.yaml
[LOW] 67-68: Base64 High Entropy String
(CKV_SECRET_6)
[LOW] 68-69: Base64 High Entropy String
(CKV_SECRET_6)
🪛 YAMLlint (1.37.1)
go/apps/api/openapi/spec/paths/_internal/vault/decrypt/InternalVaultDecryptResponseBody.yaml
[error] 8-8: no new line character at the end of file
(new-line-at-end-of-file)
go/apps/api/openapi/spec/paths/_internal/vault/encrypt/index.yaml
[error] 37-37: no new line character at the end of file
(new-line-at-end-of-file)
go/apps/api/openapi/spec/paths/_internal/vault/decrypt/InternalVaultDecryptRequestBody.yaml
[error] 13-13: no new line character at the end of file
(new-line-at-end-of-file)
go/apps/api/openapi/spec/paths/_internal/vault/encrypt/InternalVaultEncryptResponseBody.yaml
[error] 13-13: no new line character at the end of file
(new-line-at-end-of-file)
go/apps/api/openapi/spec/paths/_internal/vault/decrypt/index.yaml
[error] 37-37: no new line character at the end of file
(new-line-at-end-of-file)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Test Go API Local / Test
- GitHub Check: Test API / API Test Local
- GitHub Check: Build / Build
- GitHub Check: Test Agent Local / test_agent_local
- GitHub Check: Test Packages / Test
🔇 Additional comments (37)
go/pkg/zen/auth.go (1)
47-74: Excellent security implementation with constant-time comparison.The
StaticAuthfunction correctly usessubtle.ConstantTimeCompareto prevent timing attacks during token validation. The implementation properly reuses the existingBearerfunction and provides comprehensive error handling with appropriate fault codes.go/apps/api/routes/services.go (1)
31-31: LGTM! Consistent with existing authentication token pattern.The
VaultTokenfield follows the same pattern as the existingChproxyTokenfield, maintaining consistency in the codebase structure.go/apps/api/run.go (1)
230-230: LGTM! Clean integration of vault token configuration.The
VaultTokenfield is properly initialized from the configuration and positioned logically alongside the existingChproxyTokenfield.go/apps/api/openapi/spec/paths/_internal/chproxy/verifications/index.yaml (1)
30-30: Correct path reference fixes for error schemas.The updated relative paths correctly reference the error response schemas after the directory structure reorganization. This ensures the OpenAPI specification remains valid and functional.
Also applies to: 36-36
go/apps/api/openapi/spec/paths/_internal/chproxy/metrics/index.yaml (1)
30-30: LGTM! Path corrections align with directory restructuring.The relative path updates from
../../../to../../../../correctly adjust the error schema references to account for the internal API directory restructuring.Also applies to: 36-36
deployment/docker-compose.yaml (1)
68-68: LGTM! Environment variable follows proper naming conventions.The new
UNKEY_VAULT_AUTH_TOKENenvironment variable correctly follows theUNKEY_<SERVICE_NAME>_VARNAMEformat and uses an appropriate test token value for the development environment.go/apps/api/openapi/spec/paths/_internal/chproxy/ratelimits/index.yaml (1)
30-30: LGTM! Consistent path corrections for error schema references.The relative path updates align with the directory restructuring and are consistent with similar changes made to other internal chproxy endpoints.
Also applies to: 36-36
go/apps/api/routes/_internal_chproxy_ratelimits/handler.go (2)
1-1: LGTM! Package rename aligns with internal restructuring.The package name change from
chproxyRatelimitstointernalChproxyRatelimitscorrectly reflects the internal API reorganization.
34-37: Excellent refactoring to centralize authentication logic.Replacing manual token extraction and constant-time comparison with
zen.StaticAuthimproves code maintainability by centralizing authentication logic. This eliminates code duplication across internal endpoints and provides consistent error handling.go/apps/api/openapi/spec/paths/_internal/vault/encrypt/InternalVaultEncryptRequestBody.yaml (1)
1-14: LGTM! Well-defined OpenAPI schema.The schema is correctly structured with appropriate required fields, clear descriptions, and helpful examples. The keyring and data properties are properly defined for the encryption request.
go/apps/api/routes/_internal_chproxy_verifications/handler.go (2)
1-1: Good refactoring of package name for consistency.The package name change to
internalChproxyVerificationsbetter reflects the internal nature of this endpoint.
34-37: Excellent authentication refactoring.Replacing manual token extraction and comparison with
zen.StaticAuthcentralizes the authentication logic, improves maintainability, and ensures consistent error handling across internal endpoints. Thezen.StaticAuthfunction properly handles Bearer token extraction and constant-time comparison.go/apps/api/openapi/spec/paths/_internal/vault/encrypt/index.yaml (1)
13-31: Well-structured endpoint definition.The request body, success response, and error handling are properly defined with appropriate schema references and descriptions.
go/apps/api/routes/_internal_vault_encrypt/200_test.go (1)
1-88: LGTM! Comprehensive test coverage for success scenarios.The test suite provides excellent coverage of encryption scenarios including edge cases like empty data and large payloads. The test structure follows established patterns and assertions are appropriate for verifying encryption operations.
go/apps/api/openapi/spec/paths/_internal/vault/decrypt/index.yaml (1)
1-37: LGTM! Well-structured OpenAPI specification for internal endpoint.The specification correctly configures the internal vault decrypt endpoint with appropriate response codes, schema references, and internal-only markers. The security bypass is intentional since the endpoint implements its own static token authentication.
go/apps/api/routes/_internal_chproxy_metrics/handler.go (2)
1-1: LGTM! Package rename aligns with internal route structure.The package rename from
chproxyMetricstointernalChproxyMetricscorrectly reflects the_internalroute prefix pattern used throughout the codebase.
34-37: LGTM! Clean refactoring to centralized authentication.The replacement of manual bearer token extraction and constant-time comparison with the centralized
zen.StaticAuthfunction improves code consistency and maintainability. This aligns the authentication approach with other internal routes.go/apps/api/routes/register.go (2)
8-13: LGTM! Clean import organization with consistent naming.The import aliases using the
_internalprefix properly reflect the route structure and maintain consistency across internal endpoints.
115-136: LGTM! Proper conditional registration of vault routes.The vault routes are correctly registered only when both the vault service and token are available. The middleware stack is consistent with other internal routes, and the handler initialization follows established patterns.
go/apps/api/routes/_internal_vault_encrypt/401_test.go (2)
13-95: LGTM! Comprehensive authorization test coverage.The test suite provides excellent coverage of authorization scenarios including edge cases like malformed headers, case sensitivity, and various token validation failures. The test structure and assertions are well-designed for verifying authentication behavior.
85-94: Authorization header extra-space handling is correctThe
Bearerfunction ingo/pkg/zen/auth.goalready trims both the entire header and the extracted token:
- Line 31:
header = strings.TrimSpace(header)- Line 38:
bearer = strings.TrimSpace(bearer)Since
StaticAuthcallsBearer, the scenario with" Bearer correct-token-123 "will successfully yieldcorrect-token-123and return 200 as the test expects. No changes needed.go/apps/api/routes/_internal_vault_decrypt/200_test.go (1)
14-162: Excellent comprehensive test coverage for the decrypt endpoint.The test suite demonstrates excellent practices:
- Round-trip encryption/decryption validation ensures end-to-end functionality
- Comprehensive test cases covering edge cases (empty data, large payloads, Unicode/special characters)
- Proper use of parallelized tests and consistent token configuration
- Appropriate assertions validating both HTTP status codes and response content
go/apps/api/routes/_internal_vault_decrypt/401_test.go (1)
13-103: Comprehensive authorization test coverage with correct status code expectations.The test suite thoroughly validates authentication scenarios:
- Proper distinction between 400 (malformed requests) and 401 (invalid tokens)
- Correct handling of edge cases like missing Bearer prefix and case sensitivity
- The "bearer with extra spaces" test correctly expects 500 (vault failure) rather than 401, indicating the StaticAuth function properly handles whitespace
go/apps/api/routes/_internal_vault_decrypt/handler.go (2)
15-19: Clean handler structure with proper dependencies.The Handler struct properly encapsulates the required dependencies (Vault service, Logger, and authentication Token) with clear field types.
29-52: Well-structured handler implementation following established patterns.The Handle method demonstrates excellent practices:
- Proper authentication using zen.StaticAuth for consistent token validation
- Clean request binding with typed structures
- Appropriate error handling with fault.Wrap for internal vault errors
- Correct HTTP 200 response with structured JSON output
The implementation follows the established patterns in the codebase and maintains good separation of concerns.
go/apps/api/routes/_internal_vault_encrypt/400_test.go (3)
31-40: Correct expectation for missing keyring validation.The test correctly expects a 500 status code for missing keyring, indicating that keyring validation occurs at the vault service level rather than during request binding. This suggests the OpenAPI schema allows empty keyring values, with validation deferred to the business logic layer.
42-51: Appropriate handling of empty data as valid input.The test correctly expects a 200 status code for missing/empty data, as encrypting empty strings is a valid operation. The comment clearly explains this behavior.
53-71: Good validation of JSON binding error handling.The test properly validates that type mismatches in the JSON payload (number for keyring, boolean for data) result in a 400 Bad Request response, which is the correct behavior for JSON binding failures.
go/apps/api/routes/_internal_vault_encrypt/handler.go (2)
15-19: Consistent handler structure matching the decrypt endpoint.The Handler struct maintains the same clean dependency pattern as the decrypt handler, ensuring consistency across the internal vault endpoints.
29-53: Well-implemented encrypt handler with proper response structure.The Handle method follows the same excellent patterns as the decrypt handler:
- Consistent authentication using zen.StaticAuth
- Proper request binding and error handling
- Appropriate response including both encrypted data and key ID
- Good use of fault.Wrap for internal error context
The implementation maintains consistency with the decrypt endpoint while appropriately including the key ID in the response for encryption operations.
go/pkg/zen/auth_test.go (2)
114-196: Comprehensive test coverage for StaticAuth function.The test function provides excellent coverage of various authentication scenarios including valid token matches, mismatches, malformed headers, case sensitivity, and whitespace handling. The table-driven approach is consistent with existing tests and covers all critical edge cases.
198-218: Integration test validates real HTTP request/response flow.The integration test properly validates StaticAuth with a fully initialized session and checks both successful and failed authentication scenarios with correct error codes. This complements the unit tests well.
go/apps/api/openapi/openapi-split.yaml (2)
202-206: Path references updated correctly for internal chproxy endpoints.The path references have been properly updated to use the
_internalprefix structure, maintaining consistency with the new internal endpoint organization.
208-212: New vault endpoints added with proper structure.The new internal vault encryption and decryption endpoints are properly defined with correct path references following the established pattern for internal endpoints.
go/apps/api/openapi/gen.go (1)
166-174: Sensitive data models require strict internal scoping
InternalVaultDecryptRequestBodyandInternalVaultDecryptResponseBodycarry raw encrypted data and decrypted plaintext respectively. Double-check that:
- These models are only wired to
/_internal/*routes (no accidental public exposure).- The OpenAPI spec marks the endpoints as
internal/x-internal: true(or similar) so client SDKs are not generated for external consumers.- Logging / tracing middle-ware skips bodies based on route prefix or content-type to avoid leaking secrets.
No code change needed in this generated file, just verify upstream constraints.
Also applies to: 176-180
go/apps/api/routes/_internal_vault_decrypt/400_test.go (2)
1-11: LGTM! Clean package structure and imports.The package declaration follows Go test conventions and all imports are properly organized and used.
66-118: LGTM! Comprehensive invalid data testing.The test function covers all relevant invalid encrypted data scenarios with appropriate test cases and realistic data. The comments explaining the test approach (especially lines 107-108) add good clarity.
| // InternalVaultEncryptRequestBody defines model for InternalVaultEncryptRequestBody. | ||
| type InternalVaultEncryptRequestBody struct { | ||
| // Data The plaintext data to encrypt | ||
| Data string `json:"data"` | ||
|
|
||
| // Keyring The keyring identifier to use for encryption | ||
| Keyring string `json:"keyring"` | ||
| } | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Key-ID in response could expose crypto inventory
InternalVaultEncryptResponseBody returns keyId. For most internal flows this is fine, but if the response ever crosses trust boundaries it reveals the exact DEK/KMS key used.
Consider whether the caller truly needs this field or if it can stay server-side. If removal is desirable, drop it from the OpenAPI spec – this file is regenerated.
Also applies to: 190-197
🤖 Prompt for AI Agents
In go/apps/api/openapi/gen.go around lines 181 to 189 and 190 to 197, the
InternalVaultEncryptResponseBody struct includes a keyId field that may expose
sensitive cryptographic key information across trust boundaries. Review whether
the keyId is necessary for the caller; if not, remove the keyId field from the
response struct and update the OpenAPI specification accordingly to prevent it
from being regenerated with this field included.
| InternalVaultDecryptRequestBody: | ||
| type: object | ||
| required: | ||
| - keyring | ||
| - encrypted | ||
| properties: | ||
| keyring: | ||
| type: string | ||
| description: The keyring identifier used for encryption | ||
| example: "user_data" | ||
| encrypted: | ||
| type: string | ||
| description: Base64 encoded encrypted data to decrypt | ||
| example: "eyJhbGdvcml0aG0iOiJBRVMtMjU2LUdDTSIsIm5vbmNlIjoi..." |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Mark base-64 blob fields with an explicit format: byte.
OpenAPI uses format: byte to denote base-64 encoded strings.
Without it, generators may treat the value as arbitrary UTF-8 text and silently corrupt binary payloads.
- encrypted:
- type: string
+ encrypted:
+ type: string
+ format: byte(Do the same for any other encrypted/ciphertext properties.)
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| InternalVaultDecryptRequestBody: | |
| type: object | |
| required: | |
| - keyring | |
| - encrypted | |
| properties: | |
| keyring: | |
| type: string | |
| description: The keyring identifier used for encryption | |
| example: "user_data" | |
| encrypted: | |
| type: string | |
| description: Base64 encoded encrypted data to decrypt | |
| example: "eyJhbGdvcml0aG0iOiJBRVMtMjU2LUdDTSIsIm5vbmNlIjoi..." | |
| InternalVaultDecryptRequestBody: | |
| type: object | |
| required: | |
| - keyring | |
| - encrypted | |
| properties: | |
| keyring: | |
| type: string | |
| description: The keyring identifier used for encryption | |
| example: "user_data" | |
| encrypted: | |
| type: string | |
| format: byte | |
| description: Base64 encoded encrypted data to decrypt | |
| example: "eyJhbGdvcml0aG0iOiJBRVMtMjU2LUdDTSIsIm5vbmNlIjoi..." |
🤖 Prompt for AI Agents
In go/apps/api/openapi/openapi-generated.yaml around lines 97 to 110, the
properties representing base64 encoded data, such as "encrypted", lack the
explicit "format: byte" annotation. Add "format: byte" to these properties to
correctly indicate they contain base64 encoded binary data, preventing
generators from misinterpreting them as UTF-8 text. Apply this change to all
similar fields like "encrypted" or "ciphertext" throughout the file.
| encrypted: | ||
| type: string | ||
| description: Base64 encoded encrypted data | ||
| example: "eyJhbGdvcml0aG0iOiJBRVMtMjU2LUdDTSIsIm5vbmNlIjoi..." | ||
| keyId: | ||
| type: string | ||
| description: The ID of the data encryption key used | ||
| example: "dek_12345" |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Repeat the format: byte hint for the encryption response.
Same reasoning as above – generators and clients benefit from the correct semantic hint.
- encrypted:
- type: string
+ encrypted:
+ type: string
+ format: byte📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| encrypted: | |
| type: string | |
| description: Base64 encoded encrypted data | |
| example: "eyJhbGdvcml0aG0iOiJBRVMtMjU2LUdDTSIsIm5vbmNlIjoi..." | |
| keyId: | |
| type: string | |
| description: The ID of the data encryption key used | |
| example: "dek_12345" | |
| encrypted: | |
| type: string | |
| format: byte | |
| description: Base64 encoded encrypted data | |
| example: "eyJhbGdvcml0aG9yaXRobSI6IkFFUy0yNTYtR0NNIiwibm9uY2UiOiIifQ==" | |
| keyId: | |
| type: string | |
| description: The ID of the data encryption key used | |
| example: "dek_12345" |
🤖 Prompt for AI Agents
In go/apps/api/openapi/openapi-generated.yaml around lines 140 to 147, the
encryption response fields lack the 'format: byte' hint which helps generators
and clients interpret the data correctly. Add 'format: byte' to the 'encrypted'
field to indicate it is base64-encoded binary data, ensuring proper semantic
understanding by tools consuming this schema.
| /_internal/vault/decrypt: | ||
| post: | ||
| description: |- | ||
| Internal endpoint for decrypting data using the vault service. This endpoint is used internally by the API and should not be used by external clients. | ||
| This endpoint bypasses normal authentication and validation as it's intended for internal use only. | ||
| operationId: internalVaultDecrypt | ||
| requestBody: | ||
| content: | ||
| application/json: | ||
| schema: | ||
| $ref: '#/components/schemas/InternalVaultDecryptRequestBody' | ||
| required: true | ||
| responses: | ||
| "200": | ||
| content: | ||
| application/json: | ||
| schema: | ||
| $ref: '#/components/schemas/InternalVaultDecryptResponseBody' | ||
| description: Data successfully decrypted | ||
| "400": | ||
| content: | ||
| application/json: | ||
| schema: | ||
| $ref: '#/components/schemas/BadRequestErrorResponse' | ||
| description: Invalid request body | ||
| "500": | ||
| content: | ||
| application/json: | ||
| schema: | ||
| $ref: '#/components/schemas/InternalServerErrorResponse' | ||
| description: Internal server error during decryption | ||
| security: [] |
There was a problem hiding this comment.
Spec is missing 401 response & security scheme despite token enforcement.
Handlers reject requests when the static vault token is absent or wrong, yet the spec declares security: [] and no 401 response.
This divergence breaks contract-tests and service discovery tooling.
security: []
+ responses:
+ "401":
+ description: Invalid or missing Vault auth token
+ content:
+ application/json:
+ schema:
+ $ref: '#/components/schemas/UnauthorizedErrorResponse'
+
+ security:
+ - VaultStaticToken: []Add a components.securitySchemes.VaultStaticToken mirroring the existing StaticToken pattern used elsewhere (header‐based, no scopes).
If the endpoint truly must stay undocumented, keep x-excluded: true but still describe real behaviour for internal consumers.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In go/apps/api/openapi/openapi-generated.yaml between lines 3203 and 3235, the
endpoint /_internal/vault/decrypt incorrectly declares no security and lacks a
401 response despite enforcing a static vault token. Add a VaultStaticToken
security scheme under components.securitySchemes similar to the existing
StaticToken scheme, then update the endpoint's security field to require this
VaultStaticToken. Also, add a 401 response describing unauthorized access due to
missing or invalid token. If the endpoint must remain undocumented, add
x-excluded: true but keep these security details to reflect actual behavior.
| security: [] | ||
| operationId: internalVaultEncrypt | ||
| summary: Internal vault encryption endpoint | ||
| description: |- | ||
| Internal endpoint for encrypting data using the vault service. This endpoint is used internally by the API and should not be used by external clients. | ||
| This endpoint bypasses normal authentication and validation as it's intended for internal use only. |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Clarify authentication description.
The description states this endpoint "bypasses normal authentication" but the handler actually enforces static token authentication. Consider clarifying that it bypasses OpenAPI-level authentication but still requires internal token authentication.
security: []
operationId: internalVaultEncrypt
summary: Internal vault encryption endpoint
description: |-
- Internal endpoint for encrypting data using the vault service. This endpoint is used internally by the API and should not be used by external clients.
+ Internal endpoint for encrypting data using the vault service. This endpoint is used internally by the API and should not be used by external clients.
- This endpoint bypasses normal authentication and validation as it's intended for internal use only.
+ This endpoint bypasses OpenAPI-level authentication but requires internal static token authentication for security.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| security: [] | |
| operationId: internalVaultEncrypt | |
| summary: Internal vault encryption endpoint | |
| description: |- | |
| Internal endpoint for encrypting data using the vault service. This endpoint is used internally by the API and should not be used by external clients. | |
| This endpoint bypasses normal authentication and validation as it's intended for internal use only. | |
| security: [] | |
| operationId: internalVaultEncrypt | |
| summary: Internal vault encryption endpoint | |
| description: |- | |
| Internal endpoint for encrypting data using the vault service. This endpoint is used internally by the API and should not be used by external clients. | |
| This endpoint bypasses OpenAPI-level authentication but requires internal static token authentication for security. |
🤖 Prompt for AI Agents
In go/apps/api/openapi/spec/paths/_internal/vault/encrypt/index.yaml around
lines 6 to 12, the description incorrectly states that the endpoint bypasses
normal authentication, but the handler enforces static token authentication.
Update the description to clarify that while OpenAPI-level authentication is
bypassed, the endpoint still requires internal static token authentication for
access.
| type: object | ||
| required: | ||
| - encrypted | ||
| - keyId | ||
| properties: | ||
| encrypted: | ||
| type: string | ||
| description: Base64 encoded encrypted data | ||
| example: "eyJhbGdvcml0aG0iOiJBRVMtMjU2LUdDTSIsIm5vbmNlIjoi..." | ||
| keyId: | ||
| type: string | ||
| description: The ID of the data encryption key used | ||
| example: "dek_12345" No newline at end of file |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Well-designed response schema with minor formatting fix needed.
The schema correctly defines the essential properties for an encryption response - the encrypted data and key ID. The descriptions and examples are clear and helpful.
keyId:
type: string
description: The ID of the data encryption key used
example: "dek_12345"
+📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| type: object | |
| required: | |
| - encrypted | |
| - keyId | |
| properties: | |
| encrypted: | |
| type: string | |
| description: Base64 encoded encrypted data | |
| example: "eyJhbGdvcml0aG0iOiJBRVMtMjU2LUdDTSIsIm5vbmNlIjoi..." | |
| keyId: | |
| type: string | |
| description: The ID of the data encryption key used | |
| example: "dek_12345" | |
| type: object | |
| required: | |
| - encrypted | |
| - keyId | |
| properties: | |
| encrypted: | |
| type: string | |
| description: Base64 encoded encrypted data | |
| example: "eyJhbGdvcml0aG0iOiJBRVMtMjU2LUdDTSIsIm5vbmNlIjoi..." | |
| keyId: | |
| type: string | |
| description: The ID of the data encryption key used | |
| example: "dek_12345" | |
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 13-13: no new line character at the end of file
(new-line-at-end-of-file)
🤖 Prompt for AI Agents
In
go/apps/api/openapi/spec/paths/_internal/vault/encrypt/InternalVaultEncryptResponseBody.yaml
lines 1 to 13, the response schema is well defined but requires a minor
formatting fix. Ensure proper indentation and spacing are consistent throughout
the YAML file to maintain readability and adhere to YAML standards. Verify that
all keys and values align correctly under their respective parent nodes.
| func TestVaultDecryptBadRequest(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| h := testutil.NewHarness(t) | ||
|
|
||
| route := &handler.Handler{ | ||
| Vault: h.Vault, | ||
| Logger: h.Logger, | ||
| Token: "test-token-123", | ||
| } | ||
|
|
||
| h.Register(route) | ||
|
|
||
| headers := http.Header{ | ||
| "Content-Type": {"application/json"}, | ||
| "Authorization": {"Bearer test-token-123"}, | ||
| } | ||
|
|
||
| t.Run("missing keyring", func(t *testing.T) { | ||
| req := openapi.InternalVaultDecryptRequestBody{ | ||
| // Keyring: "", // Missing keyring | ||
| Encrypted: "fake-encrypted-data", | ||
| } | ||
|
|
||
| res := testutil.CallRoute[openapi.InternalVaultDecryptRequestBody, openapi.InternalServerErrorResponse](h, route, headers, req) | ||
| require.Equal(t, 500, res.Status) | ||
| require.NotNil(t, res.Body) | ||
| }) | ||
|
|
||
| t.Run("missing encrypted data", func(t *testing.T) { | ||
| req := openapi.InternalVaultDecryptRequestBody{ | ||
| Keyring: h.Resources().UserWorkspace.ID, | ||
| // Encrypted: "", // Missing encrypted data | ||
| } | ||
|
|
||
| res := testutil.CallRoute[openapi.InternalVaultDecryptRequestBody, openapi.InternalServerErrorResponse](h, route, headers, req) | ||
| require.Equal(t, 500, res.Status) | ||
| require.NotNil(t, res.Body) | ||
| }) | ||
|
|
||
| t.Run("invalid json body", func(t *testing.T) { | ||
| // Test with invalid JSON structure | ||
| invalidJson := map[string]interface{}{ | ||
| "keyring": 12345, // Should be string, not number | ||
| "encrypted": []string{}, // Should be string, not array | ||
| } | ||
|
|
||
| res := testutil.CallRoute[map[string]interface{}, openapi.BadRequestErrorResponse](h, route, headers, invalidJson) | ||
| require.Equal(t, 400, res.Status) | ||
| require.NotNil(t, res.Body) | ||
| }) | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify status code expectations for validation errors.
The test expects different status codes for different validation scenarios:
- Missing fields (keyring/encrypted): expects 500 (lines 38, 49)
- Invalid JSON structure: expects 400 (line 61)
This inconsistency suggests either the handler implementation or test expectations may need alignment. Typically, missing required fields would return 400 (Bad Request) rather than 500 (Internal Server Error).
🏁 Script executed:
#!/bin/bash
# Search for the handler implementation to verify error handling behavior
ast-grep --pattern 'func (h *Handler) $_(ctx context.Context, request openapi.InternalVaultDecryptRequestBody) $_ {
$$$
}'Length of output: 126
🏁 Script executed:
#!/bin/bash
# Locate and inspect the handler implementation for the internal vault decrypt route
echo "Files in the _internal_vault_decrypt directory:"
find go/apps/api/routes/_internal_vault_decrypt -maxdepth 1 -type f
echo -e "\nFirst 200 lines of handler.go:"
sed -n '1,200p' go/apps/api/routes/_internal_vault_decrypt/handler.goLength of output: 1683
🏁 Script executed:
#!/bin/bash
# List files in the OpenAPI directory
echo "OpenAPI directory contents:"
find go/apps/api/openapi -maxdepth 1 -type f
echo -e "\nSearch for InternalVaultDecryptRequestBody in openapi-generated.yaml:"
grep -R "InternalVaultDecryptRequestBody" -n go/apps/api/openapi/openapi-generated.yaml
# Show the schema for request body around its definition
line=$(grep -R "InternalVaultDecryptRequestBody" -n go/apps/api/openapi/openapi-generated.yaml | cut -d: -f1 | head -n1)
if [ -n "$line" ]; then
start=$((line - 5))
[ "$start" -lt 1 ] && start=1
sed -n "${start},$((line + 50))p" go/apps/api/openapi/openapi-generated.yaml
else
echo "Schema definition not found."
fiLength of output: 3122
Align status codes for missing required fields
The OpenAPI schema marks both keyring and encrypted as required, so zen.BindBody will reject requests missing those fields with a validation error (HTTP 400), just as it does for type mismatches. Update the two “missing field” tests to expect 400 instead of 500:
• In go/apps/api/routes/_internal_vault_decrypt/400_test.go
- Missing keyring: change
InternalServerErrorResponse→BadRequestErrorResponseand expect400 - Missing encrypted data: change
InternalServerErrorResponse→BadRequestErrorResponseand expect400
🤖 Prompt for AI Agents
In go/apps/api/routes/_internal_vault_decrypt/400_test.go between lines 13 and
64, the tests for missing required fields "keyring" and "encrypted" incorrectly
expect a 500 status and InternalServerErrorResponse. Since these fields are
required by the OpenAPI schema, the handler returns a 400
BadRequestErrorResponse for validation errors. Update the "missing keyring" and
"missing encrypted data" tests to expect status 400 and use
BadRequestErrorResponse instead of InternalServerErrorResponse.
| t.Run("encrypt with different keyrings", func(t *testing.T) { | ||
| // Use the workspace ID as keyring since that's what the vault expects | ||
| req := openapi.InternalVaultEncryptRequestBody{ | ||
| Keyring: h.Resources().UserWorkspace.ID, | ||
| Data: "test data for workspace keyring", | ||
| } | ||
|
|
||
| res := testutil.CallRoute[openapi.InternalVaultEncryptRequestBody, openapi.InternalVaultEncryptResponseBody](h, route, headers, req) | ||
| require.Equal(t, 200, res.Status) | ||
| require.NotNil(t, res.Body) | ||
| require.NotEmpty(t, res.Body.Encrypted) | ||
| require.NotEmpty(t, res.Body.KeyId) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix misleading test name and implement proper keyring variation.
The test is named "encrypt with different keyrings" but only uses one keyring (h.Resources().UserWorkspace.ID). Either rename the test to reflect what it actually tests or implement proper keyring variation.
- t.Run("encrypt with different keyrings", func(t *testing.T) {
- // Use the workspace ID as keyring since that's what the vault expects
- req := openapi.InternalVaultEncryptRequestBody{
- Keyring: h.Resources().UserWorkspace.ID,
- Data: "test data for workspace keyring",
- }
+ t.Run("encrypt with workspace keyring", func(t *testing.T) {
+ req := openapi.InternalVaultEncryptRequestBody{
+ Keyring: h.Resources().UserWorkspace.ID,
+ Data: "test data for workspace keyring",
+ }🤖 Prompt for AI Agents
In go/apps/api/routes/_internal_vault_encrypt/200_test.go around lines 75 to 87,
the test named "encrypt with different keyrings" only uses a single keyring
value. To fix this, either rename the test to accurately describe that it tests
encryption with a single keyring or modify the test to loop over multiple
distinct keyring values and verify encryption for each, ensuring the test
matches its name and intent.
| required: | ||
| - keyring | ||
| - encrypted | ||
| properties: |
There was a problem hiding this comment.
Can we just make this an array to support decrypting multiple data pieces? This would help with the listKeys endpoin to just do a single req
|
Let's delay this until after I am back in 2 weeks. There's no rush here and it runs stable |
2 participants
Vibecoded this during a call, I still need to review it
What does this PR do?
Add internal vault encryption and decryption endpoints to the API service. These endpoints allow secure data encryption and decryption using the vault service, with proper authentication via static tokens.
Type of change
How should this be tested?
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainSummary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests