Skip to content

Secret management for Platform API#2228

Open
npamudika wants to merge 22 commits into
wso2:mainfrom
npamudika:platform-api-secret-management
Open

Secret management for Platform API#2228
npamudika wants to merge 22 commits into
wso2:mainfrom
npamudika:platform-api-secret-management

Conversation

@npamudika

Copy link
Copy Markdown

Purpose

Introduces an end-to-end Secrets Management feature for the WSO2 API Platform. Currently, sensitive credentials (API keys, tokens, certificates) are stored in plain text inside artifact configuration blobs. This creates security risks: credentials are exposed in the database, audit logs, and API responses. There is also no lifecycle management (rotation, deprecation, delete protection) and no way for gateway controllers to efficiently sync only the secrets they need.

Goals

  • Provide a secure encrypted secret store with AES-GCM-256 encryption via a pluggable vault (InHouseVault)
  • Expose CRUD REST endpoints (POST/GET/PUT/DELETE /api/v1/secrets) for org-scoped secret management
  • Allow artifact configurations (REST APIs, LLM providers, LLM proxies, MCP servers) to reference secrets via {{ secret "handle" }} placeholders instead of plain-text credentials
  • Block deletion of secrets that are actively referenced by any artifact (deployed or undeployed)
  • Provide an internal gateway sync endpoint (GET /api/internal/v1/secrets) that returns only the secrets referenced by a specific gateway's deployed artifacts, with optional decrypted values for bulk startup sync

Approach

Storage: A new secrets table stores ciphertext (AES-GCM-256 BLOB), a sha256: hash for change detection, value_scope (always ORG_SHARED), status (ACTIVE/DEPRECATED), and provider (IN_BUILT). Secrets are never physically deleted — DELETE sets status = DEPRECATED.

Reference tracking: A new artifact_secret_refs table with composite PK (organization_id, artifact_uuid, secret_handle, gateway_id) tracks every {{ secret "..." }} placeholder across all artifact types. Two row types coexist:

  • gateway_id = '' — artifact-level rows written on artifact create/update (used for delete protection — blocks deletion even of undeployed drafts)
  • gateway_id = <uuid> — deployment-snapshot rows written atomically inside the SetCurrentWithDetails transaction on deploy; cleared on undeploy/archive

Delete protection: FindRefs queries artifact_secret_refs by (org_id, secret_handle) — a single indexed lookup replacing the previous LIKE-based scans across multiple tables.

Gateway sync: GetSecretHandlesByGateway queries artifact_secret_refs WHERE org = ? AND gateway_id = ? (indexed). ListByHandles filters to value_scope = 'ORG_SHARED' only. The ?includeValues=true parameter enables startup bulk fetch (decrypts all secrets server-side), avoiding N round trips for /value calls.

Validation: ValidateSecretRefs is called at artifact create/update time; rejects configs that reference non-existent or DEPRECATED secret handles.

See https://docs.google.com/document/d/1I-ls5QUvDpGLmYPe39ai_i3AEL_awv8vQbi83jOnoRg/edit?tab=t.w3lefdjelsk5#heading=h.qxwb86l2rlwz for the full feature spec including database DDL, API shapes, flow diagrams, and gateway sync sequence.

User stories

  • As a platform operator, I can store an API key as a secret once and reference it by handle in multiple artifact configs, so credentials are never stored in plain text.
  • As a platform operator, I can rotate a secret's value via PUT /api/v1/secrets/{handle} and all gateways will automatically pick up the new value on the next sync cycle (detected via hash change).
  • As a platform operator, I am prevented from deleting a secret that is still referenced by any artifact config (deployed or draft), with a clear 409 response listing the referencing artifacts.
  • As a gateway controller, I can call GET /api/internal/v1/secrets with my API key and receive only the secrets referenced by my deployed artifacts, with decrypted values available for bulk startup sync.

Documentation

