Skip to content

feat(handler): unify ?server= / ?box_id= via resolveActiveBox helper - #134

Merged
sarg3nt merged 2 commits into
mainfrom
fix/issue-112-unify-box-resolver
May 17, 2026
Merged

feat(handler): unify ?server= / ?box_id= via resolveActiveBox helper#134
sarg3nt merged 2 commits into
mainfrom
fix/issue-112-unify-box-resolver

Conversation

@sarg3nt

@sarg3nt sarg3nt commented May 17, 2026

Copy link
Copy Markdown
Owner

Summary

Three resolution conventions co-existed pre-#112 Phase 4:

  1. ?server=<id> — gear-settings deep links and the OS-Updates page.
  2. ?box_id=<id>InjectIntegrationStatus middleware (header-pill URL persistence).
  3. gearbox_active_box cookie — sticky pill selection.

resolveBoxIDFromRequest only knew ?server=, so handlers that followed the middleware convention lost pill-aware routing.

  • resolveBoxIDFromRequest now accepts both ?server= and ?box_id= as synonyms (precedence: server > box_id > cookie > default).
  • New resolveActiveBox(r) returns the full *models.BoxConfig + presence bool so handlers that need an agent URL or API key don't chase a separate getServerConfig call. Folds the "single helper for active box" invariant from the issue proposal into one call site.

Phase 4 of #112. Independent of the other open PRs — branches off main.

Test plan

  • go build ./... and go vet ./... clean
  • go test -count=1 ./internal/framework/handler/... clean — four new tests (TestResolveBoxIDFromRequest_*, TestResolveActiveBox_ReturnsServerConfig)
  • On a deployment, switch the active box via the header pill (writes the cookie), then navigate to a deep-link with neither ?server= nor ?box_id= (e.g. /gears) — confirm the cookie's box is honored.
  • Hit a deep-link with ?box_id=other-box — confirm it overrides the cookie just like ?server= does.

🤖 Generated with Claude Code

…112)

Three resolution conventions co-existed pre-#112 Phase 4:

  1. `?server=<id>` — gear-settings deep links and the OS-Updates page.
  2. `?box_id=<id>` — InjectIntegrationStatus middleware (the
     header-pill URL persistence).
  3. `gearbox_active_box` cookie — the sticky pill selection.

Handlers that wanted to honor the pill had to write the same
"check ?server=, then cookie, then default" cascade. Handlers that
followed the middleware convention got `?box_id=` for free but lost
gear-settings links. The resolveBoxIDFromRequest helper only knew
`?server=`.

Changes:

  - resolveBoxIDFromRequest now accepts BOTH `?server=` and `?box_id=`
    as synonyms (precedence: server > box_id > cookie > default).
    Handlers can use whichever name fits the surrounding code without
    losing pill-aware routing.
  - New resolveActiveBox(r) returns the full *models.BoxConfig along
    with a presence boolean, so handlers that need an agent URL or
    APIKey don't have to chase a separate getServerConfig call. Folds
    the "single helper for active box" invariant from the issue
    proposal into one call site.

Four tests cover the new contract: ?server= path, ?box_id= path,
precedence when both supplied, and the full-BoxConfig return.

Phase 4 of #112.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings May 17, 2026 16:59

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR standardizes “active box” resolution across dashboard handlers by treating ?server= and ?box_id= as synonyms (with defined precedence) and introduces a resolveActiveBox(*http.Request) helper to return the resolved *models.BoxConfig directly. This supports the broader capability-driven rendering work by making box selection consistent between deep links, middleware-persisted selection, and the active-box cookie.

Changes:

  • Extend resolveBoxIDFromRequest to accept both ?server= and ?box_id= with precedence server > box_id > cookie > default.
  • Add resolveActiveBox(r) helper to return the resolved *models.BoxConfig plus a presence boolean.
  • Add unit tests covering the new query-param alias behavior and the new helper’s happy path.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
