From 8af7222816338cd28e1410abef9969451f11b07c Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Sat, 17 Oct 2026 15:46:23 +0000 Subject: [PATCH] api/v2: stop echoing request values in error responses huma sets an unexpected property's value to the whole request object. A refused policy, or another 400, now carries its reason in message, and a failed test run one data entry per failing test naming its src. --- hscontrol/api/v2/errors.go | 79 +++++++++++++++++++++-- hscontrol/apiv2_acl_test.go | 59 +++++++++++++++++ hscontrol/apiv2_compat_test.go | 8 --- hscontrol/apiv2_oauth_test.go | 25 +++++++ hscontrol/policy/v2/sshtest.go | 20 +++++- hscontrol/policy/v2/test.go | 54 ++++++++++++++-- hscontrol/servertest/names_compat_test.go | 6 -- 7 files changed, 223 insertions(+), 28 deletions(-) diff --git a/hscontrol/api/v2/errors.go b/hscontrol/api/v2/errors.go index 82c389bc9..fe9288dda 100644 --- a/hscontrol/api/v2/errors.go +++ b/hscontrol/api/v2/errors.go @@ -2,11 +2,16 @@ package apiv2 import ( "errors" + "net/http" + "strconv" + "strings" "github.com/danielgtaylor/huma/v2" "github.com/juanfont/headscale/hscontrol/db" + policyv2 "github.com/juanfont/headscale/hscontrol/policy/v2" "github.com/juanfont/headscale/hscontrol/state" "gorm.io/gorm" + "tailscale.com/util/multierr" ) // apiError is the Tailscale API error body. The official Tailscale Go client @@ -26,6 +31,10 @@ type apiErrorData struct { Errors []string `json:"errors,omitempty"` } +// maxErrorDetails caps the errors one response lists. huma reports one per bad +// element of the body, so the caller picks how many there are. +const maxErrorDetails = 20 + // tailscaleErrorTransformer rewrites Huma's RFC 9457 error model into the // Tailscale error shape. It is registered on the v2 API config only, so the // headscale-native v1 API keeps emitting problem+json. Non-error bodies pass @@ -43,18 +52,75 @@ func tailscaleErrorTransformer(_ huma.Context, _ string, v any) (any, error) { out := apiError{Message: message, Status: em.Status} - if len(em.Errors) > 0 { - details := make([]string, 0, len(em.Errors)) - for _, d := range em.Errors { - details = append(details, d.Error()) + listed := em.Errors[:min(len(em.Errors), maxErrorDetails)] + + details := make([]string, 0, len(listed)+1) + for _, d := range listed { + // huma stores a nil detail for a nil error argument. + if d == nil { + continue } + // The value is not echoed: for an unexpected property it is the whole + // request object, once per property. + if d.Location == "" { + details = append(details, d.Message) + } else { + details = append(details, d.Message+" ("+d.Location+")") + } + } + + if more := len(em.Errors) - len(listed); more > 0 { + details = append(details, "and "+strconv.Itoa(more)+" more") + } + + if len(details) > 0 { out.Data = []apiErrorData{{Errors: details}} } return out, nil } +func (e *apiError) Error() string { return e.Message } + +func (e *apiError) GetStatus() int { return e.Status } + +// policyRefusal answers a refused policy as Tailscale does: the reason in +// message and, when tests failed, one data entry per failing test naming its +// src. +func policyRefusal(err error) error { + out := &apiError{Message: err.Error(), Status: http.StatusBadRequest} + + var failures []policyv2.TestFailure + + // Range visits every wrapped error, so errors.As would count a failure + // once per wrapper above it. + multierr.Range(err, func(e error) bool { + if tf, ok := e.(*policyv2.TestsFailedError); ok { //nolint:errorlint + failures = append(failures, tf.Failures...) + } + + return true + }) + + if len(failures) == 0 { + return out + } + + out.Message = "test(s) failed" + + for _, f := range failures[:min(len(failures), maxErrorDetails)] { + // A policy writes a user known only by name as name@. + out.Data = append(out.Data, apiErrorData{User: strings.TrimSuffix(f.Src, "@"), Errors: f.Errors}) + } + + if more := len(failures) - maxErrorDetails; more > 0 { + out.Data = append(out.Data, apiErrorData{Errors: []string{"and " + strconv.Itoa(more) + " more"}}) + } + + return out +} + // mapError translates a state/db-layer error into a Huma HTTP error // (not-found→404, invalid input→400, everything else→500). The transformer // then reshapes it into the Tailscale body. @@ -77,11 +143,12 @@ func mapError(msg string, err error) error { errors.Is(err, state.ErrGivenNameTaken), errors.Is(err, state.ErrNodeNameNotUnique), errors.Is(err, state.ErrRequestedTagsInvalidOrNotPermitted): - return huma.Error400BadRequest(msg, err) + // As on Tailscale SaaS, the reason is the message. + return huma.Error400BadRequest(err.Error(), err) // A refused policy is answered with its own reason as the message. case errors.Is(err, state.ErrInvalidPolicy): - return huma.Error400BadRequest(err.Error(), err) + return policyRefusal(err) default: return huma.Error500InternalServerError(msg, err) diff --git a/hscontrol/apiv2_acl_test.go b/hscontrol/apiv2_acl_test.go index 9e707a47d..82f367d01 100644 --- a/hscontrol/apiv2_acl_test.go +++ b/hscontrol/apiv2_acl_test.go @@ -245,6 +245,53 @@ func TestAPIv2ACLPersistFailureKeepsLivePolicy(t *testing.T) { assert.Equal(t, before, after, "an unstored policy must not be enforced") } +// TestAPIv2ACLRefusalEnvelope: a refused policy answers with the reason in +// message and, when tests fail, one data entry per failing test naming its +// src. +func TestAPIv2ACLRefusalEnvelope(t *testing.T) { + app := createTestApp(t) + app.state.CreateUserForTest("alice") + api := registerAPIV2(t, app) + + type refusal struct { + Message string `json:"message"` + Data []struct { + User string `json:"user"` + Errors []string `json:"errors"` + } `json:"data"` + } + + decode := func(resp *httptest.ResponseRecorder) refusal { + t.Helper() + require.Equalf(t, http.StatusBadRequest, resp.Code, "body: %s", resp.Body) + + var r refusal + require.NoError(t, json.Unmarshal(resp.Body.Bytes(), &r)) + + return r + } + + parse := decode(setACL(t, api, `{ this is not valid`, "application/json")) + assert.NotEmpty(t, parse.Message) + assert.NotEqual(t, "setting policy", parse.Message, "the reason is the message") + assert.Empty(t, parse.Data) + + tests := decode(setACL(t, api, `{ + "acls": [{"action": "accept", "src": ["*"], "dst": ["*:*"]}], + "tests": [ + {"src": "alice@", "deny": ["100.64.0.2:22"]}, + {"src": "100.64.0.1", "accept": ["100.64.0.2:22"]}, + {"src": "100.64.0.3", "deny": ["100.64.0.2:22", "100.64.0.4:22"]} + ] + }`, "application/json")) + assert.Equal(t, "test(s) failed", tests.Message) + require.Len(t, tests.Data, 2) + assert.Equal(t, "alice", tests.Data[0].User) + assert.Len(t, tests.Data[0].Errors, 1) + assert.Equal(t, "100.64.0.3", tests.Data[1].User) + assert.Len(t, tests.Data[1].Errors, 2) +} + func TestAPIv2ACLNonDefaultTailnet404(t *testing.T) { api := registerAPIV2(t, createTestApp(t)) @@ -253,6 +300,18 @@ func TestAPIv2ACLNonDefaultTailnet404(t *testing.T) { api.Post("/api/v2/tailnet/example.com/acl", bytes.NewReader([]byte(allowAllPolicy))).Code) } +// TestAPIv2ACLUnknownPolicyMode: the handler's 500 carries a nil error detail, +// which the error transformer must not dereference. +func TestAPIv2ACLUnknownPolicyMode(t *testing.T) { + cfg := &types.Config{Policy: types.PolicyConfig{Mode: "unknown"}} + _, api := humatest.New(t, apiv2.Config()) + apiv2.Register(api, apiv2.Backend{Cfg: cfg}) + + get := api.Get("/api/v2/tailnet/-/acl") + assert.Equal(t, http.StatusInternalServerError, get.Code) + assert.Contains(t, get.Body.String(), "unsupported policy mode") +} + func TestAPIv2ACLFileModeReadOnly(t *testing.T) { dir := t.TempDir() path := filepath.Join(dir, "acl.hujson") diff --git a/hscontrol/apiv2_compat_test.go b/hscontrol/apiv2_compat_test.go index 0d2d0e6c5..88fb096a2 100644 --- a/hscontrol/apiv2_compat_test.go +++ b/hscontrol/apiv2_compat_test.go @@ -80,9 +80,6 @@ var apiv2CompatSkipReasons = map[string]string{ "oauth/K2/0/outcome": unknownKeyType, "oauth/K5/0/status": unknownKeyType, - "oauth/J3/1/detail": refusalDetail, - "oauth/J4/0/detail": refusalDetail, - "oauth/E4/0/status": tagRefusalStatus, "oauth/E5/0/status": tagRefusalStatus, "oauth/T2/1/status": tagRefusalStatus, @@ -180,10 +177,6 @@ var apiv2CompatSkipReasons = map[string]string{ "policy-api/P5-R2/0/message": noV2Validate, "policy-api/P5-R2/0/detail": noV2Validate, - "policy-api/P3/1/detail": refusalDetail, - "policy-api/P5-R1/1/detail": refusalDetail, - "policy-api/P5-R2/1/detail": refusalDetail, - "policy-api/P7/0/body": hujsonFormatted, "policy-api/P7/1/body": hujsonFormatted, } @@ -239,7 +232,6 @@ const ( deviceOAuthClientID = "DEVICE_OAUTH_CLIENT_ID: SaaS device bodies carry oauthClientId, naming the join key or the OAuth client that made it; headscale's have no such field" taggedDeviceUser = "TAGGED_DEVICE_USER: SaaS shows a tagged device's user as the login that made its key, empty for a key a token made; headscale shows tagged-devices" emptyRouteListsOmitted = "EMPTY_ROUTE_LISTS_OMITTED: SaaS sends advertisedRoutes and enabledRoutes as [] for a device without routes under fields=all; headscale leaves both out" - refusalDetail = "REFUSAL_DETAIL: SaaS puts a refused policy's reason in message, with one detail per failing test naming the user; headscale answers setting policy with the reason as one detail without a user" noV2Validate = "NO_V2_VALIDATE: headscale's v2 API has no /acl/validate" hujsonFormatted = "HUJSON_FORMATTED: SaaS stores a HuJSON policy formatted, comments kept; headscale stores the bytes as sent" ) diff --git a/hscontrol/apiv2_oauth_test.go b/hscontrol/apiv2_oauth_test.go index f585f13f2..39db71454 100644 --- a/hscontrol/apiv2_oauth_test.go +++ b/hscontrol/apiv2_oauth_test.go @@ -240,6 +240,31 @@ func TestAPIv2OAuth_RequestBodiesBounded(t *testing.T) { assert.Equal(t, http.StatusRequestEntityTooLarge, status) } +// TestAPIv2OAuth_ValidationErrorsBounded: huma reports one error per unexpected +// property, and each carries the whole request object as its value. +func TestAPIv2OAuth_ValidationErrorsBounded(t *testing.T) { + _, baseURL, admin := newOAuthTestServer(t) + tok := mintScopedToken(t, baseURL, admin, scope.AuthKeys) + + props := map[string]int{} + for i := range 1000 { + props[fmt.Sprintf("%040d", i)] = 0 + } + + status, body := apiReq(t, http.MethodPost, baseURL+"/api/v2/tailnet/-/keys", tok, props) + require.Equal(t, http.StatusUnprocessableEntity, status) + require.Less(t, len(body), 16<<10, "the error answer grows with the request") + assert.NotContains(t, string(body), "map[") + + var resp struct { + Data []struct{ Errors []string } + } + require.NoError(t, json.Unmarshal(body, &resp)) + require.Len(t, resp.Data, 1) + require.Len(t, resp.Data[0].Errors, 21) + assert.Equal(t, "and 980 more", resp.Data[0].Errors[20]) +} + func TestAPIv2OAuth_ScopeEnforcement(t *testing.T) { _, baseURL, admin := newOAuthTestServer(t) diff --git a/hscontrol/policy/v2/sshtest.go b/hscontrol/policy/v2/sshtest.go index 9b066c200..d4c643754 100644 --- a/hscontrol/policy/v2/sshtest.go +++ b/hscontrol/policy/v2/sshtest.go @@ -5,6 +5,7 @@ import ( "maps" "net/netip" "slices" + "strings" "github.com/juanfont/headscale/hscontrol/types" "go4.org/netipx" @@ -106,6 +107,21 @@ func (r SSHPolicyTestResults) Errors() string { return failures.String() } +// failures is the breakdown apart per failing entry, so an API answer can +// name each entry's src. +func (r SSHPolicyTestResults) failures() []TestFailure { + var out []TestFailure + + for _, res := range r.Results { + if !res.Passed { + one := SSHPolicyTestResults{Results: []SSHPolicyTestResult{res}} + out = append(out, TestFailure{Src: res.Src, Errors: strings.Split(one.Errors(), "\n")}) + } + } + + return out +} + func sortedUsers(m map[string][]string) []string { return slices.Sorted(maps.Keys(m)) } @@ -141,7 +157,7 @@ func (pm *PolicyManager) RunSSHTests() error { results := runSSHPolicyTests(pm.resolvedPol, pm.users, pm.nodes) - return wrapTestResult(errSSHPolicyTestsFailed, results.AllPassed, results.Errors) + return wrapTestResult(errSSHPolicyTestsFailed, results.AllPassed, results) } // evaluateSSHTests runs the block against pol without mutating live state. @@ -157,7 +173,7 @@ func evaluateSSHTests( pol = pol.withResolvedGroups(users, nodes) results := runSSHPolicyTests(pol, users, nodes) - return wrapTestResult(errSSHPolicyTestsFailed, results.AllPassed, results.Errors) + return wrapTestResult(errSSHPolicyTestsFailed, results.AllPassed, results) } // runSSHPolicyTests evaluates every sshTests entry. The SSH rules are diff --git a/hscontrol/policy/v2/test.go b/hscontrol/policy/v2/test.go index 911857b2d..c0518657c 100644 --- a/hscontrol/policy/v2/test.go +++ b/hscontrol/policy/v2/test.go @@ -257,15 +257,57 @@ func clipTestLabel(s string) string { return strings.ToValidUTF8(s[:maxTestLabelBytes], "") + "..." } +// TestFailure is one failing tests or sshTests entry: its src as written and +// its lines of the failure breakdown. +type TestFailure struct { + Src string + Errors []string +} + +// TestsFailedError is a failed tests or sshTests run. It reads as the whole +// breakdown; Failures holds it per entry. +type TestsFailedError struct { + err error + Failures []TestFailure +} + +func (e *TestsFailedError) Error() string { return e.err.Error() } + +func (e *TestsFailedError) Unwrap() error { return e.err } + +// failures is the breakdown apart per failing entry, so an API answer can +// name each entry's src. +func (r PolicyTestResults) failures() []TestFailure { + var out []TestFailure + + for _, res := range r.Results { + if !res.Passed { + one := PolicyTestResults{Results: []PolicyTestResult{res}} + out = append(out, TestFailure{Src: res.Src, Errors: strings.Split(one.Errors(), "\n")}) + } + } + + return out +} + +// testReport is the outcome of a tests or sshTests run. +type testReport interface { + Errors() string + failures() []TestFailure +} + // wrapTestResult returns nil when every test passed, otherwise wraps the -// rendered failure breakdown in sentinel. errs is passed uncalled so it is -// only evaluated on the failure path. -func wrapTestResult(sentinel error, allPassed bool, errs func() string) error { +// rendered failure breakdown in sentinel. The report is rendered only on the +// failure path. +func wrapTestResult(sentinel error, allPassed bool, r testReport) error { if allPassed { return nil } - return fmt.Errorf("%w:\n%s", sentinel, errs()) + return &TestsFailedError{ + err: fmt.Errorf("%w:\n%s", sentinel, r.Errors()), + Failures: r.failures(), + } } // RunTests evaluates the policy's own `tests` block against the live compiled @@ -281,7 +323,7 @@ func (pm *PolicyManager) RunTests() error { results := runPolicyTests(pm.resolvedPol, pm.filter, pm.users, pm.nodes) - return wrapTestResult(errPolicyTestsFailed, results.AllPassed, results.Errors) + return wrapTestResult(errPolicyTestsFailed, results.AllPassed, results) } // evaluateTests runs the `tests` block against a fresh compilation of pol. @@ -304,7 +346,7 @@ func evaluateTests(pol *Policy, users []types.User, nodes views.Slice[types.Node results := runPolicyTests(pol, filter, users, nodes) - return wrapTestResult(errPolicyTestsFailed, results.AllPassed, results.Errors) + return wrapTestResult(errPolicyTestsFailed, results.AllPassed, results) } // runPolicyTests is the pure evaluation function: given a policy, the diff --git a/hscontrol/servertest/names_compat_test.go b/hscontrol/servertest/names_compat_test.go index da3bf98b8..d7846a504 100644 --- a/hscontrol/servertest/names_compat_test.go +++ b/hscontrol/servertest/names_compat_test.go @@ -60,16 +60,10 @@ var nameSkipReasons = map[string]string{ "names/N12a/pdb/label": adminNameFollowsHostname, "names/N12b/pdb/label": adminNameFollowsHostname, "names/N13/pdb/label": adminNameFollowsHostname, - - "names/N9a/0/message": nameTakenText, - "names/N9b/0/message": nameTakenText, - - "names-svc/S5/0/message": nameTakenText, } const ( adminNameFollowsHostname = "ADMIN_NAME_FOLLOWS_HOSTNAME: an admin name equal to the hostname's label counts as derived from it, so a hostname change replaces it; SaaS keeps an admin name across hostname changes" - nameTakenText = "NAME_TAKEN_TEXT: headscale's v2 message for a rename refused as taken is the operation, with the reason only in data; SaaS's message says the name is already taken" v2NameBare = "V2_NAME_BARE: headscale's v2 device name is the bare label; SaaS's is the FQDN under the tailnet domain" )