TW-6922: CLI OAuth login, MCP serve over OAuth, and dashboard commands signed in by nylas oauth login

[radenkovic]

Summary

OAuth support in the CLI for the Dashboard OAuth epic (TW-6922). The CLI logs in to the Nylas authorization server in dashboard-account as a static public client with PKCE, uses that session for nylas mcp serve, and, as of TW-7142, for every nylas dashboard command too, so nylas oauth login is the only login a user needs.

Server side: nylas/dashboard-v3#2633.

Changes

TW-6922 — OAuth login

• OAuth authorization server client with RFC 7636 PKCE, and a login service with token rotation
• nylas oauth login, status, token and logout, verified against a live authorization server

TW-7133 — Static public client

• Replace dynamic client registration with the built-in public client b3a94d82-fc7d-4a22-803e-e603ae0f735c and its loopback redirect URIs (any port, RFC 8252 §7.3). NYLAS_OAUTH_CLIENT_ID overrides it for a local or dev server, validated rather than silently ignored.

TW-7135 — One refresher per machine

• Several nylas mcp serve processes share one keyring session, and the server burns the whole refresh family when a rotated token is replayed. Every write to the stored session now happens under a cross-process lock, so concurrent refreshes no longer sign the user out.

TW-7134 — nylas mcp serve --auth oauth

• Proxies to the hosted MCP server with the session from nylas oauth login --for mcp, asking a credential source before every request and refreshing near expiry through the shared lock. The API key stays the default, so existing assistant configs are unchanged.

TW-7136 — Stateless MCP protocol; OAuth for mcp install

• The proxy follows the hosted server's stateless protocol: no Mcp-Session-Id, and it sends the Mcp-Method / Mcp-Name routing headers. mcp install can write an OAuth-authenticated config.

TW-7142 — Dashboard commands signed in by nylas oauth login

• After nylas oauth login, the CLI exchanges the access token at /auth/cli/oauth/exchange for a DPoP-bound dashboard session, so apps, API keys, domains and orgs work without nylas dashboard login.
• That session is marked in the keyring (origin and expiry). The server refuses to refresh it, so the CLI re-exchanges a fresh access token about a minute before it expires, on every path that loads dashboard tokens. A renewal keeps the active app; a new login resets it.
• nylas oauth logout also ends a dashboard session it created and leaves one from nylas dashboard login alone. nylas dashboard login and config reset clear the markers.
• The oauth package registers its login service as the token source, so renewals share the machine-wide refresh lock. A failed exchange is a warning on oauth login, not a failure, because the OAuth login itself succeeded.

Hardening after review (ce3bd92)

• The server's metadata is checked against the URL the CLI was pointed at: the base URL must be https (loopback excepted), the issuer must match it, and every endpoint must be on the issuer's host. A loopback base may advertise an https tunnel issuer.
• A stored session is tied to the server that issued it. Pointed at a different server, the CLI refuses to use or refresh it, and oauth logout clears it locally without revoking it on the wrong server.
• The OAuth client and the MCP proxy no longer follow redirects, so a bearer token or DPoP proof is never replayed to another host.
• The MCP proxy copies a request's arguments before it adds the grant, so the caller's request is never modified.
• dpop.New only generates a key when none is stored; any other keyring read error now fails instead of silently replacing the key.
• The encrypted file keyring takes a cross-process file lock (.secrets.lock) around every read and write, so two processes can no longer lose each other's writes.
• The session lock sits beside the encrypted file store, or in ~/.config/nylas/oauth-session.lock otherwise.
• nylas oauth login --region us|eu sets the sign-up region. A bad value in the flag is an error; a bad configured region is ignored for a plain login.
• config reset clears the OAuth session locally (documented as local-only).

Session durability and server binding (ec3d285)

• A dashboard session (from either login) records the account URL and both gateway URLs it was issued for. If any of them changes, the session is refused instead of being sent, and nylas dashboard logout clears it locally without contacting the new server. A session stored before this is adopted by the first server that reads it, except one exchanged from OAuth whose issuing account server is known to differ.
• A refresh finishes on its own 30s deadline even if the caller cancels, because the server has already rotated the token. If the rotated token cannot be stored, a still-stored spent refresh token is deleted rather than replayed.
• Session writes go server/resource, refresh token, access token, then expiry, so a partial write reads as expired and recovers with the new refresh token. The ID token is no longer stored (nothing verified or read it).
• The system keyring splits a value larger than one keychain item (Windows: 2560 bytes) into chunks under a fresh generation id, writing the header last.
• nylas dashboard orgs switch on an OAuth session signs in again with the same scopes and resource, and the organization is chosen on the consent page. --org fails if a different one was chosen.

Review fixes #1–#5 (fc7b0e8)

• oauth login clears the stored session under the lock before it writes the new one, and clears it again if the write fails. A failed login against another server can no longer leave the old refresh token filed under the new server's name.
• config reset clears the dashboard session and the OAuth session under their cross-process locks, so an in-flight mcp serve refresh cannot write the session back.
• Dashboard session renewal, login and clear run under a new dashboard-session.lock and check expiry again once they hold it. Concurrent renewals therefore exchange once and never interleave keys. Locks are always taken in the same order: dashboard, then OAuth, then .secrets.lock.
• The renewer asks for an access token that is valid for at least the renew window, so a re-exchange gets a new expiry.
• oauth login keeps an existing nylas dashboard login session for the configured server instead of silently replacing it, and says so.

Callback state checked first, review item #6 (5d38210)

• Both login flows set the expected state on the callback server before they open the browser. A fast redirect is no longer checked against an unset state.
• A callback with a missing or wrong state gets a 400, and the login keeps waiting. Previously, any page open in the browser could abort a login by hitting the fixed port.
• The error code is allow-listed (^[a-z_]{1,64}$) before it reaches the CLI's error text.

Server contract: first-party clients may exchange (c28253a)

• nylas/dashboard-v3#2776 adds a second built-in client, Nylas Mail. It also changes the exchange to accept tokens from built-in first-party clients marked exchangesForDashboardSession, instead of the CLI's id only. The CLI's client id and redirects are unchanged. The scope it now requires is the next section.
• docs/COMMANDS.md now says that with a NYLAS_OAUTH_CLIENT_ID override, login succeeds but the dashboard commands stay signed out. It also corrects the access-token lifetime to the server default of 15 minutes (it said one hour).

MCP proxy: no reply to a failed notification (a137da6)

• Found testing against prod: when the hosted MCP server refused the token, the proxy answered notifications/initialized with a JSON-RPC error ("id": null). JSON-RPC 2.0 forbids any reply to a notification. The failure now goes to stderr. An explicit "id": null is still a request and is still answered. Mutation-checked.

The exchange needs the person's consent: dashboard.session (3544529)

• nylas/dashboard-v3#2776 now refuses the exchange unless the token's scope carries dashboard.session. The consent screen shows it as "Use the Nylas Dashboard as you, with your full role in this organization". Before this, a token consented to openid email offline_access became a session with the person's full org role.
• nylas oauth login requests it by default, and --for mcp requests it too, so the dashboard commands stay signed in after either. The MCP server ignores it.
• It is never dropped as "not offered". The server accepts it only from the built-in CLI client and deliberately does not list it in scopes_supported.
• With a NYLAS_OAUTH_CLIENT_ID override it is left out and reported, because the server would refuse the whole request.
• nylas dashboard orgs switch adds it when it signs in again, so a session from before keeps working.
• Before the exchange, the renewer reads the token's (unverified) scope. If dashboard.session is missing, it says "run nylas oauth login again and allow dashboard access" instead of passing on a bare INVALID_TOKEN. The server still decides.
• Updated docs/COMMANDS.md, docs/commands/mcp.md and nylas oauth login --help.

Testing

