Review followup: PR #1601 — fix: recover addon store oauth client id

[superdav42]

Unaddressed review bot suggestions

PR #1601 was merged with unaddressed review bot feedback. Each comment
below includes its file path, line number, a direct link to the inline
review comment, and a diff fence with the code context the bot was
flagging. Resolved and outdated threads are filtered out via GitHub's
GraphQL review-thread state. Read the relevant lines, decide whether
the suggestion is correct, and either apply the fix or close this issue
with a wontfix rationale.

Source PR: #1601

---

You are the triager (worker-is-triager rule)

This issue is auto-created from review bot output and dispatched
directly to you. Review bots can be wrong: hallucinated line refs, false
premises about codebase structure, template-driven sweeps without
measurements (see GH#17832-17835 for prior art and AGENTS.md
"AI-Generated Issue Quality"). Do not assume the bot is correct. Verify before acting.

You must end in exactly one of three outcomes — no fourth "hand it back
to the human" path exists. Humans approve decisions; they do not re-do
analysis.

Outcome A — Premise falsified → close the issue

1. Read the cited file:line (listed under Files to modify below).

2. If the bot's claim is factually wrong (file doesn't exist at that
   line, function doesn't behave as described, "auto-generated" section
   isn't actually auto-generated, etc.), close the issue with a
   comment in this shape:

   Premise falsified. <what the bot claimed>. <what the code
   actually shows, with a file:line citation or one-line quote>.
   Not acting.

   No PR. No further dispatch. The closing comment trains the next
   session reading this thread and the noise filter.

Outcome B — Premise correct + fix is obvious → implement and PR

1. Verify the bot's premise as above.
2. Read the Worker Guidance section below, open a worktree, implement.
3. Open a PR with Resolves #<this-issue-number> in the body
   (use THIS issue's number, not the source PR's) so merge auto-closes it.
4. Follow the normal Lifecycle Gate (brief, tests, review-bot-gate,
   merge, postflight).

Outcome C — Premise correct but approach is a genuine judgment call

Only use this path if you reach it after Outcomes A and B don't apply:
the bot's finding is real, but the fix requires a decision that is
architectural, policy, breaking-change, or otherwise genuinely outside
what you can resolve autonomously. In that case, post a decision
comment with exactly these fields:

• Premise check: one line, confirming the finding is real.
• Analysis: 2-4 bullets on the trade-offs.
• Recommended path: the option you would take if the decision were
  yours, with rationale.
• Specific question: the single decision the human needs to make
  (yes/no or pick-one, not open-ended).

Then apply needs-maintainer-review and stop. The human wakes up to a
ready-to-approve recommendation, not a blank task.

Ambiguity about scope or style is not Outcome C. Per
AGENTS.md "Reasoning responsibility", the model does the
thinking and delivers a recommendation. Only escalate what is genuinely
a maintainer-only decision.

Worker Guidance

Files to modify:

• inc/class-addon-repository.php:59
• inc/class-addon-repository.php:70
• tests/WP_Ultimo/Addon_Repository_Test.php:264

Implementation steps (Outcome B path):

1. Read the diff block under each inline comment below — it shows the
   exact code the bot was flagging. Open the file only if you need
   surrounding context beyond what the diff tail shows.
2. Read the bot's full comment below the diff — it contains the rationale
   and any suggested change.
3. Verify the premise before implementing (see Outcome A). If the premise
   is wrong, switch to Outcome A instead of burning iterations trying to
   satisfy a wrong suggestion.
4. If multiple comments target the same file, group your edits into one
   logical commit.
5. Run shellcheck / markdownlint-cli2 / project tests as appropriate.

Verification:

• Open the new PR with Resolves #<this-issue> so this followup is auto-closed on merge.
• If the bot's suggestion was incorrect, close this issue with a Outcome A comment — do not open a no-op PR.

Inline comments

coderabbitai on inc/class-addon-repository.php:59

View inline comment

-		$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) {

📐 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.

		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

coderabbitai on inc/class-addon-repository.php:70

View inline comment

+			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;

🔒 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

coderabbitai on tests/WP_Ultimo/Addon_Repository_Test.php:264

View inline comment

@@ -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);

🎯 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.

		$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 st

PR review summaries

(none)

---

aidevops.sh v3.31.19 automated scan.

[superdav42]

CLAIM_RELEASED reason=worker_complete runner=superdav42 ts=2026-07-02T09:22:33Z aidevops_version=3.31.19 opencode_version=1.17.13