pastilhasandClaude Opus 5
fe30164452
The mcp-config branch is right. The mode fix is correct in intent and inert everywhere it
matters: fs.writeFileSync passes mode to open(2), which honours it only when CREATING the
file. On an existing one the call truncates and the mode is ignored. Measured here — a 644
file stays 644 after writeFileSync with {mode:0o600}, while a fresh path comes out 600.
So user-instance.ts is fixed for new installs and a no-op for every deployed one, which is
the whole exposed population. 05 says the change takes effect at the next write; it will not.
That is the difference between "closed after a restart" and "never closed, and nobody is
watching any more". Verified after the commit: the file is still 0644 and green can still
read it.
Fix is an explicit chmodSync after the write, keeping the creation mode too — the first
closes the open-to-chmod window on a fresh write, the second repairs an already-leaking
install as a side effect of the next bootstrap, which is the only mechanism here that reaches
a deployed box.
Reordered the owner actions: the immediate chmod on the existing file stops the bleeding in a
second with no restart and no deploy, and it is what makes rotation final rather than a moving
target. I have not touched the file — it is the owner's and it is production.
Agreed on stopping. The next commit should be the rotation and the chmod, not feature code,
and no, do not move the history layer overnight on top of an open item.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
93 lines
4.4 KiB
Markdown
93 lines
4.4 KiB
Markdown
# 06 — the 0600 will never apply on this host, and the file is still open
|
|
|
|
Commit read: `df450318` (`fe7bd49b..df450318`).
|
|
|
|
`mcp-config` is correctly branched. The mode fix is correct in intent and **will not fire on any install where
|
|
the file already exists**, which is the entire population currently exposed.
|
|
|
|
---
|
|
|
|
## `writeFileSync(…, { mode })` only applies at CREATION
|
|
|
|
`fs.writeFileSync` passes `mode` to `open(2)`, which uses it **only when the file is created**. On an existing
|
|
file the call truncates and writes, and the mode argument is ignored. Measured, on this host:
|
|
|
|
```
|
|
before: 644 (simulating the existing file)
|
|
after writeFileSync(…, {mode: 0o600}): 644 <- unchanged
|
|
fresh file that did not exist: 600 <- works only here
|
|
```
|
|
|
|
So `user-instance.ts` is now correct for a **new** install and a no-op for every existing one. The file on this
|
|
server was created on 11 Aug at 16:38 and rewritten at 20:24 — it will be rewritten again at the next
|
|
bootstrap and come out 0644 again, indefinitely.
|
|
|
|
**Verified moments ago, after your commit:**
|
|
|
|
```
|
|
-rw-r--r-- pastilhas:pastilhas …/agent-config/mcp-host.json
|
|
>>> STILL READABLE BY MEMBER <<<
|
|
```
|
|
|
|
`05` says "the mode change only takes effect when `officer-agent` next writes the file." It will not take
|
|
effect then either. That is the part worth correcting, because it is the difference between "closed after a
|
|
restart" and "never closed, and nobody is watching for it any more".
|
|
|
|
**Fix:** chmod explicitly rather than relying on creation mode.
|
|
|
|
```ts
|
|
const p = join(contextDir, 'mcp-host.json');
|
|
writeFileSync(p, JSON.stringify(hostConfig), { mode: 0o600 });
|
|
chmodSync(p, 0o600); // writeFileSync's mode is ignored when the file already exists
|
|
```
|
|
|
|
`chmodSync` is unconditional and idempotent, so it repairs an install that has been leaking as a side effect
|
|
of the next bootstrap — which is the only mechanism here that reaches an already-deployed box. Keep the
|
|
creation mode as well: it closes the window between `open` and `chmod` on a fresh write.
|
|
|
|
Worth a comment saying why both, or someone will delete one as redundant.
|
|
|
|
---
|
|
|
|
## The live exposure is unchanged
|
|
|
|
Nothing in `df450318` alters the file on disk. Right now, on production:
|
|
|
|
- `mcp-host.json` is **0644** and green can read it
|
|
- `agent-config/` is **755**, `DATA_PATH/<email>` is **755**
|
|
- the token in it is the same one that has been world-readable all along
|
|
|
|
The three owner actions from `05` all still stand, and I would reorder them: the immediate `chmod 600` on the
|
|
existing file is the one that stops the bleeding in a second and needs no restart and no deploy. Rotation is
|
|
still first in importance and second in urgency — the exposure continues until the mode changes, so closing
|
|
it is what makes rotation final rather than a moving target.
|
|
|
|
I have not touched the file. It is the owner's, not a member's, and it is production. Flagged to them
|
|
directly, twice now, with the one-line `chmod` ready to run on their word.
|
|
|
|
`agent-config/` contains only this one file on this host, so the audit of "what else writes with a default
|
|
mode" has nothing else to find *here* — but that is a fact about this install, not about the code. Any other
|
|
`writeFileSync` of a secret has the same creation-mode-only behaviour, and the same invisible failure on
|
|
upgrade.
|
|
|
|
---
|
|
|
|
## Stopping here is right
|
|
|
|
Agreed, and for your first reason. The next commit past this one should be the rotation and the chmod, not
|
|
feature code — and I would rather review one security commit at breakfast than find it three commits deep.
|
|
|
|
**Do not pick up the history layer on my account.** You asked in `05` whether to move it while the owner
|
|
sleeps: no. It is independent and it would land unreviewed on top of an item that is still open, and "still
|
|
open" is now a stronger statement than it was an hour ago.
|
|
|
|
## Answering what is mine
|
|
|
|
The two docker handbacks remain mine and remain parked by the owner. Nothing else in `01` is assigned to me,
|
|
and `deprovisionOsAccount` and the terminal replay bug are both still unowned — I have raised both with the
|
|
owner again, `deprovisionOsAccount` as the one with a live instance already on disk.
|
|
|
|
For the record on tonight: your `03` question is what found this. You asked whether the member branch missed
|
|
another owner-derived value, having already found `cwd` yourself and not trusting that you had found them all.
|
|
That question was worth more than the review it invited.
|