Skip to content

Commit d787887

Browse files
committed
fix(validator): 第二轮评审加固(null 字段/占位符逃逸/符号链接/空白/命名)
针对 PR #81 复审意见,全部落在 validate_portable_plugin.py: - 显式 "version"/"keywords"/"author"/"extensions": null 现在判为类型违规, 不再被 .get() 当作缺席放过 - ${PLUGIN_ROOT}/${PLUGIN_DATA} 开头的 cwd 做占位符展开后的包含性检查, ${PLUGIN_ROOT}/../../outside 这类逃逸不再 PASS - 技能发现跳过点前缀目录(.scratch 等主机元数据);符号链接技能目录与其 SKILL.md 解析到包根之外时报错而非校验外部文件 - stdio command 改为拒绝任何空白字符(制表符/换行 Previously 只查空格) - 技能 frontmatter name 收紧为 kebab-case(小写字母数字连字符)并限长 64, 不再沿用插件名语法(技能名不含句点)
1 parent 2f78bdf commit d787887

1 file changed

Lines changed: 89 additions & 16 deletions

File tree

‎scripts/validate_portable_plugin.py‎

Lines changed: 89 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -182,24 +182,34 @@ def check_plugin_manifest(root: Path, report: Report) -> dict | None:
182182
f"must start and end alphanumeric; no '--' or '..').",
183183
)
184184

185-
version = manifest.get("version")
186-
if version is not None and not isinstance(version, str):
187-
report.fail(where, f"version must be a string, got {type(version).__name__} (§5.4)")
185+
# §5.4: every metadata field, when present, MUST have its declared JSON type.
186+
# An explicit `null` is a type violation, not absence — detect it with `in`.
187+
if "version" in manifest and not isinstance(manifest["version"], str):
188+
report.fail(
189+
where,
190+
f"version must be a string, got {type(manifest['version']).__name__} (§5.4)",
191+
)
188192

189-
keywords = manifest.get("keywords")
190-
if keywords is not None and (
191-
not isinstance(keywords, list) or not all(isinstance(k, str) for k in keywords)
193+
if "keywords" in manifest and (
194+
not isinstance(manifest["keywords"], list)
195+
or not all(isinstance(k, str) for k in manifest["keywords"])
192196
):
193197
report.fail(where, "keywords must be an array of strings (§5.4)")
194198

195199
for scalar in ("description", "homepage", "repository", "license"):
196200
if scalar in manifest and not isinstance(manifest[scalar], str):
197-
report.fail(where, f"{scalar} must be a string (§5.4)")
201+
report.fail(
202+
where,
203+
f"{scalar} must be a string, got {type(manifest[scalar]).__name__} (§5.4)",
204+
)
198205

199-
author = manifest.get("author")
200-
if author is not None:
206+
if "author" in manifest:
207+
author = manifest["author"]
201208
if not isinstance(author, dict):
202-
report.fail(where, "author must be an object (§5.4)")
209+
report.fail(
210+
where,
211+
f"author must be an object, got {type(author).__name__} (§5.4)",
212+
)
203213
else:
204214
extra = sorted(set(author) - AUTHOR_ALLOWED)
205215
if extra:
@@ -210,10 +220,13 @@ def check_plugin_manifest(root: Path, report: Report) -> dict | None:
210220
)
211221
for key, value in author.items():
212222
if not isinstance(value, str):
213-
report.fail(where, f"author.{key} must be a string (§5.4)")
223+
report.fail(
224+
where,
225+
f"author.{key} must be a string, got {type(value).__name__} (§5.4)",
226+
)
214227

