Skip to content

Remove Dev Proxy CA certificate when uninstalling for Win#1208

Merged
waldekmastykarz merged 23 commits into
dotnet:mainfrom
bartizan:847_uninstall-root-certificate-win
Jun 13, 2025
Merged

Remove Dev Proxy CA certificate when uninstalling for Win#1208
waldekmastykarz merged 23 commits into
dotnet:mainfrom
bartizan:847_uninstall-root-certificate-win

Conversation

@bartizan

Copy link
Copy Markdown
Contributor

@bartizan bartizan marked this pull request as ready for review May 23, 2025 14:07
@bartizan bartizan requested a review from a team as a code owner May 23, 2025 14:07
@waldekmastykarz

Copy link
Copy Markdown
Collaborator

Cool! We'll check it out asap. Thank you!

@waldekmastykarz waldekmastykarz left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of things that I noticed. I'll try to run it asap to see it in action. I like the idea!

Comment thread install-beta.iss Outdated
Comment thread install-beta.iss Outdated
@bartizan bartizan marked this pull request as draft May 23, 2025 18:23
@bartizan bartizan marked this pull request as ready for review May 24, 2025 06:02
@bartizan bartizan requested a review from waldekmastykarz May 24, 2025 06:02
@waldekmastykarz waldekmastykarz requested a review from Copilot May 26, 2025 17:36

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds an automatic step to remove the Dev Proxy CA certificate when uninstalling on Windows and exposes the same functionality via the CLI.

  • Introduces a DevProxyExecutable macro and an [UninstallRun] entry in both install.iss and install-beta.iss to invoke devproxy.exe cert remove during uninstall.
  • Registers a new remove subcommand in the CLI (CreateCertRemoveCommand) and implements its handler (CertRemoveCommandHandler.RemoveCert).

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
install.iss Defines DevProxyExecutable and adds [UninstallRun] to remove the cert on uninstall
install-beta.iss Same changes as install.iss, targeting the beta executable
dev-proxy/ProxyHost.cs Adds CreateCertRemoveCommand to the list of certificate commands
dev-proxy/CommandHandlers/CertRemoveCommandHandler.cs Implements the logic to remove the trusted root certificate
Comments suppressed due to low confidence (3)

install.iss:50

  • [nitpick] The RunOnceId "RemoveCert" is quite generic and may collide with other actions; consider namespacing it (e.g. "MyApp_RemoveCert") to avoid potential conflicts.
Filename: "{app}\{#DevProxyExecutable}"; Parameters: "cert remove"; RunOnceId: "RemoveCert"; Flags: runhidden;

dev-proxy/CommandHandlers/CertRemoveCommandHandler.cs:9

  • The new RemoveCert handler isn’t covered by any unit tests. Consider adding tests to assert successful removal and proper error logging in failure scenarios.
public static void RemoveCert(ILogger logger)

dev-proxy/CommandHandlers/CertRemoveCommandHandler.cs:5

  • This file references ILogger but doesn’t include the corresponding using directive. Add using Microsoft.Extensions.Logging; (or the appropriate namespace) at the top to resolve the type.
namespace DevProxy.CommandHandlers;

@waldekmastykarz waldekmastykarz self-assigned this May 30, 2025

@waldekmastykarz waldekmastykarz left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works nicely! Let's do a few small fixes before we merge it.

Comment thread dev-proxy/ProxyHost.cs Outdated
Comment thread dev-proxy/CommandHandlers/CertRemoveCommandHandler.cs Outdated
@bartizan bartizan marked this pull request as ready for review June 4, 2025 05:54

@waldekmastykarz waldekmastykarz left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice and clean implementation for the prompt. Let's add the --force switch to the installer invocation so that it executes silently and we'll get it in

Comment thread install-beta.iss Outdated
Comment thread install.iss Outdated
@bartizan

bartizan commented Jun 4, 2025

Copy link
Copy Markdown
Contributor Author

@waldekmastykarz
I come to think how a prompt confirmation Y/n should be completed. It could be either by pressing Enter key or just single key (like 'y'or 'n'). It appears this PR implements key + Enter but trust-cert.sh we run to install a certificate uses single key.
In favor of consistency both have to be the same (I see one more fix comming). But which approach to choose?

The single key is faster, but Key+Enter method sort of safer, gives a better user control and seems to provide adherence to common CLI conventions. So I'd stick with key+enter. What do you think?

PS I am working on MacOS related part of the issue.

@bartizan bartizan marked this pull request as draft June 4, 2025 19:39
@waldekmastykarz

Copy link
Copy Markdown
Collaborator

Good point, thank you for bringing it up. Let's do y/n followed by ENTER. I'll create a separate issue for this.

@bartizan bartizan marked this pull request as ready for review June 6, 2025 11:52
@bartizan bartizan marked this pull request as draft June 11, 2025 07:40
@bartizan bartizan marked this pull request as ready for review June 11, 2025 08:58
@waldekmastykarz

Copy link
Copy Markdown
Collaborator

Hey @bartizan, is this PR ready for review or is there anything else that we need to do?

@bartizan

Copy link
Copy Markdown
Contributor Author

Hey @bartizan, is this PR ready for review or is there anything else that we need to do?

it is ready for review.
Now it should complete the whole issue for Win and Mac.

@waldekmastykarz waldekmastykarz left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nicely done and I like the refactoring of the HasRunFlag to a class with additional functionality. Let's iron out a few wrinkles and we'll get it in. 👏

Comment thread DevProxy/Proxy/ProxyEngine.cs Outdated
Comment thread DevProxy/Proxy/ProxyEngine.cs Outdated
Comment thread DevProxy/Proxy/ProxyEngine.cs Outdated
Comment thread DevProxy/Commands/CertCommand.cs Outdated
Comment thread DevProxy/Commands/CertCommand.cs Outdated
Comment thread DevProxy/Proxy/ProxyEngine.cs Outdated
@waldekmastykarz waldekmastykarz marked this pull request as draft June 12, 2025 08:46
@waldekmastykarz

Copy link
Copy Markdown
Collaborator

Marking as draft. Please mark as "Ready for review" when you've done the changes @bartizan. Thanks!

@bartizan bartizan marked this pull request as ready for review June 12, 2025 14:29

@waldekmastykarz waldekmastykarz left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome!

@waldekmastykarz waldekmastykarz merged commit 1232c7f into dotnet:main Jun 13, 2025
4 checks passed
@bartizan bartizan deleted the 847_uninstall-root-certificate-win branch June 13, 2025 06:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants