Skip to content

fix(web): Tailscale HTTPS stays on, and can be turned off, when this machine can't reach its own URL - #15027

Open
tiliakoos wants to merge 7 commits into
pingdotgg:mainfrom
tiliakoos:fix/14746-tailscale-https-switch
Open

tiliakoos wants to merge 7 commits into
pingdotgg:mainfrom
tiliakoos:fix/14746-tailscale-https-switch

Conversation

@tiliakoos

@tiliakoos tiliakoos commented Oct 3, 2026 •

Copy link
Copy Markdown
Contributor

A failed Tailscale HTTPS self-probe made saved Serve configuration appear off, and losing the endpoint removed the way to disable it.

Drive the switch from the saved Serve flag and keep it visible while that flag is enabled. Show probe health separately, retain the existing unavailable-endpoint exclusion from pairing links/QR, and let disable confirmation work without an endpoint. No backend, contract, or pairing-policy changes.

Maintainer takeover of #15027, preserving @tiliakoos's commits and merging current main. Reduced the original four added cases to three regressions in the existing logic suite and removed a redundant derived URL and unused disable callback argument.

Verification

  • Focused logic suite: 24→23 tests, all passing. Restoring main's row-state behavior produces 3 failures / 20 passes; restoring the fix produces 23 passes.
  • Web typecheck and changed-file lint/format pass.
  • Integrated current web renderer, isolated worktree state, desktop bridge stub: failed-probe saved-on state, missing-endpoint saved-on state, cancel, confirm disable, saved-off/available setup, and disable failure/retry. Confirm calls setTailscaleServeEnabled({enabled: false, port: 443}) even without an endpoint.
  • Before/after use the actual main/head source with the same failed-probe fixture and 1280×800 viewport. This is real component rendering with stubbed desktop IPC; it does not verify an external Serve mapping, Electron relaunch, or a real remote device.
  • Live data seeding was unavailable because this VM's read-only live snapshot has newer schema migrations than this checkout. Used fresh disposable state. No production state was written.

Proof

Before: main displays off although the saved flag is enabled.

Before: saved Serve enabled with failed probe appears off

After: saved state stays on, with a separate reachability warning.

After: saved Serve enabled stays on with probe warning

The control also remains available when no endpoint is reported.

After: saved Serve enabled without a MagicDNS endpoint

Missing-endpoint disable confirmation, using the stubbed desktop bridge:

https://gh-file-drop-api-prod-mi5fy3sowv63ufte.pinglabs.workers.dev/f/65e4e77c2014aa6f/15027-disable.mp4

Independent review: two full-PR rounds by Claude Opus and Codex Sol found no code bugs. Removed duplicate QR assertions after round one; round two was clean. Published head 69bd52e8f0; current-head build/typecheck, lint, web/server tests, and status checks passed. Platform-inapplicable jobs skipped; one fingerprint run was cancelled and superseded by a successful fingerprint run. No inline review findings remain. Real Electron/Tailscale verification remains outstanding as described below.

Limits

Real Tailscale Serve shutdown/re-enable and Electron restart still need desktop verification. macOS, Windows, iOS, and Android were not run on this headless Linux VM. Browser-hosted web and mobile do not own this desktop-local setting; shared pairing behavior is unchanged.

Implemented with Codex gpt-6.1-sol through T3 Code; tests by Codex gpt-6-astra; independent adversarial review by Claude claude-opus-5-5 and Codex gpt-6-sol.

…machine can't reach its own URL

The switch followed the endpoint self-probe instead of the saved setting, so a
failed MagicDNS lookup showed it off and left no way to disable the route.
Copilot AI balanced review requested due to automatic review settings October 3, 2026 04:50

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:M 30-99 changed lines (additions + deletions). labels Oct 3, 2026
@macroscopeapp

macroscopeapp Bot commented Oct 3, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at 17c9a99

Macroscope's review found this PR approvable — This is a narrowly scoped Connections settings bug fix that reconciles the Tailscale switch with the saved Serve state while preserving pairing health checks and existing disable behavior. It adds focused tests without changing backend contracts, defaults, deployment, or sensitive packages.

You can add or adjust custom eligibility rules. Learn more.

@coderabbitai

coderabbitai Bot commented Oct 3, 2026 •

Copy link
Copy Markdown
Contributor

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: 105e14e8-362b-4c68-89b5-b0c0243c54b7
📥 Commits

Reviewing files that changed from the base of the PR and between b8dabcc and 594eade.

📒 Files selected for processing: 3
  • apps/web/src/components/settings/ConnectionsSettings.logic.test.ts
  • apps/web/src/components/settings/ConnectionsSettings.logic.ts
  • apps/web/src/components/settings/ConnectionsSettings.tsx

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

The Tailscale HTTPS settings row now uses the saved Serve setting for its switch state. Endpoint availability determines reachability and whether the row displays the endpoint URL.

Changes: Tailscale HTTPS settings row

Layer / File(s) Summary
Resolve the row state
apps/web/src/components/settings/ConnectionsSettings.logic.ts, apps/web/src/components/settings/ConnectionsSettings.logic.test.ts
resolveTailscaleHttpsRowState derives switch visibility, checked state, URL, and reachability. Tests cover available, unavailable, and absent endpoints with Serve enabled or disabled.
Apply resolved state in settings
apps/web/src/components/settings/ConnectionsSettings.tsx
The row uses the resolved state for its description, reachability warning, switch visibility, and checked value. Disabling Serve calls the handler without an endpoint argument.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: utkarshusername

Merge Risk: ⚪ Minimal · up to 594ea

The Tailscale HTTPS switch now follows the saved setting, so users can turn it off when the machine cannot reach its own URL. Pairing still requires a reachable endpoint. No merge-blocking risk was found.