https://docs.google.com/document/d/1I-ls5QUvDpGLmYPe39ai_i3AEL_awv8vQbi83jOnoRg/edit?tab=t.w3lefdjelsk5#heading=h.qxwb86l2rlwz — full feature specification including API contracts, database schema, encryption design, gateway sync protocol, and flow diagrams.

Automation tests

Unit tests

  • src/internal/repository/secret_test.go — 26 tests covering SecretRepo CRUD, List pagination, updatedAfter filter, ListByHandles with scope filter, FindRefs (artifact-level and gateway-level rows), extractSecretHandles (regex, deduplication), upsertArtifactSecretRefs (insert on create, replace on update, clear when no secrets), upsertDeploymentSecretRefs (deploy, undeploy, isolation from artifact-level rows)
  • src/internal/service/secret_service_test.go — 21 tests covering Create (value scope always ORG_SHARED, type defaulting, encryption failure propagation), List (pagination shape), Get/Update, Delete (blocked by artifact-level ref, blocked by deployment ref, success, soft-delete contract), ValidateSecretRefs (missing handle, deduplication), Decrypt (DEPRECATED rejection)
  • src/internal/service/llm_secret_validation_test.go — 4 tests covering LLM provider create/update with valid and invalid {{ secret "..." }} placeholders

