refactor(auth): stop pre-escaping the OAuth provider URLs
get_auth_providers_data() returned esc_url()-ed URLs while all four of its consumers (the modal, the user card, render_social_buttons and the comment form) escaped again on output. Its sibling get_frontend_bindings_data() stores the raw URL and lets the binding list escape it, so the class was inconsistent with itself. I first reported this as a double-escaping bug that broke ampersand-bearing URLs. That was wrong: I had verified it with a stub that modelled esc_url() as htmlspecialchars(). The real esc_url() runs wp_kses_normalize_entities() first, which explicitly decodes double-encoded character references, so esc_url() is idempotent on its own output. Re-checked against the WordPress source with a faithful transcription, and re-ran the popover markup: the WeChat href is ?tab=oauth&oauth_intent=login, one entity, unchanged. So the escaping was redundant rather than broken. The producer now hands out raw URLs and each surface escapes exactly once, which is where WordPress expects it and matches get_frontend_bindings_data(). Rendered output is byte-identical. Extended the auth source contract to pin the invariant: the producer stays raw and all four surfaces escape on output. Verification: node --test across widget-system, navigation, admin, asset-ledger, dev, front-submit and visual (614 tests), plus php -l.
This commit is contained in:
1 parent
c472045ce4
commit
0bee2c605e
2 files changed
+23
-1
No files matched your search
+4
-1
@@ -219,7 +219,10 @@ final class OAuth {
|
||||
'name' => $provider,
|
||||
'label' => OAuthProviderCatalog::label($provider),
|
||||
'icon' => function_exists('aksrt_brand_icon') ? aksrt_brand_icon(OAuthProviderCatalog::icon_key($provider)) : '',
|
||||
'url' => esc_url($url),
|
||||
// Raw, matching get_frontend_bindings_data(): every surface escapes
|
||||
// once on output. esc_url() is idempotent on its own output, so
|
||||
// escaping here too was redundant rather than broken.
|
||||
'url' => $url,
|
||||
];
|
||||
}
|
||||
return $result;
|
||||
|
||||
@@ -294,3 +294,22 @@ test('the top-nav user popover reproduces the reference quick-login block', () =
|
||||
assert.doesNotMatch(oauth, /oauth-row|oauth-btn|social-login-divider/);
|
||||
assert.doesNotMatch(legacyCss, /\.oauth-row|\.oauth-btn|\.social-login-divider/);
|
||||
});
|
||||
|
||||
test('provider URLs stay raw in the data layer and are escaped once per surface', () => {
|
||||
const oauth = read('src/Auth/OAuth.php');
|
||||
const shared = read('templates/parts/auth-content.php');
|
||||
const userRenderer = read('src/WidgetSystem/Renderers/UserRenderer.php');
|
||||
const comments = read('inc/helpers/aksrt-comments.php');
|
||||
|
||||
// get_auth_providers_data() hands out raw URLs, exactly like its sibling
|
||||
// get_frontend_bindings_data() does. esc_url() is idempotent on its own
|
||||
// output, so escaping here as well was redundant rather than broken.
|
||||
assert.doesNotMatch(oauth, /'url'\s*=>\s*esc_url\(\$url\)/);
|
||||
assert.match(oauth, /'url'\s*=>\s*\$url,/);
|
||||
|
||||
// Every consumer still escapes on output, so the rendered href is unchanged.
|
||||
assert.match(shared, /esc_url\(\(string\) \(\$provider\['url'\] \?\? '#'\)\)/);
|
||||
assert.match(userRenderer, /href="' \. esc_url\(\$url\)/);
|
||||
assert.match(oauth, /'<a rel="nofollow" href="' \. esc_url\(\$url\)/);
|
||||
assert.match(comments, /<a href="<\?php echo esc_url\(\$url\); \?\>" class="but hollow social-login-item ml10">/);
|
||||
});
|
||||
Reference in new issue
Block a user