Stop carrying contact endpoints over the mesh and drop malformed raw detail - #122
Conversation
|
Warning Review limit reached
Next review available in: 38 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (18)
📝 WalkthroughWalkthroughCoT builders across C#, Kotlin, Python, Swift, and TypeScript now reject raw-detail fragments containing wrapper tags and always emit the server-reply endpoint. Parsers no longer retain contact endpoints, with cross-language tests and documentation updated accordingly. ChangesCoT rebuild hygiene
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
python/tests/test_rebuild_hygiene.py (1)
16-16: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueWildcard import flagged by Ruff (F403).
Explicit imports of the used constants would resolve the static analysis warning and avoid namespace pollution.
♻️ Suggested fix
-from meshtastic_tak.cot_type_mapper import * +from meshtastic_tak.cot_type_mapper import COTTYPE_A_F_G_U_C, COTHOW_M_G🤖 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 `@python/tests/test_rebuild_hygiene.py` at line 16, Replace the wildcard import from meshtastic_tak.cot_type_mapper in test_rebuild_hygiene.py with explicit imports for only the constants and symbols used by the test, eliminating the Ruff F403 warning and namespace pollution.Source: Linters/SAST tools
🤖 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.
Inline comments:
In `@csharp/src/Meshtastic.TAK/CotXmlBuilder.cs`:
- Around line 219-223: Route-specific contact output still omits the fixed
endpoint. Update EmitRoute in csharp/src/Meshtastic.TAK/CotXmlBuilder.cs and the
route case in swift/Sources/MeshtasticTAK/CotXmlBuilder.swift to emit
endpoint="*:-1:stcp"; add corresponding route-packet endpoint assertions in
csharp/tests/Meshtastic.TAK.Tests/RebuildHygieneTests.cs and
swift/Tests/MeshtasticTAKTests/RebuildHygieneTests.swift.
In `@README.md`:
- Line 810: Update TakPacketV2Serializer to omit or clear data.endpoint before
protobuf serialization, preventing direct callers from emitting concrete
endpoints; add a serialization-level regression test covering this behavior. In
README.md lines 810-810, retain the endpoint-elision guarantee once
serializer-side handling is implemented.
---
Nitpick comments:
In `@python/tests/test_rebuild_hygiene.py`:
- Line 16: Replace the wildcard import from meshtastic_tak.cot_type_mapper in
test_rebuild_hygiene.py with explicit imports for only the constants and symbols
used by the test, eliminating the Ruff F403 warning and namespace pollution.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 06b4ffd2-263b-40b6-862c-8054dcd98975
📒 Files selected for processing (18)
README.mdcsharp/src/Meshtastic.TAK/CotXmlBuilder.cscsharp/src/Meshtastic.TAK/CotXmlParser.cscsharp/tests/Meshtastic.TAK.Tests/RebuildHygieneTests.cskotlin/src/commonMain/kotlin/org/meshtastic/tak/CotXmlBuilder.ktkotlin/src/commonMain/kotlin/org/meshtastic/tak/CotXmlParser.ktkotlin/src/commonMain/kotlin/org/meshtastic/tak/TakPacketV2Data.ktkotlin/src/jvmTest/kotlin/org/meshtastic/tak/RebuildHygieneTest.ktpython/src/meshtastic_tak/cot_xml_builder.pypython/src/meshtastic_tak/cot_xml_parser.pypython/tests/test_rebuild_hygiene.pyswift/Sources/MeshtasticTAK/CotXmlBuilder.swiftswift/Sources/MeshtasticTAK/CotXmlParser.swiftswift/Tests/MeshtasticTAKTests/RebuildHygieneTests.swifttypescript/src/CotXmlBuilder.tstypescript/src/CotXmlParser.tstypescript/src/types.tstypescript/tests/rebuildHygiene.test.ts
| | Optimization | Savings | Description | | ||
| |-------------|---------|-------------| | ||
| | **Endpoint normalization** | ~20 B/msg | Default endpoints (`0.0.0.0:4242:tcp`, `*:-1:stcp`) normalized to empty; builder restores the default on reconstruction | | ||
| | **Endpoint dropped** | ~20 B/msg | The `<contact>` endpoint never travels over the mesh: the parser stores none, and the builder always re-emits the `*:-1:stcp` "reply via this server" form. A peer's concrete host is not reachable by other mesh members, so forwarding it would only point the receiving client at a socket it cannot open | |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Endpoint elision stops at parsing, not Kotlin serialization. TakPacketV2Serializer.kt still copies data.endpoint into the protobuf, so direct callers can send a concrete endpoint despite these unconditional documentation claims.
README.md#L810-L810: retain this guarantee only after serializer-side endpoint elision is implemented.kotlin/src/commonMain/kotlin/org/meshtastic/tak/TakPacketV2Data.kt#L48-L49: updateTakPacketV2Serializer.ktto omit or clearendpoint, and add a serialization-level regression test.
📍 Affects 2 files
README.md#L810-L810(this comment)kotlin/src/commonMain/kotlin/org/meshtastic/tak/TakPacketV2Data.kt#L48-L49
🤖 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 `@README.md` at line 810, Update TakPacketV2Serializer to omit or clear
data.endpoint before protobuf serialization, preventing direct callers from
emitting concrete endpoints; add a serialization-level regression test covering
this behavior. In README.md lines 810-810, retain the endpoint-elision guarantee
once serializer-side handling is implemented.
…detail Two rebuild fixes, applied to CotXmlParser/CotXmlBuilder in all five bindings. Contact endpoint ---------------- The parser stored any <contact endpoint="..."> that was not one of the two known defaults, and the builder re-emitted it. Over the mesh that host belongs to the sending peer's own LAN, so every other member rebuilding the event pointed its TAK client at an address it cannot reach and the client failed to create the transmission socket. The parser now never stores an endpoint, and the builder always emits the "*:-1:stcp" reply-via-this-server form, which is what makes ATAK route directed GeoChat / TAK-Talk back down the server stream. This also saves ~20 bytes per message and removes the one spot where the builder interpolated a value into an attribute without escaping it. Raw detail passthrough ---------------------- The raw-detail fallback re-emitted its stored bytes verbatim as the inner content of the single <detail> element being built. Well-formed CoT escapes a literal '<' as '<', so that content can never legitimately hold an <event> or <detail> tag token; a fragment carrying one unbalances the wrapper and hands the receiver a document it can no longer parse as a single event. Those fragments are now left out, and the rest of the event is still built normally. Adds RebuildHygiene tests to all five bindings covering both invariants, plus a mixed-case fragment so the guard's case-insensitivity cannot regress unnoticed. Verified: Kotlin 332, C# 287, TypeScript 250, Python 234, Swift 44 - all passing. No golden files changed.
d4695e0 to
182c83c
Compare
2 participants
Two fixes to how the SDK rebuilds a CoT event, applied consistently across all five bindings.
1. A peer's
<contact>endpoint no longer travels over the meshThe parser stored any
endpointattribute that wasn't one of the two known defaults (0.0.0.0:4242:tcp,*:-1:stcp), and the builder re-emitted it on rebuild.That endpoint is a host on the sending peer's own LAN. Every other mesh member that rebuilds the event hands its TAK client an address it has no route to, and the client fails to open the connection — it surfaces as a transmission socket that could not be created for
<peer-lan-ip>:4243.Now the parser never stores a contact endpoint, and the builder always emits the
*:-1:stcp"reply via this server" form — the same form real ATAK presents server-relayed contacts with, and what makes ATAK route directed GeoChat / TAK-Talk (<marti>) back down the server stream.Side benefits: ~20 bytes per message off the wire, and it removes the one place the Kotlin builder interpolated a value into an XML attribute without escaping it (the replacement is a constant).
2. Raw-detail fragments that would unbalance the wrapper are dropped
The raw-detail fallback re-emits its stored bytes verbatim as the inner content of the single
<detail>element the builder is writing into.Well-formed CoT escapes a literal
<as<, so that inner content can never legitimately contain an<event>or<detail>tag token. A fragment that does carry one is malformed, and re-emitting it as-is closes the wrapper early — the receiver is handed a document it can no longer parse as a single event.The builder now leaves such fragments out and builds the rest of the event normally, so the rebuilt document still parses. Benign fragments are unaffected and still pass through byte-for-byte.
Tests
New
RebuildHygienesuite in each of the five bindings (5 tests × 5 bindings):<foo bar="1"/>) still round-trips verbatim*:-1:stcp, with the concrete host absent<contact endpoint="…">leaves the packet's endpoint empty</DETAIL></Event><Event …>) is dropped tooThat last one closes a real gap: the guard case-folds before matching, but every other test used lowercase tokens, so removing the case-folding would have gone unnoticed. Each binding's test was checked against a mutant with the case-folding removed — the new test fails there while the others still pass.
Verification
No golden files changed —
testdata/golden/,testdata/protobuf/andcompression-report.mdare untouched, since neither fix alters the parsed protobuf for any existing fixture (everycot_xmlfixture carries only*:-1:stcp, which already normalized to empty).Note: Swift's suite needs the Xcode toolchain (
DEVELOPER_DIR=/Applications/Xcode.app/Contents/Developer); under CommandLineTools alone the test target can't compile because of the pre-existingimport TestinginCompatibilityTests.swift.Summary by CodeRabbit
Bug Fixes
Documentation