215-
extensions = manifest.get("extensions")
216-
if extensions is not None:
228+
if "extensions" in manifest:
229+
extensions = manifest["extensions"]
217230
if not isinstance(extensions, dict):
218231
# Non-fatal: the client reports and ignores it (§8.1).
219232
report.fail(
@@ -346,7 +359,9 @@ def _check_stdio(where: str, entry: dict, root: Path, report: Report) -> None:
346359
command = entry["command"]
347360
if not isinstance(command, str) or not command:
348361
report.fail(where, "command must be a non-empty string (§7.2.1)")
349-
elif " " in command.strip():
362+
elif any(ch.isspace() for ch in command):
363+
# §7.2.1: one executable token. Any whitespace — space, tab, newline —
364+
# means it is a shell command string, not a token.
350365
report.fail(
351366
where,
352367
f"command {command!r} must be a single executable token, not a shell "
@@ -400,6 +415,32 @@ def _check_stdio(where: str, entry: dict, root: Path, report: Report) -> None:
400415
target.relative_to(root.resolve())
401416
except ValueError:
402417
report.fail(where, f"cwd {cwd!r} escapes the plugin root (§4.1)")
418+
else:
419+
# §9.2 + §4.1: expand the declared placeholder and verify containment.
420+
# `${PLUGIN_ROOT}/../../outside` must not pass as conformant.
421+
if cwd.startswith("${PLUGIN_DATA}"):
422+
base = _plugin_data_dir(root)
423+
expanded = base / cwd[len("${PLUGIN_DATA}"):].lstrip("/")
424+
else:
425+
base = root.resolve()
426+
expanded = base / cwd[len("${PLUGIN_ROOT}"):].lstrip("/")
427+
try:
428+
expanded.resolve().relative_to(base.resolve())
429+
except ValueError:
430+
report.fail(
431+
where,
432+
f"cwd {cwd!r} escapes its declared root after placeholder "
433+
f"expansion (§4.1)",
434+
)
435+
436+
437+
def _plugin_data_dir(root: Path) -> Path:
438+
"""The client-managed PLUGIN_DATA location for this installed instance.
439+
440+
Mirrors the reference layout clients use: a dedicated writable directory
441+
that persists across updates. Validation only needs a stable anchor for
442+
containment checks."""
443+
return root.resolve().parent / "data" / root.name
403444

404445

405446
def _check_remote(where: str, entry: dict, report: Report) -> None:
@@ -465,7 +506,10 @@ def _check_remote(where: str, entry: dict, report: Report) -> None:
465506

466507
# --- skills/ (§6.1, §7.1) --------------------------------------------------
467508

468-
FRONTMATTER_NAME_RE = re.compile(r"^(?!.*(?:--|\.\.))[a-z0-9](?:[a-z0-9.-]*[a-z0-9])?$")
509+
# Agent Skills identifiers: lowercase alphanumerics and hyphens (kebab-case),
510+
# 1-64 characters. This is deliberately stricter than the plugin-name grammar
511+
# (§5.5) — skill names do not use periods.
512+
FRONTMATTER_NAME_RE = re.compile(r"^(?!.*--)[a-z0-9](?:[a-z0-9-]*[a-z0-9])?$")
469513

470514

471515
def check_skills(root: Path, report: Report) -> tuple[int, list[str]]:
@@ -482,16 +526,39 @@ def check_skills(root: Path, report: Report) -> tuple[int, list[str]]:
482526
)
483527
return 0, []
484528

529+
root_resolved = root.resolve()
485530
discovered: list[str] = []
486531
for child in sorted(skills_dir.iterdir()):
532+
# Dot-prefixed entries are host/tool metadata, never skills.
533+
if child.name.startswith("."):
534+
continue
487535
if not child.is_dir():
488536
# Stray files are not skills; §7.1 only looks at immediate child dirs.
489537
report.note(f"skills/{child.name} is a file, not a skill directory")
490538
continue
539+
# §4.1: symlinks may resolve within the plugin root, but a skill whose
540+
# directory resolves outside it must be skipped, not validated.
541+
if child.is_symlink():
542+
try:
543+
child.resolve().relative_to(root_resolved)
544+
except ValueError:
545+
report.fail(
546+
where,
547+
f"skills/{child.name} is a symlink escaping the plugin root (§4.1)",
548+
)
549+
continue
491550
skill_md = child / "SKILL.md"
492551
if not skill_md.is_file():
493552
report.note(f"skills/{child.name}/ has no SKILL.md and is not a skill")
494553
continue
554+
try:
555+
skill_md.resolve().relative_to(root_resolved)
556+
except ValueError:
557+
report.fail(
558+
f"skills/{child.name}/SKILL.md",
559+
"resolves outside the plugin root (§4.1)",
560+
)
561+
continue
495562
discovered.append(child.name)
496563
check_skill_md(f"skills/{child.name}/SKILL.md", child, skill_md, report)
497564
return len(discovered), discovered
@@ -524,8 +591,14 @@ def check_skill_md(where: str, skill_dir: Path, path: Path, report: Report) -> N
524591
if not name:
525592
report.fail(where, "frontmatter is missing `name`")
526593
else:
594+
if not 1 <= len(name) <= 64:
595+
report.fail(where, f"frontmatter name is {len(name)} characters, must be 1-64")
527596
if not FRONTMATTER_NAME_RE.match(name):
528-
report.fail(where, f"frontmatter name {name!r} is not a valid skill identifier")
597+
report.fail(
598+
where,
599+
f"frontmatter name {name!r} is not a valid skill identifier "
600+
f"(lowercase alphanumerics and hyphens only)",
601+
)
529602
if name != skill_dir.name:
530603
report.fail(
531604
where,

0 commit comments

Comments
 (0)