Repository navigation
Conversation
files.mjs's shell-based file operations (listShell, stat, mkdir, remove, move, copy, chmod, diskUsage, find) interpolated caller-supplied paths into double-quoted shell commands with no escaping, letting $, backticks, or an embedded double-quote break out of the command. Adds utils.quoteShellArg() (the standard POSIX '\'' single-quote technique) and applies it at all nine call sites. Closes #13.
Adds src/api/discover.mjs per the two decided mechanisms on issue #5: - subnet(cidr, port, options): a CIDR-bounded TCP-connect sweep for classic fixed-port devices, with bounded concurrency and a maxHosts safety ceiling. - mdns(options): a hand-written mDNS client (no dependency, per the issue's evaluation of multicast-dns) for wireless-debugging-advertised devices - builds a PTR query, parses PTR/SRV/TXT/A answers including DNS name-compression-pointer decoding, and correlates them into {name, host, port, txt} entries. address/port are overridable so callers (and tests) can target a specific responder instead of the real multicast group. Real-device validation is out of scope here, same posture as the rest of droidsock's protocol work - tracked by #1. Closes #5.
Copilot review on PR #17 flagged that mkdir/chmod's mode and find's maxDepth/type were still interpolated into shell commands unvalidated - mode.toString(8) silently skips the octal conversion for a non-number, letting unvalidated input reach the shell. Adds assertValidMode() and inline validation for maxDepth/type, rejecting anything outside a safe range/whitelist instead of interpolating it. Also fixes maxDepth's truthy check, which dropped a valid maxDepth: 0, and corrects quoteShellArg's JSDoc type to match its actual String() coercion.
Three issues from Copilot review on PR #18: - ingestDnsRecords() created a device entry from a lone SRV/TXT record even without a matching PTR - since joining the multicast group means this socket also receives every other service's mDNS traffic on the segment, that turned unrelated announcements into false-positive "devices". SRV/TXT now only update an entry a PTR record introduced. - mdns() always bound its socket to an ephemeral port. A compliant responder replies via multicast back to address:port, not to the querier's source port, so the real multicast path now binds to that port; a direct-unicast target (tests, or a caller pointing at a specific responder) still uses an ephemeral bind to avoid a same-host port clash. - tests/discover.test.vitest.mjs's afterEach() closed UDP sockets without awaiting completion, a source of flakiness across runs since dgram.Socket.close() is async - now awaited like the TCP servers right above it. Adds a regression test for the SRV/TXT-without-PTR case.
cldmv-bot
Bot
added
! release → master
Contributor
Author
🔒 Dependency Review
|
Follow-up Copilot review on PR #18 (1 new thread + 2 previously-suppressed findings against the original commit): - isMulticastAddress() now checks the whole 224.0.0.0-239.255.255.255 range instead of just the "224." and "239." boundary prefixes - 225.x-238.x addresses were being misdetected as unicast and bound to an ephemeral port, missing real responses. - TXT record parsing now skips a zero-length entry (RFC 6763's "no TXT data" marker) instead of recording it as a bogus {"": true}, and stops at a truncated entry (a length byte claiming more bytes than remain in the record) instead of reading past the record's bounds. - parseCidr() accepts /0 as valid CIDR syntax (computed directly, since JS's `<<` treats a shift of 32 as a no-op) rather than rejecting it as an invalid prefix - maxHosts already guards against actually sweeping it. Adds regression tests for the /0 and TXT-parsing cases.
…plit Third Copilot review on PR #18 (2 new threads + 3 suppressed findings, 2 of which are the same underlying issue the first new thread already cross-referenced at two other locations): - TXT key/value objects (readDnsRecord's parsing, ingestDnsRecords' entry creation) are now Object.create(null) instead of {} - a plain object lets a TXT key like "__proto__" interact with Object.prototype's accessor. The TXT merge step now uses Object.assign() to mutate the existing null-proto object in place instead of `{...entry.txt, ...record.data}`, since spread always produces a normal Object.prototype-based object regardless of the source's prototype - reintroducing the exact risk the null-proto objects exist to avoid. - parseCidr() now validates the "/"-split has exactly 2 parts and the prefix is a plain digit string, rejecting inputs like "1.2.3.4/24/extra" (extra segments silently dropped by destructuring) and "1.2.3.4/" (empty prefix coerced to 0 via Number("")). - subnet() now validates port/timeoutMs/concurrency/maxHosts up front - concurrency <= 0 previously spawned zero sweep workers and silently resolved to [] even when hosts were reachable. Adds regression tests for all three.
…daries Fourth Copilot review on PR #18 (1 new thread + 2 suppressed findings, the latter two being the same underlying issue): - mdns() accepted invalid port/timeoutMs with no validation, unlike subnet(). Now validates both up front, matching subnet()'s behavior. Also marks mdns() itself async so a validation throw becomes a proper rejected promise for non-await callers, rather than a synchronous throw out of a function that otherwise always returns a Promise. - ingestDnsRecords() rebuilt its A-record address cache fresh on every UDP message, so an SRV record could never resolve to a host if its A record arrived in an earlier, separate message (a real possibility once a response is split across multiple packets). The cache is now owned by mdns() and persists for the lifetime of one discovery call, passed into ingestDnsRecords() instead of being rebuilt per-message. Adds regression tests for both.
Sixth Copilot review on PR #18 (1 new thread + 4 suppressed findings, 2 of which duplicate the new thread): - readDnsRecord() joins whatever bytes an A record's rdata holds regardless of rdlength, so a malformed/hostile record (rdlength != 4) could produce a wrong-shaped host string and, worse, silently overwrite a previously-cached valid address for the same hostname. The address cache now only accepts A data that passes isValidIP(). - tests/discover.test.vitest.mjs's null-prototype TXT assertions (3 locations) used toEqual() against a plain-object literal - verified empirically that toEqual doesn't compare prototypes, so a regression back to a plain {} would still pass silently. Added an explicit Object.getPrototypeOf(...) === null check alongside each, which is what actually catches that regression (the review's literal suggestion of just using a null-proto expected value wouldn't have). Adds regression tests for the malformed-A-record case, including that it can't clobber an already-resolved valid address.
Seventh Copilot review on PR #18 (2 suppressed findings, no new threads): - mdns() now calls socket.setMulticastTTL(255) before sending its query, per RFC 6762 section 11 - a querier not sending TTL 255 risks being ignored by strict responders. Node's platform default multicast TTL is typically 1. Harmless on the direct-unicast test path, since it only governs multicast-destined packets. - tests/discover.test.vitest.mjs's encodeName() test helper length- prefixed each DNS label with the source string's .length (UTF-16 code units) instead of its UTF-8 byte length, diverging from discover.mjs's own (correct) encodeDnsName and producing a malformed encoding for any non-ASCII label. Fixed to match; added a regression test with a non-ASCII instance name round-tripping through mdns().
…st helper Eighth Copilot review on PR #18 (2 suppressed findings, no new threads): - encodeDnsName() wrote a label's length byte straight from its byte count with no bound - a label's length byte top two bits are reserved for compression-pointer signaling (RFC 1035 3.1), so only 0-63 is a valid literal-label length, and Buffer.from([n]) silently truncates n>255 to a byte. A caller-controlled serviceType exceeding either limit would corrupt the encoded message in a way no receiver (our own decodeDnsName included) could recover from. Both limits are now enforced with a clear thrown error. - That throw needed to happen somewhere an async caller can catch it: moved the buildPtrQuery() call out of the bind callback (dgram's event-emission machinery, outside any try/catch - a throw there could crash the process) to the top of mdns(), alongside the existing port/timeoutMs validation, so the async function wrapper turns it into a normal rejection. - tests/discover.test.vitest.mjs's buildMdnsResponse() had the same UTF-16-vs-UTF-8 length bug as encodeName() (fixed last round) in its TXT-entry encoding. Fixed to match. Adds regression tests for all three.
Ninth Copilot review on PR #18 (2 suppressed findings, no new threads, same root cause at two test cases). Both cross-message mDNS tests sent their two UDP packets synchronously back-to-back, relying on delivery order UDP doesn't actually guarantee - a CI-flakiness risk on runners/kernels where that happens to matter. Both now send the second packet from the first send()'s own completion callback instead, so the enqueue order is deterministic rather than assumed.
…rity PRs are signed The dependabot-security redirect path in workflow-hotfix-redirector.yml@v4 cherry-picks onto `hotfixes` and needs the bot GPG key to sign the commit, or the redirected PR is silently blocked by required-signatures. wire the CLDMV_BOT_* signing secrets into the caller. See CLDMV/.github#257.
Tenth Copilot review on PR #18 (1 suppressed finding, no new threads). listenOn() never rejected or resolved if net.Server emitted an "error" event during listen() (e.g. EADDRNOTAVAIL/EACCES) - the promise would hang until Vitest's own timeout. It also pushed the server into openServers before confirming a successful bind, so teardown could try to close a server that never actually started. Now rejects on the server's "error" event, and only registers the server for teardown after listen() succeeds.
Eleventh Copilot review on PR #18 (1 suppressed finding, no new threads). readDnsRecord() decoded PTR/SRV rdata without checking that the decoded name stayed within rdataEnd. decodeDnsName() has no notion of rdlength, so a truncated/malformed record could make it walk straight into the next record's bytes on the wire and fabricate an incorrect name (or, for SRV, priority/weight/port off a too-short rdata) instead of the parser treating the record as invalid. PTR now nulls its data when the decoded name's end exceeds rdataEnd; SRV nulls its data outright when rdlength < 6 (too small to hold priority/weight/port), and also when its target name overruns. Both follow the same "null data, caller skips it" pattern already used for malformed A records. ingestDnsRecords() now guards both consumption sites against a null record.data.
…rity PRs are signed (#20)
cldmv-bot
Bot
added
the
type: ci
cldmv-bot
Bot
added
the
type: config
Shinrai
previously approved these changes
Sep 3, 2026
cldmv-bot
Bot
changed the title
…shell-fix
Committed per-version changelog file so the next-release refresh uses it
directly as the release-PR body instead of the auto-generated commit
summary (CLDMV/.github v4 release flow, docs/changelog/v1/{version}.md
lookup) - works around CLDMV/.github#262 for this release, since the
generator finds this file before it ever needs the commit-range that bug
report is about.
cldmv-bot
Bot
added
type: documentation
Shinrai
approved these changes
Sep 3, 2026
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.
DroidSock v1.2.0 Changelog
Release Status: Minor release
Overview
v1.2.0 adds device discovery - finding ADB-capable devices on the network without already knowing an address - and closes a shell-injection gap present in every
files.*method that shells out to the device.🐛 Fixed
files.mjs- every shell-based file operation (listShell,stat,mkdir,remove,move,copy,chmod,diskUsage,find) interpolated a caller-supplied remote path into a double-quoted shell command with no escaping, letting a path containing$, backticks, or an embedded"break out of the intended command. All nine call sites now go through a sharedutils.quoteShellArg()helper using the standard POSIX'\''single-quote escaping technique.mkdir/chmod'smodeandfind'smaxDepth/typeare also now validated before being interpolated, closing a couple of related unvalidated-input gaps found during review.✨ Added
Device discovery (
discover.subnet/discover.mdns)discover.subnet(cidr, port, options)sweeps a CIDR range for a reachable TCP port - the classic fixed-addressadb connectflow - with bounded concurrency and amaxHostssafety ceiling.discover.mdns(options)is a hand-written mDNS client (built directly from RFC 1035/6762/6763, no dependency) that queries for_adb-tls-connect._tcp.local-style wireless-debugging advertisements and correlates PTR/SRV/TXT/A records into{name, host, port, txt}entries. EXPERIMENTAL - both are built from the protocol specs and covered by extensive tests against real loopback TCP/UDP sockets, but neither has been run against a real device or a real multicast network yet. See #1.📋 Known Limitations
discover.subnet/discover.mdns, like the rest of droidsock's protocol work, haven't been validated against real hardware or a real network yet. #1 tracks the real-device session needed to validate the protocol stack they depend on.← Back to README
👥 Contributors
Avg: 58.4% ·
144011b· Node lts/*