Admin Search allows for server DOS

[iamareebjamal]

Another problem with providing a lot of power with JSON:API is that it comes with a lot of responsibility on client side. A client can easily self DOS server with queries which it thinks are innocuous. This is what admin search does now.

First of all, it sends search filter events on every keystroke, an issue for which is already opened #3532

But that is not the main issue. Let's say I search for fossasia. This is the query it'll send to the server:

https://api.eventyay.com/v1/events?filter=[{"name":"name","op":"ilike","val":"%fossasia%"}]&get_trashed=true&include=tickets,sessions,speakers,owner,organizers,coorganizers,track-organizers,registrars,moderators&page[number]=1&page[size]=10"

See any problem? It requests the tickets, the sessions, the speakers, organizers,coorganizers,track-organizers,registrars,moderators of the event with the event itself. Just to render this:

Why anybody thought of this as a good idea is beyond me. This is already so bad that I was shocked. But OK, at least the page size is set to 10. How much data it could possibly fetch? Right?

This is the response:

700 nodes in included! 700. The response is 400 KB and the request takes 30 seconds to complete. 30 seconds It's a relief there are not a lot of events with a lot of speakers and sessions, or you could cook and eat instant ramen before the request completes.

But it's just one request, right?
Because of this request, every other request, requests of even 70 bytes are slowed down because the server is pummeled.

So what is happening? Apparently, JSON:API spec doesn't talk about restricting included items in the payload. Or the library used in server is poorly written, but basically, the results are limited to 10, but included items contain every single ticket, session and speaker in all the filtered 10 events!!!
Every . single . one
Just imagine the poor database. No wonder the server crashes so frequently.

Hundreds of speakers and hundreds of sessions are fetched and serialized and sent to frontend to render the above row for an event. And the fact that frontend does that on every keystroke doesn't help anything, just makes it countless times worst.
The web has no notion of canceling web requests, what is sent is sent. If you write FOSSASIA, now 8 requests are sent for each appending letter and the database and server are busy fetching 700 items for each request when it won't even be used. So, 7 out of 8 queries are useless.

That's why people hate ORMs, they allow such easy access to the database, that it becomes very easy to overfetch things in an incredibly inefficient fashion. Ember Data is an ORM, but a thousand times worse, because when it makes things easy to fetch(shoot yourself in the foot), the impact is not just of overfetching of data from DB (300~500 ms), it is going over the network and pummelling the DB, serializing thousands of items and then rendering a row (30 seconds), and crashing its own server.

I don't even know how to prevent it. This is a high priority issue for frontend to not fetch anything other than the event for admin search, but JSON:API is essentially a time bomb in the server, anyone can use the above query to take down the server and no amount of rate limiting can help when a single query can become a DOS vector.
The only thing I feel can fix this is to patch the flask-json-restapi library to not include more than 10 items, but it'll definitely break a lot of stuff, on both the site and apps.

Simply speaking, don't use include for one to many relationships, use specific endpoints to use paged data. So, if you want to get speakers, don't use event/1?include=speakers.
Use /event/1/speakers which is automatically paginated.

If the queries become more problematic, we'll have to disable included feature for one to many relationships. Because we don't want to send 1000 speakers if there are 1000 speakers, and there is no way to paginate on included data AFAIK. If I am wrong on any point, please let me know.

[iamareebjamal]

#637 is the first PR to introduce include parameter in filters if I am right

Start:

open-event-frontend/app/routes/admin/events/list.js

Lines 86 to 90 in 5ea0d16

	return this.store.query('event', {
	include : 'tickets,sessions,speakers',
	filter : filterOptions,
	'page[size]' : 10
	});

Current:

open-event-frontend/app/routes/events/list.js

Lines 85 to 90 in 1808bfa

	let queryString = {
	include : 'tickets,sessions,speakers,owner,organizers,coorganizers,track-organizers,registrars,moderators',
	filter : filterOptions,
	'page[size]' : params.per_page || 10,
	'page[number]' : params.page || 4
	};

[niranjan94]

@iamareebjamal very good point and this is something that has really gone out of hand.

Possible solution:

1. Add a debounce to the keyword search. No use making a request for every keystroke.

2. Disable include. It has always been quite hard to manage. If FE needs more info, let it use the specific relationship's endpoint instead. That way clients will be forced to think and download only the data they want when they want rather than putting it all on one include parameter and doing it the easy way.

3. Support a new query parameter to specify aggregations such as count. So the client could specify something like . (Probably a better way to specify this.)

   {
     "count": ["sessions", "speakers"]
   }

4. And the server can return only the count info in the meta section of the relationship link. (Like shown in https://jsonapi.org/format/#document-links)

   {
     "data": [
       {
         "type": "event",
         "attributes": {
           "name": "something"
         },
         "relationships": {
           "speakers": {
             "links": {
               "self": "/v1/events/285/relationships/speakers",
               "related": {
                 "href": "/v1/events/285/speakers",
                 "meta": {
                   "count": 30
                 }
               }
             }
           },
           "sessions": {
             "links": {
               "self": "/v1/events/285/relationships/sessions",
               "related": {
                 "href": "/v1/events/285/sessions",
                 "meta": {
                   "count": 10
                 }
               }
             }
           }
         },
         "id": "285",
         "links": {
           "self": "/v1/events/285"
         }
       }
     ],
     "meta": {
       "count": 1
     },
     "jsonapi": {
       "version": "1.0"
     }
   }

[iamareebjamal]

Yes, agree on all 3.

1. Debounce was set but not working, so @prateekj117 suggested to only search on enter, which will reduce even more queries for the time being.
2. Agree. Besides, for open event, we don't have many models where include is helpful. For eg, session, speakers and tracks info can and should be loaded separately with paging. It's not like facebooks need of loading post with actors and comment with users for which they created graphql. includes provide graphql like features without restrictions like implementations of GraphQL like Hasura, making them very dangerous. Both JSON:API and GraphQL suffer from this limitation, but unfortunately JSON:API is not as popular as GraphQL, so tooling and libraries are not up to mark and lack in features and restrictions, specially in Python and Flask.
   We can still allow include for one to one relationships, since it'll get tedious for querying 1-1 relationships like attendee/user etc.
3. Aggregates will surely help but will need time to implement, but are indeed very important to restrict the client from doing these kinds of operations

https://blog.apollographql.com/securing-your-graphql-api-from-malicious-queries-16130a324a6b
https://developer.github.com/v4/guides/resource-limitations/

[kushthedude]

1. Debounce was set but not working, so @prateekj117 suggested to only search on enter, which will reduce even more queries for the time being.

@prateekj117 Did you debug why it is not working ?