KofTwentyTwo/license-tool
2
0 Fork 0
Code Issues 2 Pull Requests 9 Actions Packages Projects Releases 6 Wiki Activity

policy allow deny and required license IDs are not validated #20

New Issue
Closed
opened 2026-06-04 16:19:25 +00:00 by KofTwentyTwo · 2 comments
KofTwentyTwo commented 2026-06-04 16:19:25 +00:00 (Migrated from github.com)
Copy link
Copy source

Finding

Policy license IDs are loaded as raw strings and compared by exact equality during enforcement. Typos, deprecated IDs, and malformed SPDX expressions are not rejected during config resolution.

Evidence

  • internal/config/config.go:165 to internal/config/config.go:170 validates only the top-level target license.
  • internal/config/config.go:386 to internal/config/config.go:401 merges policy.required, policy.allow, and policy.deny without validating their contents.
  • internal/policy/policy.go:304 to internal/policy/policy.go:310 provides an SPDX expression validator, but it is not wired into config loading.
  • internal/policy/policy.go:330 to internal/policy/policy.go:337 enforces policy with exact string membership checks.

Reproduction

  1. Configure policy.deny with a typo that resembles a forbidden SPDX ID.
  2. Scan a repo containing the intended forbidden license.
  3. Observe that config resolution succeeds and the typo does not match, so the intended deny rule is silently ineffective.

Why It Matters

License policy is a compliance control. Silent typos weaken or disable policy enforcement while CI still appears to be running a configured gate.

Suggested Fix

Validate required, allow, and deny entries during config merge. Reject malformed SPDX IDs or expressions with actionable config errors, and add tests for typo rejection.

## Finding Policy license IDs are loaded as raw strings and compared by exact equality during enforcement. Typos, deprecated IDs, and malformed SPDX expressions are not rejected during config resolution. ## Evidence - `internal/config/config.go:165` to `internal/config/config.go:170` validates only the top-level target license. - `internal/config/config.go:386` to `internal/config/config.go:401` merges `policy.required`, `policy.allow`, and `policy.deny` without validating their contents. - `internal/policy/policy.go:304` to `internal/policy/policy.go:310` provides an SPDX expression validator, but it is not wired into config loading. - `internal/policy/policy.go:330` to `internal/policy/policy.go:337` enforces policy with exact string membership checks. ## Reproduction 1. Configure `policy.deny` with a typo that resembles a forbidden SPDX ID. 2. Scan a repo containing the intended forbidden license. 3. Observe that config resolution succeeds and the typo does not match, so the intended deny rule is silently ineffective. ## Why It Matters License policy is a compliance control. Silent typos weaken or disable policy enforcement while CI still appears to be running a configured gate. ## Suggested Fix Validate `required`, `allow`, and `deny` entries during config merge. Reject malformed SPDX IDs or expressions with actionable config errors, and add tests for typo rejection.
KofTwentyTwo commented 2026-06-04 16:28:48 +00:00 (Migrated from github.com)
Copy link
Copy source

Status: confirmed — policy required/allow/deny values are copied verbatim during config merge and only ever exact-matched at enforcement; nothing validates them as SPDX IDs/expressions.

Severity: medium (label bug is fine; not high because it is a silent-weakening config-correctness issue, not a wrong-output or crash on valid input — the gate still runs, it just may under-enforce a misconfigured rule).

Verified root cause:

  • internal/config/config.go:mergePolicy:384-405 copies ps.Required, ps.Allow, ps.Deny straight onto the policy with no validation (p.Required = ps.Required, append(... ps.Allow ...), append(... ps.Deny ...)).
  • internal/config/config.go:168 only validates the top-level target license (spdx.Validate(cfg.License)); policy lists are never checked.
  • internal/policy/policy.go:ValidExpression:308 exists but is dead for this purpose — grep -rn ValidExpression shows references only at its own definition and in policy_test.go (TestValidExpression). It is not wired into config loading.
  • Enforcement is exact-string membership: internal/policy/policy.go:checkAllowDenyRequired at lines 239 (inList(p.Deny, id)), 247 (inList(p.Allow, id)), 255 (id != p.Required); also EvaluateRepo:175 and evaluateLicenseID:218/225. inList (policy.go:331) is a literal == loop. So a typo'd deny entry silently never matches.

Suggested fix: In mergePolicy (or a dedicated validate step on the merged policy, mirroring the existing post-merge spdx.Validate at config.go:168 — "only the final merged value matters"), reject malformed entries. required should be validated; allow/deny entries are SPDX expressions in principle but are enforced as exact IDs via inList, so the simplest correct check is spdx.Validate per entry (matching how they are actually matched). If you intend to honor full expressions in allow/deny, wire policy.ValidExpression instead and then the enforcement path also needs to stop using exact inList — that is a larger behavior change, not just validation. This mirrors the v0.2.1 universal-shebang precedent in spirit (fix every code path that shares the contract, not just one): touch the merge/validation site AND keep the enforcement matcher consistent with whatever validity rule you pick. Add tests covering typo rejection for each of required/allow/deny, and keep the 100% coverage gate (.testcoverage.yml) green by exercising both the reject and accept branches.

Corrections to the issue:

  • The body cites internal/policy/policy.go:330 to :337 for "exact string membership checks." Line 331 is the inList helper; the actual membership checks that gate enforcement live in checkAllowDenyRequired (239/247/255), EvaluateRepo (175), and evaluateLicenseID (218/225). Worth pointing the fix at those call sites, not just the helper.
  • Validator cite 304 to 310 is approximately right (ValidExpression is lines 304-310); minor.
  • Config cites 165-170 (target-license validation) and 386-401 (merge) are accurate; the function mergePolicy spans 384-405.
  • Note allow/deny are matched as exact IDs today, not as SPDX expressions, so "malformed SPDX expressions" slightly over-states current behavior — expressions are never parsed for these fields at all.

