feat(rbac)!: fail-closed authorization + default_role public access - #172
Conversation
…pped when context role is missing
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a security vulnerability where pipes with defined AllowedRoles were inadvertently accessible to requests lacking a role. By removing the conditional guard that skipped authorization checks for empty roles, the system now correctly evaluates these requests against the allowlist, defaulting to a 403 Forbidden response when no match is found. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
💤 Files with no reviewable changes (1)
📜 Recent review details🧰 Additional context used📓 Path-based instructions (5)**/*.go📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*_test.go📄 CodeRabbit inference engine (AGENTS.md)
Files:
internal/policy/**/*.go📄 CodeRabbit inference engine (AGENTS.md)
Files:
internal/policy/**/*_test.go📄 CodeRabbit inference engine (AGENTS.md)
Files:
internal/config/config.go📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (1)📓 Common learnings🧬 Code graph analysis (2)internal/policy/store.go (1)
internal/policy/store_test.go (1)
🔇 Additional comments (8)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds always-on JWT auth middleware and context helpers; centralizes policy role helpers and store warnings; replaces router role allowlist with policy-backed RequireAdmin; enforces fail-closed per-pipe allowed_roles in PipesHandler.Execute; updates handlers to use auth context and role-aware denials; broad test, config, manifest, and docs updates. ChangesAuth middleware & context
Policy & role helpers
Router & admin gate
Pipes handler & tests
Handlers and error responses
Tests, integration, playground, and manifests
Docs & config
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/api/pipes_test.go`:
- Around line 147-153: The test uses raw testify assertions
(assert.Equal/assert.NotEqual and assertJSONErrorResponse) for HTTP responses;
replace them with the repo testutil helpers: when expecting rejection replace
the pair assert.Equal(... http.StatusForbidden ...) and
assertJSONErrorResponse(t, w) with testutil.AssertJSONResponse(t, w,
http.StatusForbidden, /* expected JSON error or substring */) (keeping the same
context variables tc.allowedRoles, tc.role, tc.setRole and recorder w), and when
expecting success replace the assert.NotEqual checks with
testutil.AssertJSONContains(t, w, http.StatusOK, /* expected substring or key
present */) or testutil.AssertJSONResponse(t, w, expectedStatus, expectedBody)
as appropriate (use tc.allowedRoles/tc.role/tc.setRole to keep the test message
context and apply same change for the other occurrence around lines 176-178).
- Around line 106-118: Add a regression test row to verify that empty strings in
AllowedRoles are ignored: update the test cases table (the slice of test rows in
pipes_test.go) to include an entry with AllowedRoles set to []string{""} and the
request role empty (""), expecting the pipe to be restricted (i.e., require auth
-> expect 403/failure). Locate the table used by the test harness (the test-case
slice near the examples with descriptions like "wildcard, no role") and add a
case such as {"empty allowlist entry", []string{""}, "", false, true} to assert
a 403 response.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c52117c5-ce04-4a78-ad07-7f11a286a32a
📒 Files selected for processing (1)
internal/api/pipes_test.go
There was a problem hiding this comment.
Code Review
This pull request refactors the authorization tests for the pipes handler into a comprehensive table-driven test suite, covering scenarios for open pipes, restricted pipes, wildcard roles, and missing context roles. The feedback suggests clarifying the documentation within the test struct to better distinguish between an empty role string and a missing role in the context.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/src/content/docs/api.md`:
- Around line 325-326: Update the docs text that says auth.enabled=false leaves
all /v1/* open to include a brief exception: if a pipe defines allowed_roles
(the pipe-level `allowed_roles` setting), requests are still subject to
fail-closed checks even when `auth.enabled=false`—requests without a role (e.g.
auth disabled or JWT missing `auth.role_claim`) will be denied unless the pipe's
`allowed_roles` explicitly includes `"*"`. Reference the `auth.enabled` setting,
the `/v1/*` path, `allowed_roles`, and `auth.role_claim` so readers can locate
the relevant settings.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2368d45d-8309-4980-bfdf-159fd2cc84e3
📒 Files selected for processing (4)
CHANGELOG.mddocs/src/content/docs/api.mdinternal/api/pipes.gointernal/api/pipes_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/policy/policy_test.go`:
- Around line 327-346: Convert the new individual tests (e.g.,
TestValidate_RejectsEmptyRoleKey and the tests around lines 377-414) into a
single table-driven test using the repo convention: create tests :=
[]struct{name string; policy *Policy; wantErr bool; wantMsg string}{...} and
iterate with for _, tt := range tests { t.Run(tt.name, func(t *testing.T){ err
:= Validate(tt.policy); if tt.wantErr { require.Error(t, err);
assert.Contains(t, err.Error(), tt.wantMsg) } else { require.NoError(t, err)
}})}; reference the Policy, TablePolicy and Validate symbols to build the table
cases (include entries for empty role key, other edge-cases originally added,
and expected messages like "empty role").
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6763fc00-7703-47d1-84cb-767326ba5b2a
📒 Files selected for processing (7)
AGENTS.mdCHANGELOG.mddocs/src/content/docs/api.mdinternal/api/pipes_test.gointernal/policy/policy.gointernal/policy/policy_test.gointernal/testutil/rolematrix.go
Summary
Started as the #159 fail-open fix in pipe
AllowedRolesand grew into a fail-closed pass over the whole RBAC surface, plus opt-in public access. Authorization is now uniformly allowlist membership: a caller's role (a tokenless/roleless request is first resolved to the policydefault_role) must appear in the relevant allowlist, the built-inadminrole is the only bypass, and the*any-role wildcard is gone.Pipe authorization —
internal/api/pipes.go(Closes #159)The original bug:
ExecuteenforcedAllowedRolesonly when the request carried a non-empty role, so an empty/absent role (a JWT missingauth.role_claim, or a tokenless request) skipped the gate and a restricted pipe was served — a fail-open authorization bug, andAllowedRolesis the only gate on the execute path.Executenow runs one path for every request:default_role(viaPolicyStore/ResolveRole).policy.AdminRole) always passes.allowed_roles; empty-string entries are skipped (a stray""can't authorize a roleless caller).allowed_rolesnow authorizes nobody but the admin role (previously it was open / "still required a token" under public access). Public reach is opt-in only by listing a role thedefault_roleresolves to.policy.Evaluateand the/v1/admin/*RequireAdmingate.PipesHandler.AllowAnonymousfield/branch.Policy engine —
internal/policy/Evaluate: an empty/absent role never matches a role key; the"*"any-role role wildcard is removed (column allow-lists, e.g.allow_columns: ["*"], are unaffected); the admin-role bypass is retained. Roleless access is granted only viadefault_rolesubstitution. A nil policy (none configured yet, or deleted from KV) fails closed inEvaluateitself — only the admin role passes — so deleting the policy denies all non-admin traffic on its own.Validate: rejects empty role keys, and adefault_roleequal to the admin role (it would hand every roleless request full admin).Public / anonymous access via
default_role—internal/auth/auth.go,router.go,main.go,errors.gointernal/auth). A no-token request, or one whose token lacks the role claim, carries an empty role thatResolveRolemaps to the policydefault_rolebefore evaluation.default_roleand anonymous callers reach whatever that role is authorized for; with no usabledefault_role, a roleless request has no role and is denied 403 ("forbidden: request has no role and no public default_role is configured") by whatever gate it hits. A policy PUT/delete flips this live (no restart).token expired/invalid token) rather than a bare 403.default_roleis honored for authenticated-but-roleless requests too (valid token, no role claim).RequireAdminmiddleware (router.go), driven by the live policy admin role (policy.AdminRole, configurable viaadmin_role, defaultadmin):/v1/admin/*(raw-SQL/v1/admin/query, policy, pipe mutations, log-level), plus/v1/schema,/v1/schema/refresh, and/v1/dlq/stats— the infra endpoints that have no policy gate of their own.forbiddenForRolereturns a clearer 403 when the effective role is empty.Divergence from #159's plan
#159 (and the original PR body) treated the
"*"wildcard as kept ("wildcard intentionally permissive") and left "wildcard, no role" as a decision. We went further and removed the role wildcard entirely so authorization is pure allowlist membership with the admin role as the only bypass — no implicit "any role" grant. The role-matrix test cases were updated accordingly.Tests & docs
internal/testutil/rolematrix.go: sharedRunRoleMatrix/StandardRoleMatrix— the canonical(AllowedRoles, observed-role)matrix every allowlist gate must pass; a handler takingAllowedRoleswithout running it looks under-tested in review.internal/api/pipes_test.go(no-allowlist deny/allow, default_role resolution, param paths),internal/policy/policy_test.go(empty-role guards,*-no-longer-grants regression, default_role substitution/escalation refusal),internal/api/router_test.go,internal/auth/auth_test.go,internal/policy/store_test.go,internal/config/config_test.go.CHANGELOG.md,docs/api.md,docs/configuration.md,AGENTS.mdupdated to the fail-closed / no-wildcard public-access semantics.Breaking changes
allowed_rolesdenies all callers except the admin role (was open)."*"any-role wildcard is removed from pipeallowed_rolesand frompolicy.Evaluaterole keys. Existing"*"role entries now match nothing (columnallow_columns: ["*"]is unaffected).Related issues
RequireRoles(roles ...string)primitive — the route layer stays admin-vs-not (RequireAdmin) and finer access stays per-pipeallowed_roles+ policy.