fix(tailscale): existing Serve handlers are no longer overwritten or removed#18068

closed5

main

GitHub fork: fix/tailscale-serve-handler-preservation

opened18d agobyLlastobelus

View on GitHubTwo-way sync enabledPR last imported 8d ago

Problem

T3 Code changes Tailscale Serve configuration without reading it first, so it can replace or remove a handler that something else put on the same HTTPS port.

ensureTailscaleServe always writes the mapping and disableTailscaleServe always turns the port off. Every caller inherits that:

t3 pair --tailscale treats a failed HTTPS probe as "nothing there". On Linux with --accept-dns=false, a machine cannot resolve its own MagicDNS name, so a healthy handler looks unreachable and gets repointed. The report on the original PR traced this from source and DNS captures.
t3 serve --tailscale-serve and the desktop Tailscale HTTPS setting configure the port at startup and turn it off at shutdown, whatever is on it.
vp run dev --share clears the port before serving and again on exit.

Change

Both functions now read tailscale serve status --json and classify the selected port before changing anything:

Port state	Configure	Remove
No handler	writes the mapping	nothing to do
One private `/` proxy to exactly the expected local target	reuses it, no write	removes it
One private `/` proxy to a different target	refuses, unless pairing verified it	leaves it
Anything else: Funnel, foreground session, extra routes or authorities, TCP forwarding, PROXY protocol, file, text or redirect handlers, app capability grants	refuses	leaves it

The configuration is decoded strictly against ServeConfig in Tailscale's ipn/serve.go. If it cannot be read, or contains a field T3 Code does not recognize, nothing is written or removed, because a dropped field could make a customized handler look like a plain one. Services is accepted uninspected: services have their own addresses and these commands do not change them.

What each caller does with a refusal:

Pairing still reuses a port that answers with this environment's id without touching Serve. A refused port fails the command and suggests --tailscale-serve-port.
Dev sharing no longer clears the port first, fails on an occupied port, and on exit removes only a matching mapping.
Desktop Settings runs the same check before saving Tailscale HTTPS as on, against the backend's current local port and the HTTPS port that will be stored. If the port is occupied or the configuration cannot be read, nothing is saved, the app does not relaunch, and Settings shows guidance to inspect tailscale serve status --json and, for an occupied port, choose another HTTPS port. Turning the setting off does not call the CLI.
Server and desktop startup log the refusal as a warning and keep starting, as they do for any other Serve setup failure. At shutdown they remove the mapping only if it still matches.

Two intended consequences: a stale mapping to a different local port is no longer replaced automatically, because a failed probe cannot show whose it is; and if a future Tailscale release adds a Serve field, T3 Code stops changing Serve on machines where it appears until the decoder is updated.

Scope and approval

There is no triaged issue or maintainer approval. I am submitting this under the exception for a focused fix of an obvious bug: T3 Code silently replaces or deletes a persistent Serve handler it has not looked at.

It is one problem: every caller uses the same functions in packages/tailscale, and each caller change passes its own target or handles the refusal. The desktop check reuses the shared classifier and the existing Settings error toast, with no new IPC or contract.

This is a from-scratch rebuild of #8338, closed with a recommendation to rebuild it with a newer model.

Verification

Reproduced on upstream main (716357cbba) using two of the regression tests added here: ensureTailscaleServe ran serve --bg over a different handler without reading configuration, and resolveTailscalePairingBase did the same after a mocked ENOTFOUND probe.

After the change, these suites pass:

bash

1	pnpm exec vp test run packages/tailscale/src/tailscale.test.ts apps/server/src/cli/pair.test.ts scripts/lib/dev-share.test.ts scripts/dev-runner.test.ts apps/desktop/src/backend/DesktopServerExposure.test.ts apps/desktop/src/settings/DesktopAppSettings.test.ts

Shared helpers and CLI callers: failed probes (DNS, 502/503/504) against a different handler and an empty port, exact-mapping reuse without a write, refusal for each protected handler kind, the verified dev repoint, cleanup that leaves a changed handler alone, and unreadable configuration. An unrecognized field in any of five placements leaves every operation with only the status read.
Desktop Settings: an occupied port, an unrecognized field, unparseable output, and a failed CLI call each leave saved settings and runtime state unchanged after one status read. A matching proxy is accepted without a write, an omitted port checks the saved port, an out-of-range port checks the default it is stored as, and turning the setting off never spawns the CLI.

