feat(core): allow injected ACL scope matching - #226
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
📝 WalkthroughWalkthroughThis PR introduces pluggable scope matching to ACL evaluation, allowing callers to inject custom path-aware scope matchers. It adds ChangesACL Scope Matching and Integration
Sequence DiagramsequenceDiagram
participant Caller
participant TreeAPI as listTree()
participant QueryAPI as queryFiles()
participant ExportAPI as exportWorkspaceJson()
participant PermCheck as filePermissionAllows()
participant CustomMatcher as options.scopeMatches
Caller->>TreeAPI: listTree(..., aclOptions)
TreeAPI->>PermCheck: filePermissionAllows(..., {aclOptions, requestedPath, action: "read"})
PermCheck->>CustomMatcher: scopeMatches(scope, claims, context)
CustomMatcher-->>PermCheck: boolean
PermCheck-->>TreeAPI: boolean
Caller->>QueryAPI: queryFiles(..., aclOptions)
QueryAPI->>PermCheck: filePermissionAllows(..., {aclOptions, requestedPath, action: "read"})
PermCheck->>CustomMatcher: scopeMatches(scope, claims, context)
CustomMatcher-->>PermCheck: boolean
PermCheck-->>QueryAPI: boolean
Caller->>ExportAPI: exportWorkspaceJson(..., aclOptions)
ExportAPI->>PermCheck: filePermissionAllows(..., {aclOptions, requestedPath, action: "read"})
PermCheck->>CustomMatcher: scopeMatches(scope, claims, context)
CustomMatcher-->>PermCheck: boolean
PermCheck-->>ExportAPI: boolean
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Relayfile Eval ReviewRun: Passed: 4 | Needs human: 0 | Reviewable: 0 | Missing output: 0 | Failed: 0 | Skipped: 0 Human Review CasesNo reviewable human-review cases captured Relayfile output. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/core/src/acl.test.ts (1)
65-99: ⚡ Quick winAdd a regression test for caller-supplied
action.Please cover the case where
aclOptions.actionis passed as"write"or"manage"and assert that tree/query/export still invoke scope matching withcontext.action === "read". That would lock in the PR’s stated read-path behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/acl.test.ts` around lines 65 - 99, Add a regression test variant that passes aclOptions.action set to "write" and another with "manage" and asserts that listTree, queryFiles, and exportWorkspaceJson still call the injected scopeMatches with context.action === "read"; specifically, reuse the existing test setup (claims, rows, repo, and scopeMatches mock) but pass aclOptions = { scopeMatches, action: "write" } and then again with action: "manage", and assert the returned file lists remain ["/github/LAYOUT.md"] and that scopeMatches was called and its received context.action equals "read" for each invocation of listTree, queryFiles, and exportWorkspaceJson.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/export.ts`:
- Around line 32-36: In exportWorkspaceJson(), ensure ACL checks always use
action: "read" instead of honoring caller-provided aclOptions.action; locate
where aclOptions is spread into the ACL check object (the snippet with
"...aclOptions, action: aclOptions.action ?? 'read', requestedPath: row.path")
and change it to force action: "read" (keep requestedPath: row.path and preserve
other aclOptions fields).
In `@packages/core/src/query.ts`:
- Around line 89-94: The authorization check in queryFiles() currently lets
aclOptions.action override the intended "read" semantics when calling
filePermissionAllows, which can wrongly evaluate visibility using non-read
actions; change the call site so the action passed is always "read" (ignore or
override aclOptions.action) while still forwarding other aclOptions, i.e. call
filePermissionAllows(effectivePermissions, workspaceId, claims, { ...aclOptions,
action: "read", requestedPath: row.path }), ensuring file visibility is always
checked as a read operation.
In `@packages/core/src/tree.ts`:
- Around line 64-68: listTree() currently passes through aclOptions.action
allowing callers to change the ACL check; change it to always use action: "read"
so authorization is based on read scope only, i.e., replace the spread that
includes action: aclOptions.action ?? "read" with a literal action: "read" while
still passing requestedPath (filePath) per row; update any references where
aclOptions is merged for ACL evaluation (e.g., the object created near
listTree() that includes requestedPath) to ensure callers cannot override
action.
---
Nitpick comments:
In `@packages/core/src/acl.test.ts`:
- Around line 65-99: Add a regression test variant that passes aclOptions.action
set to "write" and another with "manage" and asserts that listTree, queryFiles,
and exportWorkspaceJson still call the injected scopeMatches with context.action
=== "read"; specifically, reuse the existing test setup (claims, rows, repo, and
scopeMatches mock) but pass aclOptions = { scopeMatches, action: "write" } and
then again with action: "manage", and assert the returned file lists remain
["/github/LAYOUT.md"] and that scopeMatches was called and its received
context.action equals "read" for each invocation of listTree, queryFiles, and
exportWorkspaceJson.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d2631df1-9f1f-4c1e-a37c-63ad6fb427df
📒 Files selected for processing (5)
packages/core/src/acl.test.tspackages/core/src/acl.tspackages/core/src/export.tspackages/core/src/query.tspackages/core/src/tree.ts
| { | ||
| ...aclOptions, | ||
| action: aclOptions.action ?? "read", | ||
| requestedPath: row.path, | ||
| }, |
There was a problem hiding this comment.
Export ACL checks should always run as "read".
This is the content-export path, so honoring a caller-provided aclOptions.action can authorize exports against the wrong action. exportWorkspaceJson() should force action: "read" and only vary requestedPath.
Suggested fix
{
...aclOptions,
- action: aclOptions.action ?? "read",
+ action: "read",
requestedPath: row.path,
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| { | |
| ...aclOptions, | |
| action: aclOptions.action ?? "read", | |
| requestedPath: row.path, | |
| }, | |
| { | |
| ...aclOptions, | |
| action: "read", | |
| requestedPath: row.path, | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/export.ts` around lines 32 - 36, In exportWorkspaceJson(),
ensure ACL checks always use action: "read" instead of honoring caller-provided
aclOptions.action; locate where aclOptions is spread into the ACL check object
(the snippet with "...aclOptions, action: aclOptions.action ?? 'read',
requestedPath: row.path") and change it to force action: "read" (keep
requestedPath: row.path and preserve other aclOptions fields).
| if ( | ||
| !filePermissionAllows(effectivePermissions, workspaceId, claims, { | ||
| ...aclOptions, | ||
| action: aclOptions.action ?? "read", | ||
| requestedPath: row.path, | ||
| }) |
There was a problem hiding this comment.
Do not let query authorization switch off "read".
queryFiles() is also a read surface. Allowing aclOptions.action to override the action here means a caller can evaluate file visibility with non-read semantics, which is the wrong contract for this API.
Suggested fix
{
...aclOptions,
- action: aclOptions.action ?? "read",
+ action: "read",
requestedPath: row.path,
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ( | |
| !filePermissionAllows(effectivePermissions, workspaceId, claims, { | |
| ...aclOptions, | |
| action: aclOptions.action ?? "read", | |
| requestedPath: row.path, | |
| }) | |
| if ( | |
| !filePermissionAllows(effectivePermissions, workspaceId, claims, { | |
| ...aclOptions, | |
| action: "read", | |
| requestedPath: row.path, | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/query.ts` around lines 89 - 94, The authorization check in
queryFiles() currently lets aclOptions.action override the intended "read"
semantics when calling filePermissionAllows, which can wrongly evaluate
visibility using non-read actions; change the call site so the action passed is
always "read" (ignore or override aclOptions.action) while still forwarding
other aclOptions, i.e. call filePermissionAllows(effectivePermissions,
workspaceId, claims, { ...aclOptions, action: "read", requestedPath: row.path
}), ensuring file visibility is always checked as a read operation.
| { | ||
| ...aclOptions, | ||
| action: aclOptions.action ?? "read", | ||
| requestedPath: filePath, | ||
| }, |
There was a problem hiding this comment.
Force "read" for tree ACL evaluation.
listTree() is a read API, so letting callers override action here changes authorization semantics and can make entry visibility depend on "write"/"manage" scopes instead of read scopes. Hardcode action: "read" and keep only requestedPath injected per row.
Suggested fix
{
...aclOptions,
- action: aclOptions.action ?? "read",
+ action: "read",
requestedPath: filePath,
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| { | |
| ...aclOptions, | |
| action: aclOptions.action ?? "read", | |
| requestedPath: filePath, | |
| }, | |
| { | |
| ...aclOptions, | |
| action: "read", | |
| requestedPath: filePath, | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/tree.ts` around lines 64 - 68, listTree() currently passes
through aclOptions.action allowing callers to change the ACL check; change it to
always use action: "read" so authorization is based on read scope only, i.e.,
replace the spread that includes action: aclOptions.action ?? "read" with a
literal action: "read" while still passing requestedPath (filePath) per row;
update any references where aclOptions is merged for ACL evaluation (e.g., the
object created near listTree() that includes requestedPath) to ensure callers
cannot override action.
|
Reviewed the checked-out PR artifacts, which identify Fixed the remaining actionable review issues:
Validation:
|
![agent-relay-code[bot]](https://avatars.githubusercontent.com/in/3624277?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed the checked-out PR artifacts, which identify AgentWorkforce/relay PR #1019 rather than relayfile PR #226.
Fixed the remaining actionable review issues:
- Added legacy
.agentworkforce/workspaces.jsonfallback and reserved-key sanitization in workspace key storage. - Validated
workspace createnames before remote workspace creation. - Kept
driveandpassthroughsignal handlers installed until async delivery-mode restore completes. - Added OG ellipsis font coverage and removed nonzero OG letter spacing.
- Added regression tests and updated
CHANGELOG.md.
Validation:
- Prettier check passed on edited files.
- Narrow TypeScript check passed for
workspace-store.ts. - Full typecheck/tests could not run because
npm installwas killed and the localnode_modulestree is incomplete/missing type packages and Vitest binaries.
|
Fixed the PR issue in acl.test.ts: the new Verification run locally:
|
|
✅ pr-reviewer applied fixes — committed and pushed Fixed the PR issue in acl.test.ts: the new Verification run locally:
|
There was a problem hiding this comment.
✅ pr-reviewer applied fixes — committed and pushed c46e903 to this PR. The notes below describe what changed.
Fixed the PR issue in acl.test.ts: the new scopeMatches callback test now types context as ScopeMatchContext, so strict TypeScript checks the public ACL option contract instead of producing an implicit any.
Verification run locally:
npm run test --workspace=packages/corepassednpm run build --workspace=packages/corepassedscripts/check-contract-surface.shpassed- Root JS/TS tests passed; root commands only failed at Go steps because
gois not installed in this environment
1 participant
Summary
Tests