Harden external plugin validation: semver, SPDX, email, and unknown-field checks (#2445)

* Harden external plugin validation: semver, SPDX, email, unknown fields 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 * Make license non-SPDX a warning and share license check with local plugins 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 * Extract shared license validation into eng/lib/license.mjs Move the SPDX constants and license validation (validateLicenseField, isRecognizedSpdxExpression) out of external-plugin-validation.mjs into a dedicated eng/lib/license.mjs module. Both the external plugin catalog validator and the local plugin.json validator now import license logic from this neutral shared module instead of one validator importing from the other. Behavior is unchanged; tests import from the new home. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: eaa5eed6-5b65-4b28-9904-24f380d26728 * Fix SPDX license parsing correctness and warning-message injection Rewrite isRecognizedSpdxExpression as a recursive-descent SPDX license expression parser to address PR review feedback: - Validate LicenseRef-/DocumentRef- grammar (non-empty idstring; DocumentRef requires the ":LicenseRef-<id>" suffix) instead of accepting any token with that prefix. - Parse parentheses with balance and placement checks rather than stripping them, so malformed groupings like "(MIT", "MIT)", and "MIT OR (Apache-2.0))" are no longer silently treated as valid. - Treat WITH as a distinct operator whose right operand must be a known SPDX license exception, so "MIT WITH Apache-2.0" and "GPL-2.0-only WITH MIT" are rejected while "GPL-2.0-only WITH Classpath-exception-2.0" is recognized. - Sanitize the untrusted license value in the non-SPDX warning (collapse whitespace, truncate, escape backticks, wrap as inline code) since the warning is rendered verbatim into a Markdown bot comment during intake. Malformed SPDX stays a warning (never an error) so proprietary and non-OSS license strings remain allowed. Adds targeted grammar and sanitization tests. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: eaa5eed6-5b65-4b28-9904-24f380d26728 * Complete backslash escaping and run source unknown-field check for all sources Address two PR review findings: - eng/lib/license.mjs: sanitizeForMessage now escapes backslashes before backticks so the inline-code warning value is fully escaped, resolving a CodeQL "incomplete string escaping" alert. A raw backslash in a license value is now doubled rather than left to combine with a following escaped backtick. - eng/external-plugin-validation.mjs: move the source unknown-field check out of validateGitHubSource into the outer source-object branch so it runs for every object source and then dispatch by source.source. A typo in the source discriminator (e.g. "soruce") or an unsupported source type now still surfaces the unknown-field warning instead of being silently skipped. Adds regression tests for both behaviors. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: eaa5eed6-5b65-4b28-9904-24f380d26728 * Harden Markdown safety of validation warnings via inlineCode helper External-plugin validation warnings are rendered verbatim as Markdown list items in the intake bot comment, so untrusted values interpolated into them are an injection vector. Add eng/lib/markdown.mjs exporting inlineCode(), which wraps a value in a code span fenced with more backticks than any run in the content (and pads leading/trailing backticks). This cannot be broken out of, unlike the previous backslash escaping, which does not work inside code spans. - lib/license.mjs: replace sanitizeForMessage with inlineCode for the non-SPDX license warning. - external-plugin-validation.mjs: wrap unknown-field keys (attacker controlled JSON object keys) with inlineCode so newlines/Markdown in a key can no longer inject a new list item. - Add inlineCode unit tests and injection-neutralization tests for both the license value and unknown-field key paths. Addresses PR review comments 3679379606 and 3679434134. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: eaa5eed6-5b65-4b28-9904-24f380d26728 * Fixing codespell --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: eaa5eed6-5b65-4b28-9904-24f380d26728
2026-08-01 23:12:29 +00:00 · 2026-07-30 14:35:49 +10:00
parent c46b4a919a
commit be7a1cf734
8 changed files with 651 additions and 23 deletions
@@ -64,7 +64,9 @@

 # ACI - Azure Container Instances abbreviation

-ignore-words-list = numer,wit,aks,edn,ser,ois,gir,rouge,categor,aline,ative,afterall,deques,dateA,dateB,TE,FillIn,alle,vai,LOD,InOut,pixelX,aNULL,Wee,Sherif,queston,extenions,Vertexes,nin,FO,CAF,Parth,ans,gud,Vally,vally,ACI
+# soruce - intentional misspelling for tests
+
+ignore-words-list = numer,wit,aks,edn,ser,ois,gir,rouge,categor,aline,ative,afterall,deques,dateA,dateB,TE,FillIn,alle,vai,LOD,InOut,pixelX,aNULL,Wee,Sherif,queston,extenions,Vertexes,nin,FO,CAF,Parth,ans,gud,Vally,vally,ACI,soruce

 # Skip certain files and directories

@@ -234,13 +234,16 @@ The repository's canonical validation rules live in `eng/external-plugin-validat

 For entries committed to `plugins/external.json`, the current marketplace validation requires:

- `name`, `description`, and `version`
- `author.name`
+- `name`, `description`, and `version` (a valid [semantic version](https://semver.org), e.g. `1.2.3`)
+- `author.name` (and, when present, a valid `author.url` and `author.email`)
 - `repository` as an HTTPS GitHub URL
 - `keywords` as lowercase hyphenated tags
+- `license`, when provided, is recommended to be an [SPDX identifier or expression](https://spdx.org/licenses) (e.g. `MIT`, `Apache-2.0`, `MIT OR Apache-2.0`), but a non-SPDX or proprietary license is allowed and only produces a warning
 - `source.source: "github"` plus `source.repo` in `owner/repo` format
 - optional `source.path` values of `/` for repository root, or a repository-relative folder where the plugin structure starts (do not point to `plugin.json` directly)

+Validation also emits non-fatal warnings to catch mistakes early: unknown/misspelled top-level, `author`, or `source` fields; `license` values that are not recognized SPDX identifiers (non-SPDX/proprietary licenses are permitted, not rejected); and marketplace entries whose `source` omits an immutable `ref`/`sha` locator. The same `license` validation is shared with local plugin `plugin.json` manifests so both are checked consistently.
+
 The public-submission policy builds on those rules and also requires `license` plus at least one immutable source locator: `source.ref`, `source.sha`, or both.

 ##### Review workflow
@@ -1,6 +1,8 @@
 import fs from "fs";
 import path from "path";
 import { ROOT_FOLDER } from "./constants.mjs";
+import { validateLicenseField } from "./lib/license.mjs";
+import { inlineCode } from "./lib/markdown.mjs";

 export const EXTERNAL_PLUGINS_FILE = path.join(ROOT_FOLDER, "plugins", "external.json");

@@ -12,6 +14,7 @@ export const EXTERNAL_PLUGIN_POLICIES = Object.freeze({
    requireKeywords: true,
    requireLicense: false,
    requireImmutableLocator: false,
+    warnMissingImmutableLocator: true,
  }),
  publicSubmission: Object.freeze({
    allowedSourceTypes: ["github"],
@@ -20,9 +23,34 @@ export const EXTERNAL_PLUGIN_POLICIES = Object.freeze({
    requireKeywords: true,
    requireLicense: true,
    requireImmutableLocator: true,
+    warnMissingImmutableLocator: false,
  }),
 });

+// Allowed keys for typo detection. Kept intentionally permissive: unknown keys
+// produce warnings (not errors) so the schema stays forward-compatible.
+const ALLOWED_PLUGIN_KEYS = Object.freeze([
+  "name",
+  "description",
+  "version",
+  "author",
+  "repository",
+  "homepage",
+  "license",
+  "keywords",
+  "tags",
+  "source",
+]);
+
+const ALLOWED_AUTHOR_KEYS = Object.freeze(["name", "url", "email"]);
+
+const ALLOWED_SOURCE_KEYS = Object.freeze(["source", "repo", "path", "ref", "sha"]);
+
+// Semantic Versioning 2.0.0 (https://semver.org). Anchored: major.minor.patch
+// with optional -prerelease and +build metadata.
+const SEMVER_PATTERN =
+  /^(0|[1-9]\d*)\.(0|[1-9]\d*)\.(0|[1-9]\d*)(?:-((?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*)(?:\.(?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*))*))?(?:\+([0-9a-zA-Z-]+(?:\.[0-9a-zA-Z-]+)*))?$/;
+
 // NOTE: Keep in sync with PLUGIN_JSON_CANDIDATES in external-plugin-quality-gates.mjs
 const EXTERNAL_PLUGIN_ROOT_MANIFEST_PATHS = Object.freeze([
  "plugin.json",
@@ -97,6 +125,12 @@ function validateVersion(version, prefix, errors) {
  if (version.length > 100) {
    errors.push(`${prefix}: "version" must be 100 characters or fewer`);
  }
+
+  if (!SEMVER_PATTERN.test(version)) {
+    errors.push(
+      `${prefix}: "version" must be a valid semantic version (e.g. "1.2.3" or "1.2.3-beta.1"); see https://semver.org`
+    );
+  }
 }

 function validateKeywords(keywords, prefix, errors, warnings, required) {
@@ -164,7 +198,7 @@ function validateHttpsUrl(value, fieldName, prefix, errors, options = {}) {
  }
 }

-function validateAuthor(author, prefix, errors, required) {
+function validateAuthor(author, prefix, errors, warnings, required) {
  if (author === undefined) {
    if (required) {
      errors.push(`${prefix}: "author" is required`);
@@ -184,18 +218,43 @@ function validateAuthor(author, prefix, errors, required) {
  if (author.url !== undefined) {
    validateHttpsUrl(author.url, "author.url", prefix, errors);
  }
+
+  if (author.email !== undefined) {
+    validateEmail(author.email, "author.email", prefix, errors);
+  }
+
+  validateKnownFields(author, ALLOWED_AUTHOR_KEYS, "author", prefix, warnings);
 }

-function validateLicense(license, prefix, errors, required) {
-  if (license === undefined) {
-    if (required) {
-      errors.push(`${prefix}: "license" is required`);
-    }
+function validateEmail(value, fieldName, prefix, errors) {
+  if (!isNonEmptyString(value)) {
+    errors.push(`${prefix}: "${fieldName}" must be a non-empty string`);
    return;
  }

-  if (!isNonEmptyString(license)) {
-    errors.push(`${prefix}: "license" must be a non-empty string`);
+  // Pragmatic email check: single "@", non-empty local part, and a dotted domain.
+  if (!/^[^\s@]+@[^\s@]+\.[^\s@]+$/.test(value)) {
+    errors.push(`${prefix}: "${fieldName}" must be a valid email address`);
+  }
+}
+
+function validateLicense(license, prefix, errors, warnings, required) {
+  const result = validateLicenseField(license, { prefix, required });
+  errors.push(...result.errors);
+  warnings.push(...result.warnings);
+}
+
+function validateKnownFields(value, allowedKeys, scope, prefix, warnings) {
+  if (!value || typeof value !== "object" || Array.isArray(value)) {
+    return;
+  }
+
+  const allowed = new Set(allowedKeys);
+  const label = scope ? `${scope}.` : "";
+  for (const key of Object.keys(value)) {
+    if (!allowed.has(key)) {
+      warnings.push(`${prefix}: unknown ${scope || "top-level"} field ${inlineCode(`${label}${key}`)} (possible typo)`);
+    }
  }
 }

@@ -289,7 +348,7 @@ function validateCommitSha(sha, prefix, errors) {
  }
 }

-function validateGitHubSource(source, prefix, errors, requireImmutableLocator) {
+function validateGitHubSource(source, prefix, errors, warnings, policy) {
  if (!source || typeof source !== "object" || Array.isArray(source)) {
    errors.push(`${prefix}: "source" must be an object`);
    return;
@@ -317,8 +376,15 @@ function validateGitHubSource(source, prefix, errors, requireImmutableLocator) {
    validateCommitSha(source.sha, prefix, errors);
  }

-  if (requireImmutableLocator && source.ref === undefined && source.sha === undefined) {
-    errors.push(`${prefix}: one of "source.ref" or "source.sha" is required for public external plugin submissions`);
+  const missingLocator = source.ref === undefined && source.sha === undefined;
+  if (missingLocator) {
+    if (policy.requireImmutableLocator) {
+      errors.push(`${prefix}: one of "source.ref" or "source.sha" is required for public external plugin submissions`);
+    } else if (policy.warnMissingImmutableLocator) {
+      warnings.push(
+        `${prefix}: "source" has no "source.ref" or "source.sha"; an immutable tag ref or commit SHA is recommended for reproducible installs`
+      );
+    }
  }
 }

@@ -338,11 +404,12 @@ export function validateExternalPlugin(plugin, index, options = {}) {
  validatePluginName(plugin.name, prefix, errors);
  validateDescription(plugin.description, prefix, errors);
  validateVersion(plugin.version, prefix, errors);
-  validateAuthor(plugin.author, prefix, errors, policy.requireAuthor);
+  validateAuthor(plugin.author, prefix, errors, warnings, policy.requireAuthor);
  validateRepository(plugin.repository, prefix, errors, policy.requireRepository);
  validateHomepage(plugin.homepage, prefix, errors);
-  validateLicense(plugin.license, prefix, errors, policy.requireLicense);
+  validateLicense(plugin.license, prefix, errors, warnings, policy.requireLicense);
  validateKeywords(plugin.keywords ?? plugin.tags, prefix, errors, warnings, policy.requireKeywords);
+  validateKnownFields(plugin, ALLOWED_PLUGIN_KEYS, "", prefix, warnings);

  if (plugin.tags !== undefined && plugin.keywords === undefined) {
    warnings.push(`${prefix}: prefer "keywords" over legacy "tags"`);
@@ -350,12 +417,15 @@ export function validateExternalPlugin(plugin, index, options = {}) {

  if (!plugin.source) {
    errors.push(`${prefix}: "source" is required`);
-  } else if (typeof plugin.source === "string") {
+  } else if (typeof plugin.source !== "object" || Array.isArray(plugin.source)) {
    errors.push(`${prefix}: "source" must be an object (local file paths are not allowed for external plugins)`);
-  } else if (!policy.allowedSourceTypes.includes(plugin.source.source)) {
-    errors.push(`${prefix}: "source.source" must be one of: ${policy.allowedSourceTypes.join(", ")}`);
-  } else if (plugin.source.source === "github") {
-    validateGitHubSource(plugin.source, prefix, errors, policy.requireImmutableLocator);
+  } else {
+    validateKnownFields(plugin.source, ALLOWED_SOURCE_KEYS, "source", prefix, warnings);
+    if (!policy.allowedSourceTypes.includes(plugin.source.source)) {
+      errors.push(`${prefix}: "source.source" must be one of: ${policy.allowedSourceTypes.join(", ")}`);
+    } else if (plugin.source.source === "github") {
+      validateGitHubSource(plugin.source, prefix, errors, warnings, policy);
+    }
  }

  return { errors, warnings };
@@ -0,0 +1,245 @@
+import assert from "node:assert/strict";
+import fs from "fs";
+import { test } from "node:test";
+import {
+  EXTERNAL_PLUGINS_FILE,
+  validateExternalPlugin,
+  validateExternalPlugins,
+} from "./external-plugin-validation.mjs";
+import { validateLicenseField } from "./lib/license.mjs";
+
+function basePlugin(overrides = {}) {
+  return {
+    name: "example-plugin",
+    description: "An example external plugin used for validation tests.",
+    version: "1.2.3",
+    author: { name: "Example Author", url: "https://github.com/example" },
+    repository: "https://github.com/example/example-plugin",
+    license: "MIT",
+    keywords: ["example", "testing"],
+    source: {
+      source: "github",
+      repo: "example/example-plugin",
+      ref: "v1.2.3",
+    },
+    ...overrides,
+  };
+}
+
+function hasError(result, needle) {
+  return result.errors.some((message) => message.includes(needle));
+}
+
+function hasWarning(result, needle) {
+  return result.warnings.some((message) => message.includes(needle));
+}
+
+function longestBacktickRun(value) {
+  return Math.max(0, ...Array.from(value.matchAll(/`+/g), (match) => match[0].length));
+}
+
+test("valid plugin has no errors under both policies", () => {
+  const marketplace = validateExternalPlugin(basePlugin(), 0, { policy: "marketplace" });
+  assert.deepEqual(marketplace.errors, []);
+
+  const publicSubmission = validateExternalPlugin(
+    basePlugin({ source: { source: "github", repo: "example/example-plugin", ref: "v1.2.3" } }),
+    0,
+    { policy: "publicSubmission" }
+  );
+  assert.deepEqual(publicSubmission.errors, []);
+});
+
+test("version must be semver", () => {
+  assert.deepEqual(validateExternalPlugin(basePlugin({ version: "1.2.3" }), 0).errors, []);
+  // Prerelease metadata is allowed (matches committed 1.0.1161-preview1).
+  assert.deepEqual(validateExternalPlugin(basePlugin({ version: "1.0.1161-preview1" }), 0).errors, []);
+  assert.deepEqual(validateExternalPlugin(basePlugin({ version: "1.2.3-beta.1+build.5" }), 0).errors, []);
+
+  assert.ok(hasError(validateExternalPlugin(basePlugin({ version: "1.0" }), 0), "semantic version"));
+  assert.ok(hasError(validateExternalPlugin(basePlugin({ version: "v1.2.3" }), 0), "semantic version"));
+  assert.ok(hasError(validateExternalPlugin(basePlugin({ version: "latest" }), 0), "semantic version"));
+});
+
+test("license accepts SPDX ids and expressions", () => {
+  assert.deepEqual(validateExternalPlugin(basePlugin({ license: "Apache-2.0" }), 0).errors, []);
+  assert.deepEqual(validateExternalPlugin(basePlugin({ license: "MIT OR Apache-2.0" }), 0).errors, []);
+  assert.deepEqual(validateExternalPlugin(basePlugin({ license: "(MIT OR Apache-2.0)" }), 0).errors, []);
+  assert.deepEqual(validateExternalPlugin(basePlugin({ license: "LicenseRef-Custom" }), 0).errors, []);
+
+  const recognized = validateExternalPlugin(basePlugin({ license: "MIT" }), 0);
+  assert.equal(recognized.warnings.filter((m) => m.includes("license")).length, 0);
+});
+
+test("non-SPDX license is a warning, never an error", () => {
+  for (const license of ["SSAL-1.0", "Proprietary", "UNLICENSED", "SEE LICENSE IN LICENSE.md", "MIT OR"]) {
+    const result = validateExternalPlugin(basePlugin({ license }), 0);
+    assert.deepEqual(result.errors, [], `expected no errors for license "${license}"`);
+    assert.ok(
+      hasWarning(result, "not a recognized SPDX identifier"),
+      `expected a warning for license "${license}"`
+    );
+  }
+});
+
+test("empty/non-string license is an error; required license is enforced", () => {
+  assert.ok(hasError(validateExternalPlugin(basePlugin({ license: "" }), 0), "non-empty string"));
+  assert.ok(hasError(validateExternalPlugin(basePlugin({ license: 42 }), 0), "non-empty string"));
+
+  // publicSubmission requires a license.
+  const noLicense = basePlugin();
+  delete noLicense.license;
+  assert.ok(
+    hasError(validateExternalPlugin(noLicense, 0, { policy: "publicSubmission" }), '"license" is required')
+  );
+});
+
+test("validateLicenseField is reusable for local plugin.json", () => {
+  assert.deepEqual(validateLicenseField("MIT"), { errors: [], warnings: [] });
+
+  const proprietary = validateLicenseField("Proprietary");
+  assert.deepEqual(proprietary.errors, []);
+  assert.equal(proprietary.warnings.length, 1);
+
+  // Optional by default: absent license produces nothing.
+  assert.deepEqual(validateLicenseField(undefined), { errors: [], warnings: [] });
+  assert.ok(validateLicenseField(undefined, { required: true }).errors.length === 1);
+});
+
+test("license grammar validates refs, parentheses, WITH exceptions, and warning sanitization", () => {
+  for (const license of [
+    "GPL-2.0-only WITH Classpath-exception-2.0",
+    "LicenseRef-Custom",
+    "DocumentRef-spdx-tool:LicenseRef-MyLicense",
+    "(MIT OR Apache-2.0)",
+    "MIT AND Apache-2.0",
+  ]) {
+    const result = validateLicenseField(license);
+    assert.deepEqual(result.errors, [], `expected no errors for recognized license "${license}"`);
+    assert.equal(result.warnings.length, 0, `expected no warnings for recognized license "${license}"`);
+  }
+
+  for (const license of [
+    "LicenseRef-",
+    "LicenseRef-@@",
+    "DocumentRef-foo",
+    "(MIT",
+    "MIT)",
+    "MIT OR (Apache-2.0))",
+    "(MIT OR Apache-2.0",
+    "MIT WITH Apache-2.0",
+    "GPL-2.0-only WITH MIT",
+  ]) {
+    const result = validateLicenseField(license);
+    assert.deepEqual(result.errors, [], `expected malformed license "${license}" to warn, not error`);
+    assert.equal(result.warnings.length, 1, `expected one warning for malformed license "${license}"`);
+    assert.ok(hasWarning(result, "not a recognized SPDX identifier"));
+  }
+
+  const injected = validateLicenseField("MIT\n\n## Injected\n| a | b |");
+  assert.deepEqual(injected.errors, []);
+  assert.equal(injected.warnings.length, 1);
+  assert.ok(!injected.warnings[0].includes("\n"));
+  assert.ok(hasWarning(injected, "not a recognized SPDX identifier"));
+
+  const backslashed = validateLicenseField("MIT \\ Custom \\`code\\`");
+  assert.deepEqual(backslashed.errors, []);
+  assert.equal(backslashed.warnings.length, 1);
+  assert.ok(backslashed.warnings[0].includes("`` MIT \\ Custom \\`code\\` ``"));
+});
+
+test("license warning wraps backticks and markdown link in an unbreakable code span", () => {
+  const license = "x` [pwn](https://evil.example)";
+  const result = validateLicenseField(license);
+
+  assert.deepEqual(result.errors, []);
+  assert.equal(result.warnings.length, 1);
+
+  const warning = result.warnings[0];
+  assert.ok(warning.includes("is not a recognized SPDX identifier"));
+  const [, wrappedValue] = warning.match(/"license" value (.+?) is not a recognized SPDX identifier/u);
+  const fence = wrappedValue.match(/^`+/u)[0];
+  const content = wrappedValue.slice(fence.length, -fence.length);
+
+  assert.ok(fence.length > longestBacktickRun(content));
+  assert.ok(content.includes("[pwn](https://evil.example)"));
+});
+
+test("author.email is validated only when present", () => {
+  assert.deepEqual(
+    validateExternalPlugin(basePlugin({ author: { name: "Example", email: "dev@example.com" } }), 0).errors,
+    []
+  );
+  assert.ok(
+    hasError(
+      validateExternalPlugin(basePlugin({ author: { name: "Example", email: "not-an-email" } }), 0),
+      "author.email"
+    )
+  );
+  // Absent email must not raise an error.
+  assert.deepEqual(validateExternalPlugin(basePlugin({ author: { name: "Example" } }), 0).errors, []);
+});
+
+test("unknown fields produce warnings, not errors", () => {
+  const topLevel = validateExternalPlugin(basePlugin({ licence: "MIT" }), 0);
+  assert.deepEqual(topLevel.errors, []);
+  assert.ok(hasWarning(topLevel, "unknown top-level field `licence`"));
+
+  const authorTypo = validateExternalPlugin(
+    basePlugin({ author: { name: "Example", emial: "dev@example.com" } }),
+    0
+  );
+  assert.ok(hasWarning(authorTypo, "unknown author field `author.emial`"));
+
+  const sourceTypo = validateExternalPlugin(
+    basePlugin({ source: { source: "github", repo: "example/example-plugin", ref: "v1.2.3", branch: "main" } }),
+    0
+  );
+  assert.ok(hasWarning(sourceTypo, "unknown source field `source.branch`"));
+
+  // A typo in the source discriminator itself is still surfaced as an unknown
+  // source field (plus the discriminator error), because the unknown-field check
+  // now runs for every source object rather than only recognized github sources.
+  const sourceDiscriminatorTypo = validateExternalPlugin(
+    basePlugin({ source: { soruce: "github", repo: "example/example-plugin", ref: "v1.2.3" } }),
+    0
+  );
+  assert.ok(hasWarning(sourceDiscriminatorTypo, "unknown source field `source.soruce`"));
+  assert.ok(hasError(sourceDiscriminatorTypo, '"source.source" must be one of'));
+
+  // Supported fields never warn.
+  const clean = validateExternalPlugin(basePlugin({ homepage: "https://github.com/example/example-plugin" }), 0);
+  assert.equal(
+    clean.warnings.filter((message) => message.includes("unknown")).length,
+    0
+  );
+});
+
+test("unknown field warning collapses malicious key markdown into one inline code span", () => {
+  const maliciousKey = "evil\n- injected [x](https://evil.example)";
+  const result = validateExternalPlugin(basePlugin({ [maliciousKey]: true }), 0);
+
+  assert.deepEqual(result.errors, []);
+  const warning = result.warnings.find((message) => message.includes("unknown top-level field"));
+  assert.ok(warning);
+  assert.ok(!warning.includes("\n"));
+  assert.ok(warning.includes("unknown top-level field `evil - injected [x](https://evil.example)`"));
+  assert.ok(warning.includes("(possible typo)"));
+});
+
+test("immutable locator: marketplace warns, publicSubmission errors", () => {
+  const noLocator = basePlugin({ source: { source: "github", repo: "example/example-plugin" } });
+
+  const marketplace = validateExternalPlugin(noLocator, 0, { policy: "marketplace" });
+  assert.deepEqual(marketplace.errors, []);
+  assert.ok(hasWarning(marketplace, "immutable tag ref or commit SHA is recommended"));
+
+  const publicSubmission = validateExternalPlugin(noLocator, 0, { policy: "publicSubmission" });
+  assert.ok(hasError(publicSubmission, 'one of "source.ref" or "source.sha" is required'));
+});
+
+test("committed external.json passes marketplace policy with zero errors", () => {
+  const plugins = JSON.parse(fs.readFileSync(EXTERNAL_PLUGINS_FILE, "utf8"));
+  const { errors } = validateExternalPlugins(plugins, { policy: "marketplace" });
+  assert.deepEqual(errors, [], `Committed external.json produced errors:\n${errors.join("\n")}`);
+});
@@ -0,0 +1,261 @@
+// Shared license validation used by both the external plugin catalog validator
+// (eng/external-plugin-validation.mjs) and the local plugin.json validator
+// (eng/validate-plugins.mjs). Keeping it here avoids one validator importing from
+// the other and gives license rules a single, dedicated home.
+//
+// The agent-plugins-spec schema (https://github.com/agentplugins/agent-plugins-spec)
+// does not enforce SPDX, and plugins may legitimately use proprietary or non-OSS
+// licenses. A non-SPDX license is therefore a warning, never an error.
+
+import { inlineCode } from "./markdown.mjs";
+
+// A single SPDX license identifier token (e.g. "MIT", "Apache-2.0", "LicenseRef-Foo").
+const SPDX_ID_PATTERN = /^[A-Za-z0-9][A-Za-z0-9.-]*\+?$/;
+const SPDX_IDSTRING_PATTERN = /^[A-Za-z0-9.-]+$/;
+
+// Curated set of common SPDX license identifiers. Not exhaustive: unrecognized
+// but well-formed identifiers produce a warning rather than an error.
+const KNOWN_SPDX_IDS = new Set([
+  "0BSD",
+  "AGPL-3.0",
+  "AGPL-3.0-only",
+  "AGPL-3.0-or-later",
+  "Apache-2.0",
+  "BSD-2-Clause",
+  "BSD-3-Clause",
+  "BSL-1.0",
+  "CC-BY-4.0",
+  "CC-BY-SA-4.0",
+  "CC0-1.0",
+  "EPL-2.0",
+  "GPL-2.0",
+  "GPL-2.0-only",
+  "GPL-2.0-or-later",
+  "GPL-3.0",
+  "GPL-3.0-only",
+  "GPL-3.0-or-later",
+  "ISC",
+  "LGPL-2.1",
+  "LGPL-2.1-only",
+  "LGPL-2.1-or-later",
+  "LGPL-3.0",
+  "LGPL-3.0-only",
+  "LGPL-3.0-or-later",
+  "MIT",
+  "MPL-2.0",
+  "Unlicense",
+  "WTFPL",
+  "Zlib",
+]);
+
+const KNOWN_SPDX_EXCEPTIONS = new Set([
+  "Classpath-exception-2.0",
+  "GCC-exception-3.1",
+  "LLVM-exception",
+  "Autoconf-exception-3.0",
+  "Bison-exception-2.2",
+  "Font-exception-2.0",
+  "GPL-3.0-linking-exception",
+  "Linux-syscall-note",
+  "OpenSSL-exception",
+  "Qt-GPL-exception-1.0",
+]);
+
+function isNonEmptyString(value) {
+  return typeof value === "string" && value.trim().length > 0;
+}
+
+function isSpdxIdString(value) {
+  return SPDX_IDSTRING_PATTERN.test(value);
+}
+
+function isRecognizedLicenseRef(token) {
+  if (token.startsWith("LicenseRef-")) {
+    return isSpdxIdString(token.slice("LicenseRef-".length));
+  }
+
+  if (!token.startsWith("DocumentRef-")) {
+    return false;
+  }
+
+  const separatorIndex = token.indexOf(":LicenseRef-");
+  if (separatorIndex === -1) {
+    return false;
+  }
+
+  const documentRef = token.slice("DocumentRef-".length, separatorIndex);
+  const licenseRef = token.slice(separatorIndex + ":LicenseRef-".length);
+  return isSpdxIdString(documentRef) && isSpdxIdString(licenseRef);
+}
+
+function isRecognizedSpdxIdToken(token) {
+  if (isRecognizedLicenseRef(token)) {
+    return true;
+  }
+
+  if (!SPDX_ID_PATTERN.test(token)) {
+    return false;
+  }
+
+  const normalized = token.endsWith("+") ? token.slice(0, -1) : token;
+  return KNOWN_SPDX_IDS.has(normalized);
+}
+
+function isRecognizedSpdxExceptionToken(token) {
+  return isSpdxIdString(token) && KNOWN_SPDX_EXCEPTIONS.has(token);
+}
+
+function tokenizeSpdxExpression(license) {
+  return license
+    .replace(/([()])/g, " $1 ")
+    .trim()
+    .split(/\s+/)
+    .filter((token) => token.length > 0);
+}
+
+// Returns true only for a well-formed SPDX license expression composed of
+// recognized identifiers joined by SPDX operators (optionally parenthesized).
+// Anything else (proprietary strings, free text, unrecognized ids) returns false so
+// the caller can warn without rejecting it — the plugin spec does not enforce SPDX.
+export function isRecognizedSpdxExpression(license) {
+  if (typeof license !== "string") {
+    return false;
+  }
+
+  const tokens = tokenizeSpdxExpression(license);
+
+  if (tokens.length === 0) {
+    return false;
+  }
+
+  let position = 0;
+
+  function peek() {
+    return tokens[position];
+  }
+
+  function consume() {
+    return tokens[position++];
+  }
+
+  function parsePrimary() {
+    const token = peek();
+
+    if (token === "(") {
+      consume();
+      if (!parseOrExpression()) {
+        return false;
+      }
+      if (peek() !== ")") {
+        return false;
+      }
+      consume();
+      return true;
+    }
+
+    if (!token || token === ")" || ["AND", "OR", "WITH"].includes(token.toUpperCase())) {
+      return false;
+    }
+
+    consume();
+    return isRecognizedSpdxIdToken(token);
+  }
+
+  function parseSimpleExpression() {
+    const token = peek();
+    if (!token || token === ")" || ["AND", "OR", "WITH"].includes(token.toUpperCase())) {
+      return false;
+    }
+
+    consume();
+    return isRecognizedSpdxIdToken(token);
+  }
+
+  function parseWithExpression() {
+    if (peek() === "(") {
+      return parsePrimary() && peek()?.toUpperCase() !== "WITH";
+    }
+
+    if (!parseSimpleExpression()) {
+      return false;
+    }
+
+    if (peek()?.toUpperCase() !== "WITH") {
+      return true;
+    }
+
+    consume();
+    const exceptionToken = peek();
+    if (!exceptionToken || exceptionToken === ")" || ["AND", "OR", "WITH"].includes(exceptionToken.toUpperCase())) {
+      return false;
+    }
+    consume();
+    return isRecognizedSpdxExceptionToken(exceptionToken);
+  }
+
+  function parseAndExpression() {
+    if (!parseWithExpression()) {
+      return false;
+    }
+
+    while (peek()?.toUpperCase() === "AND") {
+      consume();
+      if (!parseWithExpression()) {
+        return false;
+      }
+    }
+
+    return true;
+  }
+
+  function parseOrExpression() {
+    if (!parseAndExpression()) {
+      return false;
+    }
+
+    while (peek()?.toUpperCase() === "OR") {
+      consume();
+      if (!parseAndExpression()) {
+        return false;
+      }
+    }
+
+    return true;
+  }
+
+  return parseOrExpression() && position === tokens.length;
+}
+
+// Canonical license validation. A non-SPDX license is a warning (never an error)
+// so authors may use proprietary or non-OSS licenses. Returns collected
+// errors/warnings so each caller can integrate them into its own reporting.
+//
+// Options:
+//   required — when true, a missing license is an error (default false).
+//   prefix   — optional message prefix (e.g. "external.json[3]") for context.
+export function validateLicenseField(license, options = {}) {
+  const { required = false } = options;
+  const prefix = options.prefix ? `${options.prefix}: ` : "";
+  const errors = [];
+  const warnings = [];
+
+  if (license === undefined) {
+    if (required) {
+      errors.push(`${prefix}"license" is required`);
+    }
+    return { errors, warnings };
+  }
+
+  if (!isNonEmptyString(license)) {
+    errors.push(`${prefix}"license" must be a non-empty string`);
+    return { errors, warnings };
+  }
+
+  if (!isRecognizedSpdxExpression(license)) {
+    warnings.push(
+      `${prefix}"license" value ${inlineCode(license)} is not a recognized SPDX identifier; prefer a standard SPDX id (https://spdx.org/licenses), though non-SPDX or proprietary licenses are allowed`
+    );
+  }
+
+  return { errors, warnings };
+}
@@ -0,0 +1,11 @@
+export function inlineCode(value) {
+  const collapsed = String(value).replace(/\s+/g, " ").trim();
+  const content = collapsed.length > 80 ? `${collapsed.slice(0, 77)}...` : collapsed;
+  const longestBacktickRun = Math.max(0, ...Array.from(content.matchAll(/`+/g), (match) => match[0].length));
+  const fence = "`".repeat(longestBacktickRun + 1);
+  const pad = content.length === 0 || content.startsWith("`") || content.endsWith("`") ? " " : "";
+
+  // These values are rendered verbatim into Markdown bot comments; using a fence
+  // longer than any backtick run in the content prevents closing the code span.
+  return `${fence}${pad}${content}${pad}${fence}`;
+}
@@ -0,0 +1,25 @@
+import assert from "node:assert/strict";
+import { test } from "node:test";
+import { inlineCode } from "./markdown.mjs";
+
+test("inlineCode wraps plain values in a single-backtick code span", () => {
+  assert.equal(inlineCode("plain value"), "`plain value`");
+});
+
+test("inlineCode uses a fence longer than the longest backtick run", () => {
+  assert.equal(inlineCode("a `` b"), "```a `` b```");
+});
+
+test("inlineCode pads values starting or ending with a backtick", () => {
+  assert.equal(inlineCode("`leading"), "`` `leading ``");
+  assert.equal(inlineCode("trailing`"), "`` trailing` ``");
+});
+
+test("inlineCode collapses whitespace and pads empty values", () => {
+  assert.equal(inlineCode("a\n\t b"), "`a b`");
+  assert.equal(inlineCode(" \n\t "), "`  `");
+});
+
+test("inlineCode truncates values to 80 characters", () => {
+  assert.equal(inlineCode("x".repeat(81)), `\`${"x".repeat(77)}...\``);
+});
@@ -4,6 +4,7 @@ import fs from "fs";
 import path from "path";
 import { ROOT_FOLDER } from "./constants.mjs";
 import { readExternalPlugins } from "./external-plugin-validation.mjs";
+import { validateLicenseField } from "./lib/license.mjs";

 const PLUGINS_DIR = path.join(ROOT_FOLDER, "plugins");
 const EXTENSIONS_DIR = path.join(ROOT_FOLDER, "extensions");
@@ -239,6 +240,12 @@ function validatePlugin(folderName) {
  const keywordsError = validateKeywords(plugin.keywords ?? plugin.tags);
  if (keywordsError) errors.push(keywordsError);

+  // Rule 5b: license (shared with external plugins). Non-SPDX is a warning, not an error.
+  const warnings = [];
+  const licenseResult = validateLicenseField(plugin.license, { required: false });
+  errors.push(...licenseResult.errors);
+  warnings.push(...licenseResult.warnings);
+
  // Rule 6: agents, commands, skills paths
  const specErrors = validateSpecPaths(plugin);
  errors.push(...specErrors);
@@ -246,7 +253,7 @@ function validatePlugin(folderName) {
  const extensionRefErrors = validateCuratedPluginExtensionRefs(plugin);
  errors.push(...extensionRefErrors);

-  return { errors, plugin: parsedPlugin };
+  return { errors, warnings, plugin: parsedPlugin };
 }

 function validateExtensionScreenshotPath(extensionDir, pathValue, fieldName, errors) {
@@ -342,7 +349,7 @@ function validatePlugins() {
  for (const dir of pluginDirs) {
    console.log(`Validating ${dir}...`);

-    const { errors, plugin } = validatePlugin(dir);
+    const { errors, warnings, plugin } = validatePlugin(dir);

    if (errors.length > 0) {
      console.error(`❌ ${dir}:`);
@@ -352,6 +359,10 @@ function validatePlugins() {
      console.log(`✅ ${dir} is valid`);
    }

+    if (warnings?.length > 0) {
+      warnings.forEach((w) => console.warn(`⚠️  ${dir}: ${w}`));
+    }
+
    if (plugin?.name) {
      if (seenNames.has(plugin.name)) {
        console.error(`❌ Duplicate plugin name "${plugin.name}"`);