-
Notifications
You must be signed in to change notification settings - Fork 152
Expand file tree
/
Copy path.roborev.toml
More file actions
162 lines (139 loc) · 9.12 KB
/
Copy path.roborev.toml
File metadata and controls
162 lines (139 loc) · 9.12 KB
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
review_agent = ""
review_guidelines = """
The daemon and client evolve in lockstep; the daemon is restarted after
updates. API changes do not require backward compatibility shims.
TUI overflow on very narrow terminals (< 60 chars) is acceptable; omit
pedantic comments on these extreme scenarios.
## Trust model — local-only tool
roborev is a single-user local CLI. The daemon binds to 127.0.0.1, the
database is local SQLite, and all data (repo names, paths, review content,
hook output, job metadata) originates from the operator's own filesystem and
git repos. Only flag injection/sanitization issues for data from external
sources (e.g., commit messages from shared repos, API responses from remote
services). The following are all local-data-in-local-tool and are NOT
security findings:
- Missing auth on daemon endpoints (only client is the local CLI; OS-level
access control is sufficient)
- "DoS" from request bodies or thundering-herd on loopback (bounded by the
user's own repos, not external input)
- "Data exposure" from endpoints returning review content (user's own data
on their own machine)
- "Prompt injection" from hook stderr, prior review text, or agent output
embedded in prompts (all locally generated; a compromised local agent
already has full shell access)
- "Secret exposure" from hook stderr logged or forwarded to the model
- "Untrusted remap data" or "history tampering" from the remap endpoint (the
client is the user's own post-rewrite hook, data comes from the user's own
git repo, and the daemon is localhost-only)
- .githooks/ tracked as a supply-chain risk (source templates only; installed
hooks are frozen copies in .git/hooks/, unaffected by branch switches)
- "Argument injection" on internal helpers (e.g., git.GetPatchID,
git.GetCommitInfo) that receive SHAs already validated or resolved at the
call site — validation belongs at trust boundaries, not every internal call
- Race conditions in metadata handoff between CLI and daemon (correctness
concern, not exploitable by external attacker)
- "Markerless" hook blocks or interpreter mismatch — hook install/upgrade uses
marker-based detection, every generated hook includes a marker comment, and
append logic refuses to add shell snippets to non-shell hooks
- Symlink-following in hook read/write under .git/hooks/ (controlled by the
local user; a compromised local filesystem is out of scope)
## Docs publication assets
Docs media is published from maintainer-controlled orphan branches
(`docs-assets` and `docs-generated-assets`). Treat these refs as trusted
publication artifacts, not untrusted PR input: an actor who can force-push them
already has repo publication authority. Do not report speculative build-host file
exposure from malicious assets in those branches as a security vulnerability
unless the code path accepts asset content from untrusted PR input.
It is still valid to flag docs correctness and reproducibility issues, such as
silently using stale ignored assets, hydrating from the wrong ref, missing
expected files, or accidentally publishing unintended local files.
## Agent tool permissions
Review agents run with shell access to inspect diffs and source files. Codex
runs with --sandbox danger-full-access because the bwrap sandbox (used by both
read-only and workspace-write modes) blocks loopback networking, which prevents
git commands (git diff, git log, git show) from working in CI review jobs.
This is intentional — roborev is a local-only tool that runs in trusted
environments where the code is the operator's own. The allow-all-tools plus
deny-list approach used by agents like Copilot matches this trust level. The
deny-list is defense-in-depth, not a security boundary. Users reviewing
untrusted code are expected to run roborev in a sandboxed environment
(container, VM). Do not flag agent sandbox or tool permission patterns as
security vulnerabilities when they follow this model.
## Config loading and filesystem fallback
Config loading (loadGuidelines, loadCIRepoConfig) reads .roborev.toml from
the default branch and falls back to the working-tree filesystem when the
default branch has no config (no remote, config not yet committed). The
daemon never checks out PR branches — the working tree is always the
operator's own checkout. Do not flag filesystem fallback as a trust-boundary
issue or claim that PR authors can influence config via the working tree.
## CI pipeline mode (`roborev ci review`, GitHub Actions and GitLab CI)
The CI path has a documented trust model in docs/integrations/gitlab.md. Read
it before reporting a finding against `cmd/roborev/ci.go`, `internal/gitlab/`,
or the agent environment scrub, and do not re-report what it already states as
an accepted residual:
- The environment scrub keeps forge tokens out of agent subprocesses and
capability probes. It is a bound on the casual channel, not a sandbox. Agents
run as the same user as roborev, so `/proc/<ppid>/environ`, debuggers, and
similar same-UID paths remain reachable by design. Do not ask for UID/PID
isolation, container-per-agent, or a minimal environment built from trusted
config: that is an architectural change to how every agent in the project
runs, not a CI fix, and it applies identically to the pre-existing GitHub
path.
- Loader variables (`LD_PRELOAD`, `NODE_OPTIONS`, ...) are stripped from
processes roborev spawns. roborev cannot defend against one set in its own
job environment, because the dynamic loader applies it before roborev's code
runs. That mitigation is operator-side and documented.
- A default GitLab Runner leaves the job token in the checkout's `.git/config`.
roborev does not rewrite the user's git config; the fix is
`FF_GIT_URLS_WITHOUT_TOKENS` or stripping the remote, and it is documented.
- `SSL_CERT_FILE`/`SSL_CERT_DIR` stay honored so self-hosted instances behind
an internal CA keep working. A substituted CA alone is not interception.
- The notes API has no conditional write, so any head check before posting
leaves a one-round-trip TOCTOU window. Rechecking after the write only moves
it. The note body names the reviewed commit, which is the intended mitigation.
- `.roborev.toml` is read from the tree under review in the MR-head worktree
flow, so its author controls `[ci]` settings. Flags outrank the file and the
docs say to pin what must not be author-controlled. Report a *specific*
setting that is unpinnable and has a security consequence; do not re-report
the general class.
- Provider routing (`OPENAI_BASE_URL`, `GH_HOST`, `ANTHROPIC_BASE_URL`, a proxy
URL in a model spec, ACP `command`) is deliberately configurable, and agents
must be able to read provider credentials or they cannot review anything.
Redirecting a provider endpoint in an untrusted pipeline is a known,
documented gap that spans every agent and both forges; it needs one design
for trusted provider routing, not per-variable stripping bolted onto the
GitLab path. Do not re-report it as a GitLab CI finding.
- `MergeRequestDiffRefs` is a value type in the client library, not a pointer.
`mr.DiffRefs` cannot be nil and `diff_refs: null` unmarshals to the zero
struct, which the empty-base check already rejects. This has been reported
twice; it is not a nil dereference.
Findings about the GitLab MR binding (that the reviewed range must match the
merge request the note lands on) are welcome when they identify a concrete
range or head that would slip through. Prefer the smallest fix that closes it.
Name the actor when reporting a CI finding: "an attacker can set X" is only
meaningful with who they are and what they already hold. In the unprotected
setup the merge request author already holds the token, so redirecting anything
gains them nothing; in the protected setup the actor is someone who can start a
protected pipeline with variables but cannot change the job script. A finding
that does not distinguish these is not actionable.
## Verdict parsing and review output shape
Verdict parsing is intentionally simple and deterministic. Prefer explicit
structured signals such as severity labels, findings sections, and clear pass
phrases. Do not ask for elaborate natural-language contradiction parsing of
free-form prose after "No issues found." or similar pass text.
If agent review output is too chatty, includes process narration, or mixes
findings with summary/front matter in a way that makes verdict detection less
reliable, the fix should be to tighten the review prompts/templates and output
contract. Do not ask for increasingly broad deterministic heuristics to parse
arbitrary narrative text.
## Compilation, imports, and build errors
Do not flag suspected compile errors, missing imports, undeclared
identifiers, type mismatches, or other build-level issues. The local
toolchain (go build, go vet, golangci-lint) and the pre-commit hook
catch these before any commit lands; if the diff actually fails to
compile, the PR cannot merge regardless of what the review says.
Reviews see only the diff, not the rest of the package, so claims like
"function X is not defined" or "package Y is not imported" are almost
always wrong — the symbol exists elsewhere in the file or package.
Focus reviews on logic, architecture, and behavior; trust the build.
"""