Open questions:

  • Scope decision: validate allow/deny as exact SPDX IDs (cheap, matches current inList enforcement) OR as full SPDX expressions (requires also changing enforcement away from exact match)? These are different features.
  • Behavior on invalid entry: hard error at config load (consistent with the target-license behavior at config.go:168) vs. warn-and-drop? The issue says "reject with actionable config errors" — please confirm hard-fail is desired even on read-only audit/check runs.
  • Should an empty-string entry in allow/deny be an error or silently ignored (cf. uniqueSorted which already drops empties)?

Overlaps: none identified from the cited code. (I only verified this issue; a maintainer should confirm against any other open config-validation issues.)

Automated maintainer review.

**Status**: confirmed — policy `required`/`allow`/`deny` values are copied verbatim during config merge and only ever exact-matched at enforcement; nothing validates them as SPDX IDs/expressions. **Severity**: medium (label `bug` is fine; not high because it is a silent-weakening config-correctness issue, not a wrong-output or crash on valid input — the gate still runs, it just may under-enforce a misconfigured rule). **Verified root cause**: - `internal/config/config.go:mergePolicy:384-405` copies `ps.Required`, `ps.Allow`, `ps.Deny` straight onto the policy with no validation (`p.Required = ps.Required`, `append(... ps.Allow ...)`, `append(... ps.Deny ...)`). - `internal/config/config.go:168` only validates the top-level *target* license (`spdx.Validate(cfg.License)`); policy lists are never checked. - `internal/policy/policy.go:ValidExpression:308` exists but is dead for this purpose — `grep -rn ValidExpression` shows references only at its own definition and in `policy_test.go` (TestValidExpression). It is not wired into config loading. - Enforcement is exact-string membership: `internal/policy/policy.go:checkAllowDenyRequired` at lines 239 (`inList(p.Deny, id)`), 247 (`inList(p.Allow, id)`), 255 (`id != p.Required`); also `EvaluateRepo:175` and `evaluateLicenseID:218/225`. `inList` (policy.go:331) is a literal `==` loop. So a typo'd deny entry silently never matches. **Suggested fix**: In `mergePolicy` (or a dedicated validate step on the merged policy, mirroring the existing post-merge `spdx.Validate` at config.go:168 — "only the final merged value matters"), reject malformed entries. `required` should be validated; `allow`/`deny` entries are SPDX *expressions* in principle but are enforced as exact IDs via `inList`, so the simplest correct check is `spdx.Validate` per entry (matching how they are actually matched). If you intend to honor full expressions in allow/deny, wire `policy.ValidExpression` instead and then the enforcement path also needs to stop using exact `inList` — that is a larger behavior change, not just validation. This mirrors the v0.2.1 universal-shebang precedent in spirit (fix every code path that shares the contract, not just one): touch the merge/validation site AND keep the enforcement matcher consistent with whatever validity rule you pick. Add tests covering typo rejection for each of `required`/`allow`/`deny`, and keep the 100% coverage gate (`.testcoverage.yml`) green by exercising both the reject and accept branches. **Corrections to the issue**: - The body cites `internal/policy/policy.go:330` to `:337` for "exact string membership checks." Line 331 is the `inList` *helper*; the actual membership checks that gate enforcement live in `checkAllowDenyRequired` (239/247/255), `EvaluateRepo` (175), and `evaluateLicenseID` (218/225). Worth pointing the fix at those call sites, not just the helper. - Validator cite `304` to `310` is approximately right (`ValidExpression` is lines 304-310); minor. - Config cites `165-170` (target-license validation) and `386-401` (merge) are accurate; the function `mergePolicy` spans 384-405. - Note `allow`/`deny` are matched as exact IDs today, not as SPDX *expressions*, so "malformed SPDX expressions" slightly over-states current behavior — expressions are never parsed for these fields at all. **Open questions**: - Scope decision: validate `allow`/`deny` as exact SPDX IDs (cheap, matches current `inList` enforcement) OR as full SPDX expressions (requires also changing enforcement away from exact match)? These are different features. - Behavior on invalid entry: hard error at config load (consistent with the target-license behavior at config.go:168) vs. warn-and-drop? The issue says "reject with actionable config errors" — please confirm hard-fail is desired even on read-only audit/check runs. - Should an empty-string entry in `allow`/`deny` be an error or silently ignored (cf. `uniqueSorted` which already drops empties)? **Overlaps**: none identified from the cited code. (I only verified this issue; a maintainer should confirm against any other open config-validation issues.) _Automated maintainer review._
KofTwentyTwo commented 2026-06-04 20:28:34 +00:00 (Migrated from github.com)
Copy link
Copy source

Resolution note for the issue record: fixed on develop in commit 742c124e6ec2afa9312075db3112bc2def3c91ff.

Validation:

Resolution note for the issue record: fixed 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)
Sign in to join this conversation.
No Branch/Tag Specified
Labels
Clear labels
bug
Something isn't working
 dependencies
Pull requests that update a dependency file
 documentation
Improvements or additions to documentation
 duplicate
This issue or pull request already exists
 enhancement
New feature or request
 github_actions
Pull requests that update GitHub Actions code
 go
Pull requests that update go code
 good first issue
Good for newcomers
 help wanted
Extra attention is needed
 invalid
This doesn't seem right
 question
Further information is requested
 wontfix
This will not be worked on
No labels bug
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
forge-recovery (Forge Recovery) james.maes (James Maes)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: KofTwentyTwo/license-tool#20