Skip to content

Write the CLI server-connection store owner-only, and make its tests run - #1576

Open
GeiserX wants to merge 1 commit into
UsefulSoftwareCo:mainfrom
GeiserX:fix/cli-credential-store-permissions
Open

Write the CLI server-connection store owner-only, and make its tests run#1576
GeiserX wants to merge 1 commit into
UsefulSoftwareCo:mainfrom
GeiserX:fix/cli-credential-store-permissions

Conversation

@GeiserX

@GeiserX GeiserX commented Aug 13, 2026

Copy link
Copy Markdown

TL;DR

Two things, both in apps/cli/src/server-profile.*.

1. ~/.executor/server-connections.json was world-readable. It holds a bearer token, or an OAuth access token and its long-lived refresh token, and it is rewritten on every silent refresh. No mode was set, so the umask applied and it landed 0644. Now 0600, matching what the local-server manifest already does for the sibling secret in the same directory.

2. Four tests in that file never ran. They are it("…", () => Effect.gen(…)). An Effect is not a thenable, so Vitest saw a non-promise return, called the test passed, and never executed the body. I found this because my new assertions passed against code I had deliberately broken.


The permissions change

yield* fs.writeFileString(storePath, serializeCliServerConnectionStore(store), { mode: 0o600 });
yield* fs.chmod(storePath, 0o600).pipe(Effect.ignore);

Both steps are needed, and this is your own existing pattern — local-server-manifest.ts writes the sibling secret exactly this way, with a comment explaining why. mode applies only when the file is created, so it closes the window where a fresh store is briefly world-readable; the chmod covers rewriting a store that already exists with looser permissions.

That second case matters more here than it does for the manifest: this file is rewritten on every silent token refresh, so overwrite is the common path, and it is also the path that upgrades an existing user's 0644 store left behind by an older version.

The tests that were not running

This is the part worth a second look, because it is easy to reproduce:

it("round-trips named server connections", () =>
  Effect.gen(function* () { /* … */ }).pipe(Effect.provide(BunServices.layer)));

Effect has no .then, so this returns a value Vitest neither awaits nor runs. I verified it rather than assumed it: changing a pre-existing assertion to expect(store.defaultProfile).toBe("THIS_IS_DELIBERATELY_WRONG") still reported 4 passed.

Switched to it.effect, they execute and pass — so no production behaviour was wrong, the tests simply were not checking it. Two other tests in the file are ordinary synchronous ones and are left as it.

I swept the rest of the repo for the same shape and this is the only file affected. The two other candidates turned out to be a helper signature and a set of await Effect.runPromise(…) tests, both fine.

Tests

Two added, covering the two mechanisms separately: a fresh store is created 0600, and a pre-existing 0644 store is tightened on rewrite.

Mutation-checked, each mutation verified to have landed, with an unmutated control before and after:

mutation result
drop both (i.e. revert this PR) killed (2 tests)
drop the chmod killed (exactly the overwrite test)
drop the create mode survived — see below

The survivor is honest rather than a gap: with the chmod still running, the final mode is 0600 either way. The mode argument closes the window between create and chmod, which no assertion about final filesystem state can observe. It is kept for the same reason the manifest keeps it.

Package: 73 passed / 8 files. tsgo --noEmit, oxlint --deny-warnings and oxfmt --check clean.

server-connections.json holds a bearer token, or an OAuth access token and its
long-lived refresh token, rewritten on every silent refresh. It was created
with no mode, so the umask applied and it landed world-readable.

Create it 0600 with a follow-up chmod, matching the local-server manifest in
the same directory. Both steps matter: mode applies only on create, and the
chmod covers rewriting an existing looser file -- the common path here.

Separately, four tests in this file were it(..., () => Effect.gen(...)). An
Effect is not thenable, so vitest passed them without running their bodies; a
deliberately falsified assertion still passed. They are now it.effect.
@GeiserX

GeiserX commented Aug 13, 2026

Copy link
Copy Markdown
Author

Context for this one: #1585 explains why this PR and twelve others exist — they came out of a single pass over credential handling, asking for each credential where it ends up, how long it stays, and who can read it once it's there.

This PR stands alone and doesn't depend on any of the others.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant