Skip to content

fix(gateway): restore disabled auth on auto-start - #205

Merged
its-mash merged 7 commits into
mcpmux:mainfrom
Xuleileon:agent/restore-gateway-auth-setting-on-autostart
Jul 15, 2026
Merged

fix(gateway): restore disabled auth on auto-start#205
its-mash merged 7 commits into
mcpmux:mainfrom
Xuleileon:agent/restore-gateway-auth-setting-on-autostart

Conversation

@Xuleileon

Copy link
Copy Markdown
Contributor

Summary

  • centralize loading of the persisted inbound-auth setting
  • apply the setting to both manual and automatic gateway startup
  • add regression coverage for the secure default and persisted disabled state

Root cause

start_gateway restored gateway.auth_disabled after creating GatewayState, but the desktop auto-start path created the same state without restoring the setting. Relaunching the app therefore reverted a configured no-auth gateway to auth-required until the user toggled the setting again.

Impact

When Disable authentication is enabled, relaunching McpMux now preserves that behavior. A missing setting or repository read failure still defaults to authentication required.

Validation

  • cargo +1.88.0 fmt --all -- --check
  • cargo +1.88.0 test -p mcpmux --lib — 34 passed
  • cargo +1.88.0 clippy -p mcpmux --lib -- -D warnings -A clippy::uninlined_format_args

The narrow Clippy allowance is for a pre-existing Rust 1.88 lint in crates/mcpmux-core/build.rs; the changed files pass with warnings denied.

Signed-off-by: Xuleileon <xuleileon19932022@163.com>
@Xuleileon
Xuleileon marked this pull request as ready for review July 14, 2026 09:37
@Xuleileon
Xuleileon requested a review from its-mash as a code owner July 14, 2026 09:37
@its-mash

Copy link
Copy Markdown
Member

@claude review

@claude

claude Bot commented Jul 15, 2026

Copy link
Copy Markdown

Claude finished @its-mash's task in 1m 22s —— View job


Claude review

  • Gather context (fetch branch, read diff)
  • Understand the request (@claude review)
  • Review apps/desktop/src-tauri/src/commands/gateway.rs
  • Review apps/desktop/src-tauri/src/lib.rs
  • Post review feedback

Summary

This is a clean, well-scoped fix. The root cause described (auto-start built GatewayState without restoring the persisted gateway.auth_disabled setting, unlike start_gateway) checks out — I traced both code paths and confirmed the bug and the fix.

What's good

  • Correct fix, symmetric with existing pattern. The new load_gateway_auth_disabled_from_repo / load_gateway_auth_disabled helpers in gateway.rs (lines 220–234) mirror the existing load_network_access_from_repo / load_network_access pair exactly — good consistency with the established convention in this file.
  • No race condition. I checked ordering in both paths:
    • lib.rs:490-492auth_disabled is loaded (line 428-430) and applied to gw_inner_state before server.spawn() (line 537), so the HTTP listener never accepts connections with the wrong auth state.
    • gateway.rs:1056-1058 (start_gateway) — same ordering already existed and is preserved, just refactored to use the shared helper.
  • Safe default preserved. Both the missing-setting and repository-error cases fall through to unwrap_or(false) → auth required, matching the PR's stated "secure default" guarantee.
  • Regression tests are meaningful, not just padding. auth_remains_required_when_disable_setting_is_missing and persisted_disable_setting_is_restored_on_gateway_start exercise the real SqliteAppSettingsRepository against an in-memory DB rather than mocking the repo trait, so they'd have caught this exact bug (auto-start path bypassing the restore).

Minor observations (non-blocking)

  • get_gateway_auth_disabled (the #[tauri::command] at gateway.rs:1256-1264) still duplicates the same read-and-parse logic inline instead of delegating to the new load_gateway_auth_disabled_from_repo. Behaviorally they differ slightly on purpose (the command surfaces repo errors to the frontend via Result, the new helper swallows them into the secure default), so this isn't a bug — just a spot that could be tightened for a future cleanup if desired. Not worth blocking this PR on.
  • No changes needed to lib.rs beyond what's here — the auto-start path now matches start_gateway exactly in behavior for auth, network access, and public base URL restoration.

No correctness, security, or style issues found. The narrow -A clippy::uninlined_format_args allowance mentioned in the PR description is scoped to a pre-existing lint in mcpmux-core/build.rs, unrelated to these changes.

Verdict: LGTM.

@its-mash
its-mash merged commit 460da1f into mcpmux:main Jul 15, 2026
12 checks passed
crimsonsunset added a commit to crimsonsunset/mcp-mux that referenced this pull request Jul 17, 2026
Autonomous decisions:
- Adapted non-localhost consent note to fork's minimal launcher page — shows fallback immediately on network bind instead of upstream's full consent HTML
- Extracted load_gateway_auth_disabled_from_repo helper shared by manual start and auto-start paths — matches upstream mcpmux#205 pattern

Signed-off-by: crimsonsunset <jsangio1@gmail.com>
crimsonsunset added a commit to crimsonsunset/mcp-mux that referenced this pull request Jul 17, 2026
…liation

Merges upstream mcpmux/mcp-mux main (through mcpmux#215) into dev-rebased.
Resolves conflicts between upstream's original mcpmux#201/mcpmux#202/mcpmux#203/mcpmux#205/mcpmux#206
implementations and this fork's own reconciled port (Phases 1-4 of
upstream-client-mapping-reconciliation.md) by keeping the fork's version
wherever the two implement the same feature:

- Migrations: kept 036-038 (fork numbering/shape); removed upstream's
  020_inbound_client_api_keys.sql / 021_binding_type.sql /
  022_inbound_client_locked_space.sql as superseded (Decision 6 — one
  reconciled migration set, not a straight append)
- Resolver, repositories, domain, Tauri commands, Clients/Workspaces UI:
  kept the fork's Unbound-preserving, Tier 0/2 implementation over
  upstream's SpaceDefault-based one (Decisions 1/2)
- navigation.ts / HomePage.tsx: kept the fork's nav (Decision 5 — no
  Apps->Clients / Workspaces->Mapping rename); HomePage.tsx stays deleted
  (already removed by the fork's earlier Dashboard-unification commit,
  unrelated to this merge)
- Took upstream's unrelated CI hardening (apt cache pinning, Claude
  Code Actions workflow) and its new API-key e2e test as-is

Autonomous decisions:
- Fixed tests/rust/tests/database/migrations.rs upgrade-path regression
  test to reference migration 036/037/038 (not upstream's 020/021/022)
  and drop the partial indexes migration 037 creates before dropping
  the binding_type column, or the rollback step fails
- Fixed 3 e2e specs asserting upstream's rejected nav rename ('Mapping')
  to assert the fork's real label ('Projects') instead

Signed-off-by: crimsonsunset <jsangio1@gmail.com>
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