Skip to content

fix: UTF-8 safe string truncation in browser tool and debug logging - #398

Merged
jamiepine merged 5 commits into
spacedriveapp:mainfrom
l33t0:fix/utf8-byte-slicing-panic
Mar 12, 2026
Merged

fix: UTF-8 safe string truncation in browser tool and debug logging#398
jamiepine merged 5 commits into
spacedriveapp:mainfrom
l33t0:fix/utf8-byte-slicing-panic

Conversation

@l33t0

@l33t0 l33t0 commented Mar 11, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes remaining unsafe byte-level string slicing (&s[..N]) that panics when truncation lands inside a multi-byte UTF-8 character (Cyrillic, CJK, emoji).

  • src/tools/browser.rs - 3 locations: DOM node name (200), input value (100), typed text display (50)
  • src/main.rs - 2 locations: OAuth token debug logging (20)

All switched to floor_char_boundary(), matching the pattern already used across the rest of the codebase.

Note: the file_read tool mentioned in #391 is actually already safe - it uses line-based slicing and truncate_at_char_boundary(). The real culprits were these browser/debug paths.

Closes #391


Also fixes a pre-existing clippy collapsible_if lint in src/tools/send_message_to_another_channel.rs that was failing CI on main after the Signal adapter merge.

Test plan

  • cargo clippy --all-targets -- -D warnings passes clean
  • cargo test --lib -- file — all 16 tests pass
  • Manually test browser tool with CJK/Cyrillic content exceeding truncation limits

Note

This fix eliminates 5 unsafe byte-slicing vulnerabilities across two files by replacing direct slice operations with UTF-8 boundary-aware truncation. Changes are limited to debug output formatting in OAuth logging and DOM snapshot rendering for the browser tool.

Written by Tembo for commit c902563. This will update automatically on new commits.

@coderabbitai

coderabbitai Bot commented Mar 11, 2026

Copy link
Copy Markdown
Contributor

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 3cf2dc92-4545-4ee0-a941-ecf4e06f5306

📥 Commits

Reviewing files that changed from the base of the PR and between 81c1bb8 and e3b6e31.

📒 Files selected for processing (2)
  • src/main.rs
  • src/tools.rs
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/main.rs

Walkthrough

This PR introduces UTF-8 safe truncation mechanisms across the codebase to prevent panics when processing multibyte character sequences. Changes include replacing manual byte-slicing with a character-boundary-aware helper function, adding comprehensive unit tests for edge cases, and updating token output formatting in authentication status commands.

Changes

Cohort / File(s) Summary
Token Output Formatting
src/main.rs
Updated cmd_auth status output to redact access and refresh tokens with a placeholder and display byte length instead of showing first 20 characters.
UTF-8 Safe Truncation Implementation
src/tools/browser.rs
Replaced three manual byte-slicing truncations with truncate_utf8_ellipsis() helper for safe handling of node names, values, and typed text in browser snapshot rendering.
UTF-8 Truncation Test Suite
src/tools.rs
Added six unit tests validating UTF-8 boundary handling for Cyrillic, CJK, emoji, mixed multibyte content, and truncate_output function across various byte limits and edge cases.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

  • PR #276: Implements UTF-8 safe character-boundary-aware truncation using similar helper functions to prevent multibyte-slice panics.
🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Out of Scope Changes check ⚠️ Warning The PR includes a secondary fix for a clippy collapsible_if lint in src/tools/send_message_to_another_channel.rs that is not mentioned in the PR objectives or #391 requirements, representing an out-of-scope change. Either remove the collapsible_if fix from this PR and create a separate PR for it, or update the PR description to explicitly document this secondary lint fix and its justification.
Docstring Coverage ⚠️ Warning Docstring coverage is 70.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and accurately summarizes the main change: fixing UTF-8 safe string truncation across the browser tool and debug logging components.
Description check ✅ Passed The description is well-related to the changeset, providing context about UTF-8 vulnerabilities being fixed and locations affected (browser.rs and main.rs token logging).
Linked Issues check ✅ Passed The PR successfully addresses the core objective from #391: replacing unsafe byte-level string slicing with UTF-8-aware truncation using floor_char_boundary() across five locations (3 in browser.rs, 2 in main.rs) and adds comprehensive unit tests for Cyrillic, CJK, and emoji content.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Fixes unsafe byte-level string slicing that panics on multi-byte
UTF-8 content (Cyrillic, CJK, emoji) in browser tool display
truncation and OAuth token debug logging.

Closes spacedriveapp#391
@l33t0
l33t0 force-pushed the fix/utf8-byte-slicing-panic branch from c902563 to 7751f6c Compare March 11, 2026 14:36
Comment thread src/main.rs
Comment thread src/tools/browser.rs Outdated
@jamiepine
jamiepine enabled auto-merge March 12, 2026 09:48
@jamiepine
jamiepine disabled auto-merge March 12, 2026 10:39
@jamiepine
jamiepine merged commit a01f2c5 into spacedriveapp:main Mar 12, 2026
4 checks passed
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.

CRITICAL: Worker file_read Operation Panics on Multi-byte UTF-8 Content (Cyrillic, CJK, Emoji)

2 participants