gearbox/internal/framework/handler/handler.go Accepts box_id as an alias for server in request resolution and adds the resolveActiveBox helper.
gearbox/internal/framework/handler/resolve_box_test.go Adds unit tests for resolver precedence and resolveActiveBox returning the expected BoxConfig.

Comment thread gearbox/internal/framework/handler/handler.go
)

Copilot flagged that the docstring for resolveActiveBox claimed
(nil, false) for the "all-boxes / no-box context", but
resolveBoxIDFromRequest falls back to getDefaultServerID() and
getServerConfig accepts entries from the static h.servers list
regardless of DB-enabled state — so any time at least one server
is configured, this helper resolves to one.

Update the docstring to match actual behavior: (nil, false) only
fires when no servers are configured at all OR when an explicit
?server= / ?box_id= references a missing box. Handlers that need
to distinguish "no active box" from "first enabled box" should
consult the auth-context active-box set by InjectIntegrationStatus,
not call this helper.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@sarg3nt
sarg3nt merged commit 79f7bb9 into main May 17, 2026
22 checks passed
@sarg3nt
sarg3nt deleted the fix/issue-112-unify-box-resolver branch May 17, 2026 18:47
sarg3nt added a commit that referenced this pull request May 17, 2026
Two changes required by main moving forward (PRs #127, #134, #137):

1. internal/api/server_test.go was added in PR #127 (remote console)
   after Phase 1 branched. It uses the old ServerConfig.APIKey field
   that Phase 1 replaced with KeyRing. Updated the test to construct
   a one-entry KeyRing and send the legacy 64-hex bearer token.

2. PR #127 also added migration 000002_add_box_console_enabled,
   colliding with Phase 1's 000002_add_box_agent_keys. Renumbered
   Phase 1's migration to 000003. Migrations are content-addressed
   by the embedded iofs, so the rename is mechanical — no schema
   change.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
sarg3nt added a commit that referenced this pull request May 17, 2026
* feat(rotation): Phase 1 multi-key keyring plumbing (#72)

Foundation for issue #72's rotation work. Adds the data structures and
storage required for N-entry keyrings on both the agent and dashboard
sides, with no operator-visible behaviour change yet — rotation
endpoints and UI follow in Phase 2.

Agent side
----------

- `internal/framework/crypto/keyring.go` — `KeyRing` type with up to
  `MaxKeyRingEntries = 4` accepted keys, atomic tmpfile+rename on disk,
  AES-256-GCM (GBE1) encryption when `GEARBOX_AGENT_ENCRYPTION_KEY` is
  set. Wire token format: `gbx_<6-hex-kid>_<base64url(32 random bytes)>`,
  with legacy 64-hex tokens still accepted for one release cycle.
- `LoadOrCreateKeyRing(keyringPath, legacyAPIKeyPath)` migrates an
  existing `/var/lib/gearbox-agent/api-key` file into a single keyring
  entry tagged `kid="legacy"`, role=primary. Legacy file stays on disk
  as a read-only fallback.
- `KeyRingPointer` wraps `atomic.Pointer[KeyRing]` so Phase 2's
  install/use/remove endpoints can swap the live keyring without
  middleware restart. Verified by the new auth-middleware test
  `TestAPIKeyAuth_HotSwapVisibleImmediately`.
- `internal/framework/middleware/auth.go` rewritten to take a keyring
  pointer instead of a static key. Accepts both prefixed and legacy
  token formats; matched `kid` echoed back as `X-Gearbox-Kid:` header
  on every authenticated response so the dashboard can detect drift
  (consumed in Phase 5). Auth with a secondary key logs at INFO so
  the audit log can later flag "old key still in use after rotation".
- New endpoint `GET /api/v1/system/keyring` (authenticated) returns
  metadata only — kids, roles, created_at, sha256-prefix fingerprint
  for diagnostic equality checks — never the secret bytes themselves.
- `--show-api-key` and `--rotate-api-key` CLI flags work against the
  keyring; the printed key uses the new `gbx_<kid>_<b64>` wire format
  the dashboard can paste verbatim.
- `GEARBOX_AGENT_KEYRING_PATH` env var (default
  `<DataDir>/keyring.json`) is now a config field alongside the legacy
  `HAPROXY_AGENT_API_KEY_PATH`.

Dashboard side
--------------

- Migration `000002_add_box_agent_keys` adds the
  `(box_id, kid)`-keyed `box_agent_keys` table and idempotently
  backfills one `kid='legacy'` row per existing box from
  `boxes.api_key_encrypted`. The legacy column stays for one release.
- `database/box_agent_keys.go` exposes Get/Insert/SetPrimary/Delete/
  TouchLastUsed — the storage primitives Phase 2's rotator service
  composes into the install→use→remove dance.

Tests
-----

- 19 keyring unit tests covering token parsing (prefixed + legacy +
  malformed), keyring mutation, file round-trip with and without
  encryption, legacy api-key migration, and pointer hot-swap.
- 8 auth-middleware integration tests covering bearer parsing, kid
  header echo, secondary-key acceptance, and the live hot-swap path
  Phase 2 depends on.
- 5 storage tests covering primary-key lookup, atomic role flip,
  delete-refuses-last guard, and last_used_at touch.

Carry-overs to Phase 2 (intentional gaps surfaced from this PR)
---------------------------------------------------------------

- `DeleteBox` does not yet cascade to `box_agent_keys` (SQLite
  `PRAGMA foreign_keys` is off in this codebase; enabling it is a
  broader change). Phase 2's box-delete path will clean dependent
  rows explicitly. Documented in box_agent_keys_test.go.
- The dashboard's `agent.Client` does not yet send `X-Gearbox-Kid`
  on outbound requests — there's no kid to send while every box's
  keyring contains only the legacy entry. Phase 2 wires this when
  the rotator starts mutating keyrings.

Refs: research summary and implementation plan posted to #72.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* chore(rotation): address Copilot review on PR #128

Nine findings from the Copilot review on PR #128, all valid or
worth addressing. Fixed in this commit; replies + thread-resolves
go with the push.

1. LoadOrCreateKeyRing fall-through (keyring.go:131-167)
   Was: any error reading the legacy api-key file (incl. ErrKeyRequired
   from a missing encryption-key env, or a permission error) silently
   fell through to generating a fresh keyring — would rotate every
   dashboard out for a transient operator mistake.
   Now: distinguish "file doesn't exist" (proceed to fresh-gen) from
   "file exists but errored / malformed" (return the error to the
   caller). os.Stat + os.IsNotExist gates the choice explicitly.

2. MatchToken constant-time guarantee (keyring.go ~195)
   Was: the prefixed-token path returned early on the first kid match,
   making total runtime depend on which kid the request claimed — kid
   enumeration via timing. The doc said "All comparisons are
   constant-time" but the prefixed branch broke that promise.
   Now: walk every entry, compare both kid and secret with
   subtle.ConstantTimeCompare, AND the two results. Match is recorded
   without short-circuit; runtime is uniform regardless of which kid
   (if any) matches. Doc updated to reflect the actual guarantee.

3. writeKeyRingFile mutates input (keyring.go ~415)
   Was: the function populated SecretHex on each entry of the passed-
   in keyring before marshaling. KeyRing values are shared via
   atomic.Pointer and treated as immutable; mutating in-place risks
   races with concurrent middleware readers.
   Now: marshal off a local snapshot whose entries have SecretHex
   backfilled from Secret where needed. Input is never written to.

4. --rotate-api-key zero CreatedAt (main.go ~155)
   Was: the fresh KeyRingEntry built for the CLI rotate command
   omitted CreatedAt, so the keyring file got 0001-01-01T00:00:00Z
   and the /api/v1/system/keyring metadata exposed the same.
   Now: CreatedAt: time.Now().UTC().

5. handleGet nil-guard (api/keyring.go ~50)
   Was: h.keyring.Load() was dereferenced unconditionally; a future
   wiring bug that left the pointer nil would panic the agent on
   every keyring request.
   Now: nil check + 500 + log line. Fails loud rather than crashing.

6. At-most-one-primary-per-box constraint (migration 000002)
   Was: nothing in the schema stopped two rows with role='primary'
   for the same box. SetBoxPrimaryKey's transactional flip is
   correct, but a buggy code path or a manual DB edit could produce
   the invalid state and GetBoxPrimaryKey would return an arbitrary
   row.
   Now: partial unique index on box_agent_keys(box_id) WHERE
   role='primary'. SQLite supports this directly; index is dropped
   in the down migration too.

7. Test naming clarity (box_agent_keys_test.go)
   Was: TestBoxAgentKeys_MigrationBackfillsLegacyEntry was named as
   if it validated migration behaviour but actually only exercised
   InsertBoxAgentKey + GetBoxPrimaryKey roundtrip; the comment also
   misled.
   Now: split into two clearly-named tests —
   InsertAndLookup covers the roundtrip, and a new
   MigrationBackfillStatementWorks test wipes the migrated rows for
   a single box, re-executes the migration's INSERT-FROM-boxes
   statement, and asserts the row appears + reruns are idempotent.

8. DeleteBox cascade gap (servers.go DeleteBox)
   Was: the schema declared ON DELETE CASCADE but PRAGMA
   foreign_keys is off in this codebase, so deleting a box left
   orphaned box_agent_keys rows holding encrypted secrets. Phase 1
   docs flagged this as a deferred gap; Copilot pushed back, and
   fairly — it's a small, contained fix.
   Now: DeleteBox runs inside a transaction that wipes
   box_agent_keys WHERE box_id = ? before deleting from boxes. Both
   succeed or neither does.
   Test re-added: TestBoxAgentKeys_DeleteBoxClearsDependentKeys.

9. APIKeyAuth nil-guard (middleware/auth.go)
   Was: keyring.Load() was called without first checking the
   pointer itself for nil. A miswired ServerConfig would panic on
   every authenticated request.
   Now: fail-closed nil check at the top of the request handler —
   returns 401 + logs at error level. Same defensive treatment as
   fix #5.

Tests
-----

All 3 dashboard-side suites pass (`database` package, 7 new tests
including the new DeleteBox cascade test). All 3 agent-side suites
pass (`crypto`, `middleware`, `api`).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* docs(rotation): clarify constant-time precondition on MatchToken

Add a note that subtle.ConstantTimeCompare's length-dependent
fast-fail is fine here because every kid in the system is exactly 6
chars long (kidLength = 6 hex chars; the legacy entry uses 'legacy'
which is also 6 chars by deliberate convention). Custom kids of a
different length would naturally hash-mismatch — which is the
intended failure mode.

Also serves to force a synchronize event so PR #128's CI re-runs
on the fix commit; the prior synchronize from fe3c762 didn't
trigger workflows (still unclear why; not blocking the work).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(rotation): adapt phase-1 to main after rebase

Two changes required by main moving forward (PRs #127, #134, #137):

1. internal/api/server_test.go was added in PR #127 (remote console)
   after Phase 1 branched. It uses the old ServerConfig.APIKey field
   that Phase 1 replaced with KeyRing. Updated the test to construct
   a one-entry KeyRing and send the legacy 64-hex bearer token.

2. PR #127 also added migration 000002_add_box_console_enabled,
   colliding with Phase 1's 000002_add_box_agent_keys. Renumbered
   Phase 1's migration to 000003. Migrations are content-addressed
   by the embedded iofs, so the rename is mechanical — no schema
   change.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants