Validate identifiers in plugin creator workflows (#39131)

## Why

Plugin and marketplace names can flow into generated install commands, so the
plugin creator must reject names outside the supported identifier syntax before
using them or changing files.

## What changed

- Centralize validation for plugin names, including dotted names, and marketplace
  names.
- Apply validation when reading marketplaces, validating manifests, updating
  cachebusters, and scaffolding plugins.
- Validate existing marketplace state before scaffold writes so invalid or
  duplicate entries leave plugin and marketplace files unchanged.

## Testing

Add regression tests for accepted identifiers, unsafe and malformed names,
dotted plugin names, and failure paths that must not modify files.

GitOrigin-RevId: 9db68c2313a1539c5ab44d777966e33b460ceb71
This commit is contained in:
xl-openai
2026-08-18 03:25:54 +00:00
committed by copyberry
parent f97e775693
commit 5ee6baee2f
8 changed files with 431 additions and 45 deletions

View File

@@ -72,9 +72,11 @@ For updates to an existing local plugin during development, keep the scaffold fl
reference instead of hand-editing marketplace files:
```bash
python3 scripts/read_marketplace_name.py
python3 scripts/update_plugin_cachebuster.py <plugin-path>
```
For a repo/team marketplace, pass `--marketplace-path <marketplace-json-path>` to the first command.
Prefer the helper default cachebuster unless the user explicitly asks for a specific override.
See `references/installing-and-updating.md` for the expected cachebuster and reinstall flow while iterating on an existing local plugin.
@@ -112,9 +114,13 @@ See `references/installing-and-updating.md` for the expected cachebuster and rei
- Do not use `--marketplace-name` to rename an existing marketplace file in place. If the file
already exists, its top-level `name` must already match.
- If the user specifies a different marketplace path, treat that marketplace as needing explicit installation via `codex plugin marketplace add`.
- Prefer `scripts/read_marketplace_name.py` when you need the marketplace name from any
`marketplace.json` file. With no argument it reads the default personal marketplace; with an
explicit path it works for repo/team marketplaces too.
- Plugin names must match `[A-Za-z0-9_-]+(\.[A-Za-z0-9_-]+)*`.
- Marketplace names must match `[A-Za-z0-9_-]+`.
- For existing marketplaces, always validate names with `scripts/read_marketplace_name.py`; stop if
validation fails. With no argument it reads the default personal marketplace; with an explicit
path it works for repo/team marketplaces too. The scaffold validates new marketplaces itself.
- Before updating existing plugins, validate both identifiers before changing files or constructing
install commands.
- In either location, the generated source path remains `./plugins/<plugin-name>`.
- Marketplace root metadata supports top-level `name` plus optional `interface.displayName`.
- Treat plugin order in `plugins[]` as render order in Codex. Append new entries unless a user explicitly asks to reorder the list.

View File

@@ -19,13 +19,31 @@ flow first and only then switch to this reinstall flow.
## Update Loop
1. Update the plugin manifest to a single Codex cachebuster suffix:
1. Before changing the plugin, read the marketplace name from the personal marketplace file:
```bash
python3 scripts/read_marketplace_name.py
```
Here, "personal marketplace" means the marketplace whose file is at
`~/.agents/plugins/marketplace.json`. On Windows, use the equivalent path under the user profile.
The helper uses Python's home-directory resolution and prints the validated marketplace name to use
when constructing the install command. If the helper fails, stop.
To read the name from a different marketplace file, pass the path directly:
```bash
python3 scripts/read_marketplace_name.py --marketplace-path <path-to-marketplace.json>
```
2. Update the plugin manifest to a single Codex cachebuster suffix:
```bash
python3 scripts/update_plugin_cachebuster.py \
<plugin-path>
```
The helper validates the plugin name before changing the manifest. If validation fails, stop.
Prefer the default helper behavior here. If you omit `--cachebuster`, the helper uses a UTC
timestamp down to seconds, which is the recommended path for routine local iteration.
@@ -38,23 +56,6 @@ python3 scripts/update_plugin_cachebuster.py \
--cachebuster local-20260519-184516
```
2. For the default scaffolded flow, read the marketplace name from the personal marketplace file:
```bash
python3 scripts/read_marketplace_name.py
```
Here, "personal marketplace" means the marketplace whose file is at
`~/.agents/plugins/marketplace.json`. On Windows, use the equivalent path under the user profile.
The helper uses Python's home-directory resolution and prints the marketplace name to use when
constructing the install command.
To read the name from a different marketplace file, pass the path directly:
```bash
python3 scripts/read_marketplace_name.py --marketplace-path <path-to-marketplace.json>
```
3. Reinstall from that marketplace name:
```bash

View File

@@ -6,9 +6,14 @@ from __future__ import annotations
import argparse
import json
import re
import sys
from pathlib import Path
from typing import Any
sys.path.insert(0, str(Path(__file__).resolve().parent))
from identifier_validation import validate_marketplace_name
MAX_PLUGIN_NAME_LENGTH = 64
DEFAULT_INSTALL_POLICY = "AVAILABLE"
@@ -40,15 +45,6 @@ def validate_plugin_name(plugin_name: str) -> None:
)
def validate_marketplace_name(marketplace_name: str) -> None:
if not marketplace_name:
raise ValueError("Marketplace name must include at least one letter or digit.")
if re.fullmatch(r"[A-Za-z0-9_-]+", marketplace_name) is None:
raise ValueError(
"Marketplace name may only contain ASCII letters, digits, `_`, and `-`."
)
def display_name_from_plugin_name(plugin_name: str) -> str:
return " ".join(part.capitalize() for part in re.split(r"[-_]+", plugin_name))
@@ -121,15 +117,12 @@ def validate_marketplace_interface(payload: dict[str, Any]) -> None:
raise ValueError("marketplace.json field 'interface' must be an object.")
def update_marketplace_json(
def load_validated_marketplace(
marketplace_path: Path,
marketplace_name: str | None,
plugin_name: str,
install_policy: str,
auth_policy: str,
category: str,
force: bool,
) -> None:
) -> dict[str, Any]:
if marketplace_path.exists():
payload = load_json(marketplace_path)
else:
@@ -141,9 +134,11 @@ def update_marketplace_json(
validate_marketplace_interface(payload)
existing_marketplace_name = payload.get("name")
if not isinstance(existing_marketplace_name, str) or not existing_marketplace_name.strip():
raise ValueError(f"{marketplace_path} must contain a non-empty string 'name'.")
validate_marketplace_name(existing_marketplace_name)
if marketplace_name is not None:
if not isinstance(existing_marketplace_name, str) or not existing_marketplace_name.strip():
raise ValueError(f"{marketplace_path} must contain a non-empty string 'name'.")
if existing_marketplace_name != marketplace_name:
raise ValueError(
f"{marketplace_path} already uses marketplace name "
@@ -154,16 +149,33 @@ def update_marketplace_json(
plugins = payload.setdefault("plugins", [])
if not isinstance(plugins, list):
raise ValueError(f"{marketplace_path} field 'plugins' must be an array.")
if not force and any(
isinstance(entry, dict) and entry.get("name") == plugin_name for entry in plugins
):
raise FileExistsError(
f"Marketplace entry '{plugin_name}' already exists in {marketplace_path}. "
"Use --force to overwrite that entry."
)
return payload
def update_marketplace_json(
marketplace_path: Path,
marketplace_name: str | None,
plugin_name: str,
install_policy: str,
auth_policy: str,
category: str,
force: bool,
) -> None:
payload = load_validated_marketplace(marketplace_path, marketplace_name, plugin_name, force)
plugins = payload["plugins"]
new_entry = build_marketplace_entry(plugin_name, install_policy, auth_policy, category)
for index, entry in enumerate(plugins):
if isinstance(entry, dict) and entry.get("name") == plugin_name:
if not force:
raise FileExistsError(
f"Marketplace entry '{plugin_name}' already exists in {marketplace_path}. "
"Use --force to overwrite that entry."
)
plugins[index] = new_entry
break
else:
@@ -266,6 +278,10 @@ def main() -> None:
marketplace_name = args.marketplace_name.strip()
validate_marketplace_name(marketplace_name)
if args.with_marketplace:
marketplace_path = Path(args.marketplace_path).expanduser().resolve()
load_validated_marketplace(marketplace_path, marketplace_name, plugin_name, args.force)
plugin_root = (Path(args.path).expanduser().resolve() / plugin_name)
plugin_root.mkdir(parents=True, exist_ok=True)
@@ -303,7 +319,6 @@ def main() -> None:
)
if args.with_marketplace:
marketplace_path = Path(args.marketplace_path).expanduser().resolve()
update_marketplace_json(
marketplace_path,
marketplace_name,

View File

@@ -0,0 +1,20 @@
"""Validate plugin and marketplace identifiers before using them in commands."""
import re
def validate_plugin_identifier(plugin_name: str) -> None:
if re.fullmatch(r"[A-Za-z0-9_-]+(?:\.[A-Za-z0-9_-]+)*", plugin_name) is None:
raise ValueError(
"Plugin name may only contain ASCII letters, digits, `.`, `_`, and `-`, "
"with dots separating non-empty name segments."
)
def validate_marketplace_name(marketplace_name: str) -> None:
if not marketplace_name:
raise ValueError("Marketplace name must include at least one letter or digit.")
if re.fullmatch(r"[A-Za-z0-9_-]+", marketplace_name) is None:
raise ValueError(
"Marketplace name may only contain ASCII letters, digits, `_`, and `-`."
)

View File

@@ -8,6 +8,10 @@ import json
import sys
from pathlib import Path
sys.path.insert(0, str(Path(__file__).resolve().parent))
from identifier_validation import validate_marketplace_name
def default_marketplace_path() -> Path:
return Path.home() / ".agents" / "plugins" / "marketplace.json"
@@ -37,7 +41,8 @@ def main() -> None:
name = payload.get("name")
if not isinstance(name, str) or not name.strip():
raise ValueError(f"{marketplace_path} must contain a non-empty string 'name'.")
print(name.strip())
validate_marketplace_name(name)
print(name)
if __name__ == "__main__":

View File

@@ -10,6 +10,10 @@ import sys
from datetime import datetime, timezone
from pathlib import Path
sys.path.insert(0, str(Path(__file__).resolve().parent))
from identifier_validation import validate_plugin_identifier
CACHEBUSTER_PREFIX = "codex"
@@ -35,6 +39,11 @@ def main() -> None:
manifest_path = plugin_root / ".codex-plugin" / "plugin.json"
manifest = load_manifest(manifest_path)
plugin_name = manifest.get("name")
if not isinstance(plugin_name, str) or not plugin_name.strip():
raise ValueError(f"{manifest_path} must contain a non-empty string 'name'.")
validate_plugin_identifier(plugin_name)
version = manifest.get("version")
if not isinstance(version, str) or not version.strip():
raise ValueError(f"{manifest_path} must contain a non-empty string 'version'.")

View File

@@ -6,12 +6,17 @@ from __future__ import annotations
import argparse
import json
import re
import sys
from pathlib import Path, PurePosixPath
from typing import Any
from urllib.parse import urlparse
import yaml
sys.path.insert(0, str(Path(__file__).resolve().parent))
from identifier_validation import validate_plugin_identifier
TODO_MARKER = "[TODO:"
SEMVER_RE = re.compile(
@@ -111,7 +116,12 @@ def validate_manifest_shape(
errors.append(f"plugin.json field `{key}` is not accepted by plugin validation")
validate_optional_non_empty_string(manifest, "id", errors)
require_non_empty_string(manifest, "name", errors)
plugin_name = require_non_empty_string(manifest, "name", errors)
if plugin_name is not None:
try:
validate_plugin_identifier(plugin_name)
except ValueError as error:
errors.append(f"plugin.json field `name` is invalid: {error}")
version = require_non_empty_string(manifest, "version", errors)
if version is not None and SEMVER_RE.fullmatch(version) is None:
errors.append("plugin.json field `version` must be strict semver")

View File

@@ -0,0 +1,320 @@
"""Local-only regression tests for the bundled plugin-creator scripts.
Run with:
python3 -B -m unittest discover -s codex-rs/skills/tests -p 'test_*.py'
"""
import json
import runpy
import subprocess
import sys
import tempfile
import types
import unittest
from pathlib import Path
from unittest.mock import patch
SCRIPTS = (
Path(__file__).resolve().parents[1]
/ "src"
/ "assets"
/ "samples"
/ "plugin-creator"
/ "scripts"
)
class PluginCreatorSecurityTests(unittest.TestCase):
def setUp(self) -> None:
self.directory = tempfile.TemporaryDirectory()
self.addCleanup(self.directory.cleanup)
self.root = Path(self.directory.name)
self.marketplace_path = self.root / "marketplace.json"
self.plugin_root = self.root / "plugins" / "demo"
def run_script(self, script: str, *arguments: str) -> subprocess.CompletedProcess[str]:
return subprocess.run(
[sys.executable, "-B", str(SCRIPTS / script), *arguments],
capture_output=True,
text=True,
check=False,
)
def write_marketplace(self, name: object) -> None:
self.marketplace_path.write_text(
json.dumps({"name": name, "plugins": []}),
encoding="utf-8",
)
def write_plugin(self, name: object) -> Path:
manifest_path = self.plugin_root / ".codex-plugin" / "plugin.json"
manifest_path.parent.mkdir(parents=True, exist_ok=True)
manifest_path.write_text(
json.dumps({"name": name, "version": "1.0.0"}),
encoding="utf-8",
)
return manifest_path
def plugin_validation_errors(self, name: str) -> list[str]:
with (
patch.dict(sys.modules, {"yaml": types.ModuleType("yaml")}),
patch.object(sys, "path", [str(SCRIPTS), *sys.path]),
patch.object(sys, "dont_write_bytecode", True),
):
scaffold = runpy.run_path(str(SCRIPTS / "create_basic_plugin.py"))
validator = runpy.run_path(str(SCRIPTS / "validate_plugin.py"))
manifest = scaffold["build_plugin_json"]("demo", with_mcp=False, with_apps=False)
manifest["name"] = name
errors: list[str] = []
validator["validate_manifest_shape"](self.plugin_root, manifest, errors)
return errors
def test_marketplace_reader_accepts_valid_names(self) -> None:
for name in ("personal", "team-local", "team_local_123", "ABC123_-", "_", "-"):
with self.subTest(name=name):
self.write_marketplace(name)
result = self.run_script(
"read_marketplace_name.py",
"--marketplace-path",
str(self.marketplace_path),
)
self.assertEqual(result.returncode, 0, result.stderr)
self.assertEqual(result.stdout, f"{name}\n")
def test_marketplace_reader_rejects_unsafe_names_without_stdout(self) -> None:
unsafe_names = (
"team;id",
"team\nlocal",
"team local",
" team",
"team ",
"team'local",
'team"local',
"team$(id)",
"team`id`",
"team&local",
"team|local",
"team<local",
"team>local",
"team.local",
"équipe",
"",
)
for name in unsafe_names:
with self.subTest(name=name):
self.write_marketplace(name)
result = self.run_script(
"read_marketplace_name.py",
"--marketplace-path",
str(self.marketplace_path),
)
self.assertNotEqual(result.returncode, 0)
self.assertEqual(result.stdout, "")
def test_marketplace_reader_rejects_missing_and_nonstring_names(self) -> None:
for payload in ({}, {"name": None}, {"name": 123}, {"name": ["team"]}, []):
with self.subTest(payload=payload):
self.marketplace_path.write_text(json.dumps(payload), encoding="utf-8")
result = self.run_script(
"read_marketplace_name.py",
"--marketplace-path",
str(self.marketplace_path),
)
self.assertNotEqual(result.returncode, 0)
self.assertEqual(result.stdout, "")
def test_marketplace_reader_rejects_malformed_json(self) -> None:
self.marketplace_path.write_text("{", encoding="utf-8")
result = self.run_script(
"read_marketplace_name.py",
"--marketplace-path",
str(self.marketplace_path),
)
self.assertNotEqual(result.returncode, 0)
self.assertEqual(result.stdout, "")
def test_plugin_validator_accepts_canonical_names(self) -> None:
for name in ("demo", "Team_Name-1", "team.tools", "team.tools_v2"):
with self.subTest(name=name):
self.assertEqual(self.plugin_validation_errors(name), [])
def test_plugin_validator_rejects_unsafe_names(self) -> None:
unsafe_names = (
"safe;id",
"$(id)",
"safe\nid",
"safe name",
"safe`id`",
".hidden",
"trailing.",
"safe..name",
"équipe",
)
for name in unsafe_names:
with self.subTest(name=name):
errors = self.plugin_validation_errors(name)
self.assertTrue(errors)
self.assertTrue(any("name" in error for error in errors))
def test_cachebuster_rejects_unsafe_plugin_names_without_writing(self) -> None:
for name in ("safe;id", "$(id)", "safe\nid", ".hidden", "safe..name"):
with self.subTest(name=name):
manifest_path = self.write_plugin(name)
original = manifest_path.read_bytes()
result = self.run_script("update_plugin_cachebuster.py", str(self.plugin_root))
self.assertNotEqual(result.returncode, 0)
self.assertEqual(result.stdout, "")
self.assertEqual(manifest_path.read_bytes(), original)
def test_cachebuster_accepts_dotted_plugin_names(self) -> None:
manifest_path = self.write_plugin("demo.tools")
result = self.run_script(
"update_plugin_cachebuster.py",
str(self.plugin_root),
"--cachebuster",
"safe-token",
)
self.assertEqual(result.returncode, 0, result.stderr)
manifest = json.loads(manifest_path.read_text(encoding="utf-8"))
self.assertEqual(manifest["name"], "demo.tools")
self.assertEqual(manifest["version"], "1.0.0+codex.safe-token")
def test_scaffold_rejects_invalid_marketplace_before_creating_files(self) -> None:
self.write_marketplace("team;id")
original = self.marketplace_path.read_bytes()
result = self.run_script(
"create_basic_plugin.py",
"demo",
"--path",
str(self.root / "plugins"),
"--with-marketplace",
"--marketplace-path",
str(self.marketplace_path),
)
self.assertNotEqual(result.returncode, 0)
self.assertFalse(self.plugin_root.exists())
self.assertEqual(self.marketplace_path.read_bytes(), original)
def test_scaffold_force_preserves_files_when_marketplace_is_invalid(self) -> None:
self.write_marketplace("team;id")
plugin_manifest = self.write_plugin("demo")
mcp_manifest = self.plugin_root / ".mcp.json"
app_manifest = self.plugin_root / ".app.json"
mcp_manifest.write_text('{"mcpServers":{"existing":{}}}', encoding="utf-8")
app_manifest.write_text('{"apps":{"existing":{}}}', encoding="utf-8")
originals = {
path: path.read_bytes()
for path in (self.marketplace_path, plugin_manifest, mcp_manifest, app_manifest)
}
result = self.run_script(
"create_basic_plugin.py",
"demo",
"--path",
str(self.root / "plugins"),
"--with-marketplace",
"--marketplace-path",
str(self.marketplace_path),
"--with-mcp",
"--with-apps",
"--with-skills",
"--force",
)
self.assertNotEqual(result.returncode, 0)
self.assertFalse((self.plugin_root / "skills").exists())
for path, original in originals.items():
with self.subTest(path=path):
self.assertEqual(path.read_bytes(), original)
def test_scaffold_force_preserves_files_when_plugins_field_is_invalid(self) -> None:
self.marketplace_path.write_text(
json.dumps({"name": "team-local", "plugins": {}}),
encoding="utf-8",
)
plugin_manifest = self.write_plugin("demo")
mcp_manifest = self.plugin_root / ".mcp.json"
app_manifest = self.plugin_root / ".app.json"
mcp_manifest.write_text('{"mcpServers":{"existing":{}}}', encoding="utf-8")
app_manifest.write_text('{"apps":{"existing":{}}}', encoding="utf-8")
originals = {
path: path.read_bytes()
for path in (self.marketplace_path, plugin_manifest, mcp_manifest, app_manifest)
}
result = self.run_script(
"create_basic_plugin.py",
"demo",
"--path",
str(self.root / "plugins"),
"--with-marketplace",
"--marketplace-path",
str(self.marketplace_path),
"--with-mcp",
"--with-apps",
"--with-skills",
"--force",
)
self.assertNotEqual(result.returncode, 0)
self.assertFalse((self.plugin_root / "skills").exists())
for path, original in originals.items():
with self.subTest(path=path):
self.assertEqual(path.read_bytes(), original)
def test_scaffold_rejects_duplicate_marketplace_entry_before_creating_files(self) -> None:
self.marketplace_path.write_text(
json.dumps({"name": "team-local", "plugins": [{"name": "demo"}]}),
encoding="utf-8",
)
original = self.marketplace_path.read_bytes()
result = self.run_script(
"create_basic_plugin.py",
"demo",
"--path",
str(self.root / "plugins"),
"--with-marketplace",
"--marketplace-path",
str(self.marketplace_path),
)
self.assertNotEqual(result.returncode, 0)
self.assertFalse(self.plugin_root.exists())
self.assertEqual(self.marketplace_path.read_bytes(), original)
def test_scaffold_accepts_existing_valid_marketplace(self) -> None:
self.write_marketplace("team-local_123")
result = self.run_script(
"create_basic_plugin.py",
"demo",
"--path",
str(self.root / "plugins"),
"--with-marketplace",
"--marketplace-path",
str(self.marketplace_path),
)
self.assertEqual(result.returncode, 0, result.stderr)
marketplace = json.loads(self.marketplace_path.read_text(encoding="utf-8"))
self.assertEqual(marketplace["name"], "team-local_123")
self.assertEqual(marketplace["plugins"][0]["name"], "demo")
def test_scaffold_creates_missing_personal_marketplace(self) -> None:
self.assertFalse(self.marketplace_path.exists())
result = self.run_script(
"create_basic_plugin.py",
"demo",
"--path",
str(self.root / "plugins"),
"--with-marketplace",
"--marketplace-path",
str(self.marketplace_path),
)
self.assertEqual(result.returncode, 0, result.stderr)
marketplace = json.loads(self.marketplace_path.read_text(encoding="utf-8"))
self.assertEqual(marketplace["name"], "personal")
self.assertEqual(marketplace["plugins"][0]["name"], "demo")
if __name__ == "__main__":
unittest.main()