Expose UpstreamFilter through authserver.New facade

[tgrunnagle]

Summary

WithUpstreamFilter (added in #5725) let a caller narrow the upstream authorization chain, but it was only reachable through the low-level handlers.NewHandler constructor. The public pkg/authserver.New facade and its Config had no way to inject an UpstreamFilter, so any caller embedding the auth server through the facade could not install a filter at all.

This adds an UpstreamFilter field to authserver.Config and threads it into the handler options assembled in newServer, mirroring how the existing upstream-token refresher is already wired. A nil filter (the default) preserves current behavior of walking every configured upstream, so the change is fully backward-compatible.

Closes #5732

Type of change

• New feature

Test plan

• Unit tests (task test)
• Linting (task lint-fix)

pkg/authserver/integration_test.go gains TestIntegration_MultiUpstreamChain_ConfigUpstreamFilter, which drives a two-upstream chain end-to-end through authserver.New with a Config.UpstreamFilter that drops the second provider, and asserts the handler redirects straight to the client after the first callback instead of continuing on to provider-2. The pre-existing TestIntegration_MultiUpstreamSequentialChain is unmodified and now implicitly covers the nil-filter (no-behavior-change) path through the same facade wiring. The full pkg/authserver suite passes, and gofmt/lint are clean on all three changed files.

Does this introduce a user-facing change?

No end-user or CLI-facing change. It adds a new optional field, Config.UpstreamFilter, to the public pkg/authserver.Config struct for Go callers that embed the auth server; existing callers are unaffected since a nil filter preserves today's behavior.

Special notes for reviewers

• Follow-up to Filter upstream auth chain via callback hook #5725, which introduced handlers.UpstreamFilter / WithUpstreamFilter at the handler level but left them unreachable from the authserver.New facade.
• server_impl.go gains a small buildHandlerOptions helper so the handlers.Option slice is assembled once, appending the filter option only when cfg.UpstreamFilter is non-nil.
• The new integration test lives in integration_test.go rather than next to handler_chain_test.go as the issue suggested, since it specifically exercises the Config → authserver.New → handlers.NewHandler wiring; the lower-level chain-narrowing logic (computeChain) is already unit-tested in handler_chain_test.go.
• No in-repo caller currently sets Config.UpstreamFilter — wiring an actual filter policy for a real caller is out of scope for this issue and left as follow-up work.

Generated with Claude Code

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 70.74%. Comparing base (609c0ad) to head (3451b89).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5733      +/-   ##
==========================================
+ Coverage   70.68%   70.74%   +0.05%     
==========================================
  Files         682      682              
  Lines       68949    68958       +9     
==========================================
+ Hits        48736    48783      +47     
+ Misses      16665    16619      -46     
- Partials     3548     3556       +8     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[tgrunnagle]

Multi-Agent Consensus Review

Agents consulted: architecture-api-design, security, test-coverage, general-code-quality

Consensus Summary

#	Finding	Consensus	Severity	Action
1	Config.Validate() doesn't guard against UpstreamFilter set with fewer than 2 upstreams	9/10	MEDIUM	Fix
2	withUpstreamFilter test option is silently ignored by 4 of 5 setup helpers	8/10	MEDIUM	Fix
3	Typed-nil interface value can defeat the nil-filter fast path	7/10	LOW	Fix
4	New test duplicates leg-1 HTTP flow boilerplate	7/10	LOW	Fix
5	Doc comment overstates a "second failure signal" that can't occur	7/10	LOW	Fix

Overall

This threads the UpstreamFilter hook added in #5725 through the public authserver.Config/authserver.New facade, mirroring the existing UpstreamTokenRefresher wiring pattern exactly. The approach is sound: buildHandlerOptions preserves nil-filter backward compatibility byte-for-byte, there's no import cycle, and the new integration test proves the wiring with two independent signals (an HTTP status code and a storage-level assertion) rather than a shallow check.

The findings below are validation-gap and test-hygiene polish, not correctness defects. The one worth prioritizing is the Config.Validate() gap: a caller can set UpstreamFilter with a single configured upstream and get a silent no-op instead of a startup error, which cuts against this repo's own "fail loudly on invalid input" convention. The test-option no-op (a withUpstreamFilter option only 1 of 5 setup helpers actually honors) is a real footgun for future test authors, even though it doesn't touch production code. The rest are comment-accuracy and DRY nits in the new test.

Nothing here should block merge.

---

Generated with Claude Code

[JAORMX]

Panel review — PR #5733: Expose UpstreamFilter through authserver.New facade

Fixed point: origin/main
Diff: 4 files, +178 / −11, 3 commits
Author: @tgrunnagle
Closes: #5732

---

Spec — does this implement what was asked?

Source: #5732 "Expose UpstreamFilter through the pkg.authserver.New facade (Config passthrough)"

• [Implemented ✓] Add Config.UpstreamFilter field (config.go:612) and thread it through newServer only when non-nil (server_impl.go:209-210, 231-240). Matches the issue's proposed change verbatim.
• [Implemented ✓] Nil filter preserves prior behavior (walk all upstreams) — verified at computeChain (handler.go:275-277): if h.filter == nil { return append(chain, restNames...) }. Backward-compatible.
• [Implemented ✓] Import direction clean: pkg/authserver → pkg/authserver/server/handlers, no cycle.
• [Deviation, justified] Issue asked for tests "next to handler_chain_test.go"; PR places the new test in integration_test.go instead. The PR description justifies this: the new test specifically exercises Config → authserver.New → handlers.NewHandler wiring, while handler_chain_test.go already covers the lower-level computeChain narrowing. This is the right call — the test belongs with the wiring it proves. Not a finding.
• [Scope creep — acceptable] validateUpstreamFilter (config.go:859-868) rejecting UpstreamFilter != nil && len(Upstreams) < 2 is not in the issue. It is a strict improvement (fail-loudly over silent no-op) and mirrors NewHandler's "catch misconfiguration at construction" philosophy. Reasonable to keep.

Spec axis: no findings. All issue requirements met; the two deviations are improvements with clear rationale.

---

Standards — does this follow project conventions?

Sources read: CLAUDE.md, CONTRIBUTING.md, .claude/rules/{go-style,testing,security,pr-creation,cli-commands}.md
Tooling-skipped: golangci-lint, gofmt, go vet (run on every commit)

Hard violations

• [Violation] Two commit subjects exceed the 50-char limit (CONTRIBUTING.md:161, reaffirmed in CLAUDE.md "Commit Guidelines"):
  • Clean up UpstreamFilter test hygiene in integration tests — 57 chars
  • Expose UpstreamFilter through authserver.New facade — 51 chars
  • (Guard UpstreamFilter misuse in authserver.Config — 48, OK)
• [Violation] None of the three commits carry a Signed-off-by trailer. CONTRIBUTING.md:91-93 requires it on every commit for DCO attestation.

Judgement calls

• [Judgement] stubChainFilter (integration_test.go:1979-1990) is a hand-written interface double. .claude/rules/testing.md:21 says "Use go.uber.org/mock (gomock) — never hand-write mocks." It's technically a stub (fixed return, no expectations), so defensible, but the rule's spirit leans gomock. Maintainer call.
• [Judgement] assert.ErrorIs at integration_test.go:2065 where testing.md:27 prefers require.*. Defensible here (continues after the prior require.Error), but leans against convention.

Standards axis: 2 hard violations (commit subject length ×2, missing Signed-off-by ×3 commits), 2 judgement calls.

---

Domain — what do the specialist reviewers say?

Panel: oauth-expert, secure-code-reviewer, devex-reviewer

Ship-blockers

(none)

Cross-confirmed (1)

• [Low] Typed-nil UpstreamFilter panics at request time, not boot time — pkg/authserver/config.go:612, pkg/authserver/server_impl.go:237-238, pkg/authserver/server/handlers/handler.go:279
  Sources: oauth-expert, secure-code-reviewer (CWE-476 / OWASP A05:2021)

  A caller writing var f *myFilter; cfg.UpstreamFilter = f stores a typed-nil interface, which is != nil. The if filter != nil checks in validateUpstreamFilter (config.go:864) and buildHandlerOptions (server_impl.go:238) both pass, so the nil concrete pointer reaches computeChain, which calls h.filter.FilterUpstreams(...) → nil-deref panic in the /oauth/callback handler.

  Impact: Availability, not authz-bypass. net/http's default per-request recovery catches the panic → HTTP 500 + stack trace in logs, no token issued (fail-closed by accident). The authserver has no panic-recovery middleware. Trigger requires trusted operator mis-wiring, not attacker input.

  The PR's own doc comment (config.go:608-610) warns against the pattern — so this is an accepted, documented trade-off, not a regression. Both reviewers independently note the mitigation is doc-only.

  Optional hardening (strictly better — converts request-time panic to boot-time config error, matching NewHandler's philosophy at handler.go:142-162):

  func (c *Config) validateUpstreamFilter() error {
      if c.UpstreamFilter != nil {
          v := reflect.ValueOf(c.UpstreamFilter)
          if v.Kind() == reflect.Ptr && v.IsNil() {
              return fmt.Errorf("upstream_filter: assign nil itself, not a nil-valued concrete pointer")
          }
          if len(c.Upstreams) < 2 {
              return fmt.Errorf("upstream_filter is configured but has no effect with fewer than 2 upstreams")
          }
      }
      return nil
  }

Mechanical fixes

(none beyond the commit-subject/Signed-off-by Standards fixes)

Judgement calls

• [Low] stubChainFilter hand-written vs gomock — integration_test.go:1979. See Standards axis. Maintainer preference.
• [Low] assert.ErrorIs vs require.ErrorIs — integration_test.go:2065. See Standards axis.

Polish

• [Info] Field name UpstreamFilter shadows the handlers.UpstreamFilter type name. Mirroring the type name at the field site is idiomatic Go (cf. http.Server.Server, time.Ticker.Ticker) and the doc comment resolves any ambiguity. No change needed.

Gaps

• devex-reviewer produced a truncated response; its findings (field doc accuracy, backward-compat, naming, discoverability, error-message quality) are subsumed by the oauth-expert and secure-code-reviewer coverage of the same surface. No gap in coverage.

---

Summary

• Spec axis: no findings — all #5732 requirements met; deviations are improvements.
• Standards axis: 2 hard violations (commit subject length ×2, missing Signed-off-by ×3), 2 judgement calls.
• Domain axis: 1 cross-confirmed Low finding (typed-nil hazard, doc-only mitigation), 2 Low judgement calls, 1 Info polish. No ship-blockers.

Most important single issue: The typed-nil hazard (Domain, Low) is the only substantive code-level finding — and it is explicitly documented as an accepted trade-off. The Standards violations (commit subject length + missing DCO) are the ones that will block CI/DCO checks if the repo enforces them.

Each axis is orthogonal — Spec, Standards, and Domain findings don't mask each other. Verify each axis independently before shipping.

---

Review generated by the panel-review skill (Spec + Standards + Domain axes, fanned out in parallel).

[JAORMX]

Approving on code consideration. The panel review found no ship-blockers:

• Spec: all #5732 requirements met; backward-compatible (nil filter walks all upstreams, identical to pre-PR behavior).
• Domain: fail-closed-on-error contract preserved through the facade wiring (computeChain returns the filter error, never falls back to walk-all). The typed-nil hazard is explicitly documented as an accepted trade-off and is not attacker-controllable (operator-set field).

The two procedural findings from the panel stand as author follow-ups, not merge blockers:

• Two commit subjects exceed 50 chars (Clean up UpstreamFilter test hygiene in integration tests — 57; Expose UpstreamFilter through authserver.New facade — 51).
• Missing Signed-off-by trailer on all 3 commits (CONTRIBUTING.md:91-93 / DCO).

These will need addressing if the repo's DCO check is enforced, but they don't affect the code's correctness.