From b4d2309a5701a99284ac10f7c627922017cab6ab Mon Sep 17 00:00:00 2001 From: Shreyansh Sancheti <43677304+shreyanshjain7174@users.noreply.github.com> Date: Sun, 18 Oct 2026 05:45:19 +0530 Subject: [PATCH] fix: resolve four Semgrep security findings Default admin cookies to Secure except for explicit local HTTP. Direct TLS always enables Secure. OAuth state permits Strict cookies. Scope suppressions to safe parser IDs and JSON-encoded NDJSON. Test cookie modes, export headers, and receipt text encoding. Signed-off-by: Shreyansh Sancheti <43677304+shreyanshjain7174@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- README.md | 12 ++++- cmd/agentgate-mcp/main.go | 2 + internal/admin/handler.go | 5 +- internal/admin/handler_test.go | 62 +++++++++++++++++++++++++ internal/admin/session.go | 17 +++++-- internal/admin/session_test.go | 53 ++++++++++++++++----- internal/receipt/export_handler.go | 2 + internal/receipt/export_handler_test.go | 34 +++++++++++++- 8 files changed, 170 insertions(+), 17 deletions(-) diff --git a/README.md b/README.md index 43f69b8..d7b41f2 100644 --- a/README.md +++ b/README.md @@ -369,7 +369,7 @@ Set `AGENTGATE_VAULT_CACHE_TTL=0` to disable plaintext caching. Refresh sharing | `AGENTGATE_VAULT_KEY` | 32-byte vault key. Also derives receipt encryption keys and the API-key HMAC pepper. | Yes | | `AGENTGATE_VAULT_CACHE_TTL` | Decrypted-token cache duration. Default `60s`; `0` disables caching. Negative or invalid durations stop startup. | No | | `AGENTGATE_ADMIN_SECRET` | Secret for admin API access | Yes | -| `AGENTGATE_PUBLIC_URL` | Base URL used to build the OAuth callback (default `http://localhost:8080`) | No | +| `AGENTGATE_PUBLIC_URL` | Public base URL for OAuth callbacks and admin cookie security. Default `http://localhost:8080`. | No | | `AGENTGATE_API_KEY_CACHE_TTL` | Verified-key cache duration. Default `30s`; `0` disables caching. | No | | `AGENTGATE_RATE_KEY_RPS` | Requests per second per API key ID. Default `20`. Zero requires tenant admission disabled. | No | | `AGENTGATE_RATE_KEY_BURST` | Per-key token bucket burst. Default `40`. Zero requires tenant admission disabled. | No | @@ -453,6 +453,16 @@ The early rate limiter runs before these checks. Structural caps then bound fact Biscuit-go v2.2.0 deadlines do not cover check evaluation and cannot reliably stop timed-out goroutines. Runtime limits alone do not bound per-request CPU. +### Admin Session Cookies + +Set `AGENTGATE_PUBLIC_URL` to your HTTPS address behind a TLS-terminating proxy. +Admin cookies default to `Secure`, `HttpOnly`, and `SameSite=Strict`, with a 1-hour lifetime and `Path=/`. +Only explicit HTTP URLs on `localhost` or `127.0.0.1` disable `Secure` for local development. +Direct TLS requests always set `Secure`, including when the public URL uses local HTTP. +Forwarded headers cannot downgrade cookie security. +Logout clears the cookie with the same security attributes. +OAuth callbacks validate encrypted state without an admin session, so they don't need `SameSite=Lax`. + ### Service Configs Service configurations are YAML files in `configs/services/`. See `configs/services/google_workspace.yaml` for an example. The gateway itself loads the merged `configs/services.yaml`. diff --git a/cmd/agentgate-mcp/main.go b/cmd/agentgate-mcp/main.go index ca22312..757ce99 100644 --- a/cmd/agentgate-mcp/main.go +++ b/cmd/agentgate-mcp/main.go @@ -59,6 +59,8 @@ func serve(ctx context.Context, input io.Reader, output io.Writer, client *sdk.C scanner.Buffer(make([]byte, 4096), maxLineBytes) encoder := json.NewEncoder(output) for scanner.Scan() { + // parseRequest returns a value with an echoable ID on errors, never a nil pointer. + // nosemgrep: trailofbits.go.invalid-usage-of-modified-variable.invalid-usage-of-modified-variable req, envelopeError := parseRequest(scanner.Bytes()) if envelopeError != nil { if err := encoder.Encode(map[string]any{"jsonrpc": "2.0", "id": req.ID, "error": envelopeError}); err != nil { diff --git a/internal/admin/handler.go b/internal/admin/handler.go index 3de61eb..96ffb93 100644 --- a/internal/admin/handler.go +++ b/internal/admin/handler.go @@ -118,12 +118,15 @@ func (h *Handler) Login(w http.ResponseWriter, r *http.Request) { http.Error(w, "login failed", http.StatusInternalServerError) return } + cookie.Secure = cookie.Secure || r.TLS != nil http.SetCookie(w, cookie) http.Redirect(w, r, "/admin/login", http.StatusSeeOther) } func (h *Handler) Logout(w http.ResponseWriter, r *http.Request) { - http.SetCookie(w, h.sessions.ClearCookie()) + cookie := h.sessions.ClearCookie() + cookie.Secure = cookie.Secure || r.TLS != nil + http.SetCookie(w, cookie) http.Redirect(w, r, "/admin/login", http.StatusSeeOther) } diff --git a/internal/admin/handler_test.go b/internal/admin/handler_test.go index 20f4113..d97d295 100644 --- a/internal/admin/handler_test.go +++ b/internal/admin/handler_test.go @@ -65,6 +65,68 @@ func TestHandlerLoginSetsSessionCookie(t *testing.T) { } } +func TestHandlerSessionCookieSecurity(t *testing.T) { + t.Parallel() + tests := []struct { + name string + publicURL string + requestURL string + forwardedProto string + wantSecure bool + }{ + {name: "https proxy", publicURL: "https://agentgate.example", requestURL: "http://agentgate.example", wantSecure: true}, + {name: "localhost development", publicURL: "http://localhost:8080", requestURL: "http://localhost:8080"}, + {name: "loopback development", publicURL: "http://127.0.0.1:8080", requestURL: "http://127.0.0.1:8080"}, + {name: "direct tls", publicURL: "http://localhost:8080", requestURL: "https://localhost:8080", wantSecure: true}, + {name: "remote http", publicURL: "http://agentgate.example", requestURL: "http://agentgate.example", wantSecure: true}, + {name: "missing public url", requestURL: "http://agentgate.example", wantSecure: true}, + {name: "forwarded http cannot downgrade", publicURL: "https://agentgate.example", requestURL: "http://agentgate.example", forwardedProto: "http", wantSecure: true}, + {name: "forwarded https is not trusted", publicURL: "http://localhost:8080", requestURL: "http://localhost:8080", forwardedProto: "https"}, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + handler, _ := testHandler(t) + handler.sessions = NewSessionManager([]byte("dev-key-change-in-production-32b"), test.publicURL) + form := url.Values{"email": {"admin@example.com"}, "password": {"correct horse battery staple"}} + request := httptest.NewRequest(http.MethodPost, test.requestURL+"/admin/login", bytes.NewBufferString(form.Encode())) + request.Header.Set("Content-Type", "application/x-www-form-urlencoded") + request.Header.Set("X-Forwarded-Proto", test.forwardedProto) + response := httptest.NewRecorder() + handler.Login(response, request) + if response.Code != http.StatusSeeOther { + t.Fatalf("Login status = %d, want %d", response.Code, http.StatusSeeOther) + } + loginCookies := response.Result().Cookies() + if len(loginCookies) != 1 { + t.Fatalf("Login returned %d cookies, want 1", len(loginCookies)) + } + + logoutRequest := httptest.NewRequest(http.MethodPost, test.requestURL+"/admin/logout", nil) + logoutRequest.Header.Set("X-Forwarded-Proto", test.forwardedProto) + logoutRequest.Header.Set(csrfHeaderName, csrfHeaderValue) + logoutRequest.AddCookie(loginCookies[0]) + logoutResponse := httptest.NewRecorder() + handler.RequireAdmin(http.HandlerFunc(handler.Logout)).ServeHTTP(logoutResponse, logoutRequest) + if logoutResponse.Code != http.StatusSeeOther { + t.Fatalf("Logout status = %d, want %d", logoutResponse.Code, http.StatusSeeOther) + } + logoutCookies := logoutResponse.Result().Cookies() + if len(logoutCookies) != 1 || logoutCookies[0].MaxAge != -1 { + t.Fatalf("Logout cookies = %#v", logoutCookies) + } + for _, cookie := range []*http.Cookie{loginCookies[0], logoutCookies[0]} { + if cookie.Secure != test.wantSecure || !cookie.HttpOnly || + cookie.SameSite != http.SameSiteStrictMode || cookie.Path != "/" { + t.Fatalf("cookie has incorrect attributes: %#v", cookie) + } + } + if loginCookies[0].MaxAge != int(sessionTTL.Seconds()) || loginCookies[0].Expires.IsZero() { + t.Fatalf("Login cookie has incorrect lifetime: %#v", loginCookies[0]) + } + }) + } +} + func TestHandlerRequireAdmin(t *testing.T) { t.Parallel() handler, identity := testHandler(t) diff --git a/internal/admin/session.go b/internal/admin/session.go index 5db7a51..c55ed75 100644 --- a/internal/admin/session.go +++ b/internal/admin/session.go @@ -10,6 +10,7 @@ import ( "fmt" "io" "net/http" + "net/url" "strings" "time" @@ -42,9 +43,15 @@ type SessionManager struct { } func NewSessionManager(masterKey []byte, publicURL string) *SessionManager { + secure := true + parsed, err := url.Parse(publicURL) + if err == nil && parsed.Scheme == "http" && parsed.User == nil && + (strings.EqualFold(parsed.Hostname(), "localhost") || parsed.Hostname() == "127.0.0.1") { + secure = false + } return &SessionManager{ key: signer.DerivePurposeKey(masterKey, sessionKeyPurpose), - secure: strings.HasPrefix(publicURL, "https://"), + secure: secure, now: time.Now, } } @@ -76,6 +83,8 @@ func (m *SessionManager) CreateCookie(identity SessionIdentity) (*http.Cookie, e } ciphertext := gcm.Seal(nonce, nonce, payload, nil) expiresAt := m.now().Add(sessionTTL) + // Secure is disabled only for explicit loopback HTTP development URLs. + // nosemgrep: go.lang.security.audit.net.cookie-missing-secure.cookie-missing-secure return &http.Cookie{ Name: sessionCookieName, Value: base64.URLEncoding.EncodeToString(ciphertext), @@ -84,7 +93,7 @@ func (m *SessionManager) CreateCookie(identity SessionIdentity) (*http.Cookie, e MaxAge: int(sessionTTL.Seconds()), HttpOnly: true, Secure: m.secure, - SameSite: http.SameSiteLaxMode, + SameSite: http.SameSiteStrictMode, }, nil } @@ -125,6 +134,8 @@ func (m *SessionManager) Authenticate(r *http.Request) (SessionIdentity, error) } func (m *SessionManager) ClearCookie() *http.Cookie { + // Secure is disabled only for explicit loopback HTTP development URLs. + // nosemgrep: go.lang.security.audit.net.cookie-missing-secure.cookie-missing-secure return &http.Cookie{ Name: sessionCookieName, Value: "", @@ -132,6 +143,6 @@ func (m *SessionManager) ClearCookie() *http.Cookie { MaxAge: -1, HttpOnly: true, Secure: m.secure, - SameSite: http.SameSiteLaxMode, + SameSite: http.SameSiteStrictMode, } } diff --git a/internal/admin/session_test.go b/internal/admin/session_test.go index 3d5b6be..323d88a 100644 --- a/internal/admin/session_test.go +++ b/internal/admin/session_test.go @@ -26,7 +26,7 @@ func TestSessionManagerAuthenticate(t *testing.T) { if identity != (SessionIdentity{AdminID: "admin-1", OrgID: "org-1"}) { t.Fatalf("identity = %#v", identity) } - if !cookie.HttpOnly || !cookie.Secure || cookie.SameSite != http.SameSiteLaxMode || cookie.Path != "/" { + if !cookie.HttpOnly || !cookie.Secure || cookie.SameSite != http.SameSiteStrictMode || cookie.Path != "/" { t.Fatalf("cookie has unsafe attributes: %#v", cookie) } } @@ -71,16 +71,47 @@ func TestSessionManagerRejectsInvalidSessions(t *testing.T) { func TestSessionManagerCookieSecurityFollowsPublicURL(t *testing.T) { t.Parallel() - manager := NewSessionManager([]byte("dev-key-change-in-production-32b"), "http://localhost:8080") - cookie, err := manager.CreateCookie(SessionIdentity{AdminID: "admin-1", OrgID: "org-1"}) - if err != nil { - t.Fatalf("CreateCookie: %v", err) - } - if cookie.Secure { - t.Fatal("http public URL set Secure cookie") + tests := []struct { + name string + publicURL string + wantSecure bool + }{ + {name: "https", publicURL: "https://agentgate.example", wantSecure: true}, + {name: "uppercase https", publicURL: "HTTPS://agentgate.example", wantSecure: true}, + {name: "https localhost", publicURL: "https://localhost:8080", wantSecure: true}, + {name: "http localhost", publicURL: "http://localhost:8080"}, + {name: "http uppercase localhost", publicURL: "http://LOCALHOST:8080"}, + {name: "http loopback", publicURL: "http://127.0.0.1:8080"}, + {name: "http remote", publicURL: "http://agentgate.example", wantSecure: true}, + {name: "http lookalike", publicURL: "http://localhost.example", wantSecure: true}, + {name: "http userinfo", publicURL: "http://remote@localhost", wantSecure: true}, + {name: "http ipv6", publicURL: "http://[::1]:8080", wantSecure: true}, + {name: "missing url", wantSecure: true}, + {name: "relative url", publicURL: "/localhost", wantSecure: true}, + {name: "malformed url", publicURL: "http://localhost:%", wantSecure: true}, } - cleared := manager.ClearCookie() - if cleared.MaxAge != -1 || !cleared.HttpOnly || cleared.SameSite != http.SameSiteLaxMode { - t.Fatalf("ClearCookie = %#v", cleared) + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + now := time.Date(2026, time.September, 1, 12, 0, 0, 0, time.UTC) + manager := NewSessionManager([]byte("dev-key-change-in-production-32b"), test.publicURL) + manager.now = func() time.Time { return now } + cookie, err := manager.CreateCookie(SessionIdentity{AdminID: "admin-1", OrgID: "org-1"}) + if err != nil { + t.Fatalf("CreateCookie: %v", err) + } + if cookie.Name != sessionCookieName || cookie.Value == "" || + cookie.MaxAge != int(sessionTTL.Seconds()) || !cookie.Expires.Equal(now.Add(sessionTTL)) { + t.Fatalf("CreateCookie = %#v", cookie) + } + cleared := manager.ClearCookie() + if cleared.Name != sessionCookieName || cleared.Value != "" || cleared.MaxAge != -1 { + t.Fatalf("ClearCookie = %#v", cleared) + } + for _, c := range []*http.Cookie{cookie, cleared} { + if c.Secure != test.wantSecure || !c.HttpOnly || c.SameSite != http.SameSiteStrictMode || c.Path != "/" { + t.Fatalf("cookie has incorrect attributes: %#v", c) + } + } + }) } } diff --git a/internal/receipt/export_handler.go b/internal/receipt/export_handler.go index e810bb7..27eb962 100644 --- a/internal/receipt/export_handler.go +++ b/internal/receipt/export_handler.go @@ -227,6 +227,8 @@ func writeJSONLLine(w http.ResponseWriter, line []byte, err error) { if err != nil { return // the response is already committed by the time marshaling could fail; nothing safe left to do } + // Callers supply JSON-encoded records under application/x-ndjson with nosniff, not HTML. + // nosemgrep: go.lang.security.audit.xss.no-direct-write-to-responsewriter.no-direct-write-to-responsewriter w.Write(line) w.Write([]byte("\n")) } diff --git a/internal/receipt/export_handler_test.go b/internal/receipt/export_handler_test.go index 997b514..58e6feb 100644 --- a/internal/receipt/export_handler_test.go +++ b/internal/receipt/export_handler_test.go @@ -24,9 +24,12 @@ func TestExportHandler_FullRangeVerifiesOffline(t *testing.T) { if w.Code != http.StatusOK { t.Fatalf("status = %d, body = %s", w.Code, w.Body.String()) } - if ct := w.Header().Get("Content-Type"); ct != "application/x-ndjson" { + if ct := w.Result().Header.Get("Content-Type"); ct != "application/x-ndjson" { t.Fatalf("Content-Type = %s, want application/x-ndjson", ct) } + if got := w.Result().Header.Get("X-Content-Type-Options"); got != "nosniff" { + t.Fatalf("X-Content-Type-Options = %q, want nosniff", got) + } receipts, keys, manifest := parseExportBody(t, w.Body.String()) if manifest == nil { @@ -122,6 +125,12 @@ func TestExportHandler_InvalidRangesReject400(t *testing.T) { if w.Code != http.StatusBadRequest { t.Errorf("%s: status = %d, want 400", url, w.Code) } + if ct := w.Result().Header.Get("Content-Type"); ct != "application/json" { + t.Errorf("%s: Content-Type = %q, want application/json", url, ct) + } + if got := w.Result().Header.Get("X-Content-Type-Options"); got != "nosniff" { + t.Errorf("%s: X-Content-Type-Options = %q, want nosniff", url, got) + } } } @@ -219,6 +228,29 @@ func TestExportHandler_NoRawParamsOrSensitiveData(t *testing.T) { } } +func TestExportHandler_JSONEncodesReceiptText(t *testing.T) { + t.Parallel() + ledger, store := newTestLedgerAndStore(t) + draft := testDraft("github", "list_repos") + draft.HumanPrincipal = "\n{\"injected\":true}" + if _, err := ledger.Append(t.Context(), draft); err != nil { + t.Fatalf("Append: %v", err) + } + request := httptest.NewRequest(http.MethodGet, "/v1/receipts/export?from=1", nil) + response := httptest.NewRecorder() + ExportHandler(ledger.db, store)(response, request) + if response.Code != http.StatusOK { + t.Fatalf("status = %d, body = %s", response.Code, response.Body.String()) + } + if strings.Contains(response.Body.String(), "