fix: prevent fatal errors when LinkedIn authorization fails

[lucadobrescu]

When LinkedIn authorization fails during account connection, the plugin crashed to the WordPress critical-error screen instead of showing the LinkedIn error (issue #1098). The error-handling paths themselves fataled; this PR makes them report the failure and keep the dashboard alive.

What changed

• Rop_Linkedin_Service::authorize() — the catch block called Exception::getDescription(), which does not exist on plain exceptions, so any caught error (e.g. a failed token exchange) became a second fatal. It now falls back to getMessage() and no longer assumes HTTP_REFERER is set. The stale PHPStan baseline entry for this exact error is removed.

• Rop_Linkedin_Service::add_account_with_app() — an error payload without valid pages data reached array_pop( false ), a fatal TypeError on PHP 8. The payload is now validated (pages present, unserializes to a non-empty array, notify entry well-formed) and rejected with a logged error.

• Rop_Rest_Api::add_account_li() — previously ignored the add_account_with_app() return value and registered the service even after a failed add. It now answers with a code 400 response pointing to the Revive Social log.

• sign-in-btn.vue — the popup message handler parsed every message as account data. Malformed JSON and error payloads are now dropped with a logged error instead of being sent to the server and reloading the page.

• E2E tests — new linkedin-error-handling.spec.js covers seven scenarios: missing, garbled, and empty payloads, pages without a notify entry, notify-only pages (asserting no service gets registered), the LinkedIn error landing in the plugin log, and a happy-path add that verifies the account is registered. The two malformed-payload tests fail with a fatal on the previous code.

LinkedIn account connection flow

flowchart LR
    A[LinkedIn popup<br/>posts message] --> B{Changed:<br/>valid account<br/>payload?}:::changed
    B -- No --> C[Log error,<br/>stop loading]
    B -- Yes --> D[REST<br/>add_account_li]
    D --> E{New:<br/>payload passes<br/>validation?}:::added
    E -- No --> F[Code 400 +<br/>Revive Social log]
    E -- Yes --> G[Account added]
    H[authorize<br/>token exchange fails] --> I[Changed:<br/>log real LinkedIn error]:::changed

    classDef added fill:#1a7f37,color:#fff,stroke:#116329,stroke-width:3px
    classDef changed fill:#9a6700,color:#fff,stroke:#5c3d00,stroke-width:3px,stroke-dasharray:6 3

Loading

Note

With ROP_DEBUG on (any non-production environment) a missing-key payload is answered by the pre-existing Rop_Exception_Handler debug output instead of the JSON 400 response. The e2e test accepts both shapes; production behavior is the 400 response.

QA

1. Log in as an administrator and open the plugin dashboard at /wp-admin/admin.php?page=TweetOldPost (Revive Social entry in the admin menu). In the browser DevTools console, simulate the error payload LinkedIn hands back on a failed authorization:

   fetch(`${window.ropApiSettings.root}&req=add_account_li`, {
     method: 'POST',
     body: JSON.stringify({ id: 'czoyMToidXJuOmxpOnBlcnNvbjpFMkVURVNUIjs=', pages: btoa('error-not-account-data') }),
     headers: { 'Content-Type': 'application/json', 'X-WP-Nonce': window.ropApiSettings.nonce },
   }).then(r => r.text()).then(console.log);

   Expect: a JSON response with "code":"400" (no critical-error page, no HTTP 500 fatal).

2. Reload the dashboard page.

   Expect: the Accounts screen renders normally and LinkedIn still offers its sign-in button; no LinkedIn account was half-registered.

3. With a real LinkedIn app configured, start Sign in to LinkedIn from the Accounts tab and cancel/deny the authorization on the LinkedIn side.

   Expect: the dashboard stays functional and the LinkedIn error is recorded in the plugin log (Revive Social dashboard → Logs) instead of a WordPress critical-error screen.

🤖 Generated with Claude Code

[pirate-bot]

Plugin build for fc2e945 is ready 🛎️!

• Download build

[Copilot]

Pull request overview

Improves LinkedIn authorization failure handling to avoid fatal errors and partial account registration.

Changes:

• Validates LinkedIn authorization payloads and propagates failures through the REST API.
• Handles malformed popup messages without reloading the dashboard.
• Adds E2E coverage and updates the PHPStan baseline.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
vue/src/vue-elements/sign-in-btn.vue	Rejects malformed authorization messages.
tests/e2e/specs/dashboard/linkedin-error-handling.spec.js	Tests LinkedIn failure and success paths.
phpstan-baseline.neon	Removes resolved LinkedIn suppressions.
includes/admin/services/class-rop-linkedin-service.php	Improves exception and payload handling.
includes/admin/class-rop-rest-api.php	Prevents registration after failed account parsing.

Comments suppressed due to low confidence (1)

includes/admin/services/class-rop-linkedin-service.php:859

• The notify metadata is considered valid solely because the key exists. A payload such as ['notify_user_at' => []] is accepted and persisted, but the later timestamp comparison cannot behave as intended, so token-refresh notifications are silently disabled. Validate that notify_user_at is an integer timestamp before registering the account.

		if ( empty( $accounts_array ) || ! is_array( $notify_user_at ) || ! isset( $notify_user_at['notify_user_at'] ) ) {
			$this->logger->alert_error( 'Linkedin Error: received malformed account data, the account was not added.' );
			return false;

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[lucadobrescu]

Fixed in 50f9443 — every remaining entry in the pages array must now be an array carrying all the fields the loop reads (with a non-empty string id), and the array is re-indexed with array_values() before the loop so the index-based access can't hit missing offsets. Also validated the notify timestamp is numeric while there. Covered by a new e2e test with the exact string-entry payload from this comment.

[lucadobrescu]

Fixed in 50f9443 — $the_id must now be a non-empty string, so serialized arrays/objects are rejected before reaching str_replace()/the service id. Covered by a new e2e test sending an id that decodes to an array.

[lucadobrescu]

Fixed in 50f9443 — the test now clears the log via the get_log endpoint's force flag right before making the request, so the assertion can only be satisfied by this request's log entry.

[lucadobrescu]

Not changing this one here. The referrer-based browser-vs-cron detection predates this PR — this change only removes the undefined-index notice when the header is absent, keeping the existing behavior in both branches. Reworking the detection to key off the callback request/state would change behavior in the legacy own-app OAuth flow, which we can't exercise end-to-end (it needs a real LinkedIn OAuth round-trip), so it's out of scope for this fix. Worth a separate issue if the missing-referrer path proves to be a problem in practice.

[Soare-Robert-Daniel]

@lucadobrescu let's update this branch based on the new changes on development

[lucadobrescu]

@Soare-Robert-Daniel updated — development merged in (f8e71efd), which brought in the mocked E2E infra from #1094. Clean merge, all checks green.

One follow-up was needed. The e2e utils that came with #1094 enable pretty permalinks in the test site, so rest_url() — and therefore ropApiSettings.root — no longer carries a query string. The LinkedIn spec was appending &req=... to it, which produced

/wp-json/tweet-old-post/v8/api/&req=add_account_li

and the REST server resolved that to nothing, so all 10 LinkedIn tests failed with rest_no_route / 404 right after the merge. Fixed in 25e6034f by letting URL/searchParams place the ? or &, the same way the plugin's own fetchAJAX passes req through vue-resource's params option — so it works under either permalink mode. Only this spec used that pattern.

Verified locally against a fresh wp-env: 14/14 Playwright specs pass (the 9 LinkedIn ones plus accounts, general-settings, post-format ×2, publish-now) and PHPUnit is clean apart from Test_ROP::test_sdk, which asserts ROP_DEBUG is false and so only fails outside a production environment.

Heads up: the e2e job needed three attempts — the first two died in Setup WP Env on git clone https://github.com/WordPress/WordPress.git returning HTTP 502, before any test ran. Transient GitHub issue, unrelated to the diff; it passed unchanged on the third run.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

tests/e2e/specs/dashboard/linkedin-error-handling.spec.js:59

• These tests assume the authenticated-service store is initially empty, but setup never resets it; the only reset is at the end of the happy-path test and is skipped if that test fails after registering LinkedIn. A retry, a focused local run, or a following spec can therefore observe the leftover account and fail assertions such as not.toContain('linkedin'). Reset accounts in beforeEach so every scenario starts from a known state.

	test.beforeEach( async ( { page, admin } ) => {
		await admin.visitAdminPage( '/admin.php?page=TweetOldPost' );
		await page.waitForSelector( '.tab-view[type="accounts"]' );
	} );

[lucadobrescu]

Confirmed and fixed in edc94b6 — this was a real hole, not a theoretical one. Reproduced it first: { id: [VALID_ID], pages: [VALID_PAGES] } came back as

HTTP 500 {"code":"internal_server_error","message":"<p>There has been a critical error on this website.</p>..."}

which is exactly the response this PR exists to remove. The cause is as you describe — is_set_not_empty() routes array values through is_valid_serialize_data(), which explicitly handles arrays (if ( is_array( $data ) ), abstract :901) and reports them valid, so both fields reached base64_decode().

Both encoded fields must now be strings before anything decodes them, and there is a new e2e test sending your exact payload; it fails with the critical-error response when the guard is reverted and passes with it.

Worth noting for a separate issue: the same shape exists in the sibling services that pass array keys to is_set_not_empty() and then decode — class-rop-facebook-service.php:1055 (array( 'id', 'pages' )), plus the array( 'id' ) callers in the twitter, tumblr, mastodon, gmb and vk services. I have kept this PR to the LinkedIn path from #1098 rather than widening it.

[lucadobrescu]

Addressed the fresh Copilot review in edc94b61 — both findings were real.

1. Array-valued id/pages still fataled. is_set_not_empty() passes array values on to is_valid_serialize_data(), which handles arrays deliberately and calls them valid, so { id: [<valid>], pages: [<valid>] } cleared the guard and hit base64_decode(). Reproduced before fixing — the request returned HTTP 500 "There has been a critical error on this website", the very response this PR removes. Both fields are now required to be strings before decoding, with an e2e test using that payload (it fails with the critical error when the guard is reverted).

2. Tests did not reset the services store. Only the happy-path test cleaned up, and only when it passed, so with retries: 2 a failed run left a LinkedIn account registered and not.toContain('linkedin') became order-dependent. beforeEach now calls ropUtils.reset() — the fixture that arrived with #1094 — instead of adding another bespoke helper.

The earlier HTTP_REFERER thread I am still leaving as-is, for the reason given on that thread.

Local verification against a fresh wp-env: 15/15 Playwright specs pass (12 LinkedIn including the new one, plus accounts, general-settings, post-format ×2), PHPUnit 46/47, and phpcs clean on the changed file. The one PHPUnit failure is Test_ROP::test_sdk, which asserts ROP_DEBUG is false and so only fails outside a production environment.

One unrelated flake to flag, since it is in development rather than this PR: publish-now.spec.js asserts the transient "Posting to social media…" spinner is visible, but the queue is drained server-side and a wp-cron run can retire the entry before the assertion — it failed once and passed unchanged on the next run here. retries: 2 absorbs it in CI. The durable fix is to assert on the queue state or the captured mock requests rather than the spinner; I will fold that into the instant-sharing work for #1102 rather than widen this PR.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Suppressed comments (2)

vue/src/vue-elements/sign-in-btn.vue:902

• A malformed authorization message returns without unregistering the global message handler, unlike the adjacent parsed-error path. Because the popup response is terminal, this leaves the stale component listening for later messages from the auth origin and can route them using the old modal.serviceName. Remove the listener in the parse-error branch as well.

      } catch (e) {
        this.is_loading = false;
        Vue.$log.error('Received a malformed message from the authorization window', e);
        return;

includes/admin/services/class-rop-linkedin-service.php:139

• The new regression spec exercises only add_account_li, so this separate token-exchange catch path remains untested; tests/test-accounts.php covers other Rop_Linkedin_Service behavior but never makes authorize() catch a plain Exception. Add a public-seam PHPUnit case with a fake LinkedIn client whose token exchange throws, then assert the real exception message is logged without a fatal error.

			$description = method_exists( $e, 'getDescription' ) ? $e->getDescription() : $e->getMessage();
			$message     = 'Linkedin Error: Code[ ' . $e->getCode() . ' ] ' . $description;

[lucadobrescu]

Latest Copilot pass generated no new comments on the two fixes above. It did surface two suppressed findings — took one, declining the other.

Taken: leaked message listener (sign-in-btn.vue) — fixed in fc2e945f. Correct catch, and it is this PR's own code: the diff added two terminal failure branches to getChildWindowMessage(), but only the parsed-error one called removeEventListener. After a malformed payload the component stayed subscribed to the auth origin and would route a later message with the stale modal.serviceName. The parse branch now detaches too, matching its neighbour. npm run lint stays at 0 errors.

Declined: PHPUnit coverage for the token-exchange catch (class-rop-linkedin-service.php:139) — not reachable from PHPUnit as the code stands. Both exits from that block terminate the process:

• $this->error->throw_exception() ends in exit whenever ROP_DEBUG is true (class-rop-exception-handler.php:89-95), and ROP_DEBUG is true in every non-production environment, the test suite included;
• the referrer branch is exit( wp_redirect( ... ) ).

So a test driving authorize() into that catch would kill the runner, not assert on it. Making it testable means changing those exits, i.e. changing behaviour in the legacy own-app OAuth flow — the same reason I left the HTTP_REFERER thread alone. A fake-client seam also would not buy much here: \LinkedIn\Client is not in the dev checkout's vendor/ at all, so the double would be standing in for a class that is absent locally.

The line's actual behaviour is already pinned down: method_exists( $e, 'getDescription' ) was checked against the client's source, where LinkedIn\Exception extends \Exception does define getDescription(), so the guard exists for generic exceptions rather than for a missing method on the client's own class. Happy to open a follow-up issue for making authorize() return instead of exit if that is wanted as its own change.