CMM-1256: Fix 401 errors for self-hosted sites with application passwords#22603
CMM-1256: Fix 401 errors for self-hosted sites with application passwords#22603
Conversation
Generated by 🚫 Danger |
|
Claude finished @adalpari's task in 1m 49s —— View job Code Review Complete ✅Tasks
SummaryThis PR effectively solves the 401 authentication issue for self-hosted sites using application passwords. The implementation is clean, well-tested, and maintains backward compatibility. The changes follow a clear architectural pattern by threading headers through the request pipeline. ✅ Strengths
🔍 Issues Found1. Security: Potential credential exposure in logs (High Priority)Location: The code directly passes plain credentials to Recommendation: Verify that the network layer doesn't log headers, especially Authorization headers. Consider adding a code comment warning about this. // SECURITY: Ensure network layer does not log Authorization header
val authHeaderValue = Credentials.basic(
site.apiRestUsernamePlain, site.apiRestPasswordPlain
)2. Code Quality: Missing null safety check (Medium Priority)Location: The Current code: val authHeaderValue = Credentials.basic(
site.apiRestUsernamePlain, site.apiRestPasswordPlain
)Issue: If Recommendation: Use val authHeaderValue = Credentials.basic(
site.apiRestUsernamePlain!!, // Safe due to hasApplicationPassword() check
site.apiRestPasswordPlain!!
)Or more defensively: val username = site.apiRestUsernamePlain
val password = site.apiRestPasswordPlain
if (username == null || password == null) {
// This shouldn't happen, but handle defensively
return Error(BaseNetworkError(GenericErrorType.UNKNOWN).apply {
message = "Application password credentials missing"
})
}
val authHeaderValue = Credentials.basic(username, password)3. Documentation: Insufficient inline comments (Low Priority)Location: The early return for application password sites is a critical code path but lacks explanation of why this bypasses nonce authentication. Recommendation: Add a comment explaining the rationale: // Use Basic auth for sites with application passwords instead of nonce auth.
// Application passwords authenticate directly via REST API Basic auth and cannot
// authenticate via wp-login.php (which is required for nonce-based auth).
if (site.hasApplicationPassword()) {
return executeWithApplicationPassword(...)
}4. Code Quality: Parameter order inconsistency (Low Priority)Location: The Recommendation: For consistency and future-proofing, add 💡 Minor Suggestions5. Test Coverage: Edge case not testedLocation: Test file The tests cover the happy path for app password auth, but don't test the 404 retry logic for app password sites (which does exist in the implementation at ReactNativeStore.kt:280-291). Recommendation: Add a test case: @Test
fun `site with application password retries on 404 after discovery`() = test {
// Test that 404 error triggers discovery and retry for app password sites
}6. Code Clarity: Magic string could be a constantLocation: The string Current: Constant at line 359, used at line 267. This is actually fine as-is, but for reviewers scanning the code, it might be clearer to reference the constant explicitly in the function signature or local scope. Not a strong suggestion, current approach is acceptable. 🎯 Performance ConsiderationsNo performance concerns identified. The implementation:
📋 Testing AssessmentTest coverage is good with three dedicated tests:
Missing test case: 404 retry logic for app password sites (mentioned in issue #5 above) 📝 Documentation AssessmentPR description is excellent: Clear explanation of root cause, solution, and implementation details. Code documentation: Could be improved with inline comments explaining the authentication flow branching (mentioned in issue #3 above). No README or external docs need updating: This is an internal implementation detail. Verdict: ✅ Approved with Minor RecommendationsThis is a solid implementation that solves the stated problem effectively. The issues identified are mostly minor defensive coding suggestions and documentation improvements. The core logic is sound and well-tested. Recommended actions before merge:
Nice to have:
|
|
Addressed Claude comment here: 5e7098f |
|
|
|
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## trunk #22603 +/- ##
==========================================
+ Coverage 38.22% 38.23% +0.01%
==========================================
Files 2246 2246
Lines 112669 112717 +48
Branches 15688 15695 +7
==========================================
+ Hits 43066 43096 +30
- Misses 66026 66041 +15
- Partials 3577 3580 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@adalpari Claude is flagging the removal of the nonce unknown test as an issue. I added it back locally and it passes. What was the reason for its removal? |
dcalhoun
left a comment
There was a problem hiding this comment.
From a testing perspective, this worked well for me. The changes fixed the original issue. I did not encounter regressions when testing various site types, with both Gutenberg Mobile and GutenbergKit.
Test on non-app-password sites (nonce auth):
I was unable to create a new site without an application password. Is this still possible?
I attempted doing so on a fresh app install without a WP.com account logged in.
I don't believe that would result in the client app having the necessary username and password for authenticating via cookies, though. At that point, it would have an invalid app password and then prompt the user to create a new app password. IINM, testing cookie authentication is the target of that particular testing instructions. |
|
I suppose the required approach may be adding a site with a previous app release, then upgrading to this prototype build. |
This ^^^ We removed the non-AP authentication. So, the only way is to add a user with an old version of the app. Maybe we should support it in order to debug cases like this one. Anyway, the testing step is to prevent a regression but no, we cannot reproduce it with the current version |
ReactNativeStore used cookie-nonce auth for all non-WPCom sites, which fails for sites with application passwords since those credentials can't log in via wp-login.php. This adds Basic auth support using the site's application password credentials directly, bypassing the nonce flow for those sites. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add LongMethod and ReturnCount suppress annotations to executeWPAPIRequest and remove unused eq and Unknown imports from tests. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add defensive null checks for application password credentials to prevent potential NPE from race conditions. Expand inline documentation explaining the nonce auth bypass rationale. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add missing any() matcher for the headers parameter added to syncGetRequest and syncPostRequest in WPAPIGsonRequestBuilder. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
553c131 to
9da8188
Compare
Good catch! I think it was removed in one of the latest cleanings, so I missed it. cc @nbradbury |
|






Description
Fixes 401 errors when making GET requests to media and editor settings endpoints on self-hosted WordPress sites that use application passwords. The issue affected both the React Native Gutenberg editor and EditorSettingsStore when fetching settings.
Root cause: ReactNativeStore used cookie-nonce authentication for all non-WPCom sites by attempting to log in via
wp-login.php. However, sites using application passwords cannot authenticate viawp-login.php—application passwords only work with REST API Basic authentication. This caused nonce requests to fail silently, and subsequent API requests were sent without authentication headers, resulting in 401 errors.Solution: Added support for Basic authentication headers in the ReactNativeStore request pipeline. When a site has application password credentials (
apiRestUsernamePlainandapiRestPasswordPlain), the store now uses Basic auth directly instead of attempting the nonce flow. This bypasseswp-login.phpentirely and uses the credentials in the format that application passwords expect.Implementation details:
headersparameter throughout the request pipeline (WPAPIGsonRequestBuilder → ReactNativeWPAPIRestClient → ReactNativeStore)site.hasApplicationPassword()checkokhttp3.Credentials.basic()from the site's application password credentialsTesting instructions
Test on self-hosted WordPress site with application passwords configured:
Test on non-app-password sites (nonce auth):
Test on WP.com sites: