Skip to content

fix(roles): the role editor says Harper 5.3 gates a structure user's DDL and SQL by the allowlist - #1896

Merged
dawsontoth merged 4 commits into
claude/1788-operations-catalog-5-3from
claude/1885-allowlist-gate-order-5-3
Oct 12, 2026
Merged

dawsontoth merged 4 commits into
claude/1788-operations-catalog-5-3from
claude/1885-allowlist-gate-order-5-3

Conversation

@dawsontoth

@dawsontoth dawsontoth commented Oct 11, 2026 •

Copy link
Copy Markdown
Contributor

Stacked on #1884 (base claude/1788-operations-catalog-5-3). Review that one first; this diff is only the gate-order change.

The role editor and the role-assignment summary now say what Harper 5.3 actually does with a role's operations allowlist. From Harper 5.3.0-alpha.1 the list also gates a structure user's DDL and every SQL statement, and Studio still told authors the opposite.

Closes #1885.

⊙ Problem

Harper commit f8a5aa90a (harper#2176) is in every tag from v5.3.0-alpha.1 on and in no 5.2.x tag (git tag --contains; v5.2.15 is the newest 5.2 tag). It changed gate 1 of the allowlist in two ways:

  • Order. verifyPerms checks the allowlist before its super_user and structure_user returns (utility/operation_authorization.ts at v5.3.0: gate at line 773, super-user return at 799, structure-user returns at 805 and 810). Through v5.2.15 those returns come first (593, 599, 604) and the gate is at 629.
  • SQL. chooseOperation runs the gate on sql before checkASTPermissions (server/serverHelpers/serverUtilities.ts at v5.3.0, line 373). Through v5.2.15 the SQL branch goes straight to checkASTPermissions (line 239).

On 5.3, the editor told a structure-user author that create_table, drop_table and the other DDL work "regardless of the list below", and Harper refuses them. It also said "this list does not restrict" SQL, and the effective count left sql out, so read_only counted 12 operations where Harper grants 13.

❓ Your call: the issue covers only the structure-user and super-user order. I also fixed the SQL half because it comes from the same Harper commit and the same paragraph of editor copy, and it makes the count wrong on 5.3. Dropping that half means reverting the sql catalog entry and one sentence in each surface.

💡 Solution

GrantBand gains a third band, so each instance version resolves to one band that covers both 5.3 changes:

Band Versions Gate before structure_user and on SQL Every entry named
pre-5.3 up to 5.2.x no no
5.3-early 5.3.0-alpha.1 to beta.2 yes no
5.3 5.3.0-beta.3 on yes yes

allowlistGatesEveryPath(band) answers the first question, and #1884's gateVerdict still answers the second. On a gate-first instance:

  • Editor: the structure-user note says the list gates the role's DDL too, and it names the DDL the list leaves out ("Not listed, so Harper refuses them: drop_table, drop_database"). The note for the array form still says listing DDL cannot reach another database, which holds on both orders. The empty-list warning drops "except the DDL noted above" and says SQL is denied too. The headline says SQL needs sql (or a group with it) listed.
  • sql: it counts as a grant, so read_only counts 13. Its "changes nothing" chip title shows only before 5.3.
  • Assignment summary: the same structure-user and SQL wording.

Before 5.3 every sentence reads as it did. The 5.3-early band exists because 5.3.0-beta.2 already gates first while deploy_component and the other 19 entries are still unnamed. I ran this against Harper's own build (Verification).

❓ Your call: useInstanceBrowseManagePermission is true for any structure_user: true role, and so it offers DDL chrome that 5.3 refuses to an allowlisted structure user. I left the hook as it is: it can't see the version, and gating it on the list would hide DDL that works on 5.2. Its comment and AGENTS.md say why, and #1894 (P3) tracks per-operation checks. The planning review rated this "major, acknowledged". Fixing it here would mean threading the version into a hook used by 12 surfaces.

⚖️ Alternatives

The planning review (codex) returned chosen-approach-sound. Its one blocker was that the sql chip title had to follow the band too, and that is done.

🔧 Changes

❓ Your call: I first left that preparePermissionForSave.ts line alone, because #1893 rewrote the same docblock and keeps the phrase. #1893 is on stage now, and the pre-push adjudicator kept the stale line as a finding, so this PR fixes it. Merging stage into this stack will therefore conflict on that one comment line: keep stage's docblock and change only its parenthetical.

✅ Verification

  • Harper's own verifyPerms, off-repo. I ran it from the installed 5.2.3 and 5.3.0-beta.2 packages and the checkout's 5.3.1 dist, with no checkout edited:

    role → operation 5.2.3 5.3.0-beta.2 5.3.1
    structure_user: true, [read_only] → create_table allowed denied denied
    structure_user: true, [read_only] → create_database allowed denied denied
    structure_user: true, [create_table] → create_table allowed allowed allowed
    structure_user: ['dev'], [drop_table] → drop_table on other denied denied denied
    structure_user: ['dev'], [] → drop_table on dev allowed denied denied
    super_user: true, [] → create_table allowed denied denied
    gate 1 on sql, [read_only] / [insert] no gate allowed / denied allowed / denied

    Reviewers can't re-run this from the repo. AGENTS.md says how to.

  • Unit tests:

  • Mutation checks. I reverted each guard in turn and ran the role-operations tests: dropping the 5.3-early branch failed 5 tests; allowlistGatesEveryPath → false failed 9; making isOutOfGate ignore the band failed 7; showing the caveat on every chip failed 1; dropping the missing-DDL sentence failed 2; reverting the editor note, empty line and headline failed 4, 1 and 1; reverting the summary note and empty line failed 1 and 2. Computing the missing DDL from the raw list instead of the effective set stayed green. That is an equivalent mutant: no group carries DDL and no DDL name is inert, so the two sets agree on DDL today.

  • Gates. The pre-commit hook ran vitest (395 files, 3,696 passed, 11 skipped), oxlint, dprint and commitlint. npx tsc -b, npx oxlint --format stylish . and npx dprint check all exited 0. I didn't verify in a browser, because port 5173 is shared with parallel sessions. The change is copy and counts, all covered by the component tests.

  • Cross-model review. A codex planning round came first (chosen-approach-sound), then one full implementation round with codex (graded) and gemini. Its verdict was COMMENTS, with no functional defect. I fixed both codex comments in the second commit: an export nothing imported, and a "before 5.3" doc comment that now means beta.3. I rejected gemini's "wasAReleasedBeforeB may be strict": it returns b >= a (src/lib/string/wasAReleasedBeforeB.ts), and the tests pin alpha.1 → 5.3-early and beta.3 → 5.3. Its comment nit cites test comments this PR doesn't touch. Coverage gap in that round: the Cursor leg failed on the 1Password SSH agent (no-receipt), and the Harper domain adjudicator failed auth (an expired Claude OAuth session), so no adjudication ran. After the gemini-code-assist fix, a second full round ran codex (graded), gemini and the Harper domain adjudicator at 8a846dfa, with Cursor pruned on purpose. Its verdict was COMMENTS, and it found no functional defect. Its one surviving nit was the stale save-path docblock plus a reviewer-addressed test comment. 7c010178 fixes both, comments only.

  • gemini-code-assist feedback. Its one thread said the summary dropped a scoped structure user's databases on the gate-first bands. It was right, because the editor's gated note keeps the scope and verifyPerms still applies the structure_user array after gate 1 (harper utility/operation_authorization.ts:810-824). Fixed in 8a846dfa, with the test above. On the gate-first bands this note still says "needs listing too" even when every DDL operation is listed, which the domain review logged as a decision (summary-generic-ddl-note). It is coarser than the editor but never wrong about enforcement.

🤖 Generated by Claude Code (Claude Opus); posted via @dawsontoth.

🤖 Generated with Claude Code

Related PRs: #1884 overlaps (the base of this stack)
Complexity: medium

Review-Coverage: authored=claude; ran=codex,gemini; adjudicated=domain; blocked=cursor-composer(no-receipt); declined=cursor-grok,cursor-kimi,cursor-muse; rounds=2; full=1 @ 7c01017

Review-Attention: read ~5m (decisions: browse-manage-stays-ungated, three-band-enum, outofgate-single-band, summary-generic-ddl-note, do-less-alternative) @ 7c01017

dawsontoth and others added 2 commits October 11, 2026 05:19
…the allowlist

From Harper 5.3.0-alpha.1 (harper f8a5aa90a), verifyPerms runs a role's
operations allowlist ahead of its super_user and structure_user returns,
and chooseOperation runs it on `sql`. The editor and the assignment summary
still said a structure user reaches DDL regardless of the list and that the
list does not restrict SQL, and the effective count left `sql` out.

GrantBand gains a `5.3-early` band (5.3.0-alpha.1 to beta.2) between
`pre-5.3` and `5.3`, so the gate-order change and the beta.3 naming change
are both resolved from one band. The structure-user, SQL and empty-list
copy, the `sql` chip and the effective count follow it. The permission
hooks keep their behavior; their comments and AGENTS.md now say why on
each version, and #1894 tracks per-operation checks for 5.3.

Closes #1885

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…change

Nothing outside the catalog reads ALLOWLIST_GATE_RUNS_FIRST_FROM, and the
GateChange doc still said "before 5.3" for a change that landed in
5.3.0-beta.3, which the new 5.3-early band makes ambiguous.

Refs #1885

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request updates the operations allowlist editor, summary components, and tests to correctly handle Harper 5.3+ permission gating changes, specifically introducing the 5.3-early grant band for versions starting from 5.3.0-alpha.1 where the allowlist gates SQL and structure-user DDL paths. The review feedback points out an issue in RoleOperationsSummary.tsx where the database scope restriction is hidden for scoped structure users when gatesEveryPath is true, and suggests preserving this scope in the summary message.

@dawsontoth
dawsontoth added this pull request to stack #1902 October 11, 2026 21:25
@github-actions

github-actions Bot commented Oct 11, 2026 •

Copy link
Copy Markdown

Coverage Report

Status Category Percentage Covered / Total
🔵 Lines 67.58% 10122 / 14976
🔵 Statements 67.74% 10801 / 15944
🔵 Functions 60.84% 2606 / 4283
🔵 Branches 62.69% 7731 / 12331
File Coverage
File Stmts Branches Functions Lines Uncovered Lines
Changed Files
src/features/instance/config/roles/preparePermissionForSave.ts 100% 100% 100% 100%
src/features/instance/config/roles/operations/OperationsAllowlistEditor.tsx 89.47% 86.17% 91.42% 89.65% 333, 372-383
src/features/instance/config/roles/operations/RoleOperationsSummary.tsx 100% 93.18% 100% 100%
src/features/instance/config/roles/operations/operationsCatalog.ts 100% 97.36% 100% 100%
src/hooks/checkOperationPermission.ts 100% 92.85% 100% 100%
src/hooks/usePermissions.ts 59.13% 55.78% 71.42% 59.29% 59-65, 86-93, 126, 131, 202, 205, 208, 220-230, 269-271, 289-291, 313-352
src/integrations/api/localRolePermission.ts 100% 100% 100% 100%
Generated in workflow #2186 for commit 7c01017 by the Vitest Coverage Report Action

dawsontoth and others added 2 commits October 12, 2026 11:29
…ist gates its DDL

On the 5.3 bands the role summary said only that the structure-user DDL needs listing, dropping the database scope the pre-5.3 copy and the editor's gated note both keep. verifyPerms still applies the structure_user array after gate 1 (harper utility/operation_authorization.ts:810-824), so a listed DDL operation reaches only those databases.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…wlist

preparePermissionForSave's doc still described the pre-5.3 gate order as universal; from 5.3.0-alpha.1 Harper gates the structure-user DDL on the list too. Also drops a PR-number reference from a test comment in favour of the boundary it means. Comment-only; raised by the pre-push domain adjudication.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@dawsontoth
dawsontoth marked this pull request as ready for review October 12, 2026 17:36
@dawsontoth
dawsontoth requested a review from a team as a code owner October 12, 2026 17:36
@dawsontoth
dawsontoth removed this pull request from stack #1902 October 12, 2026 17:36
@dawsontoth
dawsontoth merged commit 883ff75 into claude/1788-operations-catalog-5-3 Oct 12, 2026
4 checks passed
@dawsontoth
dawsontoth deleted the claude/1885-allowlist-gate-order-5-3 branch October 12, 2026 17:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Role editor describes Harper 5.2's allowlist gate order on 5.3+, where the list also gates structure-user DDL

1 participant