Skip to content

fix(tasks)!: let allow_private_destination govern the transport it declares - #770

Merged
sroussey merged 1 commit into
claude/notify-merge-mainfrom
claude/optimistic-goldberg-hxoj5s-notify-private-destination
Aug 14, 2026
Merged

fix(tasks)!: let allow_private_destination govern the transport it declares#770
sroussey merged 1 commit into
claude/notify-merge-mainfrom
claude/optimistic-goldberg-hxoj5s-notify-private-destination

Conversation

@sroussey

Copy link
Copy Markdown
Collaborator

Stacked on #769. Base retargets to claude/notify-merge-main once that merges; review the top commit only.

Problem

postWebhookJson derived allowPrivate from classifyUrl(url).kind === "private". classifyUrl is string-only by construction and never resolves DNS, so a hostname that is not a literal IP and matches neither PRIVATE_EXACT_HOSTS nor PRIVATE_DOMAIN_SUFFIXES classifies public — and allowPrivate: false was sent even when the caller set allow_private_destination: true and held the network:private grant.

Split-horizon DNS is the concrete failure. webhookNotify({url: "https://hooks.mycorp.com/hook", allow_private_destination: true}) where the name resolves to 10.1.2.3, with network:private granted: fetchOneHop's DNS loop throws PRIVATE_DENIED"resolved to private address 10.1.2.3 … Grant the 'network:private' entitlement to allow this request." The operator has granted it, the flag is set, and no configuration makes the post work short of hard-coding the IP, which defeats their DNS and pins the request to an address that may move.

The code also contradicted its own JSDoc, which said the declaration exists precisely so allowPrivate is not "self-derived from whatever the URL happened to resolve to".

Why widen rather than document the limit

The old behavior is not a conservative policy, it is an unreachable one. The flag's effect was fully determined by the URL text: for a literal private URL it was redundant with the classifier, and for a public-looking name it was inert. There is no input for which it added a security decision — it only added a message asking for a grant the operator already held.

The relaxation stays gated by the grant, not by the flag. What moves is the authorization trigger: from "the URL string looks private" to "the caller declared a private destination", which is a superset. Every request receiving the widened transport is now entitlement-checked, where the public-looking case previously received neither the transport nor a check.

The secrecy posture only tightens — see the second breaking change below.

Breaking changes

  1. allow_private_destination: true on a genuinely public endpoint now requires the network:private grant to run at all, and widens the transport. Fail-closed, and the flag's name says what it does.
  2. A declared private destination's reply body, failure body and reason phrase are withheld, whatever the URL's spelling. These previously keyed on the static classification; they move to the same superset, so a public-looking name that is in fact internal stops echoing its reply.

Also worth stating: with no enforcer registered assertPrivateDestinationGranted returns immediately, so in an unpoliced host the flag alone widens the transport — unchanged from today's behavior for literal private URLs.

What still enforces authorization

  • assertPrivateDestinationGranted runs whenever allowPrivateDestination is set, not only when the URL classifies private. It checks network:private against the URL actually resolved — i.e. after credential resolution — so an origin-scoped policy grant still binds and a credential-supplied destination cannot slip past.
  • urlResourcePattern(url) is computed once into a local used both for that grant check and for privateResourceScopes, so the pattern the enforcer graded and the scope the transport re-enforces cannot diverge.
  • redirect: "error" is unchanged: exactly one hop, so a Location cannot pivot off the granted origin.
  • webhookPrivateEntitlements is unchanged — an unset flag still fails closed and still declares network:private.

Change

  • postWebhookJson: allowPrivate = request.allowPrivateDestination; the static classification is kept only for the "private URL, flag not set" refusal. Body / failure-body / reason-phrase suppression follow allowPrivate.
  • WebhookNotifyTask: the duplicate classifyUrl(url).kind === "private" is deleted (that duplicate was the drift), passing readSuccessBody: true as the ceiling it is and leaving postWebhookJson the single decider. classifyUrl import dropped.
  • Schema descriptions for allow_private_destination in all three tasks, and the matching packages/tasks/README.md sections, now state that the flag covers a public-looking hostname resolving into private space and that the reply is never surfaced.

Tests

Five new cases in NotifyTask.test.ts, all confirmed failing against the pre-fix tree.

