Implement buttons in VNet diag report #52346
Conversation
ravicious
added
no-changelog
ravicious
force-pushed
the
r7s/diag-buttons
branch
from
b25f6e0 to
e23d168
Compare
There was a problem hiding this comment.
I added a snapshot test for this as I feel it's one of those cases where snapshot tests are actually useful. Matter of fact, I already benefited from it – I was doing a small refactoring of reportToText and the snapshot test guarantees that the output stays the same.
| | Conflicting destination | VNet destination | Interface | Set up by | | ||
| | ----------------------- | ---------------- | --------- | --------- | | ||
| | 100.64.0.0/10 | 100.64.0.1 | utun5 | VPN: Foobar | | ||
| | 0.0.0.0/1 | 100.64.0.0/10 | utun6 | unknown | |
There was a problem hiding this comment.
I don't really want to spend time on making the whole table line up perfectly as that's not the point. I sparingly used Markdown in the report so that the output is rendered correctly when pasted into Zendesk and Slack. Slack doesn't support tables, but Zendesk does.
CLI command output looks particularly bad without code blocks, as in that case most Markdown parsers (or I guess all of them?) do not interpret a single newline character as the start of a new line. So the output from netstat ends up all garbled if code blocks are not used.
There was a problem hiding this comment.
Hmm, I should probably document that it's supposed to be Zendesk/Slack friendly. Edit: Done.
There was a problem hiding this comment.
Great that you thought of making it Zendesk/Slack friendly.
| @@ -103,6 +103,9 @@ export default function createMainProcessClient(): MainProcessClient { | |||
| showFileSaveDialog(filePath: string) { | |||
| return ipcRenderer.invoke('main-process-show-file-save-dialog', filePath); | |||
| }, | |||
| saveTextToFile(args) { | |||
There was a problem hiding this comment.
There's already showFileSaveDialog which is used for SSH file transfers. Compared to saveTextToFile, showFileSaveDialog merely shows the dialog and returns the path chosen by the user.
If I wanted to use this for saving the report, I'd have to implement an IPC API that would let the renderer save arbitrary text to an arbitrary file on the disk, which seems bad. It seems much simpler to just not let the path chosen by the user escape the main process. This way an attacker cannot abuse this through an XSS to silently write to an arbitrary file. They'd have to show a prompt first.
Mind you, I think the renderer already has this ability but in a much smaller scope, as it can do so file transfers. So technically an XSS in Connect would allow the attacker to write to an arbitrary file on disk, as long as they first store whatever text they want to save in a file on an SSH server. 🫠
ravicious
force-pushed
the
r7s/diag-buttons
branch
from
e23d168 to
2bc35d6
Compare
This was implemented back when we were thinking of showing recent VNet connections in the panel. We've never had time to implement this, so it's better to just remove this rather keeping this component in the project.
JSDocs for individual methods should be defined on the type, so that the JSDocs are available in any place that uses the type, rather than only where we use the implementation of the type.
ravicious
force-pushed
the
r7s/diag-buttons
branch
from
2bc35d6 to
a23f460
Compare
ravicious
force-pushed
the
r7s/diag-buttons-base
branch
from
a441ab7 to
2a6a20f
Compare
| Created at (local): ${localCreatedAt} | ||
| Created at (UTC): ${utcCreatedAt} |
There was a problem hiding this comment.
I'd make it less verbose and combine into one line: Created at: 2025-02-21 10:28:58 (Fri, 21 Feb 2025 09:28:58 GMT)
| | Conflicting destination | VNet destination | Interface | Set up by | | ||
| | ----------------------- | ---------------- | --------- | --------- | | ||
| | 100.64.0.0/10 | 100.64.0.1 | utun5 | VPN: Foobar | | ||
| | 0.0.0.0/1 | 100.64.0.0/10 | utun6 | unknown | |
There was a problem hiding this comment.
Great that you thought of making it Zendesk/Slack friendly.
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
|
@ravicious See the table below for backport results.
|
This PR provides the implementation for the three buttons in the top right of a VNet diag report.