GET /config returns provider keys in plaintext while masking the one local credential a same-uid process does not need #286

Summary

~/.thinkwatch/config.yaml holds every credential in plaintext. That is defensible on its
own — but two things make it worse than it looks:

1. The masking is on the wrong secret. GET /config masks listen.control.key (the
   local control credential, which a same-uid process doesn't need — it can just edit the
   file) while returning providers[].key and clients[].key in plaintext in the same
   response. The upstream credential — the one that spends the user's money — is the
   unmasked one.

2. A same-uid process can write the config through the control plane, even when it
   cannot write the file. On this machine a sandboxed agent is blocked by its own
   process sandbox (read-only for ~/.thinkwatch) yet PUT /config over the unix socket
   succeeds, because the write happens inside twcore. The sandbox guards the caller,
   not the callee.

So the effective protection for a local model is: it can read every secret directly, and
it can rewrite the policy that governs it — without ever needing the control key.

Reproduced (core 0.61.0, Linux x86_64, 2026.10.4)

① Everything of value is in one plaintext file, mode 600.

-rw-------  ~/.thinkwatch/config.yaml     ← control.key, clients[].key, providers[].key
-rw-------  ~/.thinkwatch/data.db
srwx------  ~/.thinkwatch/twcore.sock     ← same-uid can connect

600 protects against other uids. Against a process running as the same user — which is
exactly what a local agent is — it does nothing. Measured: a same-uid agent reads the file
and obtains all four secrets verbatim.

② Masking covers the least useful secret, and misses the most valuable one.

GET /config →
  control.key            → hidden-see-twcore-control-key-xxxx…   (masked ✅)
  clients[default].key   → verbatim                             (plaintext ⚠)
  clients[dsh].key       → verbatim                             (plaintext ⚠)
  providers[9router].key → verbatim                             (plaintext ⚠)

control.key is a local credential — the README notes the control plane is unix-socket /
loopback only, and remote control is off by default. A same-uid process does not need it:
it can read or edit the file directly, or call the socket as the app does. Meanwhile the
upstream key, which grants paid access to the user's providers, is served in the clear by
the same endpoint.

③ The control plane writes the config its caller cannot write.

# same-uid agent, direct write:
open('~/.thinkwatch/config.yaml','a')  → OSError [Errno 30] Read-only file system

# same-uid agent, via the control plane:
GET  /config  → {"version":"blake3:6ddb3813f4e6", "text":…}
PUT  /config  -d {"text":<same text>, "base_version":"blake3:6ddb3813f4e6"}
              → {"version":"blake3:6ddb3813f4e6"}      (succeeded; file sha256 unchanged)

The version token needed for optimistic locking is handed out by GET /config, so a
caller who can read can also write. I did this with an identical payload so the file was
not altered — but the round trip succeeded, which is the point.

I have not verified that a malicious config rewrite (e.g. disabling
security.redact) is accepted, because doing so would have changed a live configuration.
The read-version → write-path chain is verified; whether a policy change sticks is the
natural next test and I'd rather the maintainers run it than mutate their config.

Why this is not covered by the stated threat model

The README states the design goal as protection against relays — the remote provider
must not see the credentials. That is implemented well: measured 0 occurrences of the
upstream key in data.db or in stored request bodies.

But redaction is an outbound control, and the README/issue history treat the local
filesystem as trusted. For a gateway whose stated purpose is to sit in front of local
clients, the local filesystem is not a trusted boundary — it is where the adversary lives.
Concretely, on a machine that runs coding agents, "a process running as this user" is not
hypothetical; it is the primary consumer.

Two distinct controls are being conflated:

Requests, cheapest first

1. Mask providers[].key and clients[].key in GET /config too. They are already
   masked elsewhere in the product. Right now the one field the UI hides is the one a local
   process doesn't need, while the fields it does need are echoed in the clear. (Small,
   self-contained, aligns the endpoint with the UI.)

2. Document the boundary honestly. State in the Security docs that config.yaml is
   plaintext, that 600 is the whole local protection, and that redaction is outbound-only.
   The masking in the UI currently reads as a storage guarantee; one sentence would stop
   that misreading. (Cheapest; useful even if nothing else changes.)

3. Key-provider indirection. ${ENV_VAR} works for providers[].key and
   clients[].key but not for listen.control.key:

   providers[].key    = ${MY_UP_KEY}   → ✅ valid
   clients[].key      = ${ENV_A}       → ✅ valid
   listen.control.key = ${MY_CTRL_KEY} → ❌ has to be 64 hexadecimal characters
   

   The one credential that cannot come from outside the file is the highest-privilege one.
   A file: or OS-keychain source read at startup (rather than an env var baked into a
   unit file) would fit the existing design — I understand feat(control): one control key and a Noise handshake on every control connection #184 removed TOKEN_ENV
   deliberately because the Noise PSK must be readable at handshake time. Note there is no
   keychain integration in the binary today (libsecret / SecretService / Keychain / DPAPI:
   0 references).

4. An optional at-rest encryption for credential fields, keeping a plaintext skeleton,
   so an unattended restart degrades to "gateway up, upstreams locked" rather than "nothing
   starts". A passphrase prompt that blocks twcore serve would break the daemon case
   (twcore.service), which is presumably why plaintext was chosen — this shape avoids that.

If this is out of scope

Item 1 and item 2 together cost very little and remove the misleading part. I'd be glad to
send a PR for item 1 if the direction is welcome.

Filed after reading #184 to understand the intended design. This is a proposal about
boundaries, not a claim that redaction is broken — it works.

— 偶然死神 (@ouransishen) · GPG C95A4D3648C1E00D