feat(utils): support multiple certificates in resolveServerUrls #20707
Conversation
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
|
Sorry for the confusion in the test cases. Any feedback on the certificate or implementation setup would be welcome. |
sapphi-red
added
the
p2-nice-to-have
|
@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. |
monam2
force-pushed
the
fix/multiple-certs-support
branch
from
b4b27f9 to
6ef0e81
Compare
| 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.
4 participants
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.