diff --git a/contract/battery.go b/contract/battery.go index 7869cae..29539f9 100644 --- a/contract/battery.go +++ b/contract/battery.go @@ -171,6 +171,8 @@ func Cases() []Case { Notes: "Malformed cursor (camelCase key): InvalidArgument 'invalid after_cursor'."}, {Name: "tickets/list_unknown_param_ignored", Method: "GET", Path: "/api/v1/tickets?utterly_unknown=42", Auth: AuthReadTickets, Notes: "Unknown query parameter ignored: identical payload to list_default."}, + {Name: "tickets/list_modified_since_over_uint32", Method: "GET", Path: "/api/v1/tickets?modified_since=4294967296", Auth: AuthReadTickets, + Notes: "modifiedSince is uint32-backed: the handler rejects values above 4294967295 with 'value out of range', pinned here at the response level. Sent via the snake_case alias 'modified_since', which the spec validator treats as an ignored unknown param — so this does NOT exercise the request-level maximum; that bound is pinned by TestSpec_Uint32ParamsBounded (and enforced on the camelCase modifiedSince key by the request validator, cf. get_by_id_over_uint32)."}, // --- Tickets: get by id / ref --------------------------------------- {Name: "tickets/get_by_id_happy", Method: "GET", Path: "/api/v1/tickets/42", Auth: AuthReadTickets, @@ -179,6 +181,8 @@ func Cases() []Case { Notes: "NotFound names the numeric id."}, {Name: "tickets/get_by_id_parse_error", Method: "GET", Path: "/api/v1/tickets/abc", Auth: AuthReadTickets, Notes: "Non-numeric id under {ticket_id}: leaked parse-error text. Frozen."}, + {Name: "tickets/get_by_id_over_uint32", Method: "GET", Path: "/api/v1/tickets/4294967296", Auth: AuthReadTickets, + Notes: "ticketId path param is uint32-backed: a value above 4294967295 leaks the parse 'value out of range'. Pins the spec's maximum: 4294967295 on the path param."}, {Name: "tickets/get_by_ref_happy", Method: "GET", Path: "/api/v1/tickets/ref/MF1UI9HE", Auth: AuthReadTickets, Notes: "Ref binding returns the same GetTicketResponse shape."}, {Name: "tickets/get_by_ref_not_found", Method: "GET", Path: "/api/v1/tickets/ref/NOPE9999", Auth: AuthReadTickets, diff --git a/contract/goldens/tickets/get_by_id_over_uint32.golden.json b/contract/goldens/tickets/get_by_id_over_uint32.golden.json new file mode 100644 index 0000000..0b7117b --- /dev/null +++ b/contract/goldens/tickets/get_by_id_over_uint32.golden.json @@ -0,0 +1,16 @@ +{ + "auth": "read:tickets", + "body": { + "code": 3, + "details": [], + "message": "type mismatch, parameter: ticket_id, error: strconv.ParseUint: parsing \"4294967296\": value out of range" + }, + "case": "tickets/get_by_id_over_uint32", + "header": { + "Content-Type": "application/json" + }, + "method": "GET", + "notes": "ticketId path param is uint32-backed: a value above 4294967295 leaks the parse 'value out of range'. Pins the spec's maximum: 4294967295 on the path param.", + "path": "/api/v1/tickets/4294967296", + "status": 400 +} diff --git a/contract/goldens/tickets/list_modified_since_over_uint32.golden.json b/contract/goldens/tickets/list_modified_since_over_uint32.golden.json new file mode 100644 index 0000000..52d6992 --- /dev/null +++ b/contract/goldens/tickets/list_modified_since_over_uint32.golden.json @@ -0,0 +1,16 @@ +{ + "auth": "read:tickets", + "body": { + "code": 3, + "details": [], + "message": "parsing field \"modified_since\": strconv.ParseUint: parsing \"4294967296\": value out of range" + }, + "case": "tickets/list_modified_since_over_uint32", + "header": { + "Content-Type": "application/json" + }, + "method": "GET", + "notes": "modifiedSince is uint32-backed: the handler rejects values above 4294967295 with 'value out of range', pinned here at the response level. Sent via the snake_case alias 'modified_since', which the spec validator treats as an ignored unknown param — so this does NOT exercise the request-level maximum; that bound is pinned by TestSpec_Uint32ParamsBounded (and enforced on the camelCase modifiedSince key by the request validator, cf. get_by_id_over_uint32).", + "path": "/api/v1/tickets?modified_since=4294967296", + "status": 400 +} diff --git a/contract/spec_test.go b/contract/spec_test.go index 4179884..9341e23 100644 --- a/contract/spec_test.go +++ b/contract/spec_test.go @@ -482,6 +482,11 @@ var specViolatingRequests = map[string]mustFailRequest{ wantType: helpers.ParameterValidation, wantMessage: "Path parameter 'ticketId' is not a valid integer", }, + "tickets/get_by_id_over_uint32": { + reason: "ticketId above 4294967295 violates the uint32 maximum on the path parameter", + wantType: helpers.ParameterValidation, + wantMessage: "Path parameter 'ticketId' failed to validate", + }, "tickets/messages_parse_error": { reason: "non-numeric ticketId violates the integer path parameter", wantType: helpers.ParameterValidation, @@ -511,7 +516,7 @@ func TestSpec_CarveOutMapsAreLive(t *testing.T) { assert.True(t, known[name], "templateUnmatchableCases key %q names no battery case", name) } - assert.Len(t, specViolatingRequests, 9, "must-fail request entries are pinned") + assert.Len(t, specViolatingRequests, 10, "must-fail request entries are pinned") for name := range specViolatingRequests { assert.True(t, known[name], "specViolatingRequests key %q names no battery case", name) } @@ -1089,3 +1094,72 @@ func TestSpec_EveryGoldenRouteInSpec(t *testing.T) { } } } + +// uint32Max is the uint32 ceiling the request binder enforces +// (strconv.ParseUint(raw, 10, 32) in rest/query.go and rest/tickets.go): a +// value above it is rejected with a 400 "value out of range". The spec must +// advertise the same bound so it does not promise a range the API cannot +// accept. +const uint32Max = 4294967295 + +// TestSpec_Uint32ParamsBounded pins the uint32 upper bound on every +// integer-typed request parameter. Numeric query/path parameters are backed +// by uint32 proto fields (see proto/tickets.proto: ticket_id, per_page, +// modified_since, category_id, status_id, prefix_id, assigned_user_id, +// starter_user_id are all uint32); 64-bit ids serialize as decimal strings +// (type: string) and are not integer parameters. Each integer parameter +// schema — whether scalar or the items of an array parameter — must declare +// minimum: 0 AND maximum: 4294967295, so a value above the ceiling is a +// DOCUMENTED 400, not an undocumented one. Without the maximum the spec +// advertises an unbounded range the binder rejects. +func TestSpec_Uint32ParamsBounded(t *testing.T) { + _, model := loadSpec(t) + + // checkIntegerSchema asserts s carries the uint32 bound when it is an + // integer schema; returns whether it was an integer schema. + checkIntegerSchema := func(where string, s *base.Schema) bool { + if s == nil || !slices.Contains(s.Type, "integer") { + return false + } + require.NotNil(t, s.Minimum, "%s: integer parameter declares no minimum", where) + assert.Equal(t, float64(0), *s.Minimum, "%s: uint32 parameter minimum must be 0", where) + require.NotNil(t, s.Maximum, + "%s: integer parameter declares no maximum — it is backed by uint32 and a value above %d is a 400 'value out of range'; declare maximum: %d", + where, uint32Max, uint32Max) + assert.Equal(t, float64(uint32Max), *s.Maximum, + "%s: uint32 parameter maximum must be %d", where, uint32Max) + return true + } + + bounded := 0 + for pair := orderedmap.First(model.Paths.PathItems); pair != nil; pair = pair.Next() { + route := pair.Key() + for method, op := range pair.Value().GetOperations().FromOldest() { + for _, p := range op.Parameters { + if p.Schema == nil || p.Schema.IsReference() { + continue + } + s := p.Schema.Schema() + if s == nil { + continue + } + where := fmt.Sprintf("%s %s param %q", strings.ToUpper(method), route, p.Name) + if checkIntegerSchema(where, s) { + bounded++ + } + // Array parameters (the repeated-filter query keys) carry the + // integer in items. + if s.Items != nil && s.Items.IsA() && !s.Items.A.IsReference() { + if checkIntegerSchema(where+"[]", s.Items.A.Schema()) { + bounded++ + } + } + } + } + } + // Non-vacuousness: the tickets surface alone has ticketId (×2), perPage + // (×2), modifiedSince, and the repeated uint32 filter items + // (categoryId, statusId, prefixId, assignedUserId, starterUserId). + assert.GreaterOrEqual(t, bounded, 10, + "expected the uint32 parameter set to be bounded, saw only %d", bounded) +} diff --git a/datastores/tickets.go b/datastores/tickets.go index 148ed01..2a169f1 100644 --- a/datastores/tickets.go +++ b/datastores/tickets.go @@ -38,18 +38,36 @@ var ErrInvalidCursor = errors.New("invalid cursor") // Compile-time assertion: Mysql must implement referencecache.Loader. var _ referencecache.Loader = (*Mysql)(nil) +// xf_phrase title prefixes for the three NF Tickets reference families. +// +// CAUTION — the add-on's phrase naming is ASYMMETRIC, not a typo below: +// status and priority titles carry NO `ticket_` infix +// (`nf_tickets_status.`, `nf_tickets_priority.`), but prefix titles +// DO (`nf_tickets_ticket_prefix.`). This mirrors exactly what the NF +// Tickets add-on writes to xf_phrase in production; querying status/priority +// with the `ticket_` infix matches zero rows and silently warms an empty +// cache (every statusName/priorityName resolves to ""). Do not "normalise" +// these to a uniform shape — the inconsistency is in the source data, and +// the testdb fixtures (testdb/fixtures.sql) seed these exact forms. +const ( + phrasePrefixStatus = "nf_tickets_status." + phrasePrefixPriority = "nf_tickets_priority." + phrasePrefixPrefix = "nf_tickets_ticket_prefix." +) + func (ds *Mysql) LoadStatusNames(ctx context.Context) (map[uint32]string, error) { - return ds.loadPhraseMap(ctx, "nf_tickets_ticket_status.") + return ds.loadPhraseMap(ctx, phrasePrefixStatus) } func (ds *Mysql) LoadPriorityNames(ctx context.Context) (map[uint32]string, error) { - return ds.loadPhraseMap(ctx, "nf_tickets_ticket_priority.") + return ds.loadPhraseMap(ctx, phrasePrefixPriority) } func (ds *Mysql) LoadPrefixNames(ctx context.Context) (map[uint32]string, error) { - return ds.loadPhraseMap(ctx, "nf_tickets_ticket_prefix.") + return ds.loadPhraseMap(ctx, phrasePrefixPrefix) } // loadPhraseMap reads rows from xf_phrase whose title starts with the given -// prefix (e.g. "nf_tickets_ticket_status."), parses the trailing integer id, +// prefix (one of the phrasePrefix* consts above, e.g. "nf_tickets_status."), +// parses the trailing integer id, // and returns id -> phrase_text. Rows where the trailing part isn't a // uint32 are skipped (not an error — the phrase table is shared, so // unrelated rows can be in scope of the LIKE pattern at the edges). diff --git a/openapi/openapi.yaml b/openapi/openapi.yaml index e9f3344..d759d1f 100644 --- a/openapi/openapi.yaml +++ b/openapi/openapi.yaml @@ -31,6 +31,15 @@ info: description: >- Read-only access to 7Cav.us milpacs (military personnel records) and forum tickets. All endpoints require a bearer API key. + + + Request leniency (frozen behavior, PRD #112): unknown query parameters are + ignored on every operation rather than rejected — a request carrying a + parameter this document does not declare is accepted, not a 400. This is + intentional and golden-pinned (e.g. roster/unknown_query_param_ignored, + tickets/list_unknown_param_ignored). Property-based tooling that flags such + a request as "accepted a schema-violating request" is observing expected + behavior; it is not a defect. version: dev externalDocs: description: 7Cav API Repository @@ -466,6 +475,7 @@ paths: items: type: integer minimum: 0 + maximum: 4294967295 - name: excludeSubcategories in: query required: false @@ -486,6 +496,7 @@ paths: items: type: integer minimum: 0 + maximum: 4294967295 - name: prefixId in: query required: false @@ -494,6 +505,7 @@ paths: items: type: integer minimum: 0 + maximum: 4294967295 - name: assignedUserId in: query required: false @@ -502,6 +514,7 @@ paths: items: type: integer minimum: 0 + maximum: 4294967295 - name: starterUserId in: query required: false @@ -510,12 +523,14 @@ paths: items: type: integer minimum: 0 + maximum: 4294967295 - name: modifiedSince in: query required: false schema: type: integer minimum: 0 + maximum: 4294967295 - name: includeHidden in: query required: false @@ -527,6 +542,7 @@ paths: schema: type: integer minimum: 0 + maximum: 4294967295 - name: afterCursor in: query required: false @@ -534,7 +550,8 @@ paths: type: string description: >- Opaque cursor; empty string = first page. Round-trip the - nextCursor from the previous response. + nextCursor from the previous response. A malformed cursor (one + not produced by this API) is rejected with 400. responses: "200": description: One page of tickets. @@ -575,6 +592,7 @@ paths: schema: type: integer minimum: 0 + maximum: 4294967295 responses: "200": description: The ticket with its first messages. @@ -651,12 +669,14 @@ paths: schema: type: integer minimum: 0 + maximum: 4294967295 - name: perPage in: query required: false schema: type: integer minimum: 0 + maximum: 4294967295 - name: afterCursor in: query required: false @@ -664,7 +684,8 @@ paths: type: string description: >- Opaque cursor; empty string = first page. Pagination is by - message position ascending. + message position ascending. A malformed cursor (one not produced + by this API) is rejected with 400. - name: includeHidden in: query required: false diff --git a/rest/auth.go b/rest/auth.go index cd7f6ff..fd8dd41 100644 --- a/rest/auth.go +++ b/rest/auth.go @@ -39,7 +39,7 @@ func AuthMiddleware(ds datastores.Datastore, next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { token := datastores.ParseBearerToken(r.Header.Get("Authorization"), maxTokenLen) if token == "" { - Warn.Printf("Unauthorized HTTP access attempt (bad bearer scheme) from %s", r.RemoteAddr) + Warn.Printf("Unauthorized HTTP access attempt (bad bearer scheme) from %s (peer %s)", clientIP(r), r.RemoteAddr) http.Error(w, errBearerScheme, http.StatusUnauthorized) return } @@ -60,7 +60,7 @@ func AuthMiddleware(ds datastores.Datastore, next http.Handler) http.Handler { // Zero rows: an unknown/expired key. The generic 401 tier — // leaks nothing about whether a key exists, is expired, or // lacks scopes (golden-pinned, #106). - Warn.Printf("Unauthorized HTTP access attempt from %s", r.RemoteAddr) + Warn.Printf("Unauthorized HTTP access attempt from %s (peer %s)", clientIP(r), r.RemoteAddr) http.Error(w, "Unauthorized", http.StatusUnauthorized) return } diff --git a/rest/auth_test.go b/rest/auth_test.go index e724a54..e23d039 100644 --- a/rest/auth_test.go +++ b/rest/auth_test.go @@ -6,6 +6,7 @@ package rest // stacks. import ( + "bytes" "io" "net/http" "net/http/httptest" @@ -136,6 +137,68 @@ func TestAuthMiddleware_DatastoreError_Is503AndLogged(t *testing.T) { assert.NotContains(t, logged, "cav7_secrettoken", "the bearer token must NEVER be logged") } +// captureWarnLog redirects the package Warn logger into a buffer for one test +// and restores the previous writer afterwards — the 401 tiers Warn-log the +// rejected attempt (the only place a caller address is logged). +func captureWarnLog(t *testing.T) *bytes.Buffer { + t.Helper() + var buf bytes.Buffer + prev := Warn.Writer() + Warn.SetOutput(&buf) + t.Cleanup(func() { Warn.SetOutput(prev) }) + return &buf +} + +// The two 401 warn lines must report the resolved client IP and the socket +// peer — `from (peer )` — each keeping its existing +// distinct prefix (#190, ADR 0005). With a trusted peer the client IP is +// resolved from the forwarding headers. + +func TestAuthMiddleware_BadScheme401_LogsClientAndPeer(t *testing.T) { + withTrustedProxiesEnv(t, "10.0.0.0/8") + require.NoError(t, InitTrustedProxies()) + buf := captureWarnLog(t) + + ds := &fakeAuthDatastore{validateApiKey: func(string) (*datastores.ApiKeyResult, error) { + t.Fatal("ValidateApiKey must not be called for a scheme error") + return nil, nil + }} + next := http.HandlerFunc(func(http.ResponseWriter, *http.Request) {}) + req := httptest.NewRequest(http.MethodGet, "/api/v1/whatever", nil) + req.RemoteAddr = "10.0.0.5:443" + req.Header.Set("X-Forwarded-For", "203.0.113.9, 10.0.0.4") + rr := httptest.NewRecorder() + AuthMiddleware(ds, next).ServeHTTP(rr, req) + + assert.Equal(t, http.StatusUnauthorized, rr.Code) + logged := buf.String() + assert.Contains(t, logged, "bad bearer scheme", "keeps its distinct prefix") + assert.Contains(t, logged, "from 203.0.113.9 (peer 10.0.0.5:443)") +} + +func TestAuthMiddleware_UnknownKey401_LogsClientAndPeer(t *testing.T) { + withTrustedProxiesEnv(t, "10.0.0.0/8") + require.NoError(t, InitTrustedProxies()) + buf := captureWarnLog(t) + + ds := &fakeAuthDatastore{validateApiKey: func(string) (*datastores.ApiKeyResult, error) { + return nil, nil // zero rows → unknown key + }} + next := http.HandlerFunc(func(http.ResponseWriter, *http.Request) {}) + req := httptest.NewRequest(http.MethodGet, "/api/v1/whatever", nil) + req.RemoteAddr = "10.0.0.5:443" + req.Header.Set("Authorization", "Bearer cav7_unknownkey") + req.Header.Set("X-Forwarded-For", "203.0.113.9, 10.0.0.4") + rr := httptest.NewRecorder() + AuthMiddleware(ds, next).ServeHTTP(rr, req) + + assert.Equal(t, http.StatusUnauthorized, rr.Code) + logged := buf.String() + // The unknown-key tier keeps its generic prefix (no "bad bearer scheme"). + assert.NotContains(t, logged, "bad bearer scheme") + assert.Contains(t, logged, "from 203.0.113.9 (peer 10.0.0.5:443)") +} + // requireScope is the per-route authorization gate (ADR 0004: scope checks // are per-handler; one scope never implies another). func TestRequireScope(t *testing.T) { diff --git a/rest/clientip.go b/rest/clientip.go new file mode 100644 index 0000000..93f1375 --- /dev/null +++ b/rest/clientip.go @@ -0,0 +1,150 @@ +package rest + +import ( + "fmt" + "net" + "net/http" + "strings" + + "github.com/spf13/viper" +) + +// Client-IP resolution for the two 401 log sites (ADR 0005). Behind the prod +// reverse proxy (Cloudflare → nginx-proxy-manager) r.RemoteAddr is always the +// proxy, so the forwarding headers carry the real caller — but only when the +// socket peer is a configured trusted proxy. resolveClientIP is pure (no +// globals) so the full trust rule is exhaustively table-testable; clientIP is +// the thin ambient wrapper the log sites call over the cached set. + +// trustedProxies is the parsed TRUSTED_PROXIES set, populated ONCE at startup +// by InitTrustedProxies and read by clientIP. nil/empty = trust nothing, which +// makes resolveClientIP ignore every forwarding header (the prior +// proxy-address logging behavior). +var trustedProxies []*net.IPNet + +// parseTrustedProxies turns the comma-split TRUSTED_PROXIES specs into CIDR +// networks. A bare IP is normalized to a host route (/32 for v4, /128 for v6). +// Empty/whitespace-only entries are skipped so a trailing comma is harmless. +// Any non-empty malformed entry is an error — the caller makes it fatal. +func parseTrustedProxies(specs []string) ([]*net.IPNet, error) { + var nets []*net.IPNet + for _, raw := range specs { + spec := strings.TrimSpace(raw) + if spec == "" { + continue + } + if !strings.Contains(spec, "/") { + ip := net.ParseIP(spec) + if ip == nil { + return nil, fmt.Errorf("invalid trusted proxy %q", spec) + } + if ip.To4() != nil { + spec += "/32" + } else { + spec += "/128" + } + } + _, n, err := net.ParseCIDR(spec) + if err != nil { + return nil, fmt.Errorf("invalid trusted proxy %q: %w", spec, err) + } + nets = append(nets, n) + } + return nets, nil +} + +// isTrusted reports whether ip falls inside any trusted-proxy network. +func isTrusted(ip net.IP, trusted []*net.IPNet) bool { + for _, n := range trusted { + if n.Contains(ip) { + return true + } + } + return false +} + +// peerIP extracts the bare IP from r.RemoteAddr, which is host:port (or +// [v6]:port) in production. It falls back to the original string only when +// splitting/parsing is impossible. +func peerIP(remoteAddr string) string { + host, _, err := net.SplitHostPort(remoteAddr) + if err != nil { + host = remoteAddr + } + return host +} + +// resolveClientIP implements the full ADR 0005 rule. Pure: no globals, the +// trusted set is passed in. +// +// - Peer NOT trusted → forwarding headers are ignored, bare peer IP used +// (an untrusted peer could forge them). +// - Peer trusted → take the right-most X-Forwarded-For entry NOT in the +// trusted set (walk right→left, skipping trusted hops). A malformed entry +// stops the walk and marks XFF unusable. XFF absent/unusable → X-Real-IP. +// That absent/invalid → bare peer IP. +// +// A single header value is read via Header.Get (no multi-header merge). +func resolveClientIP(r *http.Request, trusted []*net.IPNet) string { + peer := peerIP(r.RemoteAddr) + + peerAddr := net.ParseIP(peer) + if peerAddr == nil || !isTrusted(peerAddr, trusted) { + return peer + } + + if ip, ok := clientFromXFF(r.Header.Get("X-Forwarded-For"), trusted); ok { + return ip + } + + if real := strings.TrimSpace(r.Header.Get("X-Real-IP")); real != "" { + if net.ParseIP(real) != nil { + return real + } + } + + return peer +} + +// clientFromXFF walks an X-Forwarded-For value right→left, skipping entries in +// the trusted set, and returns the first (right-most) untrusted address. A +// malformed entry stops the walk and makes the whole header unusable (ok = +// false): the trail can't be reasoned about past a value we can't parse. ok is +// also false when the header is empty or every entry is a trusted hop. +func clientFromXFF(header string, trusted []*net.IPNet) (string, bool) { + if strings.TrimSpace(header) == "" { + return "", false + } + parts := strings.Split(header, ",") + for i := len(parts) - 1; i >= 0; i-- { + entry := strings.TrimSpace(parts[i]) + ip := net.ParseIP(entry) + if ip == nil { + return "", false // malformed entry: XFF unusable + } + if isTrusted(ip, trusted) { + continue // trusted hop appended by our own proxies + } + return entry, true + } + return "", false +} + +// InitTrustedProxies reads/parses/caches TRUSTED_PROXIES once at startup. +// Returns an error on a non-empty-but-malformed value (the caller makes it +// fatal). Empty/unset trusts nothing. +func InitTrustedProxies() error { + raw := viper.GetString("TRUSTED_PROXIES") + nets, err := parseTrustedProxies(strings.Split(raw, ",")) + if err != nil { + return err + } + trustedProxies = nets + return nil +} + +// clientIP is the thin ambient wrapper the two 401 log sites call. It resolves +// against the cached trusted set populated by InitTrustedProxies. +func clientIP(r *http.Request) string { + return resolveClientIP(r, trustedProxies) +} diff --git a/rest/clientip_test.go b/rest/clientip_test.go new file mode 100644 index 0000000..fd0b94b --- /dev/null +++ b/rest/clientip_test.go @@ -0,0 +1,207 @@ +package rest + +import ( + "net" + "net/http" + "net/http/httptest" + "testing" + + "github.com/spf13/viper" + "github.com/stretchr/testify/require" +) + +// withTrustedProxiesEnv sets the TRUSTED_PROXIES viper key for one test and +// restores it (plus the cached set) afterwards, matching the ambient +// viper.Set + Cleanup idiom the sentry tests use. +func withTrustedProxiesEnv(t *testing.T, value string) { + t.Helper() + prior := viper.GetString("TRUSTED_PROXIES") + priorSet := trustedProxies + viper.Set("TRUSTED_PROXIES", value) + t.Cleanup(func() { + viper.Set("TRUSTED_PROXIES", prior) + trustedProxies = priorSet + }) +} + +// mustCIDRs parses the given CIDR / bare-IP strings into the cached +// trusted-set shape resolveClientIP consumes. Bare IPs are normalized to a +// host route (/32 or /128) — the same normalization InitTrustedProxies does. +func mustCIDRs(t *testing.T, specs ...string) []*net.IPNet { + t.Helper() + nets, err := parseTrustedProxies(specs) + require.NoError(t, err) + return nets +} + +// reqWith builds a request with the given RemoteAddr and optional headers +// (key/value pairs) so resolveClientIP can be table-tested without a server. +func reqWith(remoteAddr string, headers map[string]string) *http.Request { + r := httptest.NewRequest(http.MethodGet, "/api/v1/x", nil) + r.RemoteAddr = remoteAddr + for k, v := range headers { + r.Header.Set(k, v) + } + return r +} + +func TestResolveClientIP_UntrustedPeer_IgnoresHeaders(t *testing.T) { + trusted := mustCIDRs(t, "10.0.0.0/8") + r := reqWith("203.0.113.7:54321", map[string]string{ + "X-Forwarded-For": "1.2.3.4", + "X-Real-IP": "5.6.7.8", + }) + require.Equal(t, "203.0.113.7", resolveClientIP(r, trusted)) +} + +func TestResolveClientIP_TrustedPeer_ValidXFF_RightmostUntrusted(t *testing.T) { + trusted := mustCIDRs(t, "10.0.0.0/8") + // peer is a trusted hop; XFF = , . + r := reqWith("10.0.0.5:443", map[string]string{ + "X-Forwarded-For": "203.0.113.9, 10.0.0.4", + }) + require.Equal(t, "203.0.113.9", resolveClientIP(r, trusted)) +} + +func TestResolveClientIP_TrustedPeer_SpoofedLeftmost_ReturnsRightmostUntrusted(t *testing.T) { + trusted := mustCIDRs(t, "10.0.0.0/8") + // Attacker pre-seeds a spoofed left-most entry; the real client and the + // trusted hop are appended to the right by our proxies. The right-most + // untrusted entry (the proxy-observed client) wins, not the spoof. + r := reqWith("10.0.0.5:443", map[string]string{ + "X-Forwarded-For": "9.9.9.9, 203.0.113.9, 10.0.0.4", + }) + require.Equal(t, "203.0.113.9", resolveClientIP(r, trusted)) +} + +func TestResolveClientIP_TrustedPeer_MalformedXFF_FallsBackToRealIP(t *testing.T) { + trusted := mustCIDRs(t, "10.0.0.0/8") + // A malformed entry stops the walk and marks XFF unusable → X-Real-IP. + r := reqWith("10.0.0.5:443", map[string]string{ + "X-Forwarded-For": "not-an-ip, 10.0.0.4", + "X-Real-IP": "198.51.100.7", + }) + require.Equal(t, "198.51.100.7", resolveClientIP(r, trusted)) +} + +// A port-bearing right-most XFF entry (e.g. 203.0.113.9:51000) does NOT get +// its port stripped: net.ParseIP rejects host:port, so clientFromXFF treats it +// as a malformed entry, stops the walk, and marks XFF unusable — same path as +// any other unparseable entry. This PINS the current no-port-strip-on-XFF +// behavior (ADR 0005): a future refactor that starts stripping ports from XFF +// entries would flip these results and surface here as a deliberate change. +func TestResolveClientIP_TrustedPeer_PortBearingXFF_FallsBackToRealIP(t *testing.T) { + trusted := mustCIDRs(t, "10.0.0.0/8") + // Right-most untrusted entry carries a :port → unparseable → XFF unusable → + // X-Real-IP fallback (NOT the port-stripped 203.0.113.9). + r := reqWith("10.0.0.5:443", map[string]string{ + "X-Forwarded-For": "203.0.113.9:51000, 10.0.0.4", + "X-Real-IP": "198.51.100.7", + }) + require.Equal(t, "198.51.100.7", resolveClientIP(r, trusted)) +} + +func TestResolveClientIP_TrustedPeer_PortBearingXFFNoRealIP_FallsBackToPeer(t *testing.T) { + trusted := mustCIDRs(t, "10.0.0.0/8") + // Same port-bearing (unusable) XFF, but no X-Real-IP → falls through to the + // bare peer IP rather than the port-stripped XFF address. + r := reqWith("10.0.0.5:443", map[string]string{ + "X-Forwarded-For": "203.0.113.9:51000, 10.0.0.4", + }) + require.Equal(t, "10.0.0.5", resolveClientIP(r, trusted)) +} + +func TestResolveClientIP_TrustedPeer_AllTrustedXFF_ReturnsRealIP(t *testing.T) { + trusted := mustCIDRs(t, "10.0.0.0/8") + // Every XFF entry is a trusted hop → XFF yields nothing → X-Real-IP. + r := reqWith("10.0.0.5:443", map[string]string{ + "X-Forwarded-For": "10.0.0.3, 10.0.0.4", + "X-Real-IP": "198.51.100.7", + }) + require.Equal(t, "198.51.100.7", resolveClientIP(r, trusted)) +} + +func TestResolveClientIP_TrustedPeer_NoUsableXFFNoRealIP_FallsBackToPeer(t *testing.T) { + trusted := mustCIDRs(t, "10.0.0.0/8") + // XFF all-trusted (nothing usable) and no X-Real-IP → bare peer IP. + r := reqWith("10.0.0.5:443", map[string]string{ + "X-Forwarded-For": "10.0.0.3", + }) + require.Equal(t, "10.0.0.5", resolveClientIP(r, trusted)) +} + +func TestResolveClientIP_TrustedPeer_InvalidRealIP_FallsBackToPeer(t *testing.T) { + trusted := mustCIDRs(t, "10.0.0.0/8") + // No XFF, X-Real-IP is garbage → bare peer IP. + r := reqWith("10.0.0.5:443", map[string]string{ + "X-Real-IP": "garbage", + }) + require.Equal(t, "10.0.0.5", resolveClientIP(r, trusted)) +} + +func TestResolveClientIP_IPv6PeerAndCIDR_ResolvesXFF(t *testing.T) { + // Trusted proxy hops live in 2001:db8::/64; the client (2001:db9::1234) is + // outside it, so the right-most untrusted XFF entry wins. + trusted := mustCIDRs(t, "2001:db8::/64") + r := reqWith("[2001:db8::5]:443", map[string]string{ + "X-Forwarded-For": "2001:db9::1234, 2001:db8::4", + }) + require.Equal(t, "2001:db9::1234", resolveClientIP(r, trusted)) +} + +func TestResolveClientIP_BareV6PeerNoPort_YieldsBareIP(t *testing.T) { + trusted := mustCIDRs(t, "10.0.0.0/8") + // RemoteAddr without a port can't be SplitHostPort'd; the original string + // (already a bare IP) is returned. Untrusted → headers ignored. + r := reqWith("203.0.113.7", map[string]string{ + "X-Forwarded-For": "1.2.3.4", + }) + require.Equal(t, "203.0.113.7", resolveClientIP(r, trusted)) +} + +func TestResolveClientIP_EmptyTrustedSet_HonorsNoHeader(t *testing.T) { + // Empty/unset TRUSTED_PROXIES trusts nothing — every peer is untrusted, + // so headers are never honored (prior proxy-address behavior). + r := reqWith("10.0.0.5:443", map[string]string{ + "X-Forwarded-For": "203.0.113.9", + "X-Real-IP": "198.51.100.7", + }) + require.Equal(t, "10.0.0.5", resolveClientIP(r, nil)) +} + +func TestParseTrustedProxies_BareIPsNormalizedToHostRoutes(t *testing.T) { + nets, err := parseTrustedProxies([]string{"10.0.0.4", "2001:db8::4"}) + require.NoError(t, err) + require.Len(t, nets, 2) + require.Equal(t, "10.0.0.4/32", nets[0].String()) + require.Equal(t, "2001:db8::4/128", nets[1].String()) +} + +func TestParseTrustedProxies_SkipsEmptyEntries(t *testing.T) { + // A trailing/leading comma or whitespace-only entry is harmless. + nets, err := parseTrustedProxies([]string{"10.0.0.0/8", " ", ""}) + require.NoError(t, err) + require.Len(t, nets, 1) +} + +func TestParseTrustedProxies_MalformedEntryErrors(t *testing.T) { + _, err := parseTrustedProxies([]string{"10.0.0.0/8", "not-a-cidr"}) + require.Error(t, err) +} + +func TestInitTrustedProxies_EmptyTrustsNothing(t *testing.T) { + withTrustedProxiesEnv(t, "") + require.NoError(t, InitTrustedProxies()) + require.Empty(t, trustedProxies) +} + +func TestInitTrustedProxies_NonEmptyMalformedReturnsError(t *testing.T) { + withTrustedProxiesEnv(t, "10.0.0.0/8,bogus") + require.Error(t, InitTrustedProxies()) +} + +func TestInitTrustedProxies_ParsesAndCaches(t *testing.T) { + withTrustedProxiesEnv(t, "10.0.0.0/8, 192.168.1.1") + require.NoError(t, InitTrustedProxies()) + require.Len(t, trustedProxies, 2) +} diff --git a/servers/server.go b/servers/server.go index 3bfb814..a8758a9 100644 --- a/servers/server.go +++ b/servers/server.go @@ -31,6 +31,7 @@ import ( "github.com/7cav/api/datastores" milpacs "github.com/7cav/api/proto" "github.com/7cav/api/referencecache" + "github.com/7cav/api/rest" httpServices "github.com/7cav/api/servers/gateway" grpcServices "github.com/7cav/api/servers/grpc" "github.com/spf13/viper" @@ -113,6 +114,17 @@ func (server *MicroServer) Start() { Info.Println("Starting 7Cav API version:", version) + // Resolve and cache the trusted-proxy set (TRUSTED_PROXIES, ADR 0005) ONCE + // before any listener opens: the shared rest.AuthMiddleware (legacy gateway + // and new stack both delegate to it) reads this cache to resolve the client + // IP for its 401 log lines. A non-empty-but-malformed value is fatal here — + // a misconfigured trust set must not start silently trusting nothing. + // CROSS-ISSUE: #134 must preserve this call when it rebuilds the + // single-listener composition root (documentation requirement, ADR 0005). + if err := rest.InitTrustedProxies(); err != nil { + Error.Fatalf("invalid TRUSTED_PROXIES: %v", err) + } + // Phase 0 observability (PRD #112): errors-only Sentry capture, gated on // SENTRY_DSN. Disabled (local/dev) nothing is initialised — one Info line, // no client, no signal handler, so shutdown behaves exactly as before. diff --git a/servers/server_test.go b/servers/server_test.go new file mode 100644 index 0000000..d11ad5f --- /dev/null +++ b/servers/server_test.go @@ -0,0 +1,183 @@ +package servers + +// Startup-wiring guard for the trusted-proxy set (#190, ADR 0005). The +// resolver behavior is exhaustively table-tested in rest/clientip_test.go, but +// nothing pinned that the composition root actually CALLS +// rest.InitTrustedProxies() — and makes a non-empty-but-malformed +// TRUSTED_PROXIES FATAL — BEFORE any listener opens. That call is load-bearing +// twice over: skip it and every 401 log line silently logs the proxy address +// instead of the real caller (trust-nothing fallback), and a misconfigured +// trust set boots silently instead of failing loud. Because Error.Fatalf calls +// os.Exit, a direct behavioral test of the fatal path would take the test +// process down with it; a source-level convention guard is the established +// idiom in this codebase for exactly this "a refactor could silently drop the +// wiring and every existing test would still pass" hazard (see +// rest/wrapperconventions_test.go and the flush-chain composition guard). It +// also makes the cross-issue #134 documentation requirement — "preserve this +// call when rebuilding the single-listener composition root" — self-announcing +// rather than remembered: dropping or reordering the call turns this red. + +import ( + "go/ast" + "go/parser" + "go/token" + "path/filepath" + "runtime" + "testing" + + "github.com/stretchr/testify/require" +) + +// startMethodDecl parses server.go (anchored on this test file's directory so +// the guard works regardless of how the suite is invoked, same anchor as the +// rest tag-lint) and returns the *MicroServer.Start FuncDecl plus the FileSet +// for position reasoning. +func startMethodDecl(t *testing.T) (*token.FileSet, *ast.FuncDecl) { + t.Helper() + _, thisFile, _, ok := runtime.Caller(0) + require.True(t, ok, "runtime.Caller failed — cannot locate the package directory") + src := filepath.Join(filepath.Dir(thisFile), "server.go") + + fset := token.NewFileSet() + f, err := parser.ParseFile(fset, src, nil, parser.SkipObjectResolution) + require.NoError(t, err, "server.go must parse") + + for _, decl := range f.Decls { + fd, ok := decl.(*ast.FuncDecl) + if !ok || fd.Name.Name != "Start" || fd.Recv == nil { + continue + } + return fset, fd + } + t.Fatal("could not find the Start method in server.go") + return nil, nil +} + +// selectorCall reports the source position of the first call expression whose +// callee is the selector pkg.fn (e.g. rest.InitTrustedProxies), and whether +// one was found, searching the given node. +func selectorCall(node ast.Node, pkg, fn string) (token.Pos, bool) { + var pos token.Pos + found := false + ast.Inspect(node, func(n ast.Node) bool { + if found { + return false + } + call, ok := n.(*ast.CallExpr) + if !ok { + return true + } + sel, ok := call.Fun.(*ast.SelectorExpr) + if !ok { + return true + } + x, ok := sel.X.(*ast.Ident) + if ok && x.Name == pkg && sel.Sel.Name == fn { + pos = call.Pos() + found = true + return false + } + return true + }) + return pos, found +} + +// firstSelectorCallPos is selectorCall's position-only twin, returning the +// max token.Pos sentinel when the call is absent so an absent serving call +// never appears to precede the init call in the ordering check. +func firstSelectorCallPos(node ast.Node, pkg, fn string) token.Pos { + if pos, ok := selectorCall(node, pkg, fn); ok { + return pos + } + return token.Pos(int(^uint(0) >> 1)) // max int — "never, so never earlier" +} + +// firstIdentCallPos returns the position of the first call to a bare function +// named fn (e.g. servHTTP), or the max sentinel when absent. +func firstIdentCallPos(node ast.Node, fn string) token.Pos { + pos := token.Pos(int(^uint(0) >> 1)) + found := false + ast.Inspect(node, func(n ast.Node) bool { + if found { + return false + } + call, ok := n.(*ast.CallExpr) + if !ok { + return true + } + if id, ok := call.Fun.(*ast.Ident); ok && id.Name == fn { + pos = call.Pos() + found = true + return false + } + return true + }) + return pos +} + +// TestStart_WiresInitTrustedProxiesBeforeServing is the wiring guard: the +// composition root must call rest.InitTrustedProxies() before it opens any +// listener or hands off to the serve loops, and that call's error must be made +// fatal via Error.Fatalf — not silently dropped (which would boot a +// misconfigured trust set trusting nothing). Source-level because +// Error.Fatalf's os.Exit makes the fatal path untestable in-process, and +// because the failure this defends against is precisely a refactor deleting or +// reordering the call with every existing behavioral test still green. +func TestStart_WiresInitTrustedProxiesBeforeServing(t *testing.T) { + _, start := startMethodDecl(t) + + initPos, ok := selectorCall(start, "rest", "InitTrustedProxies") + require.True(t, ok, + "Start must call rest.InitTrustedProxies() — without it the 401 log lines silently log the proxy address instead of the real caller (ADR 0005), and a malformed TRUSTED_PROXIES boots trusting nothing. CROSS-ISSUE #134 must preserve this call when rebuilding the single-listener composition root.") + + // The init call must precede every listener-open and serve-loop handoff: + // the trusted set has to be cached before any request can hit the shared + // AuthMiddleware that reads it. + require.Less(t, int(initPos), int(firstSelectorCallPos(start, "net", "Listen")), + "rest.InitTrustedProxies() must run BEFORE net.Listen opens a socket — the trusted set must be cached before any request can reach the 401 log sites") + require.Less(t, int(initPos), int(firstIdentCallPos(start, "servHTTP")), + "rest.InitTrustedProxies() must run BEFORE servHTTP starts serving") + require.Less(t, int(initPos), int(firstIdentCallPos(start, "servGRPC")), + "rest.InitTrustedProxies() must run BEFORE servGRPC starts serving") +} + +// TestStart_MakesMalformedTrustedProxiesFatal pins the error handling on the +// init call: its error must reach Error.Fatalf (which os.Exits), so a +// non-empty-but-malformed TRUSTED_PROXIES kills the boot instead of being +// swallowed. The fatal call must sit inside the same if-block that tests the +// InitTrustedProxies error, so the guard cannot be satisfied by an unrelated +// Error.Fatalf elsewhere in Start. +func TestStart_MakesMalformedTrustedProxiesFatal(t *testing.T) { + _, start := startMethodDecl(t) + + // Find the if-statement whose init/cond invokes rest.InitTrustedProxies, + // then assert that branch's body fatals on the error. + var guarded bool + ast.Inspect(start, func(n ast.Node) bool { + ifStmt, ok := n.(*ast.IfStmt) + if !ok { + return true + } + // The init call lives in the if's Init (err := rest.InitTrustedProxies()) + // or, defensively, its Cond. + callInInit := false + if ifStmt.Init != nil { + _, callInInit = selectorCall(ifStmt.Init, "rest", "InitTrustedProxies") + } + callInCond := false + if ifStmt.Cond != nil { + _, callInCond = selectorCall(ifStmt.Cond, "rest", "InitTrustedProxies") + } + if !callInInit && !callInCond { + return true + } + // This is the trusted-proxy init guard — its body must fatal. + if _, fatal := selectorCall(ifStmt.Body, "Error", "Fatalf"); fatal { + guarded = true + } + return false + }) + + require.True(t, guarded, + "the rest.InitTrustedProxies() error must be made fatal via Error.Fatalf in the same if-block — a swallowed error boots a misconfigured trust set silently trusting nothing (ADR 0005)") +} diff --git a/testdb/fixtures.sql b/testdb/fixtures.sql index 29c66bf..5c66359 100644 --- a/testdb/fixtures.sql +++ b/testdb/fixtures.sql @@ -189,15 +189,21 @@ INSERT INTO xf_nf_tickets_category (3, 'Tech Support', 'TeamSpeak and game tech',0, 0, 5, 6, 20, 2, '', '', '', '', '', ''); -- Phrases backing the reference cache: status / priority / prefix names. +-- The NF Tickets add-on's own phrase naming is inconsistent: status and +-- priority phrases are titled WITHOUT a `ticket_` infix +-- (`nf_tickets_status.`, `nf_tickets_priority.`), but prefix phrases +-- DO carry it (`nf_tickets_ticket_prefix.`). These seeds mirror the +-- real-world add-on naming exactly so the reference-cache loader is exercised +-- against the formats production actually writes. (see datastores/tickets.go) INSERT INTO xf_phrase (language_id, title, phrase_text) VALUES - (0, 'nf_tickets_ticket_status.1', 'Awaiting Support'), - (0, 'nf_tickets_ticket_status.2', 'In Progress'), - (0, 'nf_tickets_ticket_status.3', 'Closed'), - (0, 'nf_tickets_ticket_priority.1', 'Low'), - (0, 'nf_tickets_ticket_priority.2', 'Normal'), - (0, 'nf_tickets_ticket_priority.3', 'High'), + (0, 'nf_tickets_status.1', 'Awaiting Support'), + (0, 'nf_tickets_status.2', 'In Progress'), + (0, 'nf_tickets_status.3', 'Closed'), + (0, 'nf_tickets_priority.1', 'Low'), + (0, 'nf_tickets_priority.2', 'Normal'), + (0, 'nf_tickets_priority.3', 'High'), (0, 'nf_tickets_ticket_prefix.1', 'Urgent'), - (0, 'nf_tickets_ticket_status_no_id', 'edge: no trailing id'); + (0, 'nf_tickets_status_no_id', 'edge: no trailing id'); -- Tickets span categories 1/2/3, statuses 1/2/3, all three states, and -- include one deleted (hidden) ticket. last_modified_date descends diff --git a/testdb/testdb_test.go b/testdb/testdb_test.go index f67f94f..7bc76e4 100644 --- a/testdb/testdb_test.go +++ b/testdb/testdb_test.go @@ -394,7 +394,10 @@ func TestFixtures_TicketRelationsResolve(t *testing.T) { db, _ := testdb.Open(t) for _, q := range []struct{ label, sql string }{ - {"status phrase", `SELECT COUNT(*) FROM xf_nf_tickets_ticket tk LEFT JOIN xf_phrase ph ON ph.title = CONCAT('nf_tickets_ticket_status.', tk.status_id) WHERE ph.phrase_id IS NULL`}, + // Status phrases carry NO `ticket_` infix in real add-on data + // (nf_tickets_status.) — only prefix does. Guarding the + // corrected join keeps the fixtures honest to production naming. + {"status phrase", `SELECT COUNT(*) FROM xf_nf_tickets_ticket tk LEFT JOIN xf_phrase ph ON ph.title = CONCAT('nf_tickets_status.', tk.status_id) WHERE ph.phrase_id IS NULL`}, {"category row", `SELECT COUNT(*) FROM xf_nf_tickets_ticket tk LEFT JOIN xf_nf_tickets_category c ON c.ticket_category_id = tk.ticket_category_id WHERE c.ticket_category_id IS NULL`}, } { var n int @@ -411,7 +414,10 @@ func TestFixtures_TicketRelationsResolve(t *testing.T) { {"hidden messages", `SELECT COUNT(*) FROM xf_nf_tickets_message WHERE message_state <> 'visible'`}, {"participants", `SELECT COUNT(*) FROM xf_nf_tickets_ticket_participant`}, {"ticket field values", `SELECT COUNT(*) FROM xf_nf_tickets_ticket_field_value`}, - {"priority phrases", `SELECT COUNT(*) FROM xf_phrase WHERE title LIKE 'nf_tickets_ticket_priority.%'`}, + // Priority phrases, like status, carry NO `ticket_` infix + // (nf_tickets_priority.); prefix phrases DO + // (nf_tickets_ticket_prefix.) — the add-on's naming asymmetry. + {"priority phrases", `SELECT COUNT(*) FROM xf_phrase WHERE title LIKE 'nf_tickets_priority.%'`}, {"prefix phrases", `SELECT COUNT(*) FROM xf_phrase WHERE title LIKE 'nf_tickets_ticket_prefix.%'`}, } { var n int