From 0689b3635a81a534c64a9d596ecca2021cbece7b Mon Sep 17 00:00:00 2001 From: hervee Date: Wed, 4 Nov 2026 15:37:27 +0100 Subject: [PATCH] WKS-3614 - Escape worker API path elements and validate worker keys --- commands/common/cmd_api.go | 11 ++++- commands/common/cmd_api_test.go | 49 ++++++++++++++++++++ commands/common/cmd_commons.go | 14 +++++- commands/common/cmd_commons_test.go | 59 +++++++++++++++++++++++- commands/common/manifest.go | 23 ++++++++++ commands/common/manifest_test.go | 69 +++++++++++++++++++++++++++++ commands/remove_cmd_test.go | 6 +++ 7 files changed, 228 insertions(+), 3 deletions(-) diff --git a/commands/common/cmd_api.go b/commands/common/cmd_api.go index ea84a5f..058f7bd 100644 --- a/commands/common/cmd_api.go +++ b/commands/common/cmd_api.go @@ -68,7 +68,16 @@ func CallWorkerAPI(c model.IntFlagProvider, params APICallParams) error { apiVersion = params.APIVersion } - apiEndpoint := fmt.Sprintf("%sworker/api/v%d/%s", utils.AddTrailingSlashIfNeeded(params.ServerURL), apiVersion, strings.Join(params.Path, "/")) + escapedPath := make([]string, len(params.Path)) + for i, p := range params.Path { + // PathEscape leaves dot-segments untouched, so they must be rejected. + if p == "." || p == ".." { + return apiError(http.StatusBadRequest, "invalid path element '%s' in worker API call", p) + } + escapedPath[i] = url.PathEscape(p) + } + + apiEndpoint := fmt.Sprintf("%sworker/api/v%d/%s", utils.AddTrailingSlashIfNeeded(params.ServerURL), apiVersion, strings.Join(escapedPath, "/")) q := url.Values{} diff --git a/commands/common/cmd_api_test.go b/commands/common/cmd_api_test.go index f0dcd31..99c2140 100644 --- a/commands/common/cmd_api_test.go +++ b/commands/common/cmd_api_test.go @@ -6,6 +6,7 @@ package common import ( "encoding/json" "net/http" + "net/http/httptest" "testing" "time" @@ -137,3 +138,51 @@ func TestCallWorkerAPI(t *testing.T) { }) } } + +func TestCallWorkerAPI_PathEscaping(t *testing.T) { + tests := []struct { + name string + path []string + wantURI string + wantErr string + }{ + {name: "ticket case", path: []string{"workers", "../x?y"}, wantURI: "/worker/api/v1/workers/..%2Fx%3Fy?projectKey=p"}, + { + name: "slashes", + path: []string{"workers", "x/../../../../artifactory/api/repositories/libs-release-local"}, + wantURI: "/worker/api/v1/workers/x%2F..%2F..%2F..%2F..%2Fartifactory%2Fapi%2Frepositories%2Flibs-release-local?projectKey=p", + }, + {name: "hash", path: []string{"execute", "a#b"}, wantURI: "/worker/api/v1/execute/a%23b?projectKey=p"}, + {name: "plain key", path: []string{"workers", "wk-1"}, wantURI: "/worker/api/v1/workers/wk-1?projectKey=p"}, + {name: "constant path", path: []string{"scaffold", "tsconfig"}, wantURI: "/worker/api/v1/scaffold/tsconfig?projectKey=p"}, + {name: "double dot", path: []string{"workers", ".."}, wantErr: "invalid path element '..' in worker API call"}, + {name: "single dot", path: []string{"workers", "."}, wantErr: "invalid path element '.' in worker API call"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var gotURI string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + gotURI = r.RequestURI + w.WriteHeader(http.StatusOK) + })) + defer srv.Close() + + err := CallWorkerAPI(IntFlagMap{}, APICallParams{ + Method: http.MethodGet, + ServerURL: srv.URL, + Path: tt.path, + ProjectKey: "p", + OkStatuses: []int{http.StatusOK}, + }) + + if tt.wantErr != "" { + assert.ErrorContains(t, err, tt.wantErr) + assert.Empty(t, gotURI, "no request should be sent") + return + } + require.NoError(t, err) + assert.Equal(t, tt.wantURI, gotURI) + }) + } +} diff --git a/commands/common/cmd_commons.go b/commands/common/cmd_commons.go index a782d81..d5f8605 100644 --- a/commands/common/cmd_commons.go +++ b/commands/common/cmd_commons.go @@ -32,6 +32,12 @@ func ExtractProjectAndKeyFromCommandContext(c stringFlagAware, args []string, mi if len(args) > 0 && len(args) > minArguments { workerKey = args[0] + // An empty argument falls back to the manifest name, which is validated there. + if workerKey != "" { + if err := ValidateWorkerKey(workerKey); err != nil { + return "", "", err + } + } } if workerKey == "" || projectKey == "" { @@ -43,7 +49,13 @@ func ExtractProjectAndKeyFromCommandContext(c stringFlagAware, args []string, mi return workerKey, projectKey, nil } - if err = ValidateManifest(manifest, nil); err != nil { + // The manifest name is only used (and must be valid) when it is the worker key. + toValidate := *manifest + if workerKey != "" { + toValidate.Name = workerKey + } + + if err = ValidateManifest(&toValidate, nil); err != nil { return "", "", err } diff --git a/commands/common/cmd_commons_test.go b/commands/common/cmd_commons_test.go index fe6097d..63d7314 100644 --- a/commands/common/cmd_commons_test.go +++ b/commands/common/cmd_commons_test.go @@ -97,6 +97,8 @@ func Test_extractProjectAndKeyFromCommandContext(t *testing.T) { minArguments int onlyGeneric bool eventType string + skipManifest bool + manifestName string assert func(t *testing.T, manifestWorkerKey, manifestProjectKey, extractedWorkerKey, extractedProjectKey string, err error) }{ { @@ -146,6 +148,55 @@ func Test_extractProjectAndKeyFromCommandContext(t *testing.T) { assert.Equal(t, manifestProjectKey, extractedProjectKey) }, }, + { + name: "empty worker key arg falls back to manifest", + c: mockStringFlagAware{}, + args: []string{""}, + minArguments: 0, + assert: func(t *testing.T, manifestWorkerKey, manifestProjectKey, extractedWorkerKey, extractedProjectKey string, err error) { + require.NoError(t, err) + assert.Equal(t, manifestWorkerKey, extractedWorkerKey) + assert.Equal(t, manifestProjectKey, extractedProjectKey) + }, + }, + { + name: "invalid worker key from args", + c: mockStringFlagAware{}, + args: []string{"../x"}, + assert: func(t *testing.T, _, _, extractedWorkerKey, _ string, err error) { + assert.EqualError(t, err, "invalid worker key '../x': must start with a letter") + assert.Empty(t, extractedWorkerKey) + }, + }, + { + name: "invalid worker key from args without manifest", + c: mockStringFlagAware{}, + args: []string{"../x"}, + skipManifest: true, + assert: func(t *testing.T, _, _, extractedWorkerKey, _ string, err error) { + assert.EqualError(t, err, "invalid worker key '../x': must start with a letter") + assert.Empty(t, extractedWorkerKey) + }, + }, + { + name: "invalid worker key from manifest", + c: mockStringFlagAware{}, + manifestName: "x?y", + assert: func(t *testing.T, _, _, _, _ string, err error) { + assert.EqualError(t, err, "invalid manifest: invalid worker key 'x?y'") + }, + }, + { + name: "valid key from args ignores invalid manifest name", + c: mockStringFlagAware{}, + args: []string{"good-key"}, + manifestName: "my.worker", + assert: func(t *testing.T, _, manifestProjectKey, extractedWorkerKey, extractedProjectKey string, err error) { + require.NoError(t, err) + assert.Equal(t, "good-key", extractedWorkerKey) + assert.Equal(t, manifestProjectKey, extractedProjectKey) + }, + }, } for _, tt := range tests { @@ -164,7 +215,13 @@ func Test_extractProjectAndKeyFromCommandContext(t *testing.T) { mf.Action = "GENERIC_EVENT" } - require.NoError(t, SaveManifest(&mf, dir)) + if tt.manifestName != "" { + mf.Name = tt.manifestName + } + + if !tt.skipManifest { + require.NoError(t, SaveManifest(&mf, dir)) + } workerKey, projectKey, err := ExtractProjectAndKeyFromCommandContext(tt.c, tt.args, tt.minArguments, tt.onlyGeneric) diff --git a/commands/common/manifest.go b/commands/common/manifest.go index b8c3af8..d7db4aa 100644 --- a/commands/common/manifest.go +++ b/commands/common/manifest.go @@ -8,6 +8,7 @@ import ( "fmt" "os" "path/filepath" + "regexp" "github.com/jfrog/jfrog-cli-platform-services/model" "github.com/jfrog/jfrog-client-go/utils/log" @@ -94,11 +95,33 @@ func ReadSourceCode(mf *model.Manifest) (string, error) { return string(sourceBytes), nil } +// workerKeyPattern mirrors the server rule (platform-extension, workerValidator.service.ts, assertWorkerKeyIsValid). +var workerKeyPattern = regexp.MustCompile(`^[A-Za-z][A-Za-z0-9_\\-]+$`) + +// ValidateWorkerKey checks the worker key against the same rules as the Workers service. +func ValidateWorkerKey(key string) error { + switch { + case len(key) < 2: + return fmt.Errorf("invalid worker key '%s': must be at least 2 chars long", key) + case len(key) > 40: + return fmt.Errorf("invalid worker key '%s': must be lower than 40 chars long", key) + case (key[0] < 'a' || key[0] > 'z') && (key[0] < 'A' || key[0] > 'Z'): + return fmt.Errorf("invalid worker key '%s': must start with a letter", key) + case !workerKeyPattern.MatchString(key): + return fmt.Errorf("invalid worker key '%s'", key) + } + return nil +} + func ValidateManifest(mf *model.Manifest, actionsMeta ActionsMetadata) error { if mf.Name == "" { return invalidManifestErr("missing name") } + if err := ValidateWorkerKey(mf.Name); err != nil { + return invalidManifestErr(err.Error()) + } + if mf.SourceCodePath == "" { return invalidManifestErr("missing source code path") } diff --git a/commands/common/manifest_test.go b/commands/common/manifest_test.go index c0d997d..22188e8 100644 --- a/commands/common/manifest_test.go +++ b/commands/common/manifest_test.go @@ -9,6 +9,7 @@ import ( "os" "path/filepath" "regexp" + "strings" "testing" "github.com/jfrog/jfrog-cli-platform-services/model" @@ -222,6 +223,26 @@ func TestManifest_Validate(t *testing.T) { assert.EqualError(t, err, invalidManifestErr("missing name").Error()) }, }, + { + name: "invalid name", + actionsMeta: sampleActions, + manifest: patchedManifestSample(func(mf *model.Manifest) { + mf.Name = "../x?y" + }), + assert: func(t *testing.T, err error) { + assert.EqualError(t, err, invalidManifestErr("invalid worker key '../x?y': must start with a letter").Error()) + }, + }, + { + name: "name too long", + actionsMeta: sampleActions, + manifest: patchedManifestSample(func(mf *model.Manifest) { + mf.Name = "a" + strings.Repeat("b", 40) + }), + assert: func(t *testing.T, err error) { + assert.ErrorContains(t, err, "must be lower than 40 chars long") + }, + }, { name: "missing source code path", actionsMeta: sampleActions, @@ -500,3 +521,51 @@ func patchedManifestSample(patch func(mf *model.Manifest)) *model.Manifest { patch(&patched) return &patched } + +func TestValidateWorkerKey(t *testing.T) { + const ( + minMsg = "must be at least 2 chars long" + maxMsg = "must be lower than 40 chars long" + startMsg = "must start with a letter" + ) + + tests := []struct { + key string + wantErr string // empty means valid, otherwise the full expected error + }{ + {key: "ab"}, + {key: "a" + strings.Repeat("b", 39)}, + {key: "my-worker"}, + {key: "wk_1"}, + {key: "A-b_C-9"}, + {key: `a\b`}, + {key: "worker-1234567890-init"}, + {key: "a", wantErr: "invalid worker key 'a': " + minMsg}, + {key: "", wantErr: "invalid worker key '': " + minMsg}, + {key: "a" + strings.Repeat("b", 40), wantErr: "invalid worker key '" + "a" + strings.Repeat("b", 40) + "': " + maxMsg}, + {key: "1worker", wantErr: "invalid worker key '1worker': " + startMsg}, + {key: "-worker", wantErr: "invalid worker key '-worker': " + startMsg}, + {key: "_w", wantErr: "invalid worker key '_w': " + startMsg}, + {key: "..", wantErr: "invalid worker key '..': " + startMsg}, + {key: "../x?y", wantErr: "invalid worker key '../x?y': " + startMsg}, + {key: "ab/cd", wantErr: "invalid worker key 'ab/cd'"}, + {key: "x?y", wantErr: "invalid worker key 'x?y'"}, + {key: "a#b", wantErr: "invalid worker key 'a#b'"}, + {key: "a.b", wantErr: "invalid worker key 'a.b'"}, + {key: "my worker", wantErr: "invalid worker key 'my worker'"}, + {key: "a%2F", wantErr: "invalid worker key 'a%2F'"}, + {key: "wé", wantErr: "invalid worker key 'wé'"}, + {key: "ab\n", wantErr: "invalid worker key 'ab\n'"}, + } + + for _, tt := range tests { + t.Run(tt.key, func(t *testing.T) { + err := ValidateWorkerKey(tt.key) + if tt.wantErr == "" { + assert.NoError(t, err) + } else { + assert.EqualError(t, err, tt.wantErr) + } + }) + } +} diff --git a/commands/remove_cmd_test.go b/commands/remove_cmd_test.go index 15473f4..85cd35d 100644 --- a/commands/remove_cmd_test.go +++ b/commands/remove_cmd_test.go @@ -45,6 +45,12 @@ func TestRemoveCommand(t *testing.T) { WithWorkers(&model.WorkerDetails{Key: "wk-1"}). WithDeleteEndpoint(), }, + { + name: "fails on invalid worker key", + commandArgs: []string{"../x"}, + serverBehavior: common.NewServerStub(t), + assert: common.AssertOutputError("invalid worker key '../x': must start with a letter"), + }, { name: "fails if timeout exceeds", commandArgs: []string{"--" + model.FlagTimeout, "500"},