Security architecture risk: 🔵 Low · up to 594ea

The change restores access to an existing exposure control without changing pairing eligibility or management permissions. No newly expanded access was identified. The remaining uncertainty concerns pre-existing shutdown and recovery behavior: a saved disabled setting is not confirmation that the external Serve mapping was removed.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The affected authority remains management of the local desktop backend's configured Serve port. The change does not add an attacker-supplied destination, broaden the configured network audience, or create cross-environment management authority.

Trust Boundaries and Controls

  • observed — Separating saved configuration from health does not promote an unavailable URL into a pairing destination. Default endpoint selection and endpoint copy options still reject unavailable endpoints before constructing credential-bearing pairing URLs. Disable also retains its desktop-bridge and environment-maintenance prerequisites.

Resilience and Maintainability Implications

  • inferred — Pre-existing persistence and cleanup are not one atomic revocation operation. Persistence can finish before detached shutdown, and cleanup is not guaranteed after failed setup or removal. The comparison does not establish that this PR worsens those failure paths, but interrupted shutdown and stale external mappings remain incompletely verified recovery cases.
Pre-merge checks: Passed 3 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Linked Issues check Warning The change satisfies the main #14746 row behavior. resolveTailscaleHttpsRowState uses tailscaleServeEnabled for switch state, keeps the switch visible without an endpoint, reports probe health sep… Update the Authorized clients visibility condition to include desktopServerExposureState?.tailscaleServeEnabled === true. Add a regression test for Network access off with saved Serve enabled.
Passed Passed checks: 3 passed
Check name Status Explanation
Out of Scope Changes check Passed The whole-PR diff is limited to the Tailscale HTTPS row resolver, its Connections settings integration, and focused regression tests. The handler signature change supports the same disable flow. No un…
Title check Passed The title is concise, specific, and accurately summarizes the primary fix: preserving the saved Tailscale HTTPS state and allowing it to be disabled when the local URL is unreachable.
Description check Passed The description explains the problem, implementation, scope, verification results, screenshots, known limitations, and test environment. It does not use the template headings exactly, and the approval…
Full details: Out of Scope Changes check

Explanation

The whole-PR diff is limited to the Tailscale HTTPS row resolver, its Connections settings integration, and focused regression tests. The handler signature change supports the same disable flow. No unrelated desktop, server, contract, pairing, or Tailscale IP probing changes are present.

Full details: Description check

Explanation

The description explains the problem, implementation, scope, verification results, screenshots, known limitations, and test environment. It does not use the template headings exactly, and the approval evidence is indirect, but it provides the required information and is substantially complete.

Full details: Linked Issues check

Explanation

The change satisfies the main #14746 row behavior. resolveTailscaleHttpsRowState uses tailscaleServeEnabled for switch state, keeps the switch visible without an endpoint, reports probe health separately, and leaves pairing selection health-gated. However, the Authorized clients visibility condition is unchanged. It still depends on Network access or an available Tailscale endpoint, not on the saved tailscaleServeEnabled flag. Authorized clients can therefore remain hidden while Serve is configured and the probe fails.

Resolution

Update the Authorized clients visibility condition to include desktopServerExposureState?.tailscaleServeEnabled === true. Add a regression test for Network access off with saved Serve enabled.

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

…-https-switch

# Conflicts:
#	apps/web/src/components/settings/ConnectionsSettings.logic.test.ts
#	apps/web/src/components/settings/ConnectionsSettings.tsx
@tiliakoos

Copy link
Copy Markdown
Contributor Author

Merged upstream/main (70d60e03a7) in b8dabcc519, so this is mergeable again. #20515 rewrote this component for the React Compiler, so the conflicts were the handleStartTailscaleServeDisable signature (now a plain function with no endpoint argument, since the switch can be turned off without one) and the memoized endpoint derivations (now plain expressions, with resolveTailscaleHttpsRowState among them). The row in renderTailscaleRow() merged by itself. The diff against main is still the three files (+104 −11).

Rerun on the merged branch: ConnectionsSettings.logic.test.ts 21 passed; lint, format and the web typecheck clean. The dev desktop app checks in the description are from the 2026-10-07 run and were not retaken.

#18758 edits the same row's no-endpoint text; the two changes do not overlap in behaviour, and whichever lands second has a one-line conflict in that ternary.

…-https-switch

# Conflicts:
#	apps/web/src/components/settings/ConnectionsSettings.logic.test.ts
#	apps/web/src/components/settings/ConnectionsSettings.tsx
@tiliakoos

Copy link
Copy Markdown
Contributor Author

Merged upstream/main (ab6be13d43) in 594eade81d, so this is mergeable again. #18758 landed on the same row and added missingTailscaleEndpointDescription for the no-endpoint text. The row keeps this fix's description while the switch is on and uses #18758's helper while it is off with no endpoint, so "Turn on Network access to detect Tailscale." still shows when detection was skipped. The other conflict was the two imports. The diff against main is still the three files (+104 −11).

Rerun on the merged branch: ConnectionsSettings.logic.test.ts 24 passed, and the two regression cases still fail with upstream's switch logic put back in the helper; lint, format and the web typecheck clean. The dev desktop app checks in the description are from the 2026-10-07 run and were not retaken.

@juliusmarminge juliusmarminge self-assigned this Oct 19, 2026
@juliusmarminge

Copy link
Copy Markdown
Member

Taking over this one for batch ssh-relay-remote-05. I will preserve contributor history, merge current main, reduce the regression tests, and verify the saved-state switch and disable behavior with independent reviews. This remains unmerged.

This branch has not been deployed

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

Labels

size:M 30-99 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants