Skip to content

fix(tasks)!: require a graded network:private grant to widen past a URL's own classification - #831

Merged
sroussey merged 1 commit into
claude/notify-merge-mainfrom
claude/notify-private-widening-fail-closed
Aug 17, 2026
Merged

fix(tasks)!: require a graded network:private grant to widen past a URL's own classification#831
sroussey merged 1 commit into
claude/notify-merge-mainfrom
claude/notify-private-widening-fail-closed

Conversation

@sroussey

Copy link
Copy Markdown
Collaborator

What this changes

allow_private_destination may now widen the transport only as far as the URL
itself already declares:

  • a URL that reads private on its own (loopback, RFC1918, link-local) is
    widened by the declaration alone — unchanged, and what FetchUrlTask already
    does for a declared private URL;
  • a URL that reads public now requires a graded network:private grant. With
    no ENTITLEMENT_ENFORCER registered there is nothing able to grade it, so the
    post is refused with PRIVATE_DENIED.

assertPrivateDestinationGranted returns its verdict
(PrivateDestinationAuthorization = "granted" | "unenforced") instead of
Promise<void>, so the caller can tell "a policy graded this and allowed it"
apart from "no policy exists". Widening Promise<void>Promise<"granted" | "unenforced"> is source-compatible for external callers.

The correction that drives the fix

The literal-169.254.169.254-plus-flag case was already reachable before
19354ebaa: with allowPrivate = classification.kind === "private" a
statically-private URL got allowPrivate: true too. So that is not what
regressed.

What 19354ebaa newly widened is the public-looking URL. Setting the
flag on https://hooks.mycorp.com/… now disables SafeFetch.server.ts:161
the resolved-address check — for a name classifyUrl reads as public. That
check is the only DNS-rebinding defence in the stack: the scope re-check is
tautological (privateResourceScopes is urlResourcePattern(url) of the same
URL), and redirect: "error" means there is no second hop to catch it later.

Meanwhile assertPrivateDestinationGranted returns immediately when the
registry carries no ENTITLEMENT_ENFORCER — and that token has no default
registration
(TaskGraphRunner.ts:896 is the only one, guarded by
enforceEntitlements, which defaults to false), while TaskRunner.registry
defaults to globalServiceRegistry. So the README's "so every widened request
is entitlement-checked" (three occurrences) was false for the common case.

Why scoped fail-closed, and not the alternatives

Blanket fail-closed was rejected. There is no default enforcer and
enforceEntitlements defaults to false, so refusing every declaration would
make the flag inert for approximately every current user, remove the stock "post
to my internal Grafana webhook" case, contradict FetchUrlTask's shipped
treatment of a declared private URL, and rewrite ~13 tests plus all the
real-transport tests (which post to 127.0.0.1 through slackNotify(input, config), a path with no way to inject a registry).

Doc-only was rejected. The DNS bypass on a public-looking hostname is
genuinely new, and nothing else in the stack catches it.

The rule that separates them: a private-reading URL is visible in the
operator's configured value
, so the flag on it authorizes nothing a reader of
that configuration could not already see. A public-reading URL is the opposite
on both counts — the widening is invisible in configuration AND it buys the
DNS-guard bypass — so it has to be graded by a policy.

Migration

BREAKING: setting allow_private_destination on a destination whose URL
does not classify private now requires a registered ENTITLEMENT_ENFORCER
granting network:private for that origin. Loopback / RFC1918 / link-local URLs
are unaffected.

The error names both remedies (register an enforcer with the grant, or clear the
flag for a genuinely public destination). 19354ebaa already carries a
BREAKING CHANGE: trailer; this commit carries its own. The packages/tasks
CHANGELOG appears to be generated from commit trailers rather than hand-edited
(19354ebaa touched no CHANGELOG), so the note lives in the commit message.

Risk to watch: a workflow that harmlessly sets the flag on a genuinely
public webhook, with no enforcer registered, now fails where it previously
posted. That is deliberate — the flag was doing something real there, just not
something anything graded — but it is worth watching for in the downstream repos
(sec, embarc-data) via bun run dev-link.

Residual, stated honestly

With no enforcer registered, the flag on a URL that already reads private is
authorized by configuration alone. If that URL arrives by dataflow rather
than from the instance's own configuration, the operator authorized nothing —
the declaration travels the same trust channel as url, and is computed from
runInputData before run-input lands.

Filed as a follow-up rather than fixed here, because the right shape is
config-only (constructor defaults / x-ui-hidden, mirroring
credential_scheme) and that is a breaking schema change affecting saved graph
JSON, the builder UI's port rendering, and Task.setInput merge semantics:

#830

