feat(mobile-control): client allowlist, automatic blocking of abusive addresses, and request hardening (all opt-in)

[anthony-lopez-pd]

Why

On a CP4N, Mobile Control's direct server is reached from the LAN through a router port map (ADDPORTMAP <port> <port> <CS address>), because Crestron places the server on the Control Subnet. Once that map exists, anything on the LAN can talk to the server, and there is currently no way to limit who. GET requests also don't log the client's address, so unwanted traffic can't be attributed to a host.

In the field, repeated vulnerability-scanner traffic against that port was followed within about a minute by the Essentials process aborting (External Debugger Dump, SimplSharpPro exited ungracefully), and after three quick restarts the watchdog gives up (LogicEngine(1) restarted too many times) until the processor is power cycled. This was seen in five separate scan windows over four outages. One of them has an exception one second before the abort:

[EROR][appServer-directServer] Caught an exception in the OnGet handler
System.IO.IOException: Unable to write data to the transport connection: Connection reset by peer
  at Socket.Send ... ResponseStream.flushHeaders ... HttpConnection.Close
  at HttpListenerResponse.Close ()
  at MobileControlWebsocketServer.Server_OnGet

The request was for a path the handler doesn't serve, so it went through the 404 branch, which writes a reply on a connection the scanner had already reset.

I have not proven this exception is the crash. It is caught by the handler. The core dumps contain no readable stack, so what I can say is that the failing operation is a write to a reset connection in the response-close path, and that the same failure on a thread-pool thread outside a try/catch would terminate the process.

Everything new is off unless configured

Setting (directServer.…)	Default	Effect when set
allowedClientNetworks	absent = no filtering	HTTP requests from other addresses are refused (connection closed, no reply). Loopback and Control Subnet always allowed.
dropUnrecognisedRequests	absent/false = reply 404, as before	Requests for unhandled paths get no reply.
autoBlock.enabled	absent/false = off	Blocks an address that sends a burst of unwanted requests, then unblocks it. See below.

Not behind a setting, and I want to be upfront about them:

• The += fix in HandleUserAppRequest (a bug fix; normal request paths never reach it).
• GET requests now log the client address at Verbose (POST already did), logged paths are truncated at 200 characters, and requests for unrecognised paths log one Information line per source address per minute. All logging only.
• If a previous autoBlock run left blocks recorded in autoBlockedIps-<deviceKey>.json, they are removed on schedule even if the setting has since been turned off. Without that file nothing happens.

What changed (six commits)

1. fix: stop mutating _userAppBaseHref: HandleUserAppRequest evaluated _userAppBaseHref += "/" inside an if condition, appending a slash to a shared field the first time the sub-expression was evaluated. Now compares against _userAppBaseHref + "/".
2. feat: allowedClientNetworks allowlist and source-address logging: described in the table above. Invalid entries are logged and skipped; a list of only invalid entries still turns filtering on, so a typo can't silently open the server. Refusals log at Warning, at most once per minute per source address.
3. fix: drop refused connections with Abort(): refused requests get HttpListenerResponse.Abort() instead of a 403, so there is no write to fail on a reset connection.
4. fix: drop connections for unrecognised GET paths: same for the 404 branch. As first written this was unconditional; commit 5 makes it opt-in.
5. feat: autoBlock …; make dropping unrecognised requests opt-in:
   • Counts unrecognised-path requests and allowlist refusals per address. At requestsPerMinute (default 10) within a minute, runs ADDBLOCKEDIP, which blocks the address completely. Removes it after blockMinutes (default 30). The processor's SETLOCKOUTTIME does not apply to manual blocks (they list as "blocked indefinitely"), so removal has to be done here.
   • Never blocks loopback, the Control Subnet, the processor's own addresses, allowedClientNetworks, or neverBlock. IPv4 only; 4-series appliances only. Anything that goes into a console command first passes a strict dotted-decimal check.
   • dryRun logs what would be blocked and blocks nothing. maxConcurrentBlocks (default 8) caps how many it holds.
   • Blocks survive reboots and the program can stop at any time, so ownership is written to autoBlockedIps-<deviceKey>.json before the command runs. After a restart it removes whatever has expired. It only ever removes addresses it added, never one added by hand, and never uses remblockedip ALL. Removal is confirmed against listblocked, and retried if the address is still listed.
   • Console calls run off the request thread.
