fix(domain-updater): don't clobber existing SSL data with empty values#90
Open
firstlayerdigital wants to merge 1 commit into
Open
Conversation
When the SSL cert fetch in domain-info fails (e.g. an apex domain with no
A record, so TLS connect to :443 errors out), the endpoint returns empty
strings for all SSL fields (issuer: "", valid_from: "", valid_to: "",
fingerprint: "", etc.).
The INSERT path in updateSSL correctly coerces empty strings to NULL via
`toDateOnly(x) || null`, so the first run succeeds with a row of nulls.
But on every subsequent run the UPDATE path hits two problems:
1. The date-comparison block runs `new Date("")` -> Invalid Date,
`diffInDays` becomes NaN, the `isNaN(diffInDays) || diffInDays > 1`
check passes, and the code pushes `toDateOnly("")` (empty string) as
a bound parameter with a `::date` cast. Postgres rejects with
`invalid input syntax for type date: ""`, which bubbles up as
`pgExecutor HTTP 500` and ends up in the changes array as an error.
2. For text/int fields, `oldVal !== newVal` is true when a previous
successful fetch populated e.g. `issuer = 'Let's Encrypt'` and the
current fetch returned "". The code then pushes `field.new ?? null`
which falls through to `""` (since `??` doesn't catch empty strings),
silently overwriting good historical data with empty.
Fix: skip the iteration entirely when the fresh value is empty. Also
guards the date path against Invalid Date even if upstream starts
sending garbage like "not a date". This preserves existing data when
fetches partially or fully fail, which matches the behavior a domain
portfolio tracker should have (stale cert info is more useful than no
cert info).
Repro: self-hosted instance (DL_ENV_TYPE=selfHosted), add a domain whose
DNS only has subdomain records (no apex A record), then hit
/domain-updater twice. First run inserts nulls, second run returns
`"(⚠️ Error in updateSSL: pgExecutor HTTP 500: ... invalid input syntax
for type date: \"\")"` in that domain's changes list.
|
@firstlayerdigital is attempting to deploy a commit to the AS93 Team on Vercel. A member of the Team first needs to authorize it. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
domain-infofails for a domain (e.g. an apex with no A record, so thetls.connect()call rejects withNo address associated with hostname), the endpoint returns empty strings for all SSL fields.updateSSLINSERT path handles this fine — it coerces""toNULLviatoDateOnly(x) || null, so the first run quietly inserts a row of nulls.::datecasts, which Postgres rejects withinvalid input syntax for type date: "".issuer/subject/fingerprint/key_sizewith""/0when a fetch partially fails —field.new ?? nullfalls through to the empty string because??doesn't catch empty strings.The bug in practice
First run (INSERT), fine:
Second run (UPDATE), crashes:
Root cause walkthrough
In the UPDATE loop (
src/server/routes/domain-updater/updateFns/ssl.ts):Postgres then rejects the
""::datecast.Fix
Skip the iteration entirely when
newValis empty (applies to dates, text, and int uniformly), and add an Invalid-Date guard in the date branch:This preserves existing SSL data when fetches partially fail, which matches the behavior a domain portfolio tracker should have — stale cert info is more useful than blanked-out cert info.
Note:
field.new ?? nullon line 96 still falls through to""for empty strings (since??only catchesnull/undefined), but the newif (!newVal) continue;guard above now prevents that path from being reached with an empty value. Left the existing??in place to minimize diff; could be tightened tofield.new || nullin a follow-up if you want belt-and-suspenders.Test plan
DL_ENV_TYPE=selfHosted), add an apex domain with no A record, hit/domain-updatertwice. Before: crashes on second run withinvalid input syntax for type date: "". After: returns"No changes, all data is up-to-date"..mjsbundle runs on CT 113 (Node 22, Postgres 17) against 6 real domains including one apex with no A record; the other 5 domains continue to pick up legitimate SSL field changes (e.g.SSL Key Sizediffs) without regression.ng test— the existing test suite targets Angular components (src/app/app.component.spec.ts) and doesn't cover the server-sideupdateFns/, so there's nothing to run against this change. Happy to add a unit test forupdateSSLif you'd like.Unrelated note (separate from this PR)
The
community-scripts/ProxmoxVEinstall script for Domain Locker does not install thewhoissystem package. This breaks WHOIS lookups for.io,.app, and some other TLDs that thewhois-jsonnpm lib doesn't handle — the native-binary fallback insrc/server/utils/whois.tssilently fails becausewhoisisn't inPATH. I'll file that separately on community-scripts.