Skip to content

fix(terminal): 校验 PTY 窗口尺寸 - #1318

Open
wxhking wants to merge 2 commits into
TencentCloud:developfrom
wxhking:fix/validate-terminal-resize
Open

wxhking wants to merge 2 commits into
TencentCloud:developfrom
wxhking:fix/validate-terminal-resize

Conversation

@wxhking

@wxhking wxhking commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

变更说明

终端 WebSocket 的初始窗口尺寸和运行时 resize 消息此前没有范围校验。异常客户端可以发送非正数或过大的尺寸,导致 PTY 调整失败,或把无效尺寸写入会话状态。

本 PR 做了以下处理:

  • 为握手参数增加列数 20–500、行数 5–200 的校验。
  • 对运行时 resize 消息使用相同范围校验,越界消息直接忽略。
  • 增加越界尺寸不会修改会话状态的单元测试。
  • 在 CHANGELOG.md 的 Unreleased 修复项中记录此变更。

风险与范围

仅修改终端 WebSocket 路由、对应单元测试和变更日志,不涉及 UI、数据库或其他功能模块。

验证

  • Ruff 检查通过。
  • Ruff 格式检查通过。
  • Python 字节码编译检查通过。
  • git diff --check 通过。
  • GitHub Actions 的完整 pytest 由 CI 执行。

Fixes #1317

@wxhking
wxhking force-pushed the fix/validate-terminal-resize branch from a2a0c64 to da1c7ac Compare September 29, 2026 07:42
@wxhking
wxhking changed the base branch from main to develop September 29, 2026 07:43
@jubaoliang

Copy link
Copy Markdown
Collaborator

分诊:PTY 尺寸校验本身正确,CI 也绿,但现在不能合。

CHANGELOG 写进了已发布的 ## [1.0.2b4],还会动 Unreleased 里已发布的条目。请把这一条中文 bullet 挪到当前 [Unreleased] → 修复,去掉无关已发布长文后再提合。

@sxh313

sxh313 commented Oct 1, 2026

Copy link
Copy Markdown
Contributor

One row the clause at terminal.py:723 still misses — found while adding the same guard to the
adb-shell route in #1476, so it is not a critique of the diff's shape, which is what I mirrored.

json.loads accepts the non-standard Infinity literal, and int(float("inf")) raises
OverflowError, which except (TypeError, ValueError) does not catch. Of the four classes that can
arrive in a resize frame, three are already covered — "abc" → ValueError, [1] → TypeError,
NaN → ValueError (NaN is truthy, so msg.get("cols") or 80 keeps it instead of falling back) —
and Infinity is the one that falls out.

On this route it is quieter than on the mobile one, because the exception never reaches the handler:
reader() dies, asyncio.wait(..., return_when=asyncio.FIRST_COMPLETED) (:734) returns on that
task, writer_task is cancelled as pending, and the exception itself is swallowed by
with contextlib.suppress(Exception): t.result() at :743. So there is no
logger.exception("terminal session failed") line at all — the handler just falls into its finally
and detaches the session, so the user loses the terminal with nothing in the log. Read from the shipped
lines at a02e3e18, not executed: this route is @posix_only in test and needs a real PTY, which this
box cannot provide, so I have no capture of it.

Adding OverflowError to the clause is a one-word change and touches nothing else in the diff. If you
would rather keep the scope as it is, that is fine too — #1476 documents the mobile route's version.

For contrast, the rows that need the range check rather than the except: cols=70000 and cols=-1
do not escape this route today, because _set_winsize at :110-115 catches Exception ("Errors are
non-fatal"), so the struct.error: 'H' format requires 0 <= number <= 65535 from
struct.pack("HHHH", …) is logged at debug level and the size simply is not applied. What does get
applied is a frame like {"cols":1,"rows":9999} — representable in an unsigned short, so it reaches
TIOCSWINSZ and resizes the PTY to a geometry no terminal can render. That is what your bounds check
fixes. The mobile route has neither protection, which is why #1475 exists.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] 终端 WebSocket 未校验 PTY 窗口尺寸

3 participants