Stop the development sandbox widening write-class authorization - #2498
Merged
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Owner
Author
|
Round-2 fixes pushed as
Verified: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
DevelopmentSandboxSettings.Enabledhad drifted from a local-dev convenience into anauthorization bypass, and it disagreed with
AutomationPolicyEngine.HasAccessAsync, which hasnever had a sandbox branch. A sandbox caller holding only a
Viewerrow passed the API bar andwas then refused at the worker.
#1861(write-capable membership in the worker mirror) sharpenedthat divergence without creating it.
Adjudicated story S2: converge on the strict end — the development sandbox never widens
write-class authorization. It stays a read and export convenience.
Removed (sandbox branch deleted; membership-backed in every environment):
AuthorizationService.CanWriteBoardAsync,GetWritableBoardIdsAsync,CanDeleteBoardAsync,CanManageBoardAccessAsyncAuthorizationService.GetUserRoleForBoardAsync— now reports the caller's real role instead ofa synthetic
UserRole.OwnerBoardAccessService.EnsureCanManageBoardAccessAsync(the service no longer takes aDevelopmentSandboxSettingsdependency at all)ProposalExecutionAuthorizationSnapshotReader— the execute path is a write lane, so itsowner-or-member scope filter now always applies (dependency removed here too)
Kept deliberately:
CanReadBoardAsync,GetReadableBoardIdsAsync— seeded local fixtures staybrowsable without hand-granting access
403unless sandbox is on):those endpoints are gated on the sandbox, not widened by it
Fixtures that relied on the bypass now get real
BoardAccessrows; no test logic was relaxed.Closes #1866
Test plan
All run in the worktree, Release,
-m:1:dotnet test backend/tests/Taskdeck.Application.Tests/Taskdeck.Application.Tests.csproj -c Release -m:1 --filter "FullyQualifiedName~AuthorizationServiceTests|FullyQualifiedName~BoardAccessServiceTests|FullyQualifiedName~AutomationPolicyEngine|FullyQualifiedName~BatchProposalExecutionServiceTests"— 123 passed, 0 faileddotnet test backend/tests/Taskdeck.Api.Tests/Taskdeck.Api.Tests.csproj -c Release -m:1 --filter "FullyQualifiedName~BatchExecuteProposalsApiTests|FullyQualifiedName~Authorization|FullyQualifiedName~BoardAccess"— 57 passed, 0 faileddotnet test backend/tests/Taskdeck.Architecture.Tests/Taskdeck.Architecture.Tests.csproj -c Release -m:1— 28 passed, 1 skipped, 0 failednode scripts/check-docs-governance.mjs— passedBatchProposalExecutionServiceTestswas substituted for an Integration filter: noTaskdeck.Integration.Testssource file referencesProposalExecutionAuthorizationSnapshot, andthat Application suite plus
BatchExecuteProposalsApiTestsare what exercise the reader.Notable expectation change:
ExecuteProposals_DevelopmentSandboxDoesNotWidenTheAuthorizationSnapshot(renamed) now expects
404instead of403— with the snapshot no longer widened, a caller withno membership sees nothing, which is exactly the non-sandbox answer.
Docs impact
docs/decisions/ADR-0068-development-sandbox-never-widens-write-authorization.md(Accepted;scope = development sandbox authorization posture; records why S1 — teaching the policy engine
the bypass — was rejected: it would spread a dev-only bypass into the review-gated automation
lane, the surface ADR-0003 / GP-06 exist to keep honest)
docs/decisions/INDEX.md— row 0068docs/platform/CONFIGURATION_REFERENCE.md— theDevelopmentSandboxsection now states what theflag does and does not relax
docs/security/BETA_THREAT_MODEL.md— the "authorization diverges under development sandboxsettings" row moves from open —
#1866to shipped, with residuals namedRisks
DevelopmentSandbox:Enabled=trueagainst a hand-made database with nomembership rows now gets
403(or404on the execute path) where the bypass previouslyanswered success. Boards created through the API set
OwnerIdto the creator, so the normal demoand dogfooding path is unaffected; the fix is one real
BoardAccessrow.GetUserRoleForBoardAsyncno longer reports a syntheticOwner. A sandboxViewerwill seeviewer-level UI — which is the point, but it is a visible behaviour change locally.
SettingsRegistration.csforce-disables the flag outside theDevelopmentenvironment and every deployment artefact pins it off.Residuals (out of scope, tracked on #1866)
LlmQueueService(:133) still relaxes its cross-user request-ownership check under sandbox.BoardJsonExportImportService(:239) retains a sandbox branch on the export/import lane.