Skip to content

docs(#5586): add security guidance for destructive forge operations #5587

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.

Sign up for GitHub

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

Open
fullsend-ai-coder wants to merge 1 commit into
base: main
Choose a base branch
from
+14 −0
Open

docs(#5586): add security guidance for destructive forge operations #5587

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions docs/contributing/forge-abstraction.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,3 +15,17 @@ All git forge operations (GitHub API calls, PR comments, issue creation, workflo
**When reviewing PRs:** Flag any direct `exec.Command("gh", ...)`, raw GitHub API calls, or other forge-specific operations outside `internal/forge/github/` as a medium-severity or higher finding. This is an architectural violation, not a style preference.

**Composite action (`action.yml`):** The forge abstraction extends to `action.yml` bash scripts. New GitHub API operations in action steps should be implemented as `fullsend` CLI subcommands (under `internal/cli/`) that use `forge.Client`, not as inline `gh api` calls. Existing `gh api` calls in `action.yml` that predate this rule are grandfathered but should be migrated when touched.

## Security considerations for destructive operations

Forge methods that modify or destroy resources — closing PRs/MRs, deleting branches/refs, merging, force-pushing — require ownership or authorization verification at the **call site**, not inside the forge method itself. The forge method is a thin transport layer; it should not encode policy about who is allowed to act. The caller has the context to decide whether the operation is safe.

**1. Verify ownership before acting.** Before invoking a destructive forge operation, the caller must verify that the authenticated user has a legitimate relationship to the target resource. For example, before closing a PR, confirm the PR was authored by the authenticated user (or by a known bot identity the caller controls). Do not assume that the ability to call a forge method implies authorization to use it on any resource.

**2. Prefer fail-closed defaults.** When ownership or authorization data is missing or ambiguous, skip the operation rather than proceeding. If an API response omits the author field, treat it as "not ours" and do not close the PR. A missed cleanup is recoverable; silently closing someone else's PR is not.

**3. Guard against predictable-name attacks.** Well-known branch names (e.g., `fullsend/onboard`, `fullsend/scaffold-install`) are predictable. An external contributor could open a PR on one of these branches before the automation runs. Without an ownership check, the automation would close a legitimate external PR. Always filter by author or other ownership signal before acting on resources identified by predictable names.

**Positive example:** `closeStaleScaffoldPRs` in `internal/layers/commit.go` demonstrates all three principles. It closes stale scaffold PRs only when the PR author matches the authenticated user (`strings.EqualFold(pr.Author, authenticatedUser)`), skips PRs with an empty author field (fail-closed), and operates on well-known scaffold branch names with full awareness that external actors could create PRs on those paths.

**When reviewing PRs:** Flag any new destructive forge operation (close, delete, merge, force-push via `forge.Client`) that lacks ownership or authorization verification at the call site. This is a medium-severity or higher finding — the same class as a forge abstraction violation.
Loading