This makes it possible for reverse proxies to transform 403 errors
into something like "upgrade to a premium account to access this
pad".
Also add some webaccess tests.
Move the handleMessageSecurity and handleMessage hooks after the call
to securityManager.checkAccess.
Benefits:
* A handleMessage plugin can safely assume the message will be
handled unless the plugin itself drops the message, so it doesn't
need to repeat the access checks done by the `handleMessage`
function.
* This paves the way for a future enhancement: pass the author ID to
the hooks.
Note: The handleMessageSecurity hook is broken in several ways:
* The hook result is ignored for `CLIENT_READY` and `SWITCH_TO_PAD`
messages because the `handleClientReady` function overwrites the
hook result. This causes the client to receive client vars with
`readonly` set to true, which causes the client to display an
immutable pad even though the pad is technically writable.
* The formatting toolbar buttons are removed for read-only pads
before the handleMessageSecurity hook even runs.
* It is awkwardly named: Without reading the documentation, how is
one supposed to know that "handle message security" actually means
"grant one-time write access to a read-only pad"?
* It is called for every message even though calls after a
`CLIENT_READY` or `SWITCH_TO_PAD` are mostly pointless.
* Why would anyone want to grant write access when the user visits a
read-only pad URL? The user should just visit the writable pad URL
instead.
* Why would anyone want to grant write access that only lasts for a
single socket.io connection?
* There are better ways to temporarily grant write access (e.g., the
authorize hook).
* This hook is inviting bugs because it breaks a core assumption
about `/p/r.*` URLs.
I think the hook should be deprecated and eventually removed.
A session's sessioninfo could go away asynchronously due to a
disconnect. Grab a reference once and use it throughout the function
to avoid dereferencing a null sessioninfo object.
Where feasible I put the await at the end of the function to
minimize the impact on latency.
My motivation for this change: Eliminate a race condition in tests I
am writing.
* `src/node/server.js` can now be run as a script (for normal
operation) or imported as a module (for tests).
* Move shutdown actions to `src/node/server.js` to be close to the
startup actions.
* Put startup and shutdown in functions so that tests can call them.
* Use `await` instead of callbacks.
* Block until the HTTP server is listening to avoid races during
test startup.
* Add a new `shutdown` hook.
* Use the `shutdown` hook to:
* close the HTTP server
* call `end()` on the stats collection to cancel its timers
* call `terminate()` on the Threads.Pool to stop the workers
* Exit with exit code 0 (instead of 1) on SIGTERM.
* Export the HTTP server so that tests can get the HTTP server's
port via `server.address().port` when `settings.port` is 0.
Avoid dereferencing `DB.db` until it is used so that it is possible to
`require('SessionStore')` before calling `DB.init()`. (This is useful
when writing tests.)
Before this change, `promises.timesLimit()` created `concurrency - 1`
too many promises. The only users of this function use a concurrency
of 500, so this meant that 499 extra promises were created each time
it was used. The bug didn't affect correctness, but it did result in a
large number of unnecessary database operations whenever a pad was
deleted. This change fixes that bug.
Also:
* Convert the function to async and have it resolve after all of the
created promises are resolved.
* Reject concurrency of 0 (unless total is 0).
* Document the function.
* Add tests.
Until now, the "mobile layout" (with right toolbar on bottom of the screen) was displayed only when screen was smaller than 800px. It made the toolbar break for screen about 1000px when a lot of plugins are in the toolbar.
Now instead, we detect with javascript when the toolbar icons overflow the natural space available, and we switch in "mobile layout" in such case
New feature to copy a pad without copying entire history. This is useful to perform a low CPU intensive operation while still copying current pad state.
Before, a malicious user could bypass authorization restrictions
imposed by the authorize hook:
* Step 1: Fetch any resource that the malicious user is authorized to
access (e.g., static content).
* Step 2: Use the signed express_sid cookie generated in step 1 to
create a socket.io connection.
* Step 3: Perform the CLIENT_READY handshake for the desired pad.
* Step 4: Profit!
Now the authorization decision made by the authorize hook is
propagated to SecurityManager so that it can approve or reject
socket.io messages as appropriate.
This also sets up future support for per-user read-only and
modify-only (no create) authorization levels.
If mocha hangs after running the tests, hit Ctrl-C and wtfnode will
print open files, open sockets, running timers, and running intervals.
Adding an `after` function that closes/stops all of those things will
ensure that mocha exits when it finishes running the tests.
Authentication plugins almost always want to read and modify
`settings.users`. The settings can already be accessed in a few other
ways, but this is much more convenient.
The authorization logic determines whether the user has already
successfully authenticated by looking to see if `req.session.user`
exists. If an authentication plugin says that it successfully
authenticated the user but it did not create `req.session.user` then
authentication will re-run for every access, and authorization plugins
will be unable to determine whether the user has been authenticated.
Return a 500 internal server error to prevent these problems.
Before it only logged an error like this:
SyntaxError: Unexpected string in JSON at position XYZ
Now it also logs the filename, making it easier to figure out where
the bad data is:
failed to read file /path/to/etherpad-lite/src/locales/en.json: SyntaxError: Unexpected string in JSON at position XYZ
package to reduce http requests: nice-select,
pad_automatic_reconnect, skin_variants, scroll, caretPosition
rename unorm in tar.json so it can be included
Before, anyone who could create a socket.io connection to Etherpad
could read, modify, and create pads at will without authenticating
first.
The `checkAccess` middleware in `webaccess.js` normally handles
authentication and authorization, but it does not run for `/socket.io`
requests. This means that the connection handler in `socketio.js` must
handle authentication and authorization. However, before this change:
* The handler did not require a signed `express_sid` cookie.
* After loading the express-session state, the handler did not check
to see if the user had authenticated.
Now the handler requires a signed `express_sid` cookie, and it ensures
that `socket.request.session.user` is non-null if authentication is
required. (`socket.request.session.user` is non-null if and only if
the user has authenticated.)
* Move session validity check and session author ID fetch to a
separate function. This separate function can be used by hooks,
making it easier for them to properly determine the author ID.
* Rewrite the remainder of checkAccess. Benefits:
- The function is more readable and maintainable now.
- Vulnerability fix: Before, the session IDs in sessionCookie
were not validated when checking settings.requireSession. Now,
sessionCookie must identify a valid session for the
settings.requireSession test to pass.
- Bug fix: Before, checkAccess would sometimes use the author ID
associated with the token even if sessionCookie identified a
valid session. Now it always uses the author ID associated
with the session if available.
There are two different ways an author ID becomes associated with a
user: either bound to a token or bound to a session ID. (The token and
session ID come from the `token` and `sessionID` cookies, or, in the
case of socket.io messages, from the `token` and `sessionID` message
properties.) When `settings.requireSession` is true or the user is
accessing a group pad, the session ID should be used. Otherwise the
token should be used.
Before this change, the `/p/:pad/import` handler was always using the
token, even when `settings.requireSession` was true. This caused the
following error because a different author ID was bound to the token
versus the session ID:
> Unable to import file into ${pad}. Author ${authorID} exists but he
> never contributed to this pad
This bug was reported in issue #4006. PR #4012 worked around the
problem by binding the same author ID to the token as well as the
session ID.
This change does the following:
* Modifies the import handler to use the session ID to obtain the
author ID (when appropriate).
* Expands the documentation for the SecurityManager checkAccess
function.
* Removes the workaround from PR #4012.
* Cleans up the `bin/createUserSession.js` test script.
* Fix line numbers top padding
This old rule was conflicting with new css rules introduced in 1.8.4
* Fixes#4228 Performance degradation for long pads
Due to layout trashing when calculating new heights
I plan on splitting authFailure into authnFailure and authzFailure so
that separate authentication and authentication plugins can coexist
peacefully. This change will make it possible to mark the authFailure
hook as deprecated (which simply logs a warning).
Not all authentication plugins require the Authorization header, so it
might not be present in subsequent attempts. (In particular, a reverse
proxy might strip it.)
Plugin authors are allowed to omit the function name in the `ep.json`
parts definition. For example:
```
{
"parts": [
{
"name": "ep_example",
"hooks": {
"authenticate": "ep_example",
"authFailure": "ep_example"
}
}
]
}
```
If omitted, the function name is assumed to be the same as the hook
name. Before this change, `hook_fn_name` for the example hooks would
both be `/opt/etherpad-lite/node_modules/ep_example`. Now they are
suffixed with `:authenticate` and `:authFailure`. This improves
logging, and it makes it possible to use `hook_fn_name` to uniquely
identify a particular hook function.
Every existing caller of `aCallFirst` expects a list and will throw an
exception if given `undefined`. (Nobody calls `callFirst`, except
maybe plugins.)
* Improve the comment describing how the access check works.
* Move the `authenticate` logic to where it is used so that people
don't have to keep jumping back and forth to understand how the
access check works.
* Break up the three steps to reduce the number of indentation
levels and improve readability. This should also make it easier to
implement and review planned future changes.
This commit fixes the error of not waiting the async code to finish.
As the forEach did not wait until the async code finish we may get a
hook set up incorrectly. To fix it, we use an "Array.map" to iterate and
wait the promises to be resolved and then returned
Includes settings
Includes i18n
Includes a nice notification
Disconnects on rate limit
Includes feeding into metrics/stats
Include console warn to server console.
Makes IE11 work again but may cause an issue for plugin testing in IE11. IE plugin tests in IE11 will probably fail as the plugin code will not execute.
1. Introduce contentcollector.js backend tests
1. Fix issue with OL LI items not being properly numbered after import
1. Fix issue with nested OL LI items being improperly numbered on export
1. Fix issue with new lines not being introduced after lists in on import #3961
1. Sanitize HTML on the way in (import)
1. Fix ExportHTML CSS because it needs to support OL > LI > OL not OL > OL [The latter being the correct format]
1. Fix backend tests.
#2412 broke this, it was discovered in https://github.com/JohnMcLear/ep_copy_paste_images/issues/20.
The limitation means that images can't be pasted within lists which IMHO is fine. I could refine this down to OL only but I think images within lists would be fine not to include.
translatewiki.net