Skip to content

Commit 1e4fe6a

Browse files
MaxBrychclaude
andcommitted
fix(web): unrecognized org roles fail closed, and the package testsuite learns canWrite
Two Criticals from review: 1. Fail-open: resolveScope's org branch gated the ACL check on hasOrgAccess (prefix match only, "org:<accountId>:") but derived canWrite from orgRole (which validates the role suffix against ORG_ROLES). A claim with an unrecognized role, e.g. "org:acc-7:contractor", passed hasOrgAccess, made orgRole return null, and null !== "member" evaluated to true — an unparseable role got write access. Fixed by resolving the role once and gating on role === null (thrown as the same "forbidden" WorkspaceAuthError), so an unrecognized role loses read too, not just write. canWrite is now literally role !== "member" over a role proven non-null. hasOrgAccess is no longer imported into context.ts (still exported and independently tested in session.ts). 2. CI break: making WorkspaceScope.canWrite required broke `pnpm --filter @netizen-labs/workspace typecheck` — 15 TS2741/ TS2345 errors across the package's own test suite, which builds scope literals directly. Added canWrite to every one. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent 2a62e31 commit 1e4fe6a

7 files changed

Lines changed: 47 additions & 15 deletions

File tree

apps/web/src/lib/workspace/context.ts

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,6 @@ import { fetchUserinfo, groupsFrom, refreshTokens } from "./oidc";
1313
import { createSessionStore } from "./session-store";
1414
import {
1515
ORG_ROLES,
16-
hasOrgAccess,
1716
isCitizenSession,
1817
isExpired,
1918
orgGroupId,
@@ -122,7 +121,18 @@ export async function resolveScope(params: {
122121
if (!params.accountId) {
123122
throw new WorkspaceAuthError("forbidden", "an org scope needs an account id");
124123
}
125-
if (!hasOrgAccess(params.session, params.accountId)) {
124+
// Resolved once, and used as the sole gate: `orgRole` returns null both
125+
// when there is no `org:<accountId>:*` claim at all AND when a claim is
126+
// present but its role suffix is not one of ORG_ROLES (e.g. a stale or
127+
// malformed "org:acc-7:contractor"). The former used to be caught by
128+
// `hasOrgAccess`, which checks the prefix alone and does not validate the
129+
// suffix — so an unrecognized role passed that gate, fell through to
130+
// `orgRole(...) !== "member"`, and got `canWrite: true` from a role this
131+
// codebase does not otherwise recognise. Gating on `role === null` instead
132+
// fails closed for that case too: an unparseable role loses read access as
133+
// well as write, deliberately, rather than defaulting to either.
134+
const role = orgRole(params.session, params.accountId);
135+
if (role === null) {
126136
throw new WorkspaceAuthError(
127137
"forbidden",
128138
`no group claim for org ${params.accountId}`,
@@ -134,9 +144,9 @@ export async function resolveScope(params: {
134144
accountId: params.accountId,
135145
folderName: orgFolderMount(params.accountId),
136146
// owner/admin write, member reads — the same mapping Task 11 applies to
137-
// Nextcloud's own ACL. `hasOrgAccess` above already proved a claim
138-
// exists, so `orgRole` cannot be null here.
139-
canWrite: orgRole(params.session, params.accountId) !== "member",
147+
// Nextcloud's own ACL. `role` is proven non-null above, so this is
148+
// exactly an owner/admin allow-list.
149+
canWrite: role !== "member",
140150
};
141151
}
142152

apps/web/tests/workspace-context.test.ts

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -91,7 +91,24 @@ describe("resolveScope", () => {
9191
);
9292
});
9393

94-
// resolveScope delegates the ACL check to hasOrgAccess, so the same
94+
// THE FAIL-OPEN REGRESSION: orgRole is the sole gate now, precisely because
95+
// it validates the role suffix and a prefix-only check does not. A claim
96+
// with a role outside ORG_ROLES must be refused entirely — not granted
97+
// read via the old prefix check and then write via `role !== "member"`,
98+
// which is what happened when resolveScope gated on hasOrgAccess (prefix
99+
// only) and computed canWrite from orgRole (suffix-validating) separately:
100+
// hasOrgAccess passed, orgRole returned null, and `null !== "member"` was
101+
// true — an unrecognized role got write access.
102+
it("refuses (not read, not write) a claim whose role is not in ORG_ROLES", async () => {
103+
const contractor = { ...session, groups: ["org:acc-7:contractor"] };
104+
await assert.rejects(
105+
() => resolveScope({ session: contractor, scopeKind: "org", accountId: "acc-7", orgName: null }),
106+
(err: unknown) => err instanceof WorkspaceAuthError && err.reason === "forbidden",
107+
);
108+
});
109+
110+
// resolveScope's org branch now gates on `orgRole` (which validates the
111+
// role suffix, not just the "org:<accountId>:" prefix), so the same
95112
// trailing-colon property has to hold one layer up: a claim for org
96113
// "acc-70" must not unlock the scope for org "acc-7".
97114
it("refuses an org whose claim is a numeric prefix of the requested id (acc-70 claim vs acc-7 request)", async () => {

packages/workspace/test/nextcloud.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ import {
1010
import type { WorkspaceScope } from "../src/types";
1111

1212
const SUB = "0xabc";
13-
const scope: WorkspaceScope = { kind: "personal", sub: SUB };
13+
const scope: WorkspaceScope = { kind: "personal", sub: SUB, canWrite: true };
1414

1515
const LISTING = `<?xml version="1.0"?>
1616
<d:multistatus xmlns:d="DAV:">

packages/workspace/test/propfind.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -227,7 +227,7 @@ describe("parsePropfind", () => {
227227
// It did not: it decoded a second time, so a name containing "%" either threw
228228
// or resolved to a different file. These pin the round trip end to end.
229229
describe("propfind -> resolvePath round trip (the encoding contract)", () => {
230-
const scope: WorkspaceScope = { kind: "personal", sub: "0xabc" };
230+
const scope: WorkspaceScope = { kind: "personal", sub: "0xabc", canWrite: true };
231231

232232
function listingOf(...encodedNames: string[]): string {
233233
const rows = encodedNames

packages/workspace/test/provenance.test.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ const scope: WorkspaceScope = {
1313
sub: "0xabc",
1414
accountId: "acc-7",
1515
folderName: "Org Feuerwehr",
16+
canWrite: true,
1617
};
1718

1819
describe("buildAction", () => {
@@ -53,7 +54,7 @@ describe("buildAction", () => {
5354
const action = buildAction({
5455
actor: { kind: "human", sub: "0xabc" },
5556
kind: "delete",
56-
scope: { kind: "personal", sub: "0xabc" },
57+
scope: { kind: "personal", sub: "0xabc", canWrite: true },
5758
path: "alt.odt",
5859
now: new Date("2026-07-28T09:00:00Z"),
5960
});

packages/workspace/test/scope.test.ts

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -9,12 +9,13 @@ import {
99
} from "../src/scope";
1010

1111
const SUB = "0x1111111111111111111111111111111111111111";
12-
const personal: WorkspaceScope = { kind: "personal", sub: SUB };
12+
const personal: WorkspaceScope = { kind: "personal", sub: SUB, canWrite: true };
1313
const org: WorkspaceScope = {
1414
kind: "org",
1515
sub: SUB,
1616
accountId: "acc-7",
1717
folderName: "Org Feuerwehr",
18+
canWrite: true,
1819
};
1920

2021
describe("scopeRoot", () => {
@@ -31,7 +32,7 @@ describe("scopeRoot", () => {
3132

3233
it("refuses an org scope with no folder name rather than falling back to the home", () => {
3334
assert.throws(
34-
() => scopeRoot({ kind: "org", sub: SUB, accountId: "acc-7" }),
35+
() => scopeRoot({ kind: "org", sub: SUB, accountId: "acc-7", canWrite: true }),
3536
ScopeViolationError,
3637
);
3738
});
@@ -153,7 +154,7 @@ describe("scopeRoot — component validation", () => {
153154
for (const badSub of badComponents) {
154155
it(`rejects a sub of ${JSON.stringify(badSub)}`, () => {
155156
assert.throws(
156-
() => scopeRoot({ kind: "personal", sub: badSub }),
157+
() => scopeRoot({ kind: "personal", sub: badSub, canWrite: true }),
157158
ScopeViolationError,
158159
);
159160
});
@@ -177,6 +178,7 @@ describe("scopeRoot — component validation", () => {
177178
sub: SUB,
178179
accountId: "acc-7",
179180
folderName: badFolderName,
181+
canWrite: true,
180182
}),
181183
ScopeViolationError,
182184
);
@@ -193,7 +195,7 @@ describe("resolvePath — component validation", () => {
193195
for (const badSub of badComponents) {
194196
it(`rejects a sub of ${JSON.stringify(badSub)} before resolving any path`, () => {
195197
assert.throws(
196-
() => resolvePath({ kind: "personal", sub: badSub }, "Dokumente/Antrag.odt"),
198+
() => resolvePath({ kind: "personal", sub: badSub, canWrite: true }, "Dokumente/Antrag.odt"),
197199
ScopeViolationError,
198200
);
199201
});
@@ -203,7 +205,7 @@ describe("resolvePath — component validation", () => {
203205
assert.throws(
204206
() =>
205207
resolvePath(
206-
{ kind: "org", sub: SUB, accountId: "acc-7", folderName: ".." },
208+
{ kind: "org", sub: SUB, accountId: "acc-7", folderName: "..", canWrite: true },
207209
"Protokolle/2026.odt",
208210
),
209211
ScopeViolationError,
@@ -219,6 +221,7 @@ describe("resolvePath — component validation", () => {
219221
sub: SUB,
220222
accountId: "acc-7",
221223
folderName: "../../other-citizen/Privat",
224+
canWrite: true,
222225
},
223226
"steuer.odt",
224227
),

packages/workspace/test/wopi.test.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ import type { DirEntry } from "../src/propfind";
1616
import type { WorkspaceScope } from "../src/types";
1717

1818
const SECRET = new Uint8Array(32).fill(7);
19-
const scope: WorkspaceScope = { kind: "personal", sub: "0xabc" };
19+
const scope: WorkspaceScope = { kind: "personal", sub: "0xabc", canWrite: true };
2020
const claims: WopiClaims = {
2121
sub: "0xabc",
2222
sessionId: "sess-1",
@@ -46,6 +46,7 @@ describe("file ids", () => {
4646
sub: "0xabc",
4747
accountId: "acct-1",
4848
folderName: "Org Feuerwehr",
49+
canWrite: true,
4950
};
5051
const trickyPath = "Elternbeirat (Grundschule)/Bericht #1 Prüfbericht Müritz.odt";
5152
const decoded = decodeFileId(encodeFileId(orgScope, trickyPath));

0 commit comments

Comments
 (0)