diff --git a/README.md b/README.md index ef100e1..d27a78c 100644 --- a/README.md +++ b/README.md @@ -393,6 +393,9 @@ services: stream: true ``` +Path params like `{id}` are escaped as single path segments, including slashes and URL delimiters. +See [path parameter rules](docs/registry.md#path-parameters) for accepted values and validation errors. + | Setting under `upstream` | Default | Meaning | |-------------------------|---------|---------| | `request_timeout` | `30s` | Context deadline across dispatch, retries, and response reading | diff --git a/docs/registry.md b/docs/registry.md index 6782287..aa3033e 100644 --- a/docs/registry.md +++ b/docs/registry.md @@ -35,6 +35,33 @@ JSON uses `body` in the response envelope. Text uses `body_text`. Validated JSON bytes pass through without compaction or HTML escaping. Envelope field order stays unchanged. Text keeps standard JSON string escaping. +## Path parameters + +Placeholders like `/repos/{owner}/{repo}` use values from `params`. +Each value is escaped as a single path segment with Go's `url.PathEscape`. +Slashes, query delimiters, fragment delimiters, percent signs, and spaces are encoded. +For example, `owner/repo` becomes `owner%2Frepo`, and `%2e%2e` becomes `%252e%252e`. +Inserted values aren't scanned again for placeholders. + +Placeholders aren't allowed after `?`. Static query strings remain supported. +Registry loading rejects query placeholders and names the service and action without exposing template values. +The gateway also rejects these templates defensively. + +Path values must be strings, integer numbers, or booleans. +Missing values, empty strings, `.` and `..` are rejected. +Objects, arrays, null, and non-integer numbers are rejected. +Integer number text stays unchanged, including large integers and integral exponent notation. +Other params can still contain objects, arrays, or null when they aren't used in the path. + +Invalid path params return HTTP 400 with code `bad_request` and fixed message `invalid params`. +The receipt records status 400, policy `deny`, and error `bad_request`. +URL validation runs before vault access or token refresh. +Invalid base URLs return HTTP 500 with code `internal` and fixed message `failed to build upstream request`. +The receipt records status 500, policy `allow`, and error `internal`. +Base URL joining preserves its existing trailing-slash trimming. +The resulting URL must retain the base URL's scheme and host. +Connectors must declare path separators in templates, not inside param values. + ## SSE negotiation and receipts Set `stream: true` only for actions that may produce SSE. diff --git a/internal/gateway/gateway.go b/internal/gateway/gateway.go index 51ca17c..1ffd3df 100644 --- a/internal/gateway/gateway.go +++ b/internal/gateway/gateway.go @@ -28,7 +28,11 @@ import ( "fmt" "io" "log/slog" + "math" + "math/big" "net/http" + "net/url" + "strconv" "strings" "time" @@ -402,6 +406,24 @@ func (s *Server) executeAttempt(ctx context.Context, att *attempt) *outcome { } } + upstreamURL, err := buildURL(svc.BaseURL, action.Path, att.params) + if err != nil { + if !errors.Is(err, errInvalidPathParams) { + return &outcome{ + status: http.StatusInternalServerError, + body: ErrorResponse{Error: "failed to build upstream request", Code: "internal"}, + policyDecision: "allow", + errorCode: "internal", + } + } + return &outcome{ + status: http.StatusBadRequest, + body: ErrorResponse{Error: "invalid params", Code: "bad_request"}, + policyDecision: "deny", + errorCode: "bad_request", + } + } + tok, err := s.cfg.Vault.Get(att.req.OnBehalfOf, att.req.Service) if err != nil { return &outcome{ @@ -434,8 +456,6 @@ func (s *Server) executeAttempt(ctx context.Context, att *attempt) *outcome { } } - upstreamURL := buildURL(svc.BaseURL, action.Path, att.params) - var bodyReader io.Reader if action.Method == "POST" || action.Method == "PUT" || action.Method == "PATCH" { bodyReader = bytes.NewReader(att.req.Params) @@ -561,19 +581,81 @@ func injectAuth(r *http.Request, auth registry.AuthCfg, tok vault.Token) { } } -// buildURL constructs the full upstream URL with path parameters -// substituted from decoded params. json.Number preserves the -// parameter's original numeric text instead of reformatting it through a -// float64 round trip. -func buildURL(baseURL, pathTemplate string, params map[string]any) string { - path := pathTemplate - for k, v := range params { - placeholder := "{" + k + "}" - if strings.Contains(path, placeholder) { - path = strings.ReplaceAll(path, placeholder, fmt.Sprint(v)) +var errInvalidPathParams = errors.New("invalid path params") + +// buildURL scans the template once. Inserted values are escaped single path segments. +func buildURL(baseURL, pathTemplate string, params map[string]any) (string, error) { + base, err := url.Parse(baseURL) + if err != nil || base.Scheme == "" || base.Host == "" { + return "", errors.New("invalid base URL") + } + if _, query, ok := strings.Cut(pathTemplate, "?"); ok && strings.Contains(query, "{") { + return "", errors.New("placeholders are not allowed after '?'") + } + + var path strings.Builder + for len(pathTemplate) > 0 { + start := strings.IndexByte(pathTemplate, '{') + if start < 0 { + path.WriteString(pathTemplate) + break + } + path.WriteString(pathTemplate[:start]) + pathTemplate = pathTemplate[start+1:] + end := strings.IndexByte(pathTemplate, '}') + if end <= 0 { + return "", errInvalidPathParams } + value, ok := params[pathTemplate[:end]] + if !ok { + return "", errInvalidPathParams + } + escaped, err := escapePathParam(value) + if err != nil { + return "", err + } + path.WriteString(escaped) + pathTemplate = pathTemplate[end+1:] + } + fullURL := strings.TrimRight(baseURL, "/") + path.String() + parsed, err := url.Parse(fullURL) + if err != nil || parsed.Scheme != base.Scheme || parsed.Host != base.Host { + return "", errors.New("invalid upstream URL origin") + } + return fullURL, nil +} + +func escapePathParam(value any) (string, error) { + var text string + switch v := value.(type) { + case string: + text = v + case bool, int, int8, int16, int32, int64, uint, uint8, uint16, uint32, uint64: + text = fmt.Sprint(v) + case json.Number: + text = v.String() + if !json.Valid([]byte(text)) { + return "", errInvalidPathParams + } + // Check integrality without rounding large integers or near-integer fractions through float64. + number, ok := new(big.Rat).SetString(text) + if !ok || !number.IsInt() { + return "", errInvalidPathParams + } + case float64: + if math.IsNaN(v) || math.IsInf(v, 0) || math.Trunc(v) != v { + return "", errInvalidPathParams + } + text = strconv.FormatFloat(v, 'f', -1, 64) + case float32: + return escapePathParam(float64(v)) + default: + return "", errInvalidPathParams + } + if text == "" || text == "." || text == ".." { + return "", errInvalidPathParams } - return strings.TrimRight(baseURL, "/") + path + return url.PathEscape(text), nil } func writeJSON(w http.ResponseWriter, status int, v interface{}) { diff --git a/internal/gateway/gateway_test.go b/internal/gateway/gateway_test.go index 9d76d52..3c1e30b 100644 --- a/internal/gateway/gateway_test.go +++ b/internal/gateway/gateway_test.go @@ -75,7 +75,7 @@ func testSetup(t *testing.T) (*Server, *vault.MemoryStore, *fakeReceipts) { _ = reg.LoadBytes([]byte(` services: github: - base_url: PLACEHOLDER + base_url: https://api.github.com auth: type: oauth2 actions: @@ -90,7 +90,7 @@ services: repo: string title: string stripe: - base_url: PLACEHOLDER + base_url: https://api.stripe.com auth: type: bearer actions: @@ -593,10 +593,10 @@ func TestBuildURL(t *testing.T) { {"no params", "https://api.example.com", "/users", ``, "https://api.example.com/users"}, {"path params", "https://api.example.com", "/repos/{owner}/{repo}", `{"owner":"octocat","repo":"hello"}`, "https://api.example.com/repos/octocat/hello"}, {"trailing slash", "https://api.example.com/", "/users", ``, "https://api.example.com/users"}, - {"null", "https://api.example.com", "/users/{id}", `null`, "https://api.example.com/users/{id}"}, + {"static query", "https://api.example.com", "/repos/{repo}?sort=updated&limit=10", `{"repo":"hello"}`, "https://api.example.com/repos/hello?sort=updated&limit=10"}, + {"null without placeholders", "https://api.example.com", "/users", `null`, "https://api.example.com/users"}, {"numeric text", "https://api.example.com", "/users/{id}/{rate}", `{"id":9007199254740993,"rate":1.00e+2}`, "https://api.example.com/users/9007199254740993/1.00e+2"}, - {"unicode and bool", "https://api.example.com", "/{name}/{flag}/{name}", `{"name":"😀","flag":true}`, "https://api.example.com/😀/true/😀"}, - {"null value", "https://api.example.com", "/{id}", `{"id":null}`, "https://api.example.com/"}, + {"unicode and bool", "https://api.example.com", "/{name}/{flag}/{name}", `{"name":"😀","flag":true}`, "https://api.example.com/%F0%9F%98%80/true/%F0%9F%98%80"}, } for _, tc := range cases { @@ -607,7 +607,10 @@ func TestBuildURL(t *testing.T) { if err != nil { t.Fatal(err) } - got := buildURL(tc.base, tc.path, params) + got, err := buildURL(tc.base, tc.path, params) + if err != nil { + t.Fatal(err) + } if got != tc.want { t.Fatalf("got %s, want %s", got, tc.want) } diff --git a/internal/gateway/path_test.go b/internal/gateway/path_test.go new file mode 100644 index 0000000..0c88f6a --- /dev/null +++ b/internal/gateway/path_test.go @@ -0,0 +1,410 @@ +package gateway + +import ( + "encoding/json" + "fmt" + "io/fs" + "math" + "net/http" + "net/http/httptest" + "path/filepath" + "strings" + "sync/atomic" + "testing" + "time" + + "github.com/Clawdlinux/agentgate/internal/receipt" + "github.com/Clawdlinux/agentgate/internal/registry" + "github.com/Clawdlinux/agentgate/internal/vault" +) + +func TestBuildURLPathSegments(t *testing.T) { + t.Parallel() + cases := []struct { + name string + value any + want string + }{ + {"traversal", "../../admin/users", "..%2F..%2Fadmin%2Fusers"}, + {"slash", "owner/repo", "owner%2Frepo"}, + {"query", "repo?admin=true", "repo%3Fadmin=true"}, + {"fragment", "repo#admin", "repo%23admin"}, + {"encoded dots", "%2e%2e", "%252e%252e"}, + {"encoded traversal", "%2E%2E%2Fadmin", "%252E%252E%252Fadmin"}, + {"space", "hello world", "hello%20world"}, + {"unicode", "café/東京", "caf%C3%A9%2F%E6%9D%B1%E4%BA%AC"}, + {"large integer", json.Number("18446744073709551616"), "18446744073709551616"}, + {"negative integer", json.Number("-9007199254740993"), "-9007199254740993"}, + {"integer exponent", json.Number("1.00e+2"), "1.00e+2"}, + {"integer decimal", json.Number("42.000"), "42.000"}, + {"integer negative exponent", json.Number("1200e-2"), "1200e-2"}, + {"integral float", float64(42), "42"}, + {"integral float32", float32(42), "42"}, + {"integer", int(42), "42"}, + {"signed integer", int64(-42), "-42"}, + {"unsigned integer", uint64(18446744073709551615), "18446744073709551615"}, + {"boolean", false, "false"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got, err := buildURL("https://api.example.com/v1///", "/repos/{repo}", map[string]any{"repo": tc.value}) + if err != nil { + t.Fatal(err) + } + if want := "https://api.example.com/v1/repos/" + tc.want; got != want { + t.Fatalf("got %q, want %q", got, want) + } + }) + } +} + +func TestBuildURLRejectsQueryPlaceholders(t *testing.T) { + t.Parallel() + for _, path := range []string{"/search?q={q}", "/repos/{repo}?q={q}"} { + t.Run(path, func(t *testing.T) { + got, err := buildURL("https://api.example.com", path, map[string]any{ + "repo": "hello", + "q": "hello&admin=true+extra", + }) + if err == nil || got != "" { + t.Fatalf("query placeholder accepted: %q, error=%v", got, err) + } + }) + } +} + +func TestBuildURLDoesNotResubstitute(t *testing.T) { + t.Parallel() + for i := 0; i < 100; i++ { + params := map[string]any{"repo": "{owner}", "owner": "octocat"} + got, err := buildURL("https://api.example.com", "/repos/{owner}/{repo}/{repo}", params) + if err != nil { + t.Fatal(err) + } + const want = "https://api.example.com/repos/octocat/%7Bowner%7D/%7Bowner%7D" + if got != want { + t.Fatalf("iteration %d: got %q, want %q", i, got, want) + } + } +} + +func TestBuildURLRejectsInvalid(t *testing.T) { + t.Parallel() + cases := []struct { + name string + base string + path string + params map[string]any + }{ + {"missing", "https://api.example.com", "/repos/{repo}", nil}, + {"empty", "https://api.example.com", "/repos/{repo}", map[string]any{"repo": ""}}, + {"dot", "https://api.example.com", "/repos/{repo}", map[string]any{"repo": "."}}, + {"dot dot", "https://api.example.com", "/repos/{repo}", map[string]any{"repo": ".."}}, + {"null", "https://api.example.com", "/repos/{repo}", map[string]any{"repo": nil}}, + {"object", "https://api.example.com", "/repos/{repo}", map[string]any{"repo": map[string]any{"id": "repo"}}}, + {"array", "https://api.example.com", "/repos/{repo}", map[string]any{"repo": []any{"repo"}}}, + {"fraction", "https://api.example.com", "/repos/{repo}", map[string]any{"repo": json.Number("1.5")}}, + {"rounded fraction", "https://api.example.com", "/repos/{repo}", map[string]any{"repo": json.Number("9007199254740993.1")}}, + {"tiny fraction", "https://api.example.com", "/repos/{repo}", map[string]any{"repo": json.Number("1e-1000")}}, + {"float fraction", "https://api.example.com", "/repos/{repo}", map[string]any{"repo": 1.5}}, + {"nan", "https://api.example.com", "/repos/{repo}", map[string]any{"repo": math.NaN()}}, + {"infinity", "https://api.example.com", "/repos/{repo}", map[string]any{"repo": math.Inf(1)}}, + {"invalid number", "https://api.example.com", "/repos/{repo}", map[string]any{"repo": json.Number("1/2")}}, + {"non JSON number", "https://api.example.com", "/repos/{repo}", map[string]any{"repo": json.Number("0xff")}}, + {"unclosed placeholder", "https://api.example.com", "/repos/{repo", map[string]any{"repo": "hello"}}, + {"empty placeholder", "https://api.example.com", "/repos/{}", map[string]any{"": "hello"}}, + {"host change", "https://api.example.com", ".evil.example/repos", nil}, + {"base escape error", "https://api.example.com/%zz", "/repos", nil}, + {"final escape error", "https://api.example.com", "/repos/%zz", nil}, + {"relative base", "/api", "/repos", nil}, + {"opaque base", "https:api.example.com", "/repos", nil}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got, err := buildURL(tc.base, tc.path, tc.params); err == nil || got != "" { + t.Fatalf("invalid URL accepted: %q, error=%v", got, err) + } + }) + } +} + +func TestBuildURLShippedTemplates(t *testing.T) { + t.Parallel() + var templates int + for _, root := range []string{"../../config", "../../configs"} { + err := filepath.WalkDir(root, func(path string, entry fs.DirEntry, err error) error { + if err != nil { + return err + } + if entry.IsDir() || (filepath.Ext(path) != ".yaml" && filepath.Ext(path) != ".yml") { + return nil + } + reg := registry.New() + if err := reg.LoadFile(path); err != nil { + return err + } + for _, name := range reg.List() { + svc, err := reg.Get(name) + if err != nil { + return err + } + for actionName, action := range svc.Actions { + templates++ + t.Run(path+"/"+name+"/"+actionName, func(t *testing.T) { + got, err := buildURL(svc.BaseURL, action.Path, map[string]any{ + "owner": "octocat", + "repo": "hello-world", + }) + if err != nil { + t.Fatal(err) + } + wantPath := strings.NewReplacer("{owner}", "octocat", "{repo}", "hello-world").Replace(action.Path) + if strings.ContainsAny(wantPath, "{}") { + t.Fatalf("add representative params for template %q", action.Path) + } + if want := strings.TrimRight(svc.BaseURL, "/") + wantPath; got != want { + t.Fatalf("got %q, want %q", got, want) + } + }) + } + } + return nil + }) + if err != nil { + t.Fatal(err) + } + } + if templates == 0 { + t.Fatal("no shipped templates checked") + } + t.Logf("checked %d shipped templates", templates) +} + +func TestActPathParamsStayEscapedOnWire(t *testing.T) { + cases := []struct { + name string + params string + want string + }{ + {"traversal", `{"repo":"../../admin/users"}`, "/repos/..%2F..%2Fadmin%2Fusers"}, + {"slash", `{"repo":"owner/repo"}`, "/repos/owner%2Frepo"}, + {"delimiters", `{"repo":"repo?admin=true#fragment"}`, "/repos/repo%3Fadmin=true%23fragment"}, + {"encoded dots", `{"repo":"%2e%2e"}`, "/repos/%252e%252e"}, + {"placeholder", `{"repo":"{owner}","owner":"octocat"}`, "/repos/%7Bowner%7D"}, + {"unicode", `{"repo":"café 東京"}`, "/repos/caf%C3%A9%20%E6%9D%B1%E4%BA%AC"}, + {"large number", `{"repo":9007199254740993}`, "/repos/9007199254740993"}, + {"boolean", `{"repo":false}`, "/repos/false"}, + {"unused structured params", `{"repo":"hello","body":{"labels":["bug"]},"optional":null}`, "/repos/hello"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + var calls atomic.Int32 + upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + calls.Add(1) + if r.RequestURI != tc.want || r.URL.RawQuery != "" || r.URL.Fragment != "" { + t.Errorf("unexpected upstream target: %q", r.RequestURI) + } + if r.Header.Get("Authorization") != "Bearer injected-secret" { + t.Error("upstream credential missing") + } + fmt.Fprint(w, `{"ok":true}`) + })) + defer upstream.Close() + srv, receipts := transportGateway(t, upstream.URL, "{}") + if err := srv.cfg.Registry.LoadBytes([]byte(fmt.Sprintf(` +services: + github: + base_url: %s + auth: {type: bearer} + actions: + list_repos: {method: GET, path: "/repos/{repo}"} +`, upstream.URL))); err != nil { + t.Fatal(err) + } + req := httptest.NewRequest("POST", "/v1/act", strings.NewReader( + `{"service":"github","action":"list_repos","on_behalf_of":"user-1","params":`+tc.params+`}`)) + req.Header.Set("Authorization", "Bearer test-key") + w := httptest.NewRecorder() + srv.ServeHTTP(w, req) + if w.Code != http.StatusOK || calls.Load() != 1 { + t.Fatalf("status=%d calls=%d body=%s", w.Code, calls.Load(), w.Body) + } + if receipts.count() != 1 { + t.Fatalf("receipts=%d, want 1", receipts.count()) + } + }) + } +} + +func TestActInvalidPathParamsAreReceipted(t *testing.T) { + cases := []struct { + name string + params string + }{ + {"missing", `{}`}, + {"empty", `{"repo":""}`}, + {"dot", `{"repo":"."}`}, + {"dot dot", `{"repo":".."}`}, + {"null", `{"repo":null}`}, + {"object", `{"repo":{"id":"hello"}}`}, + {"array", `{"repo":["hello"]}`}, + {"fraction", `{"repo":1.5}`}, + {"rounded fraction", `{"repo":9007199254740993.1}`}, + } + var calls atomic.Int32 + upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + calls.Add(1) + fmt.Fprint(w, `{"ok":true}`) + })) + defer upstream.Close() + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + srv, receipts := transportGateway(t, upstream.URL, "{}") + if err := srv.cfg.Registry.LoadBytes([]byte(fmt.Sprintf(` +services: + github: + base_url: %s + auth: {type: bearer} + actions: + list_repos: {method: GET, path: "/repos/{repo}"} +`, upstream.URL))); err != nil { + t.Fatal(err) + } + req := httptest.NewRequest("POST", "/v1/act", strings.NewReader( + `{"service":"github","action":"list_repos","on_behalf_of":"user-1","params":`+tc.params+`}`)) + req.Header.Set("Authorization", "Bearer test-key") + w := httptest.NewRecorder() + srv.ServeHTTP(w, req) + var body ErrorResponse + if err := json.Unmarshal(w.Body.Bytes(), &body); err != nil { + t.Fatal(err) + } + if w.Code != http.StatusBadRequest || body.Code != "bad_request" || body.Error != "invalid params" { + t.Errorf("status=%d body=%s", w.Code, w.Body) + } + if calls.Load() != 0 { + t.Errorf("upstream calls=%d, want 0", calls.Load()) + } + if receipts.count() != 1 { + t.Fatalf("receipts=%d, want 1", receipts.count()) + } + draft := receipts.last() + hash, err := receipt.DigestParams([]byte(tc.params)) + if err != nil { + t.Fatal(err) + } + if draft.StatusCode != 400 || draft.Error != "bad_request" || draft.PolicyDecision != "deny" || draft.ParamsSHA256 != hash { + t.Fatalf("unexpected receipt: %+v", draft) + } + }) + } +} + +type pathCountingVault struct { + vault.Store + gets atomic.Int32 + puts atomic.Int32 +} + +func (v *pathCountingVault) Get(userID, service string) (vault.Token, error) { + v.gets.Add(1) + return v.Store.Get(userID, service) +} + +func (v *pathCountingVault) Put(userID, service string, tok vault.Token) error { + v.puts.Add(1) + return v.Store.Put(userID, service, tok) +} + +func TestActInvalidPathParamsBeforeVault(t *testing.T) { + for _, hasToken := range []bool{false, true} { + t.Run(fmt.Sprintf("has_token=%t", hasToken), func(t *testing.T) { + srv, store, receipts := testSetup(t) + if hasToken { + if err := store.Put("user-1", "github", vault.Token{ + AccessToken: "expiring-token", + RefreshToken: "refresh-token", + ExpiresAt: time.Now().Add(time.Minute), + }); err != nil { + t.Fatal(err) + } + } + counted := &pathCountingVault{Store: store} + srv.cfg.Vault = counted + var refreshCalls atomic.Int32 + srv.cfg.Refreshers = map[string]vault.RefreshFunc{ + "github": func(string) (string, string, time.Duration, error) { + refreshCalls.Add(1) + return "new-token", "new-refresh", time.Hour, nil + }, + } + req := httptest.NewRequest("POST", "/v1/act", strings.NewReader( + `{"service":"github","action":"create_issue","on_behalf_of":"user-1","params":{"owner":"octocat","repo":".."}}`)) + req.Header.Set("Authorization", "Bearer test-key") + w := httptest.NewRecorder() + srv.ServeHTTP(w, req) + var body ErrorResponse + if err := json.Unmarshal(w.Body.Bytes(), &body); err != nil { + t.Fatal(err) + } + if w.Code != http.StatusBadRequest || body.Code != "bad_request" || body.Error != "invalid params" { + t.Errorf("status=%d body=%s", w.Code, w.Body) + } + if counted.gets.Load() != 0 || counted.puts.Load() != 0 || refreshCalls.Load() != 0 { + t.Errorf("vault gets=%d puts=%d refresh calls=%d, want all 0", + counted.gets.Load(), counted.puts.Load(), refreshCalls.Load()) + } + if receipts.count() != 1 { + t.Fatalf("receipts=%d, want 1", receipts.count()) + } + draft := receipts.last() + if draft.StatusCode != 400 || draft.Error != "bad_request" || draft.PolicyDecision != "deny" { + t.Fatalf("unexpected receipt: %+v", draft) + } + }) + } +} + +func TestActInvalidBaseURLIsInternal(t *testing.T) { + for _, base := range []string{ + "https://operator-private-value:private-password@api.example.com/%zz", + "/relative-base", + "https:opaque-base", + } { + t.Run(base, func(t *testing.T) { + srv, store, receipts := testSetup(t) + if err := srv.cfg.Registry.LoadBytes([]byte(fmt.Sprintf(` +services: + github: + base_url: %q + actions: + list_repos: {method: GET, path: /repos} +`, base))); err != nil { + t.Fatal(err) + } + if err := store.Put("user-1", "github", vault.Token{AccessToken: "access-token"}); err != nil { + t.Fatal(err) + } + counted := &pathCountingVault{Store: store} + srv.cfg.Vault = counted + w := transportAct(t, srv) + var body ErrorResponse + if err := json.Unmarshal(w.Body.Bytes(), &body); err != nil { + t.Fatal(err) + } + if w.Code != http.StatusInternalServerError || body.Code != "internal" || body.Error != "failed to build upstream request" { + t.Errorf("status=%d body=%s", w.Code, w.Body) + } + if counted.gets.Load() != 0 || counted.puts.Load() != 0 { + t.Errorf("vault gets=%d puts=%d, want both 0", counted.gets.Load(), counted.puts.Load()) + } + if receipts.count() != 1 { + t.Fatalf("receipts=%d, want 1", receipts.count()) + } + draft := receipts.last() + if draft.StatusCode != 500 || draft.Error != "internal" || draft.PolicyDecision != "allow" { + t.Fatalf("unexpected receipt: %+v", draft) + } + }) + } +} diff --git a/internal/registry/registry.go b/internal/registry/registry.go index bfedfcf..e18741b 100644 --- a/internal/registry/registry.go +++ b/internal/registry/registry.go @@ -92,6 +92,11 @@ func (r *Registry) LoadBytes(data []byte) error { if err := svc.Upstream.Validate(); err != nil { return fmt.Errorf("registry: service %s: %w", name, err) } + for actionName, action := range svc.Actions { + if _, query, ok := strings.Cut(action.Path, "?"); ok && strings.Contains(query, "{") { + return fmt.Errorf("registry: service %s action %s: placeholders are not allowed after '?'", name, actionName) + } + } } r.mu.Lock() defer r.mu.Unlock() diff --git a/internal/registry/registry_test.go b/internal/registry/registry_test.go index 31b585d..886fe9d 100644 --- a/internal/registry/registry_test.go +++ b/internal/registry/registry_test.go @@ -2,6 +2,7 @@ package registry import ( "errors" + "fmt" "reflect" "testing" ) @@ -123,3 +124,46 @@ func TestRegistry_List(t *testing.T) { t.Fatalf("list = %v", names) } } + +func TestRegistryRejectsQueryPlaceholders(t *testing.T) { + t.Parallel() + for _, path := range []string{ + "/search?q={q}", + "/repos/{repo}?fixed=operator-private-value&q={q}", + "/search?fixed=operator-private-value#{q}", + } { + t.Run(path, func(t *testing.T) { + r := New() + err := r.LoadBytes([]byte(fmt.Sprintf(` +services: + example: + base_url: https://api.example.com + actions: + search: + method: GET + path: %q +`, path))) + const want = "registry: service example action search: placeholders are not allowed after '?'" + if err == nil || err.Error() != want { + t.Fatalf("error=%v, want %q", err, want) + } + if r.Count() != 0 { + t.Fatalf("invalid catalog was loaded: %d services", r.Count()) + } + }) + } +} + +func TestRegistryAllowsStaticQueries(t *testing.T) { + t.Parallel() + r := New() + if err := r.LoadBytes([]byte(` +services: + example: + base_url: https://api.example.com + actions: + search: {method: GET, path: "/repos/{repo}?sort=updated&limit=10"} +`)); err != nil { + t.Fatal(err) + } +}