fix(automation)!: refuse data ops for a run with no trigger user (#3760) - #3784
Merged
Conversation
An effective `runAs:'user'` run that resolves no trigger user executed its
data nodes UNSCOPED: it presented no principal, and the data security
middleware skips when there is no principal, so the run read and wrote every
row. `runAs:'user'` is an access-NARROWING declaration, and ADR-0049's standing
rule is that failing to resolve one must never resolve to a grant. It now
throws `UnscopedRunDataAccessError` from `resolveRunDataContext` — the single
place every data node resolves its context.
This was never really about schedules. The docs, the spec, the runtime warning
and the lint all described a schedule-shaped problem, and the lint only matched
that shape, but the runtime predicate is "no user". The commonest way to have
no user is a record-change flow fired by a write that carried none: `isSystem`
does not suppress trigger dispatch — only `skipTriggers` does, and three
first-party paths set it — so plugin/service system writes, the approvals
status mirror, and a `runAs:'system'` flow's own data node all dispatched
record-change flows with `userId: undefined`. Ordinary users reach those writes
routinely, so the fail-open was reachable by unprivileged input.
Deliberately NOT implemented as "inherit the write's posture and run as
isSystem". That reads like a relabel but is an escalation: the middleware's
isSystem short-circuit (security-plugin.ts:722) fires before the
package-managed-row, system-row, audience-anchor and delegated-admin gates
(795/809/817/844), all of which a principal-less context still clears. Such a
run cannot write sys_user_position today; as isSystem it could.
- Lint `flow-schedule-runas-unscoped` -> `flow-runas-unscoped`, now FAILS the
build and covers time_relative + api (ADR-0073 D5). It documented itself as
"NEVER fails the build" — a gate that behaved as a comment. It still cannot
cover record_change, which is undecidable at authoring time.
- Three seed writes (seed-loader pass-2 back-fill, both app-plugin fallbacks)
inlined a bare `{isSystem:true}` and so seeded with automation live.
- #3712's user-less provenance path is subsumed: such runs are refused before
the approvals lock is consulted, and a schedule reaches its own record via
`runAs:'system'`. The flowRunId exemption stays live for runs with a user.
- ADR-0073 amended: its "no untrusted-input path" severity claim is falsified,
and its rejection of fail-closed expired when the example flows it cited were
fixed to declare `runAs:'system'`.
Both new gates were verified to go red against the pre-fix behavior, including
the live dogfood stack, which previously pinned the fail-open explicitly.
Refs #1888, #3712, #3749, #3456, ADR-0049, ADR-0073.
Co-Authored-By: Claude <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
github-actions
Bot
added
documentation
Contributor
📓 Docs Drift CheckThis PR changes 8 package(s): 119 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:
|
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
1 participant
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.
Closes #3760.
The finding that changed the decision
The issue offered four options and favoured Option 1 — "inherit the triggering write's posture → run explicitly
isSystem; effective permissions unchanged, so breakage is close to zero."That premise is false.
isSystem: trueis strictly wider than the principal-less context these runs carry today. The security middleware'sisSystemshort-circuit is atsecurity-plugin.ts:722— before four fail-closed gates a principal-less context still has to clear:isSystemsys_position/sys_capability)userIdnext()Concretely: today one of these cascade flows cannot write
sys_user_position—delegated-admin-gate.ts:172throws, and the repo already has a test locking that. Under Option 1 it could. So Option 1 is a real privilege escalation, not a relabel.Option 3 as literally written also doesn't work: a manufactured
{isSystem:false, positions:[], permissions:[]}satisfies the line-852 predicate exactly and falls open again. Genuine "restricted" needs a middleware change.Maintainer chose Option 2 (runtime fail-closed).
What ships
resolveRunDataContext— the documented single place every data node resolves its context — now throwsUnscopedRunDataAccessErrorinstead of handing back a principal-less envelope. The error namesrunAs:'system'as the fix.This is not a new policy: ADR-0073 D5 already ruled that "user-less +
runAs:'user'is a configuration error". It deferred the runtime half on two grounds this PR falsifies:runAs !== 'system' && !userId, and the dominant shape is a record-change flow fired by a write that carried no user —isSystemdoes not suppress trigger dispatch, onlyskipTriggersdoes, and exactly three first-party paths set it. Every plugin/service system write, the approvals status mirror, and arunAs:'system'flow's own data node dispatched record-change flows withuserId: undefined. Submitting for approval mirrors a status onto the target record — so ordinary users reach it.runAs:'system'. The stated cost expired.ADR-0073 is amended in place rather than quietly contradicted, and the refusal is marked interim and forward-compatible: when its
automationprincipal lands, the refusal point becomes the place that resolves it.Also in this PR
flow-schedule-runas-unscoped→flow-runas-unscoped, and it now fails the build.compile.tsdocumented in-line that the flow lint "NEVER fails the build" — it read as a gate and behaved as a comment, which is close to no net for the audience it protects. Extended totime_relativeandapi(ADR-0073 D5). It still cannot coverrecord_change— undecidable at authoring time, which is exactly why the runtime refusal exists; that limit is pinned by a test so nobody "fixes" the gap by guessing.AppPluginbasic-insert fallbacks inlined a bare{isSystem:true}instead of the shared seed options, seeding with record-change automation live — the self-trigger vectorskipTriggersexists to prevent, on the writes that skipped it.flowRunId(#3456 residual) #3712's user-less provenance path is subsumed, not broken. Those runs are refused before the approvals lock is consulted; a schedule reaches its own record viarunAs:'system', which the lock hook exempts on its ownisSystembranch. TheflowRunIdexemption stays live and load-bearing for what fix(approvals): a dead approval run no longer leaves the record RECORD_LOCKED (#3456) #3703 built it for — arunAs:'user'run that does have a user.Migration (breaking)
A flow reacting to system writes that must act beyond one user's grants declares
runAs: 'system'— one line of metadata, and the elevation becomes explicit and audit-attributable. Otherwise ensure the trigger supplies a user. Flows touching no data are unaffected. The failure is isolated: the record-change trigger already swallows flow errors, so the originating write still succeeds, and the engine warns at run setup, before any node executes.Verification
pnpm build(real type-check, noOS_SKIP_DTS) andpnpm test— 132/132 tasks green.expected 'done' to be undefined— the flow really did write the record under the old behaviour; reverting the seed fix turns its test red too.flow-runas-schedule.dogfood.test.ts) previously pinned the fail-open explicitly, asserting a user-less run reads and writes an admin-owned record a member cannot touch. Its own header said a fail-closed change must turn it red and "force the product decision to be revisited deliberately" — that is what happened; it is inverted to pin the refusal, with the unchanged-record assertion carrying the weight.gen:schema+gen:docs);check:api-surfaceclean.Filed separately (Prime Directive #10)
os validateruns none of the flow authoring lints, so a build-failing flow passes validate #3782 —os validateruns none of the flow lints, so a build-failing flow passesvalidate.runAs:'system'#3783 — the approvals status mirror drops the acting user, pushing every downstream record-change flow torunAs:'system';approval-node.ts:152already shows the fix.🤖 Generated with Claude Code