Refactor electron window setCertificateVerifyProc #7185
Conversation
| export enum ChromiumNetError { | ||
| SUCCESS = 0, | ||
| FAILURE = -2, | ||
| RESULT_FROM_CHROMIUM = -3, | ||
| } |
There was a problem hiding this comment.
I wonder if there's an official enum for this from eg. electron or chromium itself.
There was a problem hiding this comment.
I was wondering the same, feels bad to include error codes here from chromium... but I could not find anything.
| const lensProxyCertificate = di.inject(lensProxyCertificateInjectable).get(); | ||
| const lensProxyX509Cert = new X509Certificate(lensProxyCertificate.cert); | ||
|
|
||
| return (request: Request, shouldBeTrusted: CertificateVerificationCallback) => { |
There was a problem hiding this comment.
This function could be simpler by being named requestCanBeTrustedInjectable, and returning boolean instead of bleeding abstraction of ChromiumNetError. That would mean that call to shouldBeTrusted happens somewhere else, and it would internally translate said boolean to appropriate ChromiumNetError.
The resulting code could be dead-simple such as:
const thereIsTrust = requestCanBeTrusted(request);
shouldBeTrusted(thereIsTrust);
There was a problem hiding this comment.
I disagree. boolean is not a good fit because the return type either YES or MAYBE.
| browserWindow.webContents.session.setCertificateVerifyProc((request, shouldBeTrusted) => { | ||
| const { certificate } = request; | ||
| const cert = new X509Certificate(certificate.data); | ||
| const shouldTrustCert = cert.raw.length === lensProxyX509Cert.raw.length | ||
| && timingSafeEqual(cert.raw, lensProxyX509Cert.raw); | ||
|
|
||
| shouldBeTrusted(shouldTrustCert ? ChromiumNetError.SUCCESS : ChromiumNetError.RESULT_FROM_CHROMIUM); | ||
| }); | ||
| browserWindow.webContents.session.setCertificateVerifyProc(sessionCertificateVerifier); |
There was a problem hiding this comment.
Responsibility extracted, nice :)
I spent a moment thinking if this responsibility of "certificate verification" should be abstracted to eg. createLensWindowInjectable, and to have no knowledge of Electron. But the implementation seems to be very Electron-specific, making this the correct place.
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
Nokel81
force-pushed
the
refactor-session-certificate-verify-proc
branch
from
d055f5a to
8134ad4
Compare
|
Conflicts have been resolved. A maintainer will review the pull request shortly. |
Nokel81
force-pushed
the
refactor-session-certificate-verify-proc
branch
from
8134ad4 to
80d5b9c
Compare
Signed-off-by: Jari Kolehmainen <jari.kolehmainen@gmail.com>
Signed-off-by: Jari Kolehmainen <jari.kolehmainen@gmail.com>
Signed-off-by: Sebastian Malton <sebastian@malton.name>
Nokel81
force-pushed
the
refactor-session-certificate-verify-proc
branch
from
80d5b9c to
553ba40
Compare
Follow-up to #7180