Automatic Port Selection and Secured Communication#1038
Automatic Port Selection and Secured Communication#1038FlorianRappl wants to merge 3 commits intodevelopfrom
Conversation
…into feature/socket-enhancements
pr-comment: Run #60
🎉 All tests passed!Github Test Reporter by CTRF 💚 🔄 This comment has been updated |
|
Should we move this forward @softworkz ? |
|
Sorry, haven't come to this yet. Why is there a merge conflict? |
There is none. There was initially one as planned due to the splitting. |
|
|
Shows different for me: 
Also: 
Furthermore: |
softworkz
left a comment
softworkz
left a comment
There was a problem hiding this comment.
I'm very sorry for responding so late!
| this.process.LineReceived += Read_SocketIO_Parameters; | ||
| this.process.Run(startCmd, args, directoriy); | ||
|
|
||
| await Task.Delay(500.ms()).ConfigureAwait(false); | ||
| await tcs.Task.ConfigureAwait(false); |
There was a problem hiding this comment.
If electron errors and doesn't print that line, the task will hang forever.
Whether process will still exit, I'm not sure, but it should keep hanging their either case.
There was a problem hiding this comment.
It should hang there as we can't communicate.
What we can talk about is a barrier here (e.g., 10 seconds) - after that the process would terminate. I would, however, not make it too small. Reason for not including that right now is that I am not sure how viable it is - and choosing a number that is too low might create more problems than solve here.
There was a problem hiding this comment.
Just register the exit event to try-cancel the tcs in addition to a comfortable timeout
| if (authToken && socket.request.headers.authorization !== authToken) { | ||
| console.warn('Electron Socket authentication failed!'); | ||
| socket.disconnect(true); | ||
| return; | ||
| } |
There was a problem hiding this comment.
This makes it look like the auth token would be optional, but it isn't
(yet it should be)
There was a problem hiding this comment.
I would not make it optional - its part of the standard flow. There is no reason to have this optional.
| var match = extractor.Match(line); | ||
|
|
||
| if (match?.Success ?? false) | ||
| { | ||
| var port = int.Parse(match.Groups[1].Value); | ||
| var token = match.Groups[2].Value; | ||
|
|
||
| this.process.LineReceived -= Read_SocketIO_Parameters; | ||
| ElectronNetRuntime.ElectronAuthToken = token; | ||
| ElectronNetRuntime.ElectronSocketPort = port; | ||
| tcs.SetResult(); | ||
| } | ||
| } |
There was a problem hiding this comment.
This class is always providing the electronforcedport parameter, in that case it's pointless to monitor the output for a port number.
There was a problem hiding this comment.
I disagree. We should always monitor here. Right now the forcedport is like a recommendation - it can be also "0" / or later on use a different port in case the recommended one was blocked.
I would always follow the same logic here - makes it easier to debug and reason.
| var tcs = new TaskCompletionSource(); | ||
|
|
||
| void Read_SocketIO_Parameters(object sender, string line) | ||
| { | ||
| await Task.Delay(10.ms()).ConfigureAwait(false); | ||
| // Look for "Electron Socket: listening on port %s at ..." |
There was a problem hiding this comment.
There should be a path completely without reading the process output.
(without auth token)
There was a problem hiding this comment.
I would not do that. It's less secure and just more complicated. Let's keep it to the point.
There was a problem hiding this comment.
I do not want to have that in our application that dotnet is relying on the console output of Electron.
There was a problem hiding this comment.
I have nothing against making the auth token mandatory, but it can be provided by the dotnet side as cli param, so it doesn't need to be read it from the console output. (that's no more and no less secure)
Co-authored-by: softworkz <4985349+softworkz@users.noreply.github.com>
2 participants
Follow up for #1021 - should be reviewed / merged after #1037.
Introduces an authentication token to be used in the communication between Node.js (Electron) and .NET. The same token can be used to also guard an ASP.NET Core server - allowing only access from within the boundaries of the running application.
The port is now OS-selected - guaranteeing a free accessible port. No tools to find free ports are necessary.
The owning part (e.g., Node.js for the WebSocket connection to control the Electron application) also owns / opens the port - independent of the used startup mode.