Land wave: tickets phrase names (#189), 401 client IP (#190), OpenAPI 400s/uint32 bounds (#191)#194
Merged
Merged
Conversation
The two 401 warn lines in rest.AuthMiddleware formatted r.RemoteAddr, which behind the prod reverse proxy (Cloudflare -> nginx-proxy-manager) is always NPM's address — the client IP those lines exist to record was lost. Resolve the real client IP from forwarding headers, gated on a configured trusted-proxy set (ADR 0005). - rest/clientip.go: resolveClientIP (pure, table-tested) implements the full rule — peer untrusted -> bare peer IP; peer trusted -> right-most X-Forwarded-For entry not in the trusted set (walk right->left, skip trusted hops, malformed entry stops the walk and marks XFF unusable), fall back to X-Real-IP, then bare peer IP. InitTrustedProxies parses TRUSTED_PROXIES (comma-separated CIDRs, bare IPs normalized to /32/128) ONCE at startup into a cached []*net.IPNet; clientIP is the thin ambient wrapper the two log sites call. - rest/auth.go: both 401 lines now log "from <client-ip> (peer <RemoteAddr>)", each keeping its distinct prefix. AuthMiddleware signature unchanged (shared by both stacks). LOG-ONLY: no status/body/header change — 401 wire contract (golden-pinned #106) untouched. - servers/server.go: wired rest.InitTrustedProxies() into Start() before the Sentry setup; non-empty-malformed TRUSTED_PROXIES is fatal at boot. #134 must preserve this call when rebuilding the composition root. This was generated by AI
OpenAPI spec-precision fix — moves the document to match frozen implementation behavior; no handler change. - Add `maximum: 4294967295` to every uint32-backed integer request parameter (ticketId path ×2, perPage ×2, modifiedSince, and the five repeated-filter array items). The binder is ParseUint(_, 10, 32), so a value above the ceiling is a 400 "value out of range" — the spec no longer advertises an unbounded range it rejects. - Note malformed-cursor -> 400 on both afterCursor param descriptions. - Record the unknown-query-param leniency (frozen, golden-pinned) in the document description so property-based tooling findings are expected. - New structural guard TestSpec_Uint32ParamsBounded pins the bound on every integer parameter schema (scalar and array items). - Record two error goldens from the current stack pinning the uint32 ceiling to observed bytes: tickets/get_by_id_over_uint32 (path; wired as a must-fail request — exceeds the declared maximum) and tickets/list_modified_since_over_uint32 (query; 400 witnessed). The roster zero/UNSPECIFIED 400, profile-by-id no-selector 400, and ticket-messages cursor 400 were already declared (#127) and golden-witnessed; this change pins the remaining uint32 gap. This was generated by AI
…#190) Two review-requested coverage gaps for the trusted-proxy-gated client-IP resolution (#190, ADR 0005). Tests only — no behavior change. - rest/clientip_test.go: pin that resolveClientIP does NOT strip ports from X-Forwarded-For entries. A port-bearing right-most entry (203.0.113.9:51000) fails net.ParseIP, so it is treated as malformed, stops the right->left walk, and marks XFF unusable -> falls back to X-Real-IP (sibling case with no X-Real-IP falls through to the bare peer IP). A future refactor that starts stripping XFF ports would flip these and surface here as a deliberate change. - servers/server_test.go: source-level convention guard (the established idiom — Error.Fatalf's os.Exit makes the fatal path untestable in-process) asserting Start wires rest.InitTrustedProxies() before any listener/serve handoff and makes its error fatal via Error.Fatalf. Keeps the cross-issue #134 "preserve this call" requirement self-announcing rather than remembered. This was generated by AI
#189) The NF Tickets add-on writes status/priority phrases without the `ticket_` infix (nf_tickets_status.<id> / nf_tickets_priority.<id>); only prefix carries it. The loader queried the infixed form, matching zero rows and silently warming empty maps so every statusName/priorityName resolved to "". Correct the two prefixes (prefix unchanged, asymmetry documented via named consts + CAUTION block), and correct the MariaDB harness fixtures and referential-integrity guards that encoded the same wrong assumption so the harness reproduces the bug red before / green after. This was generated by AI
The snake_case `modified_since` alias is an ignored unknown param to the spec validator, so this golden pins the 400 response, not the request-level maximum. Clarify the note in battery.go and the golden to match; the request-level uint32 bound is pinned structurally by TestSpec_Uint32ParamsBounded. This was generated by AI
This was referenced Jun 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three independent, non-conflicting bug fixes dispatched as a supervised swarm wave — each TDD-driven with its own green CI gate, reviewed via the PR-review toolkit, then integrated and verified together against the MariaDB harness and
-race.#189 — tickets
statusName/priorityNameblank against real dataCorrects the
referencecachephrase-loader prefixes. The NF Tickets add-on writes status/priority phrases without theticket_infix (nf_tickets_status./nf_tickets_priority.); only prefix carries it. The loader queried the infixed form, matched zero rows, and silently warmed empty maps. Fixtures + referential-integrity guards corrected to the real naming so the harness reproduces the bug red→green. No public contract change.#190 — 401 logs report the reverse-proxy IP, not the client
Adds trusted-proxy-gated client-IP resolution (ADR 0005) for the two 401 auth log lines: right→left
X-Forwarded-Forwalk skipping trusted hops,X-Real-IPfallback, bare-peer fallback.TRUSTED_PROXIESis parsed once at startup; non-empty-malformed → fatal. Log-only — the 401 wire contract is untouched. Includes a source-level guard that the composition root wiresInitTrustedProxiesbefore serving.#191 — OpenAPI under-describes real error behavior
Declares the frozen 400s, bounds all 10 uint32-backed integer params with
maximum: 4294967295, notes the malformed-cursor 400 and the unknown-query-param leniency, and records replaying goldens for the new error inputs. Spec-only; no handler change.Closes #189
Closes #190
Closes #191
This was generated by AI