fix(service-settings): gate OS_* env overrides on the manifest's declared options table (#5204) - #5711
Merged
Merged
Conversation
…ared options table (#5204) A manifest's `options` table has been enforced on the write path since #5131, but `SettingsService.get()` produced an effective value by a second route that never consulted it: an `OS_*` override was reshaped by the default's type (`coerceEnvValue`) and returned straight from the top of the cascade with `locked: true`. So the providers #5094/#5133 retired from `mail.provider` could walk back in through the one door with no gate on it, and a plain typo (`OS_BRANDING_THEME_MODE=drak`) was served to every consumer as a normal value with normal-looking provenance. An override whose value the table does not declare is now ignored rather than repaired: it contributes no value and no `cascadeChain` entry, so the read API reports the layer actually in force instead of claiming `source: 'env'` for a value nothing is using. The rejection is logged once at `error` — naming the variable, the rejected value, the legal set and the consequence — and the same audit runs at `registerManifest` so a misconfigured deployment learns at boot. Registration reports but never refuses: option tables move, and a pin that was legal the day it was written must not turn an upgrade into a crash-on-start. Three sites used to read the env override in three different ways; they now share one judgment (`effectiveEnvOverride`) and one comparison (`firstRejectedOption`, also used by the save path). That is what keeps `locked` coherent: `setMany` used to pin a key on the mere PRESENCE of the variable, so an ignored value would have left the key configurable by nothing at all — env discarded, UI refused with SETTINGS_LOCKED, and `get()` advertising `locked: false` to a settings page whose save would then fail. Keys with no declared option table are untouched.
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Contributor
📓 Docs Drift CheckThis PR changes 1 package(s): 6 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:
|
Contributor
Author
|
范围外发现已按 Prime Directive #10 立项(均未指派,交 PM triage),补上编号:
两条都不是本 PR 的前置依赖:本 PR 独立可评审可合并。 Generated by Claude Code |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #5204
前提重验(先对 origin/main 逐条核实,再动手)
Issue 的两条事实与落点都成立,且比 issue 描述的更严重一层:
写入路径确实过表:
packages/services/service-settings/src/settings-service.ts的validatePatch里OPTION_BEARING_TYPES+declaredOptionValues校验在位,invalid_option由settings-service.test.ts:431起的一组用例钉住。env 路径确实不过表:
get()的 env 分支只做coerceEnvValue(envRaw, def),按默认值的类型做形状转换后直接return { source: 'env', locked: true },全程不读spec.options。新发现:同一个 env 覆盖有第三个读取点,
setMany的 pre-flight:它只看环境变量是否存在,不看值是否合法。这条对本单是决定性的:如果只按裁决改读取侧,非法 env 值会变成「读侧
locked: false、写侧照旧抛SETTINGS_LOCKED」——设置页把字段渲染成可编辑,保存却失败;更糟的是该键谁都配不了(env 值被忽略、UI 被拒),只能改环境变量才能解套,比 env 来源的 settings 值绕过 manifest 的 options 表校验 —— #5094 在写入 API 上堵住的洞,在 OS_* 覆盖这一侧原样敞开 #5204 原本的洞更差。所以三个读取点必须共用一个判断,详见下面「locked 语义」。auth.membership_policy(#5152)的那道bindAuthSettings防线保留未动。归并建议见文末。实现
按维护者裁决「方案 1 为主 + 注册时响亮 error(不拒绝启动)」:
effectiveEnvOverride(reg, namespace, key):返回「真正生效的 env 覆盖」或 null。get()、setMany的写锁、注册时扫描全部走它。报告动作放在这个函数里而不是各调用点,这样将来第四个调用者不可能读到覆盖却让拒绝无声。firstRejectedOption(allowed, value),env 侧与validatePatch共用。env 来源的 settings 值绕过 manifest 的 options 表校验 —— #5094 在写入 API 上堵住的洞,在 OS_* 覆盖这一侧原样敞开 #5204 的成因正是「两条产出有效值的路径只有一条过表」,再开一份手写比较等于把同样的缺陷推迟。invite_only被读成auto就是代价)。忽略意味着回落 cascade 下一层。auditEnvOverrides):一个永远不会生效的覆盖是部署配置错误,运维应该在启动时和其它启动输出一起看到,而不是等谁哪天打开设置页,也不是永远看不到(进程生命周期内没人读的键否则永远沉默)。只报告,不拒绝启动:option 表会变,plugin-email: SendGrid / Amazon SES 设置项同样后端无实现 —— #5087 的同形缺口 #5094 摘掉sendgrid之前,OS_MAIL_PROVIDER=sendgrid是当天写下就正确的配置,把升级变成崩溃启动恰好惩罚了这条消息想帮的人。select/radio/multiselect」。RegisteredManifest.optionTables里没有该键 = 没有可执行的枚举 = 行为完全不变(text / boolean / number / password 一律照旧)。locked 语义(本单的连带修复)
setMany的写锁改为按「是否真正生效」判断,而不是按环境变量是否存在。生效的覆盖照旧锁定该键(读侧locked: true,写侧抛SETTINGS_LOCKED),这条没变、并新增用例钉住,防止过度推广成「env 永不锁定」;被拒绝的覆盖什么也不锁,读写两侧一致地认为该键可编辑。被忽略的 env 值也不进
cascadeChain:带locked: true的 env 条目会被下面chain.find((e) => e.locked === true)捡走,变成「报告一个它根本没提供的值被自己锁住」,与读取面该给调用方的信息正相反。error 消息
级别取
error而非warn,依 AGENTS.md「Degradation log levels」的判据:降级之后系统从外面看仍然正常(读取面给出一个完全合理的值、挂着一个完全合理的来源),而运维声明的意图根本没生效——#5152 对auth.membership_policy是同样的裁断。按同一节「Say it once, at the first degradation」,去重键为「环境变量 + 值」,所以getNamespace每次页面加载不会刷屏,而一个新的坏值仍会被报告(this.env可能是活的process.env引用)。消息含变量名、被拒的值、合法值集、后果与修复动作。刻意不带结构化 meta:
packages/core/src/logger.ts的redactSensitive按includes匹配key/token/secret/password,顺手写成{ key }/{ envKey }会被脱敏成***REDACTED***,消息看着完整而诊断已被删掉(#5573 的教训)。encryptedspecifier 不回显被拒的值,与写入侧同规则。logger 经
SettingsServicePlugin注入(ctx.logger),未注入时回落console.error——没有 kernel 的场景(单测、control-plane mock、logger 之前的 bootstrap)也必须出声,沉默正是本单要修的病。multiselect 形状:按现状核实的结论
coerceEnvValue里没有任何逗号切分:仅当声明的 default 是数组/对象时才JSON.parse。所以"a,b"因此会被判非法——这不是回归:今天把字符串"a,b"交给期待数组的消费方本来就是坏的,现在改为响亮拒绝。任一成员非法则整个 env 值作废(不做部分接受),否则会造出没人配置过的组合。仓库现无
multiselect/radio生产 manifest,这两类由手搭 manifest 的用例覆盖——与OPTION_BEARING_TYPES注释里既有的理由一致:否则第一个写出 multiselect 的 manifest 会静悄悄重开这个洞。一处需要维护者知道的曝光面(未在本 PR 处理)
localization.timezone声明为select,但表里只有 17 个策展时区,而它自己的 description 写的是「IANA zone」;localization.currency同形(9 个 ISO 4217 码,description 写「ISO 4217 code」)。这类「策展便利列表」自 #5131 起在写入侧已经是穷尽式执行边界,本 PR 让 env 侧与之一致——于是一个用OS_LOCALIZATION_TIMEZONE=Europe/Zurich固定未列出合法 IANA 时区的部署,现在会被忽略并回落(带响亮 error)。仓库内没有任何地方实际设置这些变量(只有
packages/rest/src/rest-api-plugin.ts注释提及),所以不构成在库回归。张力本身是 #5131 引入、由本 PR 传播到 env 的,已另开 issue 记录,方向是在生产者侧修(补全表,或换掉 specifier 类型),而不是在消费侧放宽——因此本 PR 不为它开豁免口子。测试
packages/services/service-settings:243 passed(15 文件),其中新增 17 个用例,覆盖:非法值忽略 + error + 回落默认值 / 回落下一层(tenant 行)、合法值照常生效并锁定、无 options 键完全不受影响、注册时一次性 error、注册只报告不拒绝、只报一次而非每次读、注册后出现的新坏值仍报告、读取面(getNamespace)如实报告生效层且 chain 无 env 条目、无 logger 时回落 console.error、multiselect 整体作废与全合法通过、数值 option 经 env 往返仍匹配、encrypted specifier 不回显值,以及 locked 语义两个方向的钉子。消费半径全绿:
objectql1970、plugin-auth786(含 #5152 membership-policy 全套)、plugin-email297、rest751、cli825、verify17。plugin-auth那套之所以不受影响:它注入的是 mock 读取面(settingsStore.values直接给{ value: 'invite_only', source: 'env' }),不走真实get(),所以 #5152 这道第二防线仍被真实测着。反向验证(先预测方向,再跑)
预测:把删掉的肢体接回去(env 值无条件返回、
setMany按存在性锁定、去掉注册扫描),新钉子应转红,且是最普通的方向——本单堵的是一个根本不存在的检查,不是??别名链,所以规范非法的值此前是被接受的,不存在 #5009 那类反转。同时,描述改动前既有行为的用例应保持绿。实测一致:12 红 / 231 绿。转红的正是 12 个新行为钉子;保持绿的 5 个是「合法覆盖照旧胜出」「生效覆盖照旧锁定写入」「无 options 键不受影响」「全合法 multiselect 通过」「数值 option 往返」。
门检查
check:nul-bytesOK(5610 文件);改动文件grep -naP扫控制字节无命中;check:durability-degradation-log-levelOK(24 个 seam 全响亮);check:type-check-coverageOK——本包带 13 错的 DEBT 台账未被移动(实测仍为 13,且无一条落在改动文件里);check:startup-registry-verdict、check:init-service-contract、check:error-code-casing、check:wildcard-fallthrough均 OK;改动文件 eslint 干净。changeset 为 patch。关于 #5152 是否归并的建议(按代码证据,不在本 PR 动手)
建议保留,理由是它守的不是同一件事:
bindAuthSettings的检查跑在isExplicit('membership_policy')之后、patch.membershipPolicy之前,守的是「进入 AuthManager 的值必须是MembershipPolicy」——数据来源不止 env,还有存储行(source: 'global'/'tenant'),而存储行可能带着 option 表收紧之前写下的历史值(validatePatch的 TOUCH 闸门刻意允许这种漂移继续存在,以免把工作区锁死在自己的设置页外)。本 PR 关掉的是 env 这扇门,关不掉那条历史漂移路径。归并会把一个仍然可达的输入暴露给AuthManager。Generated by Claude Code