Skip to content

test(accounts): assert removed-account secrets are pruned from the state file#18

Merged
ualtinok merged 1 commit into
cortexkit:mainfrom
iceteaSA:test/state-secret-prune
Jul 5, 2026
Merged

test(accounts): assert removed-account secrets are pruned from the state file#18
ualtinok merged 1 commit into
cortexkit:mainfrom
iceteaSA:test/state-secret-prune

Conversation

@iceteaSA

@iceteaSA iceteaSA commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

What

Extends the existing mutateAccounts removal test (accounts-store.test.ts) to assert that a removed account's per-account secrets are scrubbed from the state file, not just that the config file's id list no longer contains it.

Test-only — zero production change (core/accounts.ts is byte-identical to main).

Why

mutateAccounts writes the config authoritatively and rebuilds the state file via stateFromStorage(next), so a removed account's access/refresh tokens are already pruned. But the removal test only verified the config file:

const cfg = JSON.parse(readFileSync(cfgPath, 'utf8'))
expect(cfg.accounts.map((a) => a.id)).toEqual(['a', 'c'])

A regression that rebuilt the config authoritatively while leaving the state entries merged in (e.g. unioning stale on-disk state into the rebuild) would leave a deleted account's credentials at rest — a credential leak — and still pass every existing assertion. The state file is where the tokens actually live, so it's the security-relevant surface to pin.

The added assertions

const stateRaw = readFileSync(statePath, 'utf8')
const state = JSON.parse(stateRaw)
expect(Object.keys(state.accounts ?? {}).sort()).toEqual(['a', 'c'])
expect(stateRaw).not.toContain('acc-b')
expect(stateRaw).not.toContain('ref-b')

Verification

  • Red-first proven load-bearing: temporarily simulating the leak (unioning stale on-disk state into the state rebuild) makes this test fail (state.accounts keys ['a','b','c']['a','c']); reverting restores green. So it guards the real regression, not just current behavior.
  • Full suite green: 461 pass / 0 fail, typecheck clean, build clean, biome clean.

Context: this is the one piece of #17 not already covered by 38df1ff (which independently landed the same mutateAccounts fix). Closed #17 as superseded and salvaged this test as a focused standalone.


View with Codesmith Autofix with Codesmith
Need help on this PR? Tag /codesmith with what you need. Autofix is disabled.


Summary by cubic

Extend the mutateAccounts removal test to verify the state file drops the deleted account and its tokens, preventing stale credentials at rest. Test-only change; production code unchanged.

Written for commit a9f2379. Summary will update on new commits.

Review in cubic

…ate file

The removal test verified the config file no longer lists the deleted id, but
not that the per-account state file is scrubbed of the removed account's
access/refresh tokens. A regression that rebuilt config authoritatively while
leaving stale state entries would leave a deleted account's credentials at rest
and pass the existing assertions. Extend the test to assert state.accounts drops
the removed id and the raw state file contains neither of its tokens.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 1 file

Re-trigger cubic

@ualtinok ualtinok merged commit 2b5190b into cortexkit:main Jul 5, 2026
4 checks passed
ualtinok added a commit that referenced this pull request Jul 5, 2026
Oracle audit of the account store (config metadata + separate state-file secrets)
found removed-account secrets could reappear at rest:

HIGH — every scalar-field writer (routing/killswitch/dump/logging/cachekeep
commands, plus the loader's mainAccountId write and updateMainRefreshState) did
loadAccounts→mutate→saveAccounts. saveAccounts union-merges its (possibly stale)
account list over the on-disk latest, so a scalar write racing a concurrent
removal resurrected the removed account into config AND re-wrote its
access/refresh/apiKey into the state file. Route all of them through
mutateAccounts (read-fresh under the lock, authoritative rewrite, no union).
saveAccounts now survives only for single-writer test seeding.

MEDIUM — saveAccountState(accounts:true) (background refresh / markUsed on a
stale snapshot) re-added a removed account's state secrets. Gate state writes on
the authoritative CONFIG roster: skip accounts absent from the roster and purge
orphan state ids (also clears the mutateAccounts config-then-state crash-window
orphan on the next write). Reads the config lock-free (atomic file) and falls
back to today's merge-only behavior when the roster is unreadable, so it can
never wipe live secrets.

Tests (teeth-verified): saveAccountState stale-snapshot no-re-add (incl api-key),
orphan-state prune, command-level scalar-after-remove no-resurrect, and a
strengthened parsed-secret assertion on the PR #18 removal test.
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.

2 participants