fix: widen server-scope boot check, filter out-of-scope key assignments

serverTouchingRoutes in cmd/main.go filtered on "server"/"console"/an exact
workflows-run match, which is how POST /api/keys/:id/assign reached
production with no scope check and no boot-time signal at all: its path
names neither. Widen the filter to also match ":serverId" and "assign",
and document at the filter why a substring match is the weak part of this
design — a route that acts on a server without saying so in its path stays
invisible to it — noting that inverting the model (every /api route
declares itself, with an exemption list) would be the stronger fix and is
left as a follow-up. Re-running the mechanical check against the widened
filter swept in no route beyond what serverScopedRoutes already declared.

GET /api/keys/:id also leaked out-of-scope hostnames: it returned every
assignment for a key, server attached, unfiltered by the caller's tag
restriction. getKey now drops any assignment whose server fails
services.ServerInTokenScope before returning the list — silently, so the
response carries no count of what was removed — while still returning the
key itself, since a restricted token may legitimately hold a key also
assigned outside its scope. GetAssignmentsWithServers has exactly one
caller (getKey), so the filtering is done in the handler rather than
threaded into the service. Recorded in serverScopedRoutes; its path
matches none of the filter's substrings either, so it is not boot-enforced
and is kept as a considered decision, same as the assign/revoke entries.
This commit is contained in:
2026-09-09 07:51:04 +00:00
parent cbf929fe2d
commit dc6e1b3c29
3 changed files with 61 additions and 13 deletions
+23 -3
View File
@@ -360,9 +360,27 @@ func boolEnv(key string) bool {
}
// serverTouchingRoutes restricts AssertServerScopeMapComplete to routes whose
// pattern names server-derived data — "server", "console" or
// "workflows/:id/run" — rather than every routeScopes entry, so unrelated
// routes are never swept in and boot never fails for no reason.
// pattern names server-derived data — "server", ":serverId", "console" or
// "assign", or the exact "workflows/:id/run" — rather than every routeScopes
// entry, so unrelated routes are never swept in and boot never fails for no
// reason. ":serverId" and "assign" were added after POST /api/keys/:id/assign
// turned out to resolve a named server (in its request body, not its path)
// with no scope check and no entry in serverScopedRoutes at all — the
// original three-way filter never saw it because nothing in its path pattern
// said "server".
//
// A substring filter is the weak part of this design: it only catches a route
// whose *path* names a server, and a route can act on a server named in its
// body, a query parameter, or an ID a handler derives some other way, with a
// path that says nothing about it — as the keys/assign route did. Each time
// that happens the fix is another substring added here, which finds this
// class of gap one instance late rather than by construction. The stronger
// design would invert the model: every /api route declares itself in
// serverScopedRoutes (or an adjacent map), with an explicit exemption list
// for the handful that genuinely act on nothing server-scoped, so a new route
// is checked by default rather than only if its path happens to match a
// pattern someone thought to add. That inversion is a larger change than
// this filter widening and is left as a follow-up, not done here.
func serverTouchingRoutes(r *gin.Engine) []string {
var out []string
for _, route := range r.Routes() {
@@ -370,7 +388,9 @@ func serverTouchingRoutes(r *gin.Engine) []string {
continue
}
if strings.Contains(route.Path, "server") ||
strings.Contains(route.Path, ":serverId") ||
strings.Contains(route.Path, "console") ||
strings.Contains(route.Path, "assign") ||
route.Path == "/api/workflows/:id/run" {
out = append(out, route.Method+" "+route.Path)
}