fix: recover addon store oauth client id#1601
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR adds legacy OAuth credential decryption support to Addon_Repository by introducing a legacy credential key constant and reworking decrypt_value() to iterate through multiple candidate keys with stricter validation. Corresponding test helpers and new tests validate both current and legacy key decryption paths. ChangesMulti-key decryption support
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant AddonRepository as Addon_Repository
participant KeyBuilder as get_decryption_keys
participant OpenSSL
Caller->>AddonRepository: decrypt_value(encoded_payload)
AddonRepository->>AddonRepository: base64_decode (strict)
AddonRepository->>AddonRepository: validate IV length
AddonRepository->>KeyBuilder: get_decryption_keys()
KeyBuilder-->>AddonRepository: [current_key, legacy_key]
loop for each candidate key
AddonRepository->>OpenSSL: openssl_decrypt(ciphertext, key, iv)
OpenSSL-->>AddonRepository: plaintext or failure
end
AddonRepository-->>Caller: first successful plaintext or ''
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🔨 Build Complete - Ready for Testing!📦 Download Build Artifact (Recommended)Download the zip build, upload to WordPress and test:
🌐 Test in WordPress Playground (Very Experimental)Click the link below to instantly test this PR in your browser - no installation needed! Login credentials: |
Collaborator
Author
Summary
Investigation
Verification
aidevops.sh v3.31.17 plugin for OpenCode v1.17.13 with gpt-5.5 spent 19m and 183,175 tokens on this with the user in an interactive session. Merged via PR #1601 to main. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with 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.
Inline comments:
In `@inc/class-addon-repository.php`:
- 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.
- Around line 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().
In `@tests/WP_Ultimo/Addon_Repository_Test.php`:
- Around line 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.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 16a817fc-d999-45c5-990f-83b7e41dc3e2
📒 Files selected for processing (2)
inc/class-addon-repository.phptests/WP_Ultimo/Addon_Repository_Test.php
|
|
||
| $iv_length = openssl_cipher_iv_length('aes-256-cbc'); | ||
|
|
||
| if (false === $iv_length || strlen($data) <= $iv_length) { |
Contributor
There was a problem hiding this comment.
📐 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
Comment on lines
+66
to
+70
| foreach ($this->get_decryption_keys() as $key) { | ||
| $decrypted = openssl_decrypt($cipher_text, 'aes-256-cbc', $key, 0, $iv); | ||
|
|
||
| if (false !== $decrypted) { | ||
| return $decrypted; |
Contributor
There was a problem hiding this comment.
🔒 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 -nRepository: 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 -nRepository: 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 -nRepository: 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().
Comment on lines
+262
to
264
| $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); |
Contributor
There was a problem hiding this comment.
🎯 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.
superdav42
added
the
review-feedback-scanned
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
client_id.client_idand supports both current and legacy decryption keys.Investigation
invalid_client/No client id suppliedauthorize response.Verification
vendor/bin/phpcs inc/class-addon-repository.php tests/WP_Ultimo/Addon_Repository_Test.phpvendor/bin/phpunit --filter Addon_Repository_Testgit diff --checkaidevops.sh v3.31.17 plugin for OpenCode v1.17.13 with gpt-5.5 spent 19m and 183,175 tokens on this with the user in an interactive session.
Summary by CodeRabbit