alibaba / alibaba/open-code-review

feat(allowlist): exclude extensionless and dotfile secrets (.env, id_rsa, .netrc) from review by default

Closed
#1,240 3 comments 0 reactions 1 assignee Claimed by @hyy321 View on GitHub
enhancement good first issue
Dominant language
Go
Stars
24.4k
Forks
1.8k
Avg merge
2d 6h
Merged PRs (30d)
105

Description

### Problem Statement

Extensionless and dotfile secrets are selected for review by default, and their contents are sent to the LLM provider in the prompt. Nothing in the codebase claims to filter credentials, so this is a missing protection rather than a broken one — but the default is the wrong way round.

`extFromPath` returns `""` when the basename has no dot at a positive index (`internal/scan/agent.go:530`, mirrored in `internal/agent/selection.go:86`), and the allowlist gate only rejects when the detected extension is non-empty:

```go
ext := extFromPath(path)
if ext != "" && !allowedext.IsAllowedExt(ext) {
return model.ExcludeExtension
}
```

A dotfile's only dot is at index 0, so `.env`, `.npmrc` and `.netrc` fall into the same empty-extension path as `Dockerfile` and `Makefile` and skip the allowlist entirely. Nothing downstream compensates: `default_exclude_patterns.json` is 56 test/generated-file patterns with no credential entry, and `providerDirIgnoreDirs` (`internal/diff/git.go:27`) covers IDE and build directories only.

The inversion this produces is the clearest symptom — the harmless template is dropped while the file holding real secrets goes through, because `.example` parses as an extension:

```
Will review (6): Excluded from review (2):
[A] .env [A] .env.example (unsupported_ext)
[A] .netrc [A] server.key (unsupported_ext)
[A] .npmrc
[A] .ssh/id_ed25519
[A] id_rsa
[A] main.go
```

Extensioned key material (`.pem`, `.key`, `.p12`) is already safe — those extensions are not in `supported_file_types.json`.

Two calibrations so the scope is not overstated:

- **A commit is not required.** An untracked `.env` is selected too, so this does not depend on a user having committed key material by mistake.
- **`.gitignore` is an effective defence.** Adding `.env` removes it from the selection set. The real exposure is sensitive files that are *not* gitignored — a forgotten `.env.local`, a mistakenly committed `id_rsa`, `.netrc`, `.npmrc`.

There is also an asymmetry worth noting: `internal/llm/raw.go:147` redacts credential-bearing HTTP headers so OCR's own provider key never reaches a raw log, with a comment stating it deliberately errs toward redacting. Reviewed file content gets no equivalent treatment.

**Reproduction**

```bash
git init repo && cd repo
git config user.email t@t.t && git config user.name t
printf 'package main\nfunc main(){}\n' > main.go
printf 'AWS_SECRET_ACCESS_KEY=wJalrXUtnFEMIexample\n' > .env
printf 'x\n' > .env.example
printf -- '-----BEGIN OPENSSH PRIVATE KEY-----\nfake\n' > id_rsa
mkdir -p .ssh && cp id_rsa .ssh/id_ed25519
printf 'machine api.example.com login me password s3cr3t\n' > .netrc
printf '//registry.npmjs.org/:_authToken=npm_faketoken\n' > .npmrc
printf 'x\n' > server.key
git add -A && git commit -qm init

ocr review --preview --commit HEAD
```

Run with an isolated `HOME` and no project config to confirm it is default behaviour, not local configuration. Output on `v1.12.0 (494bf1c8d) darwin/arm64`:

```
Preview: 8 file(s) changed | +11 -0

Will review (6):
[A] .env +1 -0
[A] .netrc +1 -0
[A] .npmrc +1 -0
[A] .ssh/id_ed25519 +2 -0
[A] id_rsa +2 -0
[A] main.go +2 -0

Excluded from review (2):
[A] .env.example (unsupported_ext)
[A] server.key (unsupported_ext)
```

To confirm the content actually leaves the machine rather than only appearing in the selection set, point `llm.url` at a local HTTP stub that logs request bodies and run `ocr review --commit HEAD`. The plaintext appears in the user message:

```

+AWS_SECRET_ACCESS_KEY=wJalrXUtnFEMIexample

```

`npm_faketoken`, `BEGIN OPENSSH PRIVATE KEY` and the `.netrc` password land in the payload the same way.

### Proposed Solution

Add `internal/config/allowlist/default_secret_patterns.json` and gate well-known credential paths on it by default, so reviewing a repository that contains one does not transmit it to a third-party model without the user having configured anything.

