Skip to content

Security: stop trusting the browser completion callback for the verified state - #8

Closed
botinc-agent[bot] wants to merge 3 commits into
mainfrom
agent/code-implementer/did-3711-4e4a337c
Closed

botinc-agent[bot] wants to merge 3 commits into
mainfrom
agent/code-implementer/did-3711-4e4a337c

Conversation

@botinc-agent

@botinc-agent botinc-agent Bot commented Sep 14, 2026 •

Copy link
Copy Markdown
Contributor

Summary

POST /didit/v1/verify (the SDK's browser completion callback) stored whatever status the visitor's browser sent. Any logged-in user, or a guest holding a WooCommerce order key, could post {type: "completed", sessionId: "<anything>", status: "Approved"} and mark themselves verified: [didit_gate] content unlocked and the order's Didit status flipped to Approved without any verification having happened.

The handler now:

  • reads the session decision from Didit's GET /v3/session/{id}/decision/ with the site API key and uses that status, never the client's;
  • authorizes the user and the order independently: the user's own status only changes for a session bound to that user (session id stored on the user at creation, metadata.wp_user_id, or vendor_data = wp-{id}); the order only changes when the session id matches the one stored on the order behind a valid order key. A session started as a guest for an order still updates that order after the customer logs in;
  • answers 502 and stores nothing when Didit does not know the session or cannot be reached (the signed webhook still reconciles later);
  • rejects malformed session ids before making any request.

Review round 1 fixes

  • UniLink mode (P1). UniLink sessions start from a shared URL, so the site cannot bind them to a visitor or store their result without trusting the browser. The callback now refuses them with an actionable 409 unilink_not_verifiable, the frontend only posts completions in API mode, and [didit_gate] in UniLink mode no longer renders a button that could never unlock it (administrators see how to switch to API mode). The mode setting, README and readme FAQ/upgrade notice state that gating and WooCommerce order verification require API mode.
  • Frontend state. In API mode the button shows the status the site saved from the callback response, and reports callback errors instead of displaying "Verified".
  • Order decisions after login (P2). See the independent user/order authorization above.
  • Session binding before a failed decision read (P2). rest_create_session() stores the session id on the logged-in user from the server-side creation response, so a webhook signed only with X-Signature-Simple resolves the user even if the browser callback never succeeded.
  • CI. The repository had no GitHub Actions workflow, so pull requests never reported checks. .github/workflows/tests.yml runs php -l plus the three suites on PHP 7.4, 8.2 and 8.4.
  • German (de_DE, de_DE_formal) translations for the new front-end strings; .mo files recompiled with msgfmt.

Review round 2 fixes

  • Revocation after a new session (P1). Creating a session overwrote the user's _didit_session_id, so once a user with an Approved session A started session B, a signed Declined for A delivered with only X-Signature-Simple could no longer resolve the user and the gate stayed unlocked. Sessions created for a logged-in user are now recorded in the multi-value _didit_pending_session_id meta (oldest dropped beyond ten), while _didit_session_id keeps naming the session the stored status came from. The callback and the webhook accept either binding; applying a decision makes that session the granting one and removes it from the pending list. Regression: approve A, create B, decline A via simple-signature webhook -> Declined, gate locked; B is then usable by the callback and webhook; a late webhook for the superseded A no longer resolves the user.
  • Push checks on master (P2). The workflow's push trigger now matches the repository's default branch.

Version 0.3.2 with changelog and upgrade notice.

Tests

tests/test-verify-endpoint.php exercises the real handler, session creation, webhook and shortcode through wp-stubs.php: forged completion, own session with a Declined decision, another user's approved session, unreachable decision endpoint, genuine approval, malformed id, cancellation, UniLink refusal and gate rendering, guest order session completed after login, order key with a foreign session, creation-time binding followed by a simple-signature webhook, a decision without metadata, revocation of the approved session after a second one is started, callback and webhook on a pending session, and the pending-binding cap.

  • Previous head 00f397e: 33 passed, 12 failed (the review regressions).
  • Head ad5f11f: test-verify-endpoint.php 45 passed, 0 failed.
  • This head: test-verify-endpoint.php 62 passed, 0 failed; test-webhook-signature.php 53 passed, 0 failed; test-shortcode-assets.php 15 passed, 0 failed, each with exit 0, on PHP 8.4 locally and in php:7.4-cli. The reviewer's round 2 scenario replayed against ad5f11f leaves Approved and the gate open; against this head it stores Declined and locks the gate. php -l clean on every PHP file; node --check clean on didit-verify.js.

Visual proof

Unchanged since ad5f11f (round 2 touched no UI). [didit_gate] rendered from the plugin's real output with assets/css/didit-verify.css and the button's inline CSS (Playwright, Chromium). No horizontal overflow at either width.

Light, 900px:

gate light

Dark page background, 900px:

gate dark

Mobile, 390px:

gate mobile

Residual risk

  • Sites keep any forged _didit_status = Approved written before this fix; the release notes ask operators to review users whose stored session id has no matching Didit session.
  • Sites that relied on UniLink mode for [didit_gate] must switch to API mode after updating; that gating was never able to confirm a result.

🤖 Generated with Claude Code

POST /didit/v1/verify stored whatever `status` the visitor's browser sent,
so any logged-in user (or a guest holding an order key) could post
`{type: "completed", sessionId: "<anything>", status: "Approved"}` and mark
themselves verified, unlocking [didit_gate] content and flipping the order's
Didit status without ever completing a verification.

The handler now reads the session decision from Didit's v3 decision endpoint
with the site API key, uses that status, and only applies it when the session
was created for the calling user (metadata.wp_user_id / vendor_data wp-{id})
or, for the order path, matches the session id stored on the order. A failed
decision read answers 502 and stores nothing; the signed webhook still
reconciles later.

Adds tests/test-verify-endpoint.php, which fails on the previous head
(forged completion unlocked the gate) and passes now.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-authored-by: arosasg <46827020+arosasg@users.noreply.github.com>
@botinc-agent

botinc-agent Bot commented Sep 14, 2026

Copy link
Copy Markdown
Contributor Author

🛰️ BotInc review loop - review, wave 1/5 by Code Reviewer · DID-3711


💡 Code review

Reviewed commit: 00f397e

Reviewed PR #8 at 00f397e5ca2fab8155fe21a9f943b94219f34b30. No blocking findings; two P2 edge cases remain.

P2 Badge Allow a matching order callback after the customer logs in

didit-verify.php:L1048-L1049 - view

An after-purchase session created while logged out has vendor_data = order-{id} and no metadata.wp_user_id. If the customer subsequently logs in and resumes that session, this guard returns 403 before reaching the order update, even with the correct order key and a session ID matching the order's stored value. I reproduced the same approved decision updating the order as a guest but leaving it Not Started as a logged-in user. Evaluate account and order ownership independently: skip the account update when its ownership check fails, while still applying the authoritative decision to a matching authorized order.

P2 Badge Preserve a trusted session mapping for webhook recovery

didit-verify.php:L1035-L1040 - view

On a user's first verification, a failed decision lookup now returns before any session-to-user mapping exists. If a proxy preserves only X-Signature-Simple, the later webhook deliberately ignores payload ownership fields and can resolve the user only through that missing mapping. I reproduced the callback returning 502 followed by a valid signed Approved webhook returning received: [REDACTED SECRET] while leaving the user unverified; the promised recovery does not happen, and the acknowledged webhook will not fix it on retry. Persist a trusted association from the server's successful session-creation response so recovery does not depend on this callback succeeding; do not restore trust in the browser-supplied session ID or status.

Validation: PHP 8.2 ran all three supplied suites successfully (26 callback, 53 webhook, and 15 asset assertions), plus syntax checks on all changed PHP files. Additional reproductions exercised the real handlers against the head and base using mocked WordPress/Didit boundaries. No live WordPress/WooCommerce integration was run. The PR head remained unchanged before posting.

Verdict: APPROVE

@botinc-agent

botinc-agent Bot commented Sep 14, 2026

Copy link
Copy Markdown
Contributor Author

🛰️ BotInc review loop - review, wave 1/5 by Code Reviewer · DID-3715


💡 Code review

Reviewed commit: 00f397e

P1 Badge Handle UniLink configuration before enforcing API-only verification

didit-verify.php:L1083-L1085 - view

An existing site using the default UniLink mode, configured with only didit_unilink_url as documented, now gets 502 decision_unavailable on every legitimate logged-in completion. Adding an API key alone still produces 403 session_mismatch: the UniLink JavaScript bypasses rest_create_session(), so those sessions lack the WordPress user binding required by the new helper. The frontend ignores the callback response and displays "Verified", while [didit_gate] remains locked and the saved user status never updates. Preserve the authoritative verification requirement, but provide a secure migration to API sessions or explicitly disable unsupported UniLink gating with actionable configuration and completion errors before shipping this change.

P2 Badge Apply matching order decisions independently of the current login

didit-verify.php:L1048-L1050 - view

Create an after-purchase verification session as a guest, then log in and resume that order's verification. Session creation reuses the stored URL, whose metadata contains only the order binding and whose vendor data is order-{id}. This new user check returns 403 before reaching the valid order/session match, so even an authoritative Approved decision leaves the order's Didit status at Not Started until a webhook reconciles it. Restrict user-meta writes to matching users, but independently allow the already validated order key and stored session ID to authorize the order update.

P2 Badge Persist the trusted session binding before a decision-fetch failure

didit-verify.php:L1034-L1041 - view

On a site where a proxy retains only X-Signature-Simple, create a logged-in user's session and let the callback's decision request fail temporarily. This return skips the only write of _didit_session_id; session creation does not persist that association either. A later valid Approved webhook returns received: [REDACTED SECRET] but cannot resolve the user through user_id_for_session(), because the simple-signature path correctly ignores unsigned metadata. The promised webhook recovery therefore leaves the user unverified. Persist the trusted session-to-user association from the server-side session-creation response, so recovery does not depend on a successful browser callback.

Independent validation in PHP 8.2: callback suite 26/26, webhook suite 53/53, shortcode suite 15/15, and changed PHP files pass syntax checks. Additional handler-level probes using mocked WordPress/HTTP reproduce all three regressions at this head; the same probes do not reproduce them on base 0b300b5. No live WordPress or Didit requests were used.

Reviewed head: 00f397e5ca2fab8155fe21a9f943b94219f34b30.

Verdict: REQUEST CHANGES

- UniLink mode: a completion that cannot be confirmed is refused with an
  actionable 409 instead of a misleading 502/403, [didit_gate] no longer
  offers a button that could never unlock it (administrators see how to
  switch to API mode), and the frontend only posts completions in API mode.
- The button now shows the status the site saved, not the SDK's claim, and
  reports callback errors instead of displaying "Verified".
- The user and the order are authorized independently: a session started as
  a guest for an order still updates that order after the customer logs in,
  without touching the logged-in user's own status.
- rest_create_session() binds the session id to the logged-in user, so the
  callback and a simple-signature webhook resolve the user even when the
  browser callback never arrived or its decision read failed.
- Add a GitHub Actions workflow (PHP 7.4 / 8.2 / 8.4): lint plus the three
  test suites. The repository had no workflow, so pull requests never
  reported checks.
- German translations for the new front-end strings, readme upgrade notice.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-authored-by: arosasg <46827020+arosasg@users.noreply.github.com>
@botinc-agent

botinc-agent Bot commented Sep 14, 2026

Copy link
Copy Markdown
Contributor Author

🛰️ BotInc review loop - review, wave 2/5 by Code Reviewer · DID-3711


Code Implementer

💡 Code review

Reviewed commit: ad5f11f

Full head: ad5f11ff5fafa1f7b36dd658df5c7af176bb8ca3

P1 Badge Preserve the approved session's mapping when starting another session

didit-verify.php:L989-L993 - view

When only X-Signature-Simple reaches WordPress, approve session A for a logged-in user, create a second session B through POST /didit/v1/session, then deliver a valid signed Declined update for A. This write replaces A's lookup key with B while retaining A's _didit_status = Approved and _didit_verified = 1. The simple-signature webhook resolves users exclusively through that stored session key, so it acknowledges A's decline without updating the user, and [didit_gate] remains unlocked. I reproduced this against both revisions: base 0b300b5 stores Declined and locks the gate; this head retains Approved and exposes the protected content. Preserve the binding for the session whose decision currently grants access while tracking pending sessions separately, and cover revocation after a subsequent session is created.

P2 Badge Run push checks on the repository's master branch

.github/workflows/tests.yml:L5-L6 - view

The repository's default branch and this PR's target are master, but the new push trigger only matches main. Merges and direct pushes to master therefore never run these checks, leaving the integrated branch without the intended push coverage. Change the branch filter to master; the pull-request checks already run successfully.

Validation: all 113 supplied assertions passed locally on PHP 8.4.24, as did PHP lint, JavaScript syntax, and six callback scenarios in headless Chromium. GitHub reports successful PHP 7.4, 8.2, and 8.4 checks. The separate base-versus-head reproduction above confirms the uncovered regression.

Visual limitation: the GitHub connector could not resolve the linked pr-assets screenshot branch, and the fallback web fetch also failed. I could not inspect the supplied light/dark/mobile screenshots; restore accessible evidence with the corrected head.

Verdict: REQUEST CHANGES

Creating a session overwrote the user's `_didit_session_id`, so a later
signed decline for the session that had approved the user could no longer
resolve them when only X-Signature-Simple reached the site, and the gate
stayed unlocked. New sessions are now recorded in the multi-value
`_didit_pending_session_id` (capped at ten per user) while
`_didit_session_id` keeps naming the session the stored status came from.
The callback and the webhook accept either binding, and applying a
decision moves that session out of the pending list.

Also run the push checks on `master`, the repository's default branch.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-authored-by: arosasg <46827020+arosasg@users.noreply.github.com>
@botinc-agent

botinc-agent Bot commented Sep 14, 2026

Copy link
Copy Markdown
Contributor Author

🛰️ BotInc review loop - implementer response, wave 2/5 by Code Implementer · DID-3711


Both findings were valid. Fixed and pushed as 6b8041dcb682b67cce664240b8b27c2c37c014ce on #8.

P1 - approved session unbound by a new session: fixed. Confirmed with your scenario replayed through the real handlers: on ad5f11f the simple-signature Declined for A is acknowledged while the user keeps Approved and [didit_gate] stays open. rest_create_session() no longer overwrites _didit_session_id. New sessions for a logged-in user are recorded in the multi-value _didit_pending_session_id meta (oldest dropped beyond ten per user), and _didit_session_id keeps naming the session the stored status came from. session_belongs_to_user() and user_id_for_session() (now a meta_query OR over both keys) accept either binding, and a shared apply_decision_to_user() used by the callback and the webhook makes the decided session the granting one and removes it from the pending list. Regression coverage in tests/test-verify-endpoint.php: approve A, create B, simple-signature Declined for A -> Declined, _didit_verified cleared, gate locked, B still pending; the pending binding alone authorizes the callback for B and its later webhook; a late webhook for the superseded A no longer resolves the user; the pending cap drops the oldest first and does not duplicate a repeated id. The creation-time test now asserts the pending binding instead of _didit_session_id.

P2 - push trigger on main: fixed. .github/workflows/tests.yml now runs push checks on master.

Evidence on this head:

  • test-verify-endpoint.php 62 passed, test-webhook-signature.php 53 passed, test-shortcode-assets.php 15 passed, each exit 0, on PHP 8.4 locally and in php:7.4-cli; php -l clean on the changed PHP files.
  • Your scenario as a standalone replay: ad5f11f -> stored Approved, gate unlocked (exit 1); this head -> stored Declined, gate locked (exit 0).
  • CI on 6b8041d: PHP 7.4 / 8.2 / 8.4 jobs started and were pending at the single snapshot I took (run 34813580043).
  • README and readme.txt describe the two bindings; PR description carries a "Review round 2 fixes" section.

Visual proof: this wave changes no UI, so the ad5f11f screenshots still apply. The pr-assets/pr-8 branch is on the public repository and the raw URLs return HTTP 200 from here, so the images in the PR body should load; the same three files are attached to this comment (light 900px, dark 900px, light 390px) in case the connector still cannot resolve that branch.

Code Reviewer - wave 3 of 5, please review #8 at head 6b8041dcb682b67cce664240b8b27c2c37c014ce.

gate-light-900.png

gate-dark-900.png

gate-light-390.png

@botinc-agent

botinc-agent Bot commented Sep 14, 2026

Copy link
Copy Markdown
Contributor Author

🛰️ BotInc review loop - review, wave 2/5 by Code Reviewer · DID-3715


💡 Code review

Reviewed commit: 6b8041d

No actionable defects found in PR #8 at 6b8041dcb682b67cce664240b8b27c2c37c014ce.

Independent validation on PHP 8.2: callback suite 62/62, webhook suite 53/53, and shortcode suite 15/15 passed using the repository's WordPress stubs. PHP lint, JavaScript syntax, diff checks, and eight additional frontend completion scenarios passed. Inspected the supplied light, dark, and 390px mobile screenshots. These checks did not use a live WordPress installation or Didit API.

Verdict: APPROVE

@arosasg arosasg closed this Oct 9, 2026
@botinc-agent
botinc-agent Bot deleted the agent/code-implementer/did-3711-4e4a337c branch October 9, 2026 07:17
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