Skip to content

fix: complete untrusted certificate support#1308

Merged
ShGKme merged 5 commits intomainfrom
fix/cert
May 28, 2025
Merged

fix: complete untrusted certificate support#1308
ShGKme merged 5 commits intomainfrom
fix/cert

Conversation

@ShGKme
Copy link
Contributor

@ShGKme ShGKme commented May 23, 2025 •
edited
Loading

☑️ Resolves

  1. Implement certificateTrustDialog as a custom window
    • 🌐 Supports l10n
    • ✨ Looks better
  2. Check certificate in session.defaultSession.setCertificateVerifyProc
    • 🤦 app.on('certificate-error' is only for web-view loading...
  3. Always use custom showCertificateTrustDialog instead of dialogs.showCertificateTrustDialog
    • 🤦 That dialog is for system-level certificate management...
    • We always need application level acceptance, like on Linux

🖼️ Screenshots

Initial Advanced Certificate
image image image

@ShGKme ShGKme added this to the 🪴 Next Release milestone May 23, 2025
@ShGKme ShGKme self-assigned this May 23, 2025
@ShGKme ShGKme added bug Something isn't working 2. developing labels May 23, 2025
@ShGKme ShGKme force-pushed the fix/cert branch 2 times, most recently from 9e9fd5b to 7c95a86 Compare May 24, 2025 07:38
@ShGKme ShGKme marked this pull request as ready for review May 24, 2025 07:38
Antreesy
Antreesy reviewed May 24, 2025
View reviewed changes
src/certificate/renderer/CertificateApp.vue
margin-block: 0;
font-size: 1.5em;
text-align: center;
text-transform: capitalize;
Copy link
Contributor

@Antreesy Antreesy May 24, 2025

Choose a reason for hiding this comment

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

What's the idea behind capitalize?

Copy link
Contributor Author

@ShGKme ShGKme May 24, 2025

Choose a reason for hiding this comment

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

Looks better for heading/titles 👀

Antreesy
Antreesy approved these changes May 26, 2025
View reviewed changes
Copy link
Contributor

@Antreesy Antreesy left a comment

Choose a reason for hiding this comment

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

Tested on Windows with local setup + mkcert

image
login window doesn't detect approved certificate, would be nice to reset error / proceed further without have to click again

@ShGKme
Copy link
Contributor Author

ShGKme commented May 26, 2025

login window doesn't detect approved certificate, would be nice to reset error / proceed further without have to click again

Was it enough to just click again, or you had to restart?

@Antreesy
Copy link
Contributor

Antreesy commented May 26, 2025 •
edited
Loading

Was it enough to just click again

Was enough, but issue doesn't look resolved from UI perspective

@nimishavijay

This comment was marked as resolved.

Sign in to view
@ShGKme

This comment was marked as resolved.

Sign in to view
@ShGKme

This comment was marked as resolved.

Sign in to view
@ShGKme

This comment was marked as outdated.

Sign in to view
@ShGKme

This comment was marked as outdated.

Sign in to view
@ShGKme
Copy link
Contributor Author

ShGKme commented May 26, 2025 •
edited
Loading

Was enough, but issue doesn't look resolved from UI perspective

For self-signed it was even worse - if you don't accept in 30s, it is considered rejected until restart.

An Electron bug. Added a workaround...

@ShGKme
Copy link
Contributor Author

ShGKme commented May 28, 2025

  • Updated design according to design review (screenshots updated)
  • Fixed an issue prompting verification of a valid certificate
  • Minor refator will rebase

Antreesy
Antreesy approved these changes May 28, 2025
View reviewed changes
Copy link
Contributor

@Antreesy Antreesy left a comment

Choose a reason for hiding this comment

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

Tested

nimishavijay
nimishavijay approved these changes May 28, 2025
View reviewed changes
Copy link
Member

@nimishavijay nimishavijay left a comment

Choose a reason for hiding this comment

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

Nice!

ShGKme added 3 commits May 28, 2025 16:56
@ShGKme
chore(deps): add vue-frag
b46665b 
Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
@ShGKme
fix(certificate): improve custom showCertificateTrust dialog
0fc3a4f 
Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
@ShGKme
fix(certificate): untrusted cert prompt not shown for requests
f30fdda 
Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
ShGKme added 2 commits May 28, 2025 17:04
@ShGKme
fix(certificate): untrusted cert prompt not shown on macOS and Windows
bfaa5ed 
Use app-level certificateTrustDialog on all OS, not only on Linux

Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
@ShGKme
fix(certificate): Electron rejects cert 30s timeout during user prompt
86b0e7f 
Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
@ShGKme
Copy link
Contributor Author

ShGKme commented May 28, 2025

  • Rebased onto main
  • Squashed fixup commits and redesigns

@ShGKme ShGKme enabled auto-merge May 28, 2025 15:17
@ShGKme ShGKme merged commit 31a4d30 into main May 28, 2025
11 checks passed
@ShGKme ShGKme deleted the fix/cert branch May 28, 2025 15:17
@klazzyblik
Copy link

klazzyblik commented May 28, 2025

Tested on Windows with local setup + mkcert

image login window doesn't detect approved certificate, would be nice to reset error / proceed further without have to click again

I'm getting this exact error and clicking log in again will not resolve, i'm stuck here
Screenshot 2025-05-28 at 4 00 25 PM

@ShGKme
Copy link
Contributor Author

ShGKme commented May 29, 2025

I'm getting this exact error and clicking log in again will not resolve, i'm stuck here

Which version do you use? Did you build the app locally? If you did, please, share the application logs (both renderer and main processes).

If you test the latest released version — it does not contain this pull request. It was released weeks ago, while the PR was merged 16 hours ago.

@ShGKme ShGKme mentioned this pull request May 30, 2025
Merged
@ShGKme
Copy link
Contributor Author

ShGKme commented Jun 2, 2025

@klazzyblik If you still have the issue in v1.2.2-beta, please, create an issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nimishavijay nimishavijay nimishavijay approved these changes

@Antreesy Antreesy Antreesy approved these changes

@nickvergessen nickvergessen Awaiting requested review from nickvergessen

Assignees

@ShGKme ShGKme

Labels

bug Something isn't working

Projects

None yet

Milestone

v1.2.2-beta

4 participants

@ShGKme @Antreesy @nimishavijay @klazzyblik