• go test ./... passes, golangci-lint run reports 0 issues
• New unit tests for every hardening item. The MCP fixes and the file lock were mutation-checked: each test fails with its fix reverted.
• Earlier unit tests: the exchange adapter (path, body, no Authorization header, DPoP), the renewer (login, skip for dashboard-login sessions, keep a fresh session, re-exchange near expiry, keep the active app), apikeys create on an expired OAuth session renewing first, refresh re-exchanging instead of refreshing, and the oauth login / logout wiring
• After fc7b0e8, 5d38210 and c28253a: go test passes for internal/app/dashboard, internal/app/oauthlogin, internal/cli/{oauth,config,dashboard,common}, internal/adapters/dashboard and internal/domain. I did not rerun the full ./... suite or golangci-lint, so CI covers those.
• The server half of the exchange passes its e2e spec against a live local stack (07-oauth-cli-session-exchange.spec.ts in #2633)
• Manual, against a local stack: the built binary through a real browser consent, then nylas dashboard apps create and nylas dashboard apps apikeys create, all succeeded
• Manual: a session issued by localhost is refused when the CLI is pointed at production, and nothing is sent
• After 3544529: go test ./internal/... passes and golangci-lint reports 0 issues on the touched packages. New tests cover:
  • the default and MCP scope lists;
  • the first-party exemption from discovery filtering (mutation-checked);
  • the client-id override leaving the scope out;
  • relogin adding the scope without duplicating it;
  • the renewer refusing a token without the scope before asking the server.
• Not yet run against a live stack. It needs nylas/dashboard-v3#2776 running locally.
• New tests for the server binding, refresh durability, write order, keyring chunking and the org switch. The server check, the cancellation fix, the spent-token drop and the write order were mutation-checked

Release order

Deploy dashboard-account with nylas/dashboard-v3#2633 and nylas/dashboard-v3#2776 before releasing this. #2776 now blocks this release: a server without it does not know dashboard.session and refuses the whole authorization request with invalid_scope, so nylas oauth login would fail. No released CLI uses the exchange yet, so nothing in the field breaks when #2776 deploys.

Related

• TW-6922 (epic), TW-7133, TW-7134, TW-7135, TW-7136, TW-7142, TW-7426
• nylas/dashboard-v3#2633, nylas/dashboard-v3#2776

🤖 Generated with Claude Code

[qasim-nylas]

Deep review: Claude + Codex

Reviewed at ec3d285. Three independent passes: Claude on the OAuth / dashboard-session core, Claude on MCP proxy / keyring / file locks, and Codex across the whole diff. Every finding below was re-checked against the code; one Codex finding (orphaned chunks when a chunk write fails) was a false positive and has been dropped, since chunkHeader(gen, i) already deletes exactly chunks 0..i-1.

Verdict: the security design is solid. The fixes needed are 1 P1 and 9 P2s, mostly about partial failures and cross-process consistency. There are no P0s.

Build and tests:

• go build, go vet and golangci-lint are clean, natively and with GOOS=windows.
• go test -race passes on every touched package.
• Integration tests compile with -tags integration.

Sev	#	Area	Summary
P1	1	oauthlogin	A failed re-login against a new server leaves the old server's refresh token filed under the new server's name
P2	2	config reset	config reset clears the OAuth and dashboard sessions without the session lock
P2	3	dashboard	Renewing the dashboard OAuth session is not serialized across processes, so the multi-key write can interleave
P2	4	dashboard	The renew-before window (60s) is longer than the access-token leeway (30s), so re-exchanges do nothing
P2	5	cli/oauth	oauth login silently replaces and orphans an existing dashboard login session
P2	6	callback server	The expected state is set asynchronously, and any stray or mismatched callback aborts the login
P2	7	keyring	A chunked keychain read that races a refresh fails and tells the user to sign in again
P2	8	mcp	Every MCP request reloads 7–9 secrets, which is very expensive on the encrypted file store
P2	9	keyring	Encrypted-store Get now needs a writable config dir
P2	10	rules	proxy.go is 616 lines, over the 600-line limit in CLAUDE.md
P3	11–24	various	See below

---

P1

1. A failed re-login against a new server leaves the old tokens filed under the new server's name

Where: internal/app/oauthlogin/store.go:91-137 (saveTokens) and internal/app/oauthlogin/service.go:215-217 (Login)

What's wrong: saveTokens writes KeyOAuthIssuer and KeyOAuthServerURL first, then scope, then the refresh token. The doc comment says a later failure is safe because loadSessionForServer will refuse the old tokens. That isn't true: the stored server URL now equals the configured one, so checkIssuer passes. Login doesn't clean up when saveTokens fails.

Failure scenario:

1. The user is logged in to server A.
2. They set NYLAS_DASHBOARD_ACCOUNT_URL to server B and run nylas oauth login.
3. One keychain write after the server URL fails (the user clicks Deny on the macOS prompt, or hits a size limit).
4. The next command sends A's refresh token to B's token endpoint, and A's access token to B's userinfo endpoint, dashboard exchange and MCP routing.

A scratch test reproduced this: refreshCalls=[<A's refresh token>] after ServerURLValue changed to B and the refresh-token write failed.

Fix: inside the lock, clear the old session before writing a new one, and clear again if the write fails.

if err := s.withSessionLock(ctx, func() error {
    if err := s.clearSession(); err != nil { // the new login replaces whatever was there
        return err
    }
    if err := s.saveTokens(metadata.Issuer, opts.Resource, tokens); err != nil {
        return errors.Join(err, s.clearSession())
    }
    return nil
}); err != nil {
    return nil, err
}

Then correct the saveTokens comment. The write order still matters for refresh, but it no longer protects a server switch.

Test:

• Log in as A, then set f.client.ServerURLValue = "https://b" and make writes to KeyOAuthRefreshToken fail.
• Login must return an error.
• Heal the store, advance the clock and call AccessToken. Expect ErrOAuthNotLoggedIn and len(RefreshCalls) == 0.

---

P2

2. config reset clears the OAuth and dashboard sessions without the session lock

Where: internal/cli/config/reset.go:66-74 and internal/app/oauthlogin/store.go:146

What's wrong: Logout takes the session lock specifically so an in-flight refresh can't write a rotated session back after the clear. config reset calls oauthlogin.ClearSession(secretStore) and clearDashboardCredentials(secretStore) with no lock.

Failure scenario: a nylas mcp serve --auth oauth process run by Claude Desktop or Cursor is mid-refresh. Reset prints "✓ OAuth session cleared", then the proxy stores the rotated tokens and the session is back.

Fix:

• Export a locked local clear, for example oauthlogin.ClearSessionLocked(ctx, secrets) or a method on Service that wraps ClearSession in withSessionLock, and use it from reset.go.
• Clear the dashboard OAuth session in the same critical section (see TW-5027: Demo friendly #3).

Test: hold the session lock from a goroutine that writes a refresh token after reset starts. Once reset returns, no OAuth key may remain.

3. Renewing the dashboard OAuth session is not serialized across processes

Where: internal/app/dashboard/oauth_session.go:64-74, 77-105 and internal/app/dashboard/auth_service.go (replaceSecretValues)

What's wrong: EnsureFresh → exchange → replaceSecretValues writes 7–9 keys with no cross-process lock. The file store only locks each individual Get/Set. The PR says every session write is locked, but that only covers the OAuth keys.

Failure scenario:

• Two nylas dashboard … commands, or a command and oauth login, both see a near-expiry session.
• Both exchange, and their Set calls interleave.
• The stored user_token comes from exchange A while org_token and expiry come from B.
• The rollback in replaceSecretValues can also restore stale values over the other process's write.
• A related case (Codex): oauth logout clears the dashboard session before it takes the OAuth lock, so a concurrent login can store a fresh dashboard session after logout reports success.

Fix:

• Run the whole exchange-and-store under the same ports.Lock used for the OAuth session. Reuse sessionLock(secrets).
• Once the lock is held, re-check EnsureFresh's expiry, so a process that waited uses the session the first one stored instead of exchanging again.
• Take the same lock for ClearIfOAuth.
• In logout.go, do the dashboard clear and svc.Logout under one lock hold, or at least clear the dashboard session after the OAuth tokens are revoked.

Test: run two EnsureFresh calls concurrently against a fake account client that returns distinct tokens. Assert that one exchange happens and that the stored user token, org token and expiry all come from it.

4. The renew-before window is longer than the token leeway, so re-exchanges do nothing

Where: internal/app/dashboard/oauth_session.go:23 (oauthSessionRenewBefore = time.Minute) and internal/domain/oauth.go:213 (oauthExpiryLeeway = 30 * time.Second)

What's wrong: the dashboard session ends when the OAuth access token does. Between T-60s and T-30s, EnsureFresh re-exchanges, but AccessToken() still returns the same token because it isn't within 30s of expiry yet.

Consequences:

• Every dashboard command in that window pays an extra exchange round trip.
• Each exchange leaves another live server-side session that is never revoked.
• The command still starts with under 60 seconds left, which is the case the constant exists to prevent.

Fix, either:

• (a) Let the token source guarantee a minimum remaining lifetime, e.g. AccessTokenValidFor(ctx, min time.Duration) on the OAuthAccessTokens port. oauthlogin refreshes when now+min >= ExpiresAt, and EnsureFresh passes oauthSessionRenewBefore.
• (b) Make oauthExpiryLeeway >= oauthSessionRenewBefore, for example 2 minutes for both.

(a) is cleaner.

Test: the fake token expires at T+45s, and the exchange returns that expiry. EnsureFresh must make the token source refresh, and the stored expiry must move past T+45s.

5. nylas oauth login silently replaces an existing nylas dashboard login session

Where: internal/cli/oauth/login.go:~115 (exchangeDashboardSessionFn) and internal/app/dashboard/oauth_session.go:58-60, 90-103

What's wrong: every OAuth login, including --for mcp, exchanges and overwrites the dashboard session. It clears the active app and region, and never revokes the replaced password/SSO session. The logout help says a dashboard login session "is left alone", but login doesn't leave it alone.

Failure scenario:

1. A dashboard login user runs nylas oauth login --for mcp just to set up MCP.
2. Their dashboard org and app selection change without notice, and the old session stays live on the server.
3. A later oauth logout then ends the new dashboard session, so they are logged out of the dashboard.

Fix: in SessionRenewer.Login, if a session is stored with no oauth origin, either:

• skip the exchange and print "Dashboard commands stay signed in via nylas dashboard login", or
• revoke it first with NewAuthService(...).WithServer(r.server).Logout(ctx) and say so.

I'd also skip the exchange for --for mcp, or confirm the server intends to accept MCP-audience tokens at /auth/cli/oauth/exchange and checks aud (see the questions at the end).

Test: store a dashboard session with no origin key. Either Login leaves KeyDashboardUserToken and KeyDashboardAppID unchanged, or it revokes the old session and prints a notice.

6. Callback server: the expected state is set asynchronously, and any stray callback aborts the login

Where: internal/app/oauthlogin/service.go:188-197 and internal/adapters/oauth/server.go:153-196

What's wrong:

• setExpectedState runs inside the WaitForCallback goroutine, and nothing orders it before browser.Open(authURL).
• handleCallback calls s.once.Do(...) on error and state-mismatch paths too. So the first request to /callback, whether it has ?error=…, no state, or a wrong state, uses up the single allowed callback. The real redirect is then ignored.
• The default port is fixed (9007), so any page open in the user's browser during a login can abort it.
• The error query value is put into the CLI error text unfiltered, so ANSI escape sequences reach the terminal.

Fix:

• Set the expected state synchronously before opening the browser. Add SetExpectedState(state) to the callback-server port, or pass state to Start, and call it before browser.Open.
• In handleCallback, check state first. Answer a missing or mismatched state with 400 without calling once.Do. Only a request with a matching state may resolve the wait, as a code or as an error.
• Allow-list error to ^[a-z_]{1,64}$ before it goes into the error message (and error_description, if you add it, to printable ASCII).

Tests:

• A wrong-state request followed by the right one still returns the code.
• ?error=%1b[31m with no state does not end WaitForCallback.
• A fake Browser.Open that fires the callback synchronously still succeeds.

7. A chunked keychain read that races a refresh fails and tells the user to sign in again

Where: internal/adapters/keyring/keyring.go:79-98 (Get) and :72 (deleteChunks(key, previous) in Set)

What's wrong:

• Get reads the header, then each chunk, and fails on any missing chunk.
• Set deletes the previous generation's chunks right after it writes the new header.
• AccessToken's fast path (service.go:266-275) reads without the session lock before every MCP request.
• Chunking starts at 2000 bytes on every OS, so a JWT carrying grants is chunked on macOS and Linux too, not only Windows.

Failure scenario:

1. Two mcp serve processes are running. A refreshes under the lock.
2. B reads header H0. A writes H1, then deletes H0's chunks.
3. B's chunk read returns NotFound, and the tool call fails with "Run nylas oauth login --for mcp to sign in again".

On macOS every item read launches a security process, which widens the window.

Fix: retry a bounded number of times when the header changed under the read.

for attempt := 0; attempt < 3; attempt++ {
    header, err := keyring.Get(serviceName, key) // map ErrNotFound → domain.ErrSecretNotFound
    if err != nil { ... }
    gen, n, ok := parseChunkHeader(header)
    if !ok { return header, nil }
    if v, err := readChunks(key, gen, n); err == nil { return v, nil }
    if again, _ := keyring.Get(serviceName, key); again == header {
        return "", fmt.Errorf("secret %s is incomplete in the system keyring", key)
    }
}

To make this testable, route keyring.Get/Set/Delete through package-level vars.

Test: a stubbed Get runs Set(key, big2) after the header read but before the first chunk read. Get must return big2, not an error.

8. Every MCP request reloads 7–9 secrets

Where: internal/app/oauthlogin/mcp_credentials.go:29 → service.go:266 (AccessToken → loadSessionForServer), called per request from internal/adapters/mcp/proxy.go:~201

What's wrong: each Credential() call re-reads the full session.

• On the encrypted file store (headless Linux, WSL, containers, NYLAS_DISABLE_KEYRING), each Get takes the exclusive .secrets.lock and derives an Argon2id key (64 MiB, t=3). Seven derivations measured about 210 ms on an M-series Mac; slower hosts pay 0.5–1 s of CPU and 64 MiB allocations per tool call.
• On macOS it is about 10 security process launches per request.
• All mcp serve processes serialize on the lock.

Fix: cache the last credential in MCPCredentials, behind a mutex, together with its ExpiresAt. Return the cached token while now + leeway < ExpiresAt, and go to the store only when it is near expiry or in Renew. This stays correct across processes because Renew and refreshLocked already re-read under the lock, and a 401 still triggers Renew.

Test: with a counting SecretStore, two consecutive Credential() calls on an unexpired token make at most one store read.

9. Encrypted-store Get now needs a writable config dir

Where: internal/adapters/keyring/file.go:109-122, 155-160 and internal/adapters/filelock/filelock.go:36-43

What's wrong: Get always runs MkdirAll and OpenFile(O_CREATE) on .secrets.lock. NewSecretStore probes fileStore.Get(KeyAPIKey) (keyring.go:~214-220). With ~/.config/nylas read-only (a Nix/home-manager symlink or a read-only container mount), that probe now returns ErrSecretStoreFailed instead of ErrSecretNotFound, so every command that opens the secret store fails. Keyring users also get a stray .secrets.lock created.

Fix:

• At the top of Get, if os.Stat(f.path) reports not-exist, return domain.ErrSecretNotFound without locking. Racing a first writer is equivalent to reading just before it.
• Optionally, on EACCES/EROFS, fall back to an O_RDONLY open of an existing lock file.

Test: with a chmod 0500 config dir and no .secrets.enc, Get returns ErrSecretNotFound, and NewSecretStore with the mock keyring succeeds.

10. proxy.go is over the project's 600-line limit

Where: internal/adapters/mcp/proxy.go goes from 533 to 616 lines. CLAUDE.md says "NEVER create files >600 lines".

Fix: move send, readResponse and cloneRPCRequest into proxy_auth.go or a new proxy_http.go, and delete the dead code in #17.

---

P3 (fix now, or open follow-ups)

#	Where	Issue	Fix
11	oauthlogin/service.go:~204	The code exchange uses what's left of the 5-minute login context, so a slow consent screen leaves it about 1s.	Give ExchangeCode and the dashboard exchange context.WithTimeout(context.WithoutCancel(ctx), 30*time.Second).
12	domain/oauth.go:218-222, store.go:106-109	A token response without expires_in gives a zero ExpiresAt, which IsExpired always treats as expired, so every call refreshes and every MCP request rotates the token.	Fall back to the JWT exp claim (DecodeOAuthAccessToken) when expires_in is 0.
13	dashboard/oauth_session.go:90-97	The exchange response isn't validated. An empty UserToken becomes nil, which deletes the session while login prints "signed in". A zero ExpiresAt is stored as year 1, so every command re-exchanges.	Reject a response with an empty UserToken or a zero ExpiresAt.
14	dashboard/oauth_session.go:119-122	isOAuthSession treats a keyring read error as "not OAuth". oauth logout then skips the dashboard clear, and refreshTokens calls account.Refresh, which the server refuses.	Return (bool, error) and surface the error.
15	adapters/dpop/dpop.go:42-50; cli/dashboard/helpers.go:121-129	A stored seed that is malformed or the wrong length is still overwritten. ClearOAuthSession builds a DPoP service before checking the origin, so oauth logout creates a key when none exists.	Return ErrDashboardDPoP on a malformed seed, and check isOAuthSession before createDPoPService.
16	cli/oauth/helpers.go:53-62; cli/dashboard/switch_org.go:128-133	Relogin replaces the OAuth session before the dashboard exchange. If the exchange fails, the dashboard stays on the old org, and a later EnsureFresh silently switches it. The --org mismatch error comes after the switch has already happened.	Exchange first and commit both, or roll back. The mismatch error should say "you are now signed in to X".
17	adapters/mcp/proxy.go:62,80,429	apiKey and injectDefaultGrant are dead in production and only read by tests. The tests at proxy_forward_test.go:211 and proxy_normalize_test.go:502 exercise a path that is never used, one without the AllowsGrantHint check.	Delete both, and point the tests at injectGrant or send.
18	adapters/mcp/proxy_auth.go:80	The 403 insufficient_scope branch has no p.oauth guard, so API-key users are told to run nylas oauth login.	Use if p.oauth && resp.StatusCode == http.StatusForbidden && …, and add a test.
19	adapters/mcp/proxy.go:609-616	cloneRPCRequest uses maps.Clone, which is shallow. normalizeListEventsArgs and normalizeAvailabilityArgs still mutate the caller's nested maps, which contradicts the PR claim.	Deep-copy the nested maps the normalizers touch, or correct the claim. Add a test that the original nested map is unchanged.
20	adapters/keyring/keyring.go:43-74	Unlocked concurrent Sets, or a crash between chunk writes and the header, leave unreachable *.chunk.<gen>.<i> items that can't be enumerated. An older CLI reads a chunked key as the literal nylas:chunked:v1:….	Document both. Consider a release note that downgrading after login needs nylas oauth login again.
21	cli/oauth/helpers.go:44-49	dashboard.OAuthTokenSource and OAuthRelogin are wired through init() into package globals, which is hidden coupling.	Inject them from cmd/nylas/main.go.
22	cli/oauth/status.go, login.go	They ignore --json and --quiet (CLAUDE.md: use common.GetOutputWriter(cmd) and common.PrintSuccess).	Route output through the common writer, at least for status.
23	cli/mcp/install.go:84-97	Re-running mcp install -a X without --auth after an --auth oauth install silently switches back to API-key args.	Keep the existing auth mode, or print that it changed.
24	docs	docs/security/overview.md is missing the OAuth keys, the chunk format and .secrets.lock (a security change is CRITICAL in documentation-maintenance.md). docs/COMMANDS.md doesn't show mcp install --auth oauth. ARCHITECTURE.md omits oauthas/, app/oauthlogin/ and cli/oauth/, and calls filelock/ "OAuth refresh" only.	Update these.

Lower priority: domain/oauth.go:102-105 lets a loopback base URL advertise endpoints on any https host. It is intentional for dev tunnels, but it means whatever listens on that local port decides where the code, PKCE verifier and refresh token go. Consider requiring an explicit opt-in env var for non-loopback endpoints.

---

Questions to confirm before release

1. MCP server rollout order. API-key users also stop sending Mcp-Session-Id and start sending Mcp-Method / Mcp-Name / Mcp-Protocol-Version. Is mcp.{us,eu}.nylas.com stateless in production before this CLI release? If not, existing nylas mcp serve users break. Please add this to "Release order".
2. Exchange audience. --for mcp access tokens carry the MCP audience and are still sent to /auth/cli/oauth/exchange. Does dashboard-account check aud, and is accepting them intended?

Verified correct

• PKCE, state and nonce: 32 bytes from crypto/rand; S256 challenge in raw base64url; state compared in constant time.
• Callback server: binds only 127.0.0.1 over tcp4. The redirect URI is validated before the browser opens. The success page reflects no input, and error pages use http.Error (text/plain, nosniff).
• Metadata: https is required except on loopback. The issuer must equal the base URL, and every endpoint must match the issuer's scheme, host and port. Case, trailing-dot, userinfo and port variants all fail closed.
• Redirects: none are followed by the oauthas client, the dashboard account client or the MCP client (NewNoRedirectClient).
• Refresh:
  • The lock is held across read, refresh and write, with the session re-read once it is held.
  • WithoutCancel plus a 30s deadline.
  • A spent refresh token is deleted only if it is still the stored value.
  • The write order is as claimed.
  • There is no lock-ordering cycle: oauth-session.lock is outer and .secrets.lock inner.
• MCP:
  • Tokens only go to the two allow-listed MCP hosts.
  • Mcp-Method / Mcp-Name are allow-listed to ^[A-Za-z0-9_./\-]{1,128}$, so no header injection.
  • The 401 retry is bounded and only happens on a Bearer challenge.
  • No token appears in errors or logs.
  • stdout carries only the protocol.
  • API key is still the default, and install never writes credentials.
• filelock:
  • flock is per open-file-description, and Windows uses LockFileEx.
  • The wait can be cancelled, and the lock is released when the holding process dies.
  • The lock file is 0600 in a 0700 dir.
  • The unsupported-platform fallback fails closed.
• Layering: domain imports only the standard library, and app imports only ports and domain.

Suggested order of work

1. TW-5028: Add codeowners #1, then [TW-4138] CLI initial version #2 and TW-5027: Demo friendly #3 (reuse one sessionLock helper for OAuth and dashboard writes), then TW-4988: update claude #6. These are the correctness and security items.
2. TW-5026: Add a dev app id #4, TW-4989: Initial Commit Nylas CLI #5 and TW-4986: fix(calendar): fix recurring events API and integration tests #8, which are user-visible behavior and performance.
3. TW-4987: fix(air): align cache header test expectations with middleware implementation #7, TW-4985: chore(release): disable automatic homebrew tap updates #9 and TW-4984: docs: add templates.md to INDEX.md #10.
4. The P3 table; anything deferred can go to follow-up tickets.
5. Re-run make ci-full, and add the tests listed under each item. Each should fail with its fix reverted, as you did for the hardening commits.

🤖 Generated with Claude Code

[qasim-nylas]

Re-review: Claude + Codex at a137da6

This follows up the earlier review at ec3d285. It covers the new commits 3819b50, fc7b0e8, 5d38210, c28253a and a137da6. Claude and Codex each reviewed the full diff independently. I checked every finding below against the code; where only one reviewer raised something, it says so.

Verdict: good progress, but it isn't ready to merge.

• Fixes TW-5026: Add a dev app id #4 and TW-4988: update claude #6 are correct.
• Fixes TW-5028: Add codeowners #1 and [TW-4138] CLI initial version #2 are correct in the sequential case.
• Fixes TW-5027: Demo friendly #3 and TW-4989: Initial Commit Nylas CLI #5 only work while nothing else writes the dashboard session, because nylas dashboard login/logout/orgs switch/apps use never take the new lock.
• Two new gaps break the PR's own invariant that a token never goes to a server that didn't issue it (N1, N2). A retried MCP request after a 401 is sent without normalization (N5).
• Items TW-4987: fix(air): align cache header test expectations with middleware implementation #7–TW-4978: feat(chat): add Slack integration with channel resolution and tools #23 from the first review are still open.

Build and tests (Claude run; the Codex sandbox could not compile):

• go build ./..., go vet ./... and GOOS=windows go build ./... pass.
• go test -race -count=1 passes on all 16 touched packages.
• golangci-lint reports 0 issues.
• ❌ internal/adapters/mcp/proxy.go is now 640 lines (533 on main; the CLAUDE.md limit is 600).

---

Fix verification (#1–#6)

#	Verdict	Notes
1	✅ OK	oauthlogin/service.go:218-231 clears under the OAuth lock, saves, and clears again on failure. The test can fail. Unlocked readers can still see a mixed session (N2).
2	✅ OK (minor gap)	reset.go:113-125 takes the dashboard lock, then the OAuth lock. clearDashboardCredentials (reset.go:128-138) discards every Delete error, so reset can print ✓ while keys remain (N10).
3	⚠️ Incomplete	Renew, Login and ClearIfOAuthThen are serialized, and expiry is re-checked under the lock. But only SessionRenewer takes dashboard-session.lock. Every other writer skips it (N3).
4	✅ OK	AccessTokenValidFor(ctx, oauthSessionRenewBefore), re-checked in refreshLocked. Tests exist.
5	⚠️ Incomplete	Correct sequentially. dashboard login doesn't take the lock, so it can store a session while an exchange is in flight, and the exchange then overwrites it (N3). hasDashboardLoginSession also treats a keyring read error as "no session".
6	✅ OK	State is set before browser.Open in both flows. A bad or missing state gets a 400 without using up once. The error code is allow-listed. There is no goroutine leak. Behaviour change: an error redirect without state now waits out the whole 5 minutes instead of failing fast (N11).

Lock ordering. Both reviewers traced every path that takes these locks. None takes a lock twice or out of order (dashboard, then OAuth, then .secrets.lock). The locks are not re-entrant: each Lock opens a new descriptor, so a second acquisition from the same process waits until its context expires. Keep that in mind when fixing N3: split out …Locked variants rather than nesting withLock.

---

New findings

N1 (P1, security invariant) renewRejected hands back a stored session without checking its server

Where: internal/app/dashboard/oauth_session.go:123-127, called from auth_service.go:225-230 · raised by Codex, confirmed

renewRejected returns stored whenever it differs from the rejected token, is from OAuth and is fresh. It never calls checkSessionServer, unlike every other load path (loadDashboardTokens).

Failure scenario:

1. A process configured for server A (a dev account URL) loads its session and sends a request.
2. Meanwhile another process runs nylas oauth login against production and stores a fresh production dashboard session.
3. A's request gets ErrDashboardSessionExpired, and renewRejected returns the production user and org tokens.
4. The retry sends them to A.

Fix: load through the same checked path while holding the lock.

err = r.withLock(ctx, func() error {
    if isOAuthSession(r.secrets) && !r.needsRenewal() {
        u, o, loadErr := loadDashboardTokens(r.secrets, r.server) // enforces checkSessionServer
        if loadErr == nil && u != rejected {
            userToken, orgToken = u, o
            return nil
        }
        if loadErr != nil && !errors.Is(loadErr, domain.ErrDashboardNotLoggedIn) {
            return loadErr // e.g. ErrDashboardServerMismatch: do not exchange over it
        }
    }
    resp, err := r.exchange(ctx, false)
    ...
})

Test: store a fresh OAuth session whose KeyDashboardSessionServer is https://b, call renewRejected on a renewer WithServer("https://a"), and assert it returns ErrDashboardServerMismatch and no token.

N2 (P2, security invariant) Unlocked session reads can mix two servers' sessions and defeat checkIssuer

Where: internal/app/oauthlogin/store.go:48-86 (loadSession), service.go:285-293 (fast path) · raised by Codex, confirmed

loadSession reads the access token first, then issuer, server URL and expiry in separate Gets, without the OAuth lock. Fix #1 made writers clear before saving, but a reader can still see a mixed session.

Failure scenario:

1. mcp serve, configured for B, reads A's access token.
2. A concurrent nylas oauth login against B clears the session and writes B's server URL and expiry.
3. The reader reads B's server URL, so checkIssuer passes, and A's access token goes to B.

The dashboard loader has the same token-then-server order (session_store.go:16-30).

Fix (cheap): read ServerURL/Issuer first, the tokens next, and at the end re-read the access token and server URL. If either changed, retry once or fail with ErrOAuthNotLoggedIn. Alternatively, do the full read under the session lock, which becomes affordable once the credential is cached (#8).

Test: a SecretStore wrapper whose hook runs a different-server Login right after the first KeyOAuthAccessToken read. Assert that AccessToken errors or returns B's token, and never returns A's.

N3 (P2) Most dashboard session writers don't take dashboard-session.lock, which is why fixes #3 and #5 are incomplete

Where: internal/app/dashboard/auth_service.go:137 (Logout), :234 (SwitchOrg), :271 (SyncSessionOrg), :285 (storeTokens), internal/cli/dashboard/apps.go:234 (apps use), internal/cli/setup/wizard_helpers.go:280 · both reviewers

Failure scenarios:

• Login overwrite:
  1. oauth login checks hasDashboardLoginSession() under the lock and finds none.
  2. It starts the exchange, a network round trip.
  3. nylas dashboard login in another terminal stores a password/SSO session.
  4. replaceSecretValues overwrites it. The old session stays live on the server, and the app/org choice is lost. This is exactly what TW-4989: Initial Commit Nylas CLI #5 was meant to prevent.
• Logout resurrection: dashboard logout runs while EnsureFresh is exchanging. Logout clears the keys, then the exchange writes a whole new session back.
• Mixed tokens: replaceSecretValues is a series of separate Set calls, so two writers can leave user token A paired with org token B.

Fix:

• Give AuthService a lock ports.CrossProcessLock, wired from common.DashboardSessionLock(secretStore) in cli/dashboard/helpers.go.
• Wrap storeTokens, clearTokens, SwitchOrg/SyncSessionOrg's store step and the apps use write in it.
• SessionRenewer.exchange already holds the lock and calls AuthService.replaceSecretValues. Split out a replaceSecretValuesLocked (and logoutLocked for ClearIfOAuthThen) so nothing takes the lock twice.
• Always take the dashboard lock before anything that can take the OAuth lock.
• Update docs/security/overview.md, which currently says every dashboard write is serialized.

Test: hold DashboardSessionLock in a goroutine and assert that AuthService.Login (password) and Logout block until it is released, mirroring TestClearSessions_WaitsForARefreshInFlight.

N4 (P2) A failed exchange after oauth login or relogin keeps the previous OAuth dashboard session

Where: internal/app/dashboard/oauth_session.go:146-157, internal/cli/oauth/login.go:117-126, internal/cli/oauth/helpers.go:53-62 · raised by Claude, confirmed

oauthlogin.Login has already replaced the OAuth session when the exchange runs. If the exchange fails, the stored dashboard session with origin=oauth still belongs to the old user or org.

Failure scenario:

1. nylas dashboard orgs switch signs in to org B, and the exchange times out. That is likely given TW-4983: feat(http): add custom User-Agent header to all API requests #11's shared 5-minute context.
2. The user sees an error, but every dashboard command keeps acting on org A for up to 15 minutes, while oauth status shows B.
3. Then EnsureFresh re-exchanges from B's token and the org changes mid-script.

Fix:

// in SessionRenewer.Login, inside withLock
resp, err = r.exchange(ctx, true)
if err != nil && isOAuthSession(r.secrets) {
    // The OAuth session it came from is gone; don't keep acting on it.
    return errors.Join(err, NewAuthService(r.account, r.secrets).clearTokens())
}

Also fixes #16: check --org before committing, or make the error say "you are now signed in to X".

Test: seed oauthSessionSecrets("old-user"), make ExchangeOAuthTokenFn fail, call Login, and assert KeyDashboardUserToken is gone.

N5 (P2) After a 401 renewal, the retried MCP request goes out without normalization (upgrades #19 to a confirmed bug)

Where: internal/adapters/mcp/proxy.go:296, 537-551, 556-602, 633-640 · raised by Claude and reproduced with an overlay test; Codex found the same

cloneRPCRequest copies only the top-level Arguments map. The first attempt's normalizers therefore mutate the caller's nested get_all_query_parameters / availability_request maps. On the retry the values are already normalized, so modified == false and the original bytes are sent.

Reproduced: list_events with "start": 1700000000. Attempt 1 sends start as a string. After the 401 renewal, attempt 2 sends 1.7e+09 as a float, which the upstream schema rejects. availability rounding is lost the same way.

Fix: deep-copy, or re-parse the pristine bytes for each attempt.

func cloneRPCRequest(req *rpcRequest) (*rpcRequest, error) {
    raw, err := json.Marshal(req)
    if err != nil { return nil, err }
    var out rpcRequest
    return &out, json.Unmarshal(raw, &out)
}

Test: script a 401 with WWW-Authenticate: Bearer error="invalid_token", then a 200. Send list_events with an integer start, and assert both recorded upstream bodies have "start":"1700000000". Also assert the caller's nested map is unchanged.

N6 (P3) Notifications that are still answered (follow-up to a137da6)

Where: proxy.go:44-52, 151-158, 168-176, 197-202, proxy_response.go:58-68 · both reviewers

The absent-versus-null check in isNotification is correct, but it runs on only one branch:

• Batches, and anything else that doesn't unmarshal into rpcRequest (e.g. params as an array), get a synthetic {"id":null,"error":…} when forwarding fails. TestIsNotification's "not an object → false" row asserts this as correct.
• The local get_grant handler answers an id-less tools/call.
• Rewrites add an id: rpcRequest.ID has no omitempty, so when injectGrant or the normalizers re-marshal an id-less message, it goes upstream with "id":null, which is a request, and the reply is relayed.
• A 200 body the upstream returns for a notification is relayed as-is.

Fix:

• Check isNotification(line) before every write: the parse-failure branch, the local handler and the success path.
• For arrays, drop the failure if every element lacks id, and otherwise return per-id errors.
• Keep the id absent through rewrites, e.g. ID json.RawMessage \json:"id,omitempty"`(aRawMessagekeepsnull` distinct from absent).

Test: extend TestProxy_FailedNotificationIsNeverAnswered with a batch notification, a params-array notification and an id-less get_grant call, and assert empty stdout. Also assert that a rewritten id-less tools/call reaches the upstream with no id key.

N7 (P3) Untrusted text reaches stderr and the terminal unescaped

Where: proxy.go:184 (log.Printf("… %s …", req.Method, err)), proxy_auth.go:93 (upstream body in the error), oauthas/client.go:140-154 → domain/oauth.go:167 → cli/oauth/helpers.go:154 (error_description) · raised by Codex, confirmed

Escape sequences or CR/LF in a method name, an upstream error body or an authorization-server error_description reach stderr unescaped, which allows terminal and log injection. The callback allow-list from #6 doesn't cover these paths.

Fix: log %q for req.Method, and cap and quote upstream bodies. Filter error_description to printable ASCII, at most 200 characters, before it goes into an error.

Test: feed "\u001b]0;pwned\u0007" through a failed notification and a token-endpoint error, and assert there's no raw \x1b in the output.

N8 (P3) DPoP key creation isn't serialized

Where: internal/cli/dashboard/helpers.go:111 → internal/adapters/dpop/dpop.go:38-62 · raised by Codex, confirmed

Two processes on first use both see no key and generate K1 and K2. The last Set stores K2, but the other process then exchanges and stores a session bound to K1. ClearOAuthSessionThen also creates a key before checking the origin (#15).

Fix: create or load the key while holding the dashboard lock (after N3), or re-read it after Set and use whatever is stored. Check isOAuthSession before createDPoPService in ClearOAuthSessionThen.

Test: two dpop.New calls whose Gets both return not-found before either Set; assert both end up using the stored key.

N9 (P3) ClearIfOAuthThen runs then() twice if releasing the lock fails

Where: oauth_session.go:79-82, 195-205 · raised by Codex, confirmed

withLock returns an unlock error after fn has already run then(). ClearIfOAuthThen treats any error as a failure to take the lock, and calls then() again without the lock. In between, a concurrent login could store a new session, and the second call would log it out.

Fix: track whether fn ran (ran := false; …; ran = true), and only fall back when !ran. Return the unlock error alongside clearErr.

Test: a lock whose unlock returns an error, with a counting then; assert exactly one call.

N10 (P3) config reset reports success when a delete fails

Where: internal/cli/config/reset.go:128-138 · raised by Codex, confirmed

Fix: collect the errors with errors.Join, ignoring domain.ErrSecretNotFound, and return them from clearSessions, so reset doesn't print "✓ cleared" when it failed.

N11 (P3) Smaller items

• The callback now waits on a stateless error redirect (server.go:189): before TW-4988: update claude #6, ?error=access_denied with no state failed immediately; now it waits the full 5 minutes. That's fine if the authorization server always echoes state on error redirects (RFC 6749 §4.1.2.1 requires it). Please confirm dashboard-account does, and add a test with error plus the correct state.
• Replaced refresh tokens aren't revoked (service.go:222, Relogin at :249-258): each repeated oauth login or orgs switch leaves a live refresh-token family on the server. Under the lock, if the old session is for the same server, call client.Revoke on its refresh token (best effort) before clearing.
• An expired dashboard login session blocks the exchange forever (TW-4989: Initial Commit Nylas CLI #5): hasDashboardLoginSession doesn't check expiry, so the user has to run dashboard logout manually. Say so in the notice.
• Tests that can't fail or cover dead code: proxy_basic_test.go:43 asserts the dead apiKey field. TestProxy_injectDefaultGrant tests a wrapper production never calls. Nothing exercises a retried request after renewal, which is why N5 went unnoticed.
• Test files over 600 lines grew further: account_client_test.go (815→850), proxy_e2e_test.go, auth/service_test.go. Not blocking, but worth splitting while you're in there.

---

Items #7–#24 from the first review

#	Status	Evidence / what to do
7	❌ Not fixed	keyring.go:89-97 fails straight away on a missing chunk. Add the bounded retry from the first review.
8	❌ Not fixed	mcp_credentials.go:29-35 → 7 Gets per request. Measured ~185 ms of Argon2id per MCP request on the file store (M-series). Cache the token and its ExpiresAt in MCPCredentials. This also makes N2's locked read affordable.
9	❌ Not fixed	Reproduced: with a 0500 config dir, Get → failed to open lock file … permission denied, which breaks every command on a read-only config. Return ErrSecretNotFound if .secrets.enc doesn't exist, before locking.
10	❌ Worse	proxy.go is 640 lines. Move send/readResponse/cloneRPCRequest/isNotification into proxy_http.go, and delete #17's dead code.
11	❌ Not fixed	service.go:207 and login.go:117 share the 5-minute login context. Use context.WithTimeout(context.WithoutCancel(ctx), 30*time.Second) for the code exchange and the dashboard exchange.
12	❌ Not fixed	flows.go:96: no expires_in → zero ExpiresAt → refresh on every call. Fall back to the JWT exp.
13	❌ Not fixed	account_client.go:168-176 / oauth_session.go:159-167: reject an empty UserToken or a zero ExpiresAt.
14	❌ Not fixed	isOAuthSession (oauth_session.go:208-211) should return (bool, error). On error, oauth logout should clear anyway or warn, not print ✓.
15	🟡 Partial	Read errors now fail closed ✓. A malformed seed is still overwritten (dpop.go:43-51), and ClearOAuthSessionThen still creates a key first (see N8).
16	❌ Not fixed	switch_org.go:124-134: the --org mismatch is detected after both sessions are replaced. See N4.
17	❌ Not fixed	Proxy.apiKey (proxy.go:63,81), injectDefaultGrant (:451-458) and sessionLockPath (cli/oauth/helpers.go:116) are all dead in production. Delete them and repoint the tests.
18	❌ Not fixed	Reproduced: an API-key proxy getting a 403 insufficient_scope tells the user to run nylas oauth login --for mcp. Add the p.oauth && guard (proxy_auth.go:79).
19	❌ Confirmed bug	See N5.
20	❌ Not fixed	Orphaned chunks, and older builds reading nylas:chunked:v1:… as the secret itself. At least document both and add a release note.
21	❌ Not fixed	cli/oauth/helpers.go:44-49 still wires init() into package globals. Inject them from cmd/nylas/main.go.
22	❌ Not fixed	oauth status/login/logout ignore --json/--quiet. Use common.GetOutputWriter(cmd).
23	❌ Not fixed	install.go:79: re-running mcp install without --auth silently rewrites an OAuth install back to API key. Keep the existing mode, or print the change.
24	🟡 Partial	COMMANDS.md ✓. security/overview.md adds the callback and lock rules, but its key inventory (lines 19-24) doesn't list the OAuth keys, and it overstates dashboard-write serialization (see N3). ARCHITECTURE.md still omits adapters/oauthas, app/oauthlogin, cli/oauth and the new ports.

---

Suggested order of work

1. N1 and N2, which restore the server-binding invariant. Both fixes are small.
2. N3, putting the dashboard lock on every writer (this completes TW-5027: Demo friendly #3 and TW-4989: Initial Commit Nylas CLI #5). Then N4 and TW-5022: fix(email): collapse excessive blank lines in HTML-to-text conversion #16.
3. N5 (the deep copy) and TW-4984: docs: add templates.md to INDEX.md #10 (the proxy.go split and TW-4981: feat(email): auto-detect inbox provider for transactional send #17's dead code, done together).
4. TW-4986: fix(calendar): fix recurring events API and integration tests #8 (credential cache), TW-4985: chore(release): disable automatic homebrew tap updates #9 (read-only config) and TW-4987: fix(air): align cache header test expectations with middleware implementation #7 (chunk retry).
5. TW-4983: feat(http): add custom User-Agent header to all API requests #11–TW-5024: fix(auth): update config file when switching default grant #14, TW-4980: feat(email): add GPG/PGP email encryption and decryption #18 and TW-4978: feat(chat): add Slack integration with channel resolution and tools #23, each a small, local change.
6. The P3 items (N6–N11, TW-5023: fix: correct Nylas dashboard URL typo #15, TW-4979: docs: streamline README for quick onboarding #20–TW-5019: Nylas Chat #22), and feat(mcp): native MCP server + Codex assistant support #24's docs. Anything deferred should become a follow-up ticket linked from the PR body.
7. Run make ci-full. Mutation-check each new test (it should fail with its fix reverted), as you did for the earlier hardening commits.

The two release questions from the first review are still open: whether the production MCP server is stateless before this CLI ships, and whether /auth/cli/oauth/exchange checks the token's aud. Please answer them in the PR body's "Release order".

🤖 Generated with Claude Code

[qasim-nylas]

Third review: Claude + Codex at 9458452

This follows the first review at ec3d285 and the re-review at a137da6. It covers 3544529 (the dashboard.session scope), 918a2f3 (fixes for N1–N3 and N5 plus follow-ups) and 9458452 (the integration base URL).

Three independent passes:

• Claude on the OAuth and dashboard-session core.
• Claude on the MCP proxy, keyring and locks.
• Codex across the whole diff.

The reviewers wrote scratch tests to reproduce bugs, then deleted them. They mutation-checked the fixes by reverting each fix through overlays or copies and re-running its tests. I re-read every P1 and P2 below in the code at 9458452. Each row says which reviewer raised it.

Verdict: a lot of real progress, but not ready to merge.

• Most items from the earlier rounds are now fixed, with tests that fail when their fix is reverted: N1, N4, N5, N7, N9, N10, TW-4987: fix(air): align cache header test expectations with middleware implementation #7, TW-4986: fix(calendar): fix recurring events API and integration tests #8, TW-4984: docs: add templates.md to INDEX.md #10, TW-4982: feat(calendar): enhance Air calendar UI with quick wins and UX improvements #12, TW-5025: feat(email): add GPG/PGP email signing and verification #13, TW-4981: feat(email): auto-detect inbox provider for transactional send #17, TW-4980: feat(email): add GPG/PGP email encryption and decryption #18, TW-5021: feat(email): support inline PGP decryption for Outlook emails #19 and TW-4978: feat(chat): add Slack integration with channel resolution and tools #23.
• proxy.go is down to 514 lines.
• Two new P1s. Both are on dashboard retry paths and break the PR's invariant that a token never goes to a server that didn't issue it. Both reviewers reproduced both, independently, and each is a small fix.
• N2 is fixed for OAuth, but not for dashboard sessions. The dashboard writers store the server label last, so the new re-read can't catch a mixed session.
• TW-4985: chore(release): disable automatic homebrew tap updates #9 (read-only config) is half fixed. It misses EROFS, and its lockless path can still run the passphrase migration.

Build and tests

Check	Result
go build ./..., go vet ./..., GOOS=windows go build ./...	✅ both reviewers
go vet -tags integration (native and Windows)	✅
golangci-lint run ./...	✅ 0 issues, native and Windows
go test -race -count=1	✅ on every touched package (Claude: 22 packages). The Codex sandbox couldn't bind sockets, so it ran the HTTP suites over in-memory transports.
scripts/security-scan.sh	✅
Mutation checks	✅ 21 of 21 selected tests fail with their fix reverted (Codex), plus Claude's own set. Not covered: see the test-gap rows in P3.
600-line limit (non-test)	✅ every touched non-test file is ≤600. proxy.go 514, keyring/file.go 557, oauthlogin/service.go 504.
600-line limit (tests)	⚠️ already over on main and grew further here: account_client_test.go 815→850, proxy_e2e_test.go 867→871, app/auth/service_test.go 842→855

---

Fix verification

Item	Verdict	Evidence / what's left
N1	✅ Fixed	oauth_session.go:134-154 loads through loadDashboardTokens(r.secrets, r.server) under the lock and returns a mismatch instead of exchanging. Mutation-checked. The same leak survives on two other retry paths: R1 and R2.
N2	⚠️ Partial	OAuth: store.go:54-73 reads the server first and re-reads the token and server last, with at most 3 attempts, then ErrOAuthNotLoggedIn. Mutation-checked. Dashboard: the same retry is in session_store.go:20-33, but it can't work with the current write order (R3).
N3	⚠️ Mostly	Every writer now takes dashboard-session.lock through withSessionLock. Both reviewers traced each caller: nothing takes the lock twice, and the order is still dashboard → OAuth → .secrets.lock. 👍 What's left: SwitchOrg/SyncSessionOrg make their network call outside the lock and then write a stale result (R4). SetActiveOrg (auth_service.go:339) has no lock and no callers, so delete it. The adopt-write in checkSessionServer (session_store.go:79) runs on the unlocked read path.
N4	✅ Fixed	oauth_session.go:100-107; mutation-checked. Nit: the best-effort revoke reuses the exchange context, which may already have expired.
N5 / #19	✅ Fixed	proxy_http.go:168-197 deep-copies with a type switch, not a JSON round trip. The notification flag survives cloning. The test asserts both attempts and the caller's map.
N6	✅ Mostly	Checked on the parse-failure, local-handler, success and batch paths. A rewritten notification goes out with no id. Leftover: a failed mixed batch gets a single {"id":null} error rather than one error per id (proxy.go:177). That's minor now that MCP 2025-06-18 has dropped batching.
N7	✅ Fixed (3 named paths)	%q logging (proxy.go:174,209), PrintableSnippet (proxy_auth.go:99), and oauthas/client.go:142-151. Leftovers in P3. The %q on req.Method has no test.
N8 / #15	⚠️ Partial	A first-use key is now created under the dashboard lock and re-read (cli/dashboard/helpers.go:53-65). A malformed seed is still regenerated outside the lock (dpop.go:42-63). ClearOAuthSessionThen still creates a key before checking the origin (helpers.go:143-149). No test covers loadOrCreateDPoPKey.
N9	✅ Fixed	ran flag at oauth_session.go:222-243; mutation-checked.
N10	✅ Fixed	reset.go:131-152; tested.
N11	✅ Mostly	The replaced refresh token is revoked only after checkIssuer(old) passes, so it only goes to the server that issued it. Tested both ways. The error-plus-state callback is tested. Still open: confirm that dashboard-account echoes state on error redirects.
#7	✅ Fixed	keyring.go:82-115: bounded to 3 attempts, and stops early if the header is unchanged. Random, write-once generation ids mean no ABA. Mutation-checked.
#8	✅ Fixed	mcp_credentials.go:39-81 caches until JWT exp minus 30s, and Renew drops the cache. A running proxy keeping its token after logout is documented in docs/commands/mcp.md:109-112.
#9	⚠️ Partial	See R7.
#10	✅ Fixed	proxy.go 514 lines, proxy_http.go 197.
#11	✅ Fixed, untested	30s deadlines at service.go:223, cli/oauth/login.go:123 and helpers.go:61-75. No test fails if they're reverted.
#12 / #13	✅ Fixed	JWT exp fallback (flows.go:96-104). Empty token and zero expiry rejected (oauth_session.go:182-184).
#14	⚠️ Partial	Only ClearIfOAuthThen uses the new (bool, error). EnsureFresh, refreshTokens, hasDashboardLoginSession and checkLegacyOAuthSessionServer (session_store.go:98) still read an error as "not OAuth". The last one fails open: a legacy OAuth session with no recorded server is adopted by whichever server reads it first.
#16	✅ Acceptable	The error now says "you are now signed in to …". The org is chosen on the consent page, so committing first is inherent.
#17 / #18	✅ Fixed	Dead code removed. p.oauth && guard added, with a test. One stale comment still names injectDefaultGrant (proxy_response.go:214).
#20	✅ Documented	docs/security/overview.md:35-40. Consider adding a line to the release notes too.
#21	❌ Not fixed	cli/oauth/helpers.go:44-49 still wires everything through init() into package globals.
#22	❌ Not fixed	oauth status/login/logout still bypass common.GetOutputWriter(cmd).
#23	✅ Fixed, test gap	install.go:71-80, 228-265. See P3.
#24	⚠️ Partial	security/overview.md now lists the keys, the chunk format and the locks. It overstates lock coverage, and :97 says .secrets.lock guards "each read", which is no longer true. In ARCHITECTURE.md, the ports list (:174-181) and the adapter table (:186-199) lack oauthas, dashboard, dpop and the new ports, and :195 still calls filelock/ "OAuth token refresh".

---

New findings

R1 (P1, server binding) The domain-service retry hands another server's session to this one

Where: internal/app/dashboard/domain_service.go:115 → auth_service.go:376 · both reviewers, both reproduced

The retry builds NewAuthService(s.account, s.secrets).WithSessionRenewer(s.renewer) without .WithServer(s.server). The reuse branch that 918a2f3 added in refreshTokens therefore calls loadDashboardTokens(s.secrets, ""), and checkSessionServer skips the check when the server is empty.

Failure scenario:

1. nylas dashboard domains … is configured for server A and gets ErrDashboardSessionExpired.
2. Meanwhile, nylas dashboard login against B runs in another terminal.
3. The retry picks up B's user and org tokens and sends them to A. The probe recorded sent=[a-user b-user].

DPoP binding limits what A can do with them, but the invariant is broken.

Fix: NewAuthService(s.account, s.secrets).WithServer(s.server).WithSessionRenewer(s.renewer). Consider making server a constructor argument, so this can't be dropped again.
Test: swap in B's session during the first domain call. Assert ErrDashboardServerMismatch and no second callback.

R2 (P1, server binding) A password refresh ignores the re-read error and files A's tokens under B's server label

Where: internal/app/dashboard/auth_service.go:373-398 · both reviewers, both reproduced

if stored, storedOrg, err := loadDashboardTokens(s.secrets, s.server); err == nil && stored != userToken {
    ... // reuse
}
resp, err := s.account.Refresh(ctx, userToken, orgToken) // ← reached on ANY re-read error

Failure scenarios:

• Server mismatch: while A waited for the lock, B replaced the session, so the re-read returned ErrDashboardServerMismatch. A still refreshes at A and writes user_token/org_token over B's, but leaves dashboard_session_server=B. The next B-configured command sends A's tokens to B. The probe stored user=a-user-2 server=https://b.
• Not logged in: after config reset or dashboard logout, the same fall-through writes the logged-out session back with no server label. checkSessionServer then lets whichever server reads it first adopt it.

Fix:

stored, storedOrg, err := loadDashboardTokens(s.secrets, s.server)
if err != nil {
    return err // mismatch or logged out: fail closed, never refresh over it
}
if stored != userToken {
    userToken, orgToken = stored, storedOrg
    return nil
}

Test: seed B's session, then run A's queued refresh. Expect a mismatch, no Refresh call, and B's state unchanged. Repeat after a clear and expect ErrDashboardNotLoggedIn.

R3 (P2, server binding) The dashboard N2 re-read can't catch a mixed session, because writers store the server label last

Where: the key order in oauthSessionStateKeys (oauth_session.go:34-39), applied in order by applySecretValues (auth_service.go:435-449). Used by storeTokensLocked (:297-314) and exchange (oauth_session.go:186-199). · both reviewers, both reproduced

What's wrong:

• user_token is written first and dashboard_session_server last, and nothing is cleared first.
• During the window between those two writes, the store holds B's user token under A's server label, and it stays that way until the label is written.
• An unlocked reader configured for A sees the same pair on both reads, so the "did it change?" check passes.
• Codex confirmed that A's loader accepted B's user token while B's writer held the lock.
• session_server_test.go:236 swaps the whole session in one step, so it can't catch this.

Fix: mirror oauthlogin.saveTokens. Delete user_token first, then write the server label, then the other keys, and write user_token last. Apply the same order to the rollback in restoreSecretValues. A reader then sees either "not logged in" or a consistent session.
Test: pause a password login and an OAuth exchange after each key is written. A reader for the old server must never get the new tokens.

R4 (P2) SwitchOrg / SyncSessionOrg commit a result fetched for a session that has since been replaced

Where: auth_service.go:237-268 (SwitchOrg) and :274-288 (SyncSessionOrg) · Codex reproduced it and rated it P1; Claude found it by reading the code and rated it P3; I've put it at P2

The network call runs outside the lock, and the write runs under the lock without checking that user_token is still the one the call used.

Failure scenario: a slow orgs switch on A overlaps a dashboard login on B. A's org token is then written into B's session, and B's checked loader accepts the mixed pair. The probe produced B's user token with A's org token. SetActiveApp has the same "write without compare" shape.

Fix: under the lock, compare the stored user_token (and the server) with the values used for the request, and return an error if they changed. This is the same idea as the check refreshTokens already does.
Test: block A's switch response, complete a service-level B login through the same lock, then release A. B's tokens and org must be unchanged.

R5 (P2) renewRejected exchanges over a session that isn't OAuth, or that was just cleared

Where: oauth_session.go:136-146 · both reviewers reproduced it

It reuses the stored session only when isOAuthSession. Anything else falls through to r.exchange(ctx, false).

Results:

• A nylas dashboard login session stored since the 401 is overwritten. This undoes TW-4989: Initial Commit Nylas CLI #5. The probe ended with user exchanged, origin oauth.
• A dashboard logout or config reset that lands between the 401 and the retry is undone.

Fix: under the lock, if the stored session is not the rejected one, return loadDashboardTokens(r.secrets, r.server) (tokens or error), whatever its origin. Exchange only when the stored token is the rejected one and it's still from OAuth.
Test: hold back an OAuth request's 401, complete a password login, then deliver the 401. Assert no exchange and an unchanged password session and app. Add a second case with the session cleared.

R6 (P2, regression from 3544529) Relogin with an empty stored scope asks for only dashboard.session

Where: oauthlogin/service.go:288-291, store.go:149, domain/oauth.go:58-65 · both reviewers, both reproduced

Why the stored scope can be empty: saveTokens stores tokens.Scope exactly as received. RFC 6749 §5.1 lets a token response, or a refresh response, leave out scope when it matches the request.

What goes wrong:

• WithDashboardSessionScope([]) returns ["dashboard.session"]. That list isn't empty, so Login no longer falls back to its default scopes.
• orgs switch therefore loses openid/offline_access, so no refresh token comes back. For MCP it also loses the data scopes.
• With NYLAS_OAUTH_CLIENT_ID set, it fails outright: the override strips dashboard.session, and the list becomes empty.

Open question: whether dashboard-account ever leaves out scope isn't confirmed. The CLI shouldn't depend on it.

Fix:

• When the code response leaves out scope, store the scopes that were requested.
• When a refresh response leaves out scope, keep the stored scope.
• In Relogin, if the stored scope is empty, start from DefaultOAuthScopes(), or MCPOAuthScopes() when a resource is set, before adding dashboard.session.

Test: leave scope out of both the code and refresh responses. Relogin must keep the identity, refresh and (for MCP) data scopes, and the resource.

R7 (P2, regression vs main) #9: the read-only fallback misses EROFS and can run the migration without .secrets.lock

Where: adapters/keyring/file.go:117-125 (lock), :166-177, and migrateToPassphrase (reached from decrypt, :376-389) · both reviewers. The missed EROFS was verified via errors.Is(syscall.EROFS, fs.ErrPermission) == false. Codex reproduced the unlocked migration write.

Two gaps:

• EROFS isn't covered. The fallback is errors.Is(err, fs.ErrPermission), which matches only EACCES/EPERM. On a real read-only mount (Docker -v …:ro, a Nix store) that already holds .secrets.enc, OpenFile(O_RDWR|O_CREATE) fails with EROFS. The NewSecretStore probe then returns ErrSecretStoreFailed, so every command fails. On main, Get took no file lock, so this case worked there.
• The fallback's premise is wrong. It assumes "can't open the lock, so nobody can write". A .secrets.lock left owned by root after sudo nylas …, or set to mode 0400, in a writable directory breaks that. A lockless Get on a legacy-keyed store with a passphrase then runs migrateToPassphrase. That writes the salt and the store under only the per-instance mutex, so two processes can leave a salt and ciphertext that don't match. That is exactly the hazard the comment at :158-163 warns about.

Fix:

• Try an O_RDONLY open of an existing lock file first. Both flock and LockFileEx work on a read-only handle.
• Only go lockless if that fails too, using a build-tagged isReadOnlyErr (Unix: EACCES/EPERM/EROFS; Windows: ERROR_ACCESS_DENIED/ERROR_WRITE_PROTECT).
• On the lockless path, never migrate: read only.

Test: inject EROFS from the lock open, and assert Get succeeds. Then use two store instances sharing a writable directory with a read-only lock file, and assert that neither migrates.

R8 (P2, needs a server-side answer) --for mcp tokens now carry dashboard.session

Where: domain/mcp_auth.go:93 · Claude

From 3544529, MCP-audience tokens, which are sent to mcp.{us,eu}.nylas.com on every request, can be exchanged for a full-role dashboard session. That works if /auth/cli/oauth/exchange accepts the MCP audience, and the PR implies it does. This is the open "exchange audience" question from the first review, and the new scope raises the stakes: a token leaked from the MCP path becomes a dashboard takeover.

Either:

• leave dashboard.session out of MCPOAuthScopes() (the dashboard commands can use a plain nylas oauth login), or
• confirm in the PR body that the exchange pins aud to the dashboard and rejects MCP-audience tokens. In that case the scope does nothing for --for mcp, so drop it anyway.

---

P3 (fix now, or open follow-ups)

#	Where	Issue	Fix	Raised by
P3-1	proxy.go:43-52, re-marshal at :358,394	Any grant-injected or normalized rewrite drops params._meta, so progress tokens are lost. It also turns large numbers into floats: id 9007199254740993 → …992, so the reply can't be matched to the request. This happens on main too; it's not new here.	Decode with UseNumber(), keep params and id as json.RawMessage, and splice in only arguments.	Claude (reproduced)
P3-2	proxy_auth.go:87-88, adapters/dashboard/http.go:274-294	Leftover N7 paths. The scope list from WWW-Authenticate isn't filtered; C1 controls such as U+009B get through. The dashboard error code/message/body is unfiltered, and the new exchange now reaches it.	Run both through PrintableSnippet.	Claude
P3-3	cli/mcp/install_test.go:61-88, install.go:239-241	The #23 tests call installServer directly. Changing RunE to ignore Changed("auth") still passes. A hand-edited "--auth=oauth" arg isn't recognised and is turned back into an API-key install.	Add a command-level test that runs install -a cursor over an OAuth config, and parse the --auth= form too.	Claude (mutation)
P3-4	cli/oauth/login.go:114, oauth_session.go:174-177	With a client-id override, dashboard.session is reported as "Not offered by this server", and the exchange error says "run nylas oauth login again and allow dashboard access", which can't help under an override. The unverified scope check also refuses a JWT with no scope claim. That's safe (it can only refuse), but it gives false refusals.	Give the override its own message. Only run the check when claims.Scope != "".	Claude
P3-5	cli/dashboard/apps.go:233, cli/setup/wizard_helpers.go:279	apps use (cmd.Context()) and the wizard (context.Background()) wait on the dashboard lock with no deadline.	Use context.WithTimeout, as the other lock users do.	Claude
P3-6	internal/adapters/nylas/integration_base_test.go:55	9458452 fixed internal/cli/integration, but this package also runs under make test-integration and still uses the production base URL.	Apply the same SetBaseURL.	Claude
P3-7	Tests	No test fails if these are reverted: the #11 deadlines, loadOrCreateDPoPKey (N8), the %q on req.Method (N7), and the nil and zero-expiry cases of #13.	Add them.	Both
P3-8	proxy_response.go:214	Stale comment naming injectDefaultGrant.	Remove it.	Claude

---

Verified correct (both reviewers)

• Lock order: dashboard → OAuth → .secrets.lock on every path, including ClearIfOAuthThen, reset.go:114-126 and renewer.Login → AccessTokenValidFor. Services the renewer builds while holding the lock carry no renewer, so nothing takes the lock twice. oauth login releases the OAuth lock before the dashboard exchange.
• Revoking the replaced refresh token: done under the OAuth lock, and only when the old session's server or issuer matches. It is never sent to another server.
• dashboard.session filtering: survives discovery filtering for the built-in client, is removed for a client-id override (an empty result errors), and is added on relogin without duplicates. Mutation-checked.
• The unverified JWT scope check can only refuse locally. The server still decides, and no token text reaches an error.
• Keyring chunks: 2000-byte chunks fit go-keyring v0.2.6 on Windows (the 2560-byte blob) and macOS (after base64). A read never mixes two generations.
• Read-only fix, apart from R7: the not-found fast paths are linearizable, Set always takes the cross-process lock, and an existing writable .secrets.lock is still used.
• MCP: tokens only go to the two allow-listed hosts. Grant hints respect the credential's grants. The 401 retry is bounded to one and re-normalizes each attempt. No client follows redirects. API key is still the default.
• PrintableSnippet strips C0, DEL, C1 and invalid UTF-8, and cuts on rune boundaries.

Suggested order of work

1. R1 and R2. They're small and restore the server-binding invariant. Add the tests described above.
2. R3 (write order, mirroring saveTokens), then R4 and R5 (compare the session under the lock before writing or exchanging).
3. R6 and R7, then answer R8 in the PR body.
4. The open TW-5024: fix(auth): update config file when switching default grant #14 and N8/TW-5023: fix: correct Nylas dashboard URL typo #15 items, then P3-1 to P3-8, TW-5020: feat(audit): add audit logging with invoker identity tracking #21, TW-5019: Nylas Chat #22 and the feat(mcp): native MCP server + Codex assistant support #24 docs. Anything deferred should become a follow-up ticket linked from the PR body.
5. Run make ci-full and mutation-check each new test, as before.

Release questions still open from the first review. "Release order" covers #2776, but not these two:

1. Is mcp.{us,eu}.nylas.com stateless in production before this CLI ships? API-key users also stop sending Mcp-Session-Id.
2. Does /auth/cli/oauth/exchange check aud? This matters more now because of R8.

🤖 Generated with Claude Code