Filter xmlrpc_enabled only partly works

[markoheijnen]

Currently the filter only blocks on when the login method is being used. This is not always the case.
Let's block it before a XML-RPC method is being called.

[mensmaximus]

The patch is working. Methods without login needs are now disabled if the filter is set to true. Still one can use the system capabilities (e.g. system.multicall). Although you cant query any method you can put some nice workload on a server because it is answering the request with a error message. Imho if I dont need xmlrpc and want to disable it there is no reason to expose it at all.

[markoheijnen]

As far as I can tell, this is the best WordPress can do since an API still need to act like an API, even when it's disabled. Most load will be generated by WordPress itself and not the XML-RPC. As I told to you in private is that an attack should be prevented on the server level.

In your case you could exit the XML-RPC request on a hook by checking the XMLRPC_REQUEST constant.

[mensmaximus]

A question of philosophy :-).

My concern is people read the codex which says the xmlrpc_enabled filter is the way to go if you want to disable xmlrpc. If you don't want to disable the system capabilities maybe the codex should be more clear (disables all methods, not system capabilities) and make a reference to the xmlrpc_element_limit filter for those (like me) who want to tighten security a bit more.

[markoheijnen]

I looked into it but as far as I tested it, all methods including the system capabilities are disabled.

[mensmaximus]

What you see is a 405 error message telling you xmlrpc has been disabled on this server. If you send a single system.multicall request to the server you get the 405 response for each single method call within the system.multicall which indicates that although not reported system.multicall is available.

[markoheijnen]

I still don't see it. The response I get is the same for methods that do exists and which don't.
What kind of library do you use for testing this out? Maybe the library have fallback for the system.multicall method.

That said, the current patch disables the XML-RPC completely and though I get you, I don't see any reason to do more here.

[SergeyBiryukov]

Introduced in [21804].

[dd32]

Will this patch still allow Pingbacks to be used? I have a feeling that's why it only blocked authenticated requests

[markoheijnen]

It will not. I can understand why it was done at that time since it was disabled by default. Never thought about it when writing the patch now. I can add a check for that. We could also add a filter to disable the pingbacks in a good way.

[dd32]

Okay, so then the original intention of [21804] was to match the XML-RPC user option, which only disabled the content-editing components (which was authenticated requests).

