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.
* `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.
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.
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.
Do not touch vendorized files (e.g. libraries that were imported from external
projects).
No functional changes.
Command:
find . -name '*.<EXTENSION>' -type f -print0 | xargs -0 sed -i 's/[[:space:]]*$//'
The new hook does the same as exportHtmlAdditionalTags, but is declared
in another hook to avoid confusion about how to export tags when they
are stored as ['tag', 'value'] on attribute pool.
This complements #2762, as per @Gared suggestions.
Richard Hansen