fix: persist refresh_token and add auto-refresh on token expiry

[rmichelena]

Problem

The MCP server breaks permanently after the access token expires (4h TTL). Three bugs cause this:

Bug 1: refresh_token discarded during authentication

In mcp_server_dash.py, the dash_authenticate() handler calls token_store.save(access_token) without passing the refresh_token that Dropbox returns in the OAuth response. The PKCE flow explicitly requests token_access_type=offline, but the token is never stored.

Bug 2: refresh_token lost due to keyring-first save strategy

save() attempts keyring first and only falls back to file storage if keyring fails. Since keyring stores a single opaque string, the refresh_token is always lost — even when the file fallback writes, it only contains {"access_token": "..."}.

This particularly affects headless Linux environments (Docker containers, servers) where keyring has no backend and silently uses a no-op store, but the bug manifests everywhere.

Bug 3: No token refresh in load()

load() validates the access token and clears everything if validation fails. There is no logic to use a refresh token to obtain a new access token. After 4 hours, the server becomes permanently unauthenticated and requires the full interactive OAuth flow again.

Fix

1. mcp_server_dash.py: Pass token_data.get("refresh_token") to save()
2. token_store.py save(): Always write to file (with both tokens) regardless of keyring success. Keyring is used as a secondary store for the access token only.
3. token_store.py load(): Try access token first. On failure, attempt refresh via dropbox.Dropbox(oauth2_refresh_token=..., app_key=...). Persist the refreshed token.
4. _read_token_file(): Returns dict[str, str | None] with both tokens instead of a single string.
5. New _save_to_file() method for clean file persistence with chmod 0600.

Testing

• Verified on headless Linux (Docker container) with Dropbox Plus account
• Token persists across server restarts with both access_token and refresh_token
• After access token expiry, load() successfully refreshes via SDK and continues operating
• OAuth PKCE flow (token_access_type=offline) now correctly round-trips the refresh token

[CLAassistant]

All committers have signed the CLA.

[rmichelena]

🔍 Multi-Model PR Review: dropbox/mcp-server-dash #11

PR: fix: persist refresh_token and add auto-refresh on token expiry
Branch: fix/token-refresh-persistence → main
Commit: ce10a84e3d803b88fcba5b93358ca9ca58e2ac69
Files: src/mcp_server_dash.py, src/token_store.py (+85 / -40)

Reviewers: GPT-5.5 · GLM-5.1 · DeepSeek V4 Pro

---

Summary

The PR adds refresh token persistence and auto-refresh-on-expiry logic to DropboxTokenStore. The core idea is sound and the implementation is mostly functional, but there are several issues around error handling, test coverage, keyring consistency, and security that should be addressed before merge.

Verdict: 🟡 Approve with changes requested

---

Findings

🔴 HIGH — Tests not updated for _read_token_file return type change (4 breakages)

File: tests/test_token_store.py — test_read_token_file_invalid (line 131), test_save_both_keyring_and_file_fail (line 113)
Flagged by: DeepSeek V4 Pro · ✅ Verified by orchestrator

_read_token_file now returns dict[str, str | None] instead of str | None. Three parametrized test cases still assert is None and will fail. Separately, test_save_both_keyring_and_file_fail expects RuntimeError but the new save() no longer raises it.

Additionally, no new tests were added for the refresh-token flow, _save_to_file, or the updated save(token, refresh_token) signature.

Fix: Update the 4 existing test cases to match new return types. Add tests for: successful refresh flow, missing APP_KEY, refresh failure, and save() with refresh_token parameter.

---

🔴 HIGH — save() silently swallows all persistence failures

File: src/token_store.py — save() (lines 159-176)
Flagged by: GPT-5.5 · GLM-5.1 · DeepSeek V4 Pro (3/3 agreement)

The old save() raised RuntimeError if both keyring and file storage failed. The new version catches all exceptions from both backends, logs them, and always updates in-memory state. The caller's except block in dash_authenticate (expecting to catch failures) is now dead code. Users are told "Successfully authenticated" even when nothing was persisted.

Trace: save() → _save_to_file() raises IOError → caught and logged → keyring.set_password() raises → caught and logged → method returns normally → caller's except never fires.

Fix: Re-raise if both storage backends fail, or have save() return a bool and check it in the caller.

---

🔴 HIGH — Transient network errors during validation trigger clear(), destroying valid tokens

File: src/token_store.py — load() (lines 103-139)
Flagged by: GPT-5.5 · GLM-5.1 · DeepSeek V4 Pro (3/3 agreement)

The broad except Exception in the access-token validation path catches network errors (not just auth errors). If the refresh path also fails due to the same transient network issue, self.clear() deletes both tokens from keyring and file. A temporary network blip at startup causes permanent credential loss.

Trace: App starts → network down → dbx.users_get_current_account() raises httpx.ConnectError → caught by broad except Exception → refresh path also fails → self.clear() → tokens deleted → user must re-authenticate.

Fix: Distinguish network/transport errors from auth errors. On network errors, return False without clearing. Only call clear() on definitive auth failures.

---

🟡 MEDIUM — Runtime token expiry still forces re-auth and deletes refresh token

File: src/mcp_server_dash.py — dash_company_search() (line 342), dash_get_file_details() (line 388)
Flagged by: GPT-5.5

The refresh logic only runs during token_store.load(). Once the server is running, dash_company_search() and dash_get_file_details() still build DashAPI(token_store.access_token) directly. On a 401, they call token_store.clear(), which deletes the refresh token too — defeating the purpose of this PR.

Fix: On 401 during runtime, attempt a token refresh before clearing credentials. Or proactively refresh before the token expires.

---

🟡 MEDIUM — Keyring never updated after token refresh (stale token permanently)

