Avoid mDNS advertisement response loops - #124
Open
Jim8y wants to merge 1 commit into
Open
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The new test’s cleanup should restore the global dgram.createSocket stub even if stop() fails, and the query filter has avoidable per-packet allocations/extra work that should be tightened given the high-frequency packet path.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR hardens the remote debugger mDNS advertiser so it only responds to valid discovery queries (PTR/SRV/TXT/A/ANY) for the expected service/instance/host names, preventing it from replying to its own announcements and other unrelated packets.
Changes:
- Added DNS question parsing with compression-loop/truncation rejection, and query filtering via
isDiscoveryQuery. - Updated the advertiser to announce only when a relevant discovery query is received (instead of substring matching).
- Added a Node test covering self-response rejection, malformed traffic, a PTR-shaped query, and truncation; added the test to solution items.
File summaries
| File | Description |
|---|---|
OneGate.Codex/onegate/skills/onegate-dapp-debug/scripts/runtime/remote-debugger-advertiser.mjs |
Adds DNS question parsing and gates announcements on relevant mDNS queries to avoid response loops. |
tests/p2-17/mdns.test.mjs |
Introduces coverage for the new query-filtering behavior and basic malformed/truncated packet handling. |
OneGateApp.slnx |
Adds the new test file under Solution Items for visibility. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+28
to
+31
| } finally { | ||
| await advertiser.stop(); | ||
| dgram.createSocket = original; | ||
| } |
Comment on lines
+74
to
+96
| function isDiscoveryQuery(packet, instanceName, hostName) { | ||
| if (packet.length < 12 || (packet.readUInt16BE(2) & 0xf800) !== 0) return false; | ||
| const count = packet.readUInt16BE(4); | ||
| let offset = 12; | ||
| let relevant = false; | ||
| try { | ||
| for (let index = 0; index < count; index++) { | ||
| const question = readQuestionName(packet, offset); | ||
| offset = question.end; | ||
| if (offset + 4 > packet.length) return false; | ||
| const type = packet.readUInt16BE(offset); | ||
| const dnsClass = packet.readUInt16BE(offset + 2) & 0x7fff; | ||
| offset += 4; | ||
| if (dnsClass !== 1) continue; | ||
| relevant ||= question.name === SERVICE_NAME && (type === 12 || type === 255) | ||
| || question.name === instanceName.toLowerCase() && [33, 16, 255].includes(type) | ||
| || question.name === hostName.toLowerCase() && (type === 1 || type === 255); | ||
| } | ||
| return relevant; | ||
| } catch { | ||
| return false; | ||
| } | ||
| } |
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
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.
Summary
Fix the existing remote-debugger mDNS advertiser replying to its own announcements and other response packets.
Validation
node --test tests/p2-17/mdns.test.mjs: 1 test passed, covering self-response rejection, malformed traffic, a real-shaped PTR query and truncation.MdnsRemoteDebuggerDiscoveryreceived the real production host advertiser and reported debuggeraudit-p2-17, port32145. The native event was not injected. A local packet capture showed the guest service query followed by the actual host PTR/SRV/TXT announcement; a 45-second observation recorded two announcements with no response storm.audit-p2-17at198.18.0.1:32145after the native listener started and the unchanged production advertiser was launched. The 45-second real UDP observation recorded two announcements, no storm, and normal advertiser exit. The fixture was removed; a full product rebuild passed with zero warnings/errors and the installed product displayed Home/four tabs. The temporary product source matched the source branch after restoration.c276f4d8dfec85e4983b79f15f0f54e58c457ba7b4158162cca4bf1507a65797, independently based onmaster@623603d634f07eaead14745da87920a356159be0.Limits
This verifies service discovery, not debugger authentication, a wallet operation or a blockchain transaction. Earlier emulator startup and listener-timing failures were retained and were not counted as passing runs. Android's system Bluetooth process reported an unrelated HCI failure; no OneGate crash was observed.
Screenshots, temporary QA fixtures and packet captures remain outside the repository. Raw packet captures contain unrelated local-network discovery traffic and are not for public upload. No screenshot upload, hosted CI pass or maintainer approval is claimed.