Skip to content

Commit abe76c6

Browse files
authored
fix(openscience): harden atlasFetch signal, sync-version coercion, atomic temp, env prefixes (#123)
Four defensive fixes in the managed-account module: - atlasFetch now COMBINES a caller-supplied signal with the timeout via AbortSignal.any instead of replacing it, so passing a signal never drops the 60s hang guard. (#46) - refreshIfStale coerces a numeric-string sync version ({"v":"5"}) instead of collapsing it to null — otherwise cached_v never updates and the CLI keeps deferring the background sync forever. (#47) - atomicWrite's temp path is unique per call (pid + uuid), so two concurrent syncs in the same process can't write the same temp file, interleave, and publish a torn synced-env.json. (#13) - filterEnvForSubprocess matches SAFE_ENV_PREFIXES exactly for plain names and only prefixes entries ending in '_' (LC_, XDG_), so HOME no longer matches HOMEBREW_GITHUB_API_TOKEN etc. (#49)
1 parent a977b71 commit abe76c6

1 file changed

Lines changed: 22 additions & 4 deletions

File tree

backend/cli/src/openscience/index.ts

Lines changed: 22 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -307,7 +307,11 @@ export namespace OpenScience {
307307
// hang wedged whole sessions for >60 min. Overridable via OPENSCIENCE_ATLAS_TIMEOUT_MS.
308308
const ATLAS_FETCH_TIMEOUT_MS = Number(process.env["OPENSCIENCE_ATLAS_TIMEOUT_MS"]) || 60_000
309309
function atlasFetch(input: string, init: RequestInit = {}, timeoutMs = ATLAS_FETCH_TIMEOUT_MS): Promise<Response> {
310-
return fetch(input, { ...init, signal: init.signal ?? AbortSignal.timeout(timeoutMs) })
310+
// Combine (don't replace) a caller's signal with the timeout, so passing an
311+
// abort signal never silently drops the hang guard this function exists for.
312+
const timeout = AbortSignal.timeout(timeoutMs)
313+
const signal = init.signal ? AbortSignal.any([init.signal, timeout]) : timeout
314+
return fetch(input, { ...init, signal })
311315
}
312316

313317
export async function getSession(): Promise<OpenScienceSession | null> {
@@ -409,7 +413,13 @@ export namespace OpenScience {
409413
})
410414
if (!res.ok) return // fail open — keep current env
411415
const body = await res.json()
412-
v = typeof body?.v === "number" ? body.v : null
416+
// Coerce numeric strings too: a backend/proxy that returns {"v":"5"} would
417+
// otherwise leave v=null forever, so cached_v never updates and the CLI
418+
// keeps deferring the background sync for the TTL — dashboard changes never
419+
// land, with no error surfaced.
420+
const raw = body?.v
421+
const num = typeof raw === "number" ? raw : typeof raw === "string" ? Number(raw) : NaN
422+
v = Number.isFinite(num) ? num : null
413423
} catch {
414424
return // network failure → use current env, retry next message
415425
}
@@ -718,7 +728,11 @@ export namespace OpenScience {
718728
* a torn openscience-synced.json throws during config load and bricks the CLI
719729
* until it's removed by hand. */
720730
async function atomicWrite(filepath: string, content: string, options?: { mode?: number }): Promise<void> {
721-
const tmp = `${filepath}.${process.pid}.tmp`
731+
// Unique per call (not just per PID): two concurrent syncs in the SAME
732+
// process (e.g. a per-request /sync and the processor's background sync)
733+
// would otherwise write the identical temp path, interleave, and publish a
734+
// torn file or fail the rename.
735+
const tmp = `${filepath}.${process.pid}.${randomUUID()}.tmp`
722736
await Bun.write(tmp, content, options)
723737
await fs.rename(tmp, filepath)
724738
}
@@ -972,7 +986,11 @@ export namespace OpenScience {
972986
if (!value) continue
973987
if (isManagedAtlasKey(value)) continue
974988
if (SHARED_PROVIDER_KEYS.has(key)) continue
975-
const isSafe = SAFE_ENV_PREFIXES.some((p) => key === p || key.startsWith(p)) || SAFE_SYNCED_KEYS.has(key)
989+
// Entries ending in `_` (LC_, XDG_) are true prefixes; the rest are exact
990+
// names. Treating all as prefixes let HOME match HOMEBREW_GITHUB_API_TOKEN,
991+
// USER match USERPROFILE, etc. — over-broad passthrough.
992+
const isSafe =
993+
SAFE_ENV_PREFIXES.some((p) => (p.endsWith("_") ? key.startsWith(p) : key === p)) || SAFE_SYNCED_KEYS.has(key)
976994
if (isSafe || !syncedSecretValues.has(key)) {
977995
result[key] = value
978996
}

0 commit comments

Comments
 (0)