Skip to content

Port outstanding fixes onto 2.0.2: security hardening, error handling, and DNSSEC/URL forwarding/DNS zone management - #27

Closed
jpmorby wants to merge 13 commits into
blesta:masterfrom
jpmorby:pr/upstream-2.0.2-fixes
Closed

jpmorby wants to merge 13 commits into
blesta:masterfrom
jpmorby:pr/upstream-2.0.2-fixes

Conversation

@jpmorby

@jpmorby jpmorby commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Summary

An audit against the shipped 2.0.2 release found that a batch of earlier fixes and features on this fork's master had diverged from upstream around Feb 2026 and never landed. This PR re-applies/re-implements them against the current 2.0.2 baseline as individual commits, security/correctness fixes first:

Security & correctness

  • Fix cURL error handling: add connect/request timeouts, stop silently falling through on curl_exec failure, and escape XML attribute values in buildRecursiveAttributes (attribute injection hardening)
  • Replace the predictable registration password (substr(base64_encode(md5($client->id_value)), 0, 15)) with a properly random one; add null-safety around registrar_lock/whois_privacy_state POST handling
  • Add null checks around getModuleRow() at its ~15 call sites to prevent fatal errors
  • Fix processResponse null handling and add early returns on API failures
  • Fix postal code being silently overwritten by a bogus default in setDomainContacts
  • Mask the API key in the admin UI instead of rendering it in plaintext; fix a few view/language key issues (including a real .FR registrant_type collision bug present in the shipped 2.0.2 views)

Other fixes

  • Fix TLD pricing loops, .FR validation, and inverted transfer-availability logic
  • Fall back to gethostbyname() for nameserver IPs when OpenSRS omits ipaddress
  • Add an Opensrs.countries lookup table and convert country fields to dropdowns

New features

  • DNSSEC DS record management (API commands + admin/client tabs)
  • URL forwarding management (API commands + admin/client tabs)
  • DNS zone management tab (admin + client views)
  • cancelService/suspendService/unsuspendService/editService/restoreDomain

Test plan

  • php -l passes on every changed/new file (verified locally)
  • Manual smoke test against an OpenSRS test/live account: domain registration (password generation), whois/postal_code handling, DNSSEC add/remove, URL forwarding, DNS zone tab, cancel/suspend/restore service actions
  • Review the DNSSEC/URL-forwarding/DNS-zone view markup for Bootstrap 5 styling consistency with the rest of the module — these were reconstructed against the current view conventions rather than ported verbatim

Jon Morby added 13 commits August 21, 2026 10:23
- Add CURLOPT_CONNECTTIMEOUT (30s) and CURLOPT_TIMEOUT (60s) to prevent
  hanging requests on unresponsive API endpoints
- Return empty OpensrsResponse instead of passing false to constructor
  when curl_exec fails, preventing downstream type errors
- Fix $siganture typo to $signature (cosmetic, no functional impact as
  the variable was used consistently)
- Escape XML attribute values with htmlspecialchars() to prevent
  malformed XML from special characters in domain contact data
- Make raw() null-safe with ?? '' fallback
- status() no longer returns null for invalid responses, so callers
  can safely compare without null checks
- Harden validateConnection() with try/catch and status()/is_success
  null-safety now that curl failures return an empty response object
  rather than throwing or returning malformed data
- Replace deterministic password (md5 of client ID) with
  cryptographically random password using random_bytes()
- Add null-coalescing defaults for registrar_lock and
  whois_privacy_state POST values to prevent PHP notices
- Add null check for $client before WHOIS fields loop in addService()
  to prevent fatal error when client_id is missing

validateConnection() try/catch and null-safety already applied in the
previous commit (cURL/response hardening).
- Add getModuleRowOrFail() helper that sets a user-friendly error when
  the module row cannot be found, instead of causing a fatal PHP error
  from accessing properties on null
- Replace all direct getModuleRow() call sites with getModuleRowOrFail()
  plus early returns in: getFilteredTldPricing, manageSettings,
  checkAvailability, getDomainContacts, setDomainContacts,
  getDomainInfo, getDomainIsLocked, getDomainIsPrivate,
  setDomainNameservers, lockDomain, unlockDomain, registerDomain,
  renewDomain, getExpirationDate, getRegistrationDate
- Rename misleading $domains_provisioning to $domains_ns in
  setDomainNameservers (it was instantiating OpensrsDomainsNs)