tsc --noEmit passes for packages/tailscale, apps/desktop, apps/server, and scripts. Lint of the twelve changed TypeScript files exits 0.

Not checked, and known limits:

Nothing ran against a real Tailscale daemon or a running desktop app; the tests fake the tailscale CLI and the HTTPS probe. The field list comes from reading ipn/serve.go, not from captured output.
The Settings error travels through the existing IPC rejection, so the toast carries Electron's Error invoking remote method prefix, like other failures of that call.
Matching is by configuration, not by creator: an identical mapping is reused and removed regardless of who made it.
Reads and writes are separate commands, and the desktop check runs before a relaunch. A handler added in between, or one already there when an earlier version saved the setting as on, is still refused at startup with only a log warning. t3 serve --tailscale-serve refusals are log-only too.

Implemented with GPT-6.1 Sol through the Codex harness. Reviewed independently by Astra High and Claude Opus 5.5. Title and description written by Claude Opus 5.5.

Checks

24 successful · 8 skipped · 1 neutral

All checks passed

Added 1 commit18d ago

fix(tailscale): preserve existing Serve handlerse2e888c

Cchatgpt-codex-connectorBot18d ago

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-10-11T02:56:26.818658Z	`e2e888c`	PR opened

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

MmacroscopeappBot18d ago

Approvability

Verdict: Not approved

Macroscope's review found this PR not approvable — This PR changes persistent Tailscale Serve setup and cleanup behavior across desktop settings, pairing, server lifecycle, and development sharing, adding substantial shared status parsing and ownership classification. The cross-cutting runtime impact and reliance on an external daemon configuration format merit human review.

^{You can add or adjust custom eligibility rules. Learn more.}

Cchatgpt-codex-connectorBotreviewed · 2 inline threads18d ago

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e2e888ceb5

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

packages/tailscale/src/tailscale.tslines 469–470Outdated · e2e888cOpen thread

