feat(mobile-control): support https/wss for direct server URLs when Secure is true - #1450
Open
jkdevito wants to merge 1 commit into
Open
feat(mobile-control): support https/wss for direct server URLs when Secure is true#1450jkdevito wants to merge 1 commit into
jkdevito wants to merge 1 commit into
Conversation
…ecure is true - Add HttpScheme/WsScheme helpers driven by directServer.Secure - Replace hardcoded http/ws literals in UserAppUrlPrefix, touchpanel app URL, _config.local.json ApiPath, remote logging POST, join-response WebSocketUrl and UserAppUrl - Pass Secure flag into the HttpServer constructor so the listener actually negotiates TLS when a cert is configured - Drop TLS 1.1 from EnabledSslProtocols, keep TLS 1.2 (TLS 1.3 is not defined in the net472 SslProtocols enum used by this project) - MobileControlTouchpanelController: rewrite the app-URL IP regex to match and preserve either http or https instead of assuming http
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the Mobile Control direct server so that when directServer.Secure is enabled, all advertised client URLs use https:// and wss:// (instead of hardcoded http:// / ws://), aligning the emitted URLs with the server’s TLS configuration.
Changes:
- Introduces
HttpScheme/WsSchemehelpers driven byDirectServer.Secureand replaces hardcoded schemes across generated URLs. - Passes
DirectServer.Secureinto theHttpServerconstructor and restricts SSL protocols toTls12. - Updates the touchpanel URL-rewrite regex to preserve an existing
http/httpsscheme.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/PepperDash.Essentials.MobileControl/WebSocketServer/MobileControlWebsocketServer.cs | Switches generated app/API/WS URLs to scheme-aware http/https + ws/wss and updates server TLS settings. |
| src/PepperDash.Essentials.MobileControl/Touchpanel/MobileControlTouchpanelController.cs | Preserves http/https when rewriting URLs to the correct processor/CS LAN host IP. |
Comments suppressed due to low confidence (1)
src/PepperDash.Essentials.MobileControl/WebSocketServer/MobileControlWebsocketServer.cs:501
ApiPathis using_parent.Config.DirectServer.Port, but the direct server may be listening onPort(e.g., when the configured port is0and the constructor falls back to50000 + ProgramNumber). This can generate an API URL pointing at the wrong port. UsePortso the advertised API path matches the actual listener port.
ApiPath = string.Format("{0}://{1}:{2}/mc/api", HttpScheme, processorIp, _parent.Config.DirectServer.Port),
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| var appUrl = $"http://{ip}:{_parent.Config.DirectServer.Port}/mc/app?token={touchpanel.Key}"; | ||
| var appUrl = $"{HttpScheme}://{ip}:{_parent.Config.DirectServer.Port}/mc/app?token={touchpanel.Key}"; |
| res.Close(); | ||
|
|
||
| var logRequest = new HttpRequestMessage(HttpMethod.Post, $"http://{_parent.Config.DirectServer.Logging.Host}:{_parent.Config.DirectServer.Logging.Port}/logs") | ||
| var logRequest = new HttpRequestMessage(HttpMethod.Post, $"{HttpScheme}://{_parent.Config.DirectServer.Logging.Host}:{_parent.Config.DirectServer.Logging.Port}/logs") |
Comment on lines
+522
to
526
| var match = Regex.Match(url, @"^(https?)://([^:/]+):\d+/mc/app\?token=.+$"); | ||
| if (match.Success) | ||
| { | ||
| string ipa = match.Groups[1].Value; | ||
| string ipa = match.Groups[2].Value; | ||
| // ip will be "192.168.1.100" |
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.
Closes #1449
Summary
Implements
docs/mobile-control-https-wss-support-plan.md: whendirectServer.Secureistrue, Mobile Control's direct server now advertiseshttps:///wss://URLs instead of hardcodedhttp:///ws://.Changes
PepperDash.Essentials.MobileControl/WebSocketServer/MobileControlWebsocketServer.csHttpScheme/WsSchemehelper properties driven by_parent.Config.DirectServer.Secure.JoinResponse.UserAppUrl.HttpServerconstructor now passes_parent.Config.DirectServer.Secureinstead of a hardcodedfalse.EnabledSslProtocolsset toSslProtocols.Tls12only (Tls13is not defined fornet472; dropped legacyTls11).PepperDash.Essentials.MobileControl/Touchpanel/MobileControlTouchpanelController.csGetUrlWithCorrectIpregexes updated to capture and preserve the scheme (http/https) instead of assuminghttp.Testing
Validated on real hardware (TS-1070 running Crestron's Zoom Room Controller app):
directServer.Secure: true, the server now advertiseshttps://<processor>:<port>/mc/app?token=…&authToken=…and the URL loads over TLS (confirmed viacurl -kv/openssl s_clientand in a desktop browser).authTokenJWT auth layer generated alongside the URL was exercised end-to-end.frame-src 'self'CSP restriction on cross-origin iframing (tracked inmobile-control-zrc-wrapper-app's integration docs; the fix there is an app-level "Option B" architecture change, not an Essentials change).Known follow-ups (not blocking this PR)
selfCres) has a SAN defect: the IP is encoded as aDNS:SAN entry rather than anIPAddress:SAN entry. Worth fixing at the cert-generation source for stricter TLS clients.wss://connection is not yet independently re-verified from a clean browser/device context.Marked as draft pending final hardware sign-off (cert-trust re-verification above).