- Add language key for module_row.missing error
- Handle null from response->errors() gracefully with fallback message
- Add early returns after processResponse() in getDomainContacts,
  getDomainInfo, getDomainIsLocked, getDomainIsPrivate, and
  getDomainNameServers to prevent dereferencing null responses
- Add null-coalescing for attribute access (lock_state, state) to
  prevent undefined index errors
- Replace gethostbyname() fallback with empty string for missing IPs
  to avoid DNS lookups that could hang or return wrong data (a later
  commit reinstates a safer gethostbyname() fallback)
- Guard nameserver_list iteration with ?? [] for missing key
- Fix addService() blanket Input::setErrors([]) that was clearing
  real registration errors along with nameserver errors
- Filter empty nameservers before passing to setDomainNameservers
setDomainContacts() was discarding any real postal_code value supplied
in $vars, always falling back to $contact['zip'], and defaulting to
'00000' when neither was present. Preserve an explicit postal_code
when one is provided.

Partial port of a8d5787 (upstream master) — only the postal_code fix;
the DNSSEC get-endpoint fix from that commit is applied separately
alongside the new DNSSEC feature port.
- Mask API key in manage view, showing only last 4 characters
- Change API key input to a password field in add/edit row views
- Remove unused $i variable in client WHOIS tab view
- Namespace .FR domain language keys (fr_registrant_type) to prevent
  collision with .UK registrant_type keys that share the same key
  name; also fixes a pre-existing bug where .FR referenced a
  nonexistent Opensrs.domain.registrant_type.company language key
- Update .FR config to reference the namespaced language keys
- Remove dead foreach(range(1,10)) loops wrapping pricing API calls;
  the API returns all periods in a single response via all_periods=1,
  so the loop just overwrote the same key 10 times
- Fix .FR domain validation: unset company fields (VAT ID, SIREN,
  trademark) for 'individual' registrants, not 'organization' ones
  (the condition was inverted)
- Replace naive checkTransferAvailability() inverse-lookup with proper
  OpensrsDomainsTransfer::checkTransfer() API call that checks the
  transferrable attribute, with fallback to old behavior
…address

Restores a DNS lookup fallback so getDomainNameServers() still returns
a usable IP when OpenSRS doesn't supply one, at the cost of the
occasional slow/hanging lookup that an earlier commit in this branch
deliberately avoided. Reinstated per explicit prioritization from
upstream master.
…downs

Defines Opensrs.countries as a full ISO 3166-1 alpha-2 country code map
at the top of the config file. Converts free-text country inputs
(owner, tech, admin, billing WHOIS contacts and the .law/.abogado
jurisdiction country) to select fields backed by this lookup table.
- Create OpensrsDomainsDnssec class wrapping provisioning modify with
  data='dnssec' for add/remove, and get with type='dnssec' for
  fetching existing DS records
- Register tabDnssec/tabClientDnssec unconditionally in the admin and
  client service tabs
- Add manageDnssec() private method handling:
  - GET: Fetches existing DS records
  - POST add_ds_record: Adds DS record with key_tag, algorithm,
    digest_type, and digest fields
  - POST delete_ds_record: Removes DS record by index
- Create tab_dnssec.pdt (admin, Bootstrap 5) and tab_client_dnssec.pdt
  (client) views with a DS record table and add form
- Provide algorithm (3-16) and digest type (1,2,4) dropdowns with
  human-readable labels
- Add all DNSSEC-related language keys
- Treat empty arrays as lists in buildRecursiveAttributes() so a
  cleared DS record set serializes as <dt_array/> instead of
  <dt_assoc/>, which OpenSRS rejects on delete

Includes fixes from upstream master applied directly during the port:
the get-DS-records endpoint uses type='dnssec' (not the incorrect
domain_auth_info from the original commit), all DNSSEC API fetches are
logged via logRequest(), and no blanket Input::setErrors([]) clears
preceding errors when the fetch fails.
- Create OpensrsDomainsForwarding class with get/set/create/delete
  methods for domain forwarding via the OpenSRS API
- Register tabUrlForwarding/tabClientUrlForwarding in service tabs,
  conditional on the dns_management service feature
