From 78cf2dfd2ddb5b6540b72f5b2bf1096598624fbf Mon Sep 17 00:00:00 2001 From: Tigran Babloyan Date: Fri, 25 Sep 2026 11:46:58 +0400 Subject: [PATCH] fix(AF-1102): fail closed on OTHER statements in denied_columns MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit DeniedColumns.rejected skipped OTHER on the premise that no permission grants it, but a request-group member admits MERGE/CALL under can_write (can_break_glass for break-glass groups), so MERGE … USING (SELECT ssn FROM users) bypassed a users.ssn denial. OTHER is never column-analysed, so it now reaches every denied entry, like any unanalysed parse (mirrors DeniedShapes in #1101). Closes #1102 --- .../accessflow/core/api/DeniedColumns.java | 14 ++--- .../core/api/DeniedColumnsTest.java | 23 +++++++- .../DefaultRequestGroupServiceCrudTest.java | 59 +++++++++++++++++++ docs/04-api-spec.md | 2 +- docs/05-backend.md | 2 +- docs/07-security.md | 10 +++- 6 files changed, 96 insertions(+), 14 deletions(-) diff --git a/backend/src/main/java/com/bablsoft/accessflow/core/api/DeniedColumns.java b/backend/src/main/java/com/bablsoft/accessflow/core/api/DeniedColumns.java index 9972f5de..e46e4f0a 100644 --- a/backend/src/main/java/com/bablsoft/accessflow/core/api/DeniedColumns.java +++ b/backend/src/main/java/com/bablsoft/accessflow/core/api/DeniedColumns.java @@ -58,18 +58,18 @@ public static boolean isQualified(String entry) { /** * @return the denied entries the parsed query reaches, sorted; empty when it reaches none. A - * data query whose columns were not analyzed (a non-JSqlParser engine) reaches every - * entry, so a deny list can never be silently skipped. DDL is refused when it reads a - * denied column through an embedded query ({@code CREATE TABLE … AS SELECT}) or touches - * a table a denied entry names at all; {@code OTHER} is never checked, since no - * permission grants it. + * query whose columns were not analyzed (a non-JSqlParser engine) reaches every entry, + * so a deny list can never be silently skipped. DDL is refused when it reads a denied + * column through an embedded query ({@code CREATE TABLE … AS SELECT}) or touches a + * table a denied entry names at all. {@code OTHER} ({@code MERGE}, {@code CALL}) is + * never column-analyzed, so it reaches every entry — a request-group member may be one. */ public static SortedSet rejected(List rawDenied, SqlParseResult parsed) { var denied = normalize(rawDenied); - if (denied.isEmpty() || parsed == null || parsed.type() == QueryType.OTHER) { + if (denied.isEmpty() || parsed == null) { return new TreeSet<>(); } - if (!parsed.columnsAnalyzed()) { + if (!parsed.columnsAnalyzed() || parsed.type() == QueryType.OTHER) { return new TreeSet<>(denied); } var out = rejected(denied, parsed.referencedColumns()); diff --git a/backend/src/test/java/com/bablsoft/accessflow/core/api/DeniedColumnsTest.java b/backend/src/test/java/com/bablsoft/accessflow/core/api/DeniedColumnsTest.java index a6fb0453..e8cd5bb8 100644 --- a/backend/src/test/java/com/bablsoft/accessflow/core/api/DeniedColumnsTest.java +++ b/backend/src/test/java/com/bablsoft/accessflow/core/api/DeniedColumnsTest.java @@ -127,12 +127,29 @@ void ddlTouchingATableWithADeniedColumnIsRefused() { } @Test - void unanalyzedDdlFailsClosedAndOtherIsNeverChecked() { + void unanalyzedDdlFailsClosed() { assertThat(DeniedColumns.rejected(DENIED, new SqlParseResult(QueryType.DDL, "ALTER VIEW v AS SELECT national_id FROM customer"))) .containsExactly("public.customer.national_id"); - assertThat(DeniedColumns.rejected(DENIED, new SqlParseResult(QueryType.OTHER, "CALL x()"))) - .isEmpty(); + } + + @Test + void otherStatementReachesEveryDeniedEntry() { + var denied = List.of("users.ssn", "public.customer.national_id"); + + assertThat(DeniedColumns.rejected(denied, new SqlParseResult(QueryType.OTHER, "CALL x()"))) + .containsExactly("public.customer.national_id", "users.ssn"); + } + + @Test + void otherStatementFailsClosedEvenWhenMarkedAnalyzed() { + var merge = new SqlParseResult(QueryType.OTHER, false, + List.of("MERGE INTO t USING (SELECT ssn FROM users) s ON (t.id = s.id) " + + "WHEN MATCHED THEN UPDATE SET t.x = s.ssn"), + Set.of("t", "users"), false, false, + Set.of(new ColumnReference(Set.of("orders"), "total")), true); + + assertThat(DeniedColumns.rejected(List.of("users.ssn"), merge)).containsExactly("users.ssn"); } @Test diff --git a/backend/src/test/java/com/bablsoft/accessflow/requestgroups/internal/DefaultRequestGroupServiceCrudTest.java b/backend/src/test/java/com/bablsoft/accessflow/requestgroups/internal/DefaultRequestGroupServiceCrudTest.java index 24b42d1a..da3cbae3 100644 --- a/backend/src/test/java/com/bablsoft/accessflow/requestgroups/internal/DefaultRequestGroupServiceCrudTest.java +++ b/backend/src/test/java/com/bablsoft/accessflow/requestgroups/internal/DefaultRequestGroupServiceCrudTest.java @@ -285,6 +285,65 @@ void breakGlassSubmitAlsoRejectsADeniedTable() { verify(stateService, org.mockito.Mockito.never()).apply(any(), any()); } + @Test + void submitRejectsAnOtherMemberWhenTheSubmitterIsDeniedAColumn() { + var group = draftGroup(); + when(groupRepository.findByIdAndOrganizationId(group.getId(), orgId)).thenReturn(Optional.of(group)); + when(itemRepository.findByGroupIdOrderBySequenceOrderAsc(group.getId())) + .thenReturn(List.of(mergeItem())); + when(datasourcePermissionLookupService.findFor(userId, datasourceId)) + .thenReturn(Optional.of(writerDenyingSsn(false))); + stubMergeParse(); + + assertThatThrownBy(() -> service.submit(new SubmitRequestGroupCommand(group.getId(), orgId, + userId, false, false, null, "1.2.3.4", "ua"))) + .isInstanceOf(RequestGroupPermissionException.class) + .hasMessageContaining("users.ssn"); + verify(stateService, org.mockito.Mockito.never()).apply(any(), any()); + } + + @Test + void breakGlassSubmitAlsoRejectsAnOtherMemberWhenTheSubmitterIsDeniedAColumn() { + var group = draftGroup(); + when(groupRepository.findByIdAndOrganizationId(group.getId(), orgId)).thenReturn(Optional.of(group)); + when(itemRepository.findByGroupIdOrderBySequenceOrderAsc(group.getId())) + .thenReturn(List.of(mergeItem())); + when(datasourcePermissionLookupService.findFor(userId, datasourceId)) + .thenReturn(Optional.of(writerDenyingSsn(true))); + stubMergeParse(); + + assertThatThrownBy(() -> service.submit(new SubmitRequestGroupCommand(group.getId(), orgId, + userId, false, true, null, "1.2.3.4", "ua"))) + .isInstanceOf(RequestGroupPermissionException.class) + .hasMessageContaining("users.ssn"); + verify(stateService, org.mockito.Mockito.never()).apply(any(), any()); + } + + private static final String MERGE_SQL = "MERGE INTO t USING (SELECT id, ssn FROM users) s " + + "ON (t.id = s.id) WHEN MATCHED THEN UPDATE SET x = s.ssn"; + + private RequestGroupItemEntity mergeItem() { + var item = new RequestGroupItemEntity(); + item.setTargetKind(com.bablsoft.accessflow.requestgroups.api.RequestGroupTargetKind.QUERY); + item.setDatasourceId(datasourceId); + item.setQueryType(QueryType.OTHER); + item.setSqlText(MERGE_SQL); + return item; + } + + private DatasourceUserPermissionView writerDenyingSsn(boolean breakGlass) { + return new DatasourceUserPermissionView(UUID.randomUUID(), userId, datasourceId, false, true, + false, breakGlass, List.of(), List.of(), List.of(), List.of("users.ssn"), List.of(), + List.of(), null, null); + } + + private void stubMergeParse() { + when(datasourceLookupService.findById(datasourceId)).thenReturn(Optional.empty()); + when(queryParser.parse(any(), any())).thenReturn(new SqlParseResult(QueryType.OTHER, false, + List.of(MERGE_SQL), java.util.Set.of("t", "users"), false, false, + java.util.Set.of(), false)); + } + private RequestGroupItemEntity deniedTableItem() { var item = new RequestGroupItemEntity(); item.setTargetKind(com.bablsoft.accessflow.requestgroups.api.RequestGroupTargetKind.QUERY); diff --git a/docs/04-api-spec.md b/docs/04-api-spec.md index 7fb205a0..f599293a 100644 --- a/docs/04-api-spec.md +++ b/docs/04-api-spec.md @@ -644,7 +644,7 @@ ADMINs may sample any datasource in their organization; non-ADMINs need a permis `restricted_columns` is a list of fully-qualified `schema.table.column` strings (case-insensitive). Values for these columns are masked with `"***"` in SELECT result rows, and the AI analyzer is told that the SQL touches restricted columns (informational — never auto-rejects). Null or empty means no column restrictions. -`denied_columns` (#935) is a list of `table.column` or `schema.table.column` strings, returned normalised (unquoted, lowercase). A query that references one — in the select list, `WHERE`, `JOIN`, `GROUP BY`, `HAVING`, `ORDER BY`, a subquery, an `UPDATE … SET` target or an `INSERT` column list, or through `*` / `t.*` / a column-list-less `INSERT` on the entry's table — is rejected **before** it is persisted. `POST /queries`, `POST /queries/dry-run` and `GET /datasources/{id}/sample-rows` answer 403 with `error: "FORBIDDEN"` and a localized `detail` naming the denied entries. Break-glass answers `error: "BREAK_GLASS_NOT_PERMITTED"`, and a request-group submit answers `error: "REQUEST_GROUP_PERMISSION_DENIED"`. The table preview is refused whenever its table has a denied column, since it reads every column. Deny beats mask for a column in both lists. Null or empty means nothing is denied. +`denied_columns` (#935) is a list of `table.column` or `schema.table.column` strings, returned normalised (unquoted, lowercase). A query that references one — in the select list, `WHERE`, `JOIN`, `GROUP BY`, `HAVING`, `ORDER BY`, a subquery, an `UPDATE … SET` target or an `INSERT` column list, or through `*` / `t.*` / a column-list-less `INSERT` on the entry's table — is rejected **before** it is persisted. `POST /queries`, `POST /queries/dry-run` and `GET /datasources/{id}/sample-rows` answer 403 with `error: "FORBIDDEN"` and a localized `detail` naming the denied entries. Break-glass answers `error: "BREAK_GLASS_NOT_PERMITTED"`, and a request-group submit answers `error: "REQUEST_GROUP_PERMISSION_DENIED"`. The table preview is refused whenever its table has a denied column, since it reads every column. A request-group `QUERY` member whose statement is neither a data query nor DDL (`MERGE`, `CALL`, …) has no analysed column references, so it is refused whenever `denied_columns` is non-empty. Deny beats mask for a column in both lists. Null or empty means nothing is denied. `denied_schemas` and `denied_tables` (#939) are table/schema deny-lists, returned normalised (unquoted, lowercase), `null` when unset. A denial **always beats** the allow-list and is evaluated after it, so `allowed_schemas: ["crm"]` + `denied_tables: ["crm.salary"]` permits `crm.customer` (and any `crm` table created later) while refusing `crm.salary`. Denials also apply with no allow-list at all. Matching fails closed, because the gate cannot know where the database resolves a name: a `denied_tables` entry matches when either name is a dot-aligned suffix of the other (bare `salary` denies `salary` in every schema; `crm.salary` denies `crm.salary`, `db.crm.salary` and an unqualified `salary`), and a `denied_schemas` entry matches any reference carrying it as a non-final segment **and every unqualified reference** — while any schema is denied, the grantee must schema-qualify table names. A `schema.*` entry in `denied_tables` denies the whole schema. Names are compared segment by segment from the right; an empty segment (SQL Server `db..salary`) matches anything, an Oracle `@dblink` suffix is ignored, and a pattern reference (`*` / `?`, e.g. an Elasticsearch index pattern `sal*`) is denied by any entry. Deny-lists apply to every engine; on engines whose names carry no schema (MongoDB, DynamoDB, Redis) any `denied_schemas` entry refuses every query, so use `denied_tables` there. A JIT approval that replaces the user's expiring direct row carries that row's denials onto the new grant. A query that reaches a denied table is rejected **before** it is persisted: `POST /queries` and `POST /queries/dry-run` answer 403 `FORBIDDEN` with `error.permission.table_denied` ("Query references one or more tables the user is denied on this datasource: …"), break-glass answers `BREAK_GLASS_NOT_PERMITTED`, and a request-group submit answers `REQUEST_GROUP_PERMISSION_DENIED`. A denied table is hidden from `GET /datasources/{id}/schema` (a denied schema disappears entirely) and answers 404 from `GET /datasources/{id}/sample-rows`, exactly like one outside the allow-list. diff --git a/docs/05-backend.md b/docs/05-backend.md index f9ea5afc..e39db07c 100644 --- a/docs/05-backend.md +++ b/docs/05-backend.md @@ -828,7 +828,7 @@ audited as `PERMISSION_GROUP_GRANTED` / `PERMISSION_GROUP_REVOKED` (connector si `rejected(denied, parsed)`. It returns every denied entry whose table matches a candidate (the schema must match only when both sides carry one) and whose column matches, or which a wildcard reaches. It fails closed: any statement that was not column-analysed rejects every entry, and a DDL - statement also rejects every entry on a table it touches. OTHER returns nothing. `rejectedForWholeTable` answers the + statement also rejects every entry on a table it touches. OTHER (`MERGE`, `CALL`) is never column-analysed and rejects every entry — no capability grants it standalone, but a request-group member admits it under `can_write` / `can_break_glass`. `rejectedForWholeTable` answers the table preview, and `union` merges grants (#1099): a column stays denied when any grant denies it, and overlapping spellings collapse to the broader entry (`users.ssn` covers `public.users.ssn`). - **Enforcement.** The following all call it: `DatasourcePermissionVerifier.verify` (submission and diff --git a/docs/07-security.md b/docs/07-security.md index 72f5e7e2..6ad0750a 100644 --- a/docs/07-security.md +++ b/docs/07-security.md @@ -763,7 +763,9 @@ Are denied_columns set? (#935, relational engines only) YES → resolve every column the parsed statement references (select items, WHERE, JOIN ON/USING, GROUP BY, HAVING, ORDER BY, subqueries, UPDATE SET targets, INSERT column lists, RETURNING) to its candidate tables; a `*` / `t.*` / column-list-less INSERT on a table a denied entry - names counts as a reference; reject (403, `error.permission.column_not_allowed`) on any hit. + names counts as a reference; an unanalysed parse and an OTHER statement (MERGE, CALL — a + request-group member) reach every entry; reject (403, `error.permission.column_not_allowed`) + on any hit. Violation → 403 ↓ Are restricted_columns set? @@ -909,7 +911,11 @@ refuses the preview), and the access simulator, which reports the refusal as deny list refuses the query rather than being skipped. DDL that touches a table with a denied column is refused outright, because DDL can expose a column without reading it (`RENAME COLUMN`, a generated column, `CREATE VIEW … AS SELECT`). DDL the parser cannot walk is refused whenever the - deny list is non-empty. `OTHER` statements are never checked, because no permission grants them. + deny list is non-empty. `OTHER` statements (`MERGE`, `CALL`, …) are never column-analysed, so + one is refused whenever the deny list is non-empty. A standalone query never reaches this — no + capability grants `OTHER` — but a request-group member can: `can_write` admits an `OTHER` member + (`can_break_glass` in a break-glass group), and without this rule + `MERGE INTO t USING (SELECT ssn FROM users) …` would read a denied `users.ssn`. - **Known limits.** The check reads the SQL, not the database catalog. It cannot see a value the database computes from a column the SQL never names: a view's or function's own body, SQL inside a string argument, or a user-defined or extension function over the row type called in PostgreSQL's