fix(modules): validate group module create per kind (Phase 4b review)
Adversarial-review finding: `create_group_script` validated every source with `validate()` regardless of kind, so an imperatively-created group module (`pic scripts deploy --group g --name kv --kind module`) skipped the two gates every other module-write path enforces (app create/update in api.rs, declarative apply in apply_service): the stricter `validate_module` shape check and the `RESERVED_MODULE_NAMES` guard. A malformed-shape group module was accepted at create (only failing later at import-resolve time) and a reserved name (`kv`, `log`, …) slipped through. Branch on kind to mirror the app path. Adds a journey asserting a reserved group-module name is rejected. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -97,10 +97,25 @@ async fn create_group_script(
|
||||
)
|
||||
.await?;
|
||||
|
||||
// Phase 4b: group modules + imports are allowed. Imports resolve
|
||||
// lexically from this group's chain at runtime (§5.5); the recorded
|
||||
// edges feed the declarative dangling-import plan check.
|
||||
let validated = s.validator.validate(&input.source)?;
|
||||
// Phase 4b: group modules + imports are allowed. Validate per kind,
|
||||
// mirroring the app-script create path (`api.rs`): a module gets the
|
||||
// stricter shape check + the reserved-name guard; an endpoint the
|
||||
// parse-only path. Without the kind branch a malformed-shape group
|
||||
// module would be accepted here and only fail later at import time,
|
||||
// and a reserved name (`kv`, `log`, …) would slip through. Imports
|
||||
// resolve lexically from this group's chain at runtime (§5.5); the
|
||||
// recorded edges feed the declarative dangling-import plan check.
|
||||
let validated = if input.kind == ScriptKind::Module {
|
||||
if crate::api::RESERVED_MODULE_NAMES.contains(&input.name.as_str()) {
|
||||
return Err(GroupScriptsApiError::Invalid(format!(
|
||||
"{:?} is a reserved module name (shadows a built-in SDK namespace)",
|
||||
input.name
|
||||
)));
|
||||
}
|
||||
s.validator.validate_module(&input.source)?
|
||||
} else {
|
||||
s.validator.validate(&input.source)?
|
||||
};
|
||||
s.sandbox_ceiling
|
||||
.check(&input.sandbox)
|
||||
.map_err(|e| GroupScriptsApiError::Invalid(e.to_string()))?;
|
||||
|
||||
@@ -137,6 +137,37 @@ fn group_module_is_lexically_sealed_under_inheritance() {
|
||||
);
|
||||
}
|
||||
|
||||
#[ignore = "needs DATABASE_URL pointing at a running Postgres"]
|
||||
#[test]
|
||||
fn reserved_group_module_name_is_rejected() {
|
||||
// Parity with app modules (api.rs): a group `module` named after a built-in
|
||||
// SDK namespace is refused at create — not silently accepted to fail later.
|
||||
let Some(fx) = common::fixture_or_skip() else {
|
||||
return;
|
||||
};
|
||||
let env = common::admin_env(fx);
|
||||
let group = common::unique_slug("gm-rsv");
|
||||
|
||||
let _g = GroupGuard::new(&env.url, &env.token, &group);
|
||||
common::pic_as(&env)
|
||||
.args(["groups", "create", &group])
|
||||
.assert()
|
||||
.success();
|
||||
|
||||
let dir = manifest_dir();
|
||||
fs::write(dir.path().join("scripts/kv.rhai"), r#"fn x() { 1 }"#).unwrap();
|
||||
let out = common::pic_as(&env)
|
||||
.args(["scripts", "deploy"])
|
||||
.arg(dir.path().join("scripts/kv.rhai"))
|
||||
.args(["--group", &group, "--name", "kv", "--kind", "module"])
|
||||
.output()
|
||||
.expect("deploy");
|
||||
assert!(
|
||||
!out.status.success(),
|
||||
"a reserved module name must be rejected for a group module too"
|
||||
);
|
||||
}
|
||||
|
||||
#[ignore = "needs DATABASE_URL pointing at a running Postgres"]
|
||||
#[test]
|
||||
fn dangling_import_is_rejected_by_plan() {
|
||||
|
||||
Reference in New Issue
Block a user