A separate file rather than more entries in `default_exclude_patterns.json`, whose 56 entries all mean "this is test or generated code, reviewing it is noise". Mixing "never send this anywhere" into that list conflates two different reasons for skipping a file, and the security list is the one that should be hard to weaken by accident.

Patterns:

```json
"**/.env", "**/.env.local", "**/.env.*.local",
"**/.ssh/**", "**/id_rsa", "**/id_dsa", "**/id_ecdsa", "**/id_ed25519",
"**/.netrc", "**/_netrc", "**/.npmrc", "**/.pypirc", "**/.dockercfg"
```

`.env.example`, `.env.sample` and `.env.template` stay reviewable — they are meant to be committed, and a review can legitimately catch a real value pasted into one.

A bare `**/credentials` is left out. It is plausible as an ordinary identifier (a Java or Go package directory, a module name), and a global filename match would misfire.

**Design decisions**

- **Not overridable by `include`.** `include` currently outranks the extension allowlist by design (#371), so this needs an explicit carve-out: `include: ["**/.env"]` stays blocked. The protection has to hold without configuration, which means a broad glob written for an unrelated reason must not defeat it.
- **No opt-out in the first iteration.** If these files do not need reviewing, nothing needs an escape hatch. Should a concrete case turn up, it should be an explicit `allow_secret_paths` glob list — per-path and auditable — rather than a boolean that disables the whole set, and never `include`.
- **Reported as its own reason.** Preview shows `secret_exclude`, not `user_exclude`, so the user can see the protection engage and tell it apart from their own config. The secret gate runs first, ahead of the user `exclude` check, so the reason is deterministic rather than depending on what else happens to match. No extra warning on a normal run — it appears wherever other exclusions already appear.
- **Both call sites.** `internal/agent/selection.go` for diff review and `internal/scan/agent.go` for `ocr scan`, which walks whole files rather than diffs and is the more exposed of the two.
- **Paths only.** A real key pasted into an ordinary `.go` file is untouched by any path rule; content-based detection carries a false-positive cost and belongs in its own issue if it is wanted at all.

### Alternatives Considered

- **Extend `default_exclude_patterns.json`.** Fewer moving parts, but conflates "noise" with "never transmit", and puts security patterns in a file contributors routinely append language-specific test globs to.
- **Fix `extFromPath` so dotfiles report an extension.** Treating `.env` as extension `.env` would make the allowlist reject it. But it also changes how every dotfile and extensionless file is classified, and would break the `Dockerfile`/`Makefile` behaviour that #440 shows people rely on. The empty-extension path is a deliberate design, not the fault here.
- **Gate the built-in tools as well.** `FileReader` (`internal/tool/filereader.go:94`) enforces only repo-root containment, and nothing under `internal/tool/` consults the allowlist, so `file_read(".env")` returns the plaintext even for a path the selection gate dropped. Out of scope on purpose: this is not a confinement boundary. The claim is that these files do not need reviewing — not that the model must be stopped from reading a repository it was deliberately pointed at, which it already reads only inside the root and only when the review calls for a path. Threading a filter through four tools and defining what a refused read returns to the model is machinery out of proportion to that.
- **Documentation only.** Tell users to add their own `exclude` entries. Insufficient: it requires knowing the behaviour exists before it costs you something, and the failure is one-way — once the request is sent, it is sent.
- **Reuse the `raw.go` keyword approach.** Substring matching on filenames (`key`, `secret`, `token`) would catch far more than intended — `keyboard.go`, `tokenizer.go`, `secretary.ts`. Explicit globs are the right granularity for paths.

### Affected Area

Review Rules, Configuration

### AI/LLM Disclosure

Investigated and written with AI assistance (Claude Code, Opus 5). The model read the filtering code, ran the reproduction above including the stub-provider capture, and drafted this report; every claim was verified by execution against `v1.12.0 (494bf1c8d)` rather than taken from the model's description of the code.

### Additional Context

Raised by @basil-k-aji-dev in #440 as a side observation while answering a question about extensionless files, with an offer to file it separately. Opening it here as its own issue, since #440 is an Ideas thread about content-based rule matching and this is a separate default-behaviour question. The mechanism is the same empty-extension path that makes `Dockerfile` reviewable without an `include` entry.

Filed publicly rather than as a private advisory: there is no third-party attack surface — the transmission is from the user to a provider the user configured, and an attacker cannot trigger it — and the behaviour was already described publicly in #440. A public issue also lets users add their own `exclude` entries in the meantime.

Related documentation gap, worth a separate issue: `pages/src/content/docs/en/review-rules.md` does not explain that extensionless files bypass the allowlist, or the distinction that a `rules` entry selects guidance while `include` decides eligibility. Both community members in #440 had to read the source and run `--preview` to establish this.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.