…	…	@@ -338,29 +443,52 @@ const runTailscaleCommand = (
	467	+	const state = servePortState(yield* readServeStatus, servePort, target);
	468	+	if (state === "matching") return;
	469	+	if (state === "occupied" \|\| (state === "replaceable" && !input.replaceVerifiedHandler)) {
	470	+	return yield* new TailscaleServePortOccupiedError({ servePort });

Cchatgpt-codex-connectorBoton lines 469–47018d ago

_{_{P2 Badge}} Report occupied ports to desktop users

When a desktop user enables Tailscale HTTPS on an occupied port, this new error cannot reach the settings flow: DesktopServerExposure.setTailscaleServeEnabled persists tailscaleServeEnabled=true and relaunches, then layerMakeServer catches the error in apps/server/src/server.ts and only logs a backend warning. The setup action therefore appears to succeed while the foreign mapping remains, and after relaunch the UI has only endpoint probe state rather than the promised explanation to choose another port. Preflight the port before persisting or carry the startup failure back to desktop settings.

AGENTS.md reference: AGENTS.md:L23-L29

Useful? React with 👍 / 👎.

Llastobelus18d ago

Confirmed: desktop settings persist the desired configuration and relaunch, while server startup reports a Serve refusal through its existing backend warning path. That does not provide the occupied-port explanation in Settings.

This PR preserves the existing handler and deliberately keeps startup failure reporting on that existing path. Desktop preflight or startup-error propagation would add a separate desktop behavior change to the handler-preservation fix. The PR description now explicitly records this limitation; I am leaving this finding open rather than marking it fixed.

Llastobelus17d ago

Fixed in 33fe2ff. Implemented the desktop fix, superseding my earlier deferral. Settings now performs the same read-only Serve check before saving an enabled setting or relaunching. An occupied port returns actionable guidance through the existing Settings error display; unreadable or unknown configuration also fails before persistence. The check uses the actual backend port and the same HTTPS-port normalization as the settings writer. Disabling does not require the Tailscale CLI.

Focused regressions cover preserved settings/runtime after refusal, matching-proxy acceptance, saved/default port selection, and CLI-free disable. Astra High and Opus 5.5 reviewed this integration with no blocking findings; scoped tests, lint and typechecks passed. Startup still rechecks before any Serve mutation.

packages/tailscale/src/tailscale.tsline 450Outdated · e2e888cOpen thread

…	…	@@ -338,29 +443,52 @@ const runTailscaleCommand = (
	447	+	["serve", "status", "--json"],
	448	+	TAILSCALE_STATUS_TIMEOUT,
	449	+	).pipe(
	450	+	Effect.flatMap(Schema.decodeEffect(Schema.fromJsonString(ServeStatus))),

Cchatgpt-codex-connectorBoton line 45018d ago

_{_{P1 Badge}} Reject unmodeled Serve fields before classifying

When an independently updated Tailscale returns any extra member inside TCP, Web, or a handler, this Schema.decodeEffect call uses Effect's default excess-property behavior and silently drops that member before servePortState runs. A root proxy that matches the expected target but carries an unmodeled capability is therefore treated as owned; on shutdown disableTailscaleServe performs the same lossy decode and runs serve ... off, deleting exactly the configuration this change is intended to preserve. Decode these nested objects strictly, or preserve and classify unknown keys as occupied.

AGENTS.md reference: AGENTS.md:L19-L21

Useful? React with 👍 / 👎.

Llastobelus18d ago

Fixed in 9e3a562bb0: Serve status decoding now rejects unknown properties instead of dropping them, including nested TCP, Web, HTTP handler, and foreground configuration. A decode failure leaves setup, verified replacement, and cleanup read-only. Known Services configuration remains separately scoped; ProxyProtocol is modeled and protected on the selected node port.

The new regression cases failed before the decoder fix. All 95 focused tests now pass, along with the package typecheck and scoped lint.

CcoderabbitaiBot18d ago

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.config.ts
Review profile: CHILL
Plan: Advanced
Run ID: a50e454c-6238-44c0-ba09-d3dfe130916c

📥 Commits

Reviewing files that changed from the base of the PR and between 22c022b294acc72a8413cbf59258a885cb1e8313 and 33fe2ffea8ef722a79f646a8e400bf0867883dc9.

📒 Files selected for processing (5)

apps/desktop/src/backend/DesktopServerExposure.test.ts
apps/desktop/src/backend/DesktopServerExposure.ts
apps/desktop/src/settings/DesktopAppSettings.ts
packages/tailscale/src/tailscale.test.ts
packages/tailscale/src/tailscale.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

📝 Walkthrough

Walkthrough

Tailscale Serve setup and cleanup now inspect the selected port’s configuration before changing it. Matching mappings can be reused, while foreign or complex handlers are preserved. Pairing, desktop exposure, and development sharing use these ownership checks.

Changes

Tailscale Serve ownership

Layer / File(s)	Summary
Serve status decoding and classification `packages/tailscale/src/tailscale.ts`, `packages/tailscale/src/tailscale.test.ts`	The client reads and strictly decodes Serve status JSON. It classifies the selected port before setup or cleanup, reuses matching handlers, and rejects occupied or unreadable configurations.
Desktop Serve enablement `apps/desktop/src/backend/DesktopServerExposure.ts`, `apps/desktop/src/backend/DesktopServerExposure.test.ts`, `apps/desktop/src/settings/DesktopAppSettings.ts`	Desktop exposure checks the selected normalized port against the current runtime target before enabling Serve or saving the setting. Tests cover matching, occupied, and unavailable configurations, invalid ports, and disabling Serve.
Pairing and guarded Serve setup `apps/server/src/cli/pair.ts`, `apps/server/src/cli/pair.test.ts`	Pairing reuses matching regular-server mappings and permits a verified development-server handler to be repointed. Other occupied handlers remain unchanged.
Development sharing and matching-handler cleanup `scripts/lib/dev-share.ts`, `scripts/lib/dev-share.test.ts`, `apps/server/src/server.ts`, `scripts/dev-runner.ts`, `scripts/dev-runner.test.ts`, `docs/user/remote-access.md`	Development sharing preserves occupied handlers and reuses matching mappings. Unsharing and shutdown disable only matching handlers. Tests, command messages, and documentation reflect these behaviors.

Priority: ➖ Normal

Estimated code review effort: 4 (Complex) | ~45 minutes

Change: Bug fix

Sequence Diagram(s)

mermaid

1	sequenceDiagram
2	participant Pairing as resolveTailscalePairingBase
3	participant Serve as ensureTailscaleServe
4	participant CLI as tailscale CLI
5	Pairing->>Serve: Configure selected Serve target
6	Serve->>CLI: Request Serve status JSON
7	CLI-->>Serve: Return Serve status JSON
8	Serve->>Serve: Classify selected-port handler
9	alt Matching handler
10	Serve-->>Pairing: Reuse existing mapping
11	else Empty or verified replaceable handler
12	Serve->>CLI: Configure HTTPS Serve handler
13	else Occupied handler
14	Serve-->>Pairing: Return occupied-port error
15	end

Possibly related PRs

pingdotgg/t3code#8338: Implements ownership-aware Tailscale Serve status checks, handler preservation, verified replacement, and target-aware cleanup.

Suggested reviewers: juliusmarminge

Merge Risk: ⚪ Minimal · up to 33fe2

This change makes Tailscale Serve setup and cleanup preserve existing handlers that it cannot safely reuse. No blocking issue was found. It was not tested against a live Tailscale daemon, which is the normal remaining uncertainty.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 33fe2

The change reduces accidental overwrites and removals of existing HTTPS handlers. No newly introduced authorization bypass or broader remote exposure was established. Concurrent configuration changes and failure recovery remain limitations of the ownership guarantee.

Retained concerns No architecture-level concerns identified.

Security review details

Security Blast Radius

inferred — The direct configuration impact of each operation is the selected HTTPS port on the local Tailscale node and its proxied application. An incorrect ownership decision can disrupt another application's handler or repoint that endpoint; the head narrows this authority compared with unconditional base mutations.

Security Findings and Attack Paths

inferred — An actor already controlling the existing descriptor-serving backend could return the public environmentId expected by pairing. Equality therefore provides identity continuity, not authenticated proof of backend ownership. This comparison and development-server repointing behavior existed in the base; the head adds configuration restrictions, so it is not retained as an introduced or worsened concern.

Trust Boundaries and Controls

observed — A failed HTTPS probe no longer establishes permission to overwrite a mapping. Mutation authority also depends on decoded daemon configuration; cleanup independently checks the expected target and preserves nonmatching handlers. Empty cleanup succeeds without issuing a removal command.

Resilience and Maintainability Implications

inferred — The ownership check is point-in-time: status inspection and configuration mutation are separate commands. A concurrent writer can change the handler between them, allowing overwrite or removal despite the earlier classification. The base already mutated unconditionally, so this remains a pre-existing limitation rather than demonstrated PR regression.

Hardening Proposals

proposed — If handler preservation must hold under concurrent configuration writers, consider a daemon-supported conditional mutation or an ownership-generation protocol enforced by all participating writers. A second status read alone would not make the transition atomic.

Pre-merge checks | ^Passed 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	Passed	Check skipped because no linked issues were found for this pull request.
Description check	Passed	The description includes all required sections. It clearly explains the problem, the cross-component fix, the scope exception, focused verification, test results, and known limitations.
Title check	Passed	The title clearly and concisely describes the main change: existing Tailscale Serve handlers are no longer overwritten or removed.

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create a new PR

Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

_{Comment @coderabbitai help to get the list of available commands.}

CcoderabbitaiBotapproved these changes18d ago

Added 1 commit18d ago

fix(tailscale): reject unknown Serve configuration before mutations9e3a562

Llastobelusreviewed18d ago

Added 2 commits18d ago

chore: refresh upstream main after SSH test repaircca0d8b
test(tailscale): return Serve configuration in the dev sharing mock22c022b

Llastobelus18d ago

Addressed the CI failures in 22c022b294:

The SSH test's missing builder qualifiers caused both the typecheck and test failures. Upstream already repaired that in #18078; this branch now includes that mainline repair.
The dev-runner mock returned node-status JSON for serve status --json. It now returns the appropriate empty Serve configuration, so the strict decoder can exercise the successful shared-development path.

All 189 targeted tests pass, including the previously failing cases. Scoped scripts, SSH and server typechecks and lint of the changed TypeScript files pass. This update adds no production behavior changes beyond the reviewed handler-preservation fix.

Added 1 commit17d ago

fix(desktop): check Tailscale HTTPS port before saving settings33fe2ff

Llastobelusreviewed17d ago

Mmaria14d ago

Note

Written by gpt-6.1-sol on behalf of Maria

Thanks for investigating the Tailscale Serve handler overwrite. This changes 1,321 lines across 13 files, which exceeds what we can take over from an unvouched contributor under our current limited review capacity. Right now we are only taking focused bug fixes from outside contributors. Please preserve the handler-overwrite reproduction in an issue and agree on a smaller fix with a maintainer before requesting reconsideration.