fix(AF-1102): fail closed on OTHER statements in denied_columns - #1103
Merged
Merged
Conversation
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
Contributor
Backend Test Results10 508 tests +4 10 508 ✅ +4 11m 26s ⏱️ -15s Results for commit 9ce885c. ± Comparison against base commit 4d28670. This pull request removes 1 and adds 5 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Contributor
Backend Code Coverage
|
…lumns-other-members # Conflicts: # backend/src/test/java/com/bablsoft/accessflow/requestgroups/internal/DefaultRequestGroupServiceCrudTest.java
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
1 participant
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.
Closes #1102
What
core.api.DeniedColumns.rejectedno longer skipsOTHERstatements.OTHER(MERGE,CALL, …) is never column-analysed bySqlParserServiceImpl, so it now fails closed like any unanalysed parse: it reaches every denied entry. It is treated that way even if a parse result claimscolumnsAnalyzed=true.Why
The old early return assumed no permission grants
OTHER. That holds for standalone queries, butDefaultRequestGroupService.validatePermissionadmits anOTHERmember for a non-admin holding onlycan_write(orcan_break_glassin a break-glass group).verifyTableAndColumnScopethen calledDeniedColumns.rejected, which returned nothing. As a result,MERGE INTO t USING (SELECT ssn FROM users) s …bypassed adenied_columns = [users.ssn]grant. This is the same short-circuit #1101 (#940) fixed inDeniedShapes.Reviewer notes
OTHERisn't reliable: aCALLcan read tables it never names, and a statement the inspector can't walk yields an empty set. So anOTHERmember is refused wheneverdenied_columnsis non-empty.OTHERat the capability check, before this matcher runs.DeniedColumnsTestreplaces the assertion that "OTHER is never checked" with two cases:CALLreaches every entry, and aMERGEparse marked analysed still fails closed.DefaultRequestGroupServiceCrudTestadds a normal (can_write-only) and a break-glassMERGEmember case; both are refused namingusers.ssn, and the group never changes state.denied_columnssemantics are updated indocs/04-api-spec.md,docs/07-security.md(the section and the enforcement flow chart) anddocs/05-backend.md(the "Matching" bullet).Local: the full backend unit suite passes (
mvn test -Dtest='!*IntegrationTest', 9,049 tests, includingApplicationModulesTestandApiPackageDependencyTest). Integration tests were not run locally.