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

valid SPDX IDs can pass validation but fail apply and license rendering #19

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

Finding

The config layer and wizard accept any non-deprecated SPDX ID from the full index, but write commands can only render the curated subset exposed by spdx.Lookup. Users can create valid configs that the tool later cannot apply.

Evidence

  • internal/spdx/spdx.go:7 to internal/spdx/spdx.go:14 documents that Validate accepts the full index while Lookup covers only the curated rendering set.
  • internal/spdx/spdx.go:143 to internal/spdx/spdx.go:166 implements that split.
  • cmd/license-tool/wizard.go:36 to cmd/license-tool/wizard.go:50 offers the full non-deprecated SPDX ID list in the picker.
  • internal/config/config.go:165 to internal/config/config.go:170 validates only with spdx.Validate.
  • internal/applier/applier.go:74 to internal/applier/applier.go:77 and internal/applier/applier.go:191 to internal/applier/applier.go:194 later fail if spdx.Lookup cannot render the ID.
  • cmd/license-tool/commands_test.go:432 to cmd/license-tool/commands_test.go:440 already documents Zlib as valid but failing later.

Reproduction

  1. Scaffold or pass a valid SPDX ID outside the curated render set, for example Zlib.
  2. Run license-tool apply <repo> or license-tool license <repo>.
  3. Observe that config validation succeeds first, then the write command fails with an unknown-license style render error.

Why It Matters

The product claims to be generic and license-agnostic, but the UI and validation steer users into accepted states that cannot execute. The failure arrives late and looks like an internal inconsistency.

Suggested Fix

Either vendor renderable detail for every SPDX ID accepted by config and the wizard, or expose a separate renderable-license validation path for write commands and picker options.

## Finding The config layer and wizard accept any non-deprecated SPDX ID from the full index, but write commands can only render the curated subset exposed by `spdx.Lookup`. Users can create valid configs that the tool later cannot apply. ## Evidence - `internal/spdx/spdx.go:7` to `internal/spdx/spdx.go:14` documents that `Validate` accepts the full index while `Lookup` covers only the curated rendering set. - `internal/spdx/spdx.go:143` to `internal/spdx/spdx.go:166` implements that split. - `cmd/license-tool/wizard.go:36` to `cmd/license-tool/wizard.go:50` offers the full non-deprecated SPDX ID list in the picker. - `internal/config/config.go:165` to `internal/config/config.go:170` validates only with `spdx.Validate`. - `internal/applier/applier.go:74` to `internal/applier/applier.go:77` and `internal/applier/applier.go:191` to `internal/applier/applier.go:194` later fail if `spdx.Lookup` cannot render the ID. - `cmd/license-tool/commands_test.go:432` to `cmd/license-tool/commands_test.go:440` already documents `Zlib` as valid but failing later. ## Reproduction 1. Scaffold or pass a valid SPDX ID outside the curated render set, for example `Zlib`. 2. Run `license-tool apply <repo>` or `license-tool license <repo>`. 3. Observe that config validation succeeds first, then the write command fails with an unknown-license style render error. ## Why It Matters The product claims to be generic and license-agnostic, but the UI and validation steer users into accepted states that cannot execute. The failure arrives late and looks like an internal inconsistency. ## Suggested Fix Either vendor renderable detail for every SPDX ID accepted by config and the wizard, or expose a separate renderable-license validation path for write commands and picker options.
KofTwentyTwo commented 2026-06-04 16:29:34 +00:00 (Migrated from github.com)
Copy link
Copy source

