Repository navigation
fix(core): sanitize peer-controlled names in cleanRemote (#75) - #76
Conversation
cleanRemote only length-capped published name, homeName and owner, so a peer could put another member's @handle or a newline into the display name the client now shows verbatim. Strip @ and control characters/newlines at the record boundary and keep the 60-character cap. Delivery fromName still goes through cleanName and the untrusted fence; list_bots friends lines take the sanitized record. Fixes ThinkFlowLab#75 Co-authored-by: Danyang Chen <birfychen@gmail.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a763683b40
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const cleanRemote = (plugin, bot) => { | ||
| const remote = toRemote(bot.handle) | ||
| return { handle: remote.handle, home: bot.home ? toRemote(bot.home).handle : remote.home, name: String(bot.name ?? remote.handle).slice(0, 60), ...(bot.homeName ? { homeName: String(bot.homeName).slice(0, 60) } : {}), ...(bot.owner ? { owner: String(bot.owner).slice(0, 60) } : {}), plugin, remote: true } | ||
| const name = cleanPublishedName(bot.name) || cleanPublishedName(remote.handle) || 'a remote Bot' |
There was a problem hiding this comment.
When Bot Social processes a normal message.received, plugins/social/src/social.js:495-496 supplies from.name from the peer's display_name, and its cleanText does not strip @. send() at line 427 prefers that value over remoteMembers.get(...).name, while delivery.cleanName also preserves @, so this roster-only sanitization is bypassed and a peer named Eve @coder.bob.home still appears in the model-visible trusted [Message from ...] header. Apply the sanitizer at the remote send boundary and cover the actual Bot Social poll payload.
AGENTS.md reference: AGENTS.md:L18-L18
Useful? React with 👍 / 👎.
| const name = cleanPublishedName(bot.name) || cleanPublishedName(remote.handle) || 'a remote Bot' | ||
| const homeName = cleanPublishedName(bot.homeName) | ||
| const owner = cleanPublishedName(bot.owner) | ||
| return { handle: remote.handle, home: bot.home ? toRemote(bot.home).handle : remote.home, name, ...(homeName ? { homeName } : {}), ...(owner ? { owner } : {}), plugin, remote: true } |
There was a problem hiding this comment.
For a profile upgraded from the previous version, state.flybot.remote and the corresponding remote-chat Bot can already contain an @-bearing peer name. This sanitizer only runs on the next remote.publish; remoteMembers.list() and list_bots read the saved records verbatim, and Bot Social's startup refresh is fire-and-forget with failures swallowed, so an unavailable/removed plugin leaves the spoofed label active indefinitely. Normalize or migrate both persisted roster and chat records on load/read rather than relying on a successful future publish.
AGENTS.md reference: AGENTS.md:L18-L18
Useful? React with 👍 / 👎.
| const same = (a, b) => JSON.stringify(a) === JSON.stringify(b) | ||
| // Peer-controlled labels (display name, home, owner): strip @ so they cannot look | ||
| // like a mention of another Bot, strip controls/newlines, keep a 60-char cap (#75). | ||
| export const cleanPublishedName = (value) => String(value ?? '').replace(/@/g, '').replace(/[\u0000-\u001f\u007f-\u009f\u2028\u2029]/g, ' ').replace(/\s+/g, ' ').trim().slice(0, 60) |
There was a problem hiding this comment.
For peer labels containing bidi controls such as U+202E or U+2066, this regex removes only C0/C1 controls and the two Unicode line separators; \s+ does not remove these Cf characters. They therefore survive in the stored name and can reorder or hide the label and its adjacent handle in trusted headers and UI sender rows, defeating the anti-impersonation boundary despite the documented claim that controls are stripped. Remove Unicode bidi/format controls and add a negative test for them.
AGENTS.md reference: AGENTS.md:L18-L18
Useful? React with 👍 / 👎.
Closes #75.
cleanRemotenow strips@and control characters/newlines from a friend Bot's publishedname,homeNameandowner, then applies the existing 60-char cap. Empty results fall back to the handle (without@) ora remote Bot. The handle field is unchanged.Model-visible paths traced: delivery
fromNameuses the sanitized name and still goes throughcleanNameplus the untrusted fence;list_botsfriend lines come from the same record; group room logs use local team names.Change lives in patch 0002-flybot-api-host (core/ + regenerated patch); ds-bot/ untouched.
Tests: negative cases for a name containing another member's handle, newlines/control chars, and the same for homeName and owner. core: 621 pass; social: 34 pass; patches check, build-client check, lint/arch-docs pass.