Skip to content

Prevent issues when license validation request fails #3346

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
Crabcyborg wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Prevent issues when license validation request fails #3346

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
104 changes: 86 additions & 18 deletions classes/models/FrmAddon.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -585,7 +585,9 @@ public function activate_defined_license() {
if ( $license && ! $this->is_active() && ! $this->checked_recently( '1 day' ) ) {
$response = $this->activate_license( $license );

if ( ! $response['success'] ) {
// Drop the license only when the API reported on it. An unreachable API says
// nothing, so the defined license is kept and checked again later.
if ( ! $response['success'] && empty( $response['inconclusive'] ) ) {
$license = '';
}
}
Expand Down Expand Up @@ -655,12 +657,21 @@ public function clear_license() {
* Don't save an invalid license.
*
* @since 6.8.3
* @since x.x Added the $is_conclusive param.
*
* @param bool|string $is_valid If license activation was successful. May be a string 'valid'.
* @param bool|string $is_valid If license activation was successful. May be a string 'valid'.
* @param bool $is_conclusive Whether the API reported on the license. False when the
* check could not reach a verdict, such as a connection
* error, in which case the saved license is left alone.
*
* @return void
*/
protected function maybe_set_active( $is_valid ) {
protected function maybe_set_active( $is_valid, $is_conclusive = true ) {
if ( ! $is_valid && ! $is_conclusive ) {
// Nothing came back about this license, so don't discard what is saved.
return;
}

update_option( $this->option_name . 'active', $is_valid );

if ( $is_valid ) {
Expand Down Expand Up @@ -1002,6 +1013,12 @@ private function is_license_revoked() {

$response = $this->get_license_status();

if ( ! empty( $response['inconclusive'] ) ) {
// The API did not report on this license, so there is nothing to act on. A
// license that activated before stays in place until the API says otherwise.
return;
}

if ( ! empty( $this->save_status['response_code'] ) && 429 === $this->save_status['response_code'] ) {

@franky-the-going-merry franky-the-going-merry Bot Sep 16, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking: this 429 guard is now unreachable.

A 429 from the license server can't produce an allowlisted status. send_mothership_request() returns either There was a 429 error: ... for a non-JSON body, or the JSON's error value — and no rate-limit code is in get_reported_license_statuses(). Either way inconclusive stays true and the new check at :1016 returns first.

That's the correct outcome, but leaving the dead branch below implies a case that can no longer reach it. Drop it — or, if the intent is to keep response-code handling explicit, that's an argument for deriving inconclusive from $this->save_status['response_code'] in the first place (see the comment on get_reported_license_statuses()).

// If we got a rate limit response, don't clear the license.
return;
Expand Down Expand Up @@ -1169,7 +1186,7 @@ private function activate_license( $license ) {
$response['success'] = true;
}

$this->maybe_set_active( $is_valid );
$this->maybe_set_active( $is_valid, empty( $response['inconclusive'] ) );

@franky-the-going-merry franky-the-going-merry Bot Sep 16, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking. This guard protects the active option, but the key was already overwritten before the request went out, so an unreachable API leaves the two describing different licenses.

activate_license() opens with:

$this->set_license( $license );   // :1162 — writes edd_<slug>_license_key
$this->license = $license;

When the check comes back inconclusive, maybe_set_active( false, false ) returns without touching edd_<slug>_license_active.

So: a site licensed under key A, someone pastes key B, the API happens to be unreachable. Result — key = B (never validated), active = valid (earned by A). The site reports itself licensed under a key the server never accepted.

There is no self-correction path. is_license_revoked() throttles on transient_key(), an md5 of the license, so B starts with a clean throttle — but when that weekly check does run and B comes back invalid, :1027 only clears on revoked/blocked/disabled/missing. invalid isn't in that set, so the stale active flag stays indefinitely.

Fix: don't persist the key until the check is conclusive. FrmAddon::set_license() is called from exactly one place — :1162 — so moving it below the get_license_status() call has no other caller to account for. Failing that, capture the previous key at the top of activate_license() and restore it when maybe_set_active() bails.

}

$this->update_last_checked( (bool) $is_valid );
Expand Down Expand Up @@ -1197,14 +1214,46 @@ private function die_if_not_allowed() {
}

/**
* The statuses the API sends to describe the license itself.
*
* Anything else that comes back, a connection error message or an unreadable
* body, says nothing about the license and must not be treated as an answer
* about it. See the inconclusive handling in get_license_status.
*
* @since x.x
*
* @return array<string>
*/
private function get_reported_license_statuses() {
return array(
'valid',
'invalid',
'expired',
'revoked',
'blocked',
'disabled',
'missing',
'inactive',
'site_inactive',
'no_activations_left',
'invalid_item_id',
'item_name_mismatch',
);
Comment on lines +1228 to +1241

@franky-the-going-merry franky-the-going-merry Bot Sep 16, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking. This allowlist is missing four rejection codes the license server actually returns, and every omission fails open.

Ground truth is edd-software-licensing v3.9.7, src/Licenses/Actions/Activate.php::get_activation_result(). Its complete error set for edd_action=activate_license is:

missing, missing_item_id, disabled, key_mismatch, expired,
missing_url, bundle_activation_not_allowed, no_activations_left,
invalid_item_id, item_name_mismatch

missing_item_id, key_mismatch, missing_url and bundle_activation_not_allowed are not in this list. send_mothership_request() collapses an error response to that bare string at :1435:

$message = is_array( $json_res ) && isset( $json_res['error'] ) ? $json_res['error'] : $json_res;

so each of those four now takes the elseif ( $status ) branch at :1292 with inconclusive left true.

Concrete failure: a customer activates a bundle license. The server replies bundle_activation_not_allowed. maybe_set_active( false, false ) returns early, so the rejected key stays in edd_<slug>_license_key, the previous active flag is untouched, and the user sees the raw code string because get_messages() has no entry for it. Before this PR that same response cleared the license. key_mismatch is the everyday version of the same thing — a key pasted with the wrong case or stray whitespace.

The list has the opposite problem too: revoked, blocked, inactive and site_inactive aren't in activate_license's result set at all.

Root fix: don't classify on the status string. The transport already knows, and send_mothership_request() already computes it — is_wp_error( $resp ), and wp_remote_retrieve_response_code( $resp ), which it stores as $this->save_status['response_code']. That is the same value is_license_revoked() reads at :1022. A reply that parsed as JSON from the server is conclusive whatever string it carries; a WP_Error or an unparseable body is not. That derivation can't drift when the server adds a code — this list will, and silently.

If the list stays for now, it at least needs the four missing codes:

Suggested change
return array(
'valid',
'invalid',
'expired',
'revoked',
'blocked',
'disabled',
'missing',
'inactive',
'site_inactive',
'no_activations_left',
'invalid_item_id',
'item_name_mismatch',
);
return array(
'valid',
'invalid',
'expired',
'revoked',
'blocked',
'disabled',
'missing',
'missing_url',
'missing_item_id',
'key_mismatch',
'bundle_activation_not_allowed',
'inactive',
'site_inactive',
'no_activations_left',
'invalid_item_id',
'item_name_mismatch',
);

}

/**
* @since x.x Added the inconclusive key, so a check that never reached the API
* can be told apart from one where the API reported on the license.
*
* @return array
*/
private function get_license_status() {
$this->set_running();

$response = array(
'status' => 'missing',
'error' => true,
'status' => 'missing',
'error' => true,
'inconclusive' => false,
);
Comment on lines 1253 to 1257

@franky-the-going-merry franky-the-going-merry Bot Sep 16, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking. 'missing' can't be both the "no answer yet" placeholder and a real verdict.

It's a genuine server status — get_activation_result() returns error => 'missing' for a key that doesn't exist — it has an entry in get_messages(), and is_license_revoked() at :1027 treats it as grounds to clear. It is also the value $response['status'] keeps when nothing overwrites it.

The PR's own test pins the collision. license_status_provider() asserts array( 'code' => 500 ) produces inconclusive = true, status = 'missing'. Follow that through activate_license(): $response['error'] is false, so it takes the else branch at :1175, $messages['missing'] exists, and the user is told "That license key is invalid" for a check that never reached a verdict. That's the same wrong-conclusion-from-a-failed-request bug this PR exists to fix, just moved from the stored state to the message.

Use a placeholder that can't be mistaken for a verdict:

Suggested change
$response = array(
'status' => 'missing',
'error' => true,
'status' => 'missing',
'error' => true,
'inconclusive' => false,
);
$response = array(
'status' => '',
'error' => true,
'inconclusive' => false,
);

That needs the if ( ! $this->license ) branch at :1259 to set 'missing' explicitly before returning (that one is a real verdict), and activate_license() to pick a connection-problem message when inconclusive is set.

Separately, 'error' here is dead. Every return path sets it to false — :1260 on the no-license branch, and :1267 as the first statement of the try, before anything can throw — so activate_license()'s if ( $response['error'] ) at :1173 never runs. That flag was already meant to carry "the request didn't work"; the PR adds inconclusive beside it instead of repairing it. Worth collapsing to one flag while this area is open.


if ( ! $this->license ) {
Expand All @@ -1216,21 +1265,37 @@ private function get_license_status() {

try {
$response['error'] = false;
$license_data = $this->send_mothership_request( 'activate_license' );

// $license_data->license will be either "valid" or "invalid"
// Until the response is read, nothing is known about the license. The default
// status above is only a placeholder, never an answer from the API.
$response['inconclusive'] = true;
$license_data = $this->send_mothership_request( 'activate_license' );
$status = '';

// $license_data['license'] will be a status such as "valid" or "invalid". A
// string response is an error message rather than a license status.
if ( is_array( $license_data ) ) {
if ( ! empty( $license_data['license'] ) && in_array( $license_data['license'], array( 'valid', 'invalid' ), true ) ) {
$response['status'] = $license_data['license'];
$this->save_status['status'] = $license_data['license'];
$is_valid = 'valid' === $license_data['license'];
if ( ! empty( $license_data['license'] ) && is_string( $license_data['license'] ) ) {
$status = $license_data['license'];
}
} else {
$response['status'] = $license_data;
} elseif ( is_string( $license_data ) ) {
$status = $license_data;
}

if ( $status && in_array( $status, $this->get_reported_license_statuses(), true ) ) {
$response['status'] = $status;
$response['inconclusive'] = false;
$is_valid = 'valid' === $status;

if ( in_array( $status, array( 'valid', 'invalid' ), true ) ) {
$this->save_status['status'] = $status;
}
} elseif ( $status ) {
// Keep the message for display, but it is not a verdict on the license.
$response['status'] = $status;
}
} catch ( Exception $e ) {
$response['status'] = $e->getMessage();
}
}//end try

$this->update_last_checked( $is_valid );
$this->done_running();
Expand Down Expand Up @@ -1319,9 +1384,12 @@ private static function set_license_from_post() {
}

/**
* @param string $action
* @since x.x The return type covers the decoded body, which is an array when the
* API answers with JSON, and an error message string otherwise.
*
* @return string
* @param string $action The request to send, such as activate_license.
*
* @return array|string
*/
public function send_mothership_request( $action ) {
$api_params = array(
Expand Down
120 changes: 120 additions & 0 deletions tests/phpunit/misc/test_FrmAddon.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,126 @@ public function test_checked_recently() {
}
}

/**
* Builds an add-on with a saved license whose API request comes back with the
* given payload, so the license checks can run without a request leaving the
* machine.
*
* @since x.x
*
* @param mixed $payload What the API request should come back with.
*
* @return PHPUnit\Framework\MockObject\MockObject
*/
private function get_licensed_addon( $payload ) {
$addon = $this->getMockBuilder( 'FrmTestAddon' )

@deepsource-io deepsource-io Bot Sep 16, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Call to an undefined method test_FrmAddon::getMockBuilder()


The method you are trying to call is not defined, which can result in a fatal error.

@deepsource-io deepsource-io Bot Sep 16, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Call to an undefined method test_FrmAddon::getMockBuilder()


The method you are trying to call is not defined, which can result in a fatal error.

@deepsource-io deepsource-io Bot Sep 16, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Call to an undefined method test_FrmAddon::getMockBuilder()


The method you are trying to call is not defined, which can result in a fatal error.

->disableOriginalConstructor()
->setMethods( array( 'send_mothership_request', 'clear_license' ) )
->getMock();

$addon->method( 'send_mothership_request' )->willReturn( $payload );

$addon->plugin_file = FrmAppHelper::plugin_path() . '/formidable.php';
$addon->plugin_slug = 'test_license';
$addon->option_name = 'edd_test_license_license_';
$addon->license = 'TEST-LICENSE-KEY';

// Clear the weekly throttle so the check under test actually runs.
$key = $this->run_private_method( array( $addon, 'transient_key' ) );
delete_option( $key );
delete_site_option( $key );

return $addon;
}

/**
* A check that never got an answer about the license must report itself as
* inconclusive, so nothing downstream treats it as a verdict.
*
* @since x.x
*
* @covers FrmAddon::get_license_status
*
* @dataProvider license_status_provider
*
* @param mixed $payload What the API request comes back with.
* @param bool $is_inconclusive Whether the check should report no verdict.
* @param string $status The status the check should report.
*
* @return void
*/
public function test_get_license_status_only_reports_a_verdict_from_the_api( $payload, $is_inconclusive, $status ) {
$addon = $this->get_licensed_addon( $payload );
$response = $this->run_private_method( array( $addon, 'get_license_status' ) );

$this->assertSame( $is_inconclusive, ! empty( $response['inconclusive'] ) );
$this->assertSame( $status, $response['status'] );
}

/**
* @since x.x
*
* @return void mixed>, mixed>>
*/
public function license_status_provider(): \Iterator {
// The API reported on the license, so the status is a verdict.
yield 'valid' => array( array( 'license' => 'valid' ), false, 'valid' );
yield 'invalid' => array( array( 'license' => 'invalid' ), false, 'invalid' );
yield 'revoked' => array( array( 'license' => 'revoked' ), false, 'revoked' );
yield 'disabled' => array( array( 'license' => 'disabled' ), false, 'disabled' );
yield 'expired' => array( array( 'license' => 'expired' ), false, 'expired' );
yield 'no_activations_left' => array( array( 'license' => 'no_activations_left' ), false, 'no_activations_left' );
// Nothing came back about the license, so there is no verdict to report.
yield 'error payload' => array( array( 'code' => 500 ), true, 'missing' );
yield 'empty payload' => array( array(), true, 'missing' );
yield 'connection error' => array( 'You had an error communicating with the Formidable API.', true, 'You had an error communicating with the Formidable API.' );
yield 'no body' => array( null, true, 'missing' );
}

/**
* A license that activated before stays in place until the API says otherwise.
* Losing the connection is not a revocation.
*
* @since x.x
*
* @covers FrmAddon::is_license_revoked
*
* @dataProvider revoked_license_provider
*
* @param mixed $payload What the API request comes back with.
* @param bool $should_clear Whether the saved license should be dropped.
*
* @return void
*/
public function test_is_license_revoked_only_clears_on_a_reported_revocation( $payload, $should_clear ) {
$addon = $this->get_licensed_addon( $payload );

$addon->expects( $should_clear ? $this->once() : $this->never() )->method( 'clear_license' );

@deepsource-io deepsource-io Bot Sep 16, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Call to an undefined method test_FrmAddon::never()


The method you are trying to call is not defined, which can result in a fatal error.

@deepsource-io deepsource-io Bot Sep 16, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Call to an undefined method test_FrmAddon::once()


The method you are trying to call is not defined, which can result in a fatal error.

@deepsource-io deepsource-io Bot Sep 16, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Call to an undefined method test_FrmAddon::never()


The method you are trying to call is not defined, which can result in a fatal error.

@deepsource-io deepsource-io Bot Sep 16, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Call to an undefined method test_FrmAddon::once()


The method you are trying to call is not defined, which can result in a fatal error.

@deepsource-io deepsource-io Bot Sep 16, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Call to an undefined method test_FrmAddon::never()


The method you are trying to call is not defined, which can result in a fatal error.

@deepsource-io deepsource-io Bot Sep 16, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Call to an undefined method test_FrmAddon::once()


The method you are trying to call is not defined, which can result in a fatal error.


$this->run_private_method( array( $addon, 'is_license_revoked' ) );
}
Comment on lines +185 to +191

@franky-the-going-merry franky-the-going-merry Bot Sep 16, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: the two changes that actually fix the reported bug have no test.

The new coverage is for get_license_status() and is_license_revoked(). Neither is where the license was getting wiped. That happens in maybe_set_active() — the new if ( ! $is_valid && ! $is_conclusive ) { return; } guard, reached through activate_license() — and in activate_defined_license()'s new && empty( $response['inconclusive'] ). Codecov agrees: 38.89% patch coverage, 22 lines missing.

get_licensed_addon() already provides everything needed. Two cases:

  • activate_license() with a connection-error payload, on an addon whose edd_<slug>_license_active is already valid — assert that option and edd_<slug>_license_key both survive the call.
  • activate_defined_license() with the same payload — assert it returns the defined key rather than ''.

Run the first against master before adding it and confirm it fails there. That's what makes it a regression test rather than a restatement of the new code.


/**
* @since x.x
*
* @return void mixed>, mixed>>
*/
public function revoked_license_provider(): \Iterator {
// The API reported the license is no longer usable.
yield 'revoked' => array( array( 'license' => 'revoked' ), true );
yield 'blocked' => array( array( 'license' => 'blocked' ), true );
yield 'disabled' => array( array( 'license' => 'disabled' ), true );
yield 'missing' => array( array( 'license' => 'missing' ), true );
// The license is still usable, or nothing came back about it.
yield 'valid' => array( array( 'license' => 'valid' ), false );
yield 'invalid' => array( array( 'license' => 'invalid' ), false );
yield 'expired' => array( array( 'license' => 'expired' ), false );
yield 'error payload' => array( array( 'code' => 500 ), false );
yield 'empty payload' => array( array(), false );
yield 'connection error' => array( 'You had an HTTP error connecting to the Formidable API', false );
yield 'no body' => array( null, false );
Comment on lines +199 to +211

@franky-the-going-merry franky-the-going-merry Bot Sep 16, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking, but it narrows what this suite proves: these payloads aren't the shape production produces for these statuses.

send_mothership_request() only returns an array when the decoded body has no error key (:1435):

$message = is_array( $json_res ) && isset( $json_res['error'] ) ? $json_res['error'] : $json_res;

Every rejection the server reports arrives through that ternary's true branch, as a bare string. So array( 'license' => 'revoked' ) isn't what a revoked license looks like here — 'revoked' is.

That matters specifically for these four rows. Before this PR the array branch forced anything other than valid/invalid to 'missing', so the string path was the only way is_license_revoked()'s clear ever fired in production. All four rows that assert clearing use the array shape; the string shape appears only in rows that assert not clearing. Add string rows for at least revoked, disabled and missing in both providers.

Two smaller things in the same block:

  • yield 'no body' => array( null, false ) at :211 models a return send_mothership_request() can't produce — it always returns at least the 'Your License Key was invalid' default. The row passes without proving anything.
  • Both providers are annotated @return void mixed>, mixed>> (:196, and the matching line in license_status_provider()) while actually returning \Iterator. Looks like a truncated generic that should read @return \Iterator<string, array<int, mixed>>. get_licensed_addon()'s @return PHPUnit\Framework\MockObject\MockObject also needs a leading backslash.

}

/**
* @covers FrmAddon::update_pro_capabilities
*/
Expand Down
Loading