Rodin
823265659a
CI / test (push) Successful in 18s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (push) Has been skipped
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (push) Has been skipped
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (push) Has been skipped
126 lines
4.8 KiB
Markdown
126 lines
4.8 KiB
Markdown
# PLAN-143: Load doc-map config from trusted (default) branch
|
|
|
|
**Issue:** #143
|
|
**Status:** Planning
|
|
**Branch:** TBD (issue-143)
|
|
|
|
---
|
|
|
|
## Problem Statement
|
|
|
|
The `--doc-map` flag reads the doc-map YAML config from the local `GITHUB_WORKSPACE` checkout, which is the **PR branch** in CI. A malicious PR author can:
|
|
|
|
1. Modify `.review-bot/doc-map.yml` in their branch to map any path glob to sensitive docs
|
|
2. review-bot reads the PR-branch doc-map config
|
|
3. Docs from the **default branch** are fetched and injected into the LLM prompt
|
|
4. Via prompt injection in those docs, the attacker could exfiltrate content
|
|
|
|
The config is the trust boundary. The *data* fetched (design docs) already comes from the default branch via VCS API. The *config* is what needs to be pinned to the default branch.
|
|
|
|
## Constraints
|
|
|
|
- Must not break existing callers (backward compatibility)
|
|
- Should have a clearly named flag/env var
|
|
- Fall back to local workspace if no trusted ref configured (for users not yet migrated)
|
|
- The gargoyle workflow (.github/workflows/review.yml) will need updating
|
|
|
|
## Proposed Approach
|
|
|
|
### Option A: Fetch via VCS API from default branch (preferred)
|
|
|
|
Add a new flag `--doc-map-trusted-ref` (default: `""` = use local workspace).
|
|
|
|
When `--doc-map-trusted-ref` is set:
|
|
1. Use the VCS API to fetch the file at `--doc-map` path from the specified ref
|
|
2. Parse the fetched content as YAML
|
|
3. Use this config (not the local workspace copy)
|
|
|
|
When `--doc-map-trusted-ref` is empty:
|
|
- Current behavior (local workspace) with a deprecation warning
|
|
|
|
This follows the same pattern as `patterns-repo` which fetches from VCS.
|
|
|
|
### Option B: Auto-detect and always use default branch
|
|
|
|
Always fetch doc-map from the default branch via VCS API, ignoring local workspace.
|
|
Simpler API but breaks local testing (where there's no VCS to fetch from).
|
|
|
|
### Recommendation
|
|
|
|
Option A — explicit `--doc-map-trusted-ref` flag. The gargoyle workflow would set:
|
|
```yaml
|
|
doc-map-trusted-ref: "main"
|
|
```
|
|
|
|
This is explicit and allows local testing to continue using local workspace.
|
|
|
|
## Implementation Plan
|
|
|
|
### Phase 1: VCS API fetch for doc-map config
|
|
|
|
**Files to change:**
|
|
- `cmd/review-bot/main.go` — add `--doc-map-trusted-ref` flag, conditional fetch logic
|
|
- `review/docmap.go` — add `FetchDocMapConfig(vcs, owner, repo, ref, path string) (*DocMapConfig, error)`
|
|
- `action.yml` — add `doc-map-trusted-ref` input
|
|
- `README.md` — document new flag
|
|
|
|
**Logic:**
|
|
```go
|
|
if *docMapTrustedRef != "" {
|
|
// Fetch from VCS (trusted branch) — secure
|
|
content, err := vcs.GetFileContent(ctx, owner, repoName, *docMapTrustedRef, resolvedDocMap)
|
|
...
|
|
docMapCfg, err = review.ParseDocMapConfigContent(content)
|
|
} else {
|
|
// Local workspace (backward compat with deprecation warning)
|
|
slog.Warn("doc-map loaded from local workspace (PR branch) — consider --doc-map-trusted-ref for security")
|
|
docMapCfg, err = review.ParseDocMapConfig(resolvedDocMap)
|
|
}
|
|
```
|
|
|
|
### Phase 2: Tests
|
|
|
|
- `TestFetchDocMapConfig_Success`: mock VCS returns valid YAML → parses correctly
|
|
- `TestFetchDocMapConfig_NotFound`: VCS returns 404 → clear error
|
|
- `TestMainSubprocess_DocMapTrustedRef`: subprocess test for the new flag
|
|
|
|
### Phase 3: Gargoyle workflow update
|
|
|
|
Update `.github/workflows/review.yml` in gargoyle to add `doc-map-trusted-ref: main`.
|
|
|
|
## State/Data Model
|
|
|
|
New flag: `--doc-map-trusted-ref` / `DOC_MAP_TRUSTED_REF` env var
|
|
- Type: string
|
|
- Default: `""` (local workspace)
|
|
- Example value: `"main"`, `"master"`, `HEAD`
|
|
|
|
## Error Cases
|
|
|
|
- VCS returns 404 for doc-map path at trusted ref → error + exit (not silent)
|
|
- VCS returns 404 but local copy exists → do NOT fall back (could be attack path)
|
|
- Parse error on fetched content → error + exit
|
|
|
|
## Edge Cases
|
|
|
|
- What if the doc-map doesn't exist at the trusted ref? → log error, exit (don't silently continue)
|
|
- What if trusted-ref is a commit SHA? → should work via VCS GetFileContent
|
|
- What if the user sets trusted-ref to the PR branch? → Works, but defeats the purpose. Not our problem to prevent.
|
|
|
|
## Open Questions
|
|
|
|
- Should we warn when `--doc-map` is set without `--doc-map-trusted-ref`? → Yes, deprecation warning pointing to docs
|
|
- Should we add `--doc-map-trusted-ref` to the `validate-docmap` subcommand? → No, that subcommand operates on local files only; it's a developer tool
|
|
|
|
## Acceptance Criteria
|
|
|
|
- [ ] `--doc-map-trusted-ref` flag added to `action.yml` and `cmd/review-bot/main.go`
|
|
- [ ] When set, doc-map config fetched from VCS at the specified ref (not local workspace)
|
|
- [ ] When unset, local workspace used with deprecation warning in logs
|
|
- [ ] 404 from VCS is a hard error (no silent fallback to local copy)
|
|
- [ ] Tests cover: fetch success, fetch 404, parse error
|
|
- [ ] Gargoyle `.github/workflows/review.yml` updated to use `doc-map-trusted-ref: main`
|
|
- [ ] README updated
|
|
- [ ] CHANGELOG updated
|
|
- [ ] `make precommit` passes
|