fix(pipes): per-pipe AllowedRoles fails open when context role is empty #159

Problem

PipesHandler.Execute in internal/api/pipes.go:86-102 enforces the per-pipe AllowedRoles allowlist only when the request context already carries a non-empty role. If the role is empty, the entire AllowedRoles check is skipped and the request is served.

// internal/api/pipes.go:86-102
if len(q.AllowedRoles) > 0 {
    role := RoleFromContext(r.Context())
    if role != "" {                       // <-- guard
        allowed := false
        for _, ar := range q.AllowedRoles {
            if ar == role || ar == "*" {
                allowed = true
                break
            }
        }
        if !allowed {
            writeJSONError(w, http.StatusForbidden, "forbidden")
            return
        }
    }
    // empty role with AllowedRoles set → falls through and serves the pipe
}

This is a fail-open per-pipe authorization bug. A pipe configured with AllowedRoles: ["admin"] is reachable by any request whose context lacks a role under any of these conditions:

• auth.enabled: false (no JWTAuthMiddleware populates the role at all).
• auth.enabled: true but the JWT lacks the configured auth.role_claim (or the claim path doesn't resolve). extractClaim returns "", ContextKeyRole is set to "", the gate falls through.
• auth.dev_mode: true — JWTAuthMiddleware sets role to "admin" for all requests, so this configuration happens to match AllowedRoles: ["admin"] and is not a vector. But any non-admin allowlist (e.g. ["service"], ["analyst"]) is bypassed.

Pipe execute routes (GET/POST /v1/pipes/{name}) sit outside the /v1/admin/* RequireRole(authEnabled, "admin", "service") block (internal/api/router.go:115-119 vs :126-158). Per-pipe AllowedRoles is the only authorization gate on the execute path — there is no outer middleware that catches the empty-role case for these routes.

Why this wasn't caught by the existing tests

The existing pipe handler tests cover three role scenarios (internal/api/pipes_test.go):

The missing row is the one that exposes the bug:

| (missing) | ["admin"] | "" (or absent) | should be 403 | currently serves the pipe |

Every existing test sets ContextKeyRole to a non-empty string, so the if role != "" branch is always taken; the implicit else branch (the actual production code path when the JWT lacks a role) has no test coverage at all. The NoRolesAllowsAll test name is misleading — it tests the case where the pipe's allowed roles is nil, not where the request's role is empty.

This is the actionable framing of the test-coverage failure: every branch of an authorization check needs an explicit negative test, and the (restricted pipe, empty role) tuple was the one nobody wrote. The bug is one line of code; the test gap is the systemic problem.

Proposed implementation order (tests-first)

The intent is for the failing test to land first so the bug is demonstrably present in the test record, then the fix flips it green.

Step 1 — add the failing test (red)

Add to internal/api/pipes_test.go:

func TestPipesHandler_Execute_RestrictedPipe_EmptyRoleDenied(t *testing.T) {
    t.Parallel()
    store := pipes.NewMemoryStore(
        &pipes.NamedQuery{
            Name:         "admin_report",
            SQL:          "SELECT * FROM clicks",
            AllowedRoles: []string{"admin"},
        },
    )
    h := NewPipesHandler(store, nil, nil, 0)

    w := httptest.NewRecorder()
    // No ContextKeyRole set — simulates auth-disabled or JWT-without-role-claim.
    r := pipesRequest(t, http.MethodPost, "/v1/pipes/admin_report/execute", "admin_report", nil)

    h.Execute(w, r)

    assert.Equal(t, http.StatusForbidden, w.Code,
        "pipe restricted to %v must reject requests with no role in context", []string{"admin"})
    assertJSONErrorResponse(t, w)
}

This should fail against main: the response will not be 403 (it'll reach executeQuery and 500 on the nil ClickHouse connection, or some other non-403 status).

While there, convert the four existing role tests into a single table-driven matrix that makes the gap structurally hard to recreate:

func TestPipesHandler_Execute_RoleAuthorization(t *testing.T) {
    t.Parallel()
    cases := []struct {
        name           string
        allowedRoles   []string
        contextRole    string // "" means no role set in context
        setRole        bool   // distinguishes "no role" from "empty role"
        wantForbidden  bool
    }{
        {"open pipe, no role", nil, "", false, false},
        {"open pipe, empty role", nil, "", true, false},
        {"open pipe, any role", nil, "anyone", true, false},
        {"restricted pipe, matching role", []string{"admin"}, "admin", true, false},
        {"restricted pipe, non-matching role", []string{"admin"}, "viewer", true, true},
        {"restricted pipe, NO role in context", []string{"admin"}, "", false, true},  // <-- the bug
        {"restricted pipe, EMPTY role in context", []string{"admin"}, "", true, true}, // <-- the bug
        {"wildcard, any role", []string{"*"}, "viewer", true, false},
        {"wildcard, no role", []string{"*"}, "", false, false}, // wildcard intentionally permissive
        {"multi-role allowlist, matching", []string{"admin", "editor"}, "editor", true, false},
        {"multi-role allowlist, non-matching", []string{"admin", "editor"}, "viewer", true, true},
    }
    for _, tc := range cases {
        t.Run(tc.name, func(t *testing.T) {
            // ...
        })
    }
}

The two // <-- the bug rows are the new coverage; the rest preserve the existing assertions. Decide explicitly whether wildcard, no role is intended to permit (current behavior) or deny — this issue does not pre-judge it, but the test should pin whichever is chosen.

Step 2 — fix the handler (green)

Close the implicit-else branch in internal/api/pipes.go:86-102. The minimal fix is to drop the if role != "" guard so an empty role is treated as "no role matches, deny":

if len(q.AllowedRoles) > 0 {
    role := RoleFromContext(r.Context())
    allowed := false
    for _, ar := range q.AllowedRoles {
        if ar == role || ar == "*" {
            allowed = true
            break
        }
    }
    if !allowed {
        writeJSONError(w, http.StatusForbidden, "forbidden")
        return
    }
}

Note that ar == role with role = "" will not match any non-empty role string, so an empty allowlist entry (which shouldn't exist but Put doesn't validate against it today) would still match an empty role. Either validate AllowedRoles entries on Put to reject empty strings, or skip empty entries during comparison. Implementation choice — call it out in the PR.

Status code choice: RequireRole returns 401 for empty-role-with-auth-enabled and 403 for role-mismatch. The per-pipe gate has no access to authEnabled, so 403 (forbidden) is the consistent answer here — the auth layer's job is to put a role in context; if there isn't one, the gate just sees "not in allowlist." If a stronger 401 distinction is wanted, that requires plumbing authEnabled into PipesHandler.

Step 3 — audit for the same pattern elsewhere

This bug is shaped like a general anti-pattern: "if a sensitive value is missing, skip the check." Grep the codebase for the same shape and add tests where they're missing:

• grep -rn 'if role != ""' internal/ — known instance: pipes.go:88. Verify others.
• grep -rn 'if .* != "" {' internal/api/ for any handler doing security-relevant string comparison gated on non-empty.
• Cross-check internal/policy/ evaluation against empty role/empty JWT claims.

Document any others found as separate issues or in this PR.

Step 4 — test-framework prevention

The systemic gap is that role-gating code has no enforced minimum coverage shape. Two complementary changes:

1. Shared auth-matrix helper in internal/testutil/. Add testutil.RunRoleMatrix(t, handler, cases) (or similar) that runs a standard (allowed_roles, observed_role) → expected matrix. Adopt it in pipes_test.go and any other per-resource role-gated handler. Goal: writing a handler that takes AllowedRoles without invoking the matrix should look obviously incomplete in review.
2. Coverage gate on internal/api/pipes.go (and other auth-touching files). The current .testcoverage.yml enforces 70% on unit. Pipes-specific minimum should be higher (90%+) — the file is small enough that 70% can miss a whole branch like this one. Alternative or addition: a per-package gate for internal/api/ that requires every branch in handlers tagged with role logic to be covered. go-test-coverage supports per-file thresholds.

Optional: explore mutation testing (e.g. go-mutesting) on internal/api/pipes.go — the if role != "" guard is the textbook case a mutation tester catches (negate the condition → tests should fail; here, they wouldn't).

Step 5 — docs

• CHANGELOG.md under [Unreleased] — security fix entry.
• docs/api.md — if pipe AllowedRoles is documented, note the corrected fail-closed semantics.
• AGENTS.md — if design decision Implement asynchronous metrics scrapers for embedded NATS and Pebble #11 references pipe-level role enforcement, ensure the description matches the fixed behavior.

Security impact

• Severity: medium-to-high depending on deployment. Most exposed configuration is auth.enabled: true with a JWT issuer that doesn't populate auth.role_claim — pipes intended for admin-only access are reachable by any authenticated principal.
• Not exploitable: dev_mode (role is forced to "admin", only matches ["admin"] allowlists).
• Trivially exploitable: auth.enabled: false with AllowedRoles configured — every pipe is open. This is documented as "no auth, no enforcement" so it's a design-consistency call rather than a strict CVE, but the per-pipe allowlist's presence suggests intent that isn't honored.

No public disclosure recommended ahead of fix since the project is pre-1.0 alpha; flag in CHANGELOG once merged.

Out of scope

• Building a route-level RequireRoles for pipe execute path. The model is per-pipe AllowedRoles, and feat(auth): RequireRoles middleware — fail closed, no permissive fallback #145 explicitly leaves reads/executes on that model. Don't expand scope to add an outer middleware unless that design decision is revisited separately.
• Renaming/consolidating RequireRole (singular, exists in router.go:209) with the proposed RequireRoles in feat(auth): RequireRoles middleware — fail closed, no permissive fallback #145. Separate concern.

Related

• Native SQL Templating #32 — Native SQL Templating. This issue was discovered while reviewing pipes.go for Native SQL Templating #32 and noted as a side observation in the issue body.
• feat(auth): RequireRoles middleware — fail closed, no permissive fallback #145 — RequireRoles middleware. Adjacent but does not cover this case: feat(auth): RequireRoles middleware — fail closed, no permissive fallback #145's "reads can stay open or use the named-pipe's own role gate" rests on the assumption that the per-pipe gate works; this issue is that it doesn't.
• internal/api/router.go:209 — existing RequireRole middleware, which fails closed correctly for the routes it protects.
• internal/api/middleware.go:122-126 — where ContextKeyRole is populated (or left empty) by JWTAuthMiddleware.
• internal/api/middleware.go:46-61 — auth-disabled and dev-mode short-circuits (relevant for understanding when role is empty vs "admin").
• internal/testutil/jwt.go — MakeJWT helper, useful for end-to-end auth tests if a router-level integration test is added.