-
-
Notifications
You must be signed in to change notification settings - Fork 7.4k
feat(utils): support multiple certificates in resolveServerUrls #20707
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
28a02ca
6611611
9effbca
2353f2e
758b481
8269330
1d5618a
6ef0e81
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -993,6 +993,29 @@ export async function resolveHostname( | |
return { host, name } | ||
} | ||
|
||
export function extractHostnamesFromCerts( | ||
certs: HttpsServerOptions['cert'] | undefined, | ||
): string[] { | ||
const certList = certs ? arraify(certs) : [] | ||
if (certList.length === 0) return [] | ||
|
||
const hostnames = certList | ||
.map((cert) => { | ||
try { | ||
return new crypto.X509Certificate(cert) | ||
} catch { | ||
return null | ||
} | ||
}) | ||
.flatMap((cert) => | ||
cert?.subjectAltName | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For whatever reason this took me a minute to parse. Have you considered moving from the ternary Quick test to show that should cover all the cases:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TypeScript cannot infer the types with .filter((cert) => Boolean(cert?.subjectAltName))
.flatMap((cert) => extractHostnamesFromSubjectAltName(cert.subjectAltName)) // cert may be null so I think it's better to keep it as-is. |
||
? extractHostnamesFromSubjectAltName(cert.subjectAltName) | ||
: [], | ||
) | ||
|
||
return unique(hostnames) | ||
} | ||
|
||
export function resolveServerUrls( | ||
server: Server, | ||
options: CommonServerOptions, | ||
|
@@ -1045,19 +1068,12 @@ export function resolveServerUrls( | |
}) | ||
} | ||
|
||
const cert = | ||
httpsOptions?.cert && !Array.isArray(httpsOptions.cert) | ||
? new crypto.X509Certificate(httpsOptions.cert) | ||
: undefined | ||
const hostnameFromCert = cert?.subjectAltName | ||
? extractHostnamesFromSubjectAltName(cert.subjectAltName) | ||
: [] | ||
|
||
if (hostnameFromCert.length > 0) { | ||
const hostnamesFromCert = extractHostnamesFromCerts(httpsOptions?.cert) | ||
if (hostnamesFromCert.length > 0) { | ||
const existings = new Set([...local, ...network]) | ||
local.push( | ||
...hostnameFromCert | ||
.map((hostname) => `https://${hostname}:${port}${base}`) | ||
...hostnamesFromCert | ||
.map((hostname) => `${protocol}://${hostname}:${port}${base}`) | ||
.filter((url) => !existings.has(url)), | ||
) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we log a warning instead of completely ignoring an invalid cert?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.