From bead2b2158f533d227cd05e77e9b6ef5714dac62 Mon Sep 17 00:00:00 2001 From: Shreyansh Sancheti <43677304+shreyanshjain7174@users.noreply.github.com> Date: Sun, 18 Oct 2026 04:10:36 +0530 Subject: [PATCH 1/2] fix(gateway): escape path params Escape each path value once to prevent credentialed endpoint traversal. Reject missing or invalid segments and record denied attempts. Signed-off-by: Shreyansh Sancheti <43677304+shreyanshjain7174@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- README.md | 3 + docs/registry.md | 20 +++ internal/gateway/gateway.go | 95 +++++++++-- internal/gateway/gateway_test.go | 10 +- internal/gateway/path_test.go | 284 +++++++++++++++++++++++++++++++ 5 files changed, 396 insertions(+), 16 deletions(-) create mode 100644 internal/gateway/path_test.go 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..e5a10c7 100644 --- a/docs/registry.md +++ b/docs/registry.md @@ -35,6 +35,26 @@ 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. + +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`. +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..1bc306a 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" @@ -434,7 +438,15 @@ func (s *Server) executeAttempt(ctx context.Context, att *attempt) *outcome { } } - upstreamURL := buildURL(svc.BaseURL, action.Path, att.params) + upstreamURL, err := buildURL(svc.BaseURL, action.Path, att.params) + if err != nil { + return &outcome{ + status: http.StatusBadRequest, + body: ErrorResponse{Error: "invalid params", Code: "bad_request"}, + policyDecision: "deny", + errorCode: "bad_request", + } + } var bodyReader io.Reader if action.Method == "POST" || action.Method == "PUT" || action.Method == "PATCH" { @@ -561,19 +573,78 @@ 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") + } + + 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..0b1ea61 100644 --- a/internal/gateway/gateway_test.go +++ b/internal/gateway/gateway_test.go @@ -593,10 +593,9 @@ 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}"}, + {"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 +606,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..7f5af40 --- /dev/null +++ b/internal/gateway/path_test.go @@ -0,0 +1,284 @@ +package gateway + +import ( + "encoding/json" + "fmt" + "io/fs" + "math" + "net/http" + "net/http/httptest" + "path/filepath" + "strings" + "sync/atomic" + "testing" + + "github.com/Clawdlinux/agentgate/internal/receipt" + "github.com/Clawdlinux/agentgate/internal/registry" +) + +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 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) + } + }) + } +} From 421fc4a8731c3ea1e2da3ba65c892d678fac83ac Mon Sep 17 00:00:00 2001 From: Shreyansh Sancheti <43677304+shreyanshjain7174@users.noreply.github.com> Date: Sun, 18 Oct 2026 04:18:04 +0530 Subject: [PATCH 2/2] fix(gateway): validate URLs before vault access Reject query placeholders before loading registry actions. Validate URLs before vault access to avoid refresh side effects. Keep invalid base URLs separate from agent input errors. Signed-off-by: Shreyansh Sancheti <43677304+shreyanshjain7174@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- docs/registry.md | 7 ++ internal/gateway/gateway.go | 31 ++++--- internal/gateway/gateway_test.go | 5 +- internal/gateway/path_test.go | 126 +++++++++++++++++++++++++++++ internal/registry/registry.go | 5 ++ internal/registry/registry_test.go | 44 ++++++++++ 6 files changed, 206 insertions(+), 12 deletions(-) diff --git a/docs/registry.md b/docs/registry.md index e5a10c7..aa3033e 100644 --- a/docs/registry.md +++ b/docs/registry.md @@ -43,6 +43,10 @@ Slashes, query delimiters, fragment delimiters, percent signs, and spaces are en 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. @@ -51,6 +55,9 @@ Other params can still contain objects, arrays, or null when they aren't used in 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. diff --git a/internal/gateway/gateway.go b/internal/gateway/gateway.go index 1bc306a..1ffd3df 100644 --- a/internal/gateway/gateway.go +++ b/internal/gateway/gateway.go @@ -406,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{ @@ -438,16 +456,6 @@ func (s *Server) executeAttempt(ctx context.Context, att *attempt) *outcome { } } - upstreamURL, err := buildURL(svc.BaseURL, action.Path, att.params) - if err != nil { - return &outcome{ - status: http.StatusBadRequest, - body: ErrorResponse{Error: "invalid params", Code: "bad_request"}, - policyDecision: "deny", - errorCode: "bad_request", - } - } - var bodyReader io.Reader if action.Method == "POST" || action.Method == "PUT" || action.Method == "PATCH" { bodyReader = bytes.NewReader(att.req.Params) @@ -581,6 +589,9 @@ func buildURL(baseURL, pathTemplate string, params map[string]any) (string, erro 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 { diff --git a/internal/gateway/gateway_test.go b/internal/gateway/gateway_test.go index 0b1ea61..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,6 +593,7 @@ 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"}, + {"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/%F0%9F%98%80/true/%F0%9F%98%80"}, diff --git a/internal/gateway/path_test.go b/internal/gateway/path_test.go index 7f5af40..0c88f6a 100644 --- a/internal/gateway/path_test.go +++ b/internal/gateway/path_test.go @@ -11,9 +11,11 @@ import ( "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) { @@ -56,6 +58,21 @@ func TestBuildURLPathSegments(t *testing.T) { } } +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++ { @@ -282,3 +299,112 @@ services: }) } } + +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) + } +}