Parsoid: Rate limit stashing requests #1140
Conversation
d00rman
force-pushed
the
parsoid/stash-ratelimit
branch
from
29819a9 to
8588055
Compare
sys/parsoid.js
Outdated
| @@ -125,6 +122,51 @@ class ParsoidService { | |||
| }; | |||
| } | |||
|
|
|||
| _initOpts(opts) { | |||
There was a problem hiding this comment.
Could use a default parameter
sys/parsoid.js
Outdated
| if (!((req.query && req.query.stash) || (req.body && req.body.stash))) { | ||
| return; | ||
| } | ||
| if (hyper._rootReq.headers['x-stash-rate-checked']) { |
There was a problem hiding this comment.
What's the point of this? I don't think it would be called recursively and if it does, it's an error.
There was a problem hiding this comment.
There are cases where parsoid calls itself via the URI, such as
Line 577 in de620a7
There was a problem hiding this comment.
Yes and no. Actually that particular line you have shown is not calling itself, it's actually constructing an artificial request for code sharing. I wanna refactor it out. The only other case is calling the pagebundle endpoint, which I wanna get rid of too. So this is not needed.
There was a problem hiding this comment.
Ah you are correct. Ok, removing.
sys/parsoid.js
Outdated
| } | ||
| hyper._rootReq.headers['x-stash-rate-checked'] = true; | ||
| const key = `${hyper.config.service_name}.parsoid_stash.${req.params.domain}.` + | ||
| `${req.params.title || 'Transform'}.${req.params.revision || 0}|` + |
There was a problem hiding this comment.
mmm... do we really wanna include title/revision in a key? This doesn't really limit anything, a malicious client can just iterate over all revisions of all pages
There was a problem hiding this comment.
Good point, will remove.
|
2 comments inlined. Plus need to update the docs. |
d00rman
force-pushed
the
parsoid/stash-ratelimit
branch
from
8588055 to
2fcf8a2
Compare
Stashing requests are expensive and take up storage, so make sure they are rate-limited. We check the rates only for external requests that explicitly request stashing. By default, at most 5 requests per second per client IP are allowed, but that can be adjusted by setting the `stash_ratelimit` in the module's configuration options. Bug: T224055
d00rman
force-pushed
the
parsoid/stash-ratelimit
branch
from
2fcf8a2 to
c424c8d
Compare
|
Ok, @Pchelolo addressed all the comments and added the rate limits to the spec docs. |
|
Waiting gor travis |
Stashing requests are expensive and take up storage, so make sure they are rate-limited. We check the rates only for external requests that explicitly request stashing. By default, at most 5 requests per second per client IP are allowed, but that can be adjusted by setting the
stash_ratelimitin the module's configuration options.Bug: T224055