apply ignores scope flags and can rewrite files outside the requested include set #14

New Issue

2026-06-04T16:19:17Z

KofTwentyTwo commented

2026-06-04 16:19:17 +00:00

(Migrated from github.com)

Finding

The shared scope flags are wired into command/config structs, but apply drops include and gitignore-layering information before enumeration. A scoped write can still mutate the whole eligible source set.

Evidence

cmd/license-tool/commands.go:48 to cmd/license-tool/commands.go:50 registers include, exclude, and no-gitignore flags.
cmd/license-tool/commands.go:415 to cmd/license-tool/commands.go:425 copies include and no-gitignore into config.Flags.
internal/config/config.go:406 to internal/config/config.go:431 merges only license, holder, year, style, and excludes.
internal/applier/applier.go:94 to internal/applier/applier.go:97 enumerates with excludes and force only.
internal/enumerate/enumerate.go:27 to internal/enumerate/enumerate.go:36 already supports the missing scope controls.

Reproduction

Create a clean git repo with two supported source files, for example a.go and b.go.
Run license-tool apply <repo> with write mode and include restricted to a.go.
Observe that both files receive managed headers.

Why It Matters

The include flag is a safety boundary for bulk source rewrites. Ignoring it can mutate files the operator explicitly intended to leave untouched.

Suggested Fix

Move runtime scan scope into an explicit options struct shared by audit and apply. Pass include and no-gitignore through to enumerate.Enumerate in the apply path, and add command-level tests that assert excluded files remain unchanged in write mode.

## Finding The shared scope flags are wired into command/config structs, but `apply` drops include and gitignore-layering information before enumeration. A scoped write can still mutate the whole eligible source set. ## Evidence - `cmd/license-tool/commands.go:48` to `cmd/license-tool/commands.go:50` registers include, exclude, and no-gitignore flags. - `cmd/license-tool/commands.go:415` to `cmd/license-tool/commands.go:425` copies include and no-gitignore into `config.Flags`. - `internal/config/config.go:406` to `internal/config/config.go:431` merges only license, holder, year, style, and excludes. - `internal/applier/applier.go:94` to `internal/applier/applier.go:97` enumerates with excludes and force only. - `internal/enumerate/enumerate.go:27` to `internal/enumerate/enumerate.go:36` already supports the missing scope controls. ## Reproduction 1. Create a clean git repo with two supported source files, for example `a.go` and `b.go`. 2. Run `license-tool apply <repo>` with write mode and include restricted to `a.go`. 3. Observe that both files receive managed headers. ## Why It Matters The include flag is a safety boundary for bulk source rewrites. Ignoring it can mutate files the operator explicitly intended to leave untouched. ## Suggested Fix Move runtime scan scope into an explicit options struct shared by audit and apply. Pass include and no-gitignore through to `enumerate.Enumerate` in the apply path, and add command-level tests that assert excluded files remain unchanged in write mode.

KofTwentyTwo commented

2026-06-04 16:29:33 +00:00

(Migrated from github.com)

Status

Confirmed. Reproduced end-to-end: apply --write --include 'a.go' wrote managed headers to both a.go and b.go, while audit --include 'a.go' correctly reports only 1 source file. The --include and --no-gitignore scope controls reach config resolution but are dropped before apply's enumeration.

Severity

