Skip to content

The #1888 user-less fail-open is wider than the lint that guards it — record-change flows fired by a system write run UNSCOPED, unlinted #3760

Closed

Description

@os-zhuang

Filed per Prime Directive #10 to capture the decision #3712 deliberately left open. #3749 (which closed #3712) decoupled write provenance from identity, so the fail-open can now be decided on its own merits instead of being forced by an unrelated fix — but it did not decide it.

The claim as it stands today

ADR-0049 / #1888 document the user-less fail-open as a schedule problem, guarded by two nets: a loud runtime [runAs] warning, and the build-time lint flow-schedule-runas-unscoped. Investigating both nets, neither is as strong as the narrative implies.

Net 1 — the lint never fails the build

packages/cli/src/commands/compile.ts:400-405, verbatim:

Flow authoring anti-pattern lint (#1874) — advisory warnings … NEVER fails the build.

It prints. Nothing stops a publish. For the audience this rule exists to protect — very often an AI generating flow metadata in bulk — an advisory line is close to no net at all.

Net 2 — the lint covers one shape; the runtime hole has more

The lint's predicate (packages/cli/src/utils/lint-flow-patterns.ts:311):

const runAs = typeof flow.runAs === 'string' ? flow.runAs : 'user';
if (isScheduleTriggered(flow, startCfg) && runAs !== 'system') { … }

The runtime's predicate (packages/services/service-automation/src/runtime-identity.ts):

export function runIsUnscopedUserMode(context) {
  return context?.runAs !== 'system' && !context?.userId;
}

isScheduleTriggered ⊊ !userId. Anything that resolves no trigger user is unscoped at runtime; only the schedule shape is flagged at authoring time.

The unlinted path

packages/triggers/trigger-record-change/src/record-change-trigger.ts:325 forwards the writer's identity with no fallback:

userId: session.userId,

and dispatch is suppressed only for skipTriggers (line 233) — isSystem does not suppress it. So:

  1. a write carrying isSystem and no userId — a plugin/service write, the approvals status mirror, or a runAs:'system' flow's own data node — lands;
  2. it dispatches the record-change flows bound to that object with userId: undefined;
  3. if such a flow left runAs at the spec default 'user', its data nodes run UNSCOPED.

Nothing flags this at authoring time, and nothing can: whether the triggering write carries a user is only knowable at run time.

Given that record-change is the most common flow type and runAs defaults to 'user', this is plausibly the dominant real-world shape of the fail-open — not the schedule case the docs and the lint both centre on.

Exposure

First-party content is clean: every schedule-triggered flow in the repo (examples/app-todo ×2, examples/app-showcase) already declares runAs: 'system', with comments citing ADR-0049. The exposure is third-party and AI-authored metadata.

Options

A pure authoring-side fix cannot close this — the record-change path is not statically decidable. The convergence point has to be the runtime.

  1. Inherit the posture of the triggering write. A cascade whose triggering write was isSystem runs explicitly isSystem; only a genuinely origin-less run (a schedule) is fail-closed or must declare. Effective permissions are unchanged (today's behaviour is unscoped), so breakage is close to zero — but the elevation stops being accidental and becomes intentional and audit-attributable, which is what ADR-0049 actually asks for.
  2. Runtime fail-closed. User-less + effective runAs:'user' → refuse the data op, with an error naming runAs:'system' as the fix. Retires the footgun outright, but converts today's working record-change flows into run-time failures that no build surfaces.
  3. Baseline-member RLS. Run restricted instead of unscoped. No elevation, but flows that see every row today would silently see fewer or none — a quieter failure mode than an error, and harder to diagnose. This is the outcome Approval: a schedule-triggered run still can't write its own locked record — it carries no ObjectQL context to hold flowRunId (#3456 residual) #3712 predicted for "manufacturing a context".
  4. Author-side hardening only. Make the lint blocking and extend it to every statically-decidable shape. Zero breakage, but explicitly leaves the record-change path open.

Option 1 looks like the best breakage-to-benefit ratio; option 2 is the most thorough and the most disruptive.

Whichever is chosen, two fixes stand on their own:

Refs #1888, #3712, #3749, #3456, ADR-0049.

Activity

  1. self-assigned this
    on Jul 28, 2026
  2. os-zhuang commented on Jul 28, 2026

    @os-zhuang
    ContributorAuthor

    Decided: Option 2 (runtime fail-closed). PR #3784.

    Recording one correction, because it inverts the option ranking in the issue body.

    Option 1's premise is wrong. It rests on "effective permissions are unchanged (today's behaviour is unscoped), so breakage is close to zero" — treating unscoped and isSystem as the same authorization. They are not. The middleware's isSystem short-circuit (security-plugin.ts:722) fires before four fail-closed gates that a principal-less context still has to clear:

    line gate principal-less isSystem
    795 package-managed row runs skipped
    809 system row (sys_position/sys_capability) runs skipped
    817 audience anchor runs skipped
    844 delegated admin (RBAC link tables) throws on no userId skipped
    852 empty-principal fall-open → next() —

    Concretely: today one of these cascade flows cannot insert into sys_user_position — delegated-admin-gate.ts:172 refuses it, and there is already a test locking that behaviour. Under Option 1 it could. So Option 1 widens the hole rather than making it explicit.

    Option 3 as literally worded also doesn't hold. Manufacturing {isSystem:false, positions:[], permissions:[]} satisfies the line-852 predicate exactly (positions empty ∧ permissions empty ∧ no userId) and falls open again. Real "restricted" requires changing the middleware — which is what the resolveRunDataContext comment left by #3712 already predicted.

    That leaves Option 2 as the only choice that closes the hole without touching the authorization boundary, and it is the one ADR-0073 D5 already ruled for ("user-less + runAs:'user' is a configuration error"). ADR-0073 deferred the runtime half on two grounds that no longer hold, both amended in the PR:

    • "an unprivileged user cannot trigger a schedule, so there is no untrusted-input path" — true of schedules, irrelevant to the actual exposure. Submitting for approval makes the approvals service mirror a status onto the target record with a system write, which dispatches record-change flows with no user. Unprivileged input reaches it routinely.
    • "fail-closed breaks legitimate scheduled CRUD (2/3 example flows relied on the default)" — those flows were fixed in fix(security): surface schedule/user-less flow runAs fail-open (#1888 follow-up) #2308 itself.

    Both standalone fixes are in the PR: the lint now actually fails the build (and is renamed flow-runas-unscoped, extended to time_relative/api per ADR-0073 D5), and the schedule-specific framing is corrected in the spec, the docs, the runtime warning and the ADR.

    Two adjacent findings filed separately: #3782 (os validate runs no flow lints at all, so a build-failing flow passes validate) and #3783 (the approvals status mirror drops the acting user, which is why so many flows are pushed to runAs:'system').

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

No labels
No labels

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions