-
Notifications
You must be signed in to change notification settings - Fork 93
Fix REST API 401 authentication errors by adding nonce middleware #274
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.
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
base: develop
Are you sure you want to change the base?
Fix REST API 401 authentication errors by adding nonce middleware #274
Conversation
WalkthroughAdds REST API nonce middleware authentication setup across control files. Migrates AI service API calls from native fetch to WordPress apiFetch with updated endpoint paths. Improves error handling and response normalization in API operations while maintaining backward compatibility. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/utils/aiService.js (1)
173-231: Critical: Broken error handling with unreachable code and undefined variables.The error handling in
generateContentis fundamentally broken after the migration toapiFetch:
- Undefined
response(lines 189, 202, 212): Theresponsevariable from the oldfetch()call no longer exists.apiFetchthrows errors directly rather than returning a Response object.- Undeclared
errorData(line 192): Used before declaration, causing aReferenceError.- Unreachable code (lines 222-228): This block comes after the catch ends and will never execute since the try block returns at line 184.
🔎 Proposed fix
try { const data = await apiFetch({ path: '/wp/v2/docs/ai/generate', method: 'POST', data: requestBody }); // Return in format expected by callers return { content: data.content || '', usage: data.usage || null }; } catch (error) { - let errorMessage = __('AI content generation failed. Please try again.', 'wedocs'); - - try { - const responseText = await response.text(); - - if (responseText.trim().startsWith('{')) { - errorData = JSON.parse(responseText); - // Use the message from the API, which should already be user-friendly - errorMessage = errorData.message || errorData.code || errorMessage; - - // Clean up HTML error messages (WordPress fatal errors) - if (errorMessage.includes('<p>') || errorMessage.includes('critical error')) { - errorMessage = __('A server error occurred. Please try again or contact support if the problem persists.', 'wedocs'); - } - } else { - // For non-JSON responses, provide user-friendly message - if (response.status === 500) { - errorMessage = __('A server error occurred. Please try again.', 'wedocs'); - } else if (response.status === 403) { - errorMessage = __('You do not have permission to perform this action.', 'wedocs'); - } else { - errorMessage = __('An error occurred. Please try again.', 'wedocs'); - } - } - } catch (parseError) { - // If we can't parse the error, provide a generic user-friendly message - if (response.status === 500) { - errorMessage = __('A server error occurred. Please try again.', 'wedocs'); - } else { - errorMessage = __('An error occurred. Please try again.', 'wedocs'); - } - } - - throw new Error(errorMessage); + // apiFetch already parses error responses and provides error.message + let errorMessage = error.message || __('AI content generation failed. Please try again.', 'wedocs'); + + // Clean up HTML error messages (WordPress fatal errors) + if (errorMessage.includes('<p>') || errorMessage.includes('critical error')) { + errorMessage = __('A server error occurred. Please try again or contact support if the problem persists.', 'wedocs'); + } + + throw new Error(errorMessage); } - - const data = await response.json(); - - // Return in format expected by callers - return { - content: data.content || '', - usage: data.usage || null - }; } catch (error) { throw error; }
🧹 Nitpick comments (2)
src/data/settings/controls.js (1)
3-6: Potential duplicate nonce middleware registration.This middleware setup is duplicated in
src/data/docs/controls.js(lines 3-6). If both modules are loaded on the same page,apiFetch.use()will register the nonce middleware twice, which adds unnecessary overhead to every request.Consider extracting this setup into a shared initialization module that ensures the middleware is registered only once (e.g., using a flag or a dedicated bootstrap file).
🔎 Proposed approach: shared initialization
Create a shared module like
src/utils/apiInit.js:import apiFetch from '@wordpress/api-fetch'; let initialized = false; export function initApiFetch() { if (initialized) return; if (typeof window.weDocsAdminVars !== 'undefined' && window.weDocsAdminVars.restNonce) { apiFetch.use(apiFetch.createNonceMiddleware(window.weDocsAdminVars.restNonce)); } initialized = true; }Then import and call
initApiFetch()in each controls file.src/utils/aiService.js (1)
341-366: Inconsistent API call approach:makeApiCallstill uses nativefetch.While
generateContentwas migrated toapiFetch, this method still uses nativefetchwith manual nonce handling. This creates inconsistency:
- Uses
weDocsEditorVars.noncehere vsweDocsAdminVars.restNoncein the middleware- Requires manual
X-WP-Nonceheader andcredentials: 'include'- Uses full
/wp-json/prefix instead of relative pathConsider migrating this method to
apiFetchas well for consistency. IfweDocsEditorVarscontext is required, you may need to register a nonce middleware for it or verify thatweDocsAdminVars.restNonceis available in the editor context.🔎 Proposed refactor to use apiFetch
async makeApiCall(provider, apiKey, endpoint, payload, model = null) { // Extract options from payload (provider-specific payloads vary) const options = this.extractOptionsFromPayload(provider, payload); - // Use WordPress REST API endpoint instead of direct API calls - const restUrl = '/wp-json/wp/v2/docs/ai/generate'; - - // Get nonce from localized script - const nonce = window.weDocsEditorVars?.nonce || ''; - - const response = await fetch(restUrl, { + const response = await apiFetch({ + path: '/wp/v2/docs/ai/generate', method: 'POST', - credentials: 'include', // Include cookies for authentication - headers: { - 'Content-Type': 'application/json', - 'X-WP-Nonce': nonce - }, - body: JSON.stringify({ + data: { prompt: options.prompt, provider: provider, model: model || options.model, maxTokens: options.maxTokens || 2000, temperature: options.temperature || 0.7, systemPrompt: options.systemPrompt - }) + } }); - if (!response.ok) { - // ... error handling ... - } - - const data = await response.json(); - - // Return in format expected by parseResponse - return data; + return response; }Note: This would require ensuring the nonce middleware is registered in the editor context as well.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/data/docs/controls.jssrc/data/settings/controls.jssrc/utils/aiService.js
🧰 Additional context used
🧬 Code graph analysis (1)
src/utils/aiService.js (3)
src/components/Settings/index.js (1)
settings(18-21)assets/js/frontend.js (1)
data(163-166)src/editor/AiDocWriterModal.js (1)
error(37-37)
🔇 Additional comments (3)
src/data/docs/controls.js (1)
3-6: Nonce middleware setup is correct but duplicated.This is identical to the setup in
src/data/settings/controls.js. The implementation is correct for enabling authenticated REST API calls. See the comment on the other file regarding consolidation to avoid duplicate registrations.src/utils/aiService.js (2)
17-17: Good addition of apiFetch import.The import of
apiFetchaligns with the PR objective to use WordPress's authenticated API fetch mechanism.
74-81: LGTM - Simplified settings fetch.The migration from native fetch to
apiFetchremoves boilerplate and leverages the nonce middleware for authentication.
Fix REST API 401 authentication errors by adding nonce middleware
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.