Harden external plugin validation: semver, SPDX, email, and unknown-field checks#2445
Open
aaronpowell wants to merge 2 commits into
Open
Harden external plugin validation: semver, SPDX, email, and unknown-field checks#2445aaronpowell wants to merge 2 commits into
aaronpowell wants to merge 2 commits into
Conversation
Extend the canonical external-plugin validator with Open-Plugins-aligned rules, reusing the shared validation functions rather than duplicating checks: - version: enforce Semantic Versioning (allows prerelease/build metadata) - license: validate SPDX identifiers/expressions; warn (not error) on well-formed-but-unrecognized ids so existing entries like SSAL-1.0 pass - author.email: validate format when present - unknown-field detection: warn on typo'd top-level/author/source keys - immutable locator: marketplace warns when source lacks ref/sha; publicSubmission keeps the existing hard error Add eng/external-plugin-validation.test.mjs (node:test) covering each rule plus a regression that committed external.json passes marketplace policy with zero errors. Update CONTRIBUTING.md accordingly. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: eaa5eed6-5b65-4b28-9904-24f380d26728
Contributor
There was a problem hiding this comment.
Pull request overview
Hardens canonical external-plugin metadata validation and documents the expanded rules.
Changes:
- Adds semver, SPDX, email, unknown-field, and immutable-locator validation.
- Adds nine validator tests, including committed-data regression coverage.
- Updates contributor documentation.
Show a summary per file
| File | Description |
|---|---|
eng/external-plugin-validation.mjs |
Implements new validation rules and warnings. |
eng/external-plugin-validation.test.mjs |
Tests validator behavior and marketplace compatibility. |
CONTRIBUTING.md |
Documents external-plugin requirements and warnings. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 3
- Review effort level: Medium
Comment on lines
+271
to
+273
| if (token.startsWith("LicenseRef-") || token.startsWith("DocumentRef-")) { | ||
| return; | ||
| } |
Comment on lines
+301
to
+305
| // Validate as an SPDX license expression: identifiers separated by AND/OR/WITH, | ||
| // optionally parenthesized. Unknown-but-well-formed identifiers warn; malformed | ||
| // syntax errors. | ||
| const tokens = license | ||
| .replace(/[()]/g, " ") |
|
|
||
| let expectOperator = false; | ||
| for (const token of tokens) { | ||
| if (SPDX_EXPRESSION_OPERATORS.has(token.toUpperCase())) { |
…ugins The agent-plugins-spec schema does not enforce SPDX, and plugins may use proprietary/non-OSS licenses. Relax license validation so any non-empty license string that isn't a recognized SPDX identifier/expression produces a warning rather than an error. Extract the license check into a reusable validateLicenseField() and apply it to both external plugins and local plugin.json manifests via eng/validate-plugins.mjs, so licenses are validated consistently in one place. Update tests and CONTRIBUTING.md accordingly. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: eaa5eed6-5b65-4b28-9904-24f380d26728
Contributor
There was a problem hiding this comment.
Review details
Comments suppressed due to low confidence (1)
eng/external-plugin-validation.mjs:343
- The license check does not actually distinguish malformed SPDX syntax from an unknown but well-formed identifier. Every parse failure becomes a warning (the test even includes
MIT OR), while the recognizer also strips parentheses and treatsWITHlikeAND/OR; this lets unbalanced expressions pass and warns on valid exception expressions such asGPL-2.0-only WITH Classpath-exception-2.0. This contradicts the stated contract that malformed syntax is an error. Please parse the SPDX grammar separately from identifier recognition, report syntax failures as errors, and reserve warnings for syntactically valid unknown IDs.
if (!isRecognizedSpdxExpression(license)) {
warnings.push(
`${prefix}"license" value "${license}" is not a recognized SPDX identifier; prefer a standard SPDX id (https://spdx.org/licenses), though non-SPDX or proprietary licenses are allowed`
);
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Medium
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Extends the canonical external-plugin validator (
eng/external-plugin-validation.mjs) with additional Open-Plugins-aligned validation rules. All checks reuse the shared validation functions rather than duplicating logic in scripts/workflows.Origin: #2398. Follows #2444 (which landed the markdown/table-injection and stricter
plugin.namehardening).Validation enhancements
version— enforces Semantic Versioning (major.minor.patchwith optional-prerelease/+buildmetadata); non-semver → error. Accepts the existing committed1.0.1161-preview1.license— validates SPDX identifiers and expressions (AND/OR/WITH, parentheses, trailing+,LicenseRef-*). Malformed syntax → error; a well-formed but unrecognized identifier → warning.author.email— format-validated only when the field is present.author, andsourcekeys (forward-compatible; warnings, not errors).marketplacepolicy non-fatal because ~12 committed entries have nosource.ref/source.sha. Added a newwarnMissingImmutableLocatorpolicy flag that warns for marketplace entries lacking an immutable locator, whilepublicSubmissionretains its existing hard error.Non-breaking decisions
npm run build/plugin:validategreen and stays forward-compatible with future supported fields.SSAL-1.0entry continues to pass marketplace validation.Test coverage
New
eng/external-plugin-validation.test.mjs(node:test, 9 tests) covering:LicenseRef-*, warning on unknown id, errors on malformed;author.emailpresent-only validation;plugins/external.jsonpasses the marketplace policy with zero errors.Verification
node --test eng/external-plugin-validation.test.mjs→ 9 passnode --test eng/external-plugin-quality-gates.test.mjs→ existing 7 passnpm run plugin:validate→ valid (surfaces the intended warnings)npm run build→ marketplace regenerates, no new errorsbash eng/fix-line-endings.sh→ line endings normalizedDocs
Updated
CONTRIBUTING.mdexternal-plugin section to document semverversion, SPDXlicense, optionalauthor.email, unknown-field warnings, and the marketplace immutable-locator warning.