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(), "