Make WordPress Core

Opened 17 years ago

Closed 16 years ago

Last modified 3 months ago

#10665 closed defect (bug) (fixed)

Protect XMLRPC against failures when WP_DEBUG enabled

Reported by: redsweater Owned by: westi
Priority: normal Milestone: 3.1
Component: XML-RPC Version:
Severity: normal Keywords: has-patch
Cc: Focuses:

Description

Further protections for xmlrpc.php to guard against errors that are printed when WP_DEBUG is enabled.

The attached patch protects against failures that occur when the optional parameters for enclosures, categories, and ping urls are omitted by the client.

Attachments (2)

XMLRPCSafety.diff​ (3.1 KB ) - added by redsweater 17 years ago.
10665.diff​ (7.2 KB ) - added by sorich87 16 years ago.

Download all attachments as: .zip

Change History (18)

#1 @redsweater
17 years ago

  • Cc jalkut@… added
  • Component General → XML-RPC
  • Owner set to josephscott

#2 @redsweater
17 years ago

I altered the patch to be especially careful that the variables being initialized are set to the same default values they would have received previously. In particular, the problematic array lookups yield initial values of NULL, which I now set explicitly as the fallback value in case the array items don't exist.

I switched from using "!empty" to "isset", so that the logic is identical to before: if the array element is set at all, it is used to initialize the variable. If it's not set, then NULL is used. For categories, an empty array is the default initialized value, which matches previous behavior as well.

#3 @nacin
17 years ago

  • Milestone Unassigned → 3.0

#4 @nacin
16 years ago

  • Milestone 3.0 → 3.1
  • Status new → assigned

#5 @nacin
16 years ago

  • Keywords needs-refresh added; has-patch removed
  • Milestone Awaiting Triage → 3.1

@sorich87
16 years ago

#6 @sorich87
16 years ago

  • Keywords has-patch added; needs-refresh removed

#7 @westi
16 years ago

  • Owner changed from josephscott to westi

I'll look into getting these committed

#8 @automattor
16 years ago

  • Resolution → fixed
  • Status assigned → closed

(In [16824]) Work around some unset variable notices in xmlrpc.php. props redsweater. props sorich87. fixes #10665

This ticket was mentioned in ​PR #10775 on ​WordPress/wordpress-develop by ​@jorgefilipecosta.


9 months ago
#9

Part of: ​https://github.com/WordPress/ai/issues/40
Inspired by the work on ​https://github.com/galatanovidiu/mcp-adapter-implementation-example/tree/experiment/layerd-mcp-tools/includes/Abilities by @galatanovidiu.

## Summary

This PR adds a core/get-current-user ability to the WordPress Abilities API. This ability returns comprehensive profile details for the current authenticated user,

## Organization

Following the pattern established in #10665 and #10747, this PR adds a new class WP_Users_Abilities in src/wp-includes/abilities/class-wp-users-abilities.php. The class is organized into:

  • Initialization: register() method that registers all user-related abilities
  • Schema Building: get_current_user_input_schema() and get_current_user_output_schema() define the ability's interface
  • Ability Registration: register_get_current_user() registers the ability with schemas and callbacks
  • Execution: execute_get_current_user() retrieves and returns current user data
  • Utilities: get_user_meta_for_response() helper for processing user meta (excluding capability-related keys)

## Key Features

  • Returns the main user fields.
  • Optional include_capabilities parameter to include all user capabilities
  • Optional include_meta parameter to include user meta fields (excludes capability-related meta)
  • Permission check requires user to be logged in
  • Exposed via REST API (show_in_rest: true)

## Test plan

  • Open wp-admin/post-new.php
  • Open the browser console and run the following examples:
// Get current user (basic)
await wp.apiFetch({
  path: '/wp-abilities/v1/abilities/core/get-current-user/run',
  method: 'POST',
  data: { input: {} }
});
  • [ ] Verify all 15 fields are returned (id, username, email, display_name, first_name, last_name, nickname, description, url, link, slug, registered_date, roles, locale, avatar_urls)
// Get current user with capabilities
await wp.apiFetch({
  path: '/wp-abilities/v1/abilities/core/get-current-user/run',
  method: 'POST',
  data: { input: { include_capabilities: true } }
});
  • [ ] Verify capabilities object is included in the response
  • [ ] Verify meta is NOT included when only include_capabilities is true