Status: confirmed — the config/wizard accept-set (spdx.Validate, 729+ ids) is strictly wider than the render/apply set (spdx.Lookup, the curated data/licenses/*.json), so a valid SPDX id like Zlib passes validation and then fails at write time.

Severity: medium (matches the bug label; not high). The failure is loud and immediate at apply/license time with a clear error, not silent corruption or data loss. The 100% workflow for any curated id (MIT/Apache/AGPL/etc.) is unaffected. The defect is a UX/consistency gap: the tool steers users toward states it cannot execute. Reproduced locally: license-tool license <dir> --license Zlib --holder Acme exits non-zero (validation passes, render fails), while --license MIT renders LICENSE/LICENSES/MIT.txt normally.

Verified root cause: the asymmetry is by design and documented at internal/spdx/spdx.go:5-14 (the two-surface contract) and implemented at Validate internal/spdx/spdx.go:146-161 (full index s.ids) vs Lookup internal/spdx/spdx.go:166-180 (curated s.licenses). The late failure is emitted from two sites, both returning fmt.Errorf("applier: unknown license %q", cfg.License):

  • internal/applier/applier.go:74-77 (Apply)
  • internal/applier/applier.go:191-194 (ManageLicenseFiles)

The permissive gate is internal/config/config.go:168-170 (validates with spdx.Validate only). The wizard widens the same way: cmd/license-tool/wizard.go:36-50 (licenseSelectOptions) lists spdx.CommonIDs() then all of spdx.IDs() (every non-deprecated id, spdx.go:203-224), so the picker offers ids that cannot render. The existing test cmd/license-tool/commands_test.go:432-441 already documents Zlib as "valid but non-curated" and asserts the late unknown license error — i.e. current behavior is pinned, not accidental.

Suggested fix: introduce a single renderable-set predicate in the spdx package (e.g. Renderable(id) bool / RenderableIDs() backed by the same s.licenses map Lookup uses) and apply it at BOTH the entry boundaries the issue spans:

  1. config gate internal/config/config.go:168-170 — fail early with a message that distinguishes "not a real SPDX id" from "valid SPDX id but no vendored rendering support," so the error arrives at config time instead of apply time.
  2. wizard picker cmd/license-tool/wizard.go:44-49 — drive options from the renderable set (or visibly disable/annotate non-renderable ids) so the UI never offers an id that fails.

This mirrors the v0.2.1 universal-shebang precedent (#5), where the fix had to touch both the render INSERT path and the detect PreserveBoundary so the two boundaries stay symmetric — here the symmetric boundaries are the config gate and the wizard picker rather than render/detect. Where a filetype-style table is the right home, the analogue is the curated data/licenses/*.json set the generator (scripts/gen_spdx.go) emits; the alternative fix the issue lists (vendor renderable detail for every accepted id) would instead widen that generated set so Lookup == Validate. Either way, keep the 100% gate green: commands_test.go:432-441 currently asserts the late unknown license error and will need updating to assert the new early error, and add coverage for the new predicate's true/false arms and the wizard option filter.

Corrections to the issue:

  • The body cites Validate at internal/spdx/spdx.go:7-14 and the split at :143-166 — the 7-14 cite is the doc comment (fine); the executable split is Validate at :146-161 and Lookup at :166-180 (the issue's :143-166 brackets the doc + part of Lookup but stops before Lookup's closure body at :172-179). Minor.
  • The body says the wizard "offers the full non-deprecated SPDX ID list" at wizard.go:36-50. Accurate, but note it offers CommonIDs() first (the curated, fully-renderable shortlist) then the full IDs() tail — so the common picks are always safe; only the long tail is affected. Narrower-than-implied impact.
  • Otherwise the evidence chain (config → applier render failure → existing Zlib test) is accurate.

Open questions (maintainer to decide):

  1. Direction of the fix: shrink the accept-set to renderable-only, or grow the render-set to match validation (vendor every id's text/header)? These have opposite implications for "license-agnostic" positioning and snapshot size.
  2. If shrinking: should a valid-but-non-renderable id be a hard error, or a warning with a documented escape hatch (e.g. user-supplied header text) for genuinely generic targets?
  3. Should Validate stay permissive for the detect/report read path (reporting an existing non-curated header is legitimate) while only the write path tightens? If so, the predicate must be applied at write/wizard boundaries only, not in config.Validate unconditionally — otherwise read-only runs over repos already carrying a non-curated license would start erroring.

Overlaps: none found among open issues (this is the Validate/Lookup asymmetry, distinct from the #5 shebang work). Conceptually shares the "keep the two boundaries symmetric" shape with #5 but is not a duplicate or dependency.

Automated maintainer review.

**Status:** confirmed — the config/wizard accept-set (`spdx.Validate`, 729+ ids) is strictly wider than the render/apply set (`spdx.Lookup`, the curated `data/licenses/*.json`), so a valid SPDX id like `Zlib` passes validation and then fails at write time. **Severity:** medium (matches the `bug` label; not high). The failure is loud and immediate at apply/license time with a clear error, not silent corruption or data loss. The 100% workflow for any curated id (MIT/Apache/AGPL/etc.) is unaffected. The defect is a UX/consistency gap: the tool steers users toward states it cannot execute. Reproduced locally: `license-tool license <dir> --license Zlib --holder Acme` exits non-zero (validation passes, render fails), while `--license MIT` renders `LICENSE`/`LICENSES/MIT.txt` normally. **Verified root cause:** the asymmetry is by design and documented at `internal/spdx/spdx.go:5-14` (the two-surface contract) and implemented at `Validate` `internal/spdx/spdx.go:146-161` (full index `s.ids`) vs `Lookup` `internal/spdx/spdx.go:166-180` (curated `s.licenses`). The late failure is emitted from two sites, both returning `fmt.Errorf("applier: unknown license %q", cfg.License)`: - `internal/applier/applier.go:74-77` (`Apply`) - `internal/applier/applier.go:191-194` (`ManageLicenseFiles`) The permissive gate is `internal/config/config.go:168-170` (validates with `spdx.Validate` only). The wizard widens the same way: `cmd/license-tool/wizard.go:36-50` (`licenseSelectOptions`) lists `spdx.CommonIDs()` then all of `spdx.IDs()` (every non-deprecated id, `spdx.go:203-224`), so the picker offers ids that cannot render. The existing test `cmd/license-tool/commands_test.go:432-441` already documents `Zlib` as "valid but non-curated" and asserts the late `unknown license` error — i.e. current behavior is pinned, not accidental. **Suggested fix:** introduce a single renderable-set predicate in the `spdx` package (e.g. `Renderable(id) bool` / `RenderableIDs()` backed by the same `s.licenses` map `Lookup` uses) and apply it at BOTH the entry boundaries the issue spans: 1. config gate `internal/config/config.go:168-170` — fail early with a message that distinguishes "not a real SPDX id" from "valid SPDX id but no vendored rendering support," so the error arrives at config time instead of apply time. 2. wizard picker `cmd/license-tool/wizard.go:44-49` — drive options from the renderable set (or visibly disable/annotate non-renderable ids) so the UI never offers an id that fails. This mirrors the v0.2.1 universal-shebang precedent (#5), where the fix had to touch both the render INSERT path and the detect `PreserveBoundary` so the two boundaries stay symmetric — here the symmetric boundaries are the config gate and the wizard picker rather than render/detect. Where a filetype-style table is the right home, the analogue is the curated `data/licenses/*.json` set the generator (`scripts/gen_spdx.go`) emits; the alternative fix the issue lists (vendor renderable detail for every accepted id) would instead widen that generated set so `Lookup` == `Validate`. Either way, keep the 100% gate green: `commands_test.go:432-441` currently asserts the *late* `unknown license` error and will need updating to assert the *new early* error, and add coverage for the new predicate's true/false arms and the wizard option filter. **Corrections to the issue:** - The body cites `Validate` at `internal/spdx/spdx.go:7-14` and the split at `:143-166` — the 7-14 cite is the doc comment (fine); the executable split is `Validate` at `:146-161` and `Lookup` at `:166-180` (the issue's `:143-166` brackets the doc + part of `Lookup` but stops before `Lookup`'s closure body at `:172-179`). Minor. - The body says the wizard "offers the full non-deprecated SPDX ID list" at `wizard.go:36-50`. Accurate, but note it offers `CommonIDs()` first (the curated, fully-renderable shortlist) then the full `IDs()` tail — so the *common* picks are always safe; only the long tail is affected. Narrower-than-implied impact. - Otherwise the evidence chain (config → applier render failure → existing `Zlib` test) is accurate. **Open questions (maintainer to decide):** 1. Direction of the fix: shrink the accept-set to renderable-only, or grow the render-set to match validation (vendor every id's text/header)? These have opposite implications for "license-agnostic" positioning and snapshot size. 2. If shrinking: should a valid-but-non-renderable id be a hard error, or a warning with a documented escape hatch (e.g. user-supplied header text) for genuinely generic targets? 3. Should `Validate` stay permissive for the *detect/report* read path (reporting an existing non-curated header is legitimate) while only the *write* path tightens? If so, the predicate must be applied at write/wizard boundaries only, not in `config.Validate` unconditionally — otherwise read-only runs over repos already carrying a non-curated license would start erroring. **Overlaps:** none found among open issues (this is the Validate/Lookup asymmetry, distinct from the #5 shebang work). Conceptually shares the "keep the two boundaries symmetric" shape with #5 but is not a duplicate or dependency. _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#19