6. fix: don't count ordinary browser requests as unwanted traffic: found while testing in a real browser. Every page load of the app makes two requests to /mc/assets/* (the browser's preload scanner asks for ./assets/* relative to /mc/ before the page's inline script has set its <base>; they 404, and the real requests then succeed under /mc/app/assets/). Browsers also ask for /favicon.ico. In commit 5 these were counted, so at the default of 10 per minute a person reloading the app five times in a minute would have been blocked. /mc/assets/* and /favicon.ico are now neither logged at Information nor counted; they are answered exactly as before. Every other unrecognised path still counts, including near-misses like /mc/assets, /mc/assetsx/ and /x/favicon.ico. A client sending only those could never be auto-blocked, which is no worse than before.

Behavior to review

• Refused clients (and, with dropUnrecognisedRequests, clients asking for unhandled paths) see a closed connection instead of a 403/404. Some clients retry on reset; in testing one retried three more times. Only the first is logged per minute.
• autoBlock blocks an address completely, every port. If the address is shared (a scan host that is also a patch or monitoring server, or a NAT address), everything behind it is cut off. neverBlock exists for that.
• A blocked scanner shows the host as unreachable partway through its scan. That is a decision for whoever owns the scanner.
• The allowlist does not cover websocket connections (their paths embed a per-client token), and neither feature can help with faults inside WebSocketSharp's own request parsing, which runs before these handlers.

Tested

On a CP4N (firmware 2.8006) with a port map from a LAN port to the Control Subnet address, one ordinary LAN client, GET /mc/api/version and made-up paths:

Config	Result
nothing new set	unknown path → 404, real request → 200, nothing blocked, no state file
allowedClientNetworks excluding the client	connection closed, no response; one Warning with the client's address for three requests
allowedClientNetworks including the client	200
dropUnrecognisedRequests: true	unknown path → connection closed; real requests still answered
autoBlock with dryRun	logged Would block at exactly the 10th unwanted request; nothing blocked; client unaffected
autoBlock with the client in neverBlock	14 unwanted requests; not blocked
autoBlock with dryRun, 8 real Chrome page loads in about a minute (16 requests to /mc/assets/)	no Would block
same, then 14 requests to made-up paths	Would block at the 10th

The client's real address was visible to the server through the port map, so all of this applies to forwarded traffic.

Pure logic was tested separately by extracting the code from this file: CIDR parsing and matching (28 cases including /25 and /29 boundaries and IPv4-mapped addresses) and the autoBlock decision pieces (41 cases; plus 18 for the browser-request exemption: window counting and expiry, address validation including newline and shell-metacharacter inputs, parsing listblocked output, IPv6 handling). There is no test project for this assembly.

Not tested

• An actual block and its expiry. The only route to the test processor was the address that would have been blocked, and a block is total, so a live test would have locked out the tester. This needs a second client. The block/unblock path is the part to try first on real hardware, ideally with blockMinutes: 1.
• Touchpanels connecting over the Control Subnet with these changes (the bench CS port had no link).
• A processor with no Control Subnet adapter.
• Any hostile or malformed traffic. Every request used was an ordinary client request.

Removing the port map also stops the traffic reaching the server at all and needs no code. This PR is for deployments that need the map to stay.

🤖 Generated with Claude Code

[Copilot]

Copilot review overview

🟡 Changes recommended

Blocking-state reliability, address handling, and concurrency defects could cause unintended access denial or permanent processor blocks.

Review effort: Balanced
Findings: 2

Open (2)

• Apply DropUnrecognisedRequests to unhandled POST requests · New
• Normalize IPv4-mapped addresses before loopback checks · New

What changed in this PR

Adds opt-in HTTP client filtering, abusive-address blocking, and safer request handling to Mobile Control’s direct server.

Changes:

• Adds CIDR allowlisting and source-aware logging.
• Adds persistent, configurable automatic IP blocking.
• Adds optional connection dropping and fixes shared path mutation.

File	Description
MobileControlWebsocketServer.cs	Implements filtering, blocking, logging, persistence, and request hardening.
MobileControlConfig.cs	Defines configuration for the new opt-in controls.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Copilot review overview

🟡 Changes recommended

Request coverage, concurrency, and persisted block ownership have unresolved correctness risks.

Review effort: Balanced
Findings: 3 · 1

Open (4)

• POST handler is registered only when remote logging is enabled · New
• Shared block state corrupts ownership across server instances · New
• Failed persistence still schedules unmanaged blocks · New
• Unhandled POST requests are not recorded · New

Resolved since last review (2)

• Normalize IPv4-mapped addresses before loopback checks
• Apply DropUnrecognisedRequests to unhandled POST requests

Previously missed (4)

In code that hasn't changed since last review

Non-atomic request tracking allows duplicate logs

src/​PepperDash.Essentials.MobileControl/​WebSocketServer/​MobileControlWebsocketServer.cs:505

The read/check/write sequence is not atomic: concurrent requests for the same source can all observe a missing or stale timestamp, then each log. This defeats the claimed once-per-minute limit precisely during concurrent scanner bursts; protect the decision and timestamp update with one lock or an atomic update loop.

Concurrent bursts can schedule duplicate block commands

src/​PepperDash.Essentials.MobileControl/​WebSocketServer/​MobileControlWebsocketServer.cs:872

After RecordUnwantedRequest releases its lock, another burst can also cross the threshold before this block is recorded. Both calls then schedule ADDBLOCKEDIP; the later command may report “already blocked” and remove the shared tracking entry, leaving the successful block unmanaged. Recheck the address while holding this lock before reserving it.

Failed block-list queries incorrectly remove ownership records

src/​PepperDash.Essentials.MobileControl/​WebSocketServer/​MobileControlWebsocketServer.cs:961

The boolean result from SendControlSystemCommand is ignored. If listblocked fails and leaves list empty, BlockListContains returns false and the code deletes its ownership record even though the address may still be blocked, preventing future retries. Treat a failed query as unverifiable and retain the entry.

Conflicting XML summaries leave logging config undocumented

src/​PepperDash.Essentials.MobileControl/​MobileControlConfig.cs:145

The pre-existing MobileControlLoggingConfig summary now precedes MobileControlAutoBlockConfig, giving the new class two conflicting <summary> elements and leaving the logging class undocumented. Remove this stale summary here and restore it immediately above MobileControlLoggingConfig.

[Copilot]

Copilot review overview

🔵 Needs a closer look

Processor-wide block lifecycle safety remains unresolved, and actual blocking and expiry still need hardware validation.

Review effort: Balanced
Findings: 2

Open (2)

• Prevent expired queued blocks from becoming indefinite · New
• Avoid buffering log uploads when remote logging is disabled · New

Resolved since last review (4)

• Failed persistence still schedules unmanaged blocks
• Shared block state corrupts ownership across server instances
• POST handler is registered only when remote logging is enabled
• Unhandled POST requests are not recorded

Previously missed (1)

In code that hasn't changed since last review

Handle IPv4 and IPv6 control subnet comparisons safely

src/​PepperDash.Essentials.MobileControl/​WebSocketServer/​MobileControlWebsocketServer.cs:428

With filtering enabled and an IPv4 Control Subnet, a native IPv6 client throws here before its configured networks are checked. GetNetworkAddress rejects differing address/mask lengths (src/PepperDash.Essentials.Core/Extensions/IpAddressExtensions.cs:56–57), so even a matching IPv6 allowlist entry cannot grant access. Skip the Control Subnet comparison when the address families differ, then continue checking the configured networks.

[Copilot]

Copilot review overview

🔵 Needs a closer look

Processor-wide blocking has unresolved recovery risks, and its block-and-expiry lifecycle still needs hardware validation.

Review effort: Balanced
Findings: 2

Open (2)

• IPv4-mapped CIDR networks never match normalized client addresses · New
• Restart recovery can remove blocks owned by others · New

Resolved since last review (2)

• Avoid buffering log uploads when remote logging is disabled
• Prevent expired queued blocks from becoming indefinite

Previously missed (1)

In code that hasn't changed since last review

Ignored load failures can erase ownership records

src/​PepperDash.Essentials.MobileControl/​WebSocketServer/​MobileControlWebsocketServer.cs:1085

The load result is ignored, so a read or deserialization failure still allows automatic blocking to start with missing ownership records. If a later save succeeds, it replaces the existing file with this incomplete dictionary, permanently losing the records needed to remove earlier indefinite processor blocks. Distinguish an absent file from a failed load, preserve the failed file, and prevent new blocks and state-file overwrites until recovery succeeds. The HTTP server can continue running without automatic blocking.

[anthony-lopez-pd]

Not changing this one in this round; leaving it open for a decision. The scenarios are real, but closing them means picking between two failure modes:

• Admin blocks the same address while the program is stopped between record and command. Persisting a pending flag and dropping pending records after a restart without remblockedip fixes this, but reopens the case the record-before-command ordering exists for: if the program stopped after ADDBLOCKEDIP ran but before the confirmed state was saved, the block would be left with no owner. listblocked cannot tell the two cases apart, because the console does not record who added a block. So an ambiguous record is either removed, which risks removing an admin block, or kept forever, which risks leaving a block nobody owns.
• Two instances blocking the same address. This needs a shared, processor-wide owner (one coordinator or a reference count) rather than per-instance files. That is a design change, and two direct servers on one processor is not a configuration we have in the field.

Today's behaviour favours never leaving an unowned block, at the cost of these narrow windows: a restart in the gap between record and command, coinciding with a manual block of the same address. Happy to implement whichever trade-off the reviewer prefers.