#4471 Honour unchecked Verify TLS Certificate

[MickeyShnaiderman-RecoLabs]

What

Unchecked Verify TLS Certificate still verified the server cert. getTLSConfig passed verifyServerCert through to Node as rejectUnauthorized, and Node treats null/undefined as verify on. That is the API default (false) and what older rows / test-connection payloads send when the checkbox is off, so self-signed Redis (no CA pasted) failed with unable to verify the first certificate.

Both the ioredis and node-redis strategies now set rejectUnauthorized only when verifyServerCert === true.

Testing

• Unit tests on both strategies: true → reject; false / undefined / null → do not reject.
• npm test -- src/modules/redis/connection/ioredis.redis.connection.strategy.spec.ts src/modules/redis/connection/node.redis.connection.strategy.spec.ts (Node 24.16.0) — 32 passed.
• Manually connected RedisInsight 3.8.0 (local package) to a Redis Cluster that presents a self-signed leaf, TLS on, Verify off, no CA — connection succeeds.

---

Refs #4471

Made with Cursor

[claude]

Thanks for this — the diagnosis is right and the tests are the good kind. On 1cb0be6 the two spec files give 32 passing tests, and the new null / undefined cases fail without the one-line change, so they genuinely pin the bug.

The part that needs addressing before merge is the other half of the semantics change. rejectUnauthorized: database.verifyServerCert meant absent → verify; === true means absent → do not verify. The UI checkbox always sends a boolean, so that path is fine, but three first-party paths create tls: true databases and never set verifyServerCert. They verify against the system CA store today and would silently stop verifying after this PR. Patches below are against 1cb0be6.

1. Azure autodiscovery

redisinsight/api/src/modules/azure/autodiscovery/azure-autodiscovery.service.ts

Both connection-detail builders hard-code tls: true (~L455 Entra ID, ~L500 access key), and the databaseService.create() payload never mentions verifyServerCert. Azure Cache endpoints present a publicly trusted certificate, and the Azure manual connection page already defaults the flag to true (ui/src/pages/autodiscover-azure/azure-manual-connection/AzureManualConnectionPage.tsx L67), so autodiscovery should say the same thing explicitly:

@@ -627,6 +627,9 @@ export class AzureAutodiscoveryService {
             username: connectionDetails.username,
             password: connectionDetails.password,
             tls: connectionDetails.tls,
+            // Azure Cache endpoints present a publicly trusted certificate,
+            // and an absent value now means "do not verify".
+            verifyServerCert: true,
             provider,
             providerDetails,
           });

2. Database import

redisinsight/api/src/modules/database-import/database-import.service.ts

fieldsMapSchema has no verifyServerCert entry at all, so the flag can never be read out of an imported file — not even out of a RedisInsight export. (ImportDatabaseDto already picks verifyServerCert, so nothing else is needed for it to survive the whitelist: true validation.)

