feat(utils): support multiple certificates in resolveServerUrls#20707
feat(utils): support multiple certificates in resolveServerUrls#20707sapphi-red merged 8 commits intovitejs:mainfrom
Conversation
|
Sorry for the confusion in the test cases. Any feedback on the certificate or implementation setup would be welcome. |
|
@sapphi-red Thanks for the suggestion — I’ve updated the code accordingly. I extracted the logic into a dedicated helper function and also renamed a few variables for clarity. PTAL. |
|
Would you update the tests to use the new function you extracted? |
|
@sapphi-red Added cases for both single and multiple certificates. |
b4b27f9 to
6ef0e81
Compare
EmperorArthur
left a comment
There was a problem hiding this comment.
I really appreciate you taking the time to work on this.
Here are a few small things that you might want to consider tweaking. It's up to you and the maintainers of course, but thought I could try to contribute a little bit.
| try { | ||
| return new crypto.X509Certificate(cert) | ||
| } catch { | ||
| return null |
There was a problem hiding this comment.
Should we log a warning instead of completely ignoring an invalid cert?
There was a problem hiding this comment.
I assume the error would be shown when starting the server. Since this is an optional feature, I think it's fine to ignore the error here.
| } | ||
| }) | ||
| .flatMap((cert) => | ||
| cert?.subjectAltName |
There was a problem hiding this comment.
For whatever reason this took me a minute to parse.
Have you considered moving from the ternary .filter((cert) => Boolean(cert?.subjectAltName))?
Quick test to show that should cover all the cases:
let c = [{subjectAltName: 'asdf'}, {}, null, {subjectAltName: null}, {subjectAltName: ''}];
c.filter((cert) => Boolean(cert?.subjectAltName)).length === 1
There was a problem hiding this comment.
TypeScript cannot infer the types with
.filter((cert) => Boolean(cert?.subjectAltName))
.flatMap((cert) => extractHostnamesFromSubjectAltName(cert.subjectAltName)) // cert may be nullso I think it's better to keep it as-is.
Description
Fixes #20681
Added support for multiple certificates in the resolveServerUrls function. Previously, only single certificates were supported, limiting HTTPS server configuration options.
All tests pass (unit, integration, and build tests all pass locally)
Test Coverage Added
Note: Some test parameters use
anytypes due to complex interface typing challenges. Type improvements are welcome.