fix(http): reject non-standard JSON numbers - #4088
Conversation
huangruiteng
left a comment
There was a problem hiding this comment.
复审结论绑定 exact head d2b46f8768832700c51e9581b60fc982170f8a3f:REQUEST_CHANGES。
动机
这个问题是真实的:Python 的默认 json.loads 会接受 RFC 8259 不允许的 NaN、Infinity、-Infinity,而 Status/Chat 两个 loopback POST 边界此前都会让这些值先进入端点逻辑。把策略收敛到两个共享 request reader 是正确方向,也比逐端点补丁更合适。
改动思路
当前实现通过 parse_constant 拒绝三个非标准常量,并复用现有的 status_server -> chat_server 工具依赖关系。失败继续由已有 ValueError -> HTTP 400 边界处理,没有改变各端点的 preview/write 权威。
但 parse_constant 只检查三种 token 拼写,并没有实现“解码后的 JSON 数字必须有限”这个语义不变量。最强反例是标准语法 1e309:Python 会直接解码成 float('inf'),不会调用该 callback。
具体改动
我做了以下复核:
- 仓库范围核对
BaseHTTPRequestHandler、rfile.read、Content-Length和 JSON request reader;两个实际解码的 POST reader 都在本 PR 内,provider gateway 只是透传 bytes。 - 复现作者矩阵:相关 152 个测试全部通过;四个改动文件 Ruff 通过,
git diff --check通过;GitHub 17 个适用检查成功,2 个发布检查按设计 skip。 - 独立验证
NaN、Infinity、-Infinity确实被新 callback 拒绝,普通有限数字保持不变。 - 反向验证
1e309:它被解码为inf;随后走真实ChatActionService.preview -> ChatActionStore路径时,_safe_json_value接受该 float,store 实际写出了Infinity,而json.dumps(..., allow_nan=False)对同一 proposal 报Out of range float values are not JSON compliant。
[P1] 仍可把非有限值写入 store / 返回非标准 JSON
触发:在合法 Chat action request 的 context 中传 1e309。
路径:ChatRequestHandler._read_json → json.loads(parse_constant=...) → ChatActionService.preview → ChatActionStore._safe_json_value → store / _send_json。
影响:一个语法合法的 JSON 请求仍能产生并持久化非标准 Infinity,也会让严格 JSON consumer 无法读取响应。这正是 PR 描述里要避免的 persistence/echo 风险,所以不是推测性边角。
最小修复:两个 reader 在 decode 后共享一次递归 finite-number 校验,任何嵌套 object/list 中的非有限 float 都在返回 payload 前 400;不需要扩成全仓 serializer 重构。回归请覆盖 NaN、Infinity、-Infinity、1e309,并保留有限 exponent/嵌套数字的正例;至少一条 Chat action context 用例应证明不能再存储或发出 Infinity。
对主干的风险
当前分支落后 main,但 main 在相关文件只新增了 Status readiness 逻辑,merge-tree 未发现文本冲突。主要风险不是 rebase,而是表面上已声明“strict JSON”却仍允许非有限持久化,后续严格 consumer 会以难诊断方式失败。
我的整体评价
方向、边界归属和改动规模都合适;阻塞点是机制只覆盖 token spelling,未覆盖它要保护的数值语义。补齐有限值不变量及真实 store/output 回归后,这个 PR 可以进入批准路径。未来向 pass:应继续放在同一个共享 reader/validator 中,不需要引入新的 JSON 框架。
English verdict: REQUEST_CHANGES — the shared ingress boundary is correct, but parse_constant misses standards-valid exponent overflow such as 1e309, which becomes inf and can still be persisted/emitted as non-standard Infinity. Add a shared recursive finite-number check and real HTTP/store regression coverage.
d2b46f8 to
c8be00a
Compare
|
已按本次 REQUEST_CHANGES 修复并 rebase 到最新 实际改动:
验证:
Future-facing pass:未扩成 serializer 或全仓 JSON 框架;共享 ingress parser 已是这两个 loopback POST reader 的最小共同权威边界。Runtime/core 变更不自合并,等待独立复审。 |
|
CI update for exact head |
c8be00a to
5bdd34f
Compare
|
@huangruiteng 已将已批准的 finite-number 修复同步最新 main,当前 head |
5bdd34f to
37ee70f
Compare
huangruiteng
left a comment
There was a problem hiding this comment.
动机
本次复审绑定 exact head 37ee70fcc52c61ed15be4540e5566ae19c989e14,比较基线 b8837d84790c568772c26a956316f5307851cb0e。PR 要修复 Python json.loads 默认接受 NaN、Infinity 与 -Infinity 的问题,避免 LoopX 的 loopback HTTP API 接收并可能回写非标准 JSON。上一轮指出只用 parse_constant 不足:1e309 不走 constant hook,却会解码为正无穷;如果进入 action store 或 response,仍会破坏严格 JSON 契约。当前 head 已补齐这一语义缺口,并把 Status 与 Chat 的真实 POST reader 收敛到同一解析边界。
改动思路
入口仍保留各 server 原有的 body length、空 body 与上限检查,读取 bytes 后统一调用 parse_strict_json_object。解析层先用 parse_constant 拒绝显式非标准 token,再递归遍历 dict/list 中所有 float,拒绝 math.isfinite 为 false 的值,因此同时覆盖显式 Infinity 和指数溢出。只有 object 顶层合法,保持原有 API shape。失败由现有 request handler 转成 400,不创建 Chat action,也不污染 store。
仓库搜索确认 Status 与 Chat 是实际 loopback JSON POST 解码入口;其 mixin/route 都复用各自 reader。provider gateway 只代理 raw bytes,不拥有 JSON 解码语义,因此不应被强行纳入。这个改动扩展了已有 Status parser 作为共享 owner,没有建立平行规则。
具体改动
完整 diff 为 4 个文件、+130/-10:两处 production reader 与两组 server 测试。生产逻辑只有一个共享严格解析器与 Chat 接线,其余增量用于覆盖非法值、合法有限指数和真实副作用边界。
关键代码讲解
loopx/status_server.py:109的_reject_non_standard_json_constant拒绝NaN/Infinity/-Infinity,保留清晰、域中性的严格 JSON 诊断。loopx/status_server.py:115的_require_finite_json_numbers递归检查 object/array 内的 float,补上1e309被标准 number grammar 解析为inf的缺口;合法1e308与-1e-308保持接受。loopx/status_server.py:126的parse_strict_json_object组合 decoder hook、递归 finite 检查和顶层 object 要求,成为唯一规则 owner。loopx/status_server.py:249与loopx/chat_server.py:489各自保留 transport 限制,随后调用共享 parser;这使两个公共 loopback POST surface 的语义一致。tests/test_chat_server_cors.py:144及后续真实 preview route 测试覆盖1e309返回 400、response 不含Infinity、action store 仍为空,证明不是 parser-only mock。
对主干的风险
最强负向场景是嵌套 payload 的 1e309:当前代码在解析后递归检查阶段拒绝,由既有 handler 返回 400,持久化和 action receipt 都没有发生。正向场景 1e308、小负指数和嵌套结构保持成功。没有新状态、CLI、权限、默认开关或 actor authority;规则是明确的 finite-number invariant,不依赖字符串黑名单。
验证结果:受影响的 Chat/Status 测试族 171 passed;当前主干 merge-tree 上聚焦回归 33 passed;Ruff 与 git diff --check 通过;merge-tree 可生成。远端失败来自旧基线 scheduler host-facts Base64 被 credential heuristic 误判,与本 PR 四个改动文件及 JSON reader 路径无关,当前主干已包含对应修复。PR 仍落后主干,合并前应 rebase 并让托管检查在新基线重跑;这不构成当前 exact-head 代码 finding。
我的整体评价
上一轮的语义反例已经用最小且完整的方式修复:不是继续枚举 token,而是在共享 decoder 后验证 JSON 数值域,并通过真实 action store 负向路径证明无副作用。代码规模与风险成比例,复用了最近的 owner,未来向无需新增 parser framework。除 rebase 后重跑远端检查外,没有阻塞 finding;批准 exact head,但本轮不合并。
English verdict: APPROVE exact head 37ee70fcc52c61ed15be4540e5566ae19c989e14. The prior 1e309 gap is closed by a recursive finite-number invariant shared by the real Status and Chat readers, with a real preview/action-store regression proving HTTP 400 and no persistence. 171 affected tests, 33 current-main merge-tree tests, Ruff and diff checks pass. Hosted failures are unrelated old-base scheduler heuristic failures; rebase and rerun checks before merge. No merge performed.
Python's JSON decoder accepts non-standard constants and can overflow valid exponent syntax to infinity. Share one recursive finite-number guard across the Status and Chat request readers. Signed-off-by: duanjialing.777 <duanjialing.777@bytedance.com>
37ee70f to
012fd40
Compare
Summary
NaN,Infinity, and-Infinityat the shared loopback HTTP request boundaryRoot cause
Python
json.loads()accepts non-standard numeric constants by default. Both local HTTP servers used that permissive default, so payloads that are invalid under RFC 8259 could enter control-plane handlers and potentially reach persistence or be echoed as invalid JSON.Validation
host alias is required/unknown reward field(s))152 passedacrosstests/test_chat_*.py,tests/test_status_server_*.py, andtests/test_feedback_goal_id_validation.pygit diff --check: passedloopx canary premerge --from-git-diff: passed (4/4 selected canaries, no manual holds)Scope
The sibling sweep found exactly two inbound loopback HTTP JSON readers; both now share one guard. No protocol fields, persistence schema, permissions, or feature defaults changed.
Future-facing pass
No additional abstraction is warranted:
status_serveralready owns the HTTP origin helpers reused by Chat, so the strict-number callback lives at that existing shared boundary.