initial
This commit is contained in:
@@ -0,0 +1,518 @@
|
||||
# Security, Reliability & PII Audit — `esp-instanda-integration`
|
||||
|
||||
**Plugin version:** 1.0.7 · **Audit date:** 2026-07-02 · **Standards:** Getfused Universal Coding Standards
|
||||
**Target:** local WordPress Studio copy of `hub-insurance` (SQLite, WP 7.0, PHP 8.5, GF 2.10.5)
|
||||
**Method:** static review → adversarial verification (one skeptic per finding + completeness critics)
|
||||
→ live read-only inspection → in-process state-machine tests (INSTANDA mocked; **zero external API traffic**).
|
||||
|
||||
> **Report-only.** No plugin code was changed. The only site writes were one throwaway transaction
|
||||
> row and a temporary read-only probe file, both deleted; the transaction table is back to its
|
||||
> original 14 rows. Severities below were **reconciled against an adversarial verification pass** —
|
||||
> it refuted 6 candidate findings and downgraded several others (see §5), which is why some items
|
||||
> are lower than a first read would suggest.
|
||||
|
||||
---
|
||||
|
||||
## 1. Executive summary
|
||||
|
||||
This is a well-built add-on. It has a write-ahead transaction log, a test-mode lock enforced in
|
||||
three independent layers, redacting logging, `Throwable` guards on every hook so a plugin bug
|
||||
can't white-screen the form, and libsodium encryption for the stored API password. Most of that
|
||||
machinery was verified to work as designed. Several things a first read flags as alarming turned
|
||||
out to be **non-issues** on inspection — no public information disclosure, no SQL injection, no
|
||||
admin-page privilege leak, and the form is *not* mis-wired into unsubmittability (§5).
|
||||
|
||||
What remains are the failure modes that matter for a form which **spends money and stores PII on
|
||||
every submission**, because the plugin's two guarantees — *no lost quotes, no duplicate policies* —
|
||||
are not fully met. The load-bearing items, each confirmed with live evidence on the running site:
|
||||
|
||||
1. **Duplicate INSTANDA quotes on repeat submits** (F-01) — proven live: one email submitted 10
|
||||
times produced 10 distinct quotes. Duplicate-prevention only stops a double-fire *within one
|
||||
request* (and production's object cache doesn't change this — it's token instability).
|
||||
2. **Completing the Go-Live flow can leave real quotes going to the Test endpoint** (F-03) —
|
||||
`switch_to_live()` flips the mode but never sets the environment.
|
||||
3. **The stored API password is recoverable from a DB dump** — confirmed on **both** the local and
|
||||
the production database: no `SPG_INSTANDA_KEY` constant anywhere, so the encryption key sits
|
||||
beside the ciphertext it protects (F-04).
|
||||
4. **Plaintext PII with indefinite retention, and failure alerts silently disabled** (no alert
|
||||
recipients) — confirmed in production too (F-05).
|
||||
5. **Config drift + a stray active feed on the test form** (F-02) — production has Form 5 correctly
|
||||
wired *and* an active feed still attached to the test Form 6; the local copy has the feed on the
|
||||
wrong form entirely.
|
||||
|
||||
Remediation is tractable and prioritized in §7. Production is verified to be **pre-launch** (still
|
||||
in INSTANDA test mode), so these are fixable before go-live.
|
||||
|
||||
---
|
||||
|
||||
## 2. Scope, environment & method
|
||||
|
||||
**In scope:** the plugin (`wp-content/plugins/esp-instanda-integration/`, 18 files, ~4,900 lines)
|
||||
and its front-end coupling in the active theme (`wp-content/themes/GFeneratePress/functions.php`).
|
||||
|
||||
**Live environment observed (read-only):**
|
||||
|
||||
| Fact | Value | Why it matters |
|
||||
|---|---|---|
|
||||
| Active theme | `GFeneratePress` | carries the Form 5 date validator |
|
||||
| Timezone | `America/New_York` (`gmt_offset -4`) | makes the TZ-mixing bug a real 4h skew (F-07) |
|
||||
| `SPG_INSTANDA_*` constants | **none defined** | credentials + key resolve to the DB (F-04) |
|
||||
| `GRAVITYSMTP_SENDGRID_API_KEY` | **defined** in `wp-config.php` | hardcoded secret (F-08) |
|
||||
| Object-cache drop-in | **absent** (`wp_using_ext_object_cache()=no`) | cross-request lock is inert locally |
|
||||
| INSTANDA feed | **Form 6** only (`event_start=7, event_end=8, customer_email=9`) | F-02 |
|
||||
| Live Form 5 | dates at 8/9, email at 10; **no feed**; page "Form Test Page" | F-02 |
|
||||
| Operating mode / env | unset→`test` / `test`; domain `consumer.instanda.us` | mode clamp active |
|
||||
| `alert_emails` | **empty** | failure alerts silently no-op (F-05) |
|
||||
| Retention / storage | `retention_days=7`, `storage_enabled=1` | delivered-only purge |
|
||||
| Cleanup cron | `esp_instanda_cleanup` daily (one instance) | correct |
|
||||
| Transaction table | 14 rows (13 delivered, 1 failed), all Form 6, `environment=test` | real test data |
|
||||
|
||||
**Verification passes:** (a) full static read of the six load-bearing files + architecture map of
|
||||
all 18; (b) an adversarial pass — one agent per candidate finding tasked with *refuting* it, plus
|
||||
three completeness critics; (c) live probes via `wp eval`/`eval-file` (`wp db query` is unusable
|
||||
on this SQLite build — it probes MySQL `sql_mode`); (d) in-process `pre_http_request` mocks driving
|
||||
the real `Api_Client`/`Transaction_Store` through every response class with **no external calls**.
|
||||
|
||||
**Production parity:** verified via read-only WP-CLI on the live Pressable `hubinsurance` site
|
||||
(§6) — object cache, secrets/constants, `sql_mode`, schema, feeds, and options.
|
||||
|
||||
---
|
||||
|
||||
## 3. Severity summary
|
||||
|
||||
| # | Finding | Severity | Status |
|
||||
|---|---------|----------|--------|
|
||||
| **F-01** | Duplicate INSTANDA quotes on repeat submits (token instability) | **High** | Live-proven |
|
||||
| **F-03** | `switch_to_live()` doesn't set env → live quotes keep hitting Test | **High** | Code-confirmed |
|
||||
| **F-04** | API password recoverable from a DB dump (self-provisioned key) | **High** | Confirmed local **+ prod** |
|
||||
| F-02 | Stray active feed on test Form 6 + local↔prod feed drift (prod Form 5 wired) | Medium | Live-confirmed |
|
||||
| F-05 | Plaintext PII, indefinite retention, alerts disabled | Medium | Live-confirmed |
|
||||
| F-06 | No rate-limit / no CAPTCHA on public → API path | Medium | Live-confirmed |
|
||||
| F-07 | Timezone mixing skews retention & resubmit self-recovery | Medium | Confirmed (UTC-4) |
|
||||
| F-08 | Hardcoded SendGrid API key in `wp-config.php` | Medium | Live-confirmed |
|
||||
| F-09 | Async recording fails silently; Form 6 has zero notifications | Medium | Confirmed |
|
||||
| F-10 | 5xx auto-retry of the mutating PUT is a duplicate vector | Medium | Verified |
|
||||
| F-11 | `Guard::wrap` fails open on validation → quote-less entries | Medium | Confirmed |
|
||||
| F-12 | Delivered transaction can be resubmitted → duplicate quote | Medium | Code-confirmed |
|
||||
| F-13 | Stranded-lead delivered row is purged by retention → lost lead | Medium | Confirmed |
|
||||
| F-14 | Write-ahead insert failure / storage-off → silent StartQuote | Medium | Confirmed |
|
||||
| F-15 | Uninstall keeps the encrypted password **and** its key + all options | Medium | Confirmed |
|
||||
| F-16 | "Gravity Forms required" admin notice is unreachable when GF is absent | Medium | Code-confirmed |
|
||||
| F-17 | `200`-without-`quoteRef` recorded as failed, not possible-duplicate | Low | Verified |
|
||||
| F-18 | Resubmit self-recovery over-matches → skips legitimate resubmits | Low | Confirmed |
|
||||
| F-19 | DB-tamper trust boundary (resubmit replays stored body; outbound mail) | Low | Confirmed |
|
||||
| F-20 | Operator-misleading dead code (retry count, resubmit event never fire) | Low | Confirmed |
|
||||
| F-21 | `event_log` non-atomic read-modify-write drops concurrent audit events | Low | Confirmed |
|
||||
| F-22 | Retention cron scheduled only at activation, never re-ensured | Low | Confirmed |
|
||||
| F-23 | Front-end hygiene: ~220 lines inline JS sitewide + observer never disconnects | Low | Confirmed |
|
||||
| F-24 | Uninstall/deactivation option residue; stale docblocks/template | Info | Confirmed |
|
||||
|
||||
§5 lists what was **checked and cleared** (including 6 refuted candidates).
|
||||
|
||||
---
|
||||
|
||||
## 4. Findings
|
||||
|
||||
### F-01 · Duplicate INSTANDA quotes on repeat submits — High · Live-proven
|
||||
|
||||
The duplicate-prevention scheme keys off `submission_token()`
|
||||
([class-submission.php:219-229](../wp-content/plugins/esp-instanda-integration/includes/class-submission.php#L219-L229)),
|
||||
which uses `GFFormsModel::get_form_unique_id` — a fresh `uniqid()` per request for this
|
||||
non-upload form (or a client-supplied value). So the per-submission lock, replay cache, and the
|
||||
"already delivered" short-circuit ([:63-71](../wp-content/plugins/esp-instanda-integration/includes/class-submission.php#L63-L71))
|
||||
only match *within a single request*. Across a browser re-POST, a back-button resubmit, or the
|
||||
retry the plugin invites after an ambiguous timeout ("*Please try again in a moment*",
|
||||
[:312](../wp-content/plugins/esp-instanda-integration/includes/class-submission.php#L312)), the
|
||||
token differs, the dedupe key differs, and a **new** `PUT /StartQuote` fires. The dedupe key is
|
||||
computed but never transmitted, and INSTANDA has no idempotency.
|
||||
|
||||
**Live proof.** 14 rows, **14 distinct dedupe keys, 13 distinct quote refs**. `zbatte@getfused.com`
|
||||
appears in **10 submissions**, each with a *different* quote ref (rows 12 and 13 are 36 s apart →
|
||||
`CVPAZE` and `Y38STE`). The delivered-dedupe short-circuit has never fired in real use.
|
||||
|
||||
**Reconciliation.** Rated **High**, not Critical: on the default posture the traffic hits the
|
||||
*Test* endpoint (mode clamp), so this creates duplicate *Test* quotes today. Once live (F-03), it
|
||||
creates duplicate real policies.
|
||||
|
||||
**Fix.** Derive the dedupe key from **submission content + form id** (hash of the mapped field
|
||||
values), so identical resubmits collapse; keep the per-request lock as a second layer; persist the
|
||||
delivered short-circuit by content hash so a re-POST returns the prior quote URL instead of a new
|
||||
quote.
|
||||
|
||||
---
|
||||
|
||||
### F-02 · Stray active feed on the test form + local↔prod feed drift — Medium · Live-confirmed
|
||||
|
||||
The two environments are configured differently, and both carry loose ends:
|
||||
|
||||
| | Form 5 "Start a Quote" (dates 8/9, email 10) | Form 6 "…(INSTANDA test)" (dates 7/8, email 9) |
|
||||
|---|---|---|
|
||||
| **Production** | active feed `8/9/10` ✓ + theme validation `8/9` ✓ → **correctly wired** | **active feed still attached** (test form) |
|
||||
| **Local** | **no feed** (theme validates it, but it never reaches INSTANDA) | active feed `7/8/9` — the only fed form; all 14 tx came from it |
|
||||
|
||||
So on production, AGENTS.md's "Form 5 is the live form" actually holds — Form 5's feed mapping and
|
||||
the theme's hardcoded `8/9` agree. The residual issues are: (a) **a still-active INSTANDA feed on
|
||||
the *test* Form 6 in production** — if that page is reachable it also creates real (currently Test)
|
||||
quotes and gets no theme date validation
|
||||
([functions.php:128-130](../wp-content/themes/GFeneratePress/functions.php#L128-L130) only handles
|
||||
form id 5); and (b) **config drift** — the local copy has the feed on Form 6 only, so testing
|
||||
Form 5 locally never exercises the INSTANDA path that runs in prod. The theme validator being
|
||||
hardcoded to a single form id ([functions.php:74-76](../wp-content/themes/GFeneratePress/functions.php#L74-L76))
|
||||
is the underlying fragility. (The earlier worry that a field mismatch *bricks* the form was
|
||||
**refuted** — §5; every fed form is individually wired correctly.)
|
||||
|
||||
**Fix.** Deactivate the INSTANDA feed on the test Form 6 in production (or retire the form); align
|
||||
the local copy with prod so tests exercise the live path; drive the theme's `GF_DATE_FORM_ID` from
|
||||
the form that actually has the active feed rather than a hardcoded constant, and add a go-live
|
||||
assertion that the public quote page embeds exactly one fed, theme-validated form.
|
||||
|
||||
---
|
||||
|
||||
### F-03 · `switch_to_live()` doesn't set the environment — High · Code-confirmed
|
||||
|
||||
The sanctioned go-live action sets the operating mode but **not** the environment:
|
||||
|
||||
```php
|
||||
// class-go-live.php
|
||||
switch_to_live() { update_option('espinstanda_operating_mode', 'live'); … } // :131 — no env write
|
||||
return_to_test() { update_option('espinstanda_operating_mode', 'test');
|
||||
update_option('espinstanda_env', 'test'); … } // :145-146 — sets BOTH
|
||||
```
|
||||
|
||||
`Config::environment()` returns Test whenever `raw_env()` isn't `'live'`
|
||||
([class-config.php:87-98](../wp-content/plugins/esp-instanda-integration/includes/class-config.php#L87-L98)),
|
||||
and `raw_env()` reads the `espinstanda_env` option (currently `test`). So an operator who completes
|
||||
the Go-Live checklist and clicks "switch to live" ends up with `operating_mode=live` but
|
||||
`environment=Test` — the test-mode lock is now *off*, yet **real customer quotes keep going to the
|
||||
INSTANDA Test endpoint with Test credentials**. The asymmetry with `return_to_test()` (which sets
|
||||
both) shows this is a bug, not a design choice.
|
||||
|
||||
**Mitigation / caveat.** If the intended Live posture is followed — `SPG_INSTANDA_ENV='live'` (or
|
||||
the settings env radio) set as part of go-live — `raw_env()` returns `'live'` via the constant and
|
||||
the switch works. So this bites the operator who relies on the Go-Live button alone. Given that
|
||||
button *is* the sanctioned flow, this is a High-priority correctness bug.
|
||||
|
||||
**Fix.** Have `switch_to_live()` set `espinstanda_env='live'` (mirroring `return_to_test()`), or
|
||||
make `environment()` follow `operating_mode` directly.
|
||||
|
||||
---
|
||||
|
||||
### F-04 · API password recoverable from a DB dump — High · Confirmed local **+ production**
|
||||
|
||||
With no `SPG_INSTANDA_KEY` constant, the libsodium key is self-provisioned into the DB option
|
||||
`espinstanda_secret_key`
|
||||
([class-config.php:193-226](../wp-content/plugins/esp-instanda-integration/includes/class-config.php#L193-L226)) —
|
||||
i.e. the key lives in the **same database** as the ciphertext it protects. **Verified on both
|
||||
environments:** neither local nor the production `hubinsurance` site defines `SPG_INSTANDA_KEY`
|
||||
(nor any `SPG_INSTANDA_*` credential/env constant), and on both the key (len 44 = a 32-byte sodium
|
||||
key) and the ciphertext (`espinstanda_password_test`, len 76) are present in the database. A
|
||||
database dump alone therefore yields the API password on the live site. The class-level docblock
|
||||
([class-config.php:13](../wp-content/plugins/esp-instanda-integration/includes/class-config.php#L13))
|
||||
asserts the *opposite* as an absolute ("*a database dump alone never exposes a usable secret*");
|
||||
only the private docblock at :205-211 admits the caveat.
|
||||
|
||||
**No mitigation in place.** The intended-to-be-hardened path — setting `SPG_INSTANDA_KEY` (and the
|
||||
credential constants) in `wp-config.php` to move secrets out of the DB — is **not used in
|
||||
production**, so the guarantee the docblock states is not met anywhere today.
|
||||
|
||||
**Fix.** Set `SPG_INSTANDA_KEY` + credential constants in prod `wp-config.php` and make it a
|
||||
**blocking go-live check**; correct the class-13 docblock to state the caveat. (This also removes
|
||||
the F-15 uninstall exposure, since the key would no longer be in the DB.)
|
||||
|
||||
---
|
||||
|
||||
### F-05 · Plaintext PII, indefinite retention, alerts disabled — Medium · Live-confirmed
|
||||
|
||||
`redacted_payload` is a misnomer: it stores the full StartQuote JSON *including the customer email*
|
||||
(`redact()` strips only auth/password tokens, never PII —
|
||||
[class-logger.php:39-43](../wp-content/plugins/esp-instanda-integration/includes/class-logger.php#L39-L43)),
|
||||
alongside plaintext `customer_email`/`role_label` columns
|
||||
([class-transaction-store.php:104-108](../wp-content/plugins/esp-instanda-integration/includes/class-transaction-store.php#L104-L108)).
|
||||
Retention purges **delivered-only**
|
||||
([:314-322](../wp-content/plugins/esp-instanda-integration/includes/class-transaction-store.php#L314-L322)),
|
||||
so failed/pending rows keep PII forever with no data-subject deletion path. On mail-send failure,
|
||||
the customer email is also logged and fanned to a public action hook
|
||||
([class-notifier.php:70-78](../wp-content/plugins/esp-instanda-integration/includes/class-notifier.php#L70-L78)).
|
||||
Live: real emails are in the table today, and `espinstanda_alert_emails` is **empty**, so the
|
||||
failure-alert path silently no-ops — the one real 400 failure generated no alert.
|
||||
|
||||
**Fix.** Rename the column and document what it holds; add a retention sweep (or email anonymization)
|
||||
for failed/pending rows; add a delete-by-email routine for data requests; require ≥1 alert recipient
|
||||
at go-live.
|
||||
|
||||
---
|
||||
|
||||
### F-06 · No rate-limit / no CAPTCHA on the public → API path — Medium · Live-confirmed
|
||||
|
||||
Every final-page submission of a form with an active feed reaches a live `PUT /StartQuote`. The
|
||||
only throttle is the per-submission lock, which doesn't limit distinct scripted submissions (it
|
||||
only dedupes a same-token double-fire). Production has a persistent object cache (verified), so the
|
||||
lock is atomic cross-request there; locally it has no object cache and is inert — either way it
|
||||
doesn't throttle distinct submits. Anti-bot is delegated to GF's `is_valid`
|
||||
([class-submission.php:51-53](../wp-content/plugins/esp-instanda-integration/includes/class-submission.php#L51-L53)),
|
||||
and **Form 6 has no CAPTCHA field** (verified) — only a honeypot, which (per the verification pass)
|
||||
actually fires on `gform_entry_is_spam` *after* the plugin's create runs in `gform_validation`, so
|
||||
it doesn't gate the create at all. A PII row is inserted before every call (F-05), and retention
|
||||
purges delivered-only, so failed/attacker traffic accumulates.
|
||||
|
||||
**Reconciliation.** Rated **Medium**, not Critical: the test-mode clamp means the flood hits the
|
||||
*Test* endpoint by default (prod is still in test mode), so the paid-API cost-DoS is gated behind
|
||||
go-live (F-03); Field_Mapper's required-field validation adds friction; edge rate-limiting
|
||||
(Cloudflare/Pressable) may exist but was not confirmed. The *mechanism* — no application-level rate
|
||||
limit and no CAPTCHA on the live form — is confirmed.
|
||||
|
||||
**Fix.** Add a CAPTCHA (or Akismet) to the live quote form; add server-side rate limiting keyed by
|
||||
IP and email *before* `create_pending`; cap/alert on pending+failed row growth.
|
||||
|
||||
---
|
||||
|
||||
### F-07 · Timezone mixing skews retention & resubmit self-recovery — Medium · Confirmed (UTC-4)
|
||||
|
||||
Rows are written with site-local `current_time('mysql')`
|
||||
([class-transaction-store.php:140,162](../wp-content/plugins/esp-instanda-integration/includes/class-transaction-store.php#L140)),
|
||||
but the retention cutoff ([:317](../wp-content/plugins/esp-instanda-integration/includes/class-transaction-store.php#L317)),
|
||||
the resubmit self-recovery window ([:246-249](../wp-content/plugins/esp-instanda-integration/includes/class-transaction-store.php#L246-L249)),
|
||||
and `compute_expiry` ([:338-342](../wp-content/plugins/esp-instanda-integration/includes/class-transaction-store.php#L338-L342))
|
||||
use UTC `gmdate()`. Live: this site is `America/New_York`; at audit time `current_time` was `16:58`
|
||||
while `gmdate` was `20:58` — a real 4-hour divergence. The purge runs ~4h off its boundary, the 24h
|
||||
self-recovery window is shifted (feeding F-01 or F-18), and `delivered_at`/`expiry` are on different
|
||||
clocks. **Fix:** use UTC everywhere (`gmdate`/`current_time('mysql', true)`) for both writes and
|
||||
cutoffs.
|
||||
|
||||
---
|
||||
|
||||
### F-08 · Hardcoded SendGrid API key in `wp-config.php` — Medium · Live-confirmed
|
||||
|
||||
`GRAVITYSMTP_SENDGRID_API_KEY` is defined in `wp-config.php` with a live-looking `SG.…` value next
|
||||
to the salts (confirmed present; value not extracted). This is the SMTP add-on's key, but it's in
|
||||
the audited config surface and powers the plugin's customer emails. **Fix:** move it to an
|
||||
environment secret; **rotate it** if this copy has ever been committed, backed up, or shared
|
||||
(escalates to High in that case).
|
||||
|
||||
---
|
||||
|
||||
### F-09 · Async recording fails silently; Form 6 has zero notifications — Medium · Confirmed
|
||||
|
||||
With `_async_feed_processing=true`, `record_feed_result` runs in a later request where
|
||||
`Submission::$current_dedupe` is null; if `on_entry_created` didn't attach the entry
|
||||
(a `Guard`-swallowed throw, storage disabled, or a missing dedupe row) it returns with **no meta,
|
||||
no note, no notification, and no log line**
|
||||
([class-instanda-addon.php:369-403](../wp-content/plugins/esp-instanda-integration/includes/class-instanda-addon.php#L369-L403)).
|
||||
Live, this is compounded: **Form 6 has zero notifications configured**, so the
|
||||
`espinstanda_quote_success` notification the code sends has nothing to fire — the customer
|
||||
"quote ready" email is currently inert. **Fix:** fall back to `find_by_entry`/stash and *log* when a
|
||||
transaction can't be resolved; configure + test the success notification on the canonical form.
|
||||
|
||||
---
|
||||
|
||||
### F-10 · 5xx auto-retry of the mutating PUT is a duplicate vector — Medium · Verified
|
||||
|
||||
`request()` re-sends the mutating `PUT /StartQuote` after a 5xx/transport error
|
||||
([class-api-client.php:121-141](../wp-content/plugins/esp-instanda-integration/includes/class-api-client.php#L121-L141)),
|
||||
contradicting the class's own "never auto-retry a mutating ambiguous call" docblock. With no
|
||||
idempotency and the dedupe key untransmitted, a gateway 5xx *after* INSTANDA processed attempt 1
|
||||
creates a second quote. Verified in-process: a persistent 500 makes **2** attempts. **Timing
|
||||
nuance:** a genuine slow timeout (8 s) exceeds the 3 s retry budget after attempt 1, so real
|
||||
timeouts make only 1 attempt — the auto-retry duplicate risk is real for *fast* 5xx/connection
|
||||
failures, not slow timeouts. **Fix:** don't auto-retry a non-idempotent create.
|
||||
|
||||
---
|
||||
|
||||
### F-11 · `Guard::wrap` fails open on validation — Medium · Confirmed
|
||||
|
||||
`Guard::wrap` returns `$args[0]` on any `Throwable`
|
||||
([class-guard.php:24-33](../wp-content/plugins/esp-instanda-integration/includes/class-guard.php#L24-L33)).
|
||||
On `gform_validation` that leaves `is_valid` **true**, so a bug produces a saved entry with no quote
|
||||
and the default confirmation — a silent quote-less lead. The fail-open trade-off ("never break the
|
||||
form") is defensible, but the validation path specifically should record a pending/failed
|
||||
transaction and fire the `esp_instanda_throwable` alert so the lost quote is visible.
|
||||
|
||||
---
|
||||
|
||||
### F-12 · A delivered transaction can be resubmitted → duplicate quote — Medium · Code-confirmed
|
||||
|
||||
`Resubmit_Handler::run` has no delivered-status guard, and the self-recovery check
|
||||
`$recent->id !== $tx->id` deliberately does **not** skip when the recent success *is* the row being
|
||||
resubmitted ([class-transactions-table.php:144-149](../wp-content/plugins/esp-instanda-integration/includes/class-transactions-table.php#L144-L149)).
|
||||
So resubmitting an already-delivered transaction (via the admin AJAX endpoint, which also lacks a
|
||||
status guard, [class-instanda-addon.php:582-604](../wp-content/plugins/esp-instanda-integration/includes/class-instanda-addon.php#L582-L604))
|
||||
fires a second `PUT /StartQuote` and overwrites the original `quote_ref` in the row — a duplicate
|
||||
quote plus loss of the first quote's reference. Requires the `espinstanda_transactions` capability.
|
||||
**Fix:** refuse to resubmit rows whose status is already `delivered`.
|
||||
|
||||
---
|
||||
|
||||
### F-13 · Stranded-lead delivered row is purged by retention — Medium · Confirmed
|
||||
|
||||
On the no-entry success path, if both `urlSingleUse` and the `continue_quote` re-mint fail, the
|
||||
quote is delivered but the customer is never notified and no GF entry exists — only the delivered
|
||||
transaction row ([class-transactions-table.php:162-173](../wp-content/plugins/esp-instanda-integration/includes/class-transactions-table.php#L162-L173)).
|
||||
That row is then purged after the retention window
|
||||
([class-transaction-store.php:314-322](../wp-content/plugins/esp-instanda-integration/includes/class-transaction-store.php#L314-L322)),
|
||||
so the lead is lost with no trace. **Fix:** exempt delivered rows with a null `entry_id` (or an
|
||||
unsent customer email) from the retention purge, or resolve them before purge.
|
||||
|
||||
---
|
||||
|
||||
### F-14 · Write-ahead insert failure / storage-off → silent StartQuote — Medium · Confirmed
|
||||
|
||||
`create_pending` returns `0` on insert failure and the caller guards all state marking behind
|
||||
`if ($tx_id)` while **still making the StartQuote call**
|
||||
([class-submission.php:86-125](../wp-content/plugins/esp-instanda-integration/includes/class-submission.php#L86-L125)).
|
||||
The same guard means that when storage is disabled (an allowed, advisory-only posture) **every**
|
||||
StartQuote failure is completely silent — no row, no alert. So the write-ahead log's core promise
|
||||
(never make an unaudited call) is void exactly when it's needed. **Fix:** if storage is enabled and
|
||||
`create_pending` returns 0, abort or log before proceeding; keep a failure record/alert independent
|
||||
of the row insert.
|
||||
|
||||
---
|
||||
|
||||
### F-15 · Uninstall keeps the encrypted password **and** its key — Medium · Confirmed
|
||||
|
||||
`uninstall.php` deletes only `espinstanda_db_version` and `espinstanda_operating_mode_audit`,
|
||||
leaving ~13 other `espinstanda_*` options including both `espinstanda_password_test` (ciphertext)
|
||||
and `espinstanda_secret_key` (its key), plus env, site domain, retention, alert emails, and the
|
||||
go-live acknowledgements ([uninstall.php:16-32](../wp-content/plugins/esp-instanda-integration/uninstall.php#L16-L32)).
|
||||
The recoverable secret pair (F-04) survives uninstall, and a reinstall inherits stale go-live state.
|
||||
**Fix:** delete all `espinstanda_*` options and transients on uninstall (behind a "remove data"
|
||||
setting if retention is ever desired).
|
||||
|
||||
---
|
||||
|
||||
### F-16 · "Gravity Forms required" notice is unreachable when GF is absent — Medium · Code-confirmed
|
||||
|
||||
The requirement notice is registered *inside* the `gform_loaded` callback
|
||||
([esp-instanda-integration.php:86-97](../wp-content/plugins/esp-instanda-integration/esp-instanda-integration.php#L86-L97)),
|
||||
but `gform_loaded` only fires when Gravity Forms is present. So if GF is deactivated, the "Gravity
|
||||
Forms is required and is not active" notice ([:69](../wp-content/plugins/esp-instanda-integration/esp-instanda-integration.php#L69))
|
||||
never registers — the plugin goes silently inactive with no explanation. **Fix:** register the
|
||||
GF-absent notice on `admin_notices` unconditionally (outside `gform_loaded`).
|
||||
|
||||
---
|
||||
|
||||
### F-17 to F-24 · Lower-severity findings
|
||||
|
||||
| # | Finding | Where | Fix |
|
||||
|---|---------|-------|-----|
|
||||
| F-17 | `200`-without-`quoteRef` recorded as `failed`/`possible_duplicate=0` though a quote may exist (verified: `ok=true, quote_ref=null` → failure branch) | [class-submission.php:101](../wp-content/plugins/esp-instanda-integration/includes/class-submission.php#L101); [class-api-client.php:160-163](../wp-content/plugins/esp-instanda-integration/includes/class-api-client.php#L160-L163) | Treat 2xx-no-`quoteRef` (and empty-string ref) as ambiguous |
|
||||
| F-18 | Resubmit self-recovery matches **any** delivered quote for the email in 24h → skips a legitimate second-event resubmit (also shifted by F-07) | [class-transactions-table.php:144-149](../wp-content/plugins/esp-instanda-integration/includes/class-transactions-table.php#L144-L149) | Scope the match to the specific transaction/dedupe |
|
||||
| F-19 | Resubmit re-PUTs the stored body verbatim; stranded-lead email uses the DB-stored recipient — DB-tamper → data injection / arbitrary mail (no host SSRF; requires DB write) | [class-transactions-table.php:151-173](../wp-content/plugins/esp-instanda-integration/includes/class-transactions-table.php#L151-L173) | Re-validate the decoded body through `Field_Mapper` before resubmit |
|
||||
| F-20 | Dead code misleads operators: `increment_retry()` has no callers (Retries column always 0); `espinstanda_resubmit_success` event registered but never dispatched | [class-transaction-store.php:191-195](../wp-content/plugins/esp-instanda-integration/includes/class-transaction-store.php#L191-L195); [class-notifier.php:20](../wp-content/plugins/esp-instanda-integration/includes/class-notifier.php#L20) | Wire up or remove |
|
||||
| F-21 | `event_log` updated via non-atomic read-modify-write of the whole JSON blob → concurrent writers (async feed + admin resubmit) silently drop audit events | [class-transaction-store.php:326-336](../wp-content/plugins/esp-instanda-integration/includes/class-transaction-store.php#L326-L336) | Append via a child rows table or a DB-side JSON op |
|
||||
| F-22 | Retention cron scheduled only in the activation hook, never re-ensured (unlike the schema, which self-heals on `admin_init`) → if the event is ever lost, purge stops permanently | [esp-instanda-integration.php:41-43](../wp-content/plugins/esp-instanda-integration/esp-instanda-integration.php#L41-L43) vs [:77-82](../wp-content/plugins/esp-instanda-integration/esp-instanda-integration.php#L77-L82) | Re-ensure the schedule on `admin_init` |
|
||||
| F-23 | Theme prints ~220 lines of inline JS in `wp_footer` on every page where `GFForms` exists, and its `MutationObserver` never `disconnect()`s where the fields never appear | [functions.php:189-422](../wp-content/themes/GFeneratePress/functions.php#L189-L422) | Gate to pages embedding the form; disconnect after N tries |
|
||||
| F-24 | Uninstall option residue (F-15); stale `class-config.php:13` docblock (F-04); bundled `form/start-a-quote.json` template no longer matches the live forms; stale `M5/M9/M10` milestone comments | various | Align docs with shipped behavior |
|
||||
|
||||
---
|
||||
|
||||
## 5. Checked and cleared (including refuted candidates)
|
||||
|
||||
The verification pass actively tried to break these and could not — they are **not** defects:
|
||||
|
||||
- **Public information disclosure** — REFUTED (95%): the public failure path returns only the
|
||||
generic `esc_html`'d message; INSTANDA `response_excerpt`/`error_detail` appear only on the
|
||||
capability-gated admin detail page.
|
||||
- **Admin submenu capability "mismatch"** — REFUTED (90%): WordPress core blocks URL access on the
|
||||
menu capability *before* the render callback runs, so a user lacking the menu cap gets a core 403,
|
||||
not a leaked page. Both directions fail safe; it's a cosmetic inconsistency, not an authz hole.
|
||||
- **Dead bulk "resubmit" as a CSRF hole** — REFUTED: `WP_List_Table` auto-emits the `_wpnonce`, and
|
||||
there is no handler, so the action is an inert no-op (dead code, not a vulnerability).
|
||||
- **`$_POST['p']` unsanitized** — correct by design: a password must not be `sanitize_text_field`'d;
|
||||
it is used only to build an outbound Basic-auth probe and is not persisted or logged.
|
||||
- **Form "bricked" by theme/plugin field conflict** — REFUTED: live Form 5 has dates at 8/9 (email
|
||||
at 10), matching the theme's constants; the plugin feed maps Form 6's 7/8/9 correctly. Neither
|
||||
form is mis-validated. (The real issue is the cross-form decoupling — F-02.)
|
||||
- **Strict-`Y-m-d` date failure** — REFUTED: live date fields are `ymd_dash` (= `Y-m-d`), which
|
||||
`Field_Mapper` accepts. (Latent coupling only: changing the field format would break parsing.)
|
||||
- **Strict-typed entry-integration callbacks TypeError** — REFUTED by test: passing a string
|
||||
`form_id` did **not** throw, because `strict_types` is caller-scoped and Gravity Forms calls the
|
||||
filters in coercive mode, so a numeric-string id coerces to `int`. Columns/merge tags register
|
||||
normally in real requests.
|
||||
- **SQL injection / stored XSS** — all `$wpdb` access uses `prepare()`/`insert()`/`update()` with
|
||||
column arrays; admin output of stored data is `esc_html`/`esc_url`'d.
|
||||
- **Test-mode lock** — enforced in three independent layers (env clamp, outbound Live assertion,
|
||||
go-live gate); a mutating Live call while locked throws. All stored data is `environment=test`.
|
||||
|
||||
---
|
||||
|
||||
## 6. Live state-machine results & production-parity gaps
|
||||
|
||||
**State-machine oracle — observed vs expected (verified in-process, INSTANDA mocked; also cross-checked
|
||||
against the 13 live delivered rows and the real 400 failure row):**
|
||||
|
||||
| Response | ok | quote_ref | is_ambiguous | attempts | resulting tx row |
|
||||
|---|---|---|---|---|---|
|
||||
| `200` + quoteRef | true | set | false | 1 | `delivered` (matches 13 live rows) |
|
||||
| `200` no body | true | **null** | false | 1 | `failed`, possible_duplicate=**0** (F-17) |
|
||||
| `400` + errors | false | null | false | 1 | `failed`, deterministic (matches live row id 8) |
|
||||
| `500` persistent | false | null | **true** | 2 | `failed`, possible_duplicate=**1** |
|
||||
| transport timeout | false | null | **true** | 2* | `failed`, possible_duplicate=1 |
|
||||
|
||||
`*` the instant mock retries; a real 8 s timeout makes 1 attempt (retry budget). `create_pending →
|
||||
pending` and `mark_ambiguous → failed/possible_duplicate=1` confirmed directly.
|
||||
|
||||
**Production (Pressable) parity — VERIFIED** via read-only WP-CLI on the live `hubinsurance` site
|
||||
(`hubinsurance.mystagingwebsite.com`, site 1705596, PHP 8.5, `wp_environment_type=production`,
|
||||
currently still in INSTANDA **test** mode — i.e. pre-launch):
|
||||
|
||||
| Question | Production result | Effect on findings |
|
||||
|---|---|---|
|
||||
| Object cache present? | **yes** (`object-cache.php` drop-in present) | The per-submission lock **is** atomic cross-request in prod (it was inert locally). Narrows F-06's lock note to local only; **F-01 unaffected** (token instability, not lock backend). |
|
||||
| `SPG_INSTANDA_KEY` / credential / `SPG_INSTANDA_ENV` constants set? | **none — all `undef`** | **F-04 confirmed for prod** — the self-provisioned key (len 44) + ciphertext (len 76) both live in the prod DB. The hardened constant posture is **not** in use anywhere. |
|
||||
| `GRAVITYSMTP_SENDGRID_API_KEY` | **defined** in prod | F-08 confirmed in prod. |
|
||||
| `alert_emails` | **empty** | F-05 confirmed in prod (failure alerts disabled). |
|
||||
| Operating mode / env | `mode=UNSET`→test, `env=test` | Prod not yet live; mode clamp active. Interacts with F-03 at go-live. |
|
||||
| MySQL `sql_mode` | `ERROR_FOR_DIVISION_BY_ZERO,NO_AUTO_CREATE_USER` (non-strict, no `NO_ZERO_DATE`) | **Defuses the schema concern** — the `DEFAULT '0000-00-00 00:00:00'` column built cleanly; `wp_esp_instanda_transactions` exists on prod. |
|
||||
| INSTANDA feeds | **two active feeds**: Form **5** (`start=8,end=9,email=10`) *and* Form **6** (`start=7,end=8,email=9`) | Reframes F-02 — see below. |
|
||||
|
||||
**F-02 in production is different from local.** On prod, **Form 5 is correctly wired**: it has its
|
||||
own active feed whose mapping (`8/9/10`) matches both its field layout and the theme's hardcoded
|
||||
`8/9`, so AGENTS.md's "Form 5 is the live form" holds there. But prod *also* carries an active feed
|
||||
on the **test** Form 6, and the local copy has the feed only on Form 6 (not Form 5). So the real
|
||||
prod issue is a **stray active feed on the test form + local↔prod configuration drift**, not lost
|
||||
quotes — hence F-02 is Medium, not the local-only "wrong form" hazard.
|
||||
|
||||
---
|
||||
|
||||
## 7. Prioritized remediation roadmap
|
||||
|
||||
**Go-live blockers:**
|
||||
1. F-03 — `switch_to_live()` must set `env=live` (or drive `environment()` from the mode); otherwise
|
||||
the go-live button leaves real quotes hitting Test.
|
||||
2. F-01 — content-based dedupe key so repeat submits collapse instead of duplicating quotes.
|
||||
3. F-04 — set `SPG_INSTANDA_KEY` + credential constants in prod (verified absent today); blocking check.
|
||||
4. F-05 — PII retention/deletion for failed rows; configure alert recipients (empty in prod).
|
||||
5. F-02 — deactivate the stray INSTANDA feed on the test Form 6 in prod; align local↔prod; add the
|
||||
"one fed, theme-validated public form" go-live assertion.
|
||||
|
||||
**Soon (data-integrity & correctness):**
|
||||
6. F-06 — CAPTCHA/Akismet + outbound rate limiting on the public create.
|
||||
7. F-07 — UTC everywhere for timestamps and cutoffs.
|
||||
8. F-09 / F-11 / F-14 — close the silent-failure paths (async recording, validation throwable, insert/storage-off).
|
||||
9. F-10 / F-12 / F-17 — stop auto-retrying the non-idempotent create; block delivered-row resubmit; flag 2xx-no-`quoteRef` as ambiguous.
|
||||
10. F-08 — move/rotate the SendGrid key out of committed config.
|
||||
|
||||
**Hygiene (low risk, do when touching the files):**
|
||||
11. F-13, F-15, F-16, F-18–F-24.
|
||||
|
||||
---
|
||||
|
||||
## 8. Appendix — method & limitations
|
||||
|
||||
- **Static:** full read of `class-submission.php`, `class-config.php`, `class-api-client.php`,
|
||||
`class-transaction-store.php`, `class-go-live.php`, `class-entry-integration.php`,
|
||||
`esp-instanda-integration.php`, and the theme `functions.php`; architecture-mapped all 18 files.
|
||||
- **Adversarial verification:** one refutation-tasked agent per candidate finding + three
|
||||
completeness critics; verdicts (CONFIRMED / PLAUSIBLE / REFUTED with 0-100 confidence) drove the
|
||||
severities here. Six candidates were refuted (§5); several were downgraded.
|
||||
- **Live (read-only):** `wp eval`/`eval-file` for theme, forms 5/6, feed mapping, options, cron,
|
||||
transaction rows, filter order, object-cache state. `wp db query` is unusable on this SQLite build.
|
||||
- **Dynamic:** in-process `pre_http_request` mock drove the real `Api_Client`/`Transaction_Store`
|
||||
through success/`200`-no-body/`400`/`500`/timeout with **no external calls**; one throwaway row
|
||||
was created and deleted.
|
||||
- **Production parity:** read-only WP-CLI on the live Pressable `hubinsurance` site via the
|
||||
Pressable MCP — object cache, `SPG_INSTANDA_*` constants, options, `sql_mode`, `SHOW CREATE TABLE`,
|
||||
and the INSTANDA feeds (§6). No writes were made to production.
|
||||
- **Limitations:** no end-to-end browser pass was run, but the redirect/entry flow is evidenced by
|
||||
the 13 live delivered rows; abuse (F-06) and duplicate (F-01) behavior were confirmed from
|
||||
existing data rather than by generating new load against the real API.
|
||||
Reference in New Issue
Block a user