Skip to content

Commit 75bd574

Browse files
committed
fix(cloud): a sign-in replaces the whole session — it no longer keeps the last one's refresh token
From review on #95. storeSession wrote the access token, and the refresh token only if the sign-in brought one. With none, the previous session's stayed in SecretStorage — and it is what the new session's first renewal was then made with. The generation counter does not help: by then the stale token IS the current session's, as far as this window can tell. Reproduced on the reviewed code, both ways it can go: - the old token is refused: the refresh is sent with it, its 401 ends the session — the access token minted by the sign-in is deleted and the "session expired" card shown to someone who signed in hours ago. This is the reviewer's case. - the old token is still good: the server renews the PREVIOUS account, and the editor ends up holding that account's access token under the new account's name and plan. Requests then run, and bill, as someone else. The review did not mention this one; it is worse. So a sign-in now settles the refresh token either way — stored, or forgotten when there is none. And it settles it FIRST, before the access token. The same bad pairing was reachable without an absent token at all: with the access token written first, a sign-in cut short before its second write (the editor closing, a keychain that locks) left the new access token over the old refresh token. Written the other way round, what a cut-short sign-in leaves is the old access token over the new refresh token, which the next renewal resolves to the account the user was signing in to. How reachable: today's backend sends a refresh token on both callback forms, so this needs a callback that arrives without one — the legacy `?token=` form accepts that. Latent, then, but the client documents the refresh token as optional and already had a test for a sign-in without one. Three cases in sessionExpiredHost.test.js (57 -> 60), run against the real functions. With the delete removed, exactly those three fail; with the old write order, exactly the ordering case. tsc --checkJs reports no new undefined names.
1 parent 76445f5 commit 75bd574

2 files changed

Lines changed: 58 additions & 2 deletions

File tree

‎extensions/levelcode-ai/extension.js‎

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3193,13 +3193,25 @@ async function webHandoffUrl() {
31933193
return (data && data.url) || null;
31943194
} catch (e) { dbg('account.handoff', { error: String((e && e.message) || e) }); return null; }
31953195
}
3196-
/** Persist an editor session: access token (required), optional refresh token, and display profile. */
3196+
/**
3197+
* Persist an editor session: access token (required), the refresh token if the sign-in brought one,
3198+
* and the display profile.
3199+
*
3200+
* A sign-in REPLACES the session; it does not top one up. A refresh token left over from whatever
3201+
* was here before is what the new session's first renewal would be made with: refused, it ends the
3202+
* session that replaced it; still good, it hands this editor the previous account's access token
3203+
* under the new account's name. So the refresh token is settled first — stored, or forgotten when
3204+
* there is none — and only then the access token. A sign-in cut short between the two (the editor
3205+
* closing, a keychain that will not write) must not leave the new access token over the old
3206+
* refresh token either.
3207+
*/
31973208
async function storeSession(access, refresh, profile) {
31983209
if (!ctx || !access) { return; }
31993210
await withSessionLock(async () => {
32003211
sessionGeneration++; // a refresh still out for the session this replaces must not touch the new one
3201-
await ctx.secrets.store(ACCOUNT_TOKEN_KEY, access);
32023212
if (refresh) { await ctx.secrets.store(ACCOUNT_REFRESH_KEY, refresh); }
3213+
else { await ctx.secrets.delete(ACCOUNT_REFRESH_KEY); }
3214+
await ctx.secrets.store(ACCOUNT_TOKEN_KEY, access);
32033215
cloudSignedIn = true;
32043216
await ctx.globalState.update(ACCOUNT_PROFILE_KEY, {
32053217
name: (profile && profile.name) || '', email: (profile && profile.email) || '', plan: (profile && profile.plan) || ''

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

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -561,6 +561,50 @@ async function test(name, fn) { await fn(); n++; console.log(' ok - ' + name);
561561
assert.ok(!t.types().includes('sessionExpired'));
562562
});
563563

564+
// The case above is a refresh already OUT when such a sign-in lands. These are the NEXT one: with no
565+
// refresh token of its own, the sign-in left the previous session's in storage, and the new
566+
// session's first renewal — hours later — was made with it.
567+
await test('a sign-in with no refresh token of its own forgets the previous session\'s', async () => {
568+
const t = boot({ access: liveAccess(), refresh: 'r-old', profile: { name: 'Ada' } });
569+
const mine = deadAccess(); // the new session's token, at the end of its eight hours
570+
await t.host.storeSession(mine, null, { name: 'Bo' });
571+
assert.strictEqual(t.secrets.get(t.K.refresh), undefined, 'the old refresh token is gone');
572+
t.refreshReply = refused; // what the server would have said to r-old
573+
await t.host.checkCloudSession('focus');
574+
assert.strictEqual(t.calls.refresh.length, 0, 'it is never sent');
575+
assert.strictEqual(t.secrets.get(t.K.token), mine, 'so its 401 cannot end the session that replaced it');
576+
assert.strictEqual(t.state.get(t.K.expired), undefined);
577+
assert.ok(!t.types().includes('sessionExpired'), 'no sign-in card');
578+
});
579+
580+
await test('…nor can a still-valid one put the previous ACCOUNT back under the new name', async () => {
581+
const t = boot({ access: liveAccess(), refresh: 'r-ada', profile: { name: 'Ada' } });
582+
const bo = deadAccess();
583+
await t.host.storeSession(bo, null, { name: 'Bo' }); // Bo signs in over Ada's session
584+
t.refreshReply = { status: 200, body: { access: liveAccess() + '.ada', refresh: 'r-ada-2' } }; // r-ada still works — for Ada
585+
await t.host.checkCloudSession('focus');
586+
assert.strictEqual(t.calls.refresh.length, 0);
587+
assert.strictEqual(t.secrets.get(t.K.token), bo, 'Bo\'s editor is not handed Ada\'s access token');
588+
assert.strictEqual(t.state.get(t.K.profile).name, 'Bo');
589+
});
590+
591+
await test('a sign-in settles the refresh token BEFORE the access token: one cut short never pairs new with old', async () => {
592+
const t = boot({ access: liveAccess(), refresh: 'r-ada' });
593+
await t.host.storeSession(liveAccess(), 'r-bo', { name: 'Bo' });
594+
assert.deepStrictEqual(t.ops.slice(0, 2), ['store ' + t.K.refresh, 'store ' + t.K.token]);
595+
const u = boot({ access: liveAccess(), refresh: 'r-ada' });
596+
await u.host.storeSession(liveAccess(), null, { name: 'Bo' });
597+
assert.deepStrictEqual(u.ops.slice(0, 2), ['forget ' + u.K.refresh, 'store ' + u.K.token]);
598+
599+
// The SECOND write is the one that fails — a keychain that locks half-way through a sign-in.
600+
const v = boot({ access: liveAccess(), refresh: 'r-ada' });
601+
const bos = liveAccess() + '.bo';
602+
let writes = 0;
603+
v.onOp = (op) => { if (/^(store|forget) /.test(op) && ++writes === 2) { v.storeError = new Error('keychain locked'); } };
604+
await assert.rejects(v.host.storeSession(bos, 'r-bo', { name: 'Bo' }), /keychain locked/);
605+
assert.ok(!(v.secrets.get(v.K.token) === bos && v.secrets.get(v.K.refresh) === 'r-ada'), 'never Bo\'s access token over Ada\'s refresh token');
606+
});
607+
564608
await test('late 401: a sign-out while the refresh is out is not turned into an expiry', async () => {
565609
const t = boot({ access: deadAccess(), refresh: 'r1', profile: { name: 'Ada' } });
566610
const reply = deferred();

0 commit comments

Comments
 (0)