We can't change the behaviour of that filter now to also block pingbacks. What we could do instead is add a new way to completely remove XML-RPC all together, but I'm not sure of the intentions behind that (as this ticket doesn't have any context as to where it came from).

IMHO this is an enhancement, and not 4.5 material.

[markoheijnen]

I can easily add a check for that one method.
You can call methods now that aren't behind the login. So the filter is broken hence it's a bug. That same goes for the user option.

[dd32]

I think that's the intention. An unauthenticated endpoint is designed for public consumption. If it's not, it should have authentication.

The filter is named badly which leads to that confusion, xmlrpc_enabled is not Is XML-RPC accessible? It's Are XML-RPC Editing functions enabled?

[markoheijnen]

Ignore pingback methods

[markoheijnen]

I understand what you are saying but reading the doc blocks and old description of the option doesn't say that it would be the case. Also for just WordPress core there isn't a lot of difference between enabled and editing functions enabled.

I added a version that checks for the pingback methods.

[mensmaximus]

Replying to dd32:

We can't change the behaviour of that filter now to also block pingbacks. What we could do instead is add a new way to completely remove XML-RPC all together, but I'm not sure of the intentions behind that (as this ticket doesn't have any context as to where it came from).

The request came originally from me. I contacted @markoheijnen on slack due to the nature of the issue I discovered. As you already said in an other comment this filter is named badly and users get a bad idea from what it is doing. The main reason behind it is to have an option to "really" disable/switch off/remove XML-RPC if a user does not need it. This has different reasons like security, philosophy and architectural design (take the one you like most).

My suggestion was to filter the server_request:

<?php
public function serve_request() {
        $enabled = apply_filters( 'xmlrpc_enabled', true );
        if ( $enabled ) {
                $this->IXR_Server($this->methods);
        }
}

I understand your concern about the history of the filter and the backward compatibility. Creating a new filter instead would be reasonable.

[mensmaximus]

Replying to markoheijnen:

I added a version that checks for the pingback methods.

The initial intention was different. The patch as it is now is not needed because its behavior would be the same as filtering all methods with the 'xmlrpc_methods' filter like:

<?php
add_filter( 'xmlrpc_methods', 'remove_xmlrpc_methods');
function remove_xmlrpc_methods( $methods ) {
    foreach ( $methods as $key => $value) {
        if ( strpos( $value, 'pingback' ) === false ) {
            unset( $methods[$key] );
        }
    }
    return $methods;
}

As stated before the intention is to completely disable XML-RPC. If disabled, no request should be processed from IXR and there should be no response to the client.

[redsweater]

Replying to mensmaximus:

As stated before the intention is to completely disable XML-RPC. If disabled, no request should be processed from IXR and there should be no response to the client.

Just to chime in here: I think you should strongly consider preserving a boilerplate error response such as the one that is currently vended. There is a practical support benefit to users of clients who rely upon XML-RPC being enabled. If it just fails without a response people will be confused and pursue other troubleshooting measures when the site could have just informed them through an error that XML-RPC is disabled.

[markoheijnen]

That's why I believe the second patch is a nice solution between completely disabling the XML-RPC and what is currently there.

The initial intention was different. The patch as it is now is not needed because its behavior would be the same as filtering all methods with the 'xmlrpc_methods' filter like:

<?php
add_filter( 'xmlrpc_methods', 'remove_xmlrpc_methods');
function remove_xmlrpc_methods( $methods ) {
    foreach ( $methods as $key => $value) {
        if ( strpos( $value, 'pingback' ) === false ) {
            unset( $methods[$key] );
        }
    }
    return $methods;
}

The code does behave differently because it will reply that the method doesn't exists. Also the system.* methods can still be performed.

[mensmaximus]

Replying to markoheijnen:

That's why I believe the second patch is a nice solution between completely disabling the XML-RPC and what is currently there.

At least the patch is far better than what the filters does today. However the codex needs to be updated to make sure users know what the filter does.

[jorbin]

@dd32 Can you review the latest patch?

[dd32]

@jorbin Sure, it allows Pingbacks through, but still has the potential to break XML-RPC use-cases for any public XML-RPC endpoints anyone has on their site while disabling the core editing/management endpoints.

I refer back to comment:12 and comment:14 - This doesn't seem like an appropriate change or direction IMHO.

[markoheijnen]

Someone has to disable it first, so the "breakage" is expected then.

[jorbin]

I think I agree with @dd32. This is a case where the filter is poorly named. I think the proper solution here is to update the documentation to make it clearer what this filter does rather than to change it's behavior 10 versions after being introduced. If someone wants to create some docs, I think we can still get this in for 4.5.

[markoheijnen]

This is really disappointing. Everyone who is using this filter that I knew about expects the behaviour of added the patch. Changing this can't hurt at all since it doesn't effect the default behaviour of WordPress.

[jorbin]

Basing our decisions upon only the users and developers we have specifically spoken is fairly poor idea. Changing the behavior of an API is something that we should do without clearly articulating and sharing well in advance. At a minimum, it's something that should never happen when we are half way through beta, but ideally it's something we never do period. Even if the original intent of this filter was to shut down XML-RPC completely, the fact that it has behaved this way for 9 versions sets the expectation that it doesn't do that.

[markoheijnen]

I totally get what you are saying and I do understand how APIs work and that you should never change the behavior of an API. In this case it's not a behavior change or at least not a breaking change since people using it should already checking if the API is functional since people can delete is or block it.

Closing this as wont-fix is not a good idea but do get that it's to late for 4.5. The only impact of causing issues I can see is people using Jetpack but my feeling is that people who are disabling the XML-RPC are not using it.

[redsweater]

I appreciate the frustration @markoheijnen expresses about the filter not seeming to fulfill its promise, but I definitely agree that preserving the status quo in this case is a safer and possibly better choice.

@markoheijnen Your argument that it only affects people who "disable it" is true, but we need also to consider that the chosen granularity of "disabling it" has a ripple effect not only on any custom site developer who chooses to disable it, but also on plugin developers for example security plugins, where the mass disabling of features in WordPress can have wide-reaching implications.

I think it's worthwhile for WordPress to consider the effects of disabling filters both when the disable capability is first added, and, as in this case, when a proposal is made to change that behavior.

In summary I agree that changing the behavior of this filter would be pulling the rug out of plugin developers and client developers who have come to expect a very specific behavior in this case.

[redsweater]

Getting to the root of what I think the intended goal is of blocking XMLPRC completely, I wonder if a new filter that blocks all API access, whether it be via XMLRPC or REST, would be suitable? If I understand it correctly, this patch wouldn't block access to to many of the same functionalities XMLRPC exposes, after the WP REST API is integrated. Maybe a better "shut it all down" filter would express the intent that any such API should be blocked?

In that case, FWIW, I would still recommend that a boilerplate error response be returned for every such "shut down" API interface.

[markoheijnen]

I have made those consideration and the only issue I can see is Jetpack since it's not obvious that it's using the XML-RPC for sending/receiving information. Maybe some other plugins do the same but in general they build custom endpoints for their needs.

Even if we disagree that we can't change the current filter then a new one should be added. I'm fine with your idea of having a filter that has the XML-RPC or REST-API as a argument. And yes, the XML-RPC error that we return today, still should be there. For the REST API it would be hiding the exposure of the API.

[mensmaximus]

Replying to redsweater:

Getting to the root of what I think the intended goal is of blocking XMLPRC completely,

Correct. The whole reason for contacting @markoheijnen was to tell that the filter does not what it "says" it would do. It does not disable xmlrpc it just removes methods with login needs.

And because any other method without login needs is still available users who disable xmlrpc for security reasons get fooled about the security of their installation.

Whether the filter gets patched as @markoheijnen already did or a new filter is introduced does not matter. What does matter is the fact that xmlrpc cant be switched off completely at the moment although the filter connotes this.

[georgestephanis]

The only impact of causing issues I can see is people using Jetpack but my feeling is that people who are disabling the XML-RPC are not using it.

Yeah, that's not the case. There are a not insignificant number of folks that click 'Disable XML-RPC' or some option in a 'Security' plugin without understanding what it means, that this would suddenly break a large number of connections if we suddenly expanded the scope of what a filter actually does.

:-1: to modifying the existing filter.

(edit: pasted the wrong quote, sorry!)

[markoheijnen]

It still will most likely only effect one plugin. Also as soon as those security plugins know about this, they will start disabling the XML-RPC on their own way. When we add a better filter then they will switch to that filter. I don't see an outcome that there will not be any breakage.

[jorbin]

If someone wants to work on updated docs for xmlrpc_enabled to better describe it's actual behavior, we have a few days to do so. I think progress should be made before the next beta (so in about 6 days). If not, we should move this out of 4.5.

Removing the close tag, as I do think a doc improvement is absolutely warranted.

[mensmaximus]

Replying to jorbin:

If someone wants to work on updated docs for xmlrpc_enabled to better describe it's actual behavior,

I would have done that. But contributing to the documentation (developer.wordpress.org) is a nightmare and simply writing a contributer note seems wrong in this case because the description has to be changed.

In the old codex where you could contribute easier I even cant find anything about the xmlrpc_enabled filter.

[mensmaximus]

My suggestions

Description:
Use this filter to disable access to any xmlrpc method that needs login credentials. Contrary to its name the filter does not disable xmlrpc as a whole. Methods like pingback.ping or any other custom methods that do not need authentication will still be accessible after returning false from the filter. This is also true for the system capabilities from IXR.

Example:
add_filter( 'xmlrpc_enabled','return_false' );

Related filters:
xmlrpc_methods - filter one or more methods
xmlrpc_element_limit - set the maximum amount of xml elements within a xmlrpc request

[westi]

Replying to jorbin:

Even if the original intent of this filter was to shut down XML-RPC completely, the fact that it has behaved this way for 9 versions sets the expectation that it doesn't do that.

The original intent of the filter was to replace the previous code / UI option whose intent disable the XML-RPC api for performing logged in actions.

It was never intended to completely turn off XML-RPC because then pingbacks wouldn't work, it was all about disabling the XMLRPC publishing interface which wasn't used by a lot of people and needed a thorough code review.

[jorbin]

Replying to mensmaximus:

Replying to jorbin:

If someone wants to work on updated docs for xmlrpc_enabled to better describe it's actual behavior,

I would have done that. But contributing to the documentation (developer.wordpress.org) is a nightmare and simply writing a contributer note seems wrong in this case because the description has to be changed.

In the old codex where you could contribute easier I even cant find anything about the xmlrpc_enabled filter.

The developer docs are generated from the inline documentation. A patch committed to the file that contains the filter ( src/wp-includes/class-wp-xmlrpc-server.php ) will cause developer.wordpress.org to update when it is refreshed for the next release.

[jorbin]

Replying to westi:

Replying to jorbin:

Even if the original intent of this filter was to shut down XML-RPC completely, the fact that it has behaved this way for 9 versions sets the expectation that it doesn't do that.

The original intent of the filter was to replace the previous code / UI option whose intent disable the XML-RPC api for performing logged in actions.

It was never intended to completely turn off XML-RPC because then pingbacks wouldn't work, it was all about disabling the XMLRPC publishing interface which wasn't used by a lot of people and needed a thorough code review.

Thanks for the historical knowledge. That makes a lot of sense and further supports the decision to not change the behavior.

[DrewAPicture]

36055.3.patch​ clarifies the hook docs for the xmlrpc_enabled filter. Part props would go to @mensmaximus for comment:37.

[DrewAPicture]

Improved filter docs

[DrewAPicture]

In 37025:

Docs: Clarify documentation for the xmlrpc_enabled filter to better explain that its scope only extends to methods requiring authentication.

When the xmlrpc_enabled filter was initially introduced in [21509], it was effectively intended to replace the `enable_xmlrpc' UI option, which only controlled whether authenticated XML-RPC methods were enabled, such as for publishing actions. This change clarifies the expected behavior and adds information about ways to more granularly control XML-RPC method and request behavior with related hooks.

Part props mensmaximus.
See #21509. Fixes #36055.

[markoheijnen]

Replying to westi:

Replying to jorbin:

Even if the original intent of this filter was to shut down XML-RPC completely, the fact that it has behaved this way for 9 versions sets the expectation that it doesn't do that.

The original intent of the filter was to replace the previous code / UI option whose intent disable the XML-RPC api for performing logged in actions.

It was never intended to completely turn off XML-RPC because then pingbacks wouldn't work, it was all about disabling the XMLRPC publishing interface which wasn't used by a lot of people and needed a thorough code review.

Sure and the second patch works exactly like that. Can we at least create it as a new feature in 4.6? Having only a doc change is no change at all.

[DrewAPicture]

Replying to markoheijnen:

Sure and the second patch works exactly like that. Can we at least create it as a new feature in 4.6? Having only a doc change is no change at all.

Feel free to create a new ticket for a new feature.