fix(llc): sanitize header string - #1289
Conversation
📝 WalkthroughWalkthroughThe PR sanitizes ChangesClient metadata transport
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1289 +/- ##
==========================================
+ Coverage 11.20% 11.22% +0.02%
==========================================
Files 686 686
Lines 50350 50361 +11
==========================================
+ Hits 5640 5652 +12
+ Misses 44710 44709 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
renefloor
left a comment
renefloor
left a comment
There was a problem hiding this comment.
🔴 Sanitizing to ASCII isn't enough — the value is also interpolated raw into WebSocket URLs
xStreamClientHeader is used unencoded as a query parameter in two places:
- packages/stream_video/lib/src/sfu/ws/sfu_ws.dart:51
- packages/stream_video/lib/src/coordinator/open_api/coordinator_ws.dart:37
The sanitizer keeps #, &, ?, % — all URL-significant. I verified the impact with Uri.parse on the real SFU URL shape:
appName "Tag #1" → query={X-Stream-Client: ...|app=Tag }
appName "Tom & Jerry" → query={X-Stream-Client: ...|app=Tom , " Jerry": "", attempt: 1}
A # in the app name silently breaks the SFU handshake entirely. This is the same class of bug the PR is fixing (unusual app names), so it belongs in this PR rather than a follow-up. Preferred fix at the call sites:
'&X-Stream-Client=${Uri.encodeQueryComponent(xStreamClientHeader)}'
That also fixes the pre-existing unencoded space in os=android 16. If you'd rather keep the change local to the sanitizer, restrict the allowlist instead of using a broad printable range — but encoding at the URL boundary is the correct layer.
🟡 CHANGELOG heading should be ## Upcoming
The repo convention is ## Upcoming (see 0849531, 32a1181, 2b87908); release commits rewrite that heading to the version number. ## Unreleased will be missed by that flow.
🟡 Empty-after-sanitization values leave malformed segments
Nothing handles a value that sanitizes to '':
- App named e.g. 日本語アプリ → app= (empty segment, no signal, still sent)
- osVersion sanitizing to empty → 'os=${name} ' with a trailing space, since the os= branch interpolates unconditionally and .trim() is applied per-value, not to the segment
Suggest treating empty-after-sanitize as absent:
String? _sanitized(String? value) {
if (value == null) return null;
final result = /* filter */ .trim();
return result.isEmpty ? null : result;
}
…then the existing if (x case final v?) patterns handle omission naturally, and the os switch can key on the sanitized version.
🔵 Minor
- Per-call cost / repetition: the getter is recomputed on every HTTP request and WS connect, re-scanning up to 5 strings each time, and _sanitizeHeaderValue appears at 6 call sites. Sanitizing once — in VideoEnvironmentCollector._collect() or the VideoEnvironment constructor — would be cheaper, remove the repetition, and benefit non-header consumers too. Negligible perf impact either way; mostly a readability win.
- codeUnits vs runes: correct as written (surrogate halves are both > 0x7E), just worth a comment so it doesn't look accidental.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/stream_video/test/src/video_environment_test.dart (1)
6-20: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover
browserNamein the all-fields test.The test named “builds header from all fields” does not set or assert the new
browsersegment. Add a browser value so this regression test covers every header component.Proposed test update
appVersion: '2.5.1', deviceModel: 'Nothing A142', + browserName: 'Chrome', ); expect( environment.xStreamClientHeader, 'stream-video-flutter-v1.4.1|app=MyApp|app_version=2.5.1|' - 'os=android 16|device_model=Nothing A142', + 'os=android 16|device_model=Nothing A142|browser=Chrome', );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/stream_video/test/src/video_environment_test.dart` around lines 6 - 20, Update the “builds header from all fields” test around VideoEnvironment to provide a browserName value and include the corresponding browser segment in the expected xStreamClientHeader assertion, ensuring every header component is covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/stream_video/test/src/video_environment_test.dart`:
- Around line 6-20: Update the “builds header from all fields” test around
VideoEnvironment to provide a browserName value and include the corresponding
browser segment in the expected xStreamClientHeader assertion, ensuring every
header component is covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6007591a-25f6-4dbd-8bf7-d1b23e66e63a
📒 Files selected for processing (5)
packages/stream_video/CHANGELOG.mdpackages/stream_video/lib/src/coordinator/open_api/coordinator_ws.dartpackages/stream_video/lib/src/sfu/ws/sfu_ws.dartpackages/stream_video/lib/src/video_environment.dartpackages/stream_video/test/src/video_environment_test.dart
2 participants
Fixes #1283
Resolves FLU-627
Fixed a
FormatExceptionwhen sending requests if the application name (or other device/app info) contains non-ASCII characters. Values included in theX-Stream-Clientheader are now sanitized to valid header characters.Summary by CodeRabbit
Bug Fixes
Tests