- Add manageUrlForwarding() private method handling:
  - GET: Fetches forwarding records from the DNS zone
  - POST set_forwarding: Adds forwarding rule with subdomain,
    destination URL, and redirect type (301/302/frame)
  - POST delete_forwarding: Removes forwarding rule by index
- Create tab_url_forwarding.pdt (admin, Bootstrap 5) and
  tab_client_url_forwarding.pdt (client) views
- Add language keys for all forwarding labels

Built on the existing OpensrsDomainsDns::getDnsZone()/setDnsZone()
commands already present in this codebase. Logs the final DS-zone
fetch via logRequest() and avoids clearing preceding errors on
failure, consistent with the DNSSEC tab added in the previous commit.
- Register tabDns/tabClientDns in service tab arrays, conditional on
  the dns_management feature being enabled for the service
- Add manageDns() private method handling:
  - GET: Fetches DNS zone records via OpensrsDomainsDns::getDnsZone()
  - POST add_record: Appends new record to zone via setDnsZone()
  - POST delete_record: Removes record by type and index via
    setDnsZone()
  - POST reset_zone: Resets zone to defaults via resetDnsZone()
- Create tab_dns.pdt (admin, Bootstrap 5) with record table, add form,
  and reset button
- Create tab_client_dns.pdt (client) with a table and form matching
  existing client tab patterns
- Add all DNS-related language keys

The OpensrsDomainsDns class already existed in this codebase but was
never wired into the module UI. Logs the final zone fetch via
logRequest() and avoids clearing preceding errors on failure,
consistent with the DNSSEC and URL forwarding tabs added previously.
…toreDomain

- cancelService: Disables auto-renew and sets expire_action to let
  the domain expire at the end of the current period via the
  provisioning modify API
- suspendService: Delegates to cancelService (standard Blesta pattern
  for domain modules — no OpenSRS suspend API exists)
- unsuspendService: Re-enables auto-renew via provisioning modify
- editService: Minimal implementation returning null to preserve
  existing service meta fields
- restoreDomain: Wires up the existing
  OpensrsDomainsProvisioning::redeem() method for domains in the
  redemption grace period, matching the three-argument
  RegistrarModule::restoreDomain() parent signature (including the
  unused $vars parameter) so the override doesn't diverge from the
  base class
- Add language keys for service lifecycle error messages

Previously these methods fell through to RegistrarModule's default
"unsupported" implementations, so cancel/suspend/unsuspend/restore
from the admin or client UI silently did nothing against OpenSRS.
@jpmorby

jpmorby commented Aug 21, 2026

Copy link
Copy Markdown
Contributor Author

Splitting this into individual PRs per your one-fix-per-PR guidance:

  1. Fix cURL error handling, add timeouts, and harden API response parsing #28 - Fix cURL error handling, add timeouts, and harden API response parsing
  2. Fix registration security and input validation #29 - Fix registration security and input validation
  3. Add null checks for getModuleRow to prevent fatal errors #30 - Add null checks for getModuleRow to prevent fatal errors
  4. Fix processResponse null handling and add early returns for API failures #31 - Fix processResponse null handling and add early returns for API failures
  5. Fix postal code being overwritten in setDomainContacts #32 - Fix postal code being overwritten in setDomainContacts
  6. Fix views, language key collisions, and minor UI issues #33 - Fix views, language key collisions, and minor UI issues
  7. Fix TLD pricing loops, .FR validation, and transfer availability check #34 - Fix TLD pricing loops, .FR validation, and transfer availability check
  8. Add Opensrs.countries lookup table and convert country fields to dropdowns #35 - Add Opensrs.countries lookup table and convert country fields to dropdowns
  9. Add DNSSEC DS record management API and tabs #36 - Add DNSSEC DS record management API and tabs
  10. Add URL forwarding API commands and management tabs #37 - Add URL forwarding API commands and management tabs
  11. Wire up DNS zone management tab with admin and client views #38 - Wire up DNS zone management tab with admin and client views
  12. Add cancelService, suspendService, unsuspendService, editService, restoreDomain #39 - Add cancelService, suspendService, unsuspendService, editService, restoreDomain

Note: the nameserver-IP gethostbyname() fallback fix from this branch is not included as a separate PR — that fix only existed to undo a regression introduced earlier within this same combined branch, and the fallback is already present unmodified on the 2.0.2 baseline, so there was nothing to change when isolated.

Closing this combined PR in favor of the above.

@jpmorby jpmorby closed this Aug 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant