Secure Connection on Shared Machines #1021
Conversation
- Create IFacade interface defining common API for SocketIO and SignalR - Update SocketIoFacade to implement IFacade - Update SignalRFacade to implement IFacade (add Connect no-op) - Update RuntimeControllerBase.Socket to return IFacade - Update RuntimeControllerAspNetBase.Socket to return IFacade - Update RuntimeControllerDotNetFirst.Socket to return IFacade - Update ElectronNetRuntime.GetSocket() to return IFacade - Update BridgeConnector.Socket to return IFacade This enables polymorphic usage of both facades throughout the codebase and prepares for full Electron API integration with SignalR mode.
- Update signalr-bridge.js to handle .NET→Electron events via 'event' channel - Add socket.io-compatible .on() and .emit() methods to SignalRBridge - Update main.js to load all Electron API modules with SignalR bridge - Update SignalRFacade.Emit() to send events via 'event' channel - Add ElectronHub.ElectronEvent() to receive Electron→.NET events - Add SignalRFacade.TriggerEvent() to invoke .NET event handlers - Remove duplicate ElectronEvent method from hub This enables full bidirectional communication: - .NET can call Electron APIs via Emit (e.g., createBrowserWindow) - Electron can send events back to .NET (e.g., BrowserWindowCreated) - Event handlers registered via On/Once now work with SignalR
- Add RunReadyCallback execution when SignalR connects - This triggers window creation and other app initialization - Fixes missing variables in main.js (desktopCapturer, electronHostHook, touchBar, shellApi) - Window now opens successfully in SignalR mode Known issue: EPIPE console error when logging to closed pipe (to be fixed)
- Wrap all console.log/error calls in try-catch to handle EPIPE - Disable SignalR client logging (LogLevel.None) - Prevents Electron crash when console pipes are closed This fixes the 'EPIPE: broken pipe, write' error that was preventing the Electron window from displaying.
- Add WebSockets middleware to ASP.NET pipeline - Move HTTPS redirect to production only - Add extensive debug logging in startSignalRApiBridge - Enable Warning level logging in SignalR client Issue: signalRBridge.connect() hangs and never resolves - Connection starts but never completes - No error messages from SignalR client - ASP.NET shuts down after timeout - ElectronHub.OnConnectedAsync never called Next: Investigate why WebSocket connection doesn't establish
ROOT CAUSE: Electron quits when app.on('ready') completes with 0 windows.
In SignalR mode, no windows are created immediately, so Electron exits,
triggering ElectronProcess_Stopped and shutting down ASP.NET.
SOLUTION: Create an invisible 1x1 keep-alive window in SignalR mode to
prevent Electron from quitting while waiting for SignalR connection.
Also:
- Make app.on('ready') async and await startSignalRApiBridge()
- Add window-all-closed handler for SignalR mode
- Add extensive debug logging to track lifecycle
- Don't subscribe to electronProcess.Ready in SignalR controller
This fixes the premature shutdown that prevented SignalR connection.
- Fix duplicate SignalR connection in main.js (removed redundant connect block) - Fix race condition: Electron now signals 'electron-host-ready' after loading API modules - Fix type conversion in SignalRFacade for event handlers (handle JsonElement and numeric types) - Fix SignalR argument mismatch: pass args as array instead of spread, use object[] instead of params - .NET now waits for electron-host-ready before calling app ready callback
- Changed event handler to receive args as single array parameter - Spread array elements as individual arguments to match Socket.IO behavior
- Destroy keep-alive window when first real window is created (enables proper window-all-closed behavior) - Update ElectronNetRuntime.AspNetWebPort with actual port after Kestrel starts (fixes http://localhost:0 issue)
- Fix middleware order: UseAntiforgery must be between UseRouting and UseEndpoints - Add UseStaticFiles() to serve wwwroot content - Fix scoped CSS bundle reference: use lowercase 'electronnet-samples-blazorsignalr.styles.css' to match generated asset name - Add HTTP request logging for debugging - Enable detailed logging for routing and static files in development
Remove excessive console logging that was added during debugging: - Removed verbose logging from Program.cs (app ready callback steps) - Removed HTTP request logging middleware - Cleaned up RuntimeControllerAspNetDotnetFirstSignalR lifecycle logging - Streamlined ElectronHub connection/event logging - Simplified SignalRFacade event handling logging - Reduced JavaScript logging in main.js and signalr-bridge.js - Reset log levels to Warning for SignalR components in appsettings Kept only essential error logging and critical state transitions. Production-ready logging levels maintained.
Added comprehensive code comments explaining: - RuntimeControllerAspNetDotnetFirstSignalR: .NET-first startup flow and key differences from Socket.IO - SignalRFacade: Type conversion handling and event propagation details - signalr-bridge.js: Socket.IO compatibility layer and arg handling - main.js: Keep-alive window pattern and SignalR startup sequence Comments focus on explaining WHY decisions were made, not just WHAT the code does.
Did you ever look at the GitHub diff before disagreeing? |
Yes. Most of the formatting changes (like 90%) are in the Host section. And that is the one that should be excluded (these are Prettier changes and the resulting formatting is actually as it should be; the previous formatting is just awful). |
|
Just to be clear: The comments I made were just examples - like I said, the whole PR is full of those kinds of changes. |
Do you know how a tree view works? Because you can just collapse the Host directory. I am sorry for not being direct here; I think I wrote that there are some changes which should not be part of the PR. But that was not the point of the request. The point was the general change. Now, again this is agreeable a mess. But "full of" would imply that all of the .NET code is also just there due to formatting changes, which certainly is not the case. So I am not sure we have the same base line here. Where I am coming from is a conceptual review - mostly of the .NET code (SignalR integration) and the design documents. This is what I am looking at, too - I was interested in your opinion on this. |
|
When there are no changes to the Host files, why are they in this PR, then? In all PRs I have made, I have cleanly separated formatting changes from functional changes. Not from a nitpicking perspective, but for the simple reason that you cannot properly review code changes when there are hundreds of changes which aren't actual changes. I'm not willing - nor do I have the time - to look through 128 changed files. From a high-level perspective, my verdict is that a PR which needs changes in 128 files isn't worth considering at all at this point (where we are just about to stabilize things). |
Developers named Claude, Copilot or GPT, I assume... |
I did not write "no changes". But nevermind (I think only 2 files contained changes - most importantly
As mentioned - fair enough. Also, of course its good to separate. The problem with the mixing of the formatting happened on my side (as the commit history shows); hence my comment to ignore these for now. That would still be around a hundred files, so as mentioned it's reasonable that this is too much. I never wrote anything against it.
Fair enough, too. Right now this is not about to rush it, but (as pointed out in the other comment that still seems to be ignored) about the concept. Maybe I should have created this as a draft, but I always dislike draft PRs especially when I want to see what our automation shows for it.
True - that's also what was communicated to me that the bulk work has been done by coding agents. But I personally do not care what tools the people use, I only followed up on devs behind the proposal and their request to have a look and see if there is something useful for us. While I understand your position I do not agree with your attitude. Maybe the writing does not help - but there is really no need to act like this. You have no time? This is fine. You think this is too much / not being brought in using the right format? Fair enough. But please keep the attitude aside. |
I'm slightly annoyed, yes. Wouldn't you be when somebody would throw gigantic AI-generated mess at you, asking to review it? I have nothing against AI, I'm using it myself, yet I'm always transparent and mention it. From a professional side I also need to say that those changes cannot be trusted. When AI is used for coding in a way that it rewrites whole files (from "memory"), which has obviously been the case here, then there's always a high risk for mistakes, because at some point information is falling out of the context window and LLMs are not aware of that. Instead, they are producing something similar but no longer accurate. |
|
Just another note - maybe there was a misunderstanding regarding my saying "I have no time". I meant and mean it literally. |
That's literally what happened here. Hence my request for you to look at the concept. Again, I take the blame that the formatting of the TS came into this one and that I should have been more direct w.r.t. the objective. However, I did write it a couple of times and you seem to ignore that point. |
softworkz
left a comment
softworkz
left a comment
There was a problem hiding this comment.
That's literally what happened here. Hence my request for you to look at the concept. Again, I take the blame that the formatting of the TS came into this one and that I should have been more direct w.r.t. the objective. However, I did write it a couple of times and you seem to ignore that point.
Yes, because with this insane diff, I'm not willing to dig through it trying to understand any concept.
Even when ignoring the Host project, these changes are done in a way that someone may do when using the code for oneself alone, but this is not a suitable contribution to an open-source project. Change all logging - WTH?
These changes are invasive without taking care about the project as-is,, which is evident from a range of indicators. There's no effort visible for making a good contribution to the project, and in this regard, I see no reason why I should take effort then, in analyzing the concept.
This is a clear rejection from my side, it's not acceptable in this form
(neither for me to look at it any further)
…into feature/secure-connection
There was a problem hiding this comment.
Thanks a lot for cleaning this up - even though it's incomplete and a lot of the ugly stuff is just hidden under the carpet - it's a lot better to look at now, and I see clearly (what I wasn't able to see before) how we can pull this out of the messiness corner:
The determination between Regular and Signalr should be a
Build-Time Decision
Rough Outline
- We add new checkbox in the project designer: Use Signaler
(i.e.: MSBuild property) - It's only visible when the project is an ASP.NET project
(+ build error when set with a non-asp.net project) - This build property gets into the project via assembly attributes (like we already have some)
- For certain files, we have two versions: One for SignalR, one for normal
- main.js, possibly some of the host files - just where it makes sense
- Maybe also for the package.json template (otherwise, we need to change it depending on "Use Signalr", so that SignalR dependencies are only included when "Use Signalr" is enabled.
- vice-versa: no SocketIO deps when SignaLR is used
- In turn, it's not an environment vaiable to decide about whether it's signalr or not
- Unsupported cases should error out clearly to avoid confusion
Benefits
- Better isolation between these modes of operation
- Higher clarity, less confusion
- Less error-prone, more robust
- Changes can be made to one, with less risk of affecting the other
- etc..
I haven't looked into every detail yet, but I hope this gives the orientation you were asking for. If you want to go for it, there's quite a bit of work to do.
I can help on the MSBuild side, if you wish.
Best,
sw
| // For SignalR modes, use port 0 for dynamic port assignment | ||
| var usePort0 = ElectronNetRuntime.StartupMethod == StartupMethod.PackagedDotnetFirstSignalR || | ||
| ElectronNetRuntime.StartupMethod == StartupMethod.UnpackedDotnetFirstSignalR; | ||
|
|
||
| var webPort = usePort0 ? 0 : PortHelper.GetFreePort(ElectronNetRuntime.AspNetWebPort ?? ElectronNetRuntime.DefaultWebPort); |
There was a problem hiding this comment.
A switch might be better readable
| services.AddSingleton<IElectronNetRuntimeController, RuntimeControllerAspNetDotnetFirst>(); | ||
| break; | ||
| case StartupMethod.PackagedDotnetFirstSignalR: | ||
| case StartupMethod.UnpackedDotnetFirstSignalR: |
There was a problem hiding this comment.
Since SignalR is always and only dotnet-first, it might add clarity to just use
PackagedSignalR
UnpackedSignalR
| /// - Event args are passed as arrays to match SignalR serialization behavior | ||
| /// - Connection ID is set by ElectronHub when Electron client connects | ||
| /// </summary> | ||
| internal class SignalRFacade : IFacade |
There was a problem hiding this comment.
- As we need an interface here, it might a good chance to get rid of the unhandy work "Facade".
How about naming it like this:
- IFacade => ISocketConnection
- SocketIoFacade => SocketIOConnection
- SignalRFacade => SignalRConnection
There was a problem hiding this comment.
Sounds good to me. Before I change that I'd like to discuss what the future of SignalR is here. Me personally, I am not a big fan and for me SocketIO would be sufficient (but, of course, I see why SignalR might make more sense / at least for ASP.NET / Blazor).
There was a problem hiding this comment.
Me personally, I am not a big fan and for me SocketIO would be sufficient
I see it the same.
I'd like to discuss what the future of SignalR is here
Yup, I'll follow up in the main comments stream.
| /** | ||
| * Environment-aware logging utility for Electron.NET | ||
| * | ||
| * Provides structured logging with log levels that respect the environment. | ||
| * Preserves console.time/timeEnd for performance measurements. | ||
| */ | ||
|
|
||
| // Log levels | ||
| const LogLevel = { | ||
| DEBUG: 0, | ||
| INFO: 1, | ||
| WARN: 2, | ||
| ERROR: 3, | ||
| SILENT: 4, | ||
| }; | ||
|
|
||
| /** | ||
| * Detect the current environment based on various indicators | ||
| * @returns {'development'|'production'|'debug'} The detected environment | ||
| */ | ||
| function detectEnvironment() { | ||
| // Check for unpacked/development mode flags | ||
| const args = process.argv.join(" ").toLowerCase(); | ||
| if ( | ||
| args.includes("--unpackeddotnet") || | ||
| args.includes("--unpackedelectron") || | ||
| args.includes("--unpackeddotnetsignalr") | ||
| ) { | ||
| return "development"; | ||
| } | ||
|
|
||
| // Check NODE_ENV | ||
| const nodeEnv = process.env.NODE_ENV?.toLowerCase(); | ||
| if (nodeEnv === "development" || nodeEnv === "dev") { | ||
| return "development"; | ||
| } | ||
|
|
||
| // Check for debugger | ||
| if ( | ||
| process.execArgv.some( | ||
| (arg) => arg.includes("inspect") || arg.includes("debug"), | ||
| ) | ||
| ) { | ||
| return "debug"; | ||
| } | ||
|
|
||
| // Default to production for packaged apps | ||
| return "production"; | ||
| } | ||
|
|
||
| // Determine current environment and default log level | ||
| const environment = detectEnvironment(); | ||
| const defaultLogLevels = { | ||
| debug: LogLevel.DEBUG, | ||
| development: LogLevel.INFO, | ||
| production: LogLevel.WARN, | ||
| }; | ||
|
|
||
| let currentLogLevel = defaultLogLevels[environment]; | ||
|
|
||
| /** | ||
| * Set the current log level | ||
| * @param {number} level - LogLevel enum value | ||
| */ | ||
| function setLogLevel(level) { | ||
| currentLogLevel = level; | ||
| } | ||
|
|
||
| /** | ||
| * Get the current log level | ||
| * @returns {number} Current LogLevel | ||
| */ | ||
| function getLogLevel() { | ||
| return currentLogLevel; | ||
| } | ||
|
|
||
| /** | ||
| * Get the current environment | ||
| * @returns {string} Current environment name | ||
| */ | ||
| function getEnvironment() { | ||
| return environment; | ||
| } | ||
|
|
||
| /** | ||
| * Check if a log level should be output | ||
| * @param {number} level - LogLevel to check | ||
| * @returns {boolean} True if the level should be logged | ||
| */ | ||
| function shouldLog(level) { | ||
| return level >= currentLogLevel; | ||
| } | ||
|
|
||
| /** | ||
| * Safe wrapper for console methods that catches EPIPE errors | ||
| */ | ||
| const safeConsole = { | ||
| log: (...args) => { | ||
| try { | ||
| console.log(...args); | ||
| } catch (e) { | ||
| // Ignore EPIPE errors when console is detached | ||
| } | ||
| }, | ||
| warn: (...args) => { | ||
| try { | ||
| console.warn(...args); | ||
| } catch (e) { | ||
| // Ignore EPIPE errors when console is detached | ||
| } | ||
| }, | ||
| error: (...args) => { | ||
| try { | ||
| console.error(...args); | ||
| } catch (e) { | ||
| // Ignore EPIPE errors when console is detached | ||
| } | ||
| }, | ||
| // Preserve timing functions as-is | ||
| time: (label) => { | ||
| try { | ||
| console.time(label); | ||
| } catch (e) { | ||
| // Ignore EPIPE errors | ||
| } | ||
| }, | ||
| timeEnd: (label) => { | ||
| try { | ||
| console.timeEnd(label); | ||
| } catch (e) { | ||
| // Ignore EPIPE errors | ||
| } | ||
| }, | ||
| }; | ||
|
|
||
| /** | ||
| * Logger with environment-aware log levels | ||
| */ | ||
| const logger = { | ||
| /** | ||
| * Log a debug message (only in debug mode) | ||
| */ | ||
| debug: (...args) => { | ||
| if (shouldLog(LogLevel.DEBUG)) { | ||
| safeConsole.log("[DEBUG]", ...args); | ||
| } | ||
| }, | ||
|
|
||
| /** | ||
| * Log an info message | ||
| */ | ||
| info: (...args) => { | ||
| if (shouldLog(LogLevel.INFO)) { | ||
| safeConsole.log("[INFO]", ...args); | ||
| } | ||
| }, | ||
|
|
||
| /** | ||
| * Log a warning message | ||
| */ | ||
| warn: (...args) => { | ||
| if (shouldLog(LogLevel.WARN)) { | ||
| safeConsole.warn("[WARN]", ...args); | ||
| } | ||
| }, | ||
|
|
||
| /** | ||
| * Log an error message | ||
| */ | ||
| error: (...args) => { | ||
| if (shouldLog(LogLevel.ERROR)) { | ||
| safeConsole.error("[ERROR]", ...args); | ||
| } | ||
| }, | ||
|
|
||
| /** | ||
| * Start a timing measurement (always logged) | ||
| */ | ||
| time: (label) => { | ||
| safeConsole.time(label); | ||
| }, | ||
|
|
||
| /** | ||
| * End a timing measurement (always logged) | ||
| */ | ||
| timeEnd: (label) => { | ||
| safeConsole.timeEnd(label); | ||
| }, | ||
| }; | ||
|
|
||
| module.exports = { | ||
| logger, | ||
| safeConsole, | ||
| LogLevel, | ||
| setLogLevel, | ||
| getLogLevel, | ||
| getEnvironment, | ||
| shouldLog, | ||
| }; |
There was a problem hiding this comment.
This looks unrelated to this PR...no?
Should be separated.
There was a problem hiding this comment.
Yeah I'd like to remove this one. I will keep it for the moment to ease debugging / see what outputs really help but generally we will not introduce it here.
There was a problem hiding this comment.
I can't see where it's used, When there's a purpose, it's fine.
Generally, it would be great when we would be able to surface any errors on the JS side back to the .net API.
There was a problem hiding this comment.
Most errors could be transported but as we communicate via the socket we would not be able to see any errors w.r.t. the socket connection.
| { | ||
| "$schema": "https://raw.githubusercontent.com/electron-userland/electron-builder/refs/heads/master/packages/app-builder-lib/scheme.json", | ||
| "compression": "maximum", | ||
| "linux": { | ||
| "target": [ | ||
| "tar.xz" | ||
| ], | ||
| "executableArgs": [ "--no-sandbox" ], | ||
| "artifactName": "${name}-${arch}-${version}.${ext}" | ||
| }, | ||
| "win": { | ||
| "target": [ | ||
| { | ||
| "target": "portable", | ||
| "arch": "x64" | ||
| } | ||
| ] | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
I know it is missing in the sample. A separate PR (or just a commit to develop) would be good.
There was a problem hiding this comment.
Yeah I will extract that out - just waiting to finish my thoughts for next steps on this one (i.e., how to proceed w.r.t. SignalR) before putting the PR into its final state.
| "start": "tsc -p ." | ||
| }, | ||
| "dependencies": { | ||
| "@microsoft/signalr": "^8.0.7", |
There was a problem hiding this comment.
Non-Blazor apps should not get this package
I share those feelings. I understand the desire of having a kind of auth pattern for socket communication. Can you lay out the advantages that using SignalR would provide besides the authentication - which I presume would also be doable via SocketIO? |
Personally, I don't see any. From my original conversation (via mail) with the original authors (or vibe coders if you prefer that label) of the PR they responded to my rejection of SignalR like this:
As the connection would anyway not be used for the Blazor app itself I think just having SocketIO would be better. Maybe we can still keep the facade / interface (but rename it as you suggested). Generally, I think the mechanism itself (just start with some random token - make sure that both sides know about it - once agreed use it for protecting the line) can be done with SocketIO, too. The mechanism of having (at build-time as you suggested) select the mode for the published app is also beneficial. This way, one can decide what side should be the carrier. To simplify things we could allow that token mechanism to only work if .NET is the initial runtime. |
I can't quite follow. I mean, the "auth" part needs to be implemented either here or there, it's the same in both cases. Of course I (and probably 'we') are biased by not using SignalR ourselves, and we might think differently if we would. But at the end it's still us, being the ones who will have to support it, going forward. And that's my primary pain point about it - when someone comes and reports an issue about it, what should we do? Become SignalR experts? Or just shrug our shoulders? If the PR would have at least been submitted by a SignalR expert who stands behind it, knows all about it, using it in their own product and would be available for assisting here with issues about it for a foreseeable amount of time - that would be a lot more convincing than the situation we're facing here. I'm not fully decided, but I'm leaning towards being egoistic, rather not taking the burden and additional technical debt. I mean - why should we? |
|
The code I submitted to Florian may give the impression that it comes from one of these too many, too enthusiastic vibe-coders. I believe I don't belong to that crowd, even if I totally agree with @softworkz initial evaluation: the submitted code stinks and was by no means intended to be submitted as a PR, without heavy rework/rewrite/checking/formatting/reviewing. It was more a proof of concept of the ideas I tried to express, and a means to verify that the architecture would work. That's why I sent it to Florian first, so that he can tidy things up (I'd love to do that myself but I have no available time, so we are subcontracting the work). I am awfully sorry if the whole experiment gave the impression that I was being rude and didn't give a s**t about the practices which are in place for ElectronNET. More so with my very long past as an Open Source contributor... I understand your reluctance to integrate SignalR into the equation (Florian had the same reaction). This adds one more moving piece to the system, and it is higher level than plain websockets (or socketio for that matter). However, apps which use a Blazor Server backend already have everything in place: SignalR is what drives Blazor's communication between the ASP.NET Core layer and the browser. My intent is very clear: I need to get rid of the port used for So SignalR elegantly solves our needs. And I will stand behind the choice. And invest what is required to have SignalR supported in the long run. |
I'm also contributing to FFmpeg (member of the GA) and when you do similar there, you may get responses where you might just want to turn around and never come back :-) Apologies accepted though.
Let me be honest with you: My intent and motivation has always been very clear as well. I'm not a hobbyist developer who is contributing to an open-source project for fun. I made that contribution for Electron.NET Core out of strategical needs. For once, I needed to be able to debug with little delay and to work and debug a Linux application from within Visual Studio on Windows. Also Electron.NET was about to die silently and slowly and it needed some boost in order to have a future (luckily, the changes are able to achieve all at once) - because nobody wants to go all in for a dead horse. At the bottom line, there's no reason for me to deal with SignalR support in Electron.NET - it makes everything just more difficult and I need to take care that there won't be any changes that would regress our usage of it - so for me it's better when this doesn't get into Electron.NET. So,, as far as I'm concerned, I'm strongly leaning towards the "no"-side regarding SignalR integration. |
3 participants
This PR was done by third-party developers; I am just here to provide their changes for review.
The basic idea is outlined in the Electron.NET SignalR Authentication Guide.
In here, the system makes sure to use exclusively respond to a request when a cookie based on an initial auth token was provided. This way, even though the application behind is open to all users of the same machine, only the "correct" one is answering.
The background for this is that sometimes the desktop application is used by connecting to a remote machine effectively creating a user session on the machine. If this is done multiple times by different users then the same application is launched multiple times on this machine. In general, multi-instance mode works fine, but has of course some limitations such as requiring dedicated ports for each running app instance.