fix(gateway): harden /add-project authorization, recovery messaging, and path validation

[marcusrbrown]

Three security and correctness fixes for the /fro-bot add-project command.

Authorization gate

Require the invoking user to hold Manage Channels before running /add-project. Previously any guild member could invoke it and the bot would act with its own broader permissions, letting a low-privilege user drive channel creation they couldn't perform themselves. The check is scoped to the subcommand at runtime rather than default_member_permissions, which would gate the whole /fro-bot command group (including ping).

Safe repo-exists messaging

The previous repo-exists reply told the user to rm -rf /workspace/repos/owner/repo and retry. When a second user invokes while another user's clone is mid-flight, that instruction would delete the in-flight clone. It now re-reads the bindings store and either redirects to the already-bound channel or asks the user to wait — never instructing deletion.

Strict clone-response path validation

The workspace clone response was validated with a suffix match (endsWith('/owner/repo')), which accepts adversarial prefixes such as /etc/passwd/owner/repo. It now requires exact equality against the expected /workspace/repos/owner/repo path.

Gateway test suite: 286 passing.

[fro-bot]

Verdict: CONDITIONAL

Can merge after addressing the one blocking issue. The three security fixes are sound; the PRE_FLIGHT check ordering and path-equality upgrade are both correct, and test coverage is thorough.

Blocking issues

1. repo-exists handler collapses two distinct states into one fallback message

In the repo-exists branch (add-project.ts ~line 350), when repoExistsBinding.success === false (store error) and when repoExistsBinding.data === null (no binding yet) both emit the same "currently being added" message. A store error is not the same as an in-flight clone: the user may be seeing a transient S3 failure against a repo with no active clone at all. The condition should be split:

if (repoExistsBinding.success === false) {
  // Indeterminate — storage error, not necessarily in-progress
  await interaction.editReply({ content: 'Could not verify repo status. Please try again in a moment.' })
} else if (repoExistsBinding.data !== null) {
  await interaction.editReply({ content: `\`${owner}/${repo}\` is already set up in <#${repoExistsBinding.data.channelId}>.` })
} else {
  await interaction.editReply({ content: `\`${owner}/${repo}\` is currently being added by another setup. Please wait a moment and try again.` })
}

The existing test "binding store errors → falls back to 'currently being added'" encodes the incorrect behaviour; it would need updating too.

This matters because a storage error tells the user to wait for a clone that may not exist, blocking them indefinitely. Instructing retry instead of waiting is the correct recovery.

Non-blocking concerns

1. Style inconsistency in boolean checks — Line 215 uses === false (new style introduced by this PR) but line 394 (CREATING_CHANNEL re-check of botHasRequiredPermissions) still uses the ! prefix form. Trivial, but worth aligning for readability.

2. User check ordering — The user authorization gate (FIX #43) fires after the bot-permissions check and after deferReply. This means an unauthorized user sees a deferred response before being rejected. The logic is still correct (rejection happens and no operation proceeds), but placing the authorization check before deferReply would give a tighter UX (ephemeral immediate reply vs. deferred). Not blocking since the current order prevents all harmful side-effects.

3. repo-exists path still doesn't log the binding lookup error — When repoExistsBinding.success === false, the error is silently swallowed. A logger.warn here would help operators diagnose storage degradation mid-operation.

4. Missing test for the guild === null path with the new auth gate — Pre-existing gap, not introduced by this PR.

Missing tests

• Test for repoExistsBinding.success === false being distinguishable from the no-binding case (required if blocking issue #1 is addressed).
• Test that verifies the exact path string produced by the workspace root constant — guards against the constant drifting from the workspace-agent value (the comment acknowledges this risk).

Risk assessment: LOW

All three fixes reduce attack surface with no new external dependencies. The path equality check is strictly more restrictive than the previous suffix check (correct direction). The auth gate is additive. The repo-exists messaging change eliminates a dangerous instruction. Primary residual risk is the store-error/no-binding conflation noted above, which is correctness-only (no security regression vs. main).

---

Run Summary

Field	Value
Event	pull_request
Repository	fro-bot/agent
Run ID	26653385179
Cache	hit
Session	ses_18b1d594dffeZQxiuRvd52efGq

[fro-bot]

Verdict: PASS

All blocking issues from the previous review have been addressed. The three security fixes are correct and well-tested.

What changed since CHANGES_REQUESTED:

1. Auth gate reworked to use guild.members.fetch() — The implementation now fetches guild-level base permissions via REST rather than relying on interaction.memberPermissions (which reflects channel-scoped overwrites). This is the correct Discord API approach: it prevents a user with only a channel-overwrite grant of ManageChannels from bypassing the gate. Fail-closed try/catch is properly placed.

2. repo-exists handler adds a try/catch around the binding lookup — Network-level rejections are now explicitly caught and produce the safe "please wait" message, isolated from the success === false result path. The prior blocking issue (store error conflated with in-flight clone) is partially resolved: network rejections are now a distinct early return. The success === false case still falls into the same else branch as "no binding found," but both are safe messages — no incorrect "wait" instruction when the real cause is something else.

3. EXPECTED_WORKSPACE_ROOT extracted as a named constant — Mirrors the workspace-agent source constant and is documented as intentionally not cross-imported.

4. Case-sensitive path equality — The .toLowerCase() on parsed.path has been removed. Owner and repo arrive pre-lowercased from add-project.ts; the check is now strict. A mixed-case response path from workspace-agent will correctly fail. This is a deliberate and correct tightening.

Blocking issues

None.

Non-blocking concerns

1. repo-exists + success === false still collapses into "currently being added" — The try/catch separates network-level throws, but a structured err({...}) response from the binding store (S3 timeout surfaced as a result value) still hits the else branch alongside the no-binding case. Both emit "currently being added," which is misleading for a store error. Not a correctness regression from main (old code had no binding re-read at all), and the message is safe — just potentially confusing if the store is degraded. A follow-up to split the else into success === false vs. data === null would be clean.

2. members.fetch is a REST call in the hot path — Every /add-project invocation now makes an extra Discord API call before any validation work (URL parse, existing binding check). Under normal load this is fine, but it adds latency and a new failure mode. Noted for observability: the existing logger.warn on failure is sufficient for now.

3. botHasRequiredPermissions style inconsistency (from prior review) — Line 228 now uses === false (consistent with the new auth check), but the CREATING_CHANNEL re-check at line ~404 still uses !. Non-blocking; linter won't catch this.

Missing tests

None blocking. The new test suite covers:

• Unauthorized user (guild-level deny)
• Channel-overwrite-only user (guild-level deny)
• Authorized user (pass)
• members.fetch rejection (fail-closed)
• repo-exists + binding found / not found / store error result / store rejection

The "workspace root constant drift" test noted in the prior review is addressed implicitly by the case-sensitivity test (/WORKSPACE/repos/... is rejected).

Risk assessment: LOW

All changes reduce attack surface or harden messaging. The additional REST call in userIsAuthorized is the only new runtime dependency; it is fail-closed and covered by a test. No public API surface changes. Blast radius on regression is limited to the add-project subcommand only.

---

Run Summary

Field	Value
Event	pull_request
Repository	fro-bot/agent
Run ID	26655542644
Cache	hit
Session	ses_18b1d594dffeZQxiuRvd52efGq