Import/export max file size & rate limiting. Import only allowed with author present on pad #3833
Conversation
Updating
This is a departure from previous versions, which did not limit import/export requests. Now such requests are ALWAYS rate limited. The default is 10 requests per IP each 90 seconds, and also applies to old instances upgraded to 1.8.3. Administrators can tune the parameters via settings.importExportRateLimiting.
JohnMcLear
changed the title
JohnMcLear
changed the title
|
Am I blocking further review of this? |
muxator
force-pushed
the
rate-limiting
branch
4 times, most recently
from
f72906d
to
eb38423
Compare
I can't remember but if they are commented out they can be left in. The idea is that we have a reference for tests for when I can figure out how to create a session and put that session present in a pad. That or provide a method to bypass that with Either way it would be nice to have a reference / reminder knowing how infrequent I contribute :D |
No they are not commented out. The test file is introduced in a commit and wiped altogether in the next one. This is why I asked. |
|
In heinsight, I should have split rate limiting and author requirements up into two PRs. My apologies @muxator. |
I was thinking about that just now. I'm going to split them.
Edit: done. The next comments apply on this split version. In particular, please have a look at commit 9847bf6 because I need help there. |
… is not on that pad Importing to a pad is allowed only if an author has a session estabilished and has already contributed to that specific pad. This means that as long as the user is on the pad (via the browser) then import is possible. Note that an author session is NOT the same as a group session, which is not required. This setting does not apply to API requests, only to /p/$PAD$/import This change of behaviour is introduced in Etherpad 1.8.3, and cannot be disabled.
From Etherpad 1.8.3 onwards, the maximum allowed size for a single imported file will always be bounded. The maximum allowed size can be configured via importMaxFileSize.
muxator
force-pushed
the
rate-limiting
branch
2 times, most recently
from
a194093
to
d75c1fc
Compare
The newly introduces environment variables are IMPORT_EXPORT_RATE_LIMIT_WINDOW and IMPORT_EXPORT_MAX_REQ_PER_IP.
muxator
force-pushed
the
rate-limiting
branch
2 times, most recently
from
0d4b9b1
to
124b396
Compare
The old loadSettings.js was a way of customizing settings upon load, because the Settings module did not offer this functionality. But it did not work well, since all the default settings were not loaded. Let's get rid of loadSettings.js for the bulk of the tests (the "backend" specs). For the "container" specs, we'll keep it in place until/if we rewrite Settings.js making it less brittle.
This is in preparation to the next activities about import/export securization.
muxator
force-pushed
the
rate-limiting
branch
2 times, most recently
from
11ebeae
to
313229c
Compare
There was a problem hiding this comment.
All done. I have one remaining point (important), and a UI note (not important)
-
Is there a use case for which a user should want to setrequireAuthorSessionToImporttofalse?If this is a needed security change, these checks should always be done, without the possibility of disabling them. -
when an import fails (for example because it is rate limited), the UI freezes and then times out. I am ok with keeping as-is, for now, but let's remember to fix it.
Edit: I decided to remove the configuration setting requireAuthorSessionToImport and always enable those checks. It will not possible to disable the new behaviour.
muxator
changed the title
Also
Tests without creating an authorID is not really feasible, I tried but I can't find a decent way.
This is good to merge as is, knowing that a future job is to write test coverage which includes a method to create an authorID that is present on a specific padID.