feat: add PATCH endpoint for secret metadata-only updates - #1303
Merged
ptone merged 5 commits intoAug 27, 2026
Conversation
added 4 commits
August 25, 2026 21:14
Users currently must re-enter the secret value to change metadata fields like description, injection mode, or progeny access. This adds a PATCH endpoint that updates only metadata while leaving the stored value untouched. Changes across 7 layers: - Backend interface: add UpdateMetaInput struct and UpdateMeta method - Store layer: add SecretMetaUpdate struct and UpdateSecretMeta method - Local backend: implement UpdateMeta via store.UpdateSecretMeta - GCP backend: implement UpdateMeta (DB-only, no new SM version) - API handler: add PATCH to handleSecretByKey, project, and broker - CLI: add `scion hub secret update` subcommand - Web UI: add edit-settings dialog mode with pencil button - Hub client: add UpdateMeta method to SecretService Includes tests for backend (4 tests) and handler (4 tests) layers. Closes #1258
…TCH tests - Fix gofmt alignment in PatchSecretRequest and UpdateSecretMetaRequest structs (CI format check blocker) - Add allowProgeny scope validation to project and broker PATCH handlers, matching the existing user-scope PATCH behavior that rejects allowProgeny on non-user-scoped secrets - Add handler tests for project-scope and broker-scope PATCH: successful metadata update (200) and allowProgeny rejection (400)
…ode in PATCH R1: When a PATCH sends only `target` without `type`, the file-type path validation (traversal and absolute-path checks) was bypassed because req.Type was empty. Now all three PATCH handlers (user, project, broker) fetch the existing secret's type to validate the target when req.Type is not specified. R2: Add injectionMode validation to all three PATCH handlers, rejecting values other than "always" or "as_needed" before building the UpdateMetaInput. Adds tests for path traversal, relative target, invalid injectionMode, and valid injectionMode across user, project, and broker scopes.
When PATCH changes a secret's type to "file" without providing a new target, the existing stored target (e.g. defaulted key name "MY_KEY") was not validated against the file-type invariant requiring an absolute path. This fix fetches and validates the stored target in all three PATCH handlers (user, project, broker scope). Also fixes misleading "expected 422" error messages in test assertions that actually check for 400 (http.StatusBadRequest), and adds a test for the type-change-to-file scenario.
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request implements metadata-only updates for secrets (PATCH) across the CLI, Hub API, and web UI, allowing users to update fields like description, injection mode, type, target, and progeny allowance without changing the secret value. The changes are well-tested across the store, backend, and API layers. The feedback recommends refactoring the duplicated PATCH validation and update logic in the HTTP handlers into a shared helper function to improve maintainability.
Comment on lines
+975
to
+1098
| func (s *Server) patchSecret(w http.ResponseWriter, r *http.Request, key string) { | ||
| ctx := r.Context() | ||
|
|
||
| r.Body = http.MaxBytesReader(w, r.Body, 128*1024) | ||
|
|
||
| var req PatchSecretRequest | ||
| if err := readJSON(r, &req); err != nil { | ||
| BadRequest(w, "Invalid request body: "+err.Error()) | ||
| return | ||
| } | ||
|
|
||
| // Validate type if provided | ||
| if req.Type != "" { | ||
| switch req.Type { | ||
| case store.SecretTypeEnvironment, store.SecretTypeVariable, store.SecretTypeFile: | ||
| // valid | ||
| default: | ||
| ValidationError(w, "type must be one of: environment, variable, file", map[string]interface{}{ | ||
| "field": "type", | ||
| "value": req.Type, | ||
| }) | ||
| return | ||
| } | ||
| } | ||
|
|
||
| // Validate injectionMode if provided | ||
| if req.InjectionMode != "" { | ||
| switch req.InjectionMode { | ||
| case store.InjectionModeAlways, store.InjectionModeAsNeeded: | ||
| // valid | ||
| default: | ||
| ValidationError(w, "injectionMode must be \"always\" or \"as_needed\"", map[string]interface{}{ | ||
| "field": "injectionMode", | ||
| "value": req.InjectionMode, | ||
| "allowed": []string{"always", "as_needed"}, | ||
| }) | ||
| return | ||
| } | ||
| } | ||
|
|
||
| scope := r.URL.Query().Get("scope") | ||
| if scope == "" { | ||
| scope = store.ScopeUser | ||
| } | ||
|
|
||
| scopeID, ok := s.resolveEnvSecretAccess(w, r, scope, r.URL.Query().Get("scopeId"), true) | ||
| if !ok { | ||
| return | ||
| } | ||
|
|
||
| // Determine the effective secret type for target validation | ||
| effectiveType := req.Type | ||
| if effectiveType == "" && req.Target != "" { | ||
| // Fetch stored type to validate target against it | ||
| existing, err := s.secretBackend.GetMeta(ctx, key, scope, scopeID) | ||
| if err != nil { | ||
| writeErrorFromErr(w, err, "") | ||
| return | ||
| } | ||
| effectiveType = existing.SecretType | ||
| } | ||
|
|
||
| // Validate file-specific target constraints (including stored target when type changes to file) | ||
| effectiveTarget := req.Target | ||
| if effectiveType == store.SecretTypeFile && effectiveTarget == "" { | ||
| existing, err := s.secretBackend.GetMeta(ctx, key, scope, scopeID) | ||
| if err != nil { | ||
| writeErrorFromErr(w, err, "") | ||
| return | ||
| } | ||
| effectiveTarget = existing.Target | ||
| } | ||
| if effectiveTarget != "" && effectiveType == store.SecretTypeFile { | ||
| if strings.Contains(effectiveTarget, "..") { | ||
| BadRequest(w, "target path must not contain '..'") | ||
| return | ||
| } | ||
| if !strings.HasPrefix(effectiveTarget, "/") && !strings.HasPrefix(effectiveTarget, "~/") { | ||
| ValidationError(w, "file secret target must be an absolute path (or start with ~/)", map[string]interface{}{ | ||
| "field": "target", | ||
| "value": effectiveTarget, | ||
| }) | ||
| return | ||
| } | ||
| } | ||
|
|
||
| // allowProgeny is only valid on user-scoped secrets | ||
| if req.AllowProgeny != nil && *req.AllowProgeny && scope != store.ScopeUser { | ||
| ValidationError(w, "allowProgeny is only supported on user-scoped secrets", map[string]interface{}{ | ||
| "field": "allowProgeny", | ||
| "scope": scope, | ||
| }) | ||
| return | ||
| } | ||
|
|
||
| input := &secret.UpdateMetaInput{ | ||
| Name: key, | ||
| Scope: scope, | ||
| ScopeID: scopeID, | ||
| Description: req.Description, | ||
| InjectionMode: req.InjectionMode, | ||
| SecretType: req.Type, | ||
| Target: req.Target, | ||
| AllowProgeny: req.AllowProgeny, | ||
| } | ||
|
|
||
| if userIdent := GetUserIdentityFromContext(ctx); userIdent != nil { | ||
| input.UpdatedBy = userIdent.ID() | ||
| } | ||
|
|
||
| meta, err := s.secretBackend.UpdateMeta(ctx, input) | ||
| if err != nil { | ||
| writeErrorFromErr(w, err, "") | ||
| return | ||
| } | ||
|
|
||
| // Manage implicit progeny policy lifecycle if AllowProgeny changed | ||
| if req.AllowProgeny != nil { | ||
| s.ensureProgenyPolicy(ctx, meta) | ||
| } | ||
|
|
||
| result := metaToStoreSecret(*meta) | ||
| writeJSON(w, http.StatusOK, result) | ||
| } |
Contributor
There was a problem hiding this comment.
There's a significant amount of duplicated logic for handling PATCH requests in patchSecret, handleProjectSecretByKey, and handleBrokerSecretByKey. The logic for validating the request, determining effective types and targets, and updating the secret is nearly identical across all three handlers.
To improve maintainability and reduce redundancy, consider refactoring this shared logic into a single helper function. This function could accept parameters like w, r, key, scope, and scopeID, and encapsulate the entire patch process.
Each of the parent handlers would then be responsible only for determining its specific scope and scopeID and calling this new helper function.
For example:
func (s *Server) patchSecretLogic(w http.ResponseWriter, r *http.Request, key, scope, scopeID string) {
// ... (shared PATCH logic here)
}
func (s *Server) patchSecret(w http.ResponseWriter, r *http.Request, key string) {
// ... (resolve scope and scopeID for user/hub)
s.patchSecretLogic(w, r, key, scope, scopeID)
}
// In handleProjectSecretByKey:
case http.MethodPatch:
s.patchSecretLogic(w, r, key, store.ScopeProject, projectID)
// In handleBrokerSecretByKey:
case http.MethodPatch:
s.patchSecretLogic(w, r, key, store.ScopeRuntimeBroker, brokerID)This change would make the code more DRY (Don't Repeat Yourself) and easier to modify in the future.
Extract patchSecretValidateAndUpdate to consolidate the duplicated PATCH validation and update logic from three handlers (patchSecret, handleProjectSecretByKey PATCH, handleBrokerSecretByKey PATCH) into a single shared helper. Pure refactor — no behavior changes. Addresses Gemini review finding on PR #1303.
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1258 - secret metadata-only updates