// Get current user with meta
await wp.apiFetch({
  path: '/wp-abilities/v1/abilities/core/get-current-user/run',
  method: 'POST',
  data: { input: { include_meta: true } }
});
  • [ ] Verify meta object is included in the response
  • [ ] Verify capabilities are NOT included when only include_meta is true
  • [ ] Verify capability-related meta keys (wp_capabilities, wp_user_level) are excluded from meta
// Check the ability schema
await wp.apiFetch({
  path: '/wp-abilities/v1/abilities/core/get-current-user',
  method: 'GET'
});
  • [ ] Verify the input schema includes include_capabilities and include_meta boolean properties with default false
  • [ ] Verify the output schema documents all fields with types and descriptions

​@justlevine commented on ​PR #10775:


8 months ago
#10

(Sorry I missed this one. Wish there was a way we could actually tag other core-ai folks for review. Will take it for a spin hopefully tomorrow or wednesday)

​@jorgefilipecosta commented on ​PR #10775:


8 months ago
#11

This PR was rebases and the conflicts were fixed. cc: @justlevine

​@justlevine commented on ​PR #10775:


8 months ago
#12

My biggest concern with this PR is that it's not automatically scalable or extendable. It's not obvious (to me or an LLM) why these are the particular set of user data we're returning, we're not exposing user meta that's registered with register_meta( 'user' ...) and a schema, and the permissions model doesn't seem to follow least privilege, nor do I see a clear way to extend it.

​@jorgefilipecosta commented on ​PR #10775:


8 months ago
#13

My biggest concern with this PR is that it's not automatically scalable or extendable. It's not obvious (to me or an LLM) why these are the particular set of user data we're returning, we're not exposing user meta that's registered with register_meta( 'user' ...) and a schema, and the permissions model doesn't seem to follow least privilege, nor do I see a clear way to extend it.

Hi @justlevine the plan is to expose user meta. The only reason we are not doing it yet is because I learned the lesson on ​https://github.com/WordPress/wordpress-develop/pull/10867 😅 and PR's rapidly become very huge and hard to review if we include the full features from the beginning. So the plan is to merge a simple user ability first, and then follow up right away with the meta expansion.

​@justlevine commented on ​PR #10775:


8 months ago
#14

PR's rapidly become very huge and hard to review if we include the full features from the beginning. So the plan is to merge a simple user ability first

100% agree. The nuance here is that "simple" means something a bit different for abilities than traditional progressive programming, since it's the schema that needs to scale, and we have a lot more leeway with the internals. My focus isn't why _not_ metadata, but why _yes_ to e.g. locale, link, and esp. capabilities (since it's input _and_ output), and how confident we feel we'll be able to scale it.

Like @aaronjorbin notes in https://core.trac.wordpress.org/ticket/64596#comment:9 , once things are in core _both_ back and forward compat take on a mind on its own, so we need to make sure we can iterate the schema in the holistic direction we want, like how we intentionally prepped for the possibility of nested namespaces

​@justlevine commented on ​PR #10775:


8 months ago
#15

This question came up tangentially elsewhere - I couldn't find a trac ticket for this ability , so sharing it here for completeness:

what are we doing with the existing core/get-user-data ability. Functionally, the only difference between the two is whether it's the _current_ user or a specified user, but thats

  • an implementation detail, not an api consideration
  • not self-documented from the name get-user-data versus get-user or even a namespaced user/get (although at least that one implies there's a difference from the fact that its namepaced).

​@gziolo commented on ​PR #10775:


3 months ago
#16

Thanks @jorgefilipecosta! I checked this port against the merged AI plugin version (​WordPress/ai#774), and it reflects it faithfully. The class body, schemas, permission model, and normalizers are the same, with only the expected Core adaptations (no namespace, no ai text domain, docblocks, and dropping the plugin-only override/init() hook). Test coverage matches too, minus the one plugin-only override test that has no meaning in Core. Nice work keeping the two in sync.

Before this lands, I think we should settle the two points I raised in the comments: the class naming convention (the WP_Abilities_ prefix, shared with the settings PR) and how this ability sits next to the existing core/get-user-info. Since nothing has shipped yet, this is the moment to align the field names and reuse one shared definition source, so we do not freeze two vocabularies for the same user data. That is the last step for me before approval.

The CI failures look unrelated. The run has a single failing test, test_new_admin_email_subject_filter (an admin email subject assertion), while all the abilities tests pass. It looks like test-state pollution, so a rebase or re-run should clear it.

Note: See TracTickets for help on using tickets.