Replaced "a public URL keeps allowPrivate false even when allow_private_destination is set" (and its comment) — it pinned the defect as if it were the invariant. Its successor asserts the widening plus the exact scope.

The load-bearing one is "a declared private destination is refused without the grant even when the URL looks public", in describe("graph-root entitlement enforcement") under browserRegistry() (grants network:http + credential, not network:private). It catches the single most dangerous way to get this fix wrong: widening allowPrivate while leaving the authorization check keyed on the static classification, which would hand out private reachability with no check at all. Its counterpart, "a grant scoped to the declared origin permits a public-looking private destination", proves the result is a gate rather than a blanket relaxation.

Also: "the widened transport is scoped to exactly the pattern the grant was checked against" (catches the two values being computed independently) and "a declared private destination never echoes its body, even for a public-looking hostname".

Unchanged regression net: "refuses a private destination that was not declared", "a public destination is fetched with allowPrivate false and no scopes", "a credential resolving to a private URL is refused at execute time", and every case in describe("graph-root entitlement enforcement").

Coverage gap

There is no test exercising a real split-horizon resolution — the case that motivated the change. SafeFetch.server.ts calls node:dns/promises.lookup directly with no injection seam, so a name that classifies public and resolves private cannot be simulated. The tests here pin the arguments handed to the transport and the authorization decision around them; the DNS half of the path is covered only by SafeFetch's own tests against literal addresses. Adding a resolver seam to SafeFetch.server.ts would close it and is left as follow-up.

Also out of scope, recorded here

SafeFetch.server.ts applies privateResourceScopes only when the static classification is private, so a redirect-following caller reaching a private address via a public name is unscoped. Unreachable from the notify tasks, which use redirect: "error".

Verification

  • bunx vitest run --project test packages/test/src/test/task/NotifyTask.test.ts — 115 passed
  • bun scripts/test.ts task vitest — 67 files, 1079 passed, 24 skipped
  • bun run build:types — 41/41 packages clean

🤖 Generated with Claude Code


Generated by Claude Code

…clares

BREAKING CHANGE: `allow_private_destination` now widens the transport for any
destination it is set on, and any such destination's reply body and reason
phrase are withheld.

`allowPrivate` was derived from `classifyUrl`, which is string-only by
construction and never resolves DNS. A hostname that is no literal IP and
matches no reserved suffix classifies public, so the flag was inert for
exactly the case it names: under split-horizon DNS a `hooks.mycorp.com`
resolving to 10.1.2.3 was refused at connect time with a message asking for
the `network:private` grant the operator already held, and no configuration
short of hard-coding the address made the post work.

The declaration now governs the transport and the GRANT still authorizes it:
`assertPrivateDestinationGranted` runs for every declared private
destination rather than only for a URL that reads private, so the set of
requests receiving the widened transport is a superset of the set that is
entitlement-checked — where previously the public-looking case was checked at
all. The resource pattern is computed once and used both for the grant check
and for `privateResourceScopes`, so the pattern graded and the scope enforced
cannot diverge. Redirects stay refused, so a `Location` cannot pivot off the
granted origin.

Body, failure-body and reason-phrase suppression move to the declaration for
the same reason: the URL alone cannot say whether the host was internal, so a
caller who declared it may be private never gets its reply back.

`WebhookNotifyTask` no longer classifies the URL a second time — that
duplicate was the drift — and passes `readSuccessBody: true` as the ceiling it
is, leaving `postWebhookJson` the single decider.

Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XKvKnyVeQQQCm6FhtaLyMa
Base automatically changed from claude/optimistic-goldberg-hxoj5s-notify-timeout to claude/notify-merge-main August 14, 2026 15:35
@sroussey
sroussey merged commit 42a171c into claude/notify-merge-main Aug 14, 2026
8 of 9 checks passed
@github-actions

Copy link
Copy Markdown

Coverage Report

Status Category Percentage Covered / Total
🔵 Lines 60% 37467 / 62442
🔵 Statements 59.5% 39304 / 66051
🔵 Functions 61% 7253 / 11890
🔵 Branches 48.28% 19088 / 39528
File CoverageNo changed files found.
Generated in workflow #3070 for commit 19354eb by the Vitest Coverage Report Action

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