fix: add SNI servername for TLS connection from host field#2105
fix: add SNI servername for TLS connection from host field#2105dougEfresh wants to merge 3 commits into
Conversation
…licitly set This allows TLS terminating proxies to determine which cert to serve
|
Hi, I’m Jit, a friendly security platform designed to help developers build secure applications from day zero with an MVS (Minimal viable security) mindset. In case there are security findings, they will be communicated to you as a comment inside the PR. Hope you’ll enjoy using Jit. Questions? Comments? Want to learn more? Get in touch with us. |
🛡️ Jit Security Scan Results
✅ No security findings were detected in this PR
Security scan by Jit
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 461c4cc. Configure here.
|
@dougEfresh Thanks for the contribution. Could you also add a test that reproduces this issue and verifies that the solution works as expected? |
|
@PavelPashov I added a couple of tests |
PavelPashov
left a comment
PavelPashov
left a comment
There was a problem hiding this comment.
Approving this as-is. As a small follow-up, it may be worth applying the same servername fallback in the Sentinel connector as well, so TLS behavior stays consistent across connection modes.
| if (options.tls) { | ||
| Object.assign(connectionOptions, options.tls); | ||
| if ("host" in connectionOptions && !("servername" in connectionOptions)) { | ||
| const host = (connectionOptions as TcpOptions).host; | ||
| if (host && !isIP(host)) { | ||
| (connectionOptions as any).servername = host; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
I think adding the tls.servername even when tls is undefined would be a good improvement.
In my case, where I'm using only the tls.servername, I would need to set an empty tls: {} to trigger this servername logic, which would be kinda weird.
I believe setting the servername for non-TLS connections isn't a breaking change, right?
3 participants
If
servernamefield is not explicitly set then usehostfield for SNI identification.This allows TLS terminating proxies to determine which cert to serve.
Note
Low Risk
Small, localized change to connection option construction plus targeted tests; behavior only changes when TLS is used without an explicit
servername.Overview
StandaloneConnectornow auto-populates TLSservernamefrom the configuredhostwhen TLS is enabled andservernameisn’t explicitly provided, while skipping IP hosts to avoid invalid SNI.Unit tests were extended to cover the new SNI defaulting behavior for hostname vs IP
hostvalues.Reviewed by Cursor Bugbot for commit 61db0c1. Bugbot is set up for automated code reviews on this repo. Configure here.