Skip to content

Commit 988be27

Browse files
committed
fix(memory): close the two remaining ways a secret reached disk
Both from PR #64 review, both correct, both the same shape — the redaction boundary had gaps I had not walked. **The supersede history.** `factControl(…, by)` stored a SECOND copy of the replacing fact's text, taken from the model's raw output (`r.facts[0] || r.summary` in extension.js) rather than from the observation `factObservation` had already scrubbed. It persists to facts.jsonl and surfaces as `supersededBy` in the panel. Same text, different door. **Edited file paths.** `outcomeEntry` redacted the title and summary and passed `filesEdited` through untouched — and `digestMarkdown` prints those straight into MEMORY.md ("- did X (a.js, b.js)"). A path is attacker-influenced in a hostile repo and user-influenced everywhere else: a downloaded `key-ghp_….txt`, an .env backup named after the token it holds. No legitimate path carries a credential prefix, so redacting costs nothing. Also adds a COMPLETENESS test that walks every writer with the same poisoned string and asserts the SERIALIZED entry — exactly what appendJournal/appendFacts put on disk — contains no secret. Both gaps here were found by review rather than by the suite; a new field on any of these writers now fails that test instead of shipping. 37 cases (was 34). Both new guards verified non-vacuous: reverting the `by` redaction fails at 31/37, reverting the file-path redaction at 32/37. All 32 extension suites green.
1 parent 13b90cf commit 988be27

2 files changed

Lines changed: 54 additions & 2 deletions

File tree

‎extensions/levelcode-ai/sessionMemory.js‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,11 @@ function memoryMdFile(root, slug) { return path.join(memoryDir(root, slug), 'MEM
3939
*/
4040
function outcomeEntry(derived, t) {
4141
const d = derived || {};
42-
const files = Array.isArray(d.filesEdited) ? d.filesEdited.slice(0, 6) : [];
42+
// Paths get redacted too. They are not free text, but they are not safe either: digestMarkdown
43+
// prints them straight into MEMORY.md ("- did X (a.js, b.js)"), and a path is attacker-influenced
44+
// in a hostile repo and user-influenced everywhere else — a downloaded `key-ghp_….txt`, an `.env`
45+
// backup named after the token it holds. Cheap, and no legitimate path carries a credential prefix.
46+
const files = (Array.isArray(d.filesEdited) ? d.filesEdited.slice(0, 6) : []).map((f) => redactSecrets(String(f)));
4347
// The title is derived from the session's opening message, so a user who pasted a token into
4448
// chat to ask about it would otherwise have it copied into journal.jsonl and MEMORY.md — files
4549
// that outlive the session and are meant to be greppable and checkinable.
@@ -196,7 +200,11 @@ function factObservation(text, sourceId, t) {
196200
function factControl(key, action, t, by) {
197201
const control = action === 'remove' ? 'remove' : action === 'supersede' ? 'supersede' : 'confirm';
198202
const e = { v: SCHEMA_V, key: String(key || ''), control, at: t || null };
199-
if (control === 'supersede' && by) { e.by = String(by); } // the fact that replaced it — the one-line history
203+
// `by` is a SECOND copy of the replacing fact's text, taken straight from the model's output
204+
// (extension.js: `r.facts[0] || r.summary`) rather than from the observation that factObservation
205+
// already scrubbed. It persists to facts.jsonl and surfaces as `supersededBy` in the panel, so
206+
// without this it was a way around the boundary — same text, different door.
207+
if (control === 'supersede' && by) { e.by = redactSecrets(String(by)); } // the fact that replaced it — the one-line history
200208
return e;
201209
}
202210
/** Append fact observations and/or control events (JSONL). Creates memory/ on first write. */

‎extensions/levelcode-ai/test/memoryPoisoning.test.js‎

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -161,6 +161,50 @@ test('SECRET: the session TITLE is scrubbed too, not just facts', () => {
161161
assert.ok(!e.summary.includes(pat), 'token survived into the summary: ' + e.summary);
162162
});
163163

164+
test('SECRET: the supersede history is scrubbed — the same text, a different door', () => {
165+
// `by` is a SECOND copy of the replacing fact, taken from the model's raw output rather than
166+
// from the observation factObservation already cleaned, and it surfaces as `supersededBy`.
167+
// Redacting only the obvious writer leaves the quiet one open.
168+
const pat = tok('ghp', '_', 'A1b2C3d4E5f6G7h8I9j0K1l2M3n4O5p6Q7r8');
169+
const c = M.factControl('old key', 'supersede', at(1), 'Superseded by the token ' + pat);
170+
assert.ok(!c.by.includes(pat), 'token survived into facts.jsonl via supersede: ' + c.by);
171+
assert.ok(c.by.includes('[redacted]'));
172+
173+
// …and it stays scrubbed once folded, which is the value the panel renders.
174+
const folded = M.foldFacts([ M.factObservation('Tokens live in Redis', 's1', at(1)), c ].map((e) => e))
175+
.find((f) => f.supersededBy);
176+
if (folded) { assert.ok(!folded.supersededBy.includes(pat)); }
177+
});
178+
179+
test('SECRET: edited file PATHS are scrubbed — digestMarkdown prints them into MEMORY.md', () => {
180+
const pat = tok('ghp', '_', 'A1b2C3d4E5f6G7h8I9j0K1l2M3n4O5p6Q7r8');
181+
const e = M.outcomeEntry({ id: 's1', title: 'Rotated the deploy key', updatedAt: at(1),
182+
filesEdited: ['src/app.js', 'tmp/key-' + pat + '.txt'] });
183+
assert.ok(!JSON.stringify(e.files).includes(pat), 'token survived in the file list: ' + JSON.stringify(e.files));
184+
assert.strictEqual(e.files[0], 'src/app.js', 'an ordinary path was mangled');
185+
186+
// End to end: the rendered digest is what actually lands in MEMORY.md.
187+
const md = M.digestMarkdown(M.buildDigest([e]), { asOf: at(2) });
188+
assert.ok(!md.includes(pat), 'token reached MEMORY.md through the file list');
189+
});
190+
191+
test('COMPLETENESS: every field that reaches disk goes through redactSecrets', () => {
192+
// The boundary is only as good as its narrowest gap, and the last two were found by review
193+
// rather than by this suite. Walk every writer with the same poisoned string so a NEW field
194+
// added to any of them fails here instead of shipping.
195+
const pat = tok('ghp', '_', 'A1b2C3d4E5f6G7h8I9j0K1l2M3n4O5p6Q7r8');
196+
const carriers = [
197+
['outcomeEntry', M.outcomeEntry({ id: 's1', title: 'x ' + pat, updatedAt: at(1),
198+
filesEdited: ['a-' + pat + '.js'] })],
199+
['factObservation', M.factObservation('the key is ' + pat, 's1', at(1))],
200+
['factControl(supersede)', M.factControl('k', 'supersede', at(1), 'replaced by ' + pat)]
201+
];
202+
for (const [what, entry] of carriers) {
203+
const serialized = JSON.stringify(entry); // exactly what appendJournal/appendFacts write
204+
assert.ok(!serialized.includes(pat), what + ' writes an unredacted secret to disk: ' + serialized);
205+
}
206+
});
207+
164208
// ---- 4. Things that must NOT be mistaken for secrets --------------------------------------------
165209

166210
test('NOT-SECRETS: hashes, SHAs and identifiers survive intact', () => {

0 commit comments

Comments
 (0)