@@ -58,4 +58,5 @@ export class DatabaseImportService {
     ['connectionType', ['connectionType']],
     ['tls', ['tls', 'ssl']],
+    ['verifyServerCert', ['verifyServerCert', 'sslOptions.rejectUnauthorized']],
     ['tlsServername', ['tlsServername']],
     ['tlsCaName', ['caCert.name']],

And since L272-278 / L286-293 force tls = true whenever a certificate is present, imports also need an explicit default:

@@ -299,3 +299,9 @@ export class DatabaseImportService {
       }
 
+      // Import files rarely carry `verifyServerCert`, and an absent value now
+      // means "do not verify". Keep the previous behaviour explicit.
+      if (data?.tls) {
+        data.verifyServerCert = data.verifyServerCert ?? true;
+      }
+
       if (data?.compressor && !(data.compressor in Compressor)) {

(data.verifyServerCert = !!data.caCert is the narrower variant, but it changes behaviour for CA-less TLS imports, so I'd default to true.)

3. Pre-setup discovery

redisinsight/api/src/modules/database-discovery/utils/pre-setup.discovery.util.ts

populateDefaultValues defaults verifyServerCert to null (L45), and L156 sets true only when both RI_REDIS_TLS_CERT and RI_REDIS_TLS_KEY are present. A CA-only pre-setup, or a bare RI_REDIS_TLS=true, stays null and so flips to verify-off:

@@ -151,9 +151,15 @@ export const prepareDatabaseFromEnvs = async (
     if (tlsCertificate && tlsKey) {
       databaseToAdd.clientCert = {
         certificate: tlsCertificate,
         key: tlsKey,
       } as ClientCertificate;
-      databaseToAdd.verifyServerCert = true;
     }
 
+    // `populateDefaultValues` leaves `verifyServerCert` null, which used to
+    // mean "verify" and now means "do not verify". Any TLS pre-setup verified
+    // the server certificate before this change, so state it explicitly.
+    if (databaseToAdd.tls) {
+      databaseToAdd.verifyServerCert = true;
+    }
+
     const preparedDatabase = populateDefaultValues(databaseToAdd);

Needs a maintainer decision: existing rows

database_instance."verifyServerCert" is created as a plain boolean with no SQL default (migration/1670252337342-database-new.ts) and the entity column is nullable: true (database.entity.ts L155) — the @ApiPropertyOptional({ default: false }) in models/database.ts is Swagger metadata, not a runtime default. So rows written before the checkbox was consistently sent hold NULL, and this PR reinterprets them from verify-on to verify-off. Two reasonable ways to land that, and it's your call rather than something the author should guess:

• a one-time migration UPDATE database_instance SET "verifyServerCert" = 0 WHERE "verifyServerCert" IS NULL, making the new reading explicit in the data and pairing with the patches above; or
• leave the data alone and document NULL as "off" in the release notes.

Issue reference

#4471 is closed as not planned and is a feature request for CA-bundle file support — it never claims the checkbox is ignored. Open #5179 (status: accepted, self-signed cert rotation) looks like the closer match if you want a linked issue.

---

Generated by Claude Code

[MickeyShnaiderman-RecoLabs]

Re: comment 5079013662
Fixed the three producer paths in 39d0770. Leaving existing NULL rows as-is (code already treats them as off). A backfill-to-true would re-break the unchecked checkbox. Happy to retarget #5179 if you prefer that over Refs #4471.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 39d07702d0

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[cursor]

Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.

^{Reviewed by Cursor Bugbot for commit 39d0770. Configure here.}

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b8c38d42f2

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[claude]

Re-reviewed at 10669e2. All three producer paths are covered and correct: Azure at azure-autodiscovery.service.ts:631 lands on the single create() call both auth paths feed, the field-map entry at database-import.service.ts:60 matches its neighbours, and the pre-setup rule at pre-setup.discovery.util.ts:159-162 replaces the client-cert-only assignment and runs before populateDefaultValues. Specs pass (112 across the five suites), tsc shows no new errors against 1cb0be6, and CI is green. Two things left.

1. null on import is now verify-off (database-import.service.ts:302-309). The suggestion in the earlier review defaulted any TLS import to verify-on (?? true); this narrows it to absent-only, so a RedisInsight export of a TLS row with verifyServerCert: null — every row created by an older client, the API, or the paths this PR just fixed — re-imports with verification off, where it verifies today. Your rationale (?? true re-checks a box the user unchecked) is fair, and NULL is genuinely ambiguous, so this is a product call rather than a bug: maintainers, please pick, because the two readings differ for real users. One small correctness note either way: === true also turns a non-boolean into false, so "sslOptions": {"rejectUnauthorized": "true"} now silently imports as verify-off instead of failing @IsBoolean. if (x === undefined) x = true; else if (x === null) x = false; keeps bogus values failing loudly.

2. The new migration keys off provider, which is hostname-derived. getHostingProvider assigns AZURE_CACHE / AZURE_CACHE_REDIS_ENTERPRISE from *.cache.windows.net / *.redisenterprise.cache.azure.net for any database whose provider isn't set explicitly (utils/hosting-provider-helper.ts:38-43, "for telemetry only"). So provider IN (…) also matches hand-added Azure-hosted databases; one with a NULL flag — older client or API caller — gets flipped to verify-on, i.e. re-broken in exactly the way this PR fixes. AND providerDetails IS NOT NULL would scope it to autodiscovered rows. Also note down() nulls rows the user explicitly checked.

Two small housekeeping items: the description still says Refs #4471 (a closed-as-not-planned CA-bundle request) — open #5179 is the closer match, and yes, please retarget; and it no longer describes what the PR does, which now includes producer-path behaviour changes and a data migration. Worth a refresh so reviewers see the migration.

---

Generated by Claude Code