Integration tests

  • src/internal/handler/secret_integration_test.go — 19 handler-level tests using real SQLite + AES-GCM vault + Gin router. Covers the full public API (/api/v1/secrets): create (201, 409 duplicate, 401 no auth), list (pagination object shape, ?limit/?offset, ?updatedAfter filter, invalid timestamp → 400), get (200 metadata-only, 404 not found), update (200 new value returned), delete (204 unreferenced, 409 with references array, 404 not found), cross-org isolation (org B sees empty list, org B gets 404 for org A secret), soft-delete verification (DB row retained with status=DEPRECATED, Decrypt returns "secret is deprecated"), and ValidateSecretRefs rejection of DEPRECATED handle references.
  • src/internal/handler/gateway_secret_integration_test.go — 18 handler-level tests for the internal GW API (/api/internal/v1/secrets). Covers: authentication (no key → 401, invalid key → 401), list scoping (only gateway's deployed secrets returned, ?updatedAfter filter, invalid timestamp → 400, no value field by default), multi-gateway isolation (GW-A secret not visible to GW-B, shared handle visible to both), undeploy removes secrets from list, deduplication of same handle across multiple artifacts, GET /:id/value (200 plaintext, 200 after rotation, 404 not found), ?includeValues=true bulk fetch (ACTIVE secrets include value, DEPRECATED secrets omit value, empty list when no deployments, hash present alongside value).

Test scenario traceability: https://docs.google.com/spreadsheets/d/1FfXxjNbNh1SgjgxabPG8VcKLkITfqQTKwH61VVmfDkk/edit?gid=750717062#gid=750717062 — 85 test cases with Test Result and Automated Test columns. 64 PASS (automated), 21 MANUAL (gateway controller sync behaviour, process startup, and AI Workspace frontend orchestration flows).

Security checks

Notes:

  • Secrets are encrypted with AES-GCM-256 before persistence; the ciphertext column stores binary blobs only — plaintext is never written to the database
  • The encryption key (PLATFORM_SECRET_ENCRYPTION_KEY) is injected via environment variable; an ephemeral key is auto-generated (with a warning log) if unset — operators must set a persistent key for production
  • Secret values are returned in API responses only at creation time (POST) and rotation time (PUT); all GET responses omit the value field
  • The internal GW endpoint is authenticated via gateway API key (SHA-256 hashed token lookup), not JWT — it is not accessible via the public API path

Samples

N/A — this is a platform infrastructure feature with no standalone samples.

Related PRs

N/A

Test environment

  • OS: macOS 14 (Darwin 24.6.0)
  • Go: 1.23
  • Database: SQLite (in-memory, for tests); PostgreSQL-compatible schema also provided (schema.postgres.sql)
  • Tested via: go test ./src/internal/... — all packages pass

@CLAassistant

CLAassistant commented Jun 19, 2026

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Naduni Pamudika seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@coderabbitai

coderabbitai Bot commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

This pull request introduces end-to-end encrypted secrets management across the platform. It adds secrets and artifact_secret_refs database tables (PostgreSQL and SQLite) to store encrypted secret values and precomputed secret handle references by artifact and gateway. An AES-256-GCM InHouseVault provides local encryption, and a complete SecretService and SecretRepository handle persistence, lifecycle management, and reference validation. REST endpoints under /api/v1/secrets support CRUD operations with plaintext-once semantics for create and update. Artifact repositories (API, LLM provider/proxy, MCP proxy) maintain artifact_secret_refs rows on create/update to track secret references. The deployment SetCurrentWithDetails method maintains gateway-scoped reference rows on deploy/undeploy. The gateway controller gains a syncSecrets loop that performs bulk synchronization with decrypted values on startup and incremental synchronization with hash-based change detection on reconnect, fetching and upserting secrets before syncing deployments. The frontend adds secret API utilities and intercepts plaintext auth credentials in LLM provider and MCP proxy creation/update flows to create secrets and substitute {{ secret "..." }} placeholders. OpenAPI specification is updated with the new /secrets surface, permission scopes are extended with ap:secret:* entries, and gateway build manifest policy versions are bumped.

Suggested reviewers

  • RakhithaRR
  • Tharsanan1
  • VirajSalaka
  • renuka-fernando
  • malinthaprasan
  • AnuGayan
  • dushaniw
  • Arshardh
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 63.12% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Secret management for Platform API' directly describes the primary feature introduced in this comprehensive pull request.
Description check ✅ Passed The description is comprehensive and well-structured, covering all required template sections: purpose with issue context, goals, implementation approach with technical details, user stories, documentation links, unit and integration test coverage with specific test counts, security checks with affirmative responses, and test environment details.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with 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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 16

🧹 Nitpick comments (1)
platform-api/src/internal/repository/secret.go (1)

269-292: ⚡ Quick win

Consider deduplicating artifact references in FindRefs.

The query may return duplicate artifact entries if the same artifact references a secret via both artifact-level rows (gateway_id='') and gateway-specific rows. This could affect UI display or delete-protection logic.

Adding DISTINCT or grouping would prevent duplicates:

Suggested fix
 func (r *SecretRepo) FindRefs(orgID, handle string) ([]model.SecretReference, error) {
 	query := r.db.Rebind(`
-		SELECT art.handle, art.name, art.kind
+		SELECT DISTINCT art.handle, art.name, art.kind
 		FROM artifact_secret_refs asr
 		JOIN artifacts art ON art.uuid = asr.artifact_uuid
 		WHERE asr.organization_id = ? AND asr.secret_handle = ?
 	`)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@platform-api/src/internal/repository/secret.go` around lines 269 - 292, The
FindRefs method in SecretRepo may return duplicate artifact entries when the
same artifact references a secret through multiple rows. Add the DISTINCT
keyword to the SELECT statement in the query to eliminate duplicate artifact
handles, names, and kinds that could be returned from the join between
artifact_secret_refs and artifacts tables.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@gateway/gateway-controller/pkg/secrets/service.go`:
- Around line 349-367: The UpsertFromPlatform method has a race condition where
the SecretExists check followed by UpdateSecret or SaveSecret can fail under
concurrent sync runs. Replace the check-then-write pattern by attempting to
update the secret first, then handle the case where the secret is not found by
calling SaveSecret instead. Remove the SecretExists call and rely on the storage
layer's UpdateSecret response or error to determine whether to proceed with
SaveSecret, making the operation atomic and resilient to concurrent access.

In `@platform-api/src/internal/handler/gateway_internal.go`:
- Around line 858-865: Instead of logging the decryption error and continuing to
return a partial response, modify the error handling in the includeValues block
to fail the entire request when h.secretService.Decrypt() returns an error. When
a decryption failure occurs for any secret, return an error from the handler
function immediately so the caller can retry the bulk sync operation
deterministically, rather than receiving a 200 response with incomplete secret
values.

In `@platform-api/src/internal/handler/gateway_secret_integration_test.go`:
- Around line 568-579: The test in the loop currently returns early if the
deprecated secret "dep-iv" is found and correctly asserts it lacks a value
field, but if the loop completes without finding "dep-iv", the test passes
silently. This weakens the assertion because the test should require that
"dep-iv" is present in the response. Add a boolean flag before the loop
(initialized to false) to track whether "dep-iv" was found, set it to true when
the item is matched, and after the loop completes, add an assertion that fails
the test if the flag is still false, ensuring "dep-iv" must be present in the
response before checking its value field.
- Around line 71-98: Add error checks for all SQL execution calls in the test
setup that currently lack them. For each db.Exec() and sqlDB.Exec() call
(including the PRAGMA foreign_keys statement at the beginning, and the INSERT
operations for the organizations table, gateways table, and gateway_tokens
table), capture the error return value and assert it is nil using t.Fatalf with
a descriptive message. This applies to the setup operations in lines 71, 87, 91,
and 96 of the diff, as well as similar unchecked db.Exec calls elsewhere in the
test file, ensuring all SQL mutations are validated before proceeding with test
execution.

In `@platform-api/src/internal/handler/secret_integration_test.go`:
- Around line 650-651: Replace the strict string equality checks for error
assertions at the two locations (comparing decryptErr.Error() against "secret is
deprecated") with sentinel error checks instead. Define a sentinel error
variable for the "secret is deprecated" error at the package level, then update
both test assertions to use errors.Is() or check for error type/wrapped error
equality rather than comparing the exact error message text. This approach keeps
tests stable when error message text is modified in the future.
- Around line 390-411: The test setup code is ignoring errors from critical
operations like os.ReadFile, db.Exec for schema and data insertion, and
vault.NewInHouseVault. Replace all blank error identifiers (the underscores in
constructs like schema, _ := os.ReadFile and v, _ := vault.NewInHouseVault) with
proper error handling by capturing the error and checking it immediately after
each operation. Use t.Fatalf or similar to fail the test if any setup step
errors occur. Apply this consistent error-checking pattern to all setup blocks
mentioned in the comment (starting at lines 390, 588, 668, 711, 750, 786, and
822) to ensure test failures are due to actual test logic issues rather than
silent setup failures.

In `@platform-api/src/internal/handler/secret.go`:
- Around line 114-115: The secretService.List method call in the handler is
missing the projectId filter parameter that is exposed in the API contract.
Extract the projectId from the incoming request (similar to how orgID, limit,
offset, and updatedAfter are being extracted), then pass it as an additional
parameter to the h.secretService.List call to align the implementation with the
API contract expectations.
- Line 226: The line using c.JSON(http.StatusNoContent, nil) is incorrect
because HTTP 204 No Content responses must not include a response body. Replace
this call with c.Status(http.StatusNoContent) to return only the status code
without writing any body content to the response.
- Around line 93-101: In the query parameter validation blocks for limit and
offset (where c.Query("limit") and c.Query("offset") are being parsed), the
current code silently falls back to default values when parsing fails or values
are out of bounds. For the limit parameter, add an upper bound check to ensure
the converted value does not exceed the documented maximum of 100, placing this
check alongside the existing v > 0 condition. For the offset parameter, add a
reasonable upper bound check as well to prevent excessively large offset values.
Instead of silently ignoring invalid values, consider adding explicit validation
logic that either rejects out-of-bounds values or caps them to their maximum
allowed values before assigning them to the limit and offset variables.

In `@platform-api/src/internal/repository/secret_test.go`:
- Around line 488-520: The test
TestSecretRepo_FindRefs_DeduplicatesAcrossGateways inserts three
artifact_secret_refs rows (one with no gateway and two with different gateways)
but only verifies that len(refs) is greater than zero, which does not actually
test deduplication. Either replace the len(refs) == 0 check with an explicit
assertion that len(refs) == 1 to verify the rows are properly deduplicated to a
single result, or rename the test function to reflect that it only verifies refs
are returned without testing deduplication behavior.
- Around line 626-634: Replace all instances where database transaction and
query operations ignore returned errors by capturing the error values and
asserting they are nil in test setup and verification paths. Specifically, for
db.Begin() calls (around lines 626 and 632), upsertArtifactSecretRefs function
calls, tx.Commit() calls, and query/scan operations (around lines 637 and 642),
change the blank identifier underscore to actual error variables and add
assertions using require.NoError or similar test assertion methods. Apply this
error checking pattern consistently across all occurrences mentioned in lines
626, 632, 637, 642, 664, 671, 699, 705, 730, 733, 772, and 775 to ensure test
reliability.

In `@platform-api/src/internal/service/llm.go`:
- Around line 328-334: The code is discarding the error returned by
marshalUpstreamForValidation when validating secret placeholders in the upstream
config. Capture the error from the marshalUpstreamForValidation call instead of
using the blank identifier, then either return the error early or log it before
proceeding to ValidateSecretRefs. This prevents validation from silently passing
when marshaling fails. Apply this fix to both the Create method (around line
328-334) and the Update method (around line 498-504) where
marshalUpstreamForValidation is called.

In `@platform-api/src/internal/service/secret_service.go`:
- Around line 191-196: The error returned by the repo.Exists() call on line 192
is being discarded with an underscore, which means database errors will be
silently ignored and treated as if the secret is missing. Capture the error as
the second return value from repo.Exists() instead of discarding it, then check
if the error is not nil and return the error to the caller so database failures
are properly surfaced rather than being masked as missing secrets.

In `@platform-api/src/resources/openapi.yaml`:
- Around line 7287-7299: The OpenAPI specification for the GET /secrets endpoint
is missing documentation for the 400 Bad Request response that occurs when the
updatedAfter parameter has an invalid format. Add a '400' response entry to the
responses section following the same pattern as the existing error responses
(using $ref to reference '`#/components/responses/BadRequest`'), positioning it
logically before the 401 response to document that invalid query parameter
validation is performed by the handler.

In `@portals/ai-workspace/src/apis/secretApis.ts`:
- Around line 79-84: The generateSecretHandle function can return an empty
string if the providerId and fieldName parameters normalize away completely
through the chaining operations. Add a guard check after the final normalization
operation in generateSecretHandle that validates the result is not empty, and
throw an appropriate error if the normalized handle is empty to prevent invalid
empty handles from being passed to downstream API calls.

In `@portals/ai-workspace/src/contexts/llmProvider/LLMProvidersContext.tsx`:
- Around line 246-260: In the updatesPayload construction within the
LLMProvidersContext component, the url property inside upstream.main is
defaulting to an empty string when not provided in the update payload. This
causes unintended URL overrides when callers only update the auth value. Instead
of defaulting to an empty string, preserve the existing URL from the current
provider state by using the current provider's upstream.main.url as the fallback
value when the update payload does not include a URL. This ensures that partial
updates (such as auth-only changes) do not inadvertently clear the URL field.

---

Nitpick comments:
In `@platform-api/src/internal/repository/secret.go`:
- Around line 269-292: The FindRefs method in SecretRepo may return duplicate
artifact entries when the same artifact references a secret through multiple
rows. Add the DISTINCT keyword to the SELECT statement in the query to eliminate
duplicate artifact handles, names, and kinds that could be returned from the
join between artifact_secret_refs and artifacts tables.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 78e1eb6c-f40b-4e38-8a5d-88289fb79269

📥 Commits

Reviewing files that changed from the base of the PR and between 623d8b4 and 75fac9d.

📒 Files selected for processing (44)
  • gateway/build-manifest.yaml
  • gateway/gateway-controller/pkg/controlplane/client.go
  • gateway/gateway-controller/pkg/controlplane/sync_secrets.go
  • gateway/gateway-controller/pkg/secrets/service.go
  • gateway/gateway-controller/pkg/utils/api_utils.go
  • platform-api/src/cmd/cmd
  • platform-api/src/config/config.go
  • platform-api/src/config/default_config.go
  • platform-api/src/internal/constants/error.go
  • platform-api/src/internal/database/schema.postgres.sql
  • platform-api/src/internal/database/schema.sqlite.sql
  • platform-api/src/internal/dto/secret.go
  • platform-api/src/internal/handler/gateway_internal.go
  • platform-api/src/internal/handler/gateway_secret_integration_test.go
  • platform-api/src/internal/handler/llm.go
  • platform-api/src/internal/handler/secret.go
  • platform-api/src/internal/handler/secret_integration_test.go
  • platform-api/src/internal/model/secret.go
  • platform-api/src/internal/repository/api.go
  • platform-api/src/internal/repository/artifact_refs.go
  • platform-api/src/internal/repository/deployment.go
  • platform-api/src/internal/repository/interfaces.go
  • platform-api/src/internal/repository/llm.go
  • platform-api/src/internal/repository/mcp.go
  • platform-api/src/internal/repository/secret.go
  • platform-api/src/internal/repository/secret_test.go
  • platform-api/src/internal/server/server.go
  • platform-api/src/internal/service/gateway_internal.go
  • platform-api/src/internal/service/llm.go
  • platform-api/src/internal/service/llm_secret_validation_test.go
  • platform-api/src/internal/service/secret_service.go
  • platform-api/src/internal/service/secret_service_test.go
  • platform-api/src/internal/vault/vault.go
  • platform-api/src/main
  • platform-api/src/resources/openapi.yaml
  • portals/ai-workspace/configs/config-platform-api-template.toml
  • portals/ai-workspace/configs/config-platform-api.toml
  • portals/ai-workspace/configs/config.toml
  • portals/ai-workspace/docker-compose.yaml
  • portals/ai-workspace/src/apis/secretApis.ts
  • portals/ai-workspace/src/auth/permissions.ts
  • portals/ai-workspace/src/config.env.ts
  • portals/ai-workspace/src/contexts/llmProvider/LLMProvidersContext.tsx
  • portals/ai-workspace/src/pages/appShell/appShellPages/externalServers/ExternalServersNew.tsx

Comment thread gateway/gateway-controller/pkg/secrets/service.go Outdated
Comment thread platform-api/src/internal/handler/gateway_internal.go Outdated
Comment thread platform-api/src/internal/handler/gateway_secret_integration_test.go Outdated
Comment thread platform-api/src/internal/handler/gateway_secret_integration_test.go Outdated
Comment thread platform-api/src/internal/handler/secret_integration_test.go
Comment thread platform-api/src/internal/service/llm.go
Comment thread platform-api/src/internal/service/secret_service.go
Comment thread platform-api/src/resources/openapi.yaml
Comment thread portals/ai-workspace/src/apis/secretApis.ts

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@gateway/gateway-controller/pkg/secrets/service.go`:
- Around line 355-363: The fallback logic after the failed UpdateSecret call
does not handle the race condition where another concurrent worker inserts the
same secret between the NotFoundError check and the SaveSecret call. Capture the
error returned from s.storage.SaveSecret and check if it is an "already exists"
error using an appropriate storage error checking function (similar to
storage.IsNotFoundError). If the error indicates the secret already exists,
return nil since another worker has successfully inserted it and the upsert is
complete. Only return the error if it is something other than an "already
exists" condition. This ensures the operation remains idempotent under
concurrent sync scenarios.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 494da115-346f-4e3a-9a00-8b215d5d3637

📥 Commits

Reviewing files that changed from the base of the PR and between 75fac9d and 7f3d32f.

📒 Files selected for processing (11)
  • gateway/gateway-controller/pkg/secrets/service.go
  • platform-api/src/internal/handler/gateway_internal.go
  • platform-api/src/internal/handler/gateway_secret_integration_test.go
  • platform-api/src/internal/handler/secret.go
  • platform-api/src/internal/repository/secret.go
  • platform-api/src/internal/repository/secret_test.go
  • platform-api/src/internal/service/llm.go
  • platform-api/src/internal/service/secret_service.go
  • platform-api/src/resources/openapi.yaml
  • portals/ai-workspace/src/apis/secretApis.ts
  • portals/ai-workspace/src/contexts/llmProvider/LLMProvidersContext.tsx
✅ Files skipped from review due to trivial changes (1)
  • portals/ai-workspace/src/apis/secretApis.ts
🚧 Files skipped from review as they are similar to previous changes (8)
  • platform-api/src/internal/handler/gateway_internal.go
  • portals/ai-workspace/src/contexts/llmProvider/LLMProvidersContext.tsx
  • platform-api/src/internal/service/llm.go
  • platform-api/src/internal/handler/secret.go
  • platform-api/src/internal/repository/secret_test.go
  • platform-api/src/internal/repository/secret.go
  • platform-api/src/internal/service/secret_service.go
  • platform-api/src/resources/openapi.yaml

Comment thread gateway/gateway-controller/pkg/secrets/service.go
@npamudika npamudika force-pushed the platform-api-secret-management branch 2 times, most recently from ebcb919 to b34f435 Compare June 22, 2026 06:24

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@platform-api/src/internal/repository/secret.go`:
- Around line 253-313: The FindRefsAndSoftDelete function has a race condition
where a concurrent transaction can insert a reference between the check for refs
and the deprecation update. Add a SELECT FOR UPDATE query on the secrets table
before querying for references to lock the secret row and prevent other
transactions from modifying it during the check. Execute this lock query against
the transaction (tx) after beginning it and before running the refsQuery,
selecting the secret by organization_id and handle, and ensure the query
succeeds and the secret exists (similar to the affected rows check on the
deprecation).
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: e2136fdc-ce26-4e3d-9b93-0fdbee9a6164

📥 Commits

Reviewing files that changed from the base of the PR and between b34f435 and 4efda1c.

📒 Files selected for processing (6)
  • platform-api/src/internal/handler/secret_integration_test.go
  • platform-api/src/internal/repository/interfaces.go
  • platform-api/src/internal/repository/secret.go
  • platform-api/src/internal/repository/secret_test.go
  • platform-api/src/internal/service/secret_service.go
  • platform-api/src/internal/service/secret_service_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
  • platform-api/src/internal/handler/secret_integration_test.go
  • platform-api/src/internal/service/secret_service.go
  • platform-api/src/internal/service/secret_service_test.go

Comment thread platform-api/src/internal/repository/secret.go
@npamudika npamudika force-pushed the platform-api-secret-management branch 2 times, most recently from e7ad901 to 6300e14 Compare June 23, 2026 08:27
@npamudika npamudika force-pushed the platform-api-secret-management branch 4 times, most recently from 45047f7 to f9b7aec Compare June 25, 2026 07:36
@npamudika npamudika force-pushed the platform-api-secret-management branch 2 times, most recently from d42e601 to fe2566d Compare June 25, 2026 10:35
@npamudika npamudika force-pushed the platform-api-secret-management branch from f94232f to 13d3b4f Compare June 25, 2026 14:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants