From ccc800640864fc88303bb7339a2fb52d6ba6719c Mon Sep 17 00:00:00 2001 From: XiaoSeS <87064762+XiaoSeS@users.noreply.github.com> Date: Fri, 23 Oct 2026 17:55:41 +0800 Subject: [PATCH] fix(authoring): enforce deployment environment authorization Signed-off-by: XiaoSeS <87064762+XiaoSeS@users.noreply.github.com> --- compose.release.yml | 1 + docs/27-authoring-drafts.md | 29 +++++ .../config/AuthoringEnvironmentConfig.java | 15 +++ .../skillhub/config/AuthoringProperties.java | 11 ++ .../adapter/LocalScriptRuntimeAdapter.java | 44 ++++--- .../src/main/resources/application.yml | 2 + .../AuthoringEnvironmentConfigTest.java | 63 ++++++++++ ...calScriptEnvironmentAuthorizationTest.java | 112 ++++++++++++++++++ .../LocalScriptEnvironmentExecutionTest.java | 108 +++++++++++++++++ .../service/RuntimeBindingValidator.java | 13 +- .../service/ScriptEnvironmentPolicy.java | 61 ++++++++++ ...meBindingEnvironmentAuthorizationTest.java | 55 +++++++++ .../service/ScriptEnvironmentPolicyTest.java | 70 +++++++++++ 13 files changed, 567 insertions(+), 17 deletions(-) create mode 100644 server/skillhub-app/src/main/java/com/iflytek/skillhub/config/AuthoringEnvironmentConfig.java create mode 100644 server/skillhub-app/src/test/java/com/iflytek/skillhub/config/AuthoringEnvironmentConfigTest.java create mode 100644 server/skillhub-app/src/test/java/com/iflytek/skillhub/service/authoring/adapter/LocalScriptEnvironmentAuthorizationTest.java create mode 100644 server/skillhub-app/src/test/java/com/iflytek/skillhub/service/authoring/adapter/LocalScriptEnvironmentExecutionTest.java create mode 100644 server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/authoring/service/ScriptEnvironmentPolicy.java create mode 100644 server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/authoring/service/RuntimeBindingEnvironmentAuthorizationTest.java create mode 100644 server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/authoring/service/ScriptEnvironmentPolicyTest.java diff --git a/compose.release.yml b/compose.release.yml index bd8ca4997..ba1ee5dc8 100644 --- a/compose.release.yml +++ b/compose.release.yml @@ -116,6 +116,7 @@ services: SKILLHUB_SECURITY_SCANNER_RETRY_MAX: ${SKILLHUB_SECURITY_SCANNER_RETRY_MAX:-3} SKILLHUB_SCAN_STREAM_RECLAIM_MIN_IDLE: ${SKILLHUB_SCAN_STREAM_RECLAIM_MIN_IDLE:-PT16M} SKILLHUB_BUILTIN_SKILLS_ENABLED: ${SKILLHUB_BUILTIN_SKILLS_ENABLED:-true} + SKILLHUB_AUTHORING_SCRIPT_ENVIRONMENT_ALLOWLIST: ${SKILLHUB_AUTHORING_SCRIPT_ENVIRONMENT_ALLOWLIST:-} SKILLHUB_AUTH_DIRECT_ENABLED: ${SKILLHUB_AUTH_DIRECT_ENABLED:-false} SKILLHUB_TRACING_MODE: ${SKILLHUB_TRACING_MODE:-none} SKILLHUB_LOG_FORMAT: ${SKILLHUB_LOG_FORMAT:-json} diff --git a/docs/27-authoring-drafts.md b/docs/27-authoring-drafts.md index b2219a997..a2350bad7 100644 --- a/docs/27-authoring-drafts.md +++ b/docs/27-authoring-drafts.md @@ -27,3 +27,32 @@ 数据库迁移为未发布的 `V69__authoring_platform.sql`。正 revision、非负大小／计数及状态转换在业务代码中验证；数据库保留主键、外键和唯一性结构约束。头像／邀请 PR #947 使用 V70–V74，须在 V69 合入后验证其升级顺序，避免把较低编号的迁移留到已应用较高版本之后。 API 的完整字段、操作与响应以服务生成的 OpenAPI 为准；本文件不替代接口定义。 + +## 本地脚本的环境变量授权 + +部署管理员通过 `skillhub.authoring.local-script.environment-allowlist` 设置可选变量名称的上限； +环境覆盖项为 `SKILLHUB_AUTHORING_SCRIPT_ENVIRONMENT_ALLOWLIST`，多个名称用英文逗号分隔。 +缺省为空：除适配器固定的启动环境外，草稿不能选择额外的服务器变量。例如部署可以显式允许 +`AUTHORING_DEMO_SETTING`，草稿绑定的 `envAllowlist` 只能选择该名称或更小的子集，不能自行授予权限。 +名称必须是匹配 `[A-Z][A-Z0-9_]{0,63}` 的大写字符串，区分大小写；绑定不保存变量值。 +已获准但在服务器进程中未定义的变量不转发。 + +此配置向所有能够运行本地脚本的草稿拥有者授予权限，包括普通成员；它不是按用户或命名空间 +划分的凭据授权。只允许可以向这些用户披露的值，因为脚本及运行日志均可能暴露收到的值。 +不要为了运行草稿而将服务器凭据加入此集合；此类需求应另行设计有范围限制的凭据绑定。 + +绑定保存、每轮 CONFIG 验证和每次任务执行前均使用同一策略检查。请求中只要有一个未授权名称， +整组请求就在读取宿主变量之前被拒绝，不静默丢弃非法名称。任务执行入口还会在后端探测、 +执行计划和进程创建之前复验；CONFIG 层记录拒绝后可能仍检查后端可用性，但不会因此获得变量权限。 +已有或排队的绑定不能保留旧权限，执行时仍须通过当前已生效配置。 +更改部署配置后必须重启应用才能新增或撤销权限；没有自动刷新磁盘配置的机制。 +已运行进程中的值无法召回，轮换或撤销已转发的值之前应按现有取消流程停止相应运行。 + +`PATH`、`HOME`、`LANG` 及 Windows 启动名称 `SYSTEMROOT`、`WINDIR`、`COMSPEC`、 +`PATHEXT`、`TEMP`、`TMP` 均为保留项，部署集合不能包含它们，草稿也不能覆盖适配器的可信启动环境。 +授权规则同时适用于 INLINE 和 DOCKER；默认执行模式仍为 Docker。 +Docker CLI 的控制环境继续继承以保持守护进程连接配置，只有通过授权的已清理环境映射会以 +`-e` 形式传给脚本容器。容器隔离和输出脱敏均不能替代环境授权。 + +`compose.release.yml` 已为空默认透传此设置；其他自定义 Compose/Helm 部署需显式将同名变量 +传入服务器进程，或使用应用配置覆盖。仅在外层 shell 或未透传的 env 文件中设置不代表应用已生效。 diff --git a/server/skillhub-app/src/main/java/com/iflytek/skillhub/config/AuthoringEnvironmentConfig.java b/server/skillhub-app/src/main/java/com/iflytek/skillhub/config/AuthoringEnvironmentConfig.java new file mode 100644 index 000000000..981f758c3 --- /dev/null +++ b/server/skillhub-app/src/main/java/com/iflytek/skillhub/config/AuthoringEnvironmentConfig.java @@ -0,0 +1,15 @@ +package com.iflytek.skillhub.config; + +import com.iflytek.skillhub.domain.authoring.service.ScriptEnvironmentPolicy; +import org.springframework.context.annotation.Bean; +import org.springframework.context.annotation.Configuration; + +/** Wires deployment configuration into the domain policy without a reverse module dependency. */ +@Configuration +public class AuthoringEnvironmentConfig { + + @Bean + public ScriptEnvironmentPolicy scriptEnvironmentPolicy(AuthoringProperties properties) { + return new ScriptEnvironmentPolicy(() -> properties.getLocalScript().getEnvironmentAllowlist()); + } +} diff --git a/server/skillhub-app/src/main/java/com/iflytek/skillhub/config/AuthoringProperties.java b/server/skillhub-app/src/main/java/com/iflytek/skillhub/config/AuthoringProperties.java index e29fd76b1..011ac03f2 100644 --- a/server/skillhub-app/src/main/java/com/iflytek/skillhub/config/AuthoringProperties.java +++ b/server/skillhub-app/src/main/java/com/iflytek/skillhub/config/AuthoringProperties.java @@ -68,6 +68,17 @@ public class AuthoringProperties { /** Where scripts run: INLINE on the server host (dev), DOCKER in a locked-down container (production). */ private ScriptExecutionMode executionMode = ScriptExecutionMode.INLINE; + /** Deployment-owned variable names available to all local-script draft owners. Empty denies all. */ + private List environmentAllowlist = List.of(); + + public List getEnvironmentAllowlist() { + return environmentAllowlist; + } + + public void setEnvironmentAllowlist(List environmentAllowlist) { + this.environmentAllowlist = environmentAllowlist == null ? List.of() : List.copyOf(environmentAllowlist); + } + private final Docker docker = new Docker(); public boolean isEnabled() { diff --git a/server/skillhub-app/src/main/java/com/iflytek/skillhub/service/authoring/adapter/LocalScriptRuntimeAdapter.java b/server/skillhub-app/src/main/java/com/iflytek/skillhub/service/authoring/adapter/LocalScriptRuntimeAdapter.java index df1f1ae7f..8b44507ea 100644 --- a/server/skillhub-app/src/main/java/com/iflytek/skillhub/service/authoring/adapter/LocalScriptRuntimeAdapter.java +++ b/server/skillhub-app/src/main/java/com/iflytek/skillhub/service/authoring/adapter/LocalScriptRuntimeAdapter.java @@ -1,6 +1,7 @@ package com.iflytek.skillhub.service.authoring.adapter; import com.iflytek.skillhub.config.AuthoringProperties; +import com.iflytek.skillhub.domain.authoring.service.ScriptEnvironmentPolicy; import com.iflytek.skillhub.domain.authoring.runtime.RuntimeEventSink; import com.iflytek.skillhub.domain.authoring.runtime.RuntimeExecutionContext; import com.iflytek.skillhub.domain.authoring.runtime.SkillRuntimeAdapter; @@ -24,7 +25,7 @@ import org.springframework.stereotype.Component; /** * Behavior-layer adapter that executes the skill's own scripts in an isolated working * directory with a scrubbed environment. Only whitelisted interpreters may run and only - * environment variables named in the binding's {@code envAllowlist} pass through. + * deployment-authorized variables selected by the binding's {@code envAllowlist} pass through. * *

