Client session handling: stop reaching into private state, validate snapshots, clear keys on logout #10

Closed
opened 2026-08-09 03:45:01 +02:00 by clawbot · 2 comments
Collaborator

Problem

Three defects in src/client.ts, all in the session lifecycle:

  1. toJSON reads a private field by string index. src/client.ts:141 does
    token: this.api["token"]! to bypass TypeScript's private-field check, with a non-null
    assertion on top. If toJSON() is called on a client with no token, this silently emits
    undefined into a snapshot that is then written to disk as a valid session file.

  2. fromJSON does not validate the snapshot. src/client.ts:107-120 runs fromBase64
    over four fields with no shape check. A truncated, hand-edited, or corrupt
    session.json throws a raw libsodium error out of the CLI with no indication that the
    session file is the problem. bin/quak.ts compounds this by swallowing session-load errors
    entirely, so a corrupt session is indistinguishable from "not logged in".

  3. logout() does not clear key material. src/client.ts:148-151 flips a flag and clears
    the API token, but masterKey, secretKey and publicKey stay live in memory. For a
    client whose whole job is holding the keys to someone's photo library, logout should mean
    the keys are gone.

Definition of done

  1. ApiClient exposes a proper accessor for the current auth token; Client.toJSON uses it
    and no longer indexes into a private field or uses a non-null assertion.
  2. toJSON() on a client with no auth token fails with a clear error rather than emitting a
    snapshot containing undefined.
  3. fromJSON validates the snapshot: required fields present, correct types, base64 decoding
    to the expected byte lengths for each key. An invalid snapshot throws an error that names
    the offending field and says the session data is invalid.
  4. bin/quak.ts distinguishes "no session file" from "session file is corrupt" and tells the
    user which one it is, with a non-zero exit code for the corrupt case.
  5. logout() zeroes the key material buffers in addition to clearing the token, and any
    subsequent operation on the logged-out client throws the same way it does today.
  6. Tests cover each of the above, including toJSON round-tripping through fromJSON
    unchanged, and every rejection case in item 3.
  7. make check green.
  8. TODO.md updated in the same commit.
## Problem Three defects in `src/client.ts`, all in the session lifecycle: 1. **`toJSON` reads a private field by string index.** `src/client.ts:141` does `token: this.api["token"]!` to bypass TypeScript's private-field check, with a non-null assertion on top. If `toJSON()` is called on a client with no token, this silently emits `undefined` into a snapshot that is then written to disk as a valid session file. 2. **`fromJSON` does not validate the snapshot.** `src/client.ts:107-120` runs `fromBase64` over four fields with no shape check. A truncated, hand-edited, or corrupt `session.json` throws a raw libsodium error out of the CLI with no indication that the session file is the problem. `bin/quak.ts` compounds this by swallowing session-load errors entirely, so a corrupt session is indistinguishable from "not logged in". 3. **`logout()` does not clear key material.** `src/client.ts:148-151` flips a flag and clears the API token, but `masterKey`, `secretKey` and `publicKey` stay live in memory. For a client whose whole job is holding the keys to someone's photo library, logout should mean the keys are gone. ## Definition of done 1. `ApiClient` exposes a proper accessor for the current auth token; `Client.toJSON` uses it and no longer indexes into a private field or uses a non-null assertion. 2. `toJSON()` on a client with no auth token fails with a clear error rather than emitting a snapshot containing `undefined`. 3. `fromJSON` validates the snapshot: required fields present, correct types, base64 decoding to the expected byte lengths for each key. An invalid snapshot throws an error that names the offending field and says the session data is invalid. 4. `bin/quak.ts` distinguishes "no session file" from "session file is corrupt" and tells the user which one it is, with a non-zero exit code for the corrupt case. 5. `logout()` zeroes the key material buffers in addition to clearing the token, and any subsequent operation on the logged-out client throws the same way it does today. 6. Tests cover each of the above, including `toJSON` round-tripping through `fromJSON` unchanged, and every rejection case in item 3. 7. `make check` green. 8. `TODO.md` updated in the same commit.
clawbot added this to the 1.0.0 milestone 2026-08-09 03:45:01 +02:00
clawbot self-assigned this 2026-08-09 03:45:01 +02:00
Author
Collaborator

Implementer brief (current tree, branch next2)

The issue's line numbers are out of date. Where each defect is today:

  • toJSON reads the private token field at src/client.ts:170 (this.api["token"]!).
  • fromJSON at src/client.ts:128-140 decodes the fields with no shape check.
  • logout is at src/client.ts:177.
  • In bin/quak.ts:34-58, loadSession returns null on any parse error, so a corrupt session.json reads as "not logged in". Fix this for definition-of-done item 4: a missing file and a corrupt file are reported differently, and the corrupt case exits non-zero.

A trap to handle explicitly: the key buffers are passed by reference into decryption (src/client.ts:205-209), and a Library built on the client may still hold them. After zeroing them in logout(), every later operation must fail loudly with the error it raises today when logged out. It must never go on decrypting with all-zero keys. Add a test for this.

Definition of done: as in the issue body. Out of scope: token refresh, and any change to the on-disk session format beyond validating it.

Model: opus-5-5

## Implementer brief (current tree, branch `next2`) The issue's line numbers are out of date. Where each defect is today: - `toJSON` reads the private token field at `src/client.ts:170` (`this.api["token"]!`). - `fromJSON` at `src/client.ts:128-140` decodes the fields with no shape check. - `logout` is at `src/client.ts:177`. - In `bin/quak.ts:34-58`, `loadSession` returns `null` on any parse error, so a corrupt `session.json` reads as "not logged in". Fix this for definition-of-done item 4: a missing file and a corrupt file are reported differently, and the corrupt case exits non-zero. A trap to handle explicitly: the key buffers are passed by reference into decryption (`src/client.ts:205-209`), and a `Library` built on the client may still hold them. After zeroing them in `logout()`, every later operation must fail loudly with the error it raises today when logged out. It must never go on decrypting with all-zero keys. Add a test for this. Definition of done: as in the issue body. Out of scope: token refresh, and any change to the on-disk session format beyond validating it. Model: opus-5-5
Author
Collaborator

Implemented in #79 (base next2): snapshot validation in fromJSON, token accessor for toJSON, key zeroing in logout with a re-check in collectionsSince after its request, and separate CLI messages for a missing and a corrupt session file.

Model: opus-5-5

Implemented in https://git.eeqj.de/sneak/quak/pulls/79 (base `next2`): snapshot validation in `fromJSON`, token accessor for `toJSON`, key zeroing in `logout` with a re-check in `collectionsSince` after its request, and separate CLI messages for a missing and a corrupt session file. Model: opus-5-5
Sign in to join this conversation.
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/quak#10