High (matches the bug label; I'd argue toward the top of it). This is a scope-escape that mutates source files in write mode — the operator's explicit safety boundary is silently ignored, so a narrowly-scoped run can rewrite the entire eligible source set. Not critical only because AtomicWrite preserves content integrity (it splices headers, never truncates) and a clean-tree gate makes the damage trivially git checkout-recoverable.

Verified root cause

Two layers drop the scope, but the operative defect is one missing pass-through:

internal/applier/applier.go:94-97 — Apply calls enumerate.Enumerate(path, enumerate.Options{Excludes: cfg.Excludes, Force: opts.Force}, ...). It sets only Excludes and Force; Includes and NoGitignore are never set, so they default to nil/false. This is the actual bug site.
internal/applier/applier.go:67 — Apply(path string, cfg model.Config, opts Options) has no channel to receive include/no-gitignore: model.Config carries neither field (internal/model/model.go:226-234 has ManageLicenseFile, Excludes, FileTypeOverrides but no Include/NoGitignore), and applier.Options (applier.go:32-45) doesn't either. So the data is unreachable from inside Apply as currently wired.
internal/config/config.go:407-432 — mergeFlags merges only License/Holder/Year/Style/Exclude into model.Config; it never propagates flags.Include/flags.NoGitignore (which are populated by applyToFlags, commands.go:415-425). So even though config.Flags carries them, they evaporate at resolution.

For contrast, the audit/check path works because buildAuditPipeline (cmd/license-tool/commands.go:353-385, specifically the enumerate.Options{Includes: shared.include, NoGitignore: shared.noGitignore, ...} at lines 357-361) reads the flags directly off sharedFlags, bypassing model.Config entirely. enumerate.Enumerate already honors both controls (internal/enumerate/enumerate.go:28-37, matchesIncludes at :200-214, walkListFiles NoGitignore gate at :262), so the enumeration layer is fine — only apply's call site starves it.

Suggested fix

Carry runtime scan scope to Apply and forward it to enumerate.Enumerate. Cleanest seam, mirroring how audit already does it: add Includes []string and NoGitignore bool to applier.Options (applier.go:32-45), populate them from shared.include/shared.noGitignore at the applier.Apply call site in newApplyCmd (commands.go:190-197), and pass them into the enumerate.Options{} literal at applier.go:94-97. This keeps include/no-gitignore as runtime scan scope (consistent with audit) rather than leaking them into model.Config, which is the persisted/file-backed shape and deliberately has no such fields.

Touch both boundaries: the same enumerate.Options{} is built in ManageLicenseFiles? — no, but note newLicenseCmd (commands.go:214-228) calls ManageLicenseFiles, which only manages the top-level LICENSE/LICENSES files and does not enumerate source, so it needs no change. The single enumeration call site that needs scope is applier.go:94-97. (This differs from the v0.2.1 universal-shebang precedent, which had to touch two symmetric preserve-boundary computations in render.preserveBoundary + detect.PreserveBoundary plus the filetype table — that pattern does not apply here; #14 is a single missing pass-through, no filetype-table or boundary change involved.)

To keep the 100% gate (.testcoverage.yml) green: add a command-level test asserting that apply --write --include <one-file> leaves the non-included sibling byte-identical, plus a --no-gitignore apply case, so both new Options fields are exercised on the write path.

Corrections to the issue

Over-broad root-cause framing. The body lists config.go:406-431 and "merges only license, holder, year, style, and excludes" as part of the defect. That is accurate as written, but it implies the fix belongs in config/mergeFlags. The minimal, correct fix is the applier.go:94-97 enumeration call (forward via applier.Options); routing include/no-gitignore through model.Config would be the wrong layer (it's the persisted config shape).
config.go:406-431 line ref: mergeFlags spans :407-432 (doc comment at :406, closing brace at :432). Minor.
Otherwise the evidence chain (commands.go:48-50 flag registration, :415-425 applyToFlags populating Include/NoGitignore, enumerate.go:27-36 supporting the controls) is accurate.

Open questions

--include semantics for apply: audit's matchesIncludes (enumerate.go:200-214) matches against both full relative path and basename. Confirm apply should inherit the identical matching semantics (it should, for consistency) — the maintainer just needs to bless that there's no apply-specific include behavior expected.
Scope of the fix: confirm --no-gitignore should also be forwarded on the apply path (it has the same drop), or whether #14 is intended to cover --include only. The body's title says "scope flags" plural and evidence cites no-gitignore, so I'd fix both.

Overlaps

None found among the cited code. This is isolated to the apply enumeration call site; it does not touch the preserve-boundary/INSERT path or the v0.2.1 universal-shebang area.

Automated maintainer review.

## Status **Confirmed.** Reproduced end-to-end: `apply --write --include 'a.go'` wrote managed headers to **both** `a.go` and `b.go`, while `audit --include 'a.go'` correctly reports only 1 source file. The `--include` and `--no-gitignore` scope controls reach config resolution but are dropped before apply's enumeration. ## Severity **High** (matches the `bug` label; I'd argue toward the top of it). This is a *scope-escape that mutates source files in write mode* — the operator's explicit safety boundary is silently ignored, so a narrowly-scoped run can rewrite the entire eligible source set. Not `critical` only because `AtomicWrite` preserves content integrity (it splices headers, never truncates) and a clean-tree gate makes the damage trivially `git checkout`-recoverable. ## Verified root cause Two layers drop the scope, but the operative defect is one missing pass-through: 1. **`internal/applier/applier.go:94-97`** — `Apply` calls `enumerate.Enumerate(path, enumerate.Options{Excludes: cfg.Excludes, Force: opts.Force}, ...)`. It sets **only** `Excludes` and `Force`; `Includes` and `NoGitignore` are never set, so they default to nil/false. This is the actual bug site. 2. **`internal/applier/applier.go:67`** — `Apply(path string, cfg model.Config, opts Options)` has no channel to receive include/no-gitignore: `model.Config` carries neither field (`internal/model/model.go:226-234` has `ManageLicenseFile`, `Excludes`, `FileTypeOverrides` but no `Include`/`NoGitignore`), and `applier.Options` (applier.go:32-45) doesn't either. So the data is unreachable from inside `Apply` as currently wired. 3. **`internal/config/config.go:407-432`** — `mergeFlags` merges only License/Holder/Year/Style/Exclude into `model.Config`; it never propagates `flags.Include`/`flags.NoGitignore` (which *are* populated by `applyToFlags`, commands.go:415-425). So even though `config.Flags` carries them, they evaporate at resolution. For contrast, the audit/check path works because `buildAuditPipeline` (`cmd/license-tool/commands.go:353-385`, specifically the `enumerate.Options{Includes: shared.include, NoGitignore: shared.noGitignore, ...}` at lines 357-361) reads the flags **directly off `sharedFlags`**, bypassing `model.Config` entirely. `enumerate.Enumerate` already honors both controls (`internal/enumerate/enumerate.go:28-37`, `matchesIncludes` at :200-214, `walkListFiles` NoGitignore gate at :262), so the enumeration layer is fine — only apply's call site starves it. ## Suggested fix Carry runtime scan scope to `Apply` and forward it to `enumerate.Enumerate`. Cleanest seam, mirroring how audit already does it: add `Includes []string` and `NoGitignore bool` to `applier.Options` (applier.go:32-45), populate them from `shared.include`/`shared.noGitignore` at the `applier.Apply` call site in `newApplyCmd` (commands.go:190-197), and pass them into the `enumerate.Options{}` literal at applier.go:94-97. This keeps include/no-gitignore as *runtime scan scope* (consistent with audit) rather than leaking them into `model.Config`, which is the persisted/file-backed shape and deliberately has no such fields. Touch **both** boundaries: the same `enumerate.Options{}` is built in `ManageLicenseFiles`? — no, but note `newLicenseCmd` (commands.go:214-228) calls `ManageLicenseFiles`, which only manages the top-level LICENSE/LICENSES files and does not enumerate source, so it needs no change. The single enumeration call site that needs scope is applier.go:94-97. (This differs from the v0.2.1 universal-shebang precedent, which had to touch two symmetric preserve-boundary computations in `render.preserveBoundary` + `detect.PreserveBoundary` plus the filetype table — that pattern does **not** apply here; #14 is a single missing pass-through, no filetype-table or boundary change involved.) To keep the 100% gate (`.testcoverage.yml`) green: add a command-level test asserting that `apply --write --include <one-file>` leaves the non-included sibling byte-identical, plus a `--no-gitignore` apply case, so both new `Options` fields are exercised on the write path. ## Corrections to the issue - **Over-broad root-cause framing.** The body lists config.go:406-431 and "merges only license, holder, year, style, and excludes" as part of the defect. That is accurate as written, but it implies the fix belongs in `config`/`mergeFlags`. The minimal, correct fix is the `applier.go:94-97` enumeration call (forward via `applier.Options`); routing include/no-gitignore through `model.Config` would be the wrong layer (it's the persisted config shape). - **`config.go:406-431` line ref:** `mergeFlags` spans :407-432 (doc comment at :406, closing brace at :432). Minor. - Otherwise the evidence chain (commands.go:48-50 flag registration, :415-425 applyToFlags populating Include/NoGitignore, enumerate.go:27-36 supporting the controls) is accurate. ## Open questions - **`--include` semantics for apply:** audit's `matchesIncludes` (enumerate.go:200-214) matches against both full relative path and basename. Confirm apply should inherit the identical matching semantics (it should, for consistency) — the maintainer just needs to bless that there's no apply-specific include behavior expected. - **Scope of the fix:** confirm `--no-gitignore` should also be forwarded on the apply path (it has the same drop), or whether #14 is intended to cover `--include` only. The body's title says "scope flags" plural and evidence cites no-gitignore, so I'd fix both. ## Overlaps None found among the cited code. This is isolated to the apply enumeration call site; it does not touch the preserve-boundary/INSERT path or the v0.2.1 universal-shebang area. --- _Automated maintainer review._

KofTwentyTwo commented

2026-06-04 20:28:09 +00:00

(Migrated from github.com)

Resolved on develop in commit 742c124e6ec2afa9312075db3112bc2def3c91ff.

Validation:

CI: success, https://github.com/KofTwentyTwo/license-tool/actions/runs/26977377097
CodeQL: success, https://github.com/KofTwentyTwo/license-tool/actions/runs/26977376421
Coverage gate: 100% (2087/2087)

Closing as fixed.

Resolved on `develop` in commit `742c124e6ec2afa9312075db3112bc2def3c91ff`. Validation: - CI: success, https://github.com/KofTwentyTwo/license-tool/actions/runs/26977377097 - CodeQL: success, https://github.com/KofTwentyTwo/license-tool/actions/runs/26977376421 - Coverage gate: 100% (2087/2087) Closing as fixed.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: KofTwentyTwo/license-tool#14