Where the process actually runs is delegated to a {@link ScriptCommandBuilder}: * the inline backend spawns the interpreter on the server host, the docker backend @@ -38,11 +39,24 @@ public class LocalScriptRuntimeAdapter implements SkillRuntimeAdapter { private static final long POLL_SLICE_MS = 200; private final AuthoringProperties properties; + private final ScriptEnvironmentPolicy environmentPolicy; + private final Function environmentLookup; private final Map commandBuilders; + @org.springframework.beans.factory.annotation.Autowired public LocalScriptRuntimeAdapter(AuthoringProperties properties, - List commandBuilders) { + List commandBuilders, + ScriptEnvironmentPolicy environmentPolicy) { + this(properties, commandBuilders, environmentPolicy, System::getenv); + } + + LocalScriptRuntimeAdapter(AuthoringProperties properties, + List commandBuilders, + ScriptEnvironmentPolicy environmentPolicy, + Function environmentLookup) { this.properties = properties; + this.environmentPolicy = environmentPolicy; + this.environmentLookup = environmentLookup; this.commandBuilders = commandBuilders.stream() .collect(Collectors.toUnmodifiableMap(ScriptCommandBuilder::backend, Function.identity())); @@ -66,6 +80,8 @@ public class LocalScriptRuntimeAdapter implements SkillRuntimeAdapter { @Override public TaskResult execute(RuntimeExecutionContext context, ValidationTaskSpec task, RuntimeEventSink sink) throws Exception { + // Recheck the effective policy before probing a backend or creating a process. + Map scrubbedEnvironment = scrubEnvironment(context); ScriptCommandBuilder builder = commandBuilder(); if (!builder.available()) { throw new IllegalStateException( @@ -75,8 +91,6 @@ public class LocalScriptRuntimeAdapter implements SkillRuntimeAdapter { String interpreter = resolveInterpreter(context); Path scriptPath = resolveInsideWorkingDirectory(context.workingDirectory(), task.script()); String runTag = context.runId() + "-" + task.name(); - Map scrubbedEnvironment = scrubEnvironment(context); - ScriptCommandBuilder.ScriptExecutionPlan plan = builder.plan( context.workingDirectory(), interpreter, scriptPath, task.safeArgs(), scrubbedEnvironment, runTag); @@ -132,20 +146,20 @@ public class LocalScriptRuntimeAdapter implements SkillRuntimeAdapter { return "sh"; } - private Map scrubEnvironment(RuntimeExecutionContext context) { + Map scrubEnvironment(RuntimeExecutionContext context) { + // Authorize the entire selection before looking up any host value. An old persisted + // binding does not retain permission after the effective deployment policy changes. + java.util.Set selected = environmentPolicy.authorize( + context.safeAdapterConfig().get("envAllowlist")); Map environment = new java.util.LinkedHashMap<>(); - environment.put("PATH", System.getenv().getOrDefault("PATH", "/usr/bin:/bin")); + String path = environmentLookup.apply("PATH"); + environment.put("PATH", path == null ? "/usr/bin:/bin" : path); environment.put("HOME", context.workingDirectory().toString()); environment.put("LANG", "C.UTF-8"); - Object allowlist = context.safeAdapterConfig().get("envAllowlist"); - if (allowlist instanceof List entries) { - for (Object entry : entries) { - if (entry != null) { - String value = System.getenv(entry.toString()); - if (value != null) { - environment.put(entry.toString(), value); - } - } + for (String name : selected) { + String value = environmentLookup.apply(name); + if (value != null) { + environment.put(name, value); } } return environment; diff --git a/server/skillhub-app/src/main/resources/application.yml b/server/skillhub-app/src/main/resources/application.yml index 4b317419e..fc6188030 100644 --- a/server/skillhub-app/src/main/resources/application.yml +++ b/server/skillhub-app/src/main/resources/application.yml @@ -145,6 +145,8 @@ skillhub: maintenance-interval-ms: ${SKILLHUB_AUTHORING_MAINTENANCE_INTERVAL_MS:30000} local-script: enabled: ${SKILLHUB_AUTHORING_LOCAL_SCRIPT_ENABLED:true} + # Deployment ceiling; draft envAllowlist may only select a subset. Empty denies all. + environment-allowlist: ${SKILLHUB_AUTHORING_SCRIPT_ENVIRONMENT_ALLOWLIST:} # docker is the production default: scripts run in a locked-down container — # no network, memory/cpu/pids caps, read-only rootfs, all capabilities dropped. # inline runs subprocesses on the server host and is only allowed with a diff --git a/server/skillhub-app/src/test/java/com/iflytek/skillhub/config/AuthoringEnvironmentConfigTest.java b/server/skillhub-app/src/test/java/com/iflytek/skillhub/config/AuthoringEnvironmentConfigTest.java new file mode 100644 index 000000000..ef22c930f --- /dev/null +++ b/server/skillhub-app/src/test/java/com/iflytek/skillhub/config/AuthoringEnvironmentConfigTest.java @@ -0,0 +1,63 @@ +package com.iflytek.skillhub.config; + +import java.util.List; +import java.util.Map; +import org.junit.jupiter.api.Test; +import org.springframework.boot.context.properties.bind.Bindable; +import org.springframework.boot.context.properties.bind.Binder; +import org.springframework.boot.context.properties.source.MapConfigurationPropertySource; +import org.springframework.boot.context.properties.EnableConfigurationProperties; +import org.springframework.context.annotation.AnnotationConfigApplicationContext; +import org.springframework.context.annotation.Configuration; +import org.springframework.core.env.MapPropertySource; +import com.iflytek.skillhub.domain.authoring.service.RuntimeBindingValidator; +import com.iflytek.skillhub.service.authoring.adapter.LocalScriptRuntimeAdapter; +import com.iflytek.skillhub.service.authoring.adapter.InlineScriptCommandBuilder; +import static org.assertj.core.api.Assertions.*; + +class AuthoringEnvironmentConfigTest { + @Configuration(proxyBeanMethods = false) + @EnableConfigurationProperties(AuthoringProperties.class) + static class PropertiesConfiguration { } + + @Test void springContainerInjectsTheSharedPolicyIntoBothEntryPoints() { + try (var context = new AnnotationConfigApplicationContext()) { + context.getEnvironment().getPropertySources().addFirst(new MapPropertySource("dummy-deployment", Map.of( + "skillhub.authoring.local-script.environment-allowlist", "AUTHORING_DEMO"))); + context.register(PropertiesConfiguration.class, AuthoringEnvironmentConfig.class, + RuntimeBindingValidator.class, LocalScriptRuntimeAdapter.class, InlineScriptCommandBuilder.class); + context.refresh(); + var validator = context.getBean(RuntimeBindingValidator.class); + assertThat(context.getBean(LocalScriptRuntimeAdapter.class).agentType()).isEqualTo("local-script"); + var config = Map.of("envAllowlist", List.of("AUTHORING_DEMO")); + assertThat(validator.validate("local-script", config, null, null)).isEmpty(); + context.getBean(AuthoringProperties.class).getLocalScript().setEnvironmentAllowlist(List.of()); + assertThat(validator.validate("local-script", config, null, null)).hasSize(1); + } + } + @Test void defaultPropertiesDenyAllAndBeanSharesEffectiveSettings() { + var properties = new AuthoringProperties(); + var policy = new AuthoringEnvironmentConfig().scriptEnvironmentPolicy(properties); + assertThatThrownBy(() -> policy.authorize(List.of("AUTHORING_DEMO"))).isInstanceOf(IllegalArgumentException.class); + properties.getLocalScript().setEnvironmentAllowlist(List.of("AUTHORING_DEMO")); + assertThat(policy.authorize(List.of("AUTHORING_DEMO"))).containsExactly("AUTHORING_DEMO"); + properties.getLocalScript().setEnvironmentAllowlist(null); + assertThatThrownBy(() -> policy.authorize(List.of("AUTHORING_DEMO"))).isInstanceOf(IllegalArgumentException.class); + } + @Test void commaSeparatedSettingBindsWithoutChangingDockerMode() { + var source = new MapConfigurationPropertySource(Map.of( + "skillhub.authoring.local-script.environment-allowlist", "AUTHORING_DEMO,AUTHORING_OTHER", + "skillhub.authoring.local-script.execution-mode", "docker")); + var properties = new Binder(source).bind("skillhub.authoring", Bindable.of(AuthoringProperties.class)).get(); + assertThat(properties.getLocalScript().getExecutionMode()).isEqualTo(AuthoringProperties.ScriptExecutionMode.DOCKER); + assertThat(properties.getLocalScript().getEnvironmentAllowlist()).containsExactly("AUTHORING_DEMO", "AUTHORING_OTHER"); + assertThat(new AuthoringEnvironmentConfig().scriptEnvironmentPolicy(properties).authorize(List.of("AUTHORING_DEMO"))) + .containsExactly("AUTHORING_DEMO"); + } + @Test void explicitEmptySettingBindsToDenyAll() { + var source = new MapConfigurationPropertySource(Map.of( + "skillhub.authoring.local-script.environment-allowlist", "")); + var properties = new Binder(source).bind("skillhub.authoring", Bindable.of(AuthoringProperties.class)).get(); + assertThat(properties.getLocalScript().getEnvironmentAllowlist()).isEmpty(); + } +} diff --git a/server/skillhub-app/src/test/java/com/iflytek/skillhub/service/authoring/adapter/LocalScriptEnvironmentAuthorizationTest.java b/server/skillhub-app/src/test/java/com/iflytek/skillhub/service/authoring/adapter/LocalScriptEnvironmentAuthorizationTest.java new file mode 100644 index 000000000..c110460d5 --- /dev/null +++ b/server/skillhub-app/src/test/java/com/iflytek/skillhub/service/authoring/adapter/LocalScriptEnvironmentAuthorizationTest.java @@ -0,0 +1,112 @@ +package com.iflytek.skillhub.service.authoring.adapter; + +import com.iflytek.skillhub.config.AuthoringProperties; +import com.iflytek.skillhub.domain.authoring.runtime.RuntimeExecutionContext; +import com.iflytek.skillhub.domain.authoring.service.ScriptEnvironmentPolicy; +import java.nio.file.Path; +import java.util.ArrayList; +import java.util.List; +import java.util.Map; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; +import static org.assertj.core.api.Assertions.*; + +class LocalScriptEnvironmentAuthorizationTest { + @TempDir Path workspace; + private static final String DEMO = "AUTHORING_DEMO"; + private final AuthoringProperties properties = new AuthoringProperties(); + private RuntimeExecutionContext context(Object selection) { + return new RuntimeExecutionContext(1L, Path.of("dummy-workspace").toAbsolutePath(), + selection == null ? Map.of() : Map.of("envAllowlist", selection), List.of(), null); + } + @Test void onlySelectedDefinedDummyValuesAreResolved() { + properties.getLocalScript().setEnvironmentAllowlist(List.of(DEMO, "AUTHORING_UNDEFINED", "AUTHORING_OTHER")); + var calls = new ArrayList(); + var adapter = adapter(calls); + var result = adapter.scrubEnvironment(context(List.of(DEMO, "AUTHORING_UNDEFINED"))); + assertThat(result).containsEntry(DEMO, "dummy-value").containsEntry("PATH", "/dummy/bin") + .containsEntry("LANG", "C.UTF-8").containsEntry("HOME", context(null).workingDirectory().toString()) + .doesNotContainKeys("AUTHORING_UNDEFINED", "AUTHORING_OTHER"); + assertThat(calls).containsExactly("PATH", DEMO, "AUTHORING_UNDEFINED"); + } + @Test void emptyBindingGetsOnlyTrustedStartupEnvironment() { + var calls = new ArrayList(); + var result = adapter(calls).scrubEnvironment(context(null)); + assertThat(result.keySet()).containsExactly("PATH", "HOME", "LANG"); + assertThat(calls).containsExactly("PATH"); + } + @Test void unauthorizedMixedSelectionIsRejectedBeforeAnyLookup() { + properties.getLocalScript().setEnvironmentAllowlist(List.of(DEMO)); + var calls = new ArrayList(); + var adapter = adapter(calls); + assertThatThrownBy(() -> adapter.scrubEnvironment(context(List.of(DEMO, "AUTHORING_DENIED")))) + .isInstanceOf(IllegalArgumentException.class).hasMessageNotContaining("dummy-value"); + assertThat(calls).isEmpty(); + } + @Test void savedBindingIsRejectedAfterEffectiveRevocation() { + properties.getLocalScript().setEnvironmentAllowlist(List.of(DEMO)); + var calls = new ArrayList(); + var adapter = adapter(calls); + var saved = context(List.of(DEMO)); + assertThat(adapter.scrubEnvironment(saved)).containsKey(DEMO); + properties.getLocalScript().setEnvironmentAllowlist(List.of()); + calls.clear(); + assertThatThrownBy(() -> adapter.scrubEnvironment(saved)).isInstanceOf(IllegalArgumentException.class); + assertThat(calls).isEmpty(); + } + @Test void reservedAliasesNeverReachLookup() { + var calls = new ArrayList(); + var adapter = adapter(calls); + for (String name : List.of("PATH", "Path", "path", "HOME", "LANG", "SYSTEMROOT", "SystemRoot")) { + assertThatThrownBy(() -> adapter.scrubEnvironment(context(List.of(name)))) + .isInstanceOf(IllegalArgumentException.class); + } + assertThat(calls).isEmpty(); + } + @Test void bothBackendModesRejectBeforeAvailabilityProbeOrPlanning() { + for (var mode : AuthoringProperties.ScriptExecutionMode.values()) { + properties.getLocalScript().setExecutionMode(mode); + String backend = mode == AuthoringProperties.ScriptExecutionMode.DOCKER ? "docker" : "inline"; + ScriptCommandBuilder builder = new ScriptCommandBuilder() { + public String backend() { return backend; } + public boolean available() { throw new AssertionError("must not probe backend"); } + public ScriptExecutionPlan plan(Path w, String i, Path s, List a, Map e, String t) { + throw new AssertionError("must not plan backend"); + } + }; + var adapter = new LocalScriptRuntimeAdapter(properties, List.of(builder), + new ScriptEnvironmentPolicy(() -> properties.getLocalScript().getEnvironmentAllowlist()), + name -> { throw new AssertionError("must not look up environment"); }); + assertThatThrownBy(() -> adapter.execute(context(List.of(DEMO)), null, null)) + .isInstanceOf(IllegalArgumentException.class); + } + } + @Test void inlinePlanReceivesOnlyAuthorizedMap() { + properties.getLocalScript().setEnvironmentAllowlist(List.of(DEMO)); + var ctx = context(List.of(DEMO)); + var environment = adapter(new ArrayList<>()).scrubEnvironment(ctx); + var plan = new InlineScriptCommandBuilder().plan(ctx.workingDirectory(), "sh", + ctx.workingDirectory().resolve("check.sh"), List.of(), environment, "dummy"); + assertThat(plan.environment()).containsEntry(DEMO, "dummy-value").hasSize(4); + assertThat(plan.command()).hasSize(2); + } + @Test void dockerPlanForwardsOnlyAuthorizedMapWithoutStartingContainer() { + properties.getLocalScript().setEnvironmentAllowlist(List.of(DEMO)); + var ctx = context(List.of(DEMO)); + var environment = adapter(new ArrayList<>()).scrubEnvironment(ctx); + var plan = new DockerScriptCommandBuilder(properties).plan(workspace, "sh", + workspace.resolve("check.sh"), List.of(), environment, "dummy"); + assertThat(plan.environment()).isNull(); // CLI control environment remains inherited. + assertThat(plan.command()).contains(DEMO + "=dummy-value", "HOME=/workspace", "--read-only"); + assertThat(plan.command()).doesNotContain("AUTHORING_UNDEFINED=", "AUTHORING_DENIED="); + assertThat(plan.command().stream().filter("-e"::equals).count()).isEqualTo(4); + } + private LocalScriptRuntimeAdapter adapter(List calls) { + return new LocalScriptRuntimeAdapter(properties, List.of(), + new ScriptEnvironmentPolicy(() -> properties.getLocalScript().getEnvironmentAllowlist()), + name -> { + calls.add(name); + return switch (name) { case "PATH" -> "/dummy/bin"; case DEMO -> "dummy-value"; default -> null; }; + }); + } +} diff --git a/server/skillhub-app/src/test/java/com/iflytek/skillhub/service/authoring/adapter/LocalScriptEnvironmentExecutionTest.java b/server/skillhub-app/src/test/java/com/iflytek/skillhub/service/authoring/adapter/LocalScriptEnvironmentExecutionTest.java new file mode 100644 index 000000000..4ee8d882d --- /dev/null +++ b/server/skillhub-app/src/test/java/com/iflytek/skillhub/service/authoring/adapter/LocalScriptEnvironmentExecutionTest.java @@ -0,0 +1,108 @@ +package com.iflytek.skillhub.service.authoring.adapter; + +import com.iflytek.skillhub.config.AuthoringProperties; +import com.iflytek.skillhub.domain.authoring.runtime.RuntimeEventSink; +import com.iflytek.skillhub.domain.authoring.runtime.RuntimeExecutionContext; +import com.iflytek.skillhub.domain.authoring.service.ScriptEnvironmentPolicy; +import com.iflytek.skillhub.domain.authoring.spec.TaskType; +import com.iflytek.skillhub.domain.authoring.spec.ValidationTaskSpec; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import java.util.Map; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.condition.EnabledOnOs; +import org.junit.jupiter.api.condition.OS; +import org.junit.jupiter.api.io.TempDir; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +@EnabledOnOs({OS.LINUX, OS.MAC}) +class LocalScriptEnvironmentExecutionTest { + + @TempDir + Path workspace; + + @Test + void realScriptReceivesSelectedSyntheticValueAndStreamsItsOutput() throws Exception { + Files.writeString(workspace.resolve("check.sh"), """ + printf '%s|%s' "$AUTHORING_DEMO" "${AUTHORING_UNSELECTED-unset}" > executed.txt + printf '%s\\n' "$AUTHORING_DEMO" + """); + var lookups = new ArrayList(); + var sink = new RecordingSink(); + + var result = adapter(lookups).execute(context(List.of("AUTHORING_DEMO")), task(), sink); + + assertThat(result.exitCode()).isZero(); + assertThat(result.stdout()).isEqualTo("synthetic-review-value"); + assertThat(result.stderr()).isEmpty(); + assertThat(Files.readString(workspace.resolve("executed.txt"))) + .isEqualTo("synthetic-review-value|unset"); + assertThat(sink.logs).containsExactly("synthetic-review-value"); + assertThat(lookups).containsExactly("PATH", "AUTHORING_DEMO"); + } + + @Test + void deniedSelectionDoesNotStartTheRealScriptOrLookUpValues() throws Exception { + Files.writeString(workspace.resolve("check.sh"), "printf 'started' > executed.txt\n"); + var lookups = new ArrayList(); + var sink = new RecordingSink(); + var adapter = adapter(lookups); + + assertThatThrownBy(() -> adapter.execute( + context(List.of("AUTHORING_DEMO", "AUTHORING_DENIED")), task(), sink)) + .isInstanceOf(IllegalArgumentException.class); + + assertThat(workspace.resolve("executed.txt")).doesNotExist(); + assertThat(lookups).isEmpty(); + assertThat(sink.logs).isEmpty(); + assertThat(sink.toolCalls).isZero(); + } + + private LocalScriptRuntimeAdapter adapter(List lookups) { + var properties = new AuthoringProperties(); + properties.getLocalScript().setExecutionMode(AuthoringProperties.ScriptExecutionMode.INLINE); + properties.getLocalScript().setEnvironmentAllowlist(List.of("AUTHORING_DEMO", "AUTHORING_UNSELECTED")); + return new LocalScriptRuntimeAdapter(properties, List.of(new InlineScriptCommandBuilder()), + new ScriptEnvironmentPolicy(() -> properties.getLocalScript().getEnvironmentAllowlist()), + name -> { + lookups.add(name); + return "PATH".equals(name) ? "/usr/bin:/bin" : "synthetic-review-value"; + }); + } + + private RuntimeExecutionContext context(List selection) { + return new RuntimeExecutionContext(1L, workspace, + Map.of("interpreter", "sh", "envAllowlist", selection), List.of(), null); + } + + private ValidationTaskSpec task() { + return new ValidationTaskSpec("environment-check", "", TaskType.SCRIPT, + "check.sh", List.of(), null, 5_000, List.of()); + } + + private static final class RecordingSink implements RuntimeEventSink { + private final List logs = Collections.synchronizedList(new ArrayList<>()); + private int toolCalls; + + @Override + public void agentMessage(String taskName, String content) { } + + @Override + public void toolCall(String taskName, String tool, String argumentsJson) { + toolCalls++; + } + + @Override + public void toolResult(String taskName, String tool, String summaryJson) { } + + @Override + public void log(String taskName, String stream, String line) { + logs.add(line); + } + } +} diff --git a/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/authoring/service/RuntimeBindingValidator.java b/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/authoring/service/RuntimeBindingValidator.java index bd6cf6da6..e5273dd80 100644 --- a/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/authoring/service/RuntimeBindingValidator.java +++ b/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/authoring/service/RuntimeBindingValidator.java @@ -29,7 +29,10 @@ public class RuntimeBindingValidator { private static final List TRANSPORTS = List.of("http", "sse", "stdio"); private static final List INTERPRETERS = List.of("sh", "bash", "python3", "node"); - public RuntimeBindingValidator() { + private final ScriptEnvironmentPolicy environmentPolicy; + + public RuntimeBindingValidator(ScriptEnvironmentPolicy environmentPolicy) { + this.environmentPolicy = environmentPolicy; } /** @@ -83,11 +86,17 @@ public class RuntimeBindingValidator { Object envAllowlist = config.get("envAllowlist"); if (envAllowlist instanceof List entries) { for (Object entry : entries) { - if (entry == null || !ENV_REF_NAME.matcher(entry.toString()).matches()) { + if (!(entry instanceof String name) || !ENV_REF_NAME.matcher(name).matches()) { findings.add(FindingDraft.error(ValidationLayer.CONFIG, "ENV_ALLOWLIST_INVALID", "Invalid environment variable name: " + entry)); } } + try { + environmentPolicy.authorize(entries); + } catch (IllegalArgumentException exception) { + findings.add(FindingDraft.error(ValidationLayer.CONFIG, "ENV_ALLOWLIST_FORBIDDEN", + "Environment selection is not permitted by deployment policy")); + } } else if (envAllowlist != null) { findings.add(FindingDraft.error(ValidationLayer.CONFIG, "ENV_ALLOWLIST_INVALID", "envAllowlist must be a list of variable names")); diff --git a/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/authoring/service/ScriptEnvironmentPolicy.java b/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/authoring/service/ScriptEnvironmentPolicy.java new file mode 100644 index 000000000..291da481c --- /dev/null +++ b/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/authoring/service/ScriptEnvironmentPolicy.java @@ -0,0 +1,61 @@ +package com.iflytek.skillhub.domain.authoring.service; + +import java.util.LinkedHashSet; +import java.util.List; +import java.util.Locale; +import java.util.Set; +import java.util.function.Supplier; +import java.util.regex.Pattern; + +/** Deployment-owned ceiling; a draft may only select a subset, never grant access. */ +public final class ScriptEnvironmentPolicy { + + private static final Pattern NAME = Pattern.compile("^[A-Z][A-Z0-9_]{0,63}$"); + private static final Set RESERVED = Set.of( + "PATH", "HOME", "LANG", "SYSTEMROOT", "WINDIR", "COMSPEC", "PATHEXT", "TEMP", "TMP"); + private final Supplier> deploymentAllowlist; + + public ScriptEnvironmentPolicy(Supplier> deploymentAllowlist) { + this.deploymentAllowlist = java.util.Objects.requireNonNull(deploymentAllowlist); + allowedNames(); + } + + /** Reads the currently effective deployment policy on every call, including execution. */ + public Set authorize(Object requested) { + Set allowed = allowedNames(); + if (requested == null) { + return Set.of(); + } + if (!(requested instanceof List entries)) { + throw new IllegalArgumentException("envAllowlist must be a list of variable names"); + } + Set selected = new LinkedHashSet<>(); + for (Object entry : entries) { + if (!(entry instanceof String name) || !validName(name) || !allowed.contains(name)) { + throw new IllegalArgumentException( + "envAllowlist contains a variable not permitted by deployment policy"); + } + selected.add(name); + } + return java.util.Collections.unmodifiableSet(selected); + } + + private Set allowedNames() { + List configured = deploymentAllowlist.get(); + if (configured == null) { + return Set.of(); + } + Set allowed = new LinkedHashSet<>(); + for (String name : configured) { + if (name == null || !validName(name)) { + throw new IllegalStateException("Invalid local-script deployment environment allowlist"); + } + allowed.add(name); + } + return Set.copyOf(allowed); + } + + private static boolean validName(String name) { + return NAME.matcher(name).matches() && !RESERVED.contains(name.toUpperCase(Locale.ROOT)); + } +} diff --git a/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/authoring/service/RuntimeBindingEnvironmentAuthorizationTest.java b/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/authoring/service/RuntimeBindingEnvironmentAuthorizationTest.java new file mode 100644 index 000000000..4f8243203 --- /dev/null +++ b/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/authoring/service/RuntimeBindingEnvironmentAuthorizationTest.java @@ -0,0 +1,55 @@ +package com.iflytek.skillhub.domain.authoring.service; + +import com.iflytek.skillhub.domain.authoring.*; +import com.iflytek.skillhub.domain.shared.exception.*; +import java.util.List; +import java.util.Map; +import java.util.Optional; +import java.util.Set; +import org.junit.jupiter.api.Test; +import static org.assertj.core.api.Assertions.*; +import static org.mockito.Mockito.*; +import static org.mockito.ArgumentMatchers.*; + +class RuntimeBindingEnvironmentAuthorizationTest { + @Test void ordinaryDraftOwnerCanSaveOnlyDeploymentApprovedSelection() { + var bindings = mock(RuntimeBindingRepository.class); + var drafts = mock(SkillDraftRepository.class); + var policy = new ScriptEnvironmentPolicy(() -> List.of("AUTHORING_DEMO")); + var service = new RuntimeBindingService(bindings, drafts, new RuntimeBindingValidator(policy)); + when(drafts.findById(7L)).thenReturn(Optional.of(new SkillDraft(1L, "ordinary-member", "test", "test", "digest"))); + when(bindings.findByDraftId(7L)).thenReturn(Optional.empty()); + when(bindings.save(any())).thenAnswer(call -> call.getArgument(0)); + var result = service.saveBinding(7L, "ordinary-member", "local-script", + Map.of("envAllowlist", List.of("AUTHORING_DEMO")), null, null, Set.of()); + assertThat(result.getConfig()).containsKey("envAllowlist"); + verify(bindings).save(any()); + clearInvocations(bindings); + assertThatThrownBy(() -> service.saveBinding(7L, "ordinary-member", "local-script", + Map.of("envAllowlist", List.of("AUTHORING_OTHER")), null, null, Set.of())) + .isInstanceOf(DomainBadRequestException.class); + verify(bindings, never()).save(any()); + } + @Test void superAdminRoleDoesNotExpandDeploymentGrant() { + var bindings = mock(RuntimeBindingRepository.class); + var drafts = mock(SkillDraftRepository.class); + when(drafts.findById(7L)).thenReturn(Optional.of(new SkillDraft(1L, "ordinary-member", "test", "test", "digest"))); + var service = new RuntimeBindingService(bindings, drafts, + new RuntimeBindingValidator(new ScriptEnvironmentPolicy(List::of))); + assertThatThrownBy(() -> service.saveBinding(7L, "admin", "local-script", + Map.of("envAllowlist", List.of("AUTHORING_DEMO")), null, null, Set.of("SUPER_ADMIN"))) + .isInstanceOf(DomainBadRequestException.class); + verify(bindings, never()).save(any()); + } + @Test void nonOwnerCannotUseAnOtherwiseAuthorizedSelection() { + var bindings = mock(RuntimeBindingRepository.class); + var drafts = mock(SkillDraftRepository.class); + when(drafts.findById(7L)).thenReturn(Optional.of(new SkillDraft(1L, "owner", "test", "test", "digest"))); + var service = new RuntimeBindingService(bindings, drafts, + new RuntimeBindingValidator(new ScriptEnvironmentPolicy(() -> List.of("AUTHORING_DEMO")))); + assertThatThrownBy(() -> service.saveBinding(7L, "other-member", "local-script", + Map.of("envAllowlist", List.of("AUTHORING_DEMO")), null, null, Set.of())) + .isInstanceOf(DomainForbiddenException.class); + verify(bindings, never()).save(any()); + } +} diff --git a/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/authoring/service/ScriptEnvironmentPolicyTest.java b/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/authoring/service/ScriptEnvironmentPolicyTest.java new file mode 100644 index 000000000..433346b3a --- /dev/null +++ b/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/authoring/service/ScriptEnvironmentPolicyTest.java @@ -0,0 +1,70 @@ +package com.iflytek.skillhub.domain.authoring.service; + +import java.util.Arrays; +import java.util.List; +import java.util.Map; +import java.util.Set; +import java.util.concurrent.atomic.AtomicReference; +import org.junit.jupiter.api.Test; +import static org.assertj.core.api.Assertions.*; + +class ScriptEnvironmentPolicyTest { + @Test void emptyDeploymentDeniesRequestedVariable() { + var policy = new ScriptEnvironmentPolicy(List::of); + assertThat(policy.authorize(null)).isEmpty(); + assertThat(policy.authorize(List.of())).isEmpty(); + assertThatThrownBy(() -> policy.authorize(List.of("AUTHORING_DEMO"))) + .isInstanceOf(IllegalArgumentException.class); + } + @Test void acceptsOnlySelectedSubsetAndDeduplicates() { + var policy = new ScriptEnvironmentPolicy(() -> List.of("AUTHORING_DEMO", "AUTHORING_OTHER")); + assertThat(policy.authorize(List.of("AUTHORING_DEMO", "AUTHORING_DEMO"))) + .containsExactly("AUTHORING_DEMO"); + assertThat(policy.authorize(List.of())).isEmpty(); + } + @Test void mixedSelectionIsRejectedAsAWhole() { + var policy = new ScriptEnvironmentPolicy(() -> List.of("AUTHORING_DEMO")); + assertThatThrownBy(() -> policy.authorize(List.of("AUTHORING_DEMO", "AUTHORING_OTHER"))) + .isInstanceOf(IllegalArgumentException.class); + } + @Test void malformedAndNonStringNamesCannotCoerceOrMatch() { + var policy = new ScriptEnvironmentPolicy(() -> List.of("AUTHORING_DEMO")); + for (Object value : List.of(42, Map.of("name", "AUTHORING_DEMO"), "authoring_demo", " AUTHORING_DEMO", "AUTHORING_DEMO ", "A-B", "A".repeat(65))) { + assertThatThrownBy(() -> policy.authorize(List.of(value))).isInstanceOf(IllegalArgumentException.class); + } + assertThatThrownBy(() -> policy.authorize(Arrays.asList((Object) null))).isInstanceOf(IllegalArgumentException.class); + assertThatThrownBy(() -> policy.authorize("AUTHORING_DEMO")).isInstanceOf(IllegalArgumentException.class); + } + @Test void reservedNamesCannotBeAuthorizedByDeployment() { + for (String reserved : List.of("PATH", "HOME", "LANG", "SYSTEMROOT", "WINDIR", "COMSPEC", "PATHEXT", "TEMP", "TMP", "Path", "path")) { + assertThatThrownBy(() -> new ScriptEnvironmentPolicy(() -> List.of(reserved))) + .isInstanceOf(IllegalStateException.class); + } + } + @Test void malformedDeploymentFailsClosed() { + assertThatThrownBy(() -> new ScriptEnvironmentPolicy(() -> List.of("bad-name"))) + .isInstanceOf(IllegalStateException.class); + assertThatThrownBy(() -> new ScriptEnvironmentPolicy(() -> Arrays.asList((String) null))) + .isInstanceOf(IllegalStateException.class); + assertThat(new ScriptEnvironmentPolicy(() -> null).authorize(null)).isEmpty(); + } + @Test void effectiveRevocationAppliesToPreviouslyValidSelection() { + var current = new AtomicReference<>(List.of("AUTHORING_DEMO")); + var policy = new ScriptEnvironmentPolicy(current::get); + var selection = List.of("AUTHORING_DEMO"); + assertThat(policy.authorize(selection)).containsExactly("AUTHORING_DEMO"); + current.set(List.of()); + assertThatThrownBy(() -> policy.authorize(selection)).isInstanceOf(IllegalArgumentException.class); + current.set(List.of("PATH")); + assertThatThrownBy(() -> policy.authorize(null)).isInstanceOf(IllegalStateException.class); + } + @Test void configValidationUsesCurrentPolicyWithoutDisclosingValues() { + var current = new AtomicReference<>(List.of("AUTHORING_DEMO")); + var validator = new RuntimeBindingValidator(new ScriptEnvironmentPolicy(current::get)); + var config = Map.of("interpreter", "sh", "envAllowlist", List.of("AUTHORING_DEMO")); + assertThat(validator.validate("local-script", config, null, null)).isEmpty(); + current.set(List.of()); + assertThat(validator.validate("local-script", config, null, null)) + .extracting(finding -> finding.ruleCode()).containsExactly("ENV_ALLOWLIST_FORBIDDEN"); + } +}