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.
|
Sorry, but this is insane. I have no time to review this. |
There hasn't even any effort been made to make this reviewable. This is full of formatting, eol and other stray changes. |
I don't agree on this assessment, but I agree that there are indeed some changes beyond the PR goal. I will try to merge those or remove them to clean this up. |
src/ElectronNET.AspNet/Runtime/Services/AspNetLifetimeAdapter.cs
Outdated
Show resolved
Hide resolved
src/ElectronNET.AspNet/Runtime/Services/AspNetLifetimeAdapter.cs
Outdated
Show resolved
Hide resolved
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
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.