Repository navigation
sec(frontend): stop group edit from dropping/widening permissions - #1730
Conversation
cristim
added
triaged
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 36 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
Comment |
Adversarial review — PR #1730 @
|
| Gate | Result |
|---|---|
npx tsc --noEmit |
exit 0 |
npx eslint src --ext .ts |
exit 0 — 125 warnings, 0 errors; I grepped the output for the three changed files and none of the warnings are in them (all pre-existing no-explicit-any in unrelated files) |
npm run build |
exit 0, webpack compiled |
npx jest (full) |
2778 passed, 8 failed, 1 pending |
On the 8 failures — the brief's count is right but its file list is incomplete. They span three files, not two:
riexchange.test.ts— 4utils.test.ts— 2approval-details.test.ts— 2 ← not named in the brief
I did not accept "pre-existing" on assertion. Root cause is the host-locale thousands separator (Expected "$1,000" / Received "$1.000"), consistent across all three files including the two approval-details ones (formats upfront and monthly savings via formatCurrency). To confirm they are not caused by this diff I ran those three files at the base commit ff808b26c and got the identical 8 failures, same names. Genuinely pre-existing (#1728), and locale-dependent, so they do not reproduce in CI.
CI on this head: Frontend build (PR), Frontend E2E (PR) and pre-commit all green; only Security Scanning → npm audit fails, on the same js-yaml GHSA-5p4m-2wfm-xmqj advisory, unrelated to this diff (#1729).
Claims re-derived
1. Does the exhaustiveness check fail the build on drift, in BOTH directions? — YES, verified by execution.
I mutated permissions.ts twice and ran tsc each time:
- Added
'brand-new-verb'to theActionunion only:
src/permissions.ts(101,7): error TS2741: Property '"brand-new-verb"' is missing in type '{...}' but required in type 'Record<Action, true>'. - Added
'brand-new-verb': trueto the check object only:
src/permissions.ts(121,3): error TS2353: Object literal may only specify known properties, and ''brand-new-verb'' does not exist in type 'Record<Action, true>'.
Both directions are caught. Record<Action, true> gets missing-key detection from the mapped type and excess-key detection from the object-literal freshness check. No asymmetry, no finding.
2. Does the union itself match the backend? — YES, verified by programmatic diff.
I parsed the Action*/Resource* constants out of internal/auth/types.go and the union members out of permissions.ts (stripping comments first — a naive quote-based regex breaks on the apostrophe in "user's scheduled purchase") and set-diffed them both ways:
ACTIONS go=20 ts=20 MATCH (in Go not TS: none; in TS not Go: none)
RESOURCES go=11 ts=11 MATCH (in Go not TS: none; in TS not Go: none)
ACTION check-object == union: MATCH
RESOURCE check-object == union: MATCH
The union is not a stale copy, so the fix has not pinned the form to outdated data. The drift has not merely moved.
3. The unknown-value path — round-trip, visibility, and injection.
-
Byte-identical round-trip: yes.
collectPermissions()(groupModals.ts:280-281) readsselect.value, the DOM property, which the parser has already entity-decoded. Confirmed by execution below. -
Visible with the select closed: yes. The appended option is the selected one, and a closed
<select>renders its selected option's label. The PR's test assertsselectedOptions[0].textContent, which is exactly that surface — the assertion matches the claim. -
Injection: closed. Verified by execution, not by reading. This was the highest-value thing to attack, given sec(frontend): escape payment field in plans and recs tables #1727 just fixed two unescaped-value XSS holes in this codebase. I wrote a throwaway probe driving five payloads through
openEditGroupModal→ save:"><img src=x onerror="window.__XSS__=1"> '><svg onload='window.__XSS__=1'> <script>window.__XSS__=1</script> " autofocus onfocus="window.__XSS__=1 a&b<c>d"e'fFor every payload: no
img/svg/script/iframein the permissions list,window.__XSS__undefined,select.valueexactly equal to the payload, the option's attribute set exactly['selected','value'](nothing smuggled in), the label rendered as text with zero child elements, and a byte-identical round-trip throughapi.updateGroup. 5/5 pass.The reason it holds:
escapeHtml(utils.ts:194) escapes& < > " '— critically including the double quote, which is what closes the attribute-breakout vector. The import ingroupModals.tsis from'../users/utils', which I checked is a bare re-export (users/utils.ts:20: export { escapeHtml } from '../utils';), i.e. the same function, not a weaker local variant. The sec(frontend): escape payment field in plans and recs tables #1727 defect is not reintroduced. See F2 for the one gap.
4. Test adequacy — the fixture genuinely exercises both directions. Verified by mutation.
Applying the fixture lesson: the risk is a fixture that cannot separate the two failure modes. Here it can. PURCHASER_PERMISSIONS contains both a droppable action (approve-any, retry-any — absent from the old 7) and a wideable resource (history — absent from the old 9). I reverted each half of the fix separately (restoring the old hardcoded list and removing that half's unknown-value fallback, since pre-fix there was none — leaving the fallback in place would have masked the mutation):
| Test | M-A: action half reverted | M-B: resource half reverted |
|---|---|---|
| 1. seeded set survives byte-identical | FAIL | FAIL |
2. approve-any/retry-any not dropped |
FAIL | PASS |
3. view:history not widened to view:* |
PASS | FAIL |
| 4. foreign pair round-trips | FAIL | FAIL |
| 5. unrecognised value visibly flagged | FAIL | FAIL |
Tests 2 and 3 are exact, complementary discriminators — each fails under precisely one half. A fix that only added the missing actions, or only the missing resources, cannot pass this suite. That is the property the #1725 fixture lacked, and this one has it.
5. Scope — confirmed. gh pr view 1730 --json closingIssuesReferences returns []: GitHub parsed no closing reference, so #1629 will not auto-close. The body states the split explicitly and points at #1550. And the fix does not depend on the backend half to be correct: it is purely client-side option rendering plus collectPermissions reading select.value; no request shape or backend behaviour changed. It stands alone.
I also checked for a second uncovered path. perm-action/perm-resource selects are rendered in exactly one place (addPermission, reached only via renderPermissions) and collected in exactly one place (collectPermissions), both fixed. The duplicate-group flow copies source.permissions straight into api.createGroup (groupModals.ts:528) without ever touching the selects, so it was never affected and still isn't.
Findings
F1 — Low/Medium · residual case of the same bug class · verified by execution
An empty-string or missing resource is still silently widened to *.
buildResourceOptions computes const isDefault = !currentValue && resource === '*', and the unknown-value append is guarded by if (currentValue && ...). So a falsy resource selects All (*) and the append never fires. Probe result against the committed code:
{ action: 'view', resource: '' } -> saved as { action: 'view', resource: '*' } FAIL
{ action: 'view' } (no resource) -> saved as { action: 'view', resource: '*' } FAIL
This is the exact escalation shape the PR fixes — a cosmetic edit silently widening a grant — just for a different input. It is reachable, not theoretical: CreateGroupAPI / UpdateGroupAPI (internal/auth/service_api.go:272 and :295) copy req.Permissions straight through apiPermissionToPermission with no validation that action/resource are non-empty or in the known vocabulary, and I found no validatePermission anywhere in internal/api or internal/auth. So {"action":"view","resource":""} is storable via the API today, and the next admin who opens that group and clicks Save turns it into view:*.
It is a chained scenario (someone must first store the malformed permission through an already-privileged endpoint), which is why this is not blocking. Two reasonable resolutions: treat a falsy-but-present resource the same as an unrecognised one (flag it rather than defaulting to *), or have #1550 reject empty action/resource on write. Worth tracking either way.
The action side does not have the mirror problem: a falsy action selects the empty placeholder and collectPermissions skips the row, which is a drop rather than a widening, and an empty action is not a meaningful grant.
F2 — Low · test coverage
Nothing pins the HTML escaping of the unknown-value option.
The escaping is correct today — I proved that with five payloads — but no test guards it. This file now renders an arbitrary stored string into both an attribute value and a text label, and the immediately preceding work on this codebase (#1727) was fixing two unescaped-value XSS holes. A single test.each over a couple of breakout payloads asserting select.value === payload, option.children.length === 0, and no injected elements would make a future refactor that swaps escapeHtml for a template or a textContent-free path fail loudly. Cheap, and it is the one assertion that would have caught #1727's class of defect. My probe is reproducible from the description above if useful.
F3 — Informational · residual seam, no change requested
The Go ↔ TS seam is still hand-maintained; the compile-time check pins the array to the union, not to Go.
Adding a constant to internal/auth/types.go without touching permissions.ts still fails nothing. I checked whether the existing generator could close this, since cmd/gen-permissions already emits permissions.generated.ts from internal/auth and is enforced by the permissions-codegen pre-commit hook with git diff --exit-code — which looked at first like the obvious reuse. It is not, and I want to be accurate about why: Go has no enumerable list of all actions/resources, only individual const declarations, so extending the generator would require adding a new hand-maintained Go slice (or AST-parsing the const block). The seam would move, not vanish. The implementer's choice is therefore defensible and I am not asking for a change here — a follow-up issue to add auth.AllActions()/AllResources() and generate the arrays would close it properly.
One wording note: the comment's "keeps there being exactly one place to update" is true frontend-side, but the real source of truth is Go and there are still two places. Worth a clause.
Proportionality
Good. No new flag, mode, or config knob; no third copy of the vocabulary. The change deletes 16 hardcoded <option> lines and replaces them with two builders reading a single derived list. The label-override maps are small and only cover the cases title-casing genuinely gets wrong (*, api-keys, ri-exchange, admin). Net +81/-17 in the modal for a fix with this blast radius is proportionate.
Residual risk
- F1 is the one live gap, and it is the same class as the bug being fixed.
- The unknown-value fallback is genuinely fail-safe rather than fail-open: it preserves rather than coerces, and flags rather than hides. That is the right default for this failure mode.
- This PR does not, and does not claim to, close sec(frontend): group edit drops unrepresentable permissions and widens their resource to * #1629 — a direct API call still bypasses the form entirely, which is what sec(auth): the #923 money separation-of-duties carve-out is voidable by any admin in one request #1550 is for. Nothing here creates a false sense that the escalation path is fully closed; the body is explicit about it and GitHub parsed no closing keyword.
The group-edit form's action/resource <select> lists were hardcoded to
7 of the backend's 20 actions and 9 of its 11 resources. A stored
permission whose action or resource matched no <option> fell back to
the browser's index-0 default: an unmatched action defaulted to the
empty "Select Action" placeholder, which collectPermissions() treats
as "skip this row" (the permission is dropped); an unmatched resource
defaulted to "All (*)" (the permission is silently widened to the
wildcard). Editing any group holding one of the missing values and
clicking Save re-submitted a materially different permission list with
no error, e.g. the seeded Purchaser group's approve-any/retry-any
verbs vanish and its view:history grant widens to view:*.
Adds ALL_ACTIONS / ALL_RESOURCES to permissions.ts, derived from the
existing hand-maintained Action/Resource closed unions (which already
mirror internal/auth/types.go) via a compile-time exhaustiveness
check, so there is exactly one place left to update when the backend
vocabulary changes. groupModals.ts now builds both <select> lists from
these instead of a third, independently drifting hardcoded copy.
As defense in depth beyond the currently-known vocabulary, a stored
permission whose value still isn't recognised gets an extra option
appended for that exact value, selected and visibly flagged ("not
recognized by this form") rather than silently coerced to a different
one, so the select always round-trips the real stored permission.
This closes the frontend half of #1629. The issue also requires a
grant-ceiling and system_managed check on UpdateGroupAPI/CreateGroupAPI
(internal/api, tracked with #1550) so the same permission list can't
be widened by a direct API call bypassing this form; that backend half
is out of scope here.
dbd6675 to
4149f6b
Compare
|
Rebased onto current |
Adversarial review of this PR confirmed the escaping in
buildActionOptions/buildResourceOptions is correct today, but nothing
guarded it going forward. Both functions interpolate a stored
permission's raw action/resource string into an HTML attribute
(value="...") and a text label ("not recognized by this form...")
whenever the value falls outside ALL_ACTIONS/ALL_RESOURCES -- the same
"API string reaching innerHTML" pattern #1727 fixed twice elsewhere in
this codebase a few hours earlier.
Adds a test.each over five breakout payloads (element injection,
attribute injection onto the <option> tag itself, a raw <script>, a
single-quote-context breakout, and a closing-tag option injection).
Each case asserts the strong properties rather than that the string
appears somewhere: no element was parsed out of the payload, the
selected <option> gained no attributes beyond value/selected, its
label has zero child elements, and the value survives byte-identical
through the DOM and through an actual save.
Verified the tests have teeth: temporarily stripped escapeHtml from
the action-side fallback option and reran -- 4 of 5 cases failed with
the exact predicted symptoms (extra attributes leaking through on the
attribute-injection payload, an extra <option> from the closing-tag
payload, etc). The fifth (single-quote breakout) correctly does not
fail either way, since the surrounding attribute is double-quoted and
a lone "'" cannot close it regardless of escaping -- kept as a
defense-in-depth case rather than dropped, since it still asserts the
escaping holds under that payload shape. Reverted the temporary
change; all 10 tests in this file pass against the real code.
|
F2 addressed (per adversarial review): added Each case asserts the strong properties the review used, not string-containment: no element parsed out of the payload ( Verified the test has teeth, not just green-by-construction: temporarily stripped New head: b3d35e6 (fast-forward from 4149f6b, no rebase needed). tsc/eslint/build clean; full suite shows the same known 8 pre-existing #1728 locale failures (unfixed on main until #1732 merges), no new failures. |
A blank resource is not a request for the "*" wildcard, but that is what it
became. The group-edit form picks its option with
`isDefault = !currentValue && resource === '*'`, so an empty stored resource
renders as the selected "All (*)" entry and saves back as view:*. Nothing
validated the list on the way in, so the same widening was reachable from
any API client with no form involved.
The grant ceiling added in the previous commit does NOT close this. Its
admin:* branch grants any (action, resource) pair that is not carved out,
and ("view", "") is not carved out, so an admin wrote a blank resource
straight through. Verified by execution before adding the guard: the write
returned nil and reached the store.
validateRequestedPermissions runs ahead of the ceiling on both write paths
and refuses blank (empty or whitespace-only) actions and resources, naming
the offending entry index. It fails before the actor lookup, so the refusal
does not depend on who is asking.
The two fields fail differently in the form, which is what shows the defect
is in the defaulting rather than the parsing: a blank action is silently
DROPPED (its index 0 is an empty placeholder) while a blank resource is
silently WIDENED (its index 0 is the wildcard). Only the resource side
escalates; both are refused, because a silently dropped permission is a
different bug rather than an acceptable one.
Unknown-but-non-blank values are deliberately still accepted: vocabulary
validation is a separate concern and rejecting values this endpoint can
already have stored would break edits of existing groups. An explicit "*"
stays a legitimate value, gated by the ceiling rather than by this check.
Mapped to 400, not 403: malformed input rather than an authorization
failure.
Refs #1730, #1550, #1629.
A blank resource is not a request for the "*" wildcard, but that is what it
became. The group-edit form picks its option with
`isDefault = !currentValue && resource === '*'`, so an empty stored resource
renders as the selected "All (*)" entry and saves back as view:*. Nothing
validated the list on the way in, so the same widening was reachable from
any API client with no form involved.
The grant ceiling added in the previous commit does NOT close this. Its
admin:* branch grants any (action, resource) pair that is not carved out,
and ("view", "") is not carved out, so an admin wrote a blank resource
straight through. Verified by execution before adding the guard: the write
returned nil and reached the store.
validateRequestedPermissions runs ahead of the ceiling on both write paths
and refuses blank (empty or whitespace-only) actions and resources, naming
the offending entry index. It fails before the actor lookup, so the refusal
does not depend on who is asking.
The two fields fail differently in the form, which is what shows the defect
is in the defaulting rather than the parsing: a blank action is silently
DROPPED (its index 0 is an empty placeholder) while a blank resource is
silently WIDENED (its index 0 is the wildcard). Only the resource side
escalates; both are refused, because a silently dropped permission is a
different bug rather than an acceptable one.
Unknown-but-non-blank values are deliberately still accepted: vocabulary
validation is a separate concern and rejecting values this endpoint can
already have stored would break edits of existing groups. An explicit "*"
stays a legitimate value, gated by the ceiling rather than by this check.
Mapped to 400, not 403: malformed input rather than an authorization
failure.
Refs #1730, #1550, #1629.
Delta review,
|
| payload | assertion that fired | mechanism |
|---|---|---|
"><img src=x onerror=alert(1)> |
attributes (:262) |
"> terminated the tag early, so the selected attribute was lost — received ["value"] |
" autofocus onfocus=alert(1) x=" |
attributes (:262) |
real attribute smuggling: autofocus, onfocus, x added to the live <option> |
<script>alert(1)</script> |
element count (:245) |
a <script> element was actually parsed into the DOM |
</option><option value="x" selected>evil</option> |
option count (:251) |
an extra <option> smuggled in, 23 vs 22 |
The second is a genuine live XSS vector (autofocus onfocus= on a rendered option). The fourth is the most consequential one for a permissions form specifically: an injected selected option changes what the operator saves. So the matrix covers attribute loss, attribute injection, element injection and option injection as four separate failure modes, not one.
The commit's disclosure about payload 5 is accurate, and disclosing it rather than quietly keeping a decorative case is the right call.
One mutation the commit did not claim: the resource side is equally covered
The commit only reports mutating the action-side fallback. buildResourceOptions:193 has the identical construction. I mutated that one instead:
✕ element injection ✕ attribute injection ✕ raw script tag
✓ single-quote breakout ✕ closing-tag option injection
Tests: 4 failed, 6 passed, 10 total
Same 4/5. Both functions are guarded, not just the one that was tested. Worth knowing, since the two are independent copies of the same pattern and a future edit could touch either.
Accuracy note: the stated reason payload 5 is inert is incomplete
The commit explains payload 5 ('><svg onload=alert(1)>) as inert because "the surrounding attribute is double-quoted and a lone ' cannot close it regardless of escaping". That is correct for the attribute context, but the same value is also interpolated into the text label (⚠ ${escapeHtml(currentValue)} (not recognized...)), and there a raw <svg onload=...> would not need to close any quote. The reason it still does not fire is the HTML parser's in-select insertion mode, which drops <svg> inside a <select> subtree.
The same effect explains something the table above makes visible: payload 1's <img> is also never parsed, so querySelectorAll('img, script, svg, iframe, style') at :245 only ever fires for script. Payload 1 is caught by the lost selected attribute instead, despite being named "element injection".
None of this weakens the suite — every payload that can produce an observable effect is caught, by an assertion that genuinely discriminates. But the img, svg, iframe, style members of that selector are inert in this context, and the payload-5 rationale as written would mislead the next person deciding whether a new payload is worth adding. A sentence noting the in-select parsing behaviour would fix both.
Widening direction is covered
Checked specifically, since widening is the security direction for a permissions control and a suite that only guards against dropping would miss it. It is covered on both sides: the pre-existing view:history specifically is not widened to view:* case names the exact escalation (leaking users/groups/config/accounts read access), and every F2 case asserts the payload reaches updateGroup byte-identically rather than merely surviving, so a value silently rewritten to something broader would fail the save assertion, not just the DOM ones.
Verdict
The rebase is clean, the F2 addition is well-constructed, and its one decorative case is disclosed rather than hidden. No blocking findings. The only thing I would change is the payload-5 rationale in the test comment, which is right about the attribute context and silent about the label context that actually decides it.
Execution-verified: the patch comparison, both mutation runs (action-side and resource-side) with per-assertion failure attribution, and the unmutated 10/10 pass. Reading-derived: the in-select parsing explanation, and the assessment that js-yaml/nanoid are unreachable from these paths.
An allowed_accounts-only PUT never reached the ceiling at all.
checkGrantCeiling opens with `if len(requested) == 0 { return nil }`, and
APIUpdateGroupRequest's "empty means not sent" contract makes an
accounts-only request the natural shape to send. Verified by execution with
an actor holding only update:groups in one group scoped to one account:
widening to more accounts, to [] and to ["*"] were all ACCEPTED, while the
control -- widening Permissions[].Constraints.AccountIDs on the same call --
was correctly refused. One account dimension was bounded carefully and its
sibling left open on the same request.
checkAccountCeiling is deliberately a separate call rather than a branch
inside checkGrantCeiling, so it cannot inherit that early return. A write is
in ceiling if it does not widen the group's existing scope, or if it stays
inside the acting principal's own.
Two traps are handled explicitly. Empty and "*" both mean UNRESTRICTED, so
this cannot be a subset test: the empty set is a subset of everything and
means the opposite of narrow. And on create there is no prior scope, where a
nil existing would read as unrestricted and swallow every check -- so
CreateGroupAPI calls checkAccountGrant directly, and an omitted
allowed_accounts is checked too, because it produces an unrestricted group.
Every refusal test sends allowed_accounts with no permissions key. A test
that included permissions would pass with the bug present, since the
permission ceiling would then run and refuse for an unrelated reason.
Also strengthens three mutation kills that rested on a missing stub rather
than on an assertion. M5, M7 and TestUpdateUser_SelfEscalationDenied panicked
on an unstubbed READ downstream of the guard, a kill that disappears the
moment someone adds a permissive stub while tidying fixtures. Those now
register the downstream reads with .Maybe() so removing the guard cannot
panic, leaving the test's own assertions as the only thing that can fail it;
M7 now dies on require.Error. FailsClosedOnGroupLoadError asserts
errors.Is(err, loadErr) rather than message text.
Splits group_ceiling_test.go, which exceeded the repo's 500-line guideline,
by concern rather than by line count: fixtures, the permission ceiling,
blank-field validation, and system_managed. Verified no test was lost -- 707
passing test names before and after, sorted and identical.
Closes #1738.
Refs #1550, #1629, #1730.
A blank resource is not a request for the "*" wildcard, but that is what it
became. The group-edit form picks its option with
`isDefault = !currentValue && resource === '*'`, so an empty stored resource
renders as the selected "All (*)" entry and saves back as view:*. Nothing
validated the list on the way in, so the same widening was reachable from
any API client with no form involved.
The grant ceiling added in the previous commit does NOT close this. Its
admin:* branch grants any (action, resource) pair that is not carved out,
and ("view", "") is not carved out, so an admin wrote a blank resource
straight through. Verified by execution before adding the guard: the write
returned nil and reached the store.
validateRequestedPermissions runs ahead of the ceiling on both write paths
and refuses blank (empty or whitespace-only) actions and resources, naming
the offending entry index. It fails before the actor lookup, so the refusal
does not depend on who is asking.
The two fields fail differently in the form, which is what shows the defect
is in the defaulting rather than the parsing: a blank action is silently
DROPPED (its index 0 is an empty placeholder) while a blank resource is
silently WIDENED (its index 0 is the wildcard). Only the resource side
escalates; both are refused, because a silently dropped permission is a
different bug rather than an acceptable one.
Unknown-but-non-blank values are deliberately still accepted: vocabulary
validation is a separate concern and rejecting values this endpoint can
already have stored would break edits of existing groups. An explicit "*"
stays a legitimate value, gated by the ceiling rather than by this check.
Mapped to 400, not 403: malformed input rather than an authorization
failure.
Refs #1730, #1550, #1629.
An allowed_accounts-only PUT never reached the ceiling at all.
checkGrantCeiling opens with `if len(requested) == 0 { return nil }`, and
APIUpdateGroupRequest's "empty means not sent" contract makes an
accounts-only request the natural shape to send. Verified by execution with
an actor holding only update:groups in one group scoped to one account:
widening to more accounts, to [] and to ["*"] were all ACCEPTED, while the
control -- widening Permissions[].Constraints.AccountIDs on the same call --
was correctly refused. One account dimension was bounded carefully and its
sibling left open on the same request.
checkAccountCeiling is deliberately a separate call rather than a branch
inside checkGrantCeiling, so it cannot inherit that early return. A write is
in ceiling if it does not widen the group's existing scope, or if it stays
inside the acting principal's own.
Two traps are handled explicitly. Empty and "*" both mean UNRESTRICTED, so
this cannot be a subset test: the empty set is a subset of everything and
means the opposite of narrow. And on create there is no prior scope, where a
nil existing would read as unrestricted and swallow every check -- so
CreateGroupAPI calls checkAccountGrant directly, and an omitted
allowed_accounts is checked too, because it produces an unrestricted group.
Every refusal test sends allowed_accounts with no permissions key. A test
that included permissions would pass with the bug present, since the
permission ceiling would then run and refuse for an unrelated reason.
Also strengthens three mutation kills that rested on a missing stub rather
than on an assertion. M5, M7 and TestUpdateUser_SelfEscalationDenied panicked
on an unstubbed READ downstream of the guard, a kill that disappears the
moment someone adds a permissive stub while tidying fixtures. Those now
register the downstream reads with .Maybe() so removing the guard cannot
panic, leaving the test's own assertions as the only thing that can fail it;
M7 now dies on require.Error. FailsClosedOnGroupLoadError asserts
errors.Is(err, loadErr) rather than message text.
Splits group_ceiling_test.go, which exceeded the repo's 500-line guideline,
by concern rather than by line count: fixtures, the permission ceiling,
blank-field validation, and system_managed. Verified no test was lost -- 707
passing test names before and after, sorted and identical.
Closes #1738.
Refs #1550, #1629, #1730.
…writes (#1737) * sec(auth): enforce a grant ceiling and system-managed guard on group writes CreateGroupAPI / UpdateGroupAPI wrote the client-supplied permission list onto a group verbatim, with no check that the caller may grant what they are granting and no consultation of the system_managed column. Because update:groups is not one of the pairs carved out of the admin:* wildcard, any admin could void the #923 money separation-of-duties control tenant-wide in a single request: PUT /api/groups/<administrators> {"permissions":[{admin,*},{execute,purchases},{approve-any,purchases}, {retry-any,purchases}]} Two rules now gate every group-permission write: 1. Ceiling: a caller may only grant permissions their own effective set already holds, matched through the same carve-out-aware logic used at enforcement time, and at constraints no broader than their own (a holder capped at $100 cannot hand out an uncapped grant). 2. Non-grantable: the three money verbs in adminCarvedOuts may never be ADDED to a group, whoever the caller is. This is load-bearing rather than belt-and-braces: migrations 000059/000064 backfill every Administrators member into the Purchaser group, so a default-deployment admin explicitly holds those verbs and rule 1 alone would let them relay the verbs onto the Administrators group. A carved-out permission already stored on the target group may be carried through an unrelated edit, but not widened, so a rename is not forced to strip it. Refusals fail closed and name the offending permission; the list is never silently narrowed to the allowed subset, which is the corruption mode #1629 reports on the frontend side. An unidentified actor, or any error resolving the actor's permissions, refuses the write. The stateless admin API key has no user row, so the ceiling measures it against a bare {admin, *} holding. It can still seed ordinary groups but is now subject to the same money carve-out as a human admin, closing the third vector in the report. system_managed is enforced on update and on delete. Delete is the third write path to a group's permissions and was named in neither issue: dropping the seeded Purchaser group destroys the only holder of the carved-out verbs, which nothing can then re-grant, so the purchase path would be dead tenant-wide. CreateGroupAPI / UpdateGroupAPI now take the acting principal, matching UpdateUserAPI's existing shape. updateGroup previously discarded its session. Refs #1550, #1629. * sec(auth): reject blank action or resource in a group permission write A blank resource is not a request for the "*" wildcard, but that is what it became. The group-edit form picks its option with `isDefault = !currentValue && resource === '*'`, so an empty stored resource renders as the selected "All (*)" entry and saves back as view:*. Nothing validated the list on the way in, so the same widening was reachable from any API client with no form involved. The grant ceiling added in the previous commit does NOT close this. Its admin:* branch grants any (action, resource) pair that is not carved out, and ("view", "") is not carved out, so an admin wrote a blank resource straight through. Verified by execution before adding the guard: the write returned nil and reached the store. validateRequestedPermissions runs ahead of the ceiling on both write paths and refuses blank (empty or whitespace-only) actions and resources, naming the offending entry index. It fails before the actor lookup, so the refusal does not depend on who is asking. The two fields fail differently in the form, which is what shows the defect is in the defaulting rather than the parsing: a blank action is silently DROPPED (its index 0 is an empty placeholder) while a blank resource is silently WIDENED (its index 0 is the wildcard). Only the resource side escalates; both are refused, because a silently dropped permission is a different bug rather than an acceptable one. Unknown-but-non-blank values are deliberately still accepted: vocabulary validation is a separate concern and rejecting values this endpoint can already have stored would break edits of existing groups. An explicit "*" stays a legitimate value, gated by the ceiling rather than by this check. Mapped to 400, not 403: malformed input rather than an authorization failure. Refs #1730, #1550, #1629. * sec(auth): block self-granting carved-out money verbs via group membership #1550's report names a one-request alternative that needs no group edit at all: PUT /api/users/{self} adding DefaultPurchaserGroupID. The #907 self-escalation guard gates self-added groups on update:users, which admin:* grants, so it passed. An admin could join the Purchaser group and pick up execute / approve-any / retry-any on purchases in a single request, voiding the #923 separation of duties exactly as writing those verbs onto their own group would. Closing only the group-permission write path would have left this open while the issue auto-closed over it. guardSelfCarvedOutGrant applies the grant ceiling's own rule to membership: you cannot grant yourself a carved-out verb you do not already hold. It keys off the permission rather than off DefaultPurchaserGroupID, so a custom group carrying a money verb is blocked identically. Deliberately still allowed, each with a negative-control test: adding a second group carrying a verb already held (not an escalation); an admin adding ANOTHER user to Purchaser (the two-person control separation of duties exists to create); and trusted internal callers with an empty actor, so bootstrap and seeding paths are unaffected. Also fixes a latent fragility this surfaced. The guard resolved the actor's permissions by re-reading their row, but applyUpdateUserRequest has already mutated the in-memory user by that point; it gave the right answer only because the write had not been committed yet. A pre-existing test had to hand back a distinct unmutated copy on the second read to avoid aliasing the just-mutated object. guardSelfEscalation now resolves permissions from the prior membership snapshot, which is the question a self-escalation guard has to ask anyway, so the second read and the test's workaround are both gone. GetUserPermissions delegates to the new permissionsForGroups helper rather than duplicating the group-walk loop. Refs #1550, #923, #907. * docs(auth): record why guardSelfEscalation reads the prior membership The shorter spelling of this guard is to re-read the actor's row, and that spelling is a silent regression: UpdateUser calls applyUpdateUserRequest before the guards run, so the in-memory user already carries the new membership and a re-read returns the old values only because the write has not been committed yet. A future caller that passes the mutated user makes the guard authorize the escalation it exists to block. Record that on the function so the next reader does not "simplify" it back, and point at the mutation that enforces it: swapping prior for next fails the suite. Refs #1550. * sec(auth): bound AllowedAccounts on group writes, the fifth write path An allowed_accounts-only PUT never reached the ceiling at all. checkGrantCeiling opens with `if len(requested) == 0 { return nil }`, and APIUpdateGroupRequest's "empty means not sent" contract makes an accounts-only request the natural shape to send. Verified by execution with an actor holding only update:groups in one group scoped to one account: widening to more accounts, to [] and to ["*"] were all ACCEPTED, while the control -- widening Permissions[].Constraints.AccountIDs on the same call -- was correctly refused. One account dimension was bounded carefully and its sibling left open on the same request. checkAccountCeiling is deliberately a separate call rather than a branch inside checkGrantCeiling, so it cannot inherit that early return. A write is in ceiling if it does not widen the group's existing scope, or if it stays inside the acting principal's own. Two traps are handled explicitly. Empty and "*" both mean UNRESTRICTED, so this cannot be a subset test: the empty set is a subset of everything and means the opposite of narrow. And on create there is no prior scope, where a nil existing would read as unrestricted and swallow every check -- so CreateGroupAPI calls checkAccountGrant directly, and an omitted allowed_accounts is checked too, because it produces an unrestricted group. Every refusal test sends allowed_accounts with no permissions key. A test that included permissions would pass with the bug present, since the permission ceiling would then run and refuse for an unrelated reason. Also strengthens three mutation kills that rested on a missing stub rather than on an assertion. M5, M7 and TestUpdateUser_SelfEscalationDenied panicked on an unstubbed READ downstream of the guard, a kill that disappears the moment someone adds a permissive stub while tidying fixtures. Those now register the downstream reads with .Maybe() so removing the guard cannot panic, leaving the test's own assertions as the only thing that can fail it; M7 now dies on require.Error. FailsClosedOnGroupLoadError asserts errors.Is(err, loadErr) rather than message text. Splits group_ceiling_test.go, which exceeded the repo's 500-line guideline, by concern rather than by line count: fixtures, the permission ceiling, blank-field validation, and system_managed. Verified no test was lost -- 707 passing test names before and after, sorted and identical. Closes #1738. Refs #1550, #1629, #1730. * sec(auth): fail closed when the actor's account scope cannot be resolved grantCeilingAccounts read the acting principal's scope from BuildAuthContext and returned it unconditionally. collectGroupsAndAccounts skips a missing or deleted group silently -- pgx.ErrNoRows, or a store returning (nil, nil) -- so an actor whose groups all failed to load produced an EMPTY account list, which IsUnrestrictedAccess reads as "all accounts". The account ceiling became a no-op on exactly the path it guards. Verified by execution across the four resolution-failure modes. A store error on the user and a missing user row both already failed closed; the two group-resolution failures did not: store error on user -> refused user missing (nil, nil) -> refused actor's only group missing (ErrNoRows) -> ACCEPTED widening to ["*"] actor's only group returns (nil, nil) -> ACCEPTED widening to ["*"] Note the asymmetry this closes: the PERMISSION ceiling already failed closed on the same input, because an empty permission set grants nothing. Only the account ceiling failed open, which is the harder direction to notice because nothing errors. Requiring at least one resolved group is the precise guard. Partial resolution still under-reports the actor's scope, which makes the ceiling stricter rather than looser, so only total failure needed closing. Refs #1550, #1738. * sec(auth): refuse a partial actor resolution that widens the grant ceiling grantCeilingAccounts closed only TOTAL resolution failure, on the premise that partial resolution under-reports the actor's scope and is therefore stricter. That premise is false for AllowedAccounts, which is a UNION in which the empty set means EVERYTHING: dropping a contributing group does not narrow it. The union of [] and ["acct-A"] is restricted; lose the group carrying ["acct-A"] and it collapses to [], and the actor may then widen any group to ["*"]. Verified by execution against the write path: baseline (both groups resolve) REFUSED widening to [*] PARTIAL (scoping group ErrNoRows) ACCEPTED widening to [*] The configuration needs TWO groups -- one granting update:groups with no allowed_accounts, one carrying the restriction -- which is why the earlier six-case single-group verification could not find it. The guard is now: refuse when the union is empty AND at least one group was skipped. The emptiness test is len(AllowedAccounts) == 0, deliberately NOT IsUnrestrictedAccess: a union containing "*" was already maximally wide at baseline, so no loss can widen it, and refusing it would be zero security benefit and pure availability cost. All seven seeded groups ship allowed_accounts = ARRAY['*'], so the broader predicate would have refused every seeded-group member with one stale membership. Both directions are mutation-verified and each is guarded by exactly one test: widening the predicate kills only WildcardActorToleratesSkippedGroup; removing the guard kills only PartialActorResolutionThatWidensIsRefused. Neither would catch the other's defect. The total-failure case is the degenerate partial one -- every group skipped -- so it is now caught by the skipped-group guard and carries that message. The existing test asserted the other guard's exact wording; its assertion is relaxed to the sentinel plus a substring true of both. NOTE ON OVERLAP: AuthContext.SkippedGroups and the counting in collectGroupsAndAccounts are also added by PR #1752 for the read path. The two changes are identical; whichever merges second should see no divergence. Refs #1550, #1748. * fix(auth): make ceiling comparisons exact, and land the third .Maybe() fix F2 -- two comparison helpers were MORE permissive than the enforcement their comments claimed to mirror. accountScopeGap documented "comparison is exact, matching MatchesAccount" but trimmed both sides. MatchesAccount is `a == accountID`: no trim, no fold. An actor whose stored scope is " acct-A" matches nothing at enforcement, yet the ceiling treated them as holding "acct-A" and let them grant it. listCovers documented values as "trimmed and lower-cased, matching matchAllRegionsConstraint" -- true for Regions only. AccountIDs, Providers and Services are enforced by containsAny, an exact case-sensitive lookup, so normalising made the ceiling looser there: a holder constrained to ["ACCT-1"] could grant ["acct-1"], a value they cannot themselves use. Both are now exact everywhere. Exact is stricter than enforcement on Regions rather than looser, so it fails closed. The rule now stated in both comments: a ceiling may exceed enforcement in strictness, never fall short. This leaves normalizeConstraintValue with no callers, so it is removed. F3 -- the third .Maybe() strengthening had landed in the wrong test. The stubs went to TestGroupOnlyAuthz_NonAdminDenied, which calls only HasPermission and UserHasAdminCapability: it never writes, never adds a group, never touches DefaultAdminGroupID, and has no AssertNotCalled for the comment to refer to. Both stubs were dead, and TestUpdateUser_SelfEscalationDenied -- the test that needed them -- was still killed by a panic on an unstubbed UpdateUser, the exact weakness the change claimed to remove. Cause: the anchor `On("GetGroup", ctx, viewerGroup().ID)` appears in three tests in that file, and a first-match replace put the stubs in the first one. The same first-match error recurred on the first attempt to fix it; the stubs are now placed by locating the target function's body explicitly. Verified by mutation rather than by the test passing: with the self-escalation guard removed, TestUpdateUser_SelfEscalationDenied is now killed by an assertion (killed-by-PANIC false, killed-by-ASSERTION true), where before the move it was killed by a panic. Refs #1550, #1748.
Group edit dropped constraints.accounts, widening the permission PermissionConstraints carries five dimensions and they all round-trip through the API, but the group edit form rendered inputs for four. constraints.accounts had nowhere to live and collectPermissions never read one back, so every save dropped it. That is a widening, not data loss. matchStringListConstraints (internal/auth/service_group.go:414) returns true whenever either list is empty, so an empty AccountIDs means no restriction on that dimension. A permission stored as "manage any scheduled purchase, but only in acct-prod-1" came back from a cosmetic rename as "manage any scheduled purchase, in every cloud account". The backend ceiling did not catch it because grantCeilingAllows short-circuits on the caller's admin:*, whose own comment says it covers any requested constraint set. The same gap failed closed in the other direction: a constrained carved-out money verb made its group uneditable, erroring on every save including a pure rename, because permissionCoveredBy could not cover a constraint set the form never sent. Representation fixes both halves. The issue's own direction, "so the form can represent everything the backend can store", was satisfied on the action/resource axis by #1730 and #1737 in August. #1629 stayed open only because #1730 deliberately omitted a closing keyword pending #1550, and nobody closed it after #1737 merged. This finishes the constraints axis. Blank and whitespace-only constraint entries are now refused in validateRequestedPermissions, alongside the existing blank action and resource rule in group_ceiling.go. This closes the class for every client including the API rather than the form alone, and the error names which permission and which constraint list carries the blank, since a generic refusal on an authorization path is something operators work around rather than fix. Review found the same widening reachable from two further directions, both now refused with one rule and one error path: A group already storing a blank entry renders as an empty box, parses back to absent, and would send no constraint at all, so the backend would never see a blank to reject. Such a row is now detected when it renders and the save is refused. Typed input is the more reachable half: "," or " , " is non-empty in the box but parseConstraintList reduces it to nothing, and the save would send an empty list. It takes a stray comma. Also refused. Both use parseConstraintList itself as the predicate rather than a second implementation of "parses to nothing" that could drift from it, and both read the same LIST_CONSTRAINT_DIMENSIONS list so the render-time check and the typed-input check cannot cover different sets. A blank box remains ordinary and saves normally, measured after trimming, since a field that looks empty must not be unfixable from the UI. Verified: the frontend regression test drives the real submit path. It builds the #group-form markup from index.html with no novalidate, installs the app's own submit listener via setupGroupHandlers, and saves by clicking the submit button. One test exists purely to pin that the harness is honest, clearing the required name input and asserting updateGroup is never called; without it a fix that never runs would pass every other test in the block. Confirmed failing pre-fix by reverting groupModals.ts to main: 6 failed, 11 passed. Backend blank-entry subtests fail pre-fix 10 of 10 with "An error is expected but got nil". The Go comment claiming this makes the encoding lossless for every value the system can store was corrected during review: " acct A " is legitimate and the form still rewrites it to "acct A". The comment now states what the guard does and what it does not. Not verified: no browser. The new accounts input is reasoned from the CSS to render full-width on its own form row, not seen. No end-to-end HTTP against a running server. Whether any deployment stores a populated constraints.accounts is unknown; if none does the frontend half is preventive, though the widening is reachable either way and the uneditable-group half bites regardless. CodeRabbit reviewed two earlier heads, each with one actionable comment, both fixed here. It was rate-limited on the final head, so that delta, 152 lines across two files, was reviewed by hand rather than by spending another attempt. Deferred: a group already storing a blank entry is now refused rather than repaired, so if any deployment is found carrying one it needs a repair migration; the follow-up carries the detection query. #1873 covers a group holding a blank permission resource being unsavable with no explanation.
1 participant
Summary
This closes only the frontend half of #1629. The issue explicitly requires two halves ("Two halves, both needed. Please do not ship only the first"): this PR fixes the Admin UI's silent data corruption, but
UpdateGroupAPI/CreateGroupAPI(internal/api) still has no grant ceiling and nosystem_managedcheck, so a direct API call bypassing this form can still widen or escalate a group's permissions. That backend half is tracked in #1550 (open, unassigned), out of scope here and outside this PR's footprint (frontend/src/**). Not using a closing keyword; #1629 should stay open until #1550 also lands.The bug
groupModals.ts'saddPermission()hardcoded the action<select>to 7 of the backend's 20 actions and the resource<select>to 9 of its 11 resources (internal/auth/types.go). When a stored permission's value matched no<option>, the browser fell back to index 0 of each select:Select Actionplaceholder.collectPermissions()treats a falsy action as "skip this row" -> the permission is silently dropped.All (*). -> the permission is silently widened to the wildcard.Concretely, on the seeded Purchaser group (
PURCHASER_PERMS):approve-any:purchasesandretry-any:purchasesvanish entirely (both are carved out ofadmin:*, so no admin can approve or retry a purchase either until someone re-seeds the group by hand), andview:historysilently becomesview:*— every Purchaser gains read access to users, groups, cloud accounts and config. An admin who opens that group, changes only the description, and clicks Save performs an unintended privilege escalation without any error or warning. This is the sentence that justifies HIGH severity: the failure isn't just data loss, it's a widening path triggered by a cosmetic edit.The fix
frontend/src/permissions.tsalready hand-maintains closed-unionAction/ResourceTS types mirroringinternal/auth/types.go's constants (used bycanAccess/isAdminelsewhere). Rather than hardcoding a third independently-drifting copy of the vocabulary ingroupModals.ts— which is how this bug happened — this addsALL_ACTIONS/ALL_RESOURCESruntime exports derived from those same union types, via a compile-time exhaustiveness check (Record<Action, true>/Record<Resource, true>literals: TS rejects the assignment if a member is missing or extra).groupModals.tsnow builds both<select>option lists from these. One source of truth; future drift between the union type and this array fails the build instead of silently reintroducing the bug.Defense in depth beyond the currently-known vocabulary: if a stored permission's value still isn't recognised (a future backend verb the frontend hasn't been taught yet, or legacy/foreign data), an extra
<option>is appended for that exact value — selected, and visibly labeled⚠ <value> (not recognized by this form). Because it's the selected option, that warning text is visible in the closed<select>itself, not just on dropdown-open. The permission still round-trips unchanged throughcollectPermissions()instead of being coerced to a different value. This never drops, never widens, never silently hides, and never blocks saving an unrelated change.Regression tests
New file
frontend/src/__tests__/group-edit-unrepresentable-permissions.test.ts(5 tests), using the real seededPURCHASER_PERMSfixture:api.updateGroupis byte-identical to the input (order and all).approve-any:purchases/retry-any:purchasesare not dropped.view:historyis not widened toview:*.<option>carries the visible "not recognized" warning text, confirmed viaselectedOptions[0].textContent.Verified fail-then-pass: stashed just the two source-file edits (kept the new tests), ran against pre-fix code — all 5 failed exactly as predicted:
approve-anyandretry-anymissing from the saved permissions arrayview:history->view:*in the saved array[]Restored the fix: all 5 pass. All 95 pre-existing
groups.test.tstests also still pass (100/100 combined).Test plan
npx tsc --noEmit— cleannpx eslinton changed files — cleannpx jestfull suite — no new failures (8 pre-existing failures inriexchange.test.ts/utils.test.tsare the knowntoLocaleString()locale mismatch, tracked separately as fix(test/frontend): 8 tests assert the host locale's thousands separator, so they fail locally and pass in CI #1728; unrelated to this diff)npm run build— clean production buildPart of #1629