fix(security): keep channel-metadata and install-policy truncation surrogate-safe#102266
Merged
Conversation
Contributor
|
Codex review: needs maintainer review before merge. Reviewed July 9, 2026, 1:28 AM ET / 05:28 UTC. Summary PR surface: Source +2, Tests +65. Total +67 across 4 files. Reproducibility: yes. Source inspection shows current main can split a surrogate pair at the channel metadata 397-code-unit cut and the install-policy 1000-code-unit cut. Review metrics: none identified. Stored data model Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Next step before merge
Security Review detailsBest possible solution: Land the helper swap with the exact boundary tests after normal maintainer review and current-head merge/check gating. Do we have a high-confidence way to reproduce the issue? Yes. Source inspection shows current main can split a surrogate pair at the channel metadata 397-code-unit cut and the install-policy 1000-code-unit cut. Is this the best way to solve the issue? Yes. Reusing the existing AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against acac359de64e. Label changesLabel justifications:
Evidence reviewedPR surface: Source +2, Tests +65. Total +67 across 4 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
Review history (3 earlier review cycles)
|
clawsweeper
Bot
added
proof: sufficient
…rrogate-safe channel-metadata.ts and install-policy.ts both had private truncateText functions using String.prototype.slice(0, N) on user-visible text. channel-metadata is explicitly user-controlled untrusted content that gets injected into LLM prompt context -- a dangling surrogate from unsafe truncation would corrupt the prompt with invalid Unicode, affecting token counting and potentially LLM response quality. Replace .slice(0, ...) with truncateUtf16Safe(...) in both files so the truncation point always falls on a complete code-point boundary. Add test coverage for buildUntrustedChannelMetadata with emoji at both the entry-level (400-char) and outer (800-char) truncation boundaries.
zw-xysk
force-pushed
the
fix/security-emoji-safe
branch
from
July 8, 2026 18:06
be886b2 to
6a94d23
Compare
clawsweeper
Bot
added
rating: 🐚 platinum hermit
Contributor
|
Land-ready proof for exact head
The reviewed implementation is the best bounded fix: it retains the existing limits, external-content wrapping, policy decisions, and fail-closed behavior while replacing only the unsafe prefix cuts. |
Contributor
|
Merged via squash.
|
github-actions Bot
pushed a commit
to Desicool/openclaw
that referenced
this pull request
Jul 9, 2026
…rrogate-safe (openclaw#102266) * fix(security): keep channel-metadata and install-policy truncation surrogate-safe channel-metadata.ts and install-policy.ts both had private truncateText functions using String.prototype.slice(0, N) on user-visible text. channel-metadata is explicitly user-controlled untrusted content that gets injected into LLM prompt context -- a dangling surrogate from unsafe truncation would corrupt the prompt with invalid Unicode, affecting token counting and potentially LLM response quality. Replace .slice(0, ...) with truncateUtf16Safe(...) in both files so the truncation point always falls on a complete code-point boundary. Add test coverage for buildUntrustedChannelMetadata with emoji at both the entry-level (400-char) and outer (800-char) truncation boundaries. * test(security): prove UTF-16 truncation boundaries --------- Co-authored-by: Peter Steinberger <[email protected]>
9 tasks
9 tasks
This was referenced Jul 17, 2026
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
2 participants
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.
What Problem This Solves
Security-owned text limits in channel metadata and install-policy results use raw UTF-16 code-unit slices. When a limit stops after the high surrogate of an astral character, OpenClaw introduces malformed text into wrapped prompt metadata, policy block reasons, or normalized findings.
Why This Change Was Made
The two private truncation helpers now use the existing
truncateUtf16Safeimplementation from@openclaw/normalization-core/utf16-slicewhile preserving their current limits and suffix behavior.No config, schema, protocol, dependency, or security decision changes.
User Impact
Long untrusted channel descriptions and long install-policy output no longer end in a surrogate half introduced by OpenClaw. Short text and text not crossing an astral-character boundary remain unchanged.
Evidence
runInstallPolicyassertion for a block reason whose emoji crosses the 1,000-unit boundary.git diff --check: passed.The submitted generic lone-surrogate test was moved from
test/security/to the owningsrc/security/shard and replaced with exact behavioral expectations. The previously untested install-policy change now has production-path coverage.