fix(relay-bench): vet the state file before truncating it and scrub peer error codes

- open without O_TRUNC, ftruncate only after the ownership/type checks pass
- getEndpoints error codes are allow-listed to a plain identifier before reaching the terminal
- README: expand the pairing-file path with $HOME, a tilde after '=' is literal
This commit is contained in:
Jinwoo-H
2026-09-07 14:48:54 -04:00
parent 81b825611a
commit 61373f3765
5 changed files with 57 additions and 6 deletions
+1 -1
View File
@@ -104,7 +104,7 @@ pbpaste | node $BENCH pair ~/.orca/relay-bench/state.json
# Or from a file you protect yourself, which `pair` requires to be mode 0600:
umask 077 && printf '%s' '<orca://pair?code=...>' > ~/.orca/relay-bench/pair.txt
node $BENCH pair ~/.orca/relay-bench/state.json --pairing-url-file=~/.orca/relay-bench/pair.txt
node $BENCH pair ~/.orca/relay-bench/state.json --pairing-url-file="$HOME/.orca/relay-bench/pair.txt"
rm ~/.orca/relay-bench/pair.txt
# Steady-state foreground reconnect, 10 times, 2 s apart, re-resolving the cell each time.
@@ -8,6 +8,7 @@ import {
fchmodSync,
constants,
fstatSync,
ftruncateSync,
lstatSync,
mkdirSync,
openSync,
@@ -43,7 +44,9 @@ export function writeSecretFile(path, contents) {
try {
fd = openSync(
path,
constants.O_WRONLY | constants.O_CREAT | constants.O_TRUNC | NOFOLLOW,
// No O_TRUNC: truncating happens only after the descriptor passes the checks below, so a
// refused file keeps its previous contents.
constants.O_WRONLY | constants.O_CREAT | NOFOLLOW,
SECRET_FILE_MODE
)
} catch (err) {
@@ -57,14 +60,15 @@ export function writeSecretFile(path, contents) {
if (!stats.isFile()) {
throw new Error(`refusing to write ${path}: not a regular file`)
}
// Before the write, not after: a pre-existing file owned by someone else would take the
// token on O_TRUNC and only then fail the chmod, leaving it readable by its owner.
// Before the truncate and write, not after: a pre-existing file owned by someone else would
// otherwise lose its contents and then fail the chmod, leaving the token readable by its owner.
if (process.platform !== 'win32' && stats.uid !== process.getuid()) {
throw new Error(`refusing to write ${path}: owned by another user`)
}
if (process.platform !== 'win32' && (stats.mode & GROUP_AND_OTHER_BITS) !== 0) {
fchmodSync(fd, SECRET_FILE_MODE)
}
ftruncateSync(fd, 0)
writeFileSync(fd, contents)
} finally {
closeSync(fd)
@@ -67,6 +67,20 @@ describe('writeSecretFile', () => {
expect(modeOf(join(dir, 'nested'))).toBe(0o700)
})
it.runIf(posix)('leaves a refused file untouched instead of truncating it first', () => {
// A writable file owned by someone else must be refused before its contents are destroyed.
const path = join(dir, 'state.json')
writeFileSync(path, 'previous contents')
const realGetuid = process.getuid
process.getuid = () => realGetuid() + 1
try {
expect(() => writeSecretFile(path, 'secret')).toThrow(/owned by another user/)
} finally {
process.getuid = realGetuid
}
expect(readFileSync(path, 'utf8')).toBe('previous contents')
})
it('truncates rather than appending to a longer previous file', () => {
const path = join(dir, 'state.json')
writeSecretFile(path, '{"a":"aaaaaaaaaaaaaaaaaaaa"}')
@@ -375,6 +375,15 @@ async function loadState(statePath) {
}
// ---------- commands ----------
// The peer picks the code, so only a plain identifier is echoed; anything else is named by kind.
const REMOTE_ERROR_CODE = /^[A-Za-z0-9_.-]{1,64}$/
export function describeRemoteErrorCode(code) {
if (typeof code !== 'string') {
return code === undefined ? 'unknown' : `non-string code (${typeof code})`
}
return REMOTE_ERROR_CODE.test(code) ? code : `unprintable code (${code.length} chars)`
}
async function pair(pairingUrl, statePath) {
const offer = decodeOffer(pairingUrl)
if (!offer.relay) {
@@ -413,7 +422,7 @@ async function pair(pairingUrl, statePath) {
if (!endpoints.ok || !endpoints.result.relay) {
// Shape only: the reply is peer-supplied and this line lands in the operator's terminal.
throw new Error(
`getEndpoints failed: ${endpoints.ok ? 'no relay block in result' : `error ${endpoints.error?.code ?? 'unknown'}`}`
`getEndpoints failed: ${endpoints.ok ? 'no relay block in result' : `error ${describeRemoteErrorCode(endpoints.error?.code)}`}`
)
}
console.log(`provisionRelay ${provisionMs} ms, getEndpoints ${endpointsMs} ms`)
@@ -2,7 +2,12 @@
// missing or malformed pairing link surfaced as a stack trace rather than usage. The link is a
// live credential, so the failure text has to name the problem without echoing the code.
import { describe, expect, it, vi } from 'vitest'
import { decodeOffer, vetCellUrl, vetRelayEndpoint } from './relay-phone-connect-bench.mjs'
import {
decodeOffer,
describeRemoteErrorCode,
vetCellUrl,
vetRelayEndpoint
} from './relay-phone-connect-bench.mjs'
const encode = (offer) => Buffer.from(JSON.stringify(offer), 'utf8').toString('base64url')
@@ -86,3 +91,22 @@ describe('vetRelayEndpoint', () => {
).resolves.toEqual({ ok: true })
})
})
describe('describeRemoteErrorCode', () => {
it('echoes a plain protocol identifier', () => {
expect(describeRemoteErrorCode('invalid_install_req')).toBe('invalid_install_req')
})
it('never echoes terminal control bytes the peer put in the code', () => {
const out = describeRemoteErrorCode('\u001b[2J\u001b]0;pwned\u0007')
expect(out).not.toContain('\u001b')
expect(out).toMatch(/unprintable code/)
})
it('names the type instead of stringifying a non-string code', () => {
expect(describeRemoteErrorCode({ toString: () => '\u001b[31m' })).toBe(
'non-string code (object)'
)
expect(describeRemoteErrorCode(undefined)).toBe('unknown')
})
})