Add session management to account-cli#260
Conversation
Introduce session lifecycle (create, list, resolve, destroy) for three modes: operator, smart-wallet, and external-eoa. Sessions are stored as JSON files under ~/.base-account/sessions/ with owner-only permissions (0o700/0o600) and secure deletion (random-byte overwrite before unlink). Key additions: - Session types with CAIP-2 chain identifiers and schema versioning - Deterministic session resolution via BASE_SESSION env var or auto-select - CLI commands: `session list`, `session info`, `session destroy [--all]` - Audit logging (JSONL) for all session create/destroy operations - Path traversal protection on session file paths - CAIP-2 parsing/validation utilities with known chain aliases - Comprehensive unit tests (615 lines) and integration tests (212 lines) Made-with: Cursor
✅ Heimdall Review Status
|
Made-with: Cursor
Made-with: Cursor
Made-with: Cursor
There was a problem hiding this comment.
should this not be index.test.ts?
Made-with: Cursor
spencerstock
left a comment
spencerstock
left a comment
There was a problem hiding this comment.
🔴 Changes requested · 52/100
This PR builds a solid session management foundation with good architecture — atomic writes, path traversal protection, audit logging, and comprehensive tests. However, there are two critical issues that must be fixed before merging:
-
Secure deletion is broken:
openSync(filePath, 'w')truncates the file before overwriting, so sensitive data blocks are released and never actually overwritten. Change to'r+'flag. -
Windows is completely broken: Strict POSIX permission checks (
0o700/0o600equality) will always fail on Windows, making the CLI unusable on that platform. Add platform guards.
Additionally, there are several important suggestions: destroyAllSessions should not depend on listSessions() (which can throw on corrupt files), address comparisons should be case-insensitive, getBaseDir() has a subtle env var precedence bug that could break tests in CI, and the path traversal guard also uses Unix-specific path separators.
Confidence: 52/100 · 🔴 2 critical · 🟡 4 suggestions
Pass 1 → Pass 2 full details
Pass 1 found 4 comments:
✅ packages/account-cli/src/utils/secure-delete.ts:12 — critical
Using the
'w'flag truncates the file to 0 bytes before you write to it.On modern filesystems, truncating a file releases its allocated data blocks back to the free space pool. When you subsequently write
randomBytes(size), the filesystem allocates new blocks for this data. The original blocks containing the sensitive session data are left untouched and can be recovered using disk forensics tools.To overwrite the existing data blocks in-place, open the file for reading and writing without truncation using the
'r+'flag:const fd = openSync(filePath, 'r+');Pass 2: Confirmed by reading the source.
openSync(filePath, 'w')truncates the file to 0 bytes (Node.js docs: flag 'w' opens for writing, truncating the file if it exists). The subsequentwriteSync(fd, randomBytes(size))writes to newly allocated blocks, not the original ones containing sensitive data. Using'r+'(open for reading and writing without truncation) is the correct fix. This completely defeats the stated security goal of preventing disk forensic recovery.
✅ packages/account-cli/src/utils/permissions.ts:11 — critical
These strict POSIX permission checks will completely break the CLI on Windows.
Node.js on Windows does not fully map POSIX permissions. A directory created with
mkdirSyncor a file created withwriteFileSyncwill typically reflect a mode like0o666or0o777when read viastatSync.mode. Consequently,mode !== 0o700andmode !== 0o600will almost always evaluate to true, and Windows users will be permanently blocked byINSECURE_PERMISSIONSerrors.Bypass these checks entirely on Windows to ensure cross-platform compatibility:
// Add to the top of both functions if (process.platform === 'win32') return;Pass 2: Confirmed. Node.js on Windows does not properly support POSIX file mode bits.
statSync().mode & 0o777will return values like 0o666 or 0o777 regardless of actual Windows ACLs. The strict equality checks (mode !== 0o700andmode !== 0o600) will always fail on Windows, making the CLI completely unusable there. The PR description doesn't mention any platform restrictions, and the package.json doesn't indicate it's POSIX-only. Aprocess.platform === 'win32'guard is the appropriate fix.
✅ packages/account-cli/src/core/session/index.ts:109 — suggestion
destroyAllSessionsrelies onlistSessions(), which throws if any session file has invalid JSON or incorrect permissions.This means if a single session file is corrupted or accidentally
chmoded by the user,session destroy --allcompletely fails, breaking the user's escape hatch to reset their state.Instead of parsing the files, just read the directory and securely delete any matching filenames directly:
export function destroyAllSessions(): string[] { const dir = sessionsDir(); if (!existsSync(dir)) return []; // ... verify dir perms ... const labels: string[] = []; for (const f of readdirSync(dir)) { const match = SESSION_FILE_RE.exec(f); if (!match) continue; const mode = match[1] as SessionMode; const key = match[2]; secureDelete(join(dir, f)); appendAuditLog('session_destroy_all', mode, key); labels.push(`${mode}:${key}`); } return labels; }Pass 2: Confirmed by reading the code.
destroyAllSessions()at line 113 callslistSessions(), which throwsCORRUPT_SESSIONon invalid JSON (line 44-45) andINSECURE_PERMISSIONSon wrong file permissions (line 40). If a user accidentallychmods a session file or a file gets corrupted,destroy --allbecomes unusable — the very command that should help reset broken state. The suggested fix of usingreaddirSync+ regex matching + directsecureDeleteis sound and removes the fragile dependency.
✅ packages/account-cli/src/commands/session/index.ts:95 — suggestion
Ethereum addresses are often mixed-case (checksummed) or lowercase. Comparing them with strictly case-sensitive
===creates brittle UX, requiring the user to type the exact casing stored in the file.If a user tries to destroy a session using a lowercase address but the session was saved with a checksummed address, it will throw an
INVALID_INPUTerror.Convert both sides to lowercase for a robust comparison:
const target = identifier.toLowerCase(); const match = sessions.find((s) => sessionKey(s).toLowerCase() === target);(This same suggestion applies to
process.env.BASE_SESSIONresolution inresolveSession).Pass 2: Confirmed. Ethereum addresses are often in EIP-55 mixed-case checksum format or all lowercase. At line 96,
sessionKey(s) === identifieruses strict equality, so0xabc123won't match0xAbc123. The same issue exists inresolveSession()at line 56 wheresessionKey(s) === envValueis also case-sensitive. This is a real UX problem since Ethereum addresses are case-insensitive by nature. The fix of.toLowerCase()on both sides is correct and should be applied in both locations.
Pass 2 added 2 new findings:
🆕 packages/account-cli/src/core/paths.ts:4 — suggestion
baseDiris initialized once at module load fromprocess.env.BASE_ACCOUNT_DIR, butgetBaseDir()also readsprocess.env.BASE_ACCOUNT_DIRat call time. This creates a subtle inconsistency: if the env var is set at startup,baseDircaptures it. IfsetBaseDir()is later called (e.g., in tests),getBaseDir()will still return the env var value instead of the value set bysetBaseDir(), because the env var check takes precedence.In the integration tests this works because
BASE_ACCOUNT_DIRis passed to the child process. But in unit tests,setBaseDir()works only becauseprocess.env.BASE_ACCOUNT_DIRis not set in the test process. If it were ever set (e.g., in CI), all unit tests would break becausesetBaseDir()would be silently ignored.Consider making
getBaseDir()simply returnbaseDirand only read the env var during initialization:export function getBaseDir(): string { return baseDir; }And update the initialization to:
let baseDir = process.env.BASE_ACCOUNT_DIR || join(homedir(), '.base-account');(which is already the case — just remove the env var check from
getBaseDir()).
🆕 packages/account-cli/src/core/paths.ts:31 — suggestion
The path traversal guard
!filePath.startsWith(${dir}/)can be bypassed on Windows where the path separator is\not/.resolve()on Windows will produce paths likeC:\Users\...\sessions\operator-foo.json, which will never start with${dir}/(with forward slash). This means the guard would reject all identifiers on Windows — or if you fix it to usepath.sep, an attacker could use forward slashes in the identifier to escape.Consider using
path.sepor normalizing both paths:const normalizedDir = resolve(dir) + path.sep; if (!filePath.startsWith(normalizedDir)) { throw new Error('Invalid session identifier: path traversal detected'); }
🔧 Fix with prompt
A reviewer gave these comments as feedback. Validate them and fix all the ones that need to be fixed.
- [critical] packages/account-cli/src/utils/secure-delete.ts:12-15
**Critical: `'w'` flag truncates the file, defeating secure deletion.**
Using `openSync(filePath, 'w')` truncates the file to 0 bytes before writing. The OS releases the original data blocks back to the free pool, and `randomBytes(size)` is written to *newly allocated* blocks. The original sensitive data remains on disk and is recoverable with forensic tools.
To overwrite the existing data blocks in-place, use the `'r+'` flag:
```ts
const fd = openSync(filePath, 'r+');
```
- [critical] packages/account-cli/src/utils/permissions.ts:11-32
**Critical: Strict POSIX permission checks will break the CLI on Windows.**
Node.js on Windows does not map POSIX permissions correctly. `statSync().mode & 0o777` will return values like `0o666` or `0o777` regardless of the actual Windows ACLs. The strict equality checks (`mode !== 0o700` / `mode !== 0o600`) will always fail, permanently blocking Windows users with `INSECURE_PERMISSIONS` errors.
Bypass these checks on Windows:
```ts
if (process.platform === 'win32') return;
```
- [suggestion] packages/account-cli/src/core/session/index.ts:109-114
`destroyAllSessions` depends on `listSessions()`, which throws on corrupted files or incorrect permissions. This means a single broken session file prevents `destroy --all` from working — defeating the user's escape hatch to reset state.
Instead, read the directory directly and delete matching filenames without parsing:
```ts
export function destroyAllSessions(): string[] {
const dir = sessionsDir();
if (!existsSync(dir)) return [];
verifyDirectoryPermissions(dir);
const labels: string[] = [];
for (const f of readdirSync(dir)) {
const match = SESSION_FILE_RE.exec(f);
if (!match) continue;
secureDelete(join(dir, f));
appendAuditLog('session_destroy_all', match[1] as SessionMode, match[2]);
labels.push(`${match[1]}:${match[2]}`);
}
return labels;
}
```
- [suggestion] packages/account-cli/src/commands/session/index.ts:95-99
Ethereum addresses are case-insensitive, but this comparison uses strict `===`. A user typing `0xeoa444` when the stored session has `0xEoa444` will get an `INVALID_INPUT` error.
Use case-insensitive comparison:
```ts
const target = identifier.toLowerCase();
const match = sessions.find((s) => sessionKey(s).toLowerCase() === target);
```
The same issue exists in `resolveSession()` at line 56 for `BASE_SESSION` env var matching.
- [suggestion] packages/account-cli/src/core/paths.ts:4-8
`getBaseDir()` re-checks `process.env.BASE_ACCOUNT_DIR` on every call, which silently overrides any value set by `setBaseDir()`. In unit tests this works only because the env var isn't set in the test process. If CI or a developer happens to set `BASE_ACCOUNT_DIR`, `setBaseDir()` would be silently ignored and tests would write to the wrong directory.
Consider having `getBaseDir()` simply return `baseDir` without the env var fallback, since the env var is already read during module initialization:
```ts
export function getBaseDir(): string {
return baseDir;
}
```
- [suggestion] packages/account-cli/src/core/paths.ts:31-37
The path traversal guard uses a hardcoded forward slash (`${dir}/`), which won't work on Windows where `resolve()` produces backslash-separated paths. This means the guard would reject *all* identifiers on Windows.
Use `path.sep` for cross-platform correctness:
```ts
import { sep } from 'node:path';
const normalizedDir = resolve(dir) + sep;
if (!filePath.startsWith(normalizedDir)) {
throw new Error('Invalid session identifier: path traversal detected');
}
```
SHA 4f8657f9 · gemini-3.1-pro-preview → claude-opus-4-6
| if (size > 0) { | ||
| const fd = openSync(filePath, 'w'); | ||
| writeSync(fd, randomBytes(size)); | ||
| closeSync(fd); |
There was a problem hiding this comment.
critical
Critical: 'w' flag truncates the file, defeating secure deletion.
Using openSync(filePath, 'w') truncates the file to 0 bytes before writing. The OS releases the original data blocks back to the free pool, and randomBytes(size) is written to newly allocated blocks. The original sensitive data remains on disk and is recoverable with forensic tools.
To overwrite the existing data blocks in-place, use the 'r+' flag:
const fd = openSync(filePath, 'r+');| export function verifyDirectoryPermissions(dir: string): void { | ||
| if (!existsSync(dir)) return; | ||
| const stats = statSync(dir); | ||
| const mode = stats.mode & 0o777; | ||
| if (mode !== REQUIRED_DIR_MODE) { | ||
| throw new CLIError( | ||
| 'INSECURE_PERMISSIONS', | ||
| `Directory ${dir} has mode ${mode.toString(8).padStart(3, '0')}, expected 700. ` + | ||
| `Fix with: chmod 700 "${dir}"` | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Verify that a file has owner-only read/write permissions (0o600). | ||
| * Throws if the file is group- or world-readable. | ||
| */ | ||
| export function verifyFilePermissions(filePath: string): void { | ||
| if (!existsSync(filePath)) return; | ||
| const stats = statSync(filePath); | ||
| const mode = stats.mode & 0o777; | ||
| if (mode !== REQUIRED_FILE_MODE) { |
There was a problem hiding this comment.
critical
Critical: Strict POSIX permission checks will break the CLI on Windows.
Node.js on Windows does not map POSIX permissions correctly. statSync().mode & 0o777 will return values like 0o666 or 0o777 regardless of the actual Windows ACLs. The strict equality checks (mode !== 0o700 / mode !== 0o600) will always fail, permanently blocking Windows users with INSECURE_PERMISSIONS errors.
Bypass these checks on Windows:
if (process.platform === 'win32') return;| appendAuditLog('session_destroy', mode, identifier); | ||
| } | ||
|
|
||
| export function destroyAllSessions(): string[] { | ||
| const sessions = listSessions(); | ||
| const labels: string[] = []; |
There was a problem hiding this comment.
destroyAllSessions depends on listSessions(), which throws on corrupted files or incorrect permissions. This means a single broken session file prevents destroy --all from working — defeating the user's escape hatch to reset state.
Instead, read the directory directly and delete matching filenames without parsing:
export function destroyAllSessions(): string[] {
const dir = sessionsDir();
if (!existsSync(dir)) return [];
verifyDirectoryPermissions(dir);
const labels: string[] = [];
for (const f of readdirSync(dir)) {
const match = SESSION_FILE_RE.exec(f);
if (!match) continue;
secureDelete(join(dir, f));
appendAuditLog('session_destroy_all', match[1] as SessionMode, match[2]);
labels.push(`${match[1]}:${match[2]}`);
}
return labels;
}| const sessions = listSessions(); | ||
| const match = sessions.find((s) => sessionKey(s) === identifier); | ||
| if (!match) { | ||
| throw new CLIError('INVALID_INPUT', `No session found for ${identifier}`); | ||
| } |
There was a problem hiding this comment.
Ethereum addresses are case-insensitive, but this comparison uses strict ===. A user typing 0xeoa444 when the stored session has 0xEoa444 will get an INVALID_INPUT error.
Use case-insensitive comparison:
const target = identifier.toLowerCase();
const match = sessions.find((s) => sessionKey(s).toLowerCase() === target);The same issue exists in resolveSession() at line 56 for BASE_SESSION env var matching.
| let baseDir = process.env.BASE_ACCOUNT_DIR || join(homedir(), '.base-account'); | ||
|
|
||
| export function getBaseDir(): string { | ||
| return process.env.BASE_ACCOUNT_DIR || baseDir; | ||
| } |
There was a problem hiding this comment.
getBaseDir() re-checks process.env.BASE_ACCOUNT_DIR on every call, which silently overrides any value set by setBaseDir(). In unit tests this works only because the env var isn't set in the test process. If CI or a developer happens to set BASE_ACCOUNT_DIR, setBaseDir() would be silently ignored and tests would write to the wrong directory.
Consider having getBaseDir() simply return baseDir without the env var fallback, since the env var is already read during module initialization:
export function getBaseDir(): string {
return baseDir;
}| export function sessionFile(mode: string, identifier: string): string { | ||
| const dir = sessionsDir(); | ||
| const filePath = resolve(dir, `${mode}-${identifier}.json`); | ||
| if (!filePath.startsWith(`${dir}/`) && filePath !== dir) { | ||
| throw new Error(`Invalid session identifier: path traversal detected`); | ||
| } | ||
| return filePath; |
There was a problem hiding this comment.
The path traversal guard uses a hardcoded forward slash (${dir}/), which won't work on Windows where resolve() produces backslash-separated paths. This means the guard would reject all identifiers on Windows.
Use path.sep for cross-platform correctness:
import { sep } from 'node:path';
const normalizedDir = resolve(dir) + sep;
if (!filePath.startsWith(normalizedDir)) {
throw new Error('Invalid session identifier: path traversal detected');
}- Fix secure deletion: use 'r+' flag instead of 'w' to overwrite existing data blocks in-place rather than truncating first - Add Windows platform guard to permission checks so POSIX-only mode verification is skipped on win32 - Make destroyAllSessions read directory directly instead of going through listSessions(), so a single corrupt file can't block the escape hatch - Use case-insensitive address comparison in resolveSession and session destroy for Ethereum address compatibility - Remove redundant env var re-check from getBaseDir() that could silently override setBaseDir() in tests - Use path.sep in path traversal guard for Windows compatibility Made-with: Cursor
|
@spencerstock ty! just addressed the comments! |
montycheese
left a comment
montycheese
left a comment
There was a problem hiding this comment.
how are you testing session creation currently?
| export type ExternalEoaSession = { | ||
| version: typeof SESSION_VERSION; | ||
| mode: 'external-eoa'; | ||
| account: `0x${string}`; |
There was a problem hiding this comment.
this is not necessarily true for a caip address
There was a problem hiding this comment.
good call! updated!
| export type SmartWalletSession = { | ||
| version: typeof SESSION_VERSION; | ||
| mode: 'smart-wallet'; | ||
| account: `0x${string}`; |
There was a problem hiding this comment.
this is not necessarily true for a caip address
There was a problem hiding this comment.
V1 smart wallet session would be EVM only, so think we can keep this one
| @@ -0,0 +1,54 @@ | |||
| import type { ChainId } from './caip.js'; | |||
|
|
|||
| export const SESSION_VERSION = 1; | |||
| session | ||
| .command('destroy') | ||
| .description('Delete a session') | ||
| .argument('[identifier]', 'Sub-account, EOA, or account address of session to destroy') |
There was a problem hiding this comment.
Sub-account, EOA, or account address of session to destroy
is a bit confusing to read here.
maybe something like "the session's wallet address to destroy"
| const destroyed = destroyAllSessions(); | ||
| formatOutput({ status: 'destroyed', sessions: destroyed }, globalOpts.json); | ||
| } else if (identifier && opts.mode) { | ||
| const mode = opts.mode as SessionMode; |
There was a problem hiding this comment.
whats the purpose of passing a mode? whats the expected use case
There was a problem hiding this comment.
this mode is being used to display to user that what session got deleted. currently this is the only human readable identifier, but im also not sure if we'd like to expose internal var to users, open to ideas
…ove destroy help text Made-with: Cursor
|
@montycheese i asked agents to help generate dummy session file in my disk for testing, i have some example in the integration test file |
5 participants
Summary
Adds session management to
@base-org/account-cli, enabling the CLI to persist, resolve, and destroy authenticated sessions for three distinct operating modes. This is the foundational layer that future commands (login,send,balance, etc.) will build on to determine who is acting and how they're authorized.Motivation
The CLI needs a way to remember authentication state between invocations. A user may authenticate as a direct account operator, create a CLI-managed smart wallet sub-account, or bring their own external EOA. Each of these has different authorization semantics, so the session model must be mode-aware and support disambiguation when multiple sessions coexist.
Session modes
operatoraccountsmart-walletsubAccountsigneraddress.external-eoaeoaAll session types carry a
versionfield (currently1) for future schema migrations, acreatedAttimestamp, and an optional CAIP-2chainId(forsmart-walletandexternal-eoa).Session resolution
When a command needs the active session,
resolveSession()applies this precedence:BASE_SESSIONenv var — if set, matches against the key field of all sessions on disk. ReturnsresolvedVia: "env_var".resolvedVia: "auto_select".BASE_SESSION, throwsMULTIPLE_SESSIONSwith a list of availablemode:addresslabels.New CLI commands
session list [--json]— enumerate all sessions on disk with mode-appropriate summariessession info [--json]— resolve and display the active session (includesresolved_via,chain_id,signerwhere applicable)session destroy <address> [--mode <mode>] [--json]— destroy a specific session by its key addresssession destroy --all [--json]— destroy all sessionsSecurity
0o700, session files with0o600. BothlistSessionsandloadSessionverify permissions before reading and throwINSECURE_PERMISSIONSif group/world bits are set.secureDelete()overwrites file contents withcrypto.randomBytesbeforeunlink, preventing recovery from disk.writeSession()writes to a temp file thenrename()s into place to avoid partial-write corruption.sessionFile()resolves the path and rejects any identifier that escapes the sessions directory.~/.base-account/logs/audit.jsonlwith timestamp, operation, mode, and identifier.File layout
Files changed (17 files, +1,614 lines)
types/session.tsSESSION_VERSION,SESSION_MODEStypes/caip.tsChainIdtype,ParsedChainId,KNOWN_CHAINSaliasestypes/audit.tsAuditOperationandAuditEntrytypestypes/index.tscore/paths.tsBASE_ACCOUNT_DIRoverride and path traversal guardcore/session.tslistSessions,loadSession,writeSession,resolveSession,destroySession,destroyAllSessionscore/audit/index.tsappendAuditLog()— append-only JSONL audit writercommands/session/index.tssession list,info,destroyutils/caip.tsparseChainId,isValidChainId,resolveChainIdhelpersutils/permissions.tsverifyDirectoryPermissions,verifyFilePermissionsutils/secure-delete.tssecureDelete()— random-overwrite + unlinkindex.tscore/session.test.tsutils/caip.test.tsutils/permissions.test.tsutils/secure-delete.test.tscommands/session/session.integration.test.tsexecFileSync(217 lines)Test plan
Manual smoke test results
Empty state — no sessions on disk
Single operator session
Single smart-wallet session
Multiple sessions — ambiguity detection
BASE_SESSION env var resolution