fix(api-gateway): return 403 for RBAC member-level denials - #11836
Open
allanmaclean wants to merge 1 commit into
Open
fix(api-gateway): return 403 for RBAC member-level denials#11836allanmaclean wants to merge 1 commit into
allanmaclean wants to merge 1 commit into
Conversation
A query an access policy denied member-level access to was still executed. It returned no rows and then failed while the result was transformed, with "You requested hidden member: '<name>'. Please make it visible using `public: true`...", surfaced to the caller as HTTP 500. That conflated an authorization failure with a server fault and advised weakening the very policy that did the denying. `applyRowLevelSecurity` already detects the denial before execution, so it now reports which members were denied and the gateway rejects the data APIs (`/load` — REST, GraphQL and subscriptions) with `403 Forbidden`. The message names the denied members the request itself asked for: the caller supplied those names, and a member that doesn't exist already fails differently (`400`, "not found for path"), so withholding them would hide nothing while making a denial hard to act on. Policies are evaluated over the members the generated SQL touches, which pulls in members the caller never named (a cube's primary key, for one) — those are logged server-side only. Dev mode is deliberately left as it was. It doesn't enforce security checks, so a request without a token — the playground's normal state — carries no security context, resolves to no groups, and is denied by every policy. Failing those would break the playground for any model using access policies, so the denial is only logged there. The SQL API keeps its empty-result semantics, and members hidden by `public: false` outside of RBAC keep their existing message, where the `public: true` advice is the correct guidance. Fixes cube-js#11769 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Check List
Issue Reference this PR resolves
Fixes #11769
Description of Changes Made
A query that an access policy denies member-level access to was still executed. It came back with no rows and then failed while the result was transformed, with
You requested hidden member: 'orders_view.status'. Please make it visible using `public: true`..., surfaced to the caller as HTTP 500.Two problems from the issue: an authorization failure is reported as a server fault, and the guidance tells the operator to set
public: true, which would weaken the very policy that did the denying.CompilerApi.applyRowLevelSecurityalready detects the denial before execution — it neutralizes the query with a1 = 0segment — so the information needed to answer correctly was there all along, just not propagated. This PR follows the issue's preferred fix (evaluate member access in JS before execution and throw a typed error):applyRowLevelSecuritynow returnsdeniedMembersalongsidedenied, collecting every member no policy grants rather than bailing on the first one.ApiGateway.getNormalizedQueriespropagates those members to its callers.ApiGateway.load— the data-API entry point shared by REST, GraphQL and subscriptions — rejects a denied query with403 Forbiddenand a message naming the denied members:Access to the following members is denied by an access policy: orders_view.status.On the third point in the issue (the restricted member name being disclosed): the response names the denied members that the request itself asked for. The caller supplied those names, and a member that doesn't exist already fails differently —
400,"'foo' not found for path 'orders.foo'"— so existence is disclosed by the status code either way. Withholding the names would hide nothing while making a denial on a many-membered query hard to act on. Policies are evaluated over the members the generated SQL touches, which pulls in members the caller never named (a cube's primary key, for one); those are filtered out of the message and logged server-side (Access Policy Denied) instead.Deliberately unchanged:
sqlApiLoad/streamignore the new flag), so the existing two-dimensional policy-overlap behavior is untouched.public: falseoutside of RBAC keep the existing message — there thepublic: trueadvice is the correct guidance, so the Rust-side check inquery_result_transform.rsis left alone.Dev mode is deliberately left as it was. It doesn't enforce security checks (
enforceSecurityChecksis false outsideNODE_ENV=production), so a request without a token — the playground's normal state — carries no security context, resolves to no groups, and is therefore denied by every policy. Returning 403 there would break the playground for any model using access policies, so the denial is only logged in dev mode.That does leave a related wart untouched: in dev mode the neutralized query is served as
[{"products.count": "0"}], because the1 = 0segment turns a policy denial into a row that reads as a legitimate answer. It's out of scope here, but happy to follow up separately if you'd like it addressed.Tests
packages/cubejs-server-core/test/unit/CompilerApi.test.ts— newapplyRowLevelSecurityunit tests: an allowed query is not denied, a denied query reports every ungranted member (not just the first) and is neutralized with therlsAccessDeniedsegment, and a user matched by no policy at all is denied.packages/cubejs-api-gateway/test/index.test.ts—/load(GET and POST) returns 403 naming the denied member, a member denied via a filter is named too, and a member the request never asked for is kept out of the response body.packages/cubejs-testing/test/smoke-rbac.test.tsandsmoke-rbac-graphql.test.ts— updated to the new error, plus assertions that the response status is403and that the denied member is named. The dev-mode tests are unchanged.Ran locally, all green:
cubejs-server-core92/92,cubejs-api-gateway239/239,smoke-rbac-graphql4/4, andsmoke-rbac91/94 against a birdbox built from this branch. The 3 remainingsmoke-rbacfailures are all[Python config]blocks failing to boot on a fallback build of the native extension ("Unable to load Python configuration because you are using the fallback build of native extension"), unrelated to this change. eslint clean on all changed files.Docs
docs/data-modeling/data-access-policies.mdx— the "Access is denied (an empty result)" bullet now states the actual per-API behavior.docs/data-modeling/access-control/member-level-security.mdx— short note on what a caller gets when querying a member their group has no access to.