File: src/token_store.py — load() (lines 136-142)
Flagged by: GPT-5.5 · DeepSeek V4 Pro (2/3 agreement)

After a successful refresh, the new access token is saved to file only. The keyring entry keeps the old expired token permanently. Every subsequent startup tries the stale keyring token first, wastes an API call, fails with AuthError, then falls through to refresh again.

Fix: After successful refresh, also update keyring: keyring.set_password(KEYRING_SERVICE, KEYRING_USERNAME, new_access_token) wrapped in suppress(Exception).

---

🟡 MEDIUM — Accesses private Dropbox SDK attribute _oauth2_access_token

File: src/token_store.py — line 120
Flagged by: GLM-5.1

new_access_token = dbx._oauth2_access_token

This accesses a private attribute of the Dropbox SDK. If the attribute is renamed or removed in a future SDK release, AttributeError propagates to the outer except Exception → self.clear() → all tokens lost.

Fix: Call the Dropbox token endpoint directly with httpx to get the response JSON containing the new access token, or use the SDK's public token management flow.

---

🟡 MEDIUM — Long-lived refresh tokens always written to plaintext file even when keyring works

File: src/token_store.py — save() (line 155+)
Flagged by: GPT-5.5

The PR always writes both tokens to ~/.mcp-server-dash/dropbox_token.json before attempting keyring. The old code only used the file as a fallback. Since refresh tokens are long-lived credentials, this is a security downgrade for systems with a working keyring. chmod(0o600) helps but doesn't match keyring-level protection.

Fix: Only write to file when keyring fails (as the old code did), or store the refresh token in keyring under a separate key.

---

Recommended Fix Order

1. Fix broken tests — blocking; CI will fail on merge
2. Restore save() failure propagation — prevents silent data loss
3. Guard clear() against transient errors — prevents credential destruction
4. Update keyring on refresh — eliminates unnecessary API calls and eventual breakage
5. Replace dbx._oauth2_access_token with public API or direct token endpoint call
6. Add runtime refresh in dash_company_search / dash_get_file_details — completes the PR's stated goal
7. Restrict plaintext file writes to keyring-fallback only

---

Reviewers: GPT-5.5 ✅ · GLM-5.1 ✅ · DeepSeek V4 Pro ✅ · 3/3 completed

[rmichelena]

🔍 Multi-Model PR Review (Round 2): dropbox/mcp-server-dash #11

PR: fix: persist refresh_token and add auto-refresh on token expiry
Commit reviewed: 5ba57927901bafc60c3ea4b3cda024c7198c8e38 ("fix: address PR review — all 7 findings")
Files: src/mcp_server_dash.py, src/token_store.py, tests/test_token_store.py (+348 / -88)

Reviewers: GPT-5.5 · GLM-5.1 · DeepSeek V4 Pro

---

Verification of Prior Findings

#	Finding	Verdict	Agreement
1	HIGH: Tests not updated for _read_token_file return type	✅ FIXED	3/3
2	HIGH: save() silently swallows all persistence failures	✅ FIXED	3/3
3	HIGH: Transient network errors trigger clear()	✅ FIXED	3/3
4	MEDIUM: Runtime token expiry forces re-auth	✅ FIXED	3/3
5	MEDIUM: Keyring never updated after refresh	✅ FIXED	3/3
6	MEDIUM: Accesses private SDK attribute _oauth2_access_token	✅ FIXED	3/3
7	MEDIUM: Refresh tokens always written to plaintext file	✅ FIXED	3/3

All 7 prior findings confirmed fixed. ✅

---

New Findings

🟡 MEDIUM — _persist() silently loses refresh_token when its keyring write fails

File: src/token_store.py — _persist() (~line 245)
Flagged by: GPT-5.5 · GLM-5.1 · DeepSeek V4 Pro (3/3 agreement)

_persist() tracks keyring_ok based only on the access_token keyring write. If access_token succeeds but the refresh_token keyring write fails (e.g. keyring backend hiccup on second write), the exception is suppressed, _persist() returns success, and no file fallback is written. The refresh token lives only in memory. After restart, the access token is durable but the refresh token is gone — and once the access token expires, the user must re-authenticate.

Trace: save("A", "R") → _persist() → keyring.set_password(...ACCESS, "A") succeeds → keyring.set_password(...REFRESH, "R") raises → suppressed → _persist() returns (keyring_ok=True) → no file written → restart → keyring has "A" but no "R" → when "A" expires, no refresh possible.

Fix: Treat access + refresh token persistence as one transaction. If the refresh_token keyring write fails, either fall back to file storage for both tokens or raise so the caller knows persistence was incomplete.

---

🟡 MEDIUM — Transient failures during refresh still destroy valid credentials

File: src/token_store.py — load() line 158, try_refresh() line 200
Flagged by: GPT-5.5

While the fix correctly avoids clear() for transient errors during access-token validation, the refresh-token exchange path still collapses all failures to None → clear(). A temporary network outage during the refresh token exchange permanently deletes the refresh token.

Trace: Stored expired access + valid refresh → _do_refresh() hits httpx.ConnectError → returns None → load() line 158 else branch → self.clear() → refresh token deleted.

Fix: Make _do_refresh() distinguish transient errors (network, 5xx) from definitive auth failures (invalid_grant). Only clear() on definitive failures; preserve credentials and return False on transient ones.

---

Verdict

🟢 LGTM — all 7 original findings properly addressed.

The two new MEDIUM findings are edge cases around partial persistence failures and transient errors during refresh. They're worth addressing but are not blockers for merge — the PR is a substantial improvement over both the original code and the first iteration.

---

Reviewers: GPT-5.5 ✅ · GLM-5.1 ✅ · DeepSeek V4 Pro ✅ · 3/3 completed