Skip to content

fix: recover addon store oauth client id #1601

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

Merged
superdav42 merged 1 commit into from
Jul 1, 2026
+115 −12
Merged

fix: recover addon store oauth client id #1601

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
51 changes: 46 additions & 5 deletions inc/class-addon-repository.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,14 @@
*/
class Addon_Repository {

/**
* Legacy key used by source packages released before the OAuth secrets were
* regenerated on every release build.
*
* @var string
*/
private const LEGACY_CREDENTIAL_KEY = '4ffb3de0414a284b500b368caedf7c40f03cac215eb83b0164e91c491ced4bdf';

private string $authorization_header = '';
private string $client_id;
private string $client_secret;
Expand All @@ -32,19 +40,52 @@ public function init() {
/**
* @param string $data base64 encoded string.
*
* @return false|string
* @return string
*/
private function decrypt_value(string $data): string {
// If the site doesn't have openssl, they just won't get auto updates.
if ( ! function_exists('openssl_decrypt') || ! function_exists('openssl_cipher_iv_length')) {
return '';
}
$key = hash_file('sha256', __FILE__); // Hash of this file
$data = base64_decode($data); // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.obfuscation_base64_decode
$iv_length = openssl_cipher_iv_length('aes-256-cbc');

$data = base64_decode($data, true); // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.obfuscation_base64_decode

if (false === $data) {
return '';
}

$iv_length = openssl_cipher_iv_length('aes-256-cbc');

if (false === $iv_length || strlen($data) <= $iv_length) {

@coderabbitai coderabbitai Bot Jul 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Keep the length comparison in Yoda form.

Line 59 introduces a non-Yoda comparison in production code.

Proposed fix 
-		if (false === $iv_length || strlen($data) <= $iv_length) {
+		if (false === $iv_length || $iv_length >= strlen($data)) {

As per coding guidelines, "inc/**/*.php: Yoda conditions are required in production code."

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (false === $iv_length || strlen($data) <= $iv_length) {
if (false === $iv_length || $iv_length >= strlen($data)) {
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@inc/class-addon-repository.php` at line 59, The conditional in the Addon
repository code has a non-Yoda length comparison, which violates the production
PHP coding guideline. Update the length check in the relevant branch of the
class-addon-repository logic so the comparison stays in Yoda form, matching the
existing style used alongside the iv_length validation and preserving the same
behavior.

Source: Coding guidelines

return '';
}

$iv = substr($data, 0, $iv_length);
$cipher_text = substr($data, $iv_length);
return openssl_decrypt($cipher_text, 'aes-256-cbc', $key, 0, $iv);

foreach ($this->get_decryption_keys() as $key) {
$decrypted = openssl_decrypt($cipher_text, 'aes-256-cbc', $key, 0, $iv);

if (false !== $decrypted) {
return $decrypted;
Comment on lines +66 to +70

@coderabbitai coderabbitai Bot Jul 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Locate the target file and nearby references to decryption / key handling.
git ls-files | rg '^inc/class-addon-repository\.php$|^inc/.*\.php$|^README\.md$|^composer\.json$'

echo '--- outline: inc/class-addon-repository.php ---'
ast-grep outline inc/class-addon-repository.php --view expanded || true

echo '--- relevant symbols / strings ---'
rg -n "get_decryption_keys|openssl_decrypt|aes-256-cbc|decrypt|cipher_text|iv|credential|license|token|payload|base64|json_decode|unserialize" inc/class-addon-repository.php inc -g '*.php' -g '*.md' | sed -n '1,240p'

echo '--- target file excerpt ---'
sed -n '1,220p' inc/class-addon-repository.php | cat -n

Repository: Ultimate-Multisite/ultimate-multisite

Length of output: 50393

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo '--- inc/class-addon-repository.php (lines 1-220) ---'
sed -n '1,220p' inc/class-addon-repository.php | cat -n

echo '--- encryption / storage references ---'
rg -n "LEGACY_CREDENTIAL_KEY|decrypt_value\\(|openssl_encrypt|save_access_token|wu-save-option|wu-refresh-token|client_id|client_secret|addon repository|oauth/token" inc/class-addon-repository.php inc -g '*.php' | sed -n '1,220p'

echo '--- credential usage in request construction ---'
sed -n '220,360p' inc/class-addon-repository.php | cat -n

Repository: Ultimate-Multisite/ultimate-multisite

Length of output: 25528

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Focus on payload shape and any validation helpers that might constrain decrypted text.
rg -n "client_id|client_secret|refresh_token|api_key|api_secret|credential|encrypt|decrypt|openssl_" inc -g '*.php' | sed -n '1,260p'

echo '--- composer / runtime hints ---'
sed -n '1,220p' composer.json | cat -n

Repository: Ultimate-Multisite/ultimate-multisite

Length of output: 38096

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo '--- inc/stuff.php ---'
sed -n '1,120p' inc/stuff.php | cat -n

echo '--- encryption helper details ---'
sed -n '1,220p' inc/helpers/class-credential-store.php | cat -n

echo '--- any format validation for addon OAuth credentials ---'
rg -n "client_id.*regex|client_secret.*regex|oauth.*format|credential.*format|validate.*client_id|validate.*client_secret|sanitize_text_field\\(.*client_id|sanitize_text_field\\(.*client_secret" inc -g '*.php' | sed -n '1,200p'

Repository: Ultimate-Multisite/ultimate-multisite

Length of output: 7900

Add an authenticated marker before accepting decrypted addon credentials.
openssl_decrypt() can return a non-false string for the wrong key, which makes the loop stop before the legacy key is tried. These OAuth values are opaque, so a format check won’t reliably catch it; use a versioned/authenticated prefix (or an AEAD mode) and reject payloads without it.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@inc/class-addon-repository.php` around lines 66 - 70, The decryption loop in
class-addon-repository::decrypt should not accept the first non-false
openssl_decrypt result as valid, since a wrong key can still produce a string
and stop the legacy key fallback too early. Update the encryption/decryption
flow to include an authenticated/versioned marker in the stored addon
credentials, and in the decrypt path verify that marker before returning the
plaintext; reject any payload missing it and continue trying the remaining keys
in get_decryption_keys().

}
}

return '';
}

/**
* Gets supported OAuth credential decryption keys.
*
* @return array
*/
private function get_decryption_keys(): array {
$keys = [
hash_file('sha256', __FILE__), // Hash of this file for freshly built release archives.
self::LEGACY_CREDENTIAL_KEY,
];

return array_values(array_unique(array_filter($keys)));
}

/**
Expand Down
76 changes: 69 additions & 7 deletions tests/WP_Ultimo/Addon_Repository_Test.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,28 @@ public function tear_down() {
parent::tear_down();
}

private function get_oauth_query_args(): array {
$query = (string) wp_parse_url($this->repo->get_oauth_url(), PHP_URL_QUERY);
$args = [];

parse_str($query, $args);

return $args;
}

private function encrypt_for_addon_repository(string $plain_text, string $key): string {
$iv_length = openssl_cipher_iv_length('aes-256-cbc');

$this->assertNotFalse($iv_length);

$iv = str_repeat('1', $iv_length);
$encrypted = openssl_encrypt($plain_text, 'aes-256-cbc', $key, 0, $iv);

$this->assertNotFalse($encrypted);

return base64_encode($iv . $encrypted); // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.obfuscation_base64_encode
}

// ------------------------------------------------------------------
// Constructor
// ------------------------------------------------------------------
Expand Down Expand Up @@ -79,7 +101,7 @@ public function test_set_update_download_headers_adds_auth_for_matching_url() {
$ref->setValue($this->repo, 'Bearer test_token_123');

$parsed_args = ['headers' => []];
$url = MULTISITE_ULTIMATE_UPDATE_URL . 'some/path';
$url = MULTISITE_ULTIMATE_UPDATE_URL . 'some/path';

$result = $this->repo->set_update_download_headers($parsed_args, $url);

Expand All @@ -96,7 +118,7 @@ public function test_set_update_download_headers_ignores_non_matching_url() {
$ref->setValue($this->repo, 'Bearer test_token_123');

$parsed_args = ['headers' => []];
$url = 'https://example.com/other/path';
$url = 'https://example.com/other/path';

$result = $this->repo->set_update_download_headers($parsed_args, $url);

Expand All @@ -105,7 +127,7 @@ public function test_set_update_download_headers_ignores_non_matching_url() {

public function test_set_update_download_headers_ignores_empty_auth_header() {
$parsed_args = ['headers' => []];
$url = MULTISITE_ULTIMATE_UPDATE_URL . 'some/path';
$url = MULTISITE_ULTIMATE_UPDATE_URL . 'some/path';

$result = $this->repo->set_update_download_headers($parsed_args, $url);

Expand Down Expand Up @@ -200,9 +222,10 @@ public function test_get_oauth_url_contains_response_type() {
}

public function test_get_oauth_url_contains_client_id() {
$url = $this->repo->get_oauth_url();
// client_id parameter may have empty value (no constant defined in test env)
$this->assertStringContainsString('client_id', $url);
$args = $this->get_oauth_query_args();

$this->assertArrayHasKey('client_id', $args);
$this->assertNotSame('', $args['client_id'], 'Packaged addon-store OAuth credentials must decrypt to a non-empty client_id.');
}

public function test_get_oauth_url_contains_redirect_uri() {
Expand Down Expand Up @@ -236,8 +259,47 @@ public function test_decrypt_value_returns_string() {

// Provide data with enough bytes for IV (16 bytes) + some cipher text
$fake_data = str_repeat('A', 32); // 16 bytes IV + 16 bytes cipher text
$result = $method->invoke($this->repo, base64_encode($fake_data));
$result = $method->invoke($this->repo, base64_encode($fake_data)); // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.obfuscation_base64_encode
// Result will be empty string or decrypted string (likely empty since data is invalid)
$this->assertIsString($result);
Comment on lines +262 to 264

@coderabbitai coderabbitai Bot Jul 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Assert the fail-closed result, not just the type.

The PR hardens malformed credential payloads to return '', but this test still allows any string.

Proposed fix 
 		$result    = $method->invoke($this->repo, base64_encode($fake_data)); // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.obfuscation_base64_encode
-		// Result will be empty string or decrypted string (likely empty since data is invalid)
-		$this->assertIsString($result);
+		// Malformed payloads should fail closed.
+		$this->assertSame('', $result);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
$result = $method->invoke($this->repo, base64_encode($fake_data)); // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.obfuscation_base64_encode
// Result will be empty string or decrypted string (likely empty since data is invalid)
$this->assertIsString($result);
$result = $method->invoke($this->repo, base64_encode($fake_data)); // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.obfuscation_base64_encode
// Malformed payloads should fail closed.
$this->assertSame('', $result);
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/WP_Ultimo/Addon_Repository_Test.php` around lines 262 - 264, The test
around Addon_Repository_Test::test_... is only asserting that the decrypted
result is a string, which does not verify the fail-closed behavior. Update the
assertion to check the actual outcome of Addon_Repository::decrypt_credentials
(or the invoked method on $this->repo) for malformed payloads, and assert that
it returns an empty string rather than any string value. Keep the existing
invalid input setup, but replace the broad type check with a strict value
assertion for '' so the test matches the hardened behavior.

}

public function test_decrypt_value_supports_current_release_archive_key() {
if ( ! function_exists('openssl_encrypt') || ! function_exists('openssl_cipher_iv_length')) {
$this->markTestSkipped('OpenSSL is required to test addon credential encryption.');
}

$method = new \ReflectionMethod(Addon_Repository::class, 'decrypt_value');

if (PHP_VERSION_ID < 80100) {
$method->setAccessible(true);
}

$key = hash_file('sha256', dirname(__DIR__, 2) . '/inc/class-addon-repository.php');

$this->assertNotFalse($key);

$payload = $this->encrypt_for_addon_repository('current-client-id', $key);

$this->assertSame('current-client-id', $method->invoke($this->repo, $payload));
}

public function test_decrypt_value_supports_legacy_credential_key() {
if ( ! function_exists('openssl_encrypt') || ! function_exists('openssl_cipher_iv_length')) {
$this->markTestSkipped('OpenSSL is required to test addon credential encryption.');
}

$method = new \ReflectionMethod(Addon_Repository::class, 'decrypt_value');

if (PHP_VERSION_ID < 80100) {
$method->setAccessible(true);
}

$payload = $this->encrypt_for_addon_repository(
'legacy-client-id',
'4ffb3de0414a284b500b368caedf7c40f03cac215eb83b0164e91c491ced4bdf'
);

$this->assertSame('legacy-client-id', $method->invoke($this->repo, $payload));
}
}
Loading