-
Notifications
You must be signed in to change notification settings - Fork 492
fix(settings): guard POST /api/setup against agent overwrites #222
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -43,9 +43,21 @@ app.put('/api/settings', async (c) => { | |||||||||||||
| }); | ||||||||||||||
|
|
||||||||||||||
| // POST /api/setup — run initial setup (write settings + create directories) | ||||||||||||||
| // Requires ?force=true if settings.json already exists with agents configured, | ||||||||||||||
| // to prevent agents from accidentally wiping a live configuration. | ||||||||||||||
| app.post('/api/setup', async (c) => { | ||||||||||||||
| const force = c.req.query('force') === 'true'; | ||||||||||||||
| const settings = (await c.req.json()) as Settings; | ||||||||||||||
|
|
||||||||||||||
| // Guard: refuse to overwrite an existing configured installation unless forced | ||||||||||||||
| if (!force && fs.existsSync(SETTINGS_FILE)) { | ||||||||||||||
| const existing = (() => { try { return JSON.parse(fs.readFileSync(SETTINGS_FILE, 'utf8')); } catch { return null; } })(); | ||||||||||||||
| if (existing?.agents && Object.keys(existing.agents).length > 0) { | ||||||||||||||
| log('WARN', '[API] Setup blocked: settings.json already has agents configured. Use ?force=true to overwrite.'); | ||||||||||||||
| return c.json({ ok: false, error: 'Settings already configured. Pass ?force=true to overwrite.' }, 409); | ||||||||||||||
|
||||||||||||||
| return c.json({ ok: false, error: 'Settings already configured. Pass ?force=true to overwrite.' }, 409); | |
| return c.json({ ok: false, error: 'Settings already configured. This endpoint is for initial setup only.' }, 409); |
The log message on line 56 already captures the ?force=true detail for human operators, so removing it from the response body doesn't sacrifice operator visibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Single .bak slot — earlier backups silently overwritten
settings.json.bak is a fixed path, so each call with ?force=true overwrites the previous backup. If an operator (or a misbehaving agent) calls POST /api/setup?force=true more than once, all backups except the most recent one are permanently lost. The PR description frames .bak as a recovery mechanism, but it only guarantees recovery from the last overwrite, not from the original state.
A timestamped backup name (e.g., settings.json.bak.<timestamp>) would make recovery more reliable without much added complexity:
| const backupPath = `${SETTINGS_FILE}.bak`; | |
| fs.copyFileSync(SETTINGS_FILE, backupPath); | |
| log('INFO', `[API] Setup: backed up existing settings to ${backupPath}`); | |
| const backupPath = `${SETTINGS_FILE}.bak.${Date.now()}`; | |
| fs.copyFileSync(SETTINGS_FILE, backupPath); | |
| log('INFO', `[API] Setup: backed up existing settings to ${backupPath}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SKILL.md teaches agents the
?force=truebypassThe SKILL.md is loaded as context by every agent that invokes this skill. This line explicitly teaches agents that a 409 can be resolved by appending
?force=true. A task-oriented agent instructed to "set up tinyclaw" (or to resolve a 409 error it received fromPOST /api/setup) will read this and know to retry with?force=true— exactly the overwrite scenario the PR is trying to prevent.If the intent is that
?force=trueis a human-operator-only escape hatch, it should not be documented in agent-readable skill files. Consider moving this detail to a separate operator runbook ordocs/file that is not included in agent context, and changing this line to reinforce the "agents must never call this endpoint" constraint only: