invalid dependency resolver tier values are silently accepted #25

New Issue

2026-06-04T16:19:31Z

KofTwentyTwo commented

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

(Migrated from github.com)

Finding

The resolver tier flag advertises a fixed set of accepted values, but command execution does not validate the value. Unknown tokens are treated like the default on-disk path.

Evidence

cmd/license-tool/commands.go:151 to cmd/license-tool/commands.go:157 binds the resolver tier as a plain string.
cmd/license-tool/commands.go:98 to cmd/license-tool/commands.go:103 passes the raw value into report options and only checks whether it equals tool.
internal/report/report.go:321 to internal/report/report.go:333 treats any value other than off as dependency resolution enabled.

Reproduction

Run audit with a resolver tier value that is not one of the documented tokens.
Observe that the command exits successfully and behaves like on-disk resolution rather than rejecting the bad value.

Why It Matters

A typo in CI can silently change dependency-resolution behavior. This is especially risky for a compliance-oriented command where resolver tier affects audit completeness.

Suggested Fix

Validate the resolver tier at flag parsing or command execution time. Reject unknown values with a usage error and add command tests for accepted and rejected tokens.

## Finding The resolver tier flag advertises a fixed set of accepted values, but command execution does not validate the value. Unknown tokens are treated like the default on-disk path. ## Evidence - `cmd/license-tool/commands.go:151` to `cmd/license-tool/commands.go:157` binds the resolver tier as a plain string. - `cmd/license-tool/commands.go:98` to `cmd/license-tool/commands.go:103` passes the raw value into report options and only checks whether it equals `tool`. - `internal/report/report.go:321` to `internal/report/report.go:333` treats any value other than `off` as dependency resolution enabled. ## Reproduction 1. Run audit with a resolver tier value that is not one of the documented tokens. 2. Observe that the command exits successfully and behaves like on-disk resolution rather than rejecting the bad value. ## Why It Matters A typo in CI can silently change dependency-resolution behavior. This is especially risky for a compliance-oriented command where resolver tier affects audit completeness. ## Suggested Fix Validate the resolver tier at flag parsing or command execution time. Reject unknown values with a usage error and add command tests for accepted and rejected tokens.

KofTwentyTwo commented

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

(Migrated from github.com)

Status: confirmed — an unknown --resolve-deps token exits 0 and runs dependency resolution as if ondisk; only off (and tool, for shell-out) are special-cased, so every other string is silently accepted.

Severity: low (label is bug, no severity label; low is the right call). Default --deps=true + default --resolve-deps=ondisk means a typo lands on the same on-disk behavior the user almost certainly wanted, so audit completeness is unchanged for the common case. The one genuinely surprising path is --resolve-deps=of (typo for off): the user intends to disable resolution and instead silently re-enables it. That's a real footgun for a compliance gate but narrow in blast radius — hence low, not medium.

Verified root cause: there is no tier validator anywhere. Repro: built main (v0.2.1), ran audit . --resolve-deps=bogus-typo-tier --format=json in a temp dir → exit 0, full report rendered, dependencies: [] (resolution path entered). For contrast --format=bogus-fmt correctly exits 2. The defect lives across three spots, all matching the issue's cites:

cmd/license-tool/commands.go:bindAuditFlags:156 — cmd.Flags().StringVar(&f.resolveDeps, "resolve-deps", "ondisk", ...) binds a raw string with no validation.
cmd/license-tool/commands.go:newAuditCmd:101-102 and newCheckCmd:133-134 — the raw value is forwarded into report.Options{ResolveDeps: f.resolveDeps, AllowToolShellOut: f.resolveDeps == "tool"}; the only comparison is == "tool".
internal/report/report.go:Audit:324 — if opts.IncludeDeps && opts.ResolveDeps != "off" && pipe.ResolveDeps != nil — the gate is != "off", so any non-off token (including garbage) enables resolution. ParseFormat (report.go:48) is the only token validator in the package; the tier has no equivalent.

Suggested fix: add a report.ParseResolveDeps(raw string) (string, error) mirroring the existing ParseFormat precedent (report.go:48) — accept ondisk/tool/off (and "" → ondisk), reject everything else with fmt.Errorf("report: unknown resolver tier %q", raw). Call it in both command RunE bodies (newAuditCmd ~line 94 and newCheckCmd ~line 126, right next to the existing ParseFormat calls) before constructing report.Options, so audit and check reject identically — this is the same dual-boundary discipline the v0.2.1 universal-shebang fix applied (both the INSERT path in internal/render/render.go and the PreserveBoundary in internal/detect/detect.go). Note there is no filetype-table analog here (tier values aren't per-language), so the "filetype table" leg of the usual three-touch pattern doesn't apply to this issue. Keep the 100% gate green: add cmd and/or report tests exercising each accepted token plus at least one rejected token (the report_test.go ParseFormat table at line 27 is the template). Validating centrally in report also lets AllowToolShellOut stay derived from the now-validated value.

Corrections to the issue: none material. Line references (commands.go 151-157 / 98-103, report.go 321-333) all land on the right code; the exact decision points are commands.go:156 + 102/134 and report.go:324. One framing nuance worth flagging for the maintainer (not an error in the body): impact is narrower than "silently change dependency-resolution behavior" implies — an unknown token resolves to the default (ondisk) behavior, so it only diverges from intent when the user meant off; it does not, e.g., silently turn on tool shell-out (that still requires the literal tool).

Open questions:

Where should validation live — in the cmd layer (parse-time, like ParseFormat) or defensively in report.Audit as well? Doing both is safest but the cmd-layer parse is the user-facing one; pick one as the source of truth to avoid a redundant untested branch fighting the 100% gate.
Should --resolve-deps also imply/interact with --deps? Today --deps=false makes the tier moot (report.go:324 short-circuits on IncludeDeps), so --deps=false --resolve-deps=off is redundant — decide whether to leave that as-is or warn.
Empty-string handling: confirm "" should map to ondisk (matching the flag default) rather than erroring, since the default is set at bind time and "" shouldn't normally reach the parser.

Overlaps: none found among the cited code paths.

Automated maintainer review.

**Status**: confirmed — an unknown `--resolve-deps` token exits 0 and runs dependency resolution as if `ondisk`; only `off` (and `tool`, for shell-out) are special-cased, so every other string is silently accepted. **Severity**: low (label is `bug`, no severity label; low is the right call). Default `--deps=true` + default `--resolve-deps=ondisk` means a typo lands on the same on-disk behavior the user almost certainly wanted, so audit *completeness* is unchanged for the common case. The one genuinely surprising path is `--resolve-deps=of` (typo for `off`): the user intends to disable resolution and instead silently re-enables it. That's a real footgun for a compliance gate but narrow in blast radius — hence low, not medium. **Verified root cause**: there is no tier validator anywhere. Repro: built `main` (`v0.2.1`), ran `audit . --resolve-deps=bogus-typo-tier --format=json` in a temp dir → exit 0, full report rendered, `dependencies: []` (resolution path entered). For contrast `--format=bogus-fmt` correctly exits 2. The defect lives across three spots, all matching the issue's cites: - `cmd/license-tool/commands.go:bindAuditFlags:156` — `cmd.Flags().StringVar(&f.resolveDeps, "resolve-deps", "ondisk", ...)` binds a raw string with no validation. - `cmd/license-tool/commands.go:newAuditCmd:101-102` and `newCheckCmd:133-134` — the raw value is forwarded into `report.Options{ResolveDeps: f.resolveDeps, AllowToolShellOut: f.resolveDeps == "tool"}`; the only comparison is `== "tool"`. - `internal/report/report.go:Audit:324` — `if opts.IncludeDeps && opts.ResolveDeps != "off" && pipe.ResolveDeps != nil` — the gate is `!= "off"`, so any non-`off` token (including garbage) enables resolution. `ParseFormat` (report.go:48) is the *only* token validator in the package; the tier has no equivalent. **Suggested fix**: add a `report.ParseResolveDeps(raw string) (string, error)` mirroring the existing `ParseFormat` precedent (report.go:48) — accept `ondisk`/`tool`/`off` (and `""` → `ondisk`), reject everything else with `fmt.Errorf("report: unknown resolver tier %q", raw)`. Call it in **both** command RunE bodies (`newAuditCmd` ~line 94 and `newCheckCmd` ~line 126, right next to the existing `ParseFormat` calls) before constructing `report.Options`, so audit and check reject identically — this is the same dual-boundary discipline the v0.2.1 universal-shebang fix applied (both the INSERT path in `internal/render/render.go` and the `PreserveBoundary` in `internal/detect/detect.go`). Note there is no filetype-table analog here (tier values aren't per-language), so the "filetype table" leg of the usual three-touch pattern doesn't apply to this issue. Keep the 100% gate green: add `cmd` and/or `report` tests exercising each accepted token plus at least one rejected token (the `report_test.go` `ParseFormat` table at line 27 is the template). Validating centrally in `report` also lets `AllowToolShellOut` stay derived from the now-validated value. **Corrections to the issue**: none material. Line references (commands.go 151-157 / 98-103, report.go 321-333) all land on the right code; the exact decision points are commands.go:156 + 102/134 and report.go:324. One framing nuance worth flagging for the maintainer (not an error in the body): impact is narrower than "silently change dependency-resolution behavior" implies — an unknown token resolves to the *default* (`ondisk`) behavior, so it only diverges from intent when the user meant `off`; it does not, e.g., silently turn on tool shell-out (that still requires the literal `tool`). **Open questions**: - Where should validation live — in the `cmd` layer (parse-time, like `ParseFormat`) or defensively in `report.Audit` as well? Doing both is safest but the `cmd`-layer parse is the user-facing one; pick one as the source of truth to avoid a redundant untested branch fighting the 100% gate. - Should `--resolve-deps` also imply/interact with `--deps`? Today `--deps=false` makes the tier moot (report.go:324 short-circuits on `IncludeDeps`), so `--deps=false --resolve-deps=off` is redundant — decide whether to leave that as-is or warn. - Empty-string handling: confirm `""` should map to `ondisk` (matching the flag default) rather than erroring, since the default is set at bind time and `""` shouldn't normally reach the parser. **Overlaps**: none found among the cited code paths. _Automated maintainer review._

KofTwentyTwo commented

2026-06-04 20:28:22 +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#25