Page MenuHomePhabricator

Ban a set of titles
Closed, DeclinedPublic

Description

Add the facility to return a 403 Forbidden for a set of titles in the web api.

Event Timeline

Arlolra triaged this task as Medium priority.

It could be a config setting (enableTitleBans .. or some such) that triggers the checks:

  • when enabled, the config can additionally provide either a set of oldids or a set of titles.
  • the oldids would be used the most .. titles would ban all revisions with that title (and there might be scenarios where that might be appropriate).

As discussed on IRC earlier, I think it would be nice if this kind of rate limiting / blocking was automated. Service-runner has support for distributed rate limiting (https://github.com/wikimedia/service-runner/blob/master/lib/ratelimiter.js), based on https://github.com/gwicke/limitation.

As discussed on IRC earlier, I think it would be nice if this kind of rate limiting / blocking was automated. Service-runner has support for distributed rate limiting (https://github.com/wikimedia/service-runner/blob/master/lib/ratelimiter.js), based on https://github.com/gwicke/limitation.

Ya, we could do instead .. i was mostly hesitant if we needed such a defensive solution on all the time given that parsoid is behind restbase. But, let me bring this up at the team meeting.

This follow-up task from an incident report has not been updated recently. If it is no longer valid, please add a comment explaining why. If it is still valid, please prioritize it appropriately relative to your other work. If you have any questions, feel free to ask me (Greg Grossmeier).

In the last discussion we had about this a few months back, we didn't think we wanted to do a defensive across-the-board rate limiting. The only reason to consider this is if RESTBase / ChangeProp started misbehaving and DOS-ed Parsoid. Given that all Parsoid clients hit RESTBase, I don't think it makes sense to add more defensive cruft to Parsoid to let Parsoid be accessible when RESTBase itself misbehaves.

I am happy to consider counter arguments and revise my stand on this.

But, the original idea of banning a set of titles / oldids might still be relevant for scenarios where a page triggers pathological behavior in Parsoid. However, at this point, this is a low priority issue since we continue to be behind RESTBase which has limited retries, and it implements rate limits for clients hitting it. So, the pathological behavior should be limited to the # of retries. But, in this case, it might make sense to protect against a RESTBase / ChangeProp bug wherein the page is repeatedly retried which might lead to an eventual DOS of the Parsoid cluster. A simple ban of such titles might help the cluster recover. We'll take a look at this.

This discussion came up a couple times and the consensus was that given that Parsoid is behind RESTBase, we are going to rely on RESTBase's title banning code. There isn't any real reason at this time to duplicate this defensive code.