SUPER-752 host auth refresh#4738
Conversation
|
Capy auto-review is paused for this organization because the monthly auto-review limit has been reached. Increase the limit or turn it off in billing settings to resume automatic reviews. |
📝 WalkthroughWalkthroughThis PR implements OAuth session token persistence by adding a config-file-backed token source in host-service. The CLI exports its config path, passes it to spawned host processes, and implements atomic writes with crash-safety. The host-service reads/refreshes tokens from the config file and wires them into the JWT provider. Error messages and sensitive environment variables are sanitized throughout. ChangesOAuth Session Management with Config-File Persistence
🎯 4 (Complex) | ⏱️ ~60 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Ready to review this PR? Stage has broken it down into 14 individual chapters for you: 
Chapters generated by Stage for commit 7231222 on May 21, 2026 10:11pm UTC. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
packages/cli/src/lib/config.test.ts (1)
43-67: ⚡ Quick winRestore mocks in
finallyto prevent cross-test leakage on assertion failure.If a test throws before the tail cleanup lines, the spy remains active and can affect following tests.
Suggested fix
- const writeSpy = spyOn(fs, "writeFileSync"); - const renameSpy = spyOn(fs, "renameSync").mockImplementation( + const writeSpy = spyOn(fs, "writeFileSync"); + const renameSpy = spyOn(fs, "renameSync").mockImplementation( (oldPath: PathLike, newPath: PathLike) => { ... }, ); - - writeConfig(config); - - expect(writeSpy).toHaveBeenCalledWith(...); - ... - - writeSpy.mockRestore(); - renameSpy.mockRestore(); + try { + writeConfig(config); + expect(writeSpy).toHaveBeenCalledWith(...); + ... + } finally { + writeSpy.mockRestore(); + renameSpy.mockRestore(); + }Also applies to: 86-100
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/lib/config.test.ts` around lines 43 - 67, The test creates spies writeSpy and renameSpy around fs.writeFileSync and fs.renameSync while asserting behavior of writeConfig/readConfig, but the mocks are only restored at the end so an early assertion failure can leak spies into other tests; wrap the test body that sets up writeSpy and renameSpy (and uses originalRenameSync, writeConfig, readConfig, fs.statSync checks) in a try/finally and call writeSpy.mockRestore() and renameSpy.mockRestore() inside finally so restores always run; apply the same try/finally pattern to the other similar test block that sets up the same spies.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cli/src/commands/auth/logout/command.ts`:
- Around line 37-42: The loop that waits until Date.now() < deadline using
HOST_SHUTDOWN_TIMEOUT_MS and isProcessAlive(manifest.pid) can time out and still
allow execution to continue and clear credentials; change the logic so that
after the wait loop you explicitly check isProcessAlive(manifest.pid) and, if it
is still alive, throw an error or return a failure (failing the logout) instead
of proceeding to clear credentials; apply the same fix for the second similar
wait block (the code around the other wait with HOST_SHUTDOWN_POLL_MS) so both
places prevent credential clearing when the host did not stop within the
timeout.
In `@packages/cli/src/lib/config.ts`:
- Around line 41-48: The current atomic write uses a fixed tmpPath
(`${CONFIG_PATH}.tmp`) which can be clobbered by concurrent processes; change
the temp file creation to use a unique name per write (e.g., include process id
and a random/uuid suffix or use a secure temp API) so each writer writes to its
own tmp file, then chmod and fs.renameSync from that unique tmpPath to
CONFIG_PATH; update references to tmpPath, CONFIG_PATH, and the chmod/rename
logic to operate on the new unique temp file and ensure any cleanup handles the
unique filename.
In `@packages/cli/src/lib/resolve-auth.test.ts`:
- Around line 17-23: The spawned child process for the test (spawnSync using
process.execPath with the evaluated "source") inherits the parent's environment
so an existing SUPERSET_AUTH_CONFIG_PATH can bypass SUPERSET_HOME_DIR and cause
writeConfig() to target a real/shared file; update the env passed to spawnSync
to explicitly unset or remove SUPERSET_AUTH_CONFIG_PATH (e.g., do not propagate
it or set it to an empty value) when constructing the env object alongside
SUPERSET_HOME_DIR and SUPERSET_API_URL so each test uses the per-test tempHome
and stays isolated.
In
`@packages/host-service/src/providers/auth/JwtApiAuthProvider/JwtApiAuthProvider.ts`:
- Around line 233-237: The refresh logic in JwtApiAuthProvider is overwriting
newer CLI-authenticated credentials because it blindly uses the cached
currentCredential and writes/persists/wipes to the shared authConfigPath; modify
the refresh and error-handling flow (around currentCredential,
readCurrentCredential, loadSessionToken, and the invalid-grant path) to first
re-read the on-disk auth block and compare its identity/version to
this.currentCredential before persisting or clearing tokens — only persist or
remove the on-disk session when the on-disk credential still matches the
in-memory credential that triggered the refresh, otherwise skip writing (or
acquire a cross-process lock/version bump) to avoid clobbering newer
credentials.
In
`@packages/host-service/src/providers/auth/JwtAuthProvider/JwtAuthProvider.test.ts`:
- Around line 19-32: The test in JwtAuthProvider.test.ts replaces
globalThis.fetch with fetchMock but restores it only at the end, which can leak
the mock if an assertion throws; update the test around
JwtApiAuthProvider/getJwt so globalThis.fetch is restored in a finally block (or
use an afterEach) — ensure you keep the fetchMock assignment and assertions
(fetchMock, getSessionToken, provider.getJwt) but move the globalThis.fetch =
originalFetch into a finally so the original fetch is always restored even on
test failure.
In `@packages/shared/src/auth/token-refresh.ts`:
- Around line 27-36: The token refresh fetch in token-refresh.ts lacks a
timeout; wrap the request in an AbortController with a ~10 second timer, pass
controller.signal to fetch(`${apiUrl}/api/auth/oauth2/token`, ...), and clear
the timer once the fetch resolves; ensure the refreshToken flow (the function
performing the fetch) properly catches an AbortError and surfaces a
deterministic error so callers can recover instead of hanging. Reference the
fetch call and the surrounding token refresh function to place the
AbortController, timer (setTimeout -> controller.abort()), signal injection, and
timer cleanup.
- Around line 46-57: The OAuth2 JSON response is currently asserted into a shape
without runtime checks; add a Zod schema and validate the parsed JSON before
using it: define a zod object for { access_token: string, token_type: string,
expires_in?: number, refresh_token?: string }, call schema.parse (or safeParse
and throw a clear error) on the result of await response.json(), then use the
validated values (use validated.expires_in with default 3600 to compute
expiresIn and validated.refresh_token ?? refreshToken for refreshToken) when
building the returned LoginResult (accessToken, refreshToken, expiresAt) so
malformed responses fail-fast and downstream consumers (JwtApiAuthProvider,
resolve-auth) get only valid data.
---
Nitpick comments:
In `@packages/cli/src/lib/config.test.ts`:
- Around line 43-67: The test creates spies writeSpy and renameSpy around
fs.writeFileSync and fs.renameSync while asserting behavior of
writeConfig/readConfig, but the mocks are only restored at the end so an early
assertion failure can leak spies into other tests; wrap the test body that sets
up writeSpy and renameSpy (and uses originalRenameSync, writeConfig, readConfig,
fs.statSync checks) in a try/finally and call writeSpy.mockRestore() and
renameSpy.mockRestore() inside finally so restores always run; apply the same
try/finally pattern to the other similar test block that sets up the same spies.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1c597af2-3069-41bc-8a66-1ac9ef82783e
📒 Files selected for processing (34)
packages/cli/src/commands/auth/logout/command.test.tspackages/cli/src/commands/auth/logout/command.tspackages/cli/src/commands/start/command.test.tspackages/cli/src/lib/auth.tspackages/cli/src/lib/config.test.tspackages/cli/src/lib/config.tspackages/cli/src/lib/host/spawn.test.tspackages/cli/src/lib/host/spawn.tspackages/cli/src/lib/resolve-auth.test.tspackages/cli/src/lib/resolve-auth.tspackages/host-service/src/app.tspackages/host-service/src/env.test.tspackages/host-service/src/env.tspackages/host-service/src/errors.tspackages/host-service/src/events/event-bus.tspackages/host-service/src/events/index.tspackages/host-service/src/events/types.tspackages/host-service/src/providers/auth/JwtApiAuthProvider/JwtApiAuthProvider.test.tspackages/host-service/src/providers/auth/JwtApiAuthProvider/JwtApiAuthProvider.tspackages/host-service/src/providers/auth/JwtApiAuthProvider/index.tspackages/host-service/src/providers/auth/JwtAuthProvider/JwtAuthProvider.test.tspackages/host-service/src/providers/auth/JwtAuthProvider/JwtAuthProvider.tspackages/host-service/src/providers/auth/JwtAuthProvider/index.tspackages/host-service/src/providers/auth/hint.tspackages/host-service/src/providers/auth/index.tspackages/host-service/src/providers/auth/types.tspackages/host-service/src/serve.tspackages/host-service/src/trpc/auth-expired-middleware.test.tspackages/host-service/src/trpc/error-types.tspackages/host-service/src/trpc/index.tspackages/host-service/src/types.tspackages/shared/package.jsonpackages/shared/src/auth/index.tspackages/shared/src/auth/token-refresh.ts
| const deadline = Date.now() + HOST_SHUTDOWN_TIMEOUT_MS; | ||
| while (Date.now() < deadline) { | ||
| if (!isProcessAlive(manifest.pid)) return; | ||
| await new Promise((resolve) => setTimeout(resolve, HOST_SHUTDOWN_POLL_MS)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Fail logout when host doesn’t stop within timeout.
After Line 42, execution continues and credentials are cleared even if the host is still running. That breaks the “terminate on logout” contract and can leave an active process after logout.
Suggested fix
const deadline = Date.now() + HOST_SHUTDOWN_TIMEOUT_MS;
while (Date.now() < deadline) {
if (!isProcessAlive(manifest.pid)) return;
await new Promise((resolve) => setTimeout(resolve, HOST_SHUTDOWN_POLL_MS));
}
+
+ throw new CLIError(
+ `Host service did not stop within ${HOST_SHUTDOWN_TIMEOUT_MS}ms (pid ${manifest.pid})`,
+ );
}Also applies to: 49-52
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/cli/src/commands/auth/logout/command.ts` around lines 37 - 42, The
loop that waits until Date.now() < deadline using HOST_SHUTDOWN_TIMEOUT_MS and
isProcessAlive(manifest.pid) can time out and still allow execution to continue
and clear credentials; change the logic so that after the wait loop you
explicitly check isProcessAlive(manifest.pid) and, if it is still alive, throw
an error or return a failure (failing the logout) instead of proceeding to clear
credentials; apply the same fix for the second similar wait block (the code
around the other wait with HOST_SHUTDOWN_POLL_MS) so both places prevent
credential clearing when the host did not stop within the timeout.
| const result = spawnSync(process.execPath, ["--eval", source], { | ||
| cwd: process.cwd(), | ||
| env: { | ||
| ...process.env, | ||
| SUPERSET_HOME_DIR: tempHome, | ||
| SUPERSET_API_URL: "https://api.example.com", | ||
| }, |
There was a problem hiding this comment.
Clear SUPERSET_AUTH_CONFIG_PATH from spawned scenarios.
These child processes inherit the parent env, so an existing SUPERSET_AUTH_CONFIG_PATH will override SUPERSET_HOME_DIR and make writeConfig() hit a real/shared auth file instead of the per-test temp home. That breaks isolation and can mutate a developer's local credentials while this suite runs.
Suggested fix
function runScenario(source: string): ScenarioResult {
const tempHome = fs.mkdtempSync(
path.join(os.tmpdir(), "superset-cli-resolve-auth-"),
);
tempHomes.push(tempHome);
+ const { SUPERSET_AUTH_CONFIG_PATH: _ignoredAuthConfigPath, ...env } =
+ process.env;
+
const result = spawnSync(process.execPath, ["--eval", source], {
cwd: process.cwd(),
env: {
- ...process.env,
+ ...env,
SUPERSET_HOME_DIR: tempHome,
SUPERSET_API_URL: "https://api.example.com",
},
encoding: "utf-8",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const result = spawnSync(process.execPath, ["--eval", source], { | |
| cwd: process.cwd(), | |
| env: { | |
| ...process.env, | |
| SUPERSET_HOME_DIR: tempHome, | |
| SUPERSET_API_URL: "https://api.example.com", | |
| }, | |
| const { SUPERSET_AUTH_CONFIG_PATH: _ignoredAuthConfigPath, ...env } = | |
| process.env; | |
| const result = spawnSync(process.execPath, ["--eval", source], { | |
| cwd: process.cwd(), | |
| env: { | |
| ...env, | |
| SUPERSET_HOME_DIR: tempHome, | |
| SUPERSET_API_URL: "https://api.example.com", | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/cli/src/lib/resolve-auth.test.ts` around lines 17 - 23, The spawned
child process for the test (spawnSync using process.execPath with the evaluated
"source") inherits the parent's environment so an existing
SUPERSET_AUTH_CONFIG_PATH can bypass SUPERSET_HOME_DIR and cause writeConfig()
to target a real/shared file; update the env passed to spawnSync to explicitly
unset or remove SUPERSET_AUTH_CONFIG_PATH (e.g., do not propagate it or set it
to an empty value) when constructing the env object alongside SUPERSET_HOME_DIR
and SUPERSET_API_URL so each test uses the per-test tempHome and stays isolated.
| const credential = this.currentCredential ?? this.readCurrentCredential(); | ||
| if (!credential) { | ||
| return this.loadSessionToken(); | ||
| } | ||
| this.currentCredential = credential; |
There was a problem hiding this comment.
Avoid clobbering newer CLI credentials from stale in-memory auth state.
currentCredential is cached across calls, but authConfigPath is shared with the CLI. If the user re-authenticates while the host is running, the next refresh here can write the old session back to disk, and the invalid-grant path can strip the refresh token from the newer session. Persist/wipe only when the on-disk auth block still matches the credential that triggered the refresh failure, or add a cross-process lock/version check before writing.
Also applies to: 325-330, 416-427
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@packages/host-service/src/providers/auth/JwtApiAuthProvider/JwtApiAuthProvider.ts`
around lines 233 - 237, The refresh logic in JwtApiAuthProvider is overwriting
newer CLI-authenticated credentials because it blindly uses the cached
currentCredential and writes/persists/wipes to the shared authConfigPath; modify
the refresh and error-handling flow (around currentCredential,
readCurrentCredential, loadSessionToken, and the invalid-grant path) to first
re-read the on-disk auth block and compare its identity/version to
this.currentCredential before persisting or clearing tokens — only persist or
remove the on-disk session when the on-disk credential still matches the
in-memory credential that triggered the refresh, otherwise skip writing (or
acquire a cross-process lock/version bump) to avoid clobbering newer
credentials.
| const response = await fetch(`${apiUrl}/api/auth/oauth2/token`, { | ||
| method: "POST", | ||
| headers: { "Content-Type": "application/x-www-form-urlencoded" }, | ||
| body: new URLSearchParams({ | ||
| grant_type: "refresh_token", | ||
| refresh_token: refreshToken, | ||
| client_id: CLIENT_ID, | ||
| resource: apiUrl, | ||
| }), | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "token-refresh.ts" -type fRepository: superset-sh/superset
Length of output: 108
🏁 Script executed:
cat -n packages/shared/src/auth/token-refresh.tsRepository: superset-sh/superset
Length of output: 1809
🏁 Script executed:
# Check if AbortController is already used elsewhere in the codebase
rg "AbortController" --type ts --type tsxRepository: superset-sh/superset
Length of output: 92
🏁 Script executed:
# Check the full context of the refreshAccessToken function
sed -n '1,60p' packages/shared/src/auth/token-refresh.tsRepository: superset-sh/superset
Length of output: 1396
Add a timeout/cancellation path for token refresh requests.
The fetch call on line 27 has no timeout, allowing token refresh to hang indefinitely and block authentication recovery flows. Implement AbortController with a reasonable timeout (e.g., 10 seconds) and signal it to the fetch request.
Proposed fix
export async function refreshAccessToken(
refreshToken: string,
): Promise<LoginResult> {
const apiUrl = getApiUrl();
+ const controller = new AbortController();
+ const timeout = setTimeout(() => controller.abort(), 10_000);
+ let response: Response;
+ try {
- const response = await fetch(`${apiUrl}/api/auth/oauth2/token`, {
+ response = await fetch(`${apiUrl}/api/auth/oauth2/token`, {
method: "POST",
headers: { "Content-Type": "application/x-www-form-urlencoded" },
body: new URLSearchParams({
grant_type: "refresh_token",
refresh_token: refreshToken,
client_id: CLIENT_ID,
resource: apiUrl,
}),
+ signal: controller.signal,
});
+ } catch {
+ throw new CLIError(
+ "Token refresh failed: network error",
+ "Check connectivity and run `superset auth login` again.",
+ );
+ } finally {
+ clearTimeout(timeout);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const response = await fetch(`${apiUrl}/api/auth/oauth2/token`, { | |
| method: "POST", | |
| headers: { "Content-Type": "application/x-www-form-urlencoded" }, | |
| body: new URLSearchParams({ | |
| grant_type: "refresh_token", | |
| refresh_token: refreshToken, | |
| client_id: CLIENT_ID, | |
| resource: apiUrl, | |
| }), | |
| }); | |
| export async function refreshAccessToken( | |
| refreshToken: string, | |
| ): Promise<LoginResult> { | |
| const apiUrl = getApiUrl(); | |
| const controller = new AbortController(); | |
| const timeout = setTimeout(() => controller.abort(), 10_000); | |
| let response: Response; | |
| try { | |
| response = await fetch(`${apiUrl}/api/auth/oauth2/token`, { | |
| method: "POST", | |
| headers: { "Content-Type": "application/x-www-form-urlencoded" }, | |
| body: new URLSearchParams({ | |
| grant_type: "refresh_token", | |
| refresh_token: refreshToken, | |
| client_id: CLIENT_ID, | |
| resource: apiUrl, | |
| }), | |
| signal: controller.signal, | |
| }); | |
| } catch { | |
| throw new CLIError( | |
| "Token refresh failed: network error", | |
| "Check connectivity and run `superset auth login` again.", | |
| ); | |
| } finally { | |
| clearTimeout(timeout); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/shared/src/auth/token-refresh.ts` around lines 27 - 36, The token
refresh fetch in token-refresh.ts lacks a timeout; wrap the request in an
AbortController with a ~10 second timer, pass controller.signal to
fetch(`${apiUrl}/api/auth/oauth2/token`, ...), and clear the timer once the
fetch resolves; ensure the refreshToken flow (the function performing the fetch)
properly catches an AbortError and surfaces a deterministic error so callers can
recover instead of hanging. Reference the fetch call and the surrounding token
refresh function to place the AbortController, timer (setTimeout ->
controller.abort()), signal injection, and timer cleanup.
| const data = (await response.json()) as { | ||
| access_token: string; | ||
| token_type: string; | ||
| expires_in?: number; | ||
| refresh_token?: string; | ||
| }; | ||
|
|
||
| const expiresIn = data.expires_in ?? 60 * 60; | ||
| return { | ||
| accessToken: data.access_token, | ||
| refreshToken: data.refresh_token ?? refreshToken, | ||
| expiresAt: Date.now() + expiresIn * 1000, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if the file exists and read it
find . -path "*/packages/shared/src/auth/token-refresh.ts" -type fRepository: superset-sh/superset
Length of output: 108
🏁 Script executed:
# List files in the packages directory to understand structure
git ls-files | grep -E "^packages/shared/src/auth" | head -20Repository: superset-sh/superset
Length of output: 385
🏁 Script executed:
# Check project dependencies for zod
find . -name "package.json" -type f | head -5Repository: superset-sh/superset
Length of output: 202
🏁 Script executed:
# Read the token-refresh.ts file to verify snippet
cat -n ./packages/shared/src/auth/token-refresh.tsRepository: superset-sh/superset
Length of output: 1809
🏁 Script executed:
# Check for zod in package.json files
find . -name "package.json" | xargs grep -l '"zod"' 2>/dev/null | head -10Repository: superset-sh/superset
Length of output: 332
🏁 Script executed:
# Check packages/shared/package.json specifically
cat ./packages/shared/package.json | grep -A 30 '"dependencies"'Repository: superset-sh/superset
Length of output: 333
🏁 Script executed:
# Find where refreshAccessToken is called
rg "refreshAccessToken" --type ts --type tsx -A 3 -B 1Repository: superset-sh/superset
Length of output: 92
🏁 Script executed:
# Check how LoginResult is used downstream
rg "LoginResult" --type ts --type tsx -B 2 -A 2Repository: superset-sh/superset
Length of output: 92
🏁 Script executed:
# Look for other zod validation patterns in the codebase
rg "z\.object\|z\.string\|z\.number" --type ts --type tsx | head -20Repository: superset-sh/superset
Length of output: 92
🏁 Script executed:
# Find where refreshAccessToken is called
rg "refreshAccessToken" -A 3 -B 1 --max-count 10Repository: superset-sh/superset
Length of output: 9840
🏁 Script executed:
# Check how LoginResult is used downstream
rg "LoginResult" -B 2 -A 2Repository: superset-sh/superset
Length of output: 4092
🏁 Script executed:
# Look for other zod validation patterns in the codebase
rg "from ['\"]zod['\"]" -A 5 | head -40Repository: superset-sh/superset
Length of output: 2375
Add runtime validation for OAuth2 token response.
The code uses a type assertion on line 46 without validating the JSON shape at runtime. If the OAuth2 endpoint returns a malformed response (missing access_token, invalid expires_in, etc.), an invalid LoginResult is returned and propagated downstream to JwtApiAuthProvider and resolve-auth, breaking authentication behavior.
Zod is already a project dependency. Add schema validation before returning:
Proposed fix
+import { z } from "zod";
+
const CLIENT_ID = "superset-cli";
+
+const TokenRefreshResponseSchema = z.object({
+ access_token: z.string().min(1),
+ token_type: z.string().min(1),
+ expires_in: z.number().int().positive().optional(),
+ refresh_token: z.string().min(1).optional(),
+});
...
- const data = (await response.json()) as {
- access_token: string;
- token_type: string;
- expires_in?: number;
- refresh_token?: string;
- };
+ const data = TokenRefreshResponseSchema.parse(await response.json());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const data = (await response.json()) as { | |
| access_token: string; | |
| token_type: string; | |
| expires_in?: number; | |
| refresh_token?: string; | |
| }; | |
| const expiresIn = data.expires_in ?? 60 * 60; | |
| return { | |
| accessToken: data.access_token, | |
| refreshToken: data.refresh_token ?? refreshToken, | |
| expiresAt: Date.now() + expiresIn * 1000, | |
| import { z } from "zod"; | |
| const CLIENT_ID = "superset-cli"; | |
| const TokenRefreshResponseSchema = z.object({ | |
| access_token: z.string().min(1), | |
| token_type: z.string().min(1), | |
| expires_in: z.number().int().positive().optional(), | |
| refresh_token: z.string().min(1).optional(), | |
| }); | |
| // ... other code ... | |
| const data = TokenRefreshResponseSchema.parse(await response.json()); | |
| const expiresIn = data.expires_in ?? 60 * 60; | |
| return { | |
| accessToken: data.access_token, | |
| refreshToken: data.refresh_token ?? refreshToken, | |
| expiresAt: Date.now() + expiresIn * 1000, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/shared/src/auth/token-refresh.ts` around lines 46 - 57, The OAuth2
JSON response is currently asserted into a shape without runtime checks; add a
Zod schema and validate the parsed JSON before using it: define a zod object for
{ access_token: string, token_type: string, expires_in?: number, refresh_token?:
string }, call schema.parse (or safeParse and throw a clear error) on the result
of await response.json(), then use the validated values (use
validated.expires_in with default 3600 to compute expiresIn and
validated.refresh_token ?? refreshToken for refreshToken) when building the
returned LoginResult (accessToken, refreshToken, expiresAt) so malformed
responses fail-fast and downstream consumers (JwtApiAuthProvider, resolve-auth)
get only valid data.
Greptile SummaryThis PR fixes a silent auth expiry bug in CLI-spawned host-service daemons: the host's OAuth access token could expire while the daemon kept running, and the existing 401 retry link had no way to recover without a restart. The fix introduces
Confidence Score: 4/5Safe to merge with one fix: The core token-refresh and atomic-write logic is well-designed and well-tested. One defect: when packages/host-service/src/providers/auth/ConfigFileSessionTokenSource/ConfigFileSessionTokenSource.ts — the
|
| Filename | Overview |
|---|---|
| packages/host-service/src/providers/auth/ConfigFileSessionTokenSource/ConfigFileSessionTokenSource.ts | New config-backed token source with single-flight refresh and atomic config writes; refreshNeeded is not cleared on refresh failure, causing perpetual OAuth retry attempts on every subsequent request. |
| packages/host-service/src/providers/auth/JwtAuthProvider/JwtAuthProvider.ts | Minimal, clean addition of optional onInvalidateCache callback forwarded when the JWT cache is cleared; no issues. |
| packages/host-service/src/serve.ts | Correctly wires ConfigFileSessionTokenSource when SUPERSET_AUTH_CONFIG_PATH is present and falls back to static AUTH_TOKEN otherwise; no issues. |
| packages/cli/src/lib/config.ts | Upgrades to atomic write-via-rename with UUID-based temp files and cleans up the temp on rename failure; safe and correct. |
| packages/cli/src/lib/auth.ts | Replaces the raw response body in error suggestions with a static LOGIN_AGAIN_SUGGESTION, preventing token/cookie payloads from leaking into user-visible output. |
| packages/cli/src/lib/host/spawn.ts | Extracts env construction into createHostServiceEnv; conditionally passes SUPERSET_AUTH_CONFIG_PATH only for OAuth sessions; no issues. |
| apps/desktop/src/main/lib/host-service-coordinator.ts | Unconditionally sets SUPERSET_AUTH_CONFIG_PATH for all desktop-spawned hosts, unlike the CLI which gates it on OAuth sessions; AUTH_TOKEN is effectively unused for desktop sessions now. |
| packages/host-service/src/terminal/env-strip.ts | Adds SUPERSET_AUTH_CONFIG_PATH to the PTY strip list and introduces SENSITIVE_AUTH_KEYS for explicit refresh-token keys; correct and well-commented. |
| packages/host-service/src/providers/auth/JwtAuthProvider/JwtAuthProvider.test.ts | Comprehensive test suite covering pre-401 token use, refresh after invalidation, concurrent single-flight, rotated refresh token persistence, missing refresh token, sanitized failures, and external-write detection; no coverage for failed-refresh + subsequent request behavior. |
| packages/cli/src/lib/config.test.ts | Tests unique temp paths, rename-failure recovery, and correct final config path; well-structured with real FS and mocked rename. |
Sequence Diagram
sequenceDiagram
participant App as tRPC Caller
participant Retry as 401 Retry Link
participant JWT as JwtApiAuthProvider
participant CFS as ConfigFileSessionTokenSource
participant FS as config.json
participant OAuth as OAuth Server
App->>JWT: getHeaders()
JWT->>CFS: "getSessionToken() [refreshNeeded=false]"
CFS->>FS: readConfig()
FS-->>CFS: "{ auth: { accessToken: stale } }"
CFS-->>JWT: stale.jwt.token
JWT-->>App: Authorization: Bearer stale.jwt.token
App->>+Retry: "cloud request -> 401"
Retry->>JWT: invalidateCache()
JWT->>JWT: "cachedJwt = null"
JWT->>CFS: "onInvalidateCache() -> refreshNeeded = true"
Retry->>JWT: getHeaders() [retry]
JWT->>CFS: "getSessionToken() [refreshNeeded=true]"
CFS->>FS: readConfig()
FS-->>CFS: "{ auth: { accessToken: stale, refreshToken: rt } }"
CFS->>OAuth: POST /api/auth/oauth2/token
OAuth-->>CFS: "{ access_token: fresh, refresh_token: rotated }"
CFS->>FS: readConfig() [stale-check]
CFS->>FS: "writeConfig({ auth: { accessToken: fresh, refreshToken: rotated } })"
CFS->>CFS: "refreshNeeded = false"
CFS-->>JWT: fresh.jwt.token
JWT-->>Retry: Authorization: Bearer fresh.jwt.token
Retry-->>-App: success
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
packages/host-service/src/providers/auth/ConfigFileSessionTokenSource/ConfigFileSessionTokenSource.ts:120-122
**`refreshNeeded` persists after a failed refresh, causing perpetual OAuth attempts**
When `refreshAccessToken` throws — network error, non-2xx response, or missing `access_token` — the `.finally()` only clears `refreshPromise`. `refreshNeeded` stays `true`, and `JwtApiAuthProvider` never re-populates `cachedJwt` on failure. Every subsequent `getHeaders()` call therefore sees `cachedJwt = null`, calls `getSessionToken()`, finds `refreshNeeded = true` with no in-flight promise, and launches a new OAuth refresh. Clearing `refreshNeeded` in the same `finally` block ensures a subsequent `invalidateCache()` from the next 401 is what re-arms the retry, rather than every request doing so automatically.
```suggestion
this.refreshPromise = this.refreshAccessToken(auth).finally(() => {
this.refreshPromise = null;
this.refreshNeeded = false;
});
```
Reviews (2): Last reviewed commit: "refactor(cli): inline host-service env c..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const expiresAt = readJwtExp(credential.accessToken); | ||
| const needsRefresh = | ||
| this.expired !== null || | ||
| (expiresAt !== null && expiresAt - Date.now() <= JWT_REFRESH_BUFFER_MS); | ||
| if (!needsRefresh) { | ||
| return credential.accessToken; |
There was a problem hiding this comment.
credential.expiresAt is ignored in needsRefresh
needsRefresh only uses readJwtExp(credential.accessToken). If readJwtExp returns null — e.g., an opaque access token, a JWT without an exp claim, or a Base64-decoding failure — needsRefresh stays false indefinitely regardless of this.expired, and the stale token is returned forever. credential.expiresAt is already populated and validated by isSupersetAuthConfig, and adding || credential.expiresAt - Date.now() <= JWT_REFRESH_BUFFER_MS to the condition would cover this gap without any other changes.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/host-service/src/providers/auth/JwtApiAuthProvider/JwtApiAuthProvider.ts
Line: 239-244
Comment:
**`credential.expiresAt` is ignored in `needsRefresh`**
`needsRefresh` only uses `readJwtExp(credential.accessToken)`. If `readJwtExp` returns `null` — e.g., an opaque access token, a JWT without an `exp` claim, or a Base64-decoding failure — `needsRefresh` stays `false` indefinitely regardless of `this.expired`, and the stale token is returned forever. `credential.expiresAt` is already populated and validated by `isSupersetAuthConfig`, and adding `|| credential.expiresAt - Date.now() <= JWT_REFRESH_BUFFER_MS` to the condition would cover this gap without any other changes.
How can I resolve this? If you propose a fix, please make it concise.| function readStatusCode(error: unknown): number | undefined { | ||
| if (isObject(error) && typeof error.statusCode === "number") { | ||
| return error.statusCode; | ||
| } | ||
| const message = error instanceof Error ? error.message : String(error); | ||
| const match = /Token refresh failed:\s*(\d{3})/.exec(message); | ||
| if (!match?.[1]) return undefined; | ||
| return Number.parseInt(match[1], 10); | ||
| } | ||
|
|
||
| function errorIndicatesInvalidGrant(error: unknown): boolean { | ||
| const message = error instanceof Error ? error.message : String(error); | ||
| const suggestion = | ||
| isObject(error) && typeof error.suggestion === "string" | ||
| ? error.suggestion | ||
| : ""; | ||
| return /\binvalid_grant\b/i.test(`${message}\n${suggestion}`); | ||
| } | ||
|
|
||
| function reasonForRefreshError(error: unknown): RefreshFailureClassification { | ||
| const statusCode = readStatusCode(error); | ||
| if (statusCode === undefined) { | ||
| return { reason: "network_error" }; | ||
| } | ||
| if ( | ||
| statusCode === 401 || | ||
| ((statusCode === 400 || statusCode === 403) && | ||
| errorIndicatesInvalidGrant(error)) | ||
| ) { | ||
| return { reason: "invalid_grant", statusCode }; | ||
| } | ||
| return { reason: "http_error", statusCode }; |
There was a problem hiding this comment.
reasonForRefreshError and the private CLIError message format
readStatusCode extracts the HTTP status code via a regex on the message string "Token refresh failed:\s*(\d{3})". This string is produced by the private, unexported CLIError inside token-refresh.ts. If that message format changes — or if another error type is ever thrown by refreshAccessToken — readStatusCode returns undefined, which causes reasonForRefreshError to classify the failure as "network_error" instead of "http_error" or "invalid_grant". The practical consequence is that a genuine invalid_grant 4xx response would be treated as a retryable transient error, silently bypassing credential wipe.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/host-service/src/providers/auth/JwtApiAuthProvider/JwtApiAuthProvider.ts
Line: 105-136
Comment:
**Implicit coupling between `reasonForRefreshError` and the private `CLIError` message format**
`readStatusCode` extracts the HTTP status code via a regex on the message string `"Token refresh failed:\s*(\d{3})"`. This string is produced by the private, unexported `CLIError` inside `token-refresh.ts`. If that message format changes — or if another error type is ever thrown by `refreshAccessToken` — `readStatusCode` returns `undefined`, which causes `reasonForRefreshError` to classify the failure as `"network_error"` instead of `"http_error"` or `"invalid_grant"`. The practical consequence is that a genuine `invalid_grant` 4xx response would be treated as a retryable transient error, silently bypassing credential wipe.
How can I resolve this? If you propose a fix, please make it concise.| function getApiUrl(): string { | ||
| return process.env.SUPERSET_API_URL || "https://api.superset.sh"; | ||
| } |
There was a problem hiding this comment.
getApiUrl() returns a hardcoded production URL when SUPERSET_API_URL is unset. In a test or staging environment where the variable is accidentally absent, token refreshes silently hit the production auth server rather than failing fast. The CLI's original implementation read from the validated env.SUPERSET_API_URL, which would throw on startup if the variable was missing. Throwing when SUPERSET_API_URL is undefined would be safer.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/shared/src/auth/token-refresh.ts
Line: 19-21
Comment:
**Silent fallback URL could refresh tokens against production**
`getApiUrl()` returns a hardcoded production URL when `SUPERSET_API_URL` is unset. In a test or staging environment where the variable is accidentally absent, token refreshes silently hit the production auth server rather than failing fast. The CLI's original implementation read from the validated `env.SUPERSET_API_URL`, which would throw on startup if the variable was missing. Throwing when `SUPERSET_API_URL` is undefined would be safer.
How can I resolve this? If you propose a fix, please make it concise.| isInAnyExpiredState?(): boolean; | ||
| isInExpiredState?(): boolean; |
There was a problem hiding this comment.
isInExpiredState is a dead alias for isInAnyExpiredState
The implementation just calls this.isInAnyExpiredState(), and the tRPC middleware exclusively calls isInAnyExpiredState. No caller uses isInExpiredState, making both the interface entry and the implementation method unreachable dead code. Keeping both in the interface creates confusion about whether callers should prefer one over the other.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/host-service/src/providers/auth/types.ts
Line: 24-25
Comment:
**`isInExpiredState` is a dead alias for `isInAnyExpiredState`**
The implementation just calls `this.isInAnyExpiredState()`, and the tRPC middleware exclusively calls `isInAnyExpiredState`. No caller uses `isInExpiredState`, making both the interface entry and the implementation method unreachable dead code. Keeping both in the interface creates confusion about whether callers should prefer one over the other.
How can I resolve this? If you propose a fix, please make it concise.![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 34 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| migrationsFolder: string, | ||
| ): NodeJS.ProcessEnv { | ||
| const childEnv: NodeJS.ProcessEnv = { ...process.env }; | ||
| for (const key of Object.keys(childEnv)) { |
There was a problem hiding this comment.
hmm was this the reason we needed this change in this file? What does this change do?
There was a problem hiding this comment.
Two things:
-
Extracted
createHostServiceEnv— the inline env block was growing (newSUPERSET_AUTH_CONFIG_PATH+ the filter); pulling it out keepsspawnHostServicefocused. -
Strips
*REFRESH_TOKEN*env keys (broad match on purpose — catches CI vars too). New in this PR the daemon re-reads~/.superset/config.jsonfor refreshed tokens. I want the config file to be the only path a refresh token reaches the daemon, not env inheritance. A stray var from the parent shell/CI would otherwise sit in/proc/<pid>/environfor the daemon's lifetime.
Defense-in-depth, not load-bearing. Happy to narrow to a permitted list if you'd rather.
| writeFileSync, | ||
| }; | ||
|
|
||
| export function writeConfigFile( |
There was a problem hiding this comment.
Could you help me understand the goal with this change too? Was it an attempt to make writeConfig more robust? if so, was there a detected gap?
There was a problem hiding this comment.
Pushed 791bd69 — dropped the writeConfigFile export and inlined the atomic-write logic into writeConfig. Public surface is back to status-quo (one exported writer). Tests rewritten to use mock.module("node:fs", ...) with snapshotted real impls; same three behavioral assertions, all passing.
Substantive answer to your original question, for the record:
Robustness for a real gap this PR introduces. Before, only the CLI wrote to config.json. After, the daemon also writes when it refreshes tokens on a 401 (ConfigFileSessionTokenSource.ts:178). CLI and daemon can now write concurrently.
The daemon already has a CAS-style check before its write (re-reads, bails if a CLI-set apiKey appeared or another writer beat it) — that handles write-write conflicts on its side. Atomic-write is still load-bearing because the CLI's readConfig is a plain readFileSync + JSON.parse with no retry; a torn write from the daemon would crash that read and log the user out.
writeConfig now switches to write-temp + rename: .<uuid>.<pid>.config.tmp → chmod 0600 → renameSync onto target → unlinkSync on failure. Readers see either old or new, never partial.
One open thing: the daemon has a near-identical atomic-write helper at ConfigFileSessionTokenSource.ts:55-75 (same shape, node:fs/promises instead of sync). Worth factoring into @superset/shared so CLI and daemon stay in sync if the pattern evolves, or leave them as intentional twins given the sync/async + process-boundary split?
justincrich
force-pushed
the
super-752-host-auth-refresh
branch
2 times, most recently
from
35ecc98 to
0a5a1ec
Compare
There was a problem hiding this comment.
1 issue found across 18 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| this.refreshPromise = this.refreshAccessToken(auth).finally(() => { | ||
| this.refreshPromise = null; | ||
| }); |
There was a problem hiding this comment.
refreshNeeded persists after a failed refresh, causing perpetual OAuth attempts
When refreshAccessToken throws — network error, non-2xx response, or missing access_token — the .finally() only clears refreshPromise. refreshNeeded stays true, and JwtApiAuthProvider never re-populates cachedJwt on failure. Every subsequent getHeaders() call therefore sees cachedJwt = null, calls getSessionToken(), finds refreshNeeded = true with no in-flight promise, and launches a new OAuth refresh. Clearing refreshNeeded in the same finally block ensures a subsequent invalidateCache() from the next 401 is what re-arms the retry, rather than every request doing so automatically.
| this.refreshPromise = this.refreshAccessToken(auth).finally(() => { | |
| this.refreshPromise = null; | |
| }); | |
| this.refreshPromise = this.refreshAccessToken(auth).finally(() => { | |
| this.refreshPromise = null; | |
| this.refreshNeeded = false; | |
| }); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/host-service/src/providers/auth/ConfigFileSessionTokenSource/ConfigFileSessionTokenSource.ts
Line: 120-122
Comment:
**`refreshNeeded` persists after a failed refresh, causing perpetual OAuth attempts**
When `refreshAccessToken` throws — network error, non-2xx response, or missing `access_token` — the `.finally()` only clears `refreshPromise`. `refreshNeeded` stays `true`, and `JwtApiAuthProvider` never re-populates `cachedJwt` on failure. Every subsequent `getHeaders()` call therefore sees `cachedJwt = null`, calls `getSessionToken()`, finds `refreshNeeded = true` with no in-flight promise, and launches a new OAuth refresh. Clearing `refreshNeeded` in the same `finally` block ensures a subsequent `invalidateCache()` from the next 401 is what re-arms the retry, rather than every request doing so automatically.
```suggestion
this.refreshPromise = this.refreshAccessToken(auth).finally(() => {
this.refreshPromise = null;
this.refreshNeeded = false;
});
```
How can I resolve this? If you propose a fix, please make it concise.
Drop the `writeConfigFile` / `ConfigWriterFs` / `defaultConfigWriterFs`
exports. Atomic write logic moves directly into `writeConfig` — the only
public API callers actually use. Reverts the public surface to match
status-quo (one exported writer).
Tests use `mock.module("node:fs", ...)` with snapshotted real impls
captured before the mock takes effect. Same three behavioral assertions:
unique temp filenames, rename-failure preserves old config, writes to
exported path. 12/12 tests pass across the cli package.
…s auth config path from desktop PR feedback: - Replace ad-hoc REFRESH_TOKEN scrub in createHostServiceEnv with explicit protection at the canonical host-service-to-terminal boundary (SENSITIVE_AUTH_KEYS in env-strip.ts). Equivalent test coverage moves from spawn.test.ts to env.test.ts. - Mirror SUPERSET_AUTH_CONFIG_PATH propagation in the desktop's HostServiceCoordinator.buildEnv() so a desktop-launched host-service can honor an existing config-file refresh token (no-op when the file lacks one; ConfigFileSessionTokenSource gates on auth.refreshToken). Verification: - bun test packages/host-service/src/terminal/env.test.ts (40 pass) - bun test packages/cli/src/lib/host/spawn.test.ts (1 pass) Pre-existing failures (proven via git stash; not from these changes): - bun run typecheck fails in @superset/db (Cannot find module 'pg') — worktree node_modules is missing pg entirely; environment setup issue. - bun run lint fails on .claude/settings.json formatting (Biome wants multi-line objects expanded) — a Claude Code local-config file outside this PR's scope.
Reverts the createHostServiceEnv helper added during HOST-AUTH work — it added an unnecessary level of indirection for a one-line conditional spread. The fix (passing SUPERSET_AUTH_CONFIG_PATH only when authConfigPath is provided) is preserved inline in spawnHostService. Behavior unchanged; spawn.test.ts still passes.
justincrich
force-pushed
the
super-752-host-auth-refresh
branch
from
83cbfce to
7231222
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/cli/src/lib/config.test.ts (1)
70-115: ⚡ Quick winAdd an explicit mode assertion for
config.jsonpermissions.Current tests verify atomicity and cleanup well, but they no longer pin the
0o600invariant on the final config file. Adding this closes an easy security-regression gap.Proposed test addition
describe("config writes", () => { + test("writeConfig enforces 0o600 on final config file", () => { + writeConfig({ apiKey: "sk_live_mode_check" }); + const mode = realFs.statSync(SUPERSET_CONFIG_PATH).mode & 0o777; + expect(mode).toBe(0o600); + }); + test("writeConfig uses unique temp files", () => { writeConfig({ apiKey: "sk_live_one" }); writeConfig({ apiKey: "sk_live_two" });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/lib/config.test.ts` around lines 70 - 115, Add an assertion in the existing "config writes" tests (the tests exercising writeConfig and reading SUPERSET_CONFIG_PATH) that verifies the final config file permissions are set to 0o600; after each read of SUPERSET_CONFIG_PATH (e.g., in "writeConfig uses unique temp files", "writeConfig preserves old config if rename fails" where final file is inspected, and "writeConfig writes the exported Superset config path") call fs.statSync(SUPERSET_CONFIG_PATH).mode (or the test environment equivalent) and assert the permission bits equal 0o600 so the tests pin the file-mode invariant for the file produced by writeConfig.packages/cli/src/lib/host/spawn.test.ts (1)
77-98: ⚡ Quick winAdd the negative-path env test for omitted
authConfigPath.Please also assert
SUPERSET_AUTH_CONFIG_PATHis absent whenauthConfigPathis not provided, so static-token behavior is locked in by this unit suite.Proposed test addition
describe("spawnHostService", () => { test("passes SUPERSET_AUTH_CONFIG_PATH when provided", async () => { @@ expect(spawnCalls[0]?.options.env?.AUTH_TOKEN).toBe("session-token"); }); + + test("does not pass SUPERSET_AUTH_CONFIG_PATH when omitted", async () => { + globalThis.fetch = mock(async () => new Response("ok", { status: 200 })) as unknown as typeof fetch; + + await spawnHostService({ + organizationId: "00000000-0000-0000-0000-000000000001", + sessionToken: "session-token", + api: createApi(), + port: 54879, + daemon: true, + }); + + expect(spawnCalls[0]?.options.env?.SUPERSET_AUTH_CONFIG_PATH).toBeUndefined(); + }); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/lib/host/spawn.test.ts` around lines 77 - 98, Add a negative-path unit test in spawn.test.ts that calls spawnHostService without the authConfigPath and asserts the child process env does not include SUPERSET_AUTH_CONFIG_PATH; specifically reuse the existing test setup (mocked global.fetch, spawnMock/spawnCalls) and after awaiting spawnHostService({... with no authConfigPath ...}) assert spawnMock was called once and that spawnCalls[0]?.options.env?.SUPERSET_AUTH_CONFIG_PATH is undefined (and keep the existing assertion that AUTH_TOKEN equals "session-token"). This ensures spawnHostService's logic around SUPERSET_AUTH_CONFIG_PATH is covered when authConfigPath is omitted.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/cli/src/lib/config.test.ts`:
- Around line 70-115: Add an assertion in the existing "config writes" tests
(the tests exercising writeConfig and reading SUPERSET_CONFIG_PATH) that
verifies the final config file permissions are set to 0o600; after each read of
SUPERSET_CONFIG_PATH (e.g., in "writeConfig uses unique temp files",
"writeConfig preserves old config if rename fails" where final file is
inspected, and "writeConfig writes the exported Superset config path") call
fs.statSync(SUPERSET_CONFIG_PATH).mode (or the test environment equivalent) and
assert the permission bits equal 0o600 so the tests pin the file-mode invariant
for the file produced by writeConfig.
In `@packages/cli/src/lib/host/spawn.test.ts`:
- Around line 77-98: Add a negative-path unit test in spawn.test.ts that calls
spawnHostService without the authConfigPath and asserts the child process env
does not include SUPERSET_AUTH_CONFIG_PATH; specifically reuse the existing test
setup (mocked global.fetch, spawnMock/spawnCalls) and after awaiting
spawnHostService({... with no authConfigPath ...}) assert spawnMock was called
once and that spawnCalls[0]?.options.env?.SUPERSET_AUTH_CONFIG_PATH is undefined
(and keep the existing assertion that AUTH_TOKEN equals "session-token"). This
ensures spawnHostService's logic around SUPERSET_AUTH_CONFIG_PATH is covered
when authConfigPath is omitted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1959982b-cf92-440d-884c-4b3c965a59a7
📒 Files selected for processing (18)
apps/desktop/src/main/lib/host-service-coordinator.tspackages/cli/src/commands/start/command.tspackages/cli/src/lib/auth.test.tspackages/cli/src/lib/auth.tspackages/cli/src/lib/config.test.tspackages/cli/src/lib/config.tspackages/cli/src/lib/host/spawn.test.tspackages/cli/src/lib/host/spawn.tspackages/host-service/src/env.test.tspackages/host-service/src/env.tspackages/host-service/src/providers/auth/ConfigFileSessionTokenSource/ConfigFileSessionTokenSource.tspackages/host-service/src/providers/auth/ConfigFileSessionTokenSource/index.tspackages/host-service/src/providers/auth/JwtAuthProvider/JwtAuthProvider.test.tspackages/host-service/src/providers/auth/JwtAuthProvider/JwtAuthProvider.tspackages/host-service/src/providers/auth/index.tspackages/host-service/src/serve.tspackages/host-service/src/terminal/env-strip.tspackages/host-service/src/terminal/env.test.ts
✅ Files skipped from review due to trivial changes (1)
- packages/host-service/src/providers/auth/ConfigFileSessionTokenSource/index.ts
…UPER-750) Greptile flagged that the previous "sanitizes OAuth refresh failure details" test (covering refreshAccessToken in auth.ts) was dropped when the test file was rewritten for SUPER-750. The function still ships and exports; its error-path silently strips access_token/refresh_token/ redirect-with-auth-code/cookie from the thrown CLIError. Without this test, an accidental regression could leak tokens into CLI error output. Restored from cf3330e (SUPER-752 superset-sh#4738) verbatim, retitled with "(regression)" suffix to match the AC-2 baseline naming.
2 participants
Overview
Linear: https://linear.app/superset-sh/issue/SUPER-752/cli-host-service-silently-breaks-when-its-auth-session-expires-never
CLI-spawned host-service starts with an OAuth access token that can expire while the daemon keeps running. The host cloud client already retries once after a 401; this PR makes that retry path refresh from the CLI auth config instead of requiring a host restart or user re-login.
What Changed
Config-backed host auth now returns the stored config access token normally. After
JwtApiAuthProvider.invalidateCache()is called by the existing 401 retry link, the next request refreshes withauth.refreshToken, single-flights concurrent callers, atomically updates onlyauth, preserves rotated refresh tokens, and skips writing if the on-disk auth changed during refresh.CLI-spawned host-service receives the CLI auth config path only for OAuth-backed sessions, while static token behavior remains unchanged when no config path is available.
AUTH_TOKENbehavior.Config writes use unique temp files and sanitized OAuth failures avoid exposing raw response bodies.
Tests cover the retained recovery path and support behavior.
Manual Verification
The CLI's own cloud commands (
workspaces list,auth whoami, etc.) do not exercise this PR — they hit cloud directly with the CLI's bearer and have no retry-on-401. The refresh path lives inside the host-service, so you have to drive a cloud-bound request from inside the running host. Either route below works.Route A — via the desktop app
superset start(or use the desktop's bundled host).~/.superset/config.json(or$SUPERSET_HOME_DIR/config.json): setauth.accessTokento"invalid.invalid.invalid". Leaveauth.refreshTokenintact.ctx.api.*(open/refresh a workspace, list workspaces, etc.).accessTokenis now a fresh JWT (no longerinvalid…);refreshTokenis still present (rotated value is fine).Route B — via curl, no desktop required
Expected:
[]).accessshows"invalid.invalid.invalid".accessno longer starts withinvalid;refreshstill present (will be the cloud-rotated value if rotation is enabled).Negative case (sanitized failure)
Repeat step 3 after also deleting
auth.refreshToken. Expect a clean error response (no raw OAuth body, no token/cookie payload leaked) and a "log in again" hint.Validation
Summary by CodeRabbit
New Features
Bug Fixes
Tests