fix(desktop): create connection.json owner-only
`connection.json` under the desktop app's Electron `userData` was written with no
file mode, so it landed at the `0644` umask default — while its two
credential-bearing neighbours in the same directory, `desktop-installation.json`
and `native-oauth-tokens.json`, were already `0600`. That file holds the
safeStorage-encrypted gateway token plus the fields that are NOT encrypted: the
gateway URL and the SSH host, user, and key path.
- Route the single write choke point through a helper that creates the file
owner-only and atomically.
- Tighten an already-existing `0644` file once per launch on the read path, so
installs that already have one do not stay world-readable until the next save.
- Refuse to act on a path that is a symlink or not owned by the current user,
matching the guards `desktop-installation.ts` already applies to its sibling.
The symlink guard alone turned out to be insufficient, and that is worth
recording: `writeSecretFileAtomic` tightens its *temp* path, so a symlink planted
at `connection.json.tmp` meant `writeFileSync` followed it, the guard correctly
bailed, and `renameSync` then moved the link onto `connection.json` permanently.
Measured, guard-only vs. as-landed:
guards only token leaked: true config is a symlink: true 755
guards + temp unlink token leaked: false config is a symlink: false 600
So the temp path is unlinked before the write.
Issue #77486's headline claim — that a dashboard session token is persisted in
plaintext — does not hold against main. The token has been safeStorage-encrypted
since the desktop app reached mainline in 51c68d4ab, and `encryptDesktopSecret`
aborts with an actionable message rather than degrading to plaintext when
safeStorage is unavailable. The `{ encoding: 'plain', value }` literal does exist
at main.ts:7084, but only on the `persistToken: false` branch, whose sole caller
is the connection-test handler, which never writes. So no mainline path *writes*
a plaintext token. The commits that did contain a plaintext-writing fallback
(d3d177283, d208f2c2c) are not ancestors of main — they live only on
upstream/bb/gui-* and the desktop-pr20059-installers pre-release tag.
At-rest migration of legacy non-safeStorage payloads is deliberately NOT included.
An earlier revision of this branch implemented it and it was removed after review
reproduced two token-loss paths: it force-converts the opt-in plaintext choice
PR #62319 adds (silently reverting the user's decision, then destroying the token
on the next launch without the `--password-store=basic` flag), and it converts a
portable credential into a keychain-bound one with no consent — destroying the
only recoverable copy while not remediating the real exposure, since every
existing backup still holds the plaintext and the true remedy is rotation. It also
persisted raw `parsed`, bypassing `sanitizeConnectionProfiles`. A comment at the
read path records the three preconditions any future attempt needs.
`decryptDesktopSecret`'s non-safeStorage read fallback is untouched — it is what
lets a pre-release or hand-edited config work at all.
Windows still inherits the userData directory ACL rather than an explicit
owner-only one; mode bits are advisory there, so that half is deferred to
PR #77527 rather than growing a second ACL implementation here.
e2e: `at-rest-connection-token.spec.ts` asserts the at-rest contract
implementation-independently — the token's plaintext value (and its base64 form)
must not appear in a raw-bytes scan of any file under userData or HERMES_HOME,
AND the app must still put the exact original token on the wire after a restart,
so a fix that simply drops the token cannot pass. Proven non-vacuous by mutation:
writing `{ encoding: 'plain', value }` still fails the scan while the
file-exists and gateway-URL guards pass. The migration case is a documented
`test.fixme` naming its three blockers.
Electron project 928 -> 924 tests (-9 migration, +5 new guard and
mechanism-isolation). Two of those five exist because reverting either owner-only
mechanism alone initially scored zero failures — they were masking each other, so
either could have been deleted green.
(cherry picked from commit 6e01add657)
This commit is contained in:
张豪杰
Teknium
parent
7626105380
commit
7e151bd9d3
|
|
@ -0,0 +1,720 @@
|
|||
/**
|
||||
* E2E at-rest contract for the remote-gateway session token (issue #77486).
|
||||
*
|
||||
* The reported bug: configuring a remote gateway persisted the dashboard
|
||||
* session token as PLAINTEXT into `connection.json` under the app's userData
|
||||
* dir (macOS `~/Library/Application Support/Hermes/connection.json`, Windows
|
||||
* `AppData\Roaming\Hermes\connection.json`). Anything that can read the file
|
||||
* — a backup, a sync client, another local process, a support bundle — got a
|
||||
* live gateway credential.
|
||||
*
|
||||
* The contract these tests encode is deliberately stated WITHOUT naming a
|
||||
* storage strategy:
|
||||
*
|
||||
* 1. ABSENT FROM DISK. After the app has been configured with a remote
|
||||
* gateway token, the token's plaintext value must not appear anywhere in
|
||||
* `connection.json`, in any sibling file the app writes under userData,
|
||||
* or in HERMES_HOME (logs included).
|
||||
* 2. STILL FUNCTIONAL. After a restart, the app must still be able to USE
|
||||
* that credential — it decrypts the stored blob and puts the exact
|
||||
* original token on the wire.
|
||||
*
|
||||
* Both halves matter and neither is sufficient alone. (1) alone is trivially
|
||||
* satisfied by a "fix" that drops the token on the floor; (2) alone is
|
||||
* satisfied by the bug itself. So (2) is verified through the app's own
|
||||
* connection test against a fake gateway that records the
|
||||
* `X-Hermes-Session-Token` header it receives — a dropped or mangled token
|
||||
* cannot produce that header.
|
||||
*
|
||||
* We deliberately do NOT assert `encoding === 'safeStorage'` or any other
|
||||
* shape of the stored blob. That would be a change-detector: a fix that moved
|
||||
* to the OS keychain proper, to an async safeStorage provider, or to a
|
||||
* separate credential file would break the test while being *more* correct.
|
||||
* The load-bearing assertion is the raw-bytes absence of the secret.
|
||||
*
|
||||
* Two at-rest paths, hence two tests — one enforced, one a documented gap:
|
||||
*
|
||||
* 1. A NEWLY configured token (ACTIVE). The app's own write path routes
|
||||
* through the strict `encryptDesktopSecret`; this test holds it there
|
||||
* against regression.
|
||||
* 2. An EXISTING plaintext `connection.json` (`test.fixme`). Legacy payloads
|
||||
* are deliberately NOT migrated yet. The test is kept, disabled, with a
|
||||
* precise reason — see the block comment above it.
|
||||
*
|
||||
* ── Correcting the record on (2) ────────────────────────────────────────
|
||||
*
|
||||
* An earlier revision of this file asserted that migration and justified it by
|
||||
* claiming the first implementation (`d3d177283`) fell back to
|
||||
* `{ encoding: 'plain', value }` when `isEncryptionAvailable()` was false.
|
||||
* That citation is FALSE for this codebase. What is actually true:
|
||||
*
|
||||
* git merge-base --is-ancestor d3d1772837a7b0552940b55455ae734c72e0a8f1 HEAD -> 1 (NOT an ancestor)
|
||||
* git merge-base --is-ancestor 51c68d4ab1a9e3c62fb1048fccb84144c409f0e7 HEAD -> 0 (IS an ancestor)
|
||||
* git log -S 'Fall through to plaintext' upstream/main -- apps/desktop -> (no commits)
|
||||
*
|
||||
* `d3d177283` exists only on `upstream/bb/gui-mainmerge-tmp`,
|
||||
* `brooklyn/gui-installer-prereqs`, and the `desktop-pr20059-installers`
|
||||
* pre-release tag. Mainline NEVER shipped a code path that wrote a plaintext
|
||||
* gateway token: `51c68d4ab` ("Add Hermes desktop app (#20059)"), the commit
|
||||
* that brought the desktop app to mainline, already contained the strict
|
||||
* throw ("Secure token storage is unavailable, …") in `hardening.cjs`.
|
||||
*
|
||||
* One `{ encoding: 'plain', value }` literal does remain on mainline
|
||||
* (`electron/main.ts`, in `coerceDesktopConnectionConfig`), but it is
|
||||
* unreachable as an at-rest write: it is gated on `persistToken === false`,
|
||||
* whose only caller is the connection-TEST handler, which never calls
|
||||
* `writeDesktopConnectionConfig`. That token stays in memory for the duration
|
||||
* of one probe.
|
||||
*
|
||||
* So the affected population is not "anyone who configured a gateway before
|
||||
* the fix". It is narrow and non-mainline: pre-release `bb/gui` installs
|
||||
* (including the `desktop-pr20059-installers` build) plus hand-edited or
|
||||
* hand-migrated `connection.json` files. Those files DO still work, because
|
||||
* `decryptDesktopSecret` returns any non-safeStorage `value` verbatim on read
|
||||
* — the read path is intentionally unchanged, so nobody is signed out. That
|
||||
* read-path acceptance, not a mainline writer, is what makes the fixme'd
|
||||
* fixture realistic.
|
||||
*
|
||||
* Migration is DEFERRED, not forgotten. An adversarial review of the
|
||||
* migration that briefly lived here returned DO NOT SHIP, having reproduced
|
||||
* two token-loss scenarios: it silently reverts and then destroys the opt-in
|
||||
* plaintext choice that open upstream PR #62319 deliberately adds; and it
|
||||
* converts a portable credential into a keychain-bound one with no consent,
|
||||
* destroying the only recoverable copy while not actually remediating the
|
||||
* exposure (the plaintext is already in backups, so the real remedy is
|
||||
* ROTATION). The prerequisites are enumerated above test 2.
|
||||
*
|
||||
* Environment limits are encoded rather than papered over. Electron's
|
||||
* safeStorage is unavailable on Linux with no keyring, which is the shape of
|
||||
* this suite's CI runner (ubuntu-latest, see .github/workflows/e2e-desktop.yml).
|
||||
* The absence assertion is unconditional there — it is the security
|
||||
* requirement, and it must hold in every environment. Only the *other* half is
|
||||
* conditional: with secure storage the save must succeed, and without it the
|
||||
* save must fail loudly (which is what the current strict `encryptDesktopSecret`
|
||||
* does) instead of quietly writing plaintext. See the branch comments in each
|
||||
* test for the reasoning, including the one case this spec refuses to invent a
|
||||
* policy for.
|
||||
*
|
||||
* Prerequisite: `npm run build` must have been run so dist/ exists.
|
||||
*/
|
||||
|
||||
import * as fs from 'node:fs'
|
||||
import * as http from 'node:http'
|
||||
import type { AddressInfo } from 'node:net'
|
||||
import * as path from 'node:path'
|
||||
|
||||
import { buildAppEnv, createSandbox, launchDesktop, type Sandbox } from './fixtures'
|
||||
import { allowErrorBanners, type ElectronApplication, expect, type Page, test } from './test'
|
||||
|
||||
/**
|
||||
* The secret under test. Long, random-looking, and unique to this spec so a
|
||||
* raw-bytes scan cannot produce a false negative by colliding with ordinary
|
||||
* config content. Kept to `[A-Za-z0-9-]` on purpose: encodeURIComponent() is
|
||||
* the identity function over this alphabet, so the raw-bytes needle also
|
||||
* covers the URL-encoded form the WS dialer builds (`?token=…`).
|
||||
*/
|
||||
const SENTINEL_TOKEN = 'hermes-e2e-at-rest-sentinel-Zq7Z4hV9nX2pL8sK3tB6wR1yM5jD0fG'
|
||||
|
||||
/** Skip absurdly large files during the leak scan (Chromium caches). */
|
||||
const MAX_SCAN_BYTES = 16 * 1024 * 1024
|
||||
|
||||
/**
|
||||
* One fixed Electron app name for this spec, instead of the timestamped one
|
||||
* `buildAppEnv` generates. On macOS the safeStorage keychain item is derived
|
||||
* from the app name, so a per-launch name would (a) make the post-restart
|
||||
* decrypt fail for the wrong reason and (b) leave a fresh keychain entry on
|
||||
* the developer's login keychain on every run. Safe because the suite runs
|
||||
* one worker at a time and both launches here are sequential; the
|
||||
* single-instance lock keys off userData, which is per-sandbox.
|
||||
*/
|
||||
const STABLE_APP_NAME = 'HermesE2EAtRestStorage'
|
||||
|
||||
// ─── Fake gateway ───────────────────────────────────────────────────────
|
||||
|
||||
interface FakeGateway {
|
||||
url: string
|
||||
/** Every `X-Hermes-Session-Token` value the app has sent us. */
|
||||
sessionTokens: string[]
|
||||
close: () => Promise<void>
|
||||
}
|
||||
|
||||
/**
|
||||
* A minimal stand-in for a remote Hermes gateway. It serves the public
|
||||
* `/api/status` probe (which the desktop connection test hits first, with the
|
||||
* session token in a header) and refuses the WebSocket upgrade immediately so
|
||||
* the second leg of the connection test fails fast instead of burning the
|
||||
* probe's 10s connect timeout. We only care about the header it captured.
|
||||
*
|
||||
* The e2e mock-server is an OpenAI-compatible *inference* mock, not a gateway,
|
||||
* so it cannot answer /api/status — hence this small local server.
|
||||
*/
|
||||
async function startFakeGateway(): Promise<FakeGateway> {
|
||||
const sessionTokens: string[] = []
|
||||
|
||||
const server = http.createServer((req, res) => {
|
||||
const token = req.headers['x-hermes-session-token']
|
||||
|
||||
if (typeof token === 'string' && token) {
|
||||
sessionTokens.push(token)
|
||||
}
|
||||
|
||||
if (req.url?.startsWith('/api/status')) {
|
||||
res.writeHead(200, { 'Content-Type': 'application/json' })
|
||||
res.end(JSON.stringify({ auth_required: false, ok: true, version: '0.0.0-e2e-fake' }))
|
||||
|
||||
return
|
||||
}
|
||||
|
||||
res.writeHead(404, { 'Content-Type': 'application/json' })
|
||||
res.end(JSON.stringify({ detail: 'not found' }))
|
||||
})
|
||||
|
||||
// Refuse the WS leg at once: the connection test's WS probe should return a
|
||||
// fast failure rather than hang. The status header is already captured.
|
||||
server.on('upgrade', (req, socket) => {
|
||||
const token = new URL(req.url ?? '/', 'http://127.0.0.1').searchParams.get('token')
|
||||
|
||||
if (token) {
|
||||
sessionTokens.push(token)
|
||||
}
|
||||
|
||||
socket.destroy()
|
||||
})
|
||||
|
||||
await new Promise<void>(resolve => server.listen(0, '127.0.0.1', resolve))
|
||||
|
||||
const { port } = server.address() as AddressInfo
|
||||
|
||||
return {
|
||||
close: () =>
|
||||
new Promise<void>(resolve => {
|
||||
server.closeAllConnections?.()
|
||||
server.close(() => resolve())
|
||||
}),
|
||||
sessionTokens,
|
||||
url: `http://127.0.0.1:${port}`,
|
||||
}
|
||||
}
|
||||
|
||||
// ─── On-disk leak scanning ──────────────────────────────────────────────
|
||||
|
||||
interface Needle {
|
||||
bytes: Buffer
|
||||
label: string
|
||||
}
|
||||
|
||||
/**
|
||||
* The forms a leak could take. Raw bytes, not JSON.parse + field inspection:
|
||||
* the point is that the secret is nowhere in the file — including inside a
|
||||
* nested field, a cached WS URL, or a field name nobody thought to check.
|
||||
*
|
||||
* The base64 needle catches the cheapest wrong "fix": base64 is an encoding,
|
||||
* not encryption, so a token that is merely base64'd is still plaintext at
|
||||
* rest. A real ciphertext will contain neither needle.
|
||||
*/
|
||||
function secretNeedles(secret: string): Needle[] {
|
||||
return [
|
||||
{ bytes: Buffer.from(secret, 'utf8'), label: 'plaintext' },
|
||||
{ bytes: Buffer.from(Buffer.from(secret, 'utf8').toString('base64'), 'utf8'), label: 'base64' },
|
||||
]
|
||||
}
|
||||
|
||||
/** Relative paths of every file under `root` whose bytes contain a needle. */
|
||||
function scanTreeForSecret(root: string, needles: Needle[]): string[] {
|
||||
const hits: string[] = []
|
||||
|
||||
const walk = (dir: string): void => {
|
||||
let entries: fs.Dirent[]
|
||||
|
||||
try {
|
||||
entries = fs.readdirSync(dir, { withFileTypes: true })
|
||||
} catch {
|
||||
return
|
||||
}
|
||||
|
||||
for (const entry of entries) {
|
||||
const full = path.join(dir, entry.name)
|
||||
|
||||
if (entry.isDirectory()) {
|
||||
walk(full)
|
||||
|
||||
continue
|
||||
}
|
||||
|
||||
if (!entry.isFile()) {
|
||||
continue
|
||||
}
|
||||
|
||||
try {
|
||||
if (fs.statSync(full).size > MAX_SCAN_BYTES) {
|
||||
continue
|
||||
}
|
||||
} catch {
|
||||
continue
|
||||
}
|
||||
|
||||
let buf: Buffer
|
||||
|
||||
try {
|
||||
buf = fs.readFileSync(full)
|
||||
} catch {
|
||||
continue
|
||||
}
|
||||
|
||||
for (const needle of needles) {
|
||||
if (buf.includes(needle.bytes)) {
|
||||
hits.push(`${path.relative(root, full)} [${needle.label}]`)
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
walk(root)
|
||||
|
||||
return hits
|
||||
}
|
||||
|
||||
/**
|
||||
* Read a file's bytes, or an empty buffer when it does not exist. A correct
|
||||
* fix is allowed to delete/replace `connection.json` rather than rewrite it,
|
||||
* and the refusal path may never create it at all — neither should crash the
|
||||
* scan before its assertion runs.
|
||||
*/
|
||||
function readIfExists(filePath: string): Buffer {
|
||||
try {
|
||||
return fs.readFileSync(filePath)
|
||||
} catch {
|
||||
return Buffer.alloc(0)
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* The stored token's `encoding` tag, for diagnostics only — never its value.
|
||||
* Reported on failure so a red run says *why* (e.g. still `plain`) instead of
|
||||
* only that a scan matched. Deliberately NOT an assertion: which encoding a
|
||||
* correct fix chooses is its own business.
|
||||
*/
|
||||
function storedTokenEncoding(connectionFile: string): string {
|
||||
try {
|
||||
const parsed = JSON.parse(readIfExists(connectionFile).toString('utf8'))
|
||||
|
||||
return String(parsed?.remote?.token?.encoding ?? '<none>')
|
||||
} catch {
|
||||
return '<unparsable>'
|
||||
}
|
||||
}
|
||||
|
||||
// ─── App helpers ────────────────────────────────────────────────────────
|
||||
|
||||
/**
|
||||
* Launch the desktop app against `sandbox` with a fake boot failure injected.
|
||||
*
|
||||
* The credential path we are testing is entirely main-process (IPC handler →
|
||||
* coerce → safeStorage → userData write) and does not need a live agent
|
||||
* backend, so we skip spawning `hermes serve` (no Python needed, ~3s launch,
|
||||
* hermetic). This is also a real user situation rather than an artificial one:
|
||||
* the boot-failure overlay's own recovery affordance is "Connection settings",
|
||||
* i.e. pointing the app at a remote gateway is exactly what a user does from
|
||||
* this state. BOOT_FAKE_ERROR short-circuits startHermes() *before* remote
|
||||
* resolution, so no launch ever dials the fake gateway on its own.
|
||||
*/
|
||||
async function launchAgainst(sandbox: Sandbox): Promise<{ app: ElectronApplication; page: Page }> {
|
||||
const env = buildAppEnv(sandbox, {
|
||||
HERMES_DESKTOP_APP_NAME: STABLE_APP_NAME,
|
||||
HERMES_DESKTOP_BOOT_FAKE_ERROR: 'E2E at-rest storage spec: local backend intentionally not started',
|
||||
})
|
||||
|
||||
const { app, page } = await launchDesktop(env)
|
||||
|
||||
// The capability bridge is what we drive; it lands with the preload, well
|
||||
// before the app would be "ready" in the boot sense.
|
||||
await page.waitForFunction(
|
||||
() => Boolean((window as unknown as { hermesDesktop?: Record<string, unknown> }).hermesDesktop?.saveConnectionConfig),
|
||||
undefined,
|
||||
{ timeout: 60_000 },
|
||||
)
|
||||
|
||||
return { app, page }
|
||||
}
|
||||
|
||||
/**
|
||||
* Ask the running app where userData actually is, the same way the app does
|
||||
* (`app.getPath('userData')`). The fixtures point userData at a temp sandbox,
|
||||
* so a home-relative hardcoded path would test the wrong file — or no file.
|
||||
*/
|
||||
async function resolveUserDataDir(app: ElectronApplication): Promise<string> {
|
||||
return app.evaluate(({ app: electronApp }) => electronApp.getPath('userData'))
|
||||
}
|
||||
|
||||
interface SafeStorageCapability {
|
||||
available: boolean
|
||||
backend: string
|
||||
}
|
||||
|
||||
/**
|
||||
* What secure storage is actually capable of on THIS host, asked after ready
|
||||
* (on Linux the answer is meaningless before then).
|
||||
*
|
||||
* `backend` matters for the honest reading of a green run: on Linux with no
|
||||
* keyring, Electron can still report encryption as available while selecting
|
||||
* the `basic_text` backend, which encrypts with a hardcoded password — the
|
||||
* bytes on disk are not the plaintext, but they are not meaningfully
|
||||
* protected either. We record it rather than assert on it, because which
|
||||
* posture Hermes should take there (refuse to save vs. accept basic_text) is
|
||||
* a product decision, not something this test should silently ratify.
|
||||
*/
|
||||
async function readSafeStorageCapability(app: ElectronApplication): Promise<SafeStorageCapability> {
|
||||
return app.evaluate(async ({ app: electronApp, safeStorage }) => {
|
||||
await electronApp.whenReady()
|
||||
|
||||
let available = false
|
||||
let backend = 'unavailable'
|
||||
|
||||
try {
|
||||
available = safeStorage.isEncryptionAvailable()
|
||||
} catch {
|
||||
available = false
|
||||
}
|
||||
|
||||
try {
|
||||
// Linux-oriented API; other platforms may not implement it.
|
||||
backend = safeStorage.getSelectedStorageBackend?.() ?? 'n/a'
|
||||
} catch {
|
||||
backend = 'n/a'
|
||||
}
|
||||
|
||||
return { available, backend }
|
||||
})
|
||||
}
|
||||
|
||||
interface SaveOutcome {
|
||||
config: { remoteTokenPreview?: null | string; remoteTokenSet?: boolean; remoteUrl?: string } | null
|
||||
error: null | string
|
||||
}
|
||||
|
||||
/**
|
||||
* Drive the app's REAL save surface: the same `saveConnectionConfig` payload
|
||||
* Settings → Gateway sends (see src/app/settings/gateway-settings.tsx). We use
|
||||
* save rather than apply so the app persists the credential without trying to
|
||||
* re-home onto the fake gateway.
|
||||
*/
|
||||
async function saveRemoteToken(page: Page, remoteUrl: string, remoteToken?: string): Promise<SaveOutcome> {
|
||||
return page.evaluate(
|
||||
async ([url, token]) => {
|
||||
const desktop = (window as unknown as { hermesDesktop: any }).hermesDesktop
|
||||
|
||||
try {
|
||||
const config = await desktop.saveConnectionConfig({
|
||||
mode: 'remote',
|
||||
remoteAuthMode: 'token',
|
||||
...(token ? { remoteToken: token } : {}),
|
||||
remoteUrl: url,
|
||||
})
|
||||
|
||||
return { config, error: null }
|
||||
} catch (error) {
|
||||
return { config: null, error: error instanceof Error ? error.message : String(error) }
|
||||
}
|
||||
},
|
||||
[remoteUrl, remoteToken ?? ''] as const,
|
||||
)
|
||||
}
|
||||
|
||||
/**
|
||||
* Make the app USE the stored credential. No token in the payload, so the main
|
||||
* process must read `connection.json`, decrypt what it stored, and put the
|
||||
* plaintext on the wire itself. `buildRemoteBlock` throws "Remote gateway
|
||||
* session token is required." when the stored blob no longer decrypts, so a
|
||||
* fix that dropped the token fails here instead of quietly passing the
|
||||
* absence assertion.
|
||||
*/
|
||||
async function exerciseStoredToken(page: Page, remoteUrl: string): Promise<{ error: null | string }> {
|
||||
return page.evaluate(async url => {
|
||||
const desktop = (window as unknown as { hermesDesktop: any }).hermesDesktop
|
||||
|
||||
try {
|
||||
await desktop.testConnectionConfig({ mode: 'remote', remoteUrl: url })
|
||||
|
||||
return { error: null }
|
||||
} catch (error) {
|
||||
// A failing WS leg is expected (the fake gateway refuses the upgrade).
|
||||
// The assertion is on what the gateway received, not on this result.
|
||||
return { error: error instanceof Error ? error.message : String(error) }
|
||||
}
|
||||
}, remoteUrl)
|
||||
}
|
||||
|
||||
// ─── Tests ──────────────────────────────────────────────────────────────
|
||||
|
||||
let gateway: FakeGateway | null = null
|
||||
let sandbox: Sandbox | null = null
|
||||
let app: ElectronApplication | null = null
|
||||
|
||||
test.beforeAll(async () => {
|
||||
gateway = await startFakeGateway()
|
||||
})
|
||||
|
||||
test.afterAll(async () => {
|
||||
await gateway?.close()
|
||||
gateway = null
|
||||
})
|
||||
|
||||
test.beforeEach(() => {
|
||||
// Boot is intentionally failed in this spec (see launchAgainst), so the
|
||||
// boot-failure overlay's error banner is expected, not a failure.
|
||||
allowErrorBanners()
|
||||
})
|
||||
|
||||
test.afterEach(async () => {
|
||||
await app?.close().catch(() => undefined)
|
||||
app = null
|
||||
sandbox?.cleanup()
|
||||
sandbox = null
|
||||
})
|
||||
|
||||
test.describe('remote gateway session token at rest', () => {
|
||||
test('a newly configured token is never written to userData in plaintext, and still works after restart', async () => {
|
||||
const fake = gateway!
|
||||
sandbox = createSandbox('at-rest-fresh')
|
||||
|
||||
const first = await launchAgainst(sandbox)
|
||||
app = first.app
|
||||
|
||||
const capability = await readSafeStorageCapability(app)
|
||||
const userDataDir = await resolveUserDataDir(app)
|
||||
const connectionFile = path.join(userDataDir, 'connection.json')
|
||||
|
||||
test.info().annotations.push({
|
||||
description: `isEncryptionAvailable=${capability.available} backend=${capability.backend}`,
|
||||
type: 'safeStorage',
|
||||
})
|
||||
|
||||
const saved = await saveRemoteToken(first.page, fake.url, SENTINEL_TOKEN)
|
||||
|
||||
// Defined degradation, not a silent plaintext write. Where secure storage
|
||||
// works, the save must succeed. Where it genuinely does not (headless
|
||||
// Linux with no keyring, per Electron's safeStorage docs), refusing the
|
||||
// save with a loud error is an acceptable outcome — what is NEVER
|
||||
// acceptable is reporting success while leaving the secret readable on
|
||||
// disk. The absence assertion below runs in both branches.
|
||||
if (capability.available) {
|
||||
expect(
|
||||
saved.error,
|
||||
'secure storage is available on this host, so saving a remote gateway token must succeed',
|
||||
).toBeNull()
|
||||
expect(saved.config?.remoteTokenSet).toBe(true)
|
||||
} else {
|
||||
expect(
|
||||
saved.error,
|
||||
'secure storage is unavailable, so the save must fail loudly rather than persist a plaintext token',
|
||||
).not.toBeNull()
|
||||
}
|
||||
|
||||
// Guard against a vacuous pass: when the save succeeded, the artifact must
|
||||
// exist and must be the file the app really wrote for THIS connection.
|
||||
// Without this, "no plaintext on disk" would also be true if nothing had
|
||||
// been saved at all. Only asserted on the success branch — a refused save
|
||||
// legitimately leaves no file behind.
|
||||
const rawConnection = readIfExists(connectionFile)
|
||||
|
||||
if (capability.available) {
|
||||
expect(fs.existsSync(connectionFile), `expected the app to write ${connectionFile}`).toBe(true)
|
||||
expect(
|
||||
rawConnection.includes(Buffer.from(fake.url, 'utf8')),
|
||||
'connection.json should record the configured gateway URL (proves this is the real artifact)',
|
||||
).toBe(true)
|
||||
}
|
||||
|
||||
// ── The load-bearing assertion ─────────────────────────────────────
|
||||
const needles = secretNeedles(SENTINEL_TOKEN)
|
||||
|
||||
const connectionHits = needles.filter(needle => rawConnection.includes(needle.bytes)).map(needle => needle.label)
|
||||
expect(
|
||||
connectionHits,
|
||||
`the gateway session token must not be recoverable from ${connectionFile} ` +
|
||||
`(stored token encoding is "${storedTokenEncoding(connectionFile)}")`,
|
||||
).toEqual([])
|
||||
|
||||
// …and not in any sibling file the app writes alongside it, nor in
|
||||
// HERMES_HOME (desktop.log lives there).
|
||||
expect(
|
||||
scanTreeForSecret(userDataDir, needles),
|
||||
'the gateway session token leaked into a userData file',
|
||||
).toEqual([])
|
||||
expect(
|
||||
scanTreeForSecret(sandbox.hermesHome, needles),
|
||||
'the gateway session token leaked into a HERMES_HOME file (logs included)',
|
||||
).toEqual([])
|
||||
|
||||
if (!capability.available) {
|
||||
// Nothing was stored, so there is no round trip to verify. The refusal
|
||||
// itself was already asserted above.
|
||||
return
|
||||
}
|
||||
|
||||
// ── Secondary: the credential must still be USABLE ─────────────────
|
||||
// Restart against the same userData so the token comes off disk, not out
|
||||
// of a live process's memory.
|
||||
await app.close().catch(() => undefined)
|
||||
app = null
|
||||
|
||||
const second = await launchAgainst(sandbox)
|
||||
app = second.app
|
||||
|
||||
expect(
|
||||
await resolveUserDataDir(app),
|
||||
'the restarted app must resolve the same userData dir, or this is not a round trip',
|
||||
).toBe(userDataDir)
|
||||
|
||||
const reread = await second.page.evaluate(async () => {
|
||||
const desktop = (window as unknown as { hermesDesktop: any }).hermesDesktop
|
||||
|
||||
return desktop.getConnectionConfig()
|
||||
})
|
||||
|
||||
expect(reread.remoteTokenSet, 'the stored token must survive a restart').toBe(true)
|
||||
expect(reread.remoteUrl).toBe(fake.url)
|
||||
|
||||
const before = fake.sessionTokens.length
|
||||
await exerciseStoredToken(second.page, fake.url)
|
||||
|
||||
// The gateway is the witness: the app decrypted its stored blob and put
|
||||
// the original secret on the wire. A dropped, truncated, or re-encoded
|
||||
// token cannot produce this.
|
||||
expect(
|
||||
fake.sessionTokens.slice(before),
|
||||
'the app must send the exact stored token to the gateway after a restart',
|
||||
).toContain(SENTINEL_TOKEN)
|
||||
})
|
||||
|
||||
/**
|
||||
* DEFERRED GAP — legacy plaintext payloads are not migrated.
|
||||
*
|
||||
* Held as `fixme` rather than deleted: the fixture below is the correct
|
||||
* fixture for the population that a migration must eventually cover, and
|
||||
* the harness (seed → boot-poll → authoritative re-save → raw-bytes scan →
|
||||
* wire check) is the harness such a migration needs. Keeping it typechecked
|
||||
* and listed makes the gap visible in `--list` and in every report; deleting
|
||||
* it would make the gap invisible and cost the next implementer this setup.
|
||||
*
|
||||
* It is NOT enabled because the migration it asserted was reviewed
|
||||
* DO NOT SHIP. Before this can be un-fixme'd, three prerequisites (see the
|
||||
* header, and the matching note in electron/main.ts readDesktopConnectionConfig):
|
||||
*
|
||||
* 1. Sequence with #62319's opt-in plaintext marker, so a user who
|
||||
* deliberately chose plaintext is not silently overridden. This
|
||||
* fixture has NO marker, so it stays in scope for migration — but the
|
||||
* implementation must be able to tell the two apart.
|
||||
* 2. Write through the config sanitizer, not around it.
|
||||
* 3. Surface ROTATION guidance. Re-encrypting cannot un-expose a secret
|
||||
* that is already in a backup; it only prevents future exposure.
|
||||
*
|
||||
* Un-fixme'ing this without (1) risks destroying a deliberate user choice,
|
||||
* and without (3) it reports a remediation it did not actually perform.
|
||||
*/
|
||||
test('an existing plaintext connection.json is migrated off plaintext and keeps working', async () => {
|
||||
test.fixme(
|
||||
true,
|
||||
'Deferred: legacy plaintext connection.json is intentionally NOT migrated. ' +
|
||||
'Affected population is pre-release bb/gui installs (incl. the desktop-pr20059-installers build) ' +
|
||||
'plus hand-edited configs — mainline never wrote a plaintext gateway token. ' +
|
||||
'Blocked on: (1) #62319 opt-in-marker coordination, (2) writing through the config sanitizer, ' +
|
||||
'(3) surfacing token-rotation guidance. Re-encrypting alone does not remediate an already-backed-up secret.',
|
||||
)
|
||||
|
||||
const fake = gateway!
|
||||
sandbox = createSandbox('at-rest-migrate')
|
||||
|
||||
// Seed the file an affected user has on disk. This is live, usable
|
||||
// plaintext rather than a strawman, because `decryptDesktopSecret` returns
|
||||
// `value` verbatim for any non-safeStorage encoding — the READ path
|
||||
// accepts it. Note what does NOT justify this fixture: mainline never
|
||||
// WROTE this shape to disk. `coerceDesktopConnectionConfig` does build it,
|
||||
// but only under `persistToken: false`, whose sole caller is the
|
||||
// connection-test handler, which never persists. The writers were
|
||||
// non-mainline pre-release builds and hand edits. There is deliberately no
|
||||
// opt-in marker here, so this payload is in scope for a future migration.
|
||||
fs.writeFileSync(
|
||||
path.join(sandbox.userDataDir, 'connection.json'),
|
||||
JSON.stringify(
|
||||
{
|
||||
mode: 'remote',
|
||||
profiles: {},
|
||||
remote: {
|
||||
authMode: 'token',
|
||||
token: { encoding: 'plain', value: SENTINEL_TOKEN },
|
||||
url: fake.url,
|
||||
},
|
||||
},
|
||||
null,
|
||||
2,
|
||||
),
|
||||
'utf8',
|
||||
)
|
||||
|
||||
const launched = await launchAgainst(sandbox)
|
||||
app = launched.app
|
||||
|
||||
const capability = await readSafeStorageCapability(app)
|
||||
|
||||
test.info().annotations.push({
|
||||
description: `isEncryptionAvailable=${capability.available} backend=${capability.backend}`,
|
||||
type: 'safeStorage',
|
||||
})
|
||||
|
||||
if (!capability.available) {
|
||||
// With no secure storage there is nowhere to migrate the secret TO, and
|
||||
// scrubbing it would silently sign the user out of a working gateway.
|
||||
// Asserting either outcome here would be inventing policy.
|
||||
test.skip(
|
||||
true,
|
||||
'secure storage unavailable on this host — the correct migration policy for an existing plaintext file is undecided',
|
||||
)
|
||||
|
||||
return
|
||||
}
|
||||
|
||||
const userDataDir = await resolveUserDataDir(app)
|
||||
const connectionFile = path.join(userDataDir, 'connection.json')
|
||||
const needles = secretNeedles(SENTINEL_TOKEN)
|
||||
|
||||
// Two chances, so the test does not depend on WHERE the fix hooks the
|
||||
// migration: (a) on read at boot, (b) on the next authoritative write.
|
||||
// Poll for (a) first.
|
||||
const deadline = Date.now() + 15_000
|
||||
let stillPlaintext = true
|
||||
|
||||
while (Date.now() < deadline) {
|
||||
stillPlaintext = readIfExists(connectionFile).includes(needles[0].bytes)
|
||||
|
||||
if (!stillPlaintext) {
|
||||
break
|
||||
}
|
||||
|
||||
await launched.page.waitForTimeout(500)
|
||||
}
|
||||
|
||||
if (stillPlaintext) {
|
||||
// (b) A real save through the app's own surface, carrying no new token —
|
||||
// the stored blob is inherited. Re-persisting an inherited secret is the
|
||||
// other place plaintext must not survive.
|
||||
const resaved = await saveRemoteToken(launched.page, fake.url)
|
||||
expect(resaved.error, 'a re-save that inherits the stored token must not fail').toBeNull()
|
||||
}
|
||||
|
||||
expect(
|
||||
scanTreeForSecret(userDataDir, needles),
|
||||
'an existing plaintext gateway token must not remain readable under userData after the app has run ' +
|
||||
`(stored token encoding is still "${storedTokenEncoding(connectionFile)}")`,
|
||||
).toEqual([])
|
||||
|
||||
// And the migration must not have cost the user their credential.
|
||||
const before = fake.sessionTokens.length
|
||||
await exerciseStoredToken(launched.page, fake.url)
|
||||
|
||||
expect(
|
||||
fake.sessionTokens.slice(before),
|
||||
'the migrated token must still reach the gateway unchanged',
|
||||
).toContain(SENTINEL_TOKEN)
|
||||
})
|
||||
})
|
||||
|
|
@ -20,9 +20,54 @@ import {
|
|||
resolveReadableFileForIpc,
|
||||
resolveRequestedPathForIpc,
|
||||
resolveTimeoutMs,
|
||||
sensitiveFileBlockReason
|
||||
SAFE_STORAGE_ENCODING,
|
||||
SECRET_FILE_MODE,
|
||||
sensitiveFileBlockReason,
|
||||
tightenSecretFileMode,
|
||||
writeSecretFileAtomic
|
||||
} from './hardening'
|
||||
|
||||
/**
|
||||
* Real temp dir per test: the property under test IS the on-disk mode after a
|
||||
* temp-file-then-rename, which a mocked fs would assert into existence rather
|
||||
* than verify. `platform` is still injected so the Windows branch is coverable
|
||||
* from a POSIX run.
|
||||
*/
|
||||
function withTempDir(run: (dir: string) => void) {
|
||||
const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'hermes-secret-file-'))
|
||||
|
||||
try {
|
||||
run(dir)
|
||||
} finally {
|
||||
fs.rmSync(dir, { force: true, recursive: true })
|
||||
}
|
||||
}
|
||||
|
||||
function modeOf(filePath: string) {
|
||||
return fs.statSync(filePath).mode & 0o777
|
||||
}
|
||||
|
||||
/**
|
||||
* No file other than the target may survive a write, and nothing left in the
|
||||
* directory may contain the payload. Asserts the CONTRACT (no readable debris)
|
||||
* instead of a literal directory listing, so adding a lock file or renaming the
|
||||
* staging file does not break the test.
|
||||
*/
|
||||
function assertNoSecretDebris(dir: string, targetName: string, secret: string) {
|
||||
for (const name of fs.readdirSync(dir)) {
|
||||
if (name === targetName) {
|
||||
continue
|
||||
}
|
||||
|
||||
// Substring, not RegExp: a real token can contain regex metacharacters.
|
||||
assert.equal(
|
||||
fs.readFileSync(path.join(dir, name), 'utf8').includes(secret),
|
||||
false,
|
||||
`leftover file ${name} still contains the secret`
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
async function rejectsWithCode(promise, code: string) {
|
||||
await assert.rejects(promise, (error: any) => {
|
||||
assert.equal(error?.code, code)
|
||||
|
|
@ -99,9 +144,27 @@ test('encryptDesktopSecret stores safeStorage base64 payload', () => {
|
|||
encryptString: value => Buffer.from(`enc:${value}`, 'utf8')
|
||||
})
|
||||
|
||||
assert.deepEqual(secret, {
|
||||
encoding: 'safeStorage',
|
||||
value: Buffer.from('enc:token-123', 'utf8').toString('base64')
|
||||
// Contract: the payload is tagged with the SAME constant main's
|
||||
// decryptDesktopSecret dispatches on, and `value` is the keychain ciphertext
|
||||
// base64'd — not the token itself.
|
||||
assert.equal(secret?.encoding, SAFE_STORAGE_ENCODING)
|
||||
assert.equal(Buffer.from(String(secret?.value), 'base64').toString('utf8'), 'enc:token-123')
|
||||
assert.doesNotMatch(String(secret?.value), /token-123/, 'the plaintext is not recoverable from the payload')
|
||||
})
|
||||
|
||||
// ─── Owner-only credential files (connection.json) ─────────────────────────
|
||||
|
||||
test('writeSecretFileAtomic creates the file owner-only, not at the 0644 umask default', () => {
|
||||
withTempDir(dir => {
|
||||
const target = path.join(dir, 'connection.json')
|
||||
const payload = JSON.stringify({ remote: { token: { encoding: SAFE_STORAGE_ENCODING, value: 'BLOB' } } })
|
||||
|
||||
writeSecretFileAtomic(target, payload)
|
||||
|
||||
assert.equal(modeOf(target), SECRET_FILE_MODE)
|
||||
assert.equal(modeOf(target) & 0o077, 0, 'no group/other bits')
|
||||
assert.equal(fs.readFileSync(target, 'utf8'), payload, 'content round-trips')
|
||||
assertNoSecretDebris(dir, 'connection.json', 'BLOB')
|
||||
})
|
||||
})
|
||||
|
||||
|
|
@ -318,6 +381,285 @@ test('resolvePersistedRemoteToken keeps the existing token when no new token is
|
|||
assert.equal(called, false, 'an empty incoming token must not re-encrypt anything')
|
||||
})
|
||||
|
||||
test('writeSecretFileAtomic does not inherit loose bits from a stale temp file', () => {
|
||||
// renameSync keeps the TEMP file's permissions, and writeFileSync's `mode`
|
||||
// is ignored when the path already exists — so a temp left by a crashed
|
||||
// earlier write would otherwise hand 0644 straight to the target.
|
||||
withTempDir(dir => {
|
||||
const target = path.join(dir, 'connection.json')
|
||||
fs.writeFileSync(`${target}.tmp`, 'stale', { mode: 0o666 })
|
||||
assert.notEqual(modeOf(`${target}.tmp`), SECRET_FILE_MODE)
|
||||
|
||||
writeSecretFileAtomic(target, 'fresh')
|
||||
|
||||
assert.equal(modeOf(target), SECRET_FILE_MODE)
|
||||
assert.equal(fs.readFileSync(target, 'utf8'), 'fresh')
|
||||
})
|
||||
})
|
||||
|
||||
/**
|
||||
* Owner-only is carried by two independent mechanisms — the create-time `mode`
|
||||
* and the chmod before the rename — because each covers a case the other
|
||||
* cannot. The next two tests knock out one mechanism at a time (with the REAL
|
||||
* fs doing the actual write, so the assertion is still the on-disk mode) and
|
||||
* require the survivor to hold the line on its own. Without them, either
|
||||
* mechanism could be deleted with every test still green.
|
||||
*/
|
||||
function fsWith(overrides: Record<string, unknown>) {
|
||||
return { ...fs, ...overrides } as any
|
||||
}
|
||||
|
||||
test('the written file is owner-only even where chmod does nothing', () => {
|
||||
// Windows, and any mount that refuses chmod. The create-time `mode` is what
|
||||
// covers this — there is no second chance to tighten.
|
||||
withTempDir(dir => {
|
||||
const target = path.join(dir, 'connection.json')
|
||||
// Pin a permissive umask so a mode-less create WOULD land
|
||||
// group/other-readable — otherwise a restrictive-umask host could pass this
|
||||
// for free. Synchronous and restored in `finally`, and the electron project
|
||||
// runs one process per file, so no other test observes it.
|
||||
const previousUmask = process.umask(0o022)
|
||||
|
||||
try {
|
||||
const witness = path.join(dir, 'witness.json')
|
||||
fs.writeFileSync(witness, 'x')
|
||||
assert.notEqual(modeOf(witness), SECRET_FILE_MODE, 'the ambient default is NOT already owner-only')
|
||||
|
||||
writeSecretFileAtomic(target, 'tok', { fs: fsWith({ chmodSync: () => void 0 }) })
|
||||
} finally {
|
||||
process.umask(previousUmask)
|
||||
}
|
||||
|
||||
assert.equal(modeOf(target), SECRET_FILE_MODE, 'created owner-only, not tightened after the fact')
|
||||
})
|
||||
})
|
||||
|
||||
test('the written file is owner-only even when a stale temp cannot be removed', () => {
|
||||
// The unlink is best-effort; if the stale temp survives, writeFileSync's
|
||||
// `mode` is ignored on an existing path and only the chmod before the rename
|
||||
// can still fix the bits.
|
||||
withTempDir(dir => {
|
||||
const target = path.join(dir, 'connection.json')
|
||||
fs.writeFileSync(`${target}.tmp`, 'stale', { mode: 0o666 })
|
||||
|
||||
writeSecretFileAtomic(target, 'tok', { fs: fsWith({ rmSync: () => void 0 }) })
|
||||
|
||||
assert.equal(modeOf(target), SECRET_FILE_MODE, 'tightened before the rename handed the bits over')
|
||||
assert.equal(fs.readFileSync(target, 'utf8'), 'tok')
|
||||
})
|
||||
})
|
||||
|
||||
test('writeSecretFileAtomic cannot be redirected through a symlink planted at the temp path', () => {
|
||||
// A stale temp path is attacker-controllable in a shared temp/userData dir.
|
||||
// Following it would write the token into the victim file AND then rename the
|
||||
// link over connection.json, so every later write leaks too.
|
||||
withTempDir(dir => {
|
||||
const target = path.join(dir, 'connection.json')
|
||||
const victim = path.join(dir, 'victim.txt')
|
||||
fs.writeFileSync(victim, 'original', { mode: 0o644 })
|
||||
|
||||
try {
|
||||
fs.symlinkSync(victim, `${target}.tmp`, 'file')
|
||||
} catch (error: any) {
|
||||
if (error?.code === 'EPERM' || error?.code === 'EACCES') {
|
||||
return
|
||||
}
|
||||
|
||||
throw error
|
||||
}
|
||||
|
||||
writeSecretFileAtomic(target, 'tok-live-42')
|
||||
|
||||
assert.equal(fs.readFileSync(victim, 'utf8'), 'original', 'the symlink target was not written through')
|
||||
assert.equal(modeOf(victim), 0o644, 'the victim file was not chmodded either')
|
||||
assert.equal(fs.readFileSync(target, 'utf8'), 'tok-live-42')
|
||||
assert.equal(fs.lstatSync(target).isSymbolicLink(), false, 'the target is a real file, not the planted link')
|
||||
assert.equal(modeOf(target), SECRET_FILE_MODE)
|
||||
})
|
||||
})
|
||||
|
||||
test('tightenSecretFileMode tightens a pre-existing world-readable config in place', () => {
|
||||
// The upgrade path: a connection.json written by an older build sits at 0644
|
||||
// with a real (encrypted) token in it. Tightening must change the mode and
|
||||
// nothing else — the token has to stay readable or the user loses their
|
||||
// configured gateway.
|
||||
withTempDir(dir => {
|
||||
const target = path.join(dir, 'connection.json')
|
||||
|
||||
const legacy = JSON.stringify({
|
||||
mode: 'remote',
|
||||
remote: {
|
||||
url: 'https://gw.example.com',
|
||||
authMode: 'token',
|
||||
token: { encoding: SAFE_STORAGE_ENCODING, value: 'BLOB' }
|
||||
}
|
||||
})
|
||||
|
||||
fs.writeFileSync(target, legacy, { mode: 0o644 })
|
||||
assert.equal(modeOf(target), 0o644)
|
||||
|
||||
assert.equal(tightenSecretFileMode(target), true)
|
||||
|
||||
assert.equal(modeOf(target), SECRET_FILE_MODE)
|
||||
assert.deepEqual(JSON.parse(fs.readFileSync(target, 'utf8')), JSON.parse(legacy), 'contents untouched')
|
||||
})
|
||||
})
|
||||
|
||||
test('tightenSecretFileMode leaves a non-safeStorage token payload readable', () => {
|
||||
// A hand-edited config (or one from a pre-release build) can hold a
|
||||
// non-safeStorage token payload, which decryptDesktopSecret still reads
|
||||
// verbatim on purpose. Tightening the mode must not disturb that fallback —
|
||||
// it only narrows who can open the file.
|
||||
withTempDir(dir => {
|
||||
const target = path.join(dir, 'connection.json')
|
||||
|
||||
const legacyPlain = JSON.stringify({
|
||||
mode: 'remote',
|
||||
remote: { url: 'https://gw.example.com', authMode: 'token', token: { encoding: 'plain', value: 'tok-live-42' } }
|
||||
})
|
||||
|
||||
fs.writeFileSync(target, legacyPlain, { mode: 0o644 })
|
||||
|
||||
tightenSecretFileMode(target)
|
||||
|
||||
assert.equal(modeOf(target), SECRET_FILE_MODE)
|
||||
assert.equal(JSON.parse(fs.readFileSync(target, 'utf8')).remote.token.value, 'tok-live-42')
|
||||
})
|
||||
})
|
||||
|
||||
test('tightenSecretFileMode is idempotent and never throws on an unusable path', () => {
|
||||
withTempDir(dir => {
|
||||
const target = path.join(dir, 'connection.json')
|
||||
writeSecretFileAtomic(target, '{}')
|
||||
|
||||
assert.equal(tightenSecretFileMode(target), true)
|
||||
assert.equal(tightenSecretFileMode(target), true)
|
||||
assert.equal(modeOf(target), SECRET_FILE_MODE)
|
||||
|
||||
// Missing file (fresh install, nothing saved yet) reports failure quietly
|
||||
// instead of breaking the read path it is called from.
|
||||
assert.equal(tightenSecretFileMode(path.join(dir, 'absent.json')), false)
|
||||
})
|
||||
})
|
||||
|
||||
test('tightenSecretFileMode refuses to chmod a symlink instead of following it to its target', () => {
|
||||
// Matches readInstallationId in desktop-installation.ts. Without the lstat
|
||||
// guard a link planted at the config path sends the chmod to whatever it
|
||||
// resolves to — someone else's file gets its mode rewritten.
|
||||
withTempDir(dir => {
|
||||
const target = path.join(dir, 'connection.json')
|
||||
const victim = path.join(dir, 'victim.txt')
|
||||
fs.writeFileSync(victim, 'not mine', { mode: 0o644 })
|
||||
|
||||
try {
|
||||
fs.symlinkSync(victim, target, 'file')
|
||||
} catch (error: any) {
|
||||
if (error?.code === 'EPERM' || error?.code === 'EACCES') {
|
||||
return
|
||||
}
|
||||
|
||||
throw error
|
||||
}
|
||||
|
||||
assert.equal(tightenSecretFileMode(target), false, 'reports "not tightened" rather than acting on the link')
|
||||
assert.equal(modeOf(victim), 0o644, 'the symlink target keeps its own mode')
|
||||
})
|
||||
})
|
||||
|
||||
test('tightenSecretFileMode only touches a regular file the current user owns', () => {
|
||||
// Directories, sockets, fifos and files owned by another account are all
|
||||
// "not ours to chmod". Injected lstat so the foreign-owner branch is
|
||||
// reachable without a second OS account.
|
||||
const chmodded: string[] = []
|
||||
|
||||
const fakeFs = (stat: Record<string, unknown>) =>
|
||||
({
|
||||
chmodSync: (filePath: string) => void chmodded.push(filePath),
|
||||
lstatSync: () => ({ isFile: () => true, isSymbolicLink: () => false, mode: 0o644, uid: 0, ...stat }),
|
||||
renameSync: () => void 0,
|
||||
rmSync: () => void 0,
|
||||
writeFileSync: () => void 0
|
||||
}) as any
|
||||
|
||||
const uid = typeof process.getuid === 'function' ? process.getuid() : 0
|
||||
|
||||
assert.equal(
|
||||
tightenSecretFileMode('/x/connection.json', { fs: fakeFs({ isFile: () => false }), platform: 'linux' }),
|
||||
false
|
||||
)
|
||||
assert.equal(
|
||||
tightenSecretFileMode('/x/connection.json', { fs: fakeFs({ uid: uid + 1 }), platform: 'linux' }),
|
||||
false,
|
||||
'a file owned by another user is left alone'
|
||||
)
|
||||
assert.deepEqual(chmodded, [], 'nothing was chmodded on the rejected paths')
|
||||
|
||||
// The same fs shape, but ours and loose: now it tightens.
|
||||
assert.equal(tightenSecretFileMode('/x/connection.json', { fs: fakeFs({ uid }), platform: 'linux' }), true)
|
||||
assert.deepEqual(chmodded, ['/x/connection.json'])
|
||||
})
|
||||
|
||||
test('tightenSecretFileMode leaves Windows alone rather than flipping the read-only bit', () => {
|
||||
const chmods: string[] = []
|
||||
|
||||
const fakeFs = {
|
||||
chmodSync: (filePath: string) => void chmods.push(filePath),
|
||||
lstatSync: () => ({
|
||||
isFile: () => true,
|
||||
isSymbolicLink: () => false,
|
||||
mode: 0o644,
|
||||
uid: typeof process.getuid === 'function' ? process.getuid() : 0
|
||||
}),
|
||||
renameSync: () => void 0,
|
||||
rmSync: () => void 0,
|
||||
writeFileSync: () => void 0
|
||||
} as any
|
||||
|
||||
assert.equal(tightenSecretFileMode('C:\\Users\\me\\connection.json', { fs: fakeFs, platform: 'win32' }), true)
|
||||
assert.deepEqual(chmods, [], 'no chmod on win32')
|
||||
|
||||
// Same fs, POSIX: the chmod does happen, proving the platform gate is what
|
||||
// suppressed it above.
|
||||
assert.equal(tightenSecretFileMode('/home/me/connection.json', { fs: fakeFs, platform: 'linux' }), true)
|
||||
assert.ok(chmods.includes('/home/me/connection.json'), 'the POSIX path was tightened')
|
||||
})
|
||||
|
||||
test('a token is never persisted in plaintext when safeStorage is unavailable', () => {
|
||||
// The defined degradation for `isEncryptionAvailable() === false` (Linux with
|
||||
// no keyring): encryptDesktopSecret throws with an actionable message, so the
|
||||
// save aborts before any write. It must never fall back to a plaintext
|
||||
// payload — the file mode is defense in depth, not a substitute for the
|
||||
// keychain.
|
||||
const unavailable = {
|
||||
isEncryptionAvailable: () => false,
|
||||
encryptString: () => Buffer.from('unused', 'utf8')
|
||||
}
|
||||
|
||||
assert.throws(
|
||||
() => encryptDesktopSecret('tok-live-42', unavailable),
|
||||
(error: unknown) => {
|
||||
assert.ok(error instanceof Error, 'aborts instead of returning a payload')
|
||||
assert.match(String((error as Error).message), /Secure token storage is unavailable/)
|
||||
assert.doesNotMatch(String((error as Error).message), /tok-live-42/, 'the secret is not echoed in the error')
|
||||
|
||||
return true
|
||||
}
|
||||
)
|
||||
|
||||
// And a throwing keychain (available, but encryptString fails) is the same
|
||||
// contract — no silent plaintext.
|
||||
assert.throws(
|
||||
() =>
|
||||
encryptDesktopSecret('tok-live-42', {
|
||||
isEncryptionAvailable: () => true,
|
||||
encryptString: () => {
|
||||
throw new Error('keyring locked')
|
||||
}
|
||||
}),
|
||||
/Failed to encrypt the remote gateway token/
|
||||
)
|
||||
})
|
||||
|
||||
test('sensitiveFileBlockReason blocks obvious secret file patterns', () => {
|
||||
assert.match(String(sensitiveFileBlockReason('/tmp/.env')), /\.env/)
|
||||
assert.equal(sensitiveFileBlockReason('/tmp/.env.example'), null)
|
||||
|
|
|
|||
|
|
@ -35,6 +35,119 @@ function dataUrlReadMaxBytesFromMb(maxMb) {
|
|||
const SAFE_ENV_SUFFIXES = new Set(['dist', 'example', 'sample', 'template'])
|
||||
const SENSITIVE_EXTENSIONS = new Set(['.kdbx', '.p12', '.pem', '.pfx'])
|
||||
|
||||
// Owner-only mode for userData files that carry credentials (the encrypted
|
||||
// gateway token in connection.json, and the URL/SSH fields alongside it).
|
||||
// connection.json was the odd one out: its two credential-bearing neighbours
|
||||
// under userData are already 0600 — desktop-installation.json
|
||||
// (desktop-installation.ts) and native-oauth-tokens.json (main.ts
|
||||
// `_nativeTokenStoreIo`) — while connection.json was written with no mode at
|
||||
// all and landed at the 0644 umask default. This makes the three consistent.
|
||||
const SECRET_FILE_MODE = 0o600
|
||||
|
||||
// The encoding tag that marks a payload as OS-encrypted. One constant because
|
||||
// the writer (encryptDesktopSecret, here) and the reader
|
||||
// (decryptDesktopSecret, in main.ts) have to agree on the exact string across
|
||||
// a file boundary, and the native-token store round-trips the same shape.
|
||||
const SAFE_STORAGE_ENCODING = 'safeStorage'
|
||||
|
||||
interface SecretFileFs {
|
||||
chmodSync: typeof fs.chmodSync
|
||||
lstatSync: typeof fs.lstatSync
|
||||
renameSync: typeof fs.renameSync
|
||||
rmSync: typeof fs.rmSync
|
||||
writeFileSync: typeof fs.writeFileSync
|
||||
}
|
||||
|
||||
interface SecretFileOptions {
|
||||
encoding?: BufferEncoding
|
||||
fs?: SecretFileFs
|
||||
platform?: string
|
||||
}
|
||||
|
||||
/**
|
||||
* Tighten an existing credential file to owner-only (0600), returning whether
|
||||
* the file now has that mode.
|
||||
*
|
||||
* Exists because `fs.writeFileSync(path, data, { mode })` only applies `mode`
|
||||
* when it CREATES the file — rewriting an existing path silently keeps the old
|
||||
* bits. So a file already on disk at 0644 (every connection.json written
|
||||
* before this change, since that write passed no mode at all) needs an explicit
|
||||
* chmod; a fresh `mode:` alone would never tighten it.
|
||||
*
|
||||
* Guards match `readInstallationId` in desktop-installation.ts, which does the
|
||||
* same job for the sibling userData credential file: only ever chmod a regular
|
||||
* file we own, never a symlink and never another user's file. Without them a
|
||||
* symlink planted at the path would send the chmod to whatever it resolves to.
|
||||
*
|
||||
* POSIX only. Windows has no meaningful chmod (Node maps it to the read-only
|
||||
* bit), and userData there is already ACL'd to the user profile, so we report
|
||||
* success without touching the file rather than flipping it read-only and
|
||||
* breaking the next write.
|
||||
*
|
||||
* Never throws: a chmod can legitimately fail (read-only mount, file owned by
|
||||
* another user), and failing to tighten a file is not a reason to lose the
|
||||
* user's configured gateway.
|
||||
*/
|
||||
function tightenSecretFileMode(filePath, options: SecretFileOptions = {}) {
|
||||
const fsImpl = options.fs || fs
|
||||
const platform = options.platform || process.platform
|
||||
|
||||
if (platform === 'win32') {
|
||||
return true
|
||||
}
|
||||
|
||||
try {
|
||||
const stat = fsImpl.lstatSync(filePath)
|
||||
|
||||
if (!stat.isFile() || stat.isSymbolicLink()) {
|
||||
return false
|
||||
}
|
||||
|
||||
if (typeof process.getuid === 'function' && stat.uid !== process.getuid()) {
|
||||
return false
|
||||
}
|
||||
|
||||
if ((stat.mode & 0o777) === SECRET_FILE_MODE) {
|
||||
return true
|
||||
}
|
||||
|
||||
fsImpl.chmodSync(filePath, SECRET_FILE_MODE)
|
||||
|
||||
return true
|
||||
} catch {
|
||||
return false
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Atomically write a credential file, owner-only wherever the OS expresses
|
||||
* permissions as mode bits.
|
||||
*
|
||||
* On POSIX the file is owner-only from the moment it exists. On Windows this
|
||||
* only gets the atomic rename: `tightenSecretFileMode` no-ops there (Node maps
|
||||
* chmod to the read-only bit), so the file inherits the userData directory's
|
||||
* ACL rather than an explicit owner-only one. Tightening Windows ACLs is being
|
||||
* handled once, for the Python `_secure_file`, in PR #77527 — the desktop
|
||||
* should follow that rather than start a second ACL story here.
|
||||
*
|
||||
* The temp-then-rename dance is what makes the mode subtle: `renameSync` keeps
|
||||
* the TEMP file's permissions, so writing the temp at the default umask (0644)
|
||||
* hands those bits to the target — and a crashed earlier write can leave a
|
||||
* stale temp file whose existing loose bits `mode:` will not correct. Hence the
|
||||
* unlink of any stale temp (which also drops a planted symlink, so the write
|
||||
* cannot be redirected), the create-time `mode`, and the chmod before the
|
||||
* rename.
|
||||
*/
|
||||
function writeSecretFileAtomic(targetPath, data, options: SecretFileOptions = {}) {
|
||||
const fsImpl = options.fs || fs
|
||||
const tmp = targetPath + '.tmp'
|
||||
|
||||
fsImpl.rmSync(tmp, { force: true })
|
||||
fsImpl.writeFileSync(tmp, data, { encoding: options.encoding, mode: SECRET_FILE_MODE })
|
||||
tightenSecretFileMode(tmp, options)
|
||||
fsImpl.renameSync(tmp, targetPath)
|
||||
}
|
||||
|
||||
function resolveTimeoutMs(timeoutMs, fallbackMs = DEFAULT_FETCH_TIMEOUT_MS) {
|
||||
const fallback =
|
||||
Number.isFinite(fallbackMs) && Number(fallbackMs) > 0 ? Math.round(Number(fallbackMs)) : DEFAULT_FETCH_TIMEOUT_MS
|
||||
|
|
@ -86,7 +199,7 @@ function encryptDesktopSecret(value, safeStorageApi, options: { allowPlainText?:
|
|||
|
||||
try {
|
||||
return {
|
||||
encoding: 'safeStorage',
|
||||
encoding: SAFE_STORAGE_ENCODING,
|
||||
value: safeStorageApi.encryptString(raw).toString('base64')
|
||||
}
|
||||
} catch (error) {
|
||||
|
|
@ -440,6 +553,10 @@ export {
|
|||
resolveReadableFileForIpc,
|
||||
resolveRequestedPathForIpc,
|
||||
resolveTimeoutMs,
|
||||
SAFE_STORAGE_ENCODING,
|
||||
SECRET_FILE_MODE,
|
||||
sensitiveFileBlockReason,
|
||||
TEXT_PREVIEW_SOURCE_MAX_BYTES
|
||||
TEXT_PREVIEW_SOURCE_MAX_BYTES,
|
||||
tightenSecretFileMode,
|
||||
writeSecretFileAtomic
|
||||
}
|
||||
|
|
|
|||
|
|
@ -142,7 +142,10 @@ import {
|
|||
resolveReadableFileForIpc,
|
||||
resolveRequestedPathForIpc,
|
||||
resolveTimeoutMs,
|
||||
TEXT_PREVIEW_SOURCE_MAX_BYTES
|
||||
SAFE_STORAGE_ENCODING,
|
||||
TEXT_PREVIEW_SOURCE_MAX_BYTES,
|
||||
tightenSecretFileMode,
|
||||
writeSecretFileAtomic
|
||||
} from './hardening'
|
||||
import { cursorPointInWindow } from './hud-cursor'
|
||||
import { snapHudBounds } from './hud-snap'
|
||||
|
|
@ -6676,7 +6679,7 @@ function decryptDesktopSecret(secret) {
|
|||
return ''
|
||||
}
|
||||
|
||||
if (secret.encoding === 'safeStorage') {
|
||||
if (secret.encoding === SAFE_STORAGE_ENCODING) {
|
||||
try {
|
||||
return safeStorage.decryptString(Buffer.from(value, 'base64'))
|
||||
} catch {
|
||||
|
|
@ -6684,6 +6687,10 @@ function decryptDesktopSecret(secret) {
|
|||
}
|
||||
}
|
||||
|
||||
// Any other encoding (a hand-edited config, or one written by a pre-release
|
||||
// build) is returned verbatim on purpose: this fallback is what lets such a
|
||||
// config connect at all. Not a plaintext-writing path — nothing in this file
|
||||
// persists a token this way.
|
||||
return value
|
||||
}
|
||||
|
||||
|
|
@ -6789,6 +6796,22 @@ function readDesktopConnectionConfig() {
|
|||
const raw = fs.readFileSync(DESKTOP_CONNECTION_CONFIG_PATH, 'utf8')
|
||||
const parsed = JSON.parse(raw)
|
||||
|
||||
// Tighten an install written before this file was owner-only. Every write
|
||||
// now goes out at 0600, but a file already on disk keeps its old 0644 bits
|
||||
// until something chmods it, and waiting for the user's next Settings save
|
||||
// would leave it group/other-readable indefinitely. Runs on a cache miss
|
||||
// only (once per launch, plus after an external edit); chmod moves ctime,
|
||||
// not mtime, so it cannot invalidate the cache it sits inside.
|
||||
tightenSecretFileMode(DESKTOP_CONNECTION_CONFIG_PATH)
|
||||
|
||||
// NOT done here: migrating a legacy non-safeStorage token payload to
|
||||
// ciphertext at rest. Deferred deliberately — it has to honor the opt-in
|
||||
// plaintext choice PR #62319 adds (re-encrypting it converts a portable
|
||||
// credential into a keychain-bound one and can lose the token), write
|
||||
// through sanitizeConnectionProfiles below rather than persisting raw
|
||||
// `parsed`, and tell the user to ROTATE, since every existing backup copy
|
||||
// still holds the old secret. Do not add it without those three.
|
||||
|
||||
if (parsed && typeof parsed === 'object') {
|
||||
const remote = parsed.remote && typeof parsed.remote === 'object' ? parsed.remote : {}
|
||||
// authMode lives on the remote sub-object: 'oauth' (cookie + ws-ticket)
|
||||
|
|
@ -6816,7 +6839,14 @@ function readDesktopConnectionConfig() {
|
|||
|
||||
function writeDesktopConnectionConfig(config) {
|
||||
fs.mkdirSync(path.dirname(DESKTOP_CONNECTION_CONFIG_PATH), { recursive: true })
|
||||
writeFileAtomic(DESKTOP_CONNECTION_CONFIG_PATH, JSON.stringify(config, null, 2))
|
||||
// Owner-only, not writeFileAtomic: this is the single choke point for every
|
||||
// connection.json write (the IPC save/apply handlers and
|
||||
// persistSshConnectionToken all land here), and the file carries the
|
||||
// safeStorage-encrypted gateway token plus its URL and SSH host/user/keyPath.
|
||||
// safeStorage keeps the token opaque; 0600 keeps the whole record — and the
|
||||
// fields that are NOT encrypted — off other local accounts, matching
|
||||
// native-oauth-tokens.json and desktop-installation.json.
|
||||
writeSecretFileAtomic(DESKTOP_CONNECTION_CONFIG_PATH, JSON.stringify(config, null, 2))
|
||||
connectionConfigCache = config
|
||||
connectionConfigCacheMtime = fs.statSync(DESKTOP_CONNECTION_CONFIG_PATH).mtimeMs
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in New Issue