Repository navigation
fix: surface actual server error in OAuth code exchange UI - #7
superdav42 wants to merge 3 commits into
Conversation
Point wp-cli at shared WordPress 7.0-RC2 multisite dev install at ../wordpress (wordpress.local:8080). Documents reset workflow and WP-CLI usage in AGENTS.md.
apiFetch throws a plain object {code, message, data} for WP_Error
responses, not an Error instance. The instanceof Error check always
evaluated to false, so the generic fallback message was always shown
instead of the actual server error (e.g. insufficient_scope 403).
Replace err instanceof Error ? err.message : fallback with
err?.message || fallback in both handleStartOAuth and handleExchangeCode.
Also add WP_DEBUG error_log calls in PoolManager::exchangeCode() to
surface the token endpoint HTTP status and body when the exchange fails,
making it easier to diagnose whether the failure is a network error,
a non-200 response, or a malformed response body.
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 11 minutes and 25 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdded documentation and configuration files for WordPress plugin development environment, enhanced error message extraction in OAuth form handling via optional chaining, and implemented WP_DEBUG-guarded error logging in the OAuth token exchange flow. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
js/connector.jsx (1)
333-339: Inconsistent error handling inhandleRemoveandhandleRefresh.These two handlers still use the old
err instanceof Errorpattern (lines 337 and 366), whilehandleStartOAuthandhandleExchangeCodewere updated to useerr?.message. For consistency and to surface server error messages in all cases, consider applying the same fix here.♻️ Proposed fix for consistency
const handleRemove = async ( email ) => { setIsBusy( true ); setNotice( null ); try { await apiFetch( { method: 'DELETE', path: '/anthropic-max-pool/v1/accounts/' + encodeURIComponent( email ), } ); await fetchAccounts(); } catch ( err ) { setNotice( { status: 'error', - message: - err instanceof Error - ? err.message - : __( 'Failed to remove account.' ), + message: err?.message || __( 'Failed to remove account.' ), } ); } finally { setIsBusy( false ); } }; const handleRefresh = async ( email ) => { setIsBusy( true ); setNotice( null ); try { await apiFetch( { method: 'POST', path: '/anthropic-max-pool/v1/accounts/' + encodeURIComponent( email ) + '/refresh', } ); setNotice( { status: 'success', message: __( 'Token refreshed.' ), } ); await fetchAccounts(); } catch ( err ) { setNotice( { status: 'error', - message: - err instanceof Error - ? err.message - : __( 'Refresh failed.' ), + message: err?.message || __( 'Refresh failed.' ), } ); } finally { setIsBusy( false ); } };Also applies to: 362-369
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/connector.jsx` around lines 333 - 339, The catch blocks in handleRemove and handleRefresh use the old err instanceof Error pattern; update them to use the optional chaining pattern similar to handleStartOAuth/handleExchangeCode by setting the error message to err?.message with a fallback (e.g., __('Failed to remove account.') or __('Failed to refresh account.')), and keep the setNotice call and status:'error' unchanged so server-sent error strings surface consistently; locate the catch blocks inside handleRemove and handleRefresh and replace the ternary err instanceof Error ? err.message : ... with err?.message ?? __('...') as appropriate.src/OAuthPool/PoolManager.php (2)
716-718: Consider redacting or truncating the response body before logging.The response body from the token endpoint may contain sensitive information (e.g., error details that include partial tokens, account identifiers, or other diagnostic data). Even with
WP_DEBUGgated, this could end up in production logs if debug mode is accidentally left enabled.Consider truncating or sanitizing the body before logging:
🛡️ Proposed safer logging
if (defined('WP_DEBUG') && WP_DEBUG) { - error_log('[AnthropicMax] Token exchange HTTP ' . $status . ': ' . wp_remote_retrieve_body($response)); + $body = wp_remote_retrieve_body($response); + $truncated = strlen($body) > 500 ? substr($body, 0, 500) . '...[truncated]' : $body; + error_log('[AnthropicMax] Token exchange HTTP ' . $status . ': ' . $truncated); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/OAuthPool/PoolManager.php` around lines 716 - 718, The log currently writes the full HTTP response body from wp_remote_retrieve_body($response) when WP_DEBUG is true; change this to sanitize and/or truncate before calling error_log: parse the body (json_decode if JSON) and remove/replace sensitive fields like access_token, refresh_token, id_token, or any keys matching /(token|secret|key|id)/i, then stringify and truncate to a safe length (e.g., 200 chars) and log that sanitized string along with $status; update the error_log call that uses WP_DEBUG, wp_remote_retrieve_body, $response and $status to use the sanitized/truncated value instead.
724-726: Same truncation consideration applies here.This also logs the full response body, which could be large or contain unexpected content. Consider applying the same truncation approach for consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/OAuthPool/PoolManager.php` around lines 724 - 726, In PoolManager (the token exchange error handling block around the if (defined('WP_DEBUG') && WP_DEBUG) check) avoid logging the entire remote response body; instead apply the same truncation logic used elsewhere (e.g., a helper or use mb_substr/strlen checks) to limit the logged length and append an ellipsis when truncated, then log that truncated string in the processLogger/error_log call so large or unexpected response bodies are not fully emitted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@js/connector.jsx`:
- Around line 333-339: The catch blocks in handleRemove and handleRefresh use
the old err instanceof Error pattern; update them to use the optional chaining
pattern similar to handleStartOAuth/handleExchangeCode by setting the error
message to err?.message with a fallback (e.g., __('Failed to remove account.')
or __('Failed to refresh account.')), and keep the setNotice call and
status:'error' unchanged so server-sent error strings surface consistently;
locate the catch blocks inside handleRemove and handleRefresh and replace the
ternary err instanceof Error ? err.message : ... with err?.message ?? __('...')
as appropriate.
In `@src/OAuthPool/PoolManager.php`:
- Around line 716-718: The log currently writes the full HTTP response body from
wp_remote_retrieve_body($response) when WP_DEBUG is true; change this to
sanitize and/or truncate before calling error_log: parse the body (json_decode
if JSON) and remove/replace sensitive fields like access_token, refresh_token,
id_token, or any keys matching /(token|secret|key|id)/i, then stringify and
truncate to a safe length (e.g., 200 chars) and log that sanitized string along
with $status; update the error_log call that uses WP_DEBUG,
wp_remote_retrieve_body, $response and $status to use the sanitized/truncated
value instead.
- Around line 724-726: In PoolManager (the token exchange error handling block
around the if (defined('WP_DEBUG') && WP_DEBUG) check) avoid logging the entire
remote response body; instead apply the same truncation logic used elsewhere
(e.g., a helper or use mb_substr/strlen checks) to limit the logged length and
append an ellipsis when truncated, then log that truncated string in the
processLogger/error_log call so large or unexpected response bodies are not
fully emitted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 530c40c3-4245-4a82-a6fa-5622e39e7484
📒 Files selected for processing (4)
AGENTS.mdjs/connector.jsxsrc/OAuthPool/PoolManager.phpwp-cli.yml
plugins_url() with __DIR__ from inc/admin.php fails when the plugin directory is a symlink to a path outside WP_PLUGIN_DIR (e.g. a git worktree). WordPress cannot walk up to find the plugin root from the real path. Fix: define ANTHROPIC_MAX_AI_PROVIDER_URL in plugin.php using plugin_dir_url(__FILE__), which resolves via the symlink at load time and always produces the correct URL regardless of where the real files live. Use this constant in admin.php instead of plugins_url(). Also switch the script version from the static plugin version string to filemtime() so the browser cache is busted on every build.
|
Superseded — the fixes in this PR are inferior duplicates of already-merged PR #5 (getJsonSchema, #state fragment stripping). Local main was 3 commits behind origin and the session was patching stale code. Closing and resetting local main to origin/main. |
1 participant
Problem
After commit e1f3833 (scope validation), the OAuth code exchange can now fail with a 403
insufficient_scopeerror in addition to the existing 400exchange_failedpath. Both failures show the generic "Code exchange failed." message in the UI because the error handling usederr instanceof Error— butapiFetchthrows a plain object{code, message, data}for WP_Error responses, not anErrorinstance. Theinstanceofcheck always evaluated tofalse, swallowing the actual server message.Changes
js/connector.jsx— replaceerr instanceof Error ? err.message : fallbackwitherr?.message || fallbackin bothhandleStartOAuthandhandleExchangeCode. This surfaces the actual server error (e.g. the scope validation message) instead of the generic fallback.src/OAuthPool/PoolManager.php— addWP_DEBUGerror_logcalls inexchangeCode()at each null-return point (wp_error, non-200 HTTP status, bad response body). Enables diagnosis of token endpoint failures without modifying production code.Diagnosis
If you're seeing "Code exchange failed." after this PR, enable
WP_DEBUGinwp-config.phpand check the PHP error log. You'll see one of:[AnthropicMax] Token exchange HTTP 400: {"error":"invalid_grant",...}— code is expired or already used[AnthropicMax] Token exchange HTTP 4xx: ...— token endpoint rejected the request[AnthropicMax] Token exchange bad response body: ...— unexpected response formatIf the exchange succeeds (200) but scope validation fails, the UI will now show the full message: "Authorization succeeded but the token is missing the required 'user:inference' scope..." — which means the account doesn't have a Claude Max subscription.
Testing
WP_DEBUGinwp-config.phpSummary by CodeRabbit
Documentation
Bug Fixes
Chores