Tests

  • 3 new cases in graph-root entitlement enforcement:
    • a public-looking declared private destination is refused when no enforcer can grade it
      (the gate; verified failing against the pre-fix source)
    • a statically private declared destination still posts with no enforcer registered
      (pins that this is scoped, not blanket)
    • a grant makes the public-looking destination reachable again
      (pins that 19354ebaa's split-horizon feature survives)
  • 3 existing cases swapped from webhookNotify(...) to
    new WebhookNotifyTask().run(input, { registry: grantingRegistry([...]) })
    — registry only, assertions unchanged.
  • New file-level grantingRegistry(resources) helper; the graph-root suite's
    three ad-hoc copies now use it.
  • Comment-only: no registered enforcer leaves a declared private post working
    (still passes — 127.0.0.1 is statically private) and the
    NotifyTaskTransport.test.ts header. All real-transport tests pass unchanged
    (8 in that file, including the getSafeFetchImpl().name === "serverSafeFetch"
    tripwire).

Verification: NotifyTask.test.ts, NotifyTaskTransport.test.ts,
FetchUrlSsrf.test.ts and SafeFetchServerTransport.test.ts — 4 files, 270
tests, all passing. bun run format clean.

Out of scope, explicitly

  • Making allow_private_destination config-only (the follow-up above).
  • Registering a default ENTITLEMENT_ENFORCER, or changing
    enforceEntitlements' default.
  • Renaming FetchUrlTask's credential_key title.
  • Any change to SafeFetch.server.ts, including the tautological
    privateResourceScopes re-check.

🤖 Generated with Claude Code

https://claude.ai/code/session_01HJRf3YFa8DjmjsZvXz8xDT


Generated by Claude Code

…RL's own classification

BREAKING CHANGE: setting `allow_private_destination` on a destination whose URL
does not classify private now requires a registered `ENTITLEMENT_ENFORCER`
granting `network:private` for that origin. Loopback / RFC1918 / link-local URLs
are unaffected — a declaration on one of those is still authorized by the
declaration alone.

`19354ebaa` did not newly expose the literal-`169.254.169.254`-plus-flag case:
with `allowPrivate = classification.kind === "private"` a statically private URL
already received `allowPrivate: true`. What it newly widened is the
PUBLIC-LOOKING URL — setting the flag there now disables
`SafeFetch.server.ts`'s resolved-address check for a name `classifyUrl` reads as
public. That check is the only DNS-rebinding defence in the stack: the scope
re-check is tautological (`privateResourceScopes` is `urlResourcePattern(url)`
of the same URL) and `redirect: "error"` means there is no second hop.

Meanwhile `assertPrivateDestinationGranted` returned immediately when the
registry carried no `ENTITLEMENT_ENFORCER`, and that token has no default
registration — `TaskGraphRunner` is the only registrar and is guarded by
`enforceEntitlements`, which defaults to false — while `TaskRunner.registry`
defaults to `globalServiceRegistry`. So the documented guarantee that "every
widened request is entitlement-checked" was false for the common case.

The rule now enforced: a declaration may widen the transport only as far as the
URL itself already declares. A private-reading URL is visible in the operator's
configured value, so the flag on it authorizes nothing reading that
configuration would not already show. A public-reading one is invisible in
configuration AND buys the DNS-guard bypass, so it must be graded by a policy —
and with no enforcer to grade it, the post is refused with `PRIVATE_DENIED`
naming both remedies.

Failing closed on EVERY declaration was rejected: with no default enforcer it
would make the flag inert for approximately every current user, remove the stock
"post to my internal webhook" case, and contradict `FetchUrlTask`'s shipped
treatment of a declared private URL. Documenting the gap alone was rejected too:
the DNS bypass on a public-looking hostname is genuinely new.

`assertPrivateDestinationGranted` now returns whether a policy actually graded
the destination (`"granted" | "unenforced"`) rather than `void`, so the caller
can tell "allowed by policy" from "no policy exists". Widening the return type
is source-compatible for external callers.

Residual, unfixed here: with no enforcer registered the flag on an
already-private URL is authorized by configuration alone, and if that URL
arrives by dataflow the operator authorized nothing. The right shape is a
config-only port, which is a breaking schema change affecting saved graph JSON
and the builder UI's port rendering; tracked separately.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HJRf3YFa8DjmjsZvXz8xDT
@github-actions

Copy link
Copy Markdown

Coverage Report

Status Category Percentage Covered / Total
🔵 Lines 60.56% 39110 / 64575
🔵 Statements 60.05% 41043 / 68348
🔵 Functions 61.13% 7572 / 12385
🔵 Branches 48.93% 20054 / 40983
File CoverageNo changed files found.
Generated in workflow #3201 for commit 966faec by the Vitest Coverage Report Action

Copy link
Copy Markdown
Collaborator Author

CI status: every test job is green (7/7 suites, plus build, test-discovery, merge-vitest-coverage). The one red check, typecheck-budget, fails on the base branch too and is not from this PR.

The failure is:

packages/job-queue/src/job/__tests__/JobQueueWorker.test.ts(461,11):
  error TS6133: 'id' is declared but its value is never read.

git diff --name-only claude/notify-merge-main..HEAD on this branch touches only packages/tasks/** and packages/test/src/test/task/**git diff --stat claude/notify-merge-main..HEAD -- packages/job-queue/ is empty. That file is byte-identical to the base, so this PR cannot have introduced an unused-variable error in it. The declaration at line 461 (const id = await storage.add({...})) is present on claude/notify-merge-main itself.

The fix belongs in #744, not here — one line, either consume id or drop the binding. I have not pushed it, since that would mean writing to another in-flight branch from a stacked PR and would conflict the moment #744 updates.

Happy to re-run once the base is green.


Generated by Claude Code

@sroussey
sroussey merged commit 325fa6e into claude/notify-merge-main Aug 17, 2026
10 of 11 checks passed
@sroussey
sroussey deleted the claude/notify-private-widening-fail-closed branch August 17, 2026 20:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants