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.
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.
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.
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.)
* 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.
Includes settings
Includes i18n
Includes a nice notification
Disconnects on rate limit
Includes feeding into metrics/stats
Include console warn to server console.
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.
4177b3f943 moved the font-face declarations from src/static/css/pad.css to two
imported files (src/static/css/pad/fonts.css, src/static/css/pad/toolbar.css)
in a different directory.
This results in the font files being invoked from CSSes residing in different
directories in the minified and un-minified case. URLs in the src attribute are
relative to the stylesheet path [0], and so we have to start requiring clean-css
to rebase them.
Before this change, the non minified casse worked by chance, because there were
a lot of "..", which ended up resolving to the root of the site anyways.
Fixes#3956
[0] https://developer.mozilla.org/en-US/docs/Web/CSS/@font-face/src
Before this change, a client would require two versions of the same assets (with
and without randomVersionString), wasting resources and triggering all sorts of
hard to debug inconsistencies.
This change should have been part of 95fd5ce2a4 and completes it.
After each Eterpad restart, the clients will request a new version of the
static assets, even if they are not modified. This is the price we pay for
knowing that no stale files are going to be served ever again. We could also
have used a salted hash of the Etherpad version, but we chose the simpler way.
For the rationale behind using a random string at each restart, see #3958.
ACHTUNG: this may prevent caching HTTP proxies to work.
Closes#3955.
"token" is a random token representing the author, of the form
t.randomstring_of_lenght_20. The random string is generated by the client. The
cookie is used for every pad in the web UI, and is not used for HTTP API.
This comes from the discussion at https://github.com/ether/etherpad-lite/issues/3563
In this way, if the browser sends a list of preferred languages via
Accept-Language HTTP header, Etherpad will honor that.
Before this change, Etherpad always forced on the user the language from
padOptions.lang in settings.json.
This reverts a feature that was introduced in 295672f598.
If Etherpad is hosted on Windows the frontend test URI needs to be
/tests/frontend/index.html (docs say .../frontend/), otherwise there is this
error: ERR_TOO_MANY_REDIRECTS.
Fixes#3804.
Starting with Etherpad 1.8.3 we decided to use Colibris as default skin for new
installs. Without this change, when starting with no settings.json file,
Etherpad would (wrongly) use "no-skin".
This change should have been part of 70bc71c0c3.
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.
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.
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.
Sometimes, RFC 6265-compliant [0] web servers may send back a cookie whose value
is enclosed in double quotes, such as:
Set-Cookie: sessionCookie="s.37cf5299fbf981e14121fba3a588c02b,s.2b21517bf50729d8130ab85736a11346"; Version=1; Path=/; Domain=localhost; Discard
Where the double quotes at the start and the end of the header value are just
delimiters. This is perfectly legal: Etherpad parsing logic should cope with
that, and remove the quotes early in the request phase.
Somehow, this does not happen, and in such cases the actual value that
sessionCookie ends up having is:
sessionCookie = '"s.37cf5299fbf981e14121fba3a588c02b,s.2b21517bf50729d8130ab85736a11346"'
As quick measure, let's strip the double quotes (when present).
Note that here we are being minimal, limiting ourselves to just removing quotes
at the start and the end of the string.
Fixes#3819.
Also, see #3820.
[0] https://tools.ietf.org/html/rfc6265
This yields better conversion results, but requires the previous change,
otherwise there would have been difficulties in locating the temporary file
name.
In the next commit, we are going to change the conversion method to
"html:XHTML Writer File:UTF8". Without this change, that conversion method name
would end up in the extension of the temporary file that is created as an
intermediate step. In this way, the file extensione will always stay ".html".
No functional changes, hopefully. Only the extension of the temporary file
should change.
This change is meant to ease using LibreOffice as converter. When LibreOffice
converts a file, it adds some classes to the <title> tag.
This is a quick & dirty way of matching the <title> and comment it out
independently on the classes that are set on it.
Clearing the authorship colors of a document with at least two authors, and then
undoing that action caused a disconnect from the pad.
This change disallows undoing clearing authorship colors in order to prevent
the problem from affecting users, and adds the relative test coverage.
This is a change of behaviour, and is documented in the changelog.
Fixes#2802 (sidestepping it).
The previous syntax caused a deprecation warning on Node 10.
However, due to the very old version of log4js Etherpad is currently using,
customError objects are going to be displayed as { inspect: [Function: inspect] }.
This needs to be addressed later, updating log4js.
Fixes#3834.
For some weird reason, these seem to be part of the original swagger
implementation but tests assume they're turned off.
Perhaps a difference between /rest and /api?
- Tests pass ✅
- Added openapi-backend hook
- Generating OpenAPI v3 definitions for each API version
- Definitions served /api/openapi.json /api/{version}/openapi.json
The saved revision "star" button appeared in the timeslider toolbar too.
This change introduces a second flag "page" in toolbar.menu(), which controls
whether the toolbar is being drawn for a pad or in the timeslider page.
Fixes#3767.
Revision b480416375 fixed a bug, but introduced a regression, and the "save
revision" "star" button started to appear both on the left and the right
toolbar.
This change introduces a flag "whichMenu" in toolbar.menu(), that controls
whether the left, the right or the timeslider toolbar is being drawn.
By specification [0], the if-modified-since HTTP header sent by browsers does
not include milliseconds.
Before this patch, let's say a file was generate at time:
t_real-file = 2020-03-22T02:15:53.548Z (note the fractional seconds)
When issuing a conditional request, the browser would truncate the fractional
part, and only request an if-modified-since with this contents:
t_if-modified-since = 2020-03-22T02:15:53.000Z
The minify() function would return HTTP/304 only if
t_if-modified-since >= t_real-file, but this would never be true unless, by
chance, a file was generated at XX.000Z.
This resulted in that file being minified/compressed again and resent to the
client for no reason. After this patch, the server correctly responds with
HTTP/304 without doing any computation, and the browser uses the cached file.
[0] https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/If-Modified-Since
CleanCSS 3.4.19 had a Regex Denial of Service vulnerability and has to be
updated. The major version bump requires the following changes:
1. Disabling rebase is necessary because otherwise the URLs for the web fonts
become wrong;
EXAMPLE 1:
/static/css/src/static/font/fontawesome-etherpad.woff
instead of
/static/font/fontawesome-etherpad.woff
EXAMPLE 2 (this is more surprising):
/p/src/static/font/opendyslexic.otf
instead of
/static/font/opendyslexic.otf
2. CleanCSS.minify() can either receive a string containing the CSS, or an array
of strings. In that case each array element is interpreted as an absolute
local path from which the CSS file is read.
In version 4.x, CleanCSS API was simplified, eliminating the relativeTo
parameter, and thus we cannot use our already loaded "content" argument, but
we have to wrap the absolute path to the CSS in an array and ask the library
to read it by itself.
Fixes#3616.
For some reason authorInfo is sometimes null, and therefore it is not possible
to get colorId from it.
This resulted in the following stack trace:
[2020-03-16 09:27:17.291] [ERROR] console - (node:1746) UnhandledPromiseRejectionWarning: TypeError: Cannot read property 'colorId' of null
at <BASEDIR>/src/node/handler/PadMessageHandler.js:1199:37
at runMicrotasks (<anonymous>)
at processTicksAndRejections (internal/process/task_queues.js:97:5)
at async Promise.all (index 0)
at async handleClientReady (<BASEDIR>/src/node/handler/PadMessageHandler.js:1171:5)
[2020-03-16 09:27:17.291] [ERROR] console - (node:1746) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). To terminate the node process on unhandled promise rejection, use the CLI flag `--unhandled-rejections=strict` (see https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode). (rejection id: 76)
[2020-03-16 09:27:19.034] [WARN] message - Dropped message, USERINFO_UPDATE Session not ready.[object Object]
Which is due to a bug in Etherpad that we are not going to solve now.
As a workaround, when this happens, let's set the username to "Anonymous" (if
it is not already set), and colorId to the fixed value "#daf0b2". Warning
messages are written in the logs to signal this condition.
This is no definitive solution, but fixes#3612 (via a workaround).
Before this patch, visiting the read-only URL for a random pad would remove
the "Save Revision" (the "star" icon) from all the other RW pads. The only way
to make it appear again was to restart the server.
This change does not fix the underlying bug: after visiting a read only link
the "star" button would still disapper, but it is explictly reinserted via an
ad-hoc condition.
Fixes#3702
By specification, when settings.allowUnknownFileEnds is true and the user tries
to import a file with an unknown extension (this includes no extension),
Etherpad tries to import it as txt.
This broke in Etherpad 1.8.0, that abruptly terminates the processing with an
UnhandledPromiseRejectionWarning.
This patch restores the intended behaviour, and allows to import as text a file
with an unknown extension (on no extension).
In order to catch the UnhandledPromiseRejectionWarning we had to use
fsp_rename(), which is declared earlier in the code and is promised based
instead of fs.rename(), which is callback based.
Fixes#3710.
Before this change, invoking a non existing API method would return an HTTP/200
response with a JSON payload {"code":3,"message":"no such function"}.
This commit changes the HTTP status code to 404, leaving the payload as-is.
Before:
curl --verbose "http://localhost:9001/api/1/notExisting?apikey=ABCDEF"
< HTTP/1.1 200 OK
< X-Powered-By: Express
[...]
{"code":3,"message":"no such function","data":null}
After:
curl --verbose "http://localhost:9001/api/1/notExisting?apikey=ABCDEF"
< HTTP/1.1 404 OK
< X-Powered-By: Express
[...]
{"code":3,"message":"no such function","data":null}
Fixes#3546.
Steps to reproduce (via HTTP API):
1. create a group via createGroup()
2. create a group pad inside that group via createGroupPad()
3. make that pad public calling setPublicStatus(true)
4. access the pad via a clean web browser (with no sessions)
5. UnhandledPromiseRejectionWarning: apierror: sessionID does not exist
This was due to an overlook in 769933786c: "apierror: sessionID does not
exist" may be a legal condition if we are also visiting a public pad. The
function that could throw that error was sessionManager.getSessionInfo(), and
thus it needed to be inside the try...catch block.
Please note that calling getText() on the pad always return the pad contents,
*even for non-public pads*, because the API bypasses the security checks and
directly talks to the DB layer.
Fixes#3600.
The mechanism used for determining if the application is being served over SSL
is wrapped by the "express-session" library for "express_sid", and manual for
the "language" cookie, but it's very similar in both cases.
The "secure" flag is set if one of these is true:
1. we are directly serving Etherpad over SSL using the native nodejs
functionality, via the "ssl" options in settings.json
2. Etherpad is being served in plaintext by nodejs, but we are using a reverse
proxy for terminating the SSL for us;
In this case, the user has to be instructed to properly set trustProxy: true
in settings.json, and the information wheter the application is over SSL or
not will be extracted from the X-Forwarded-Proto HTTP header.
Please note that this will not be compatible with applications being served over
http and https at the same time.
The change on webaccess.js amends 009b61b338, which did not work when the SSL
termination was performed by a reverse proxy.
Reference for automatic "express_sid" configuration:
https://github.com/expressjs/session/blob/v1.17.0/README.md#cookiesecureCloses#3561.
The "io" cookie is created by socket.io, and its purpose is to offer an handle
to perform load balancing with session stickiness when the library falls back to
long polling or below.
In Etherpad's case, if an operator needs to load balance, he can use the
"express_sid" cookie, and thus "io" is of no use.
Moreover, socket.io API does not offer a way of setting the "secure" flag on it,
and thus is a liability.
Let's simply nuke it.
References:
https://socket.io/docs/using-multiple-nodes/#Sticky-load-balancinghttps://github.com/socketio/socket.io/issues/2276#issuecomment-147184662 (not totally true, actually, see above)
The change that implemented #3648 (7c099fef5e) was incorrect, and resulted
in disabling every user at startup.
The problem was twofold:
1. _.filter() on an object returns an array of the object's enumerable values
and strips out the keys, see: https://stackoverflow.com/questions/11697702/how-to-use-underscore-js-filter-with-an-object
To filter an object, the function that needs to be used is _.pick();
2. The logic condition on userProperties.password was plain wrong (it should
have been an AND instead of an OR).
This change corrects 1) and 2), and writes more specific logs when something
goes wrong.
Closes#3661.
Nodejs 8 will be EOLed on December 31th, 2019 (https://github.com/nodejs/Release).
This means any future Etherpad version released from 2020 on should require at
least the next LTS (10.13.0). Let's keep some margin and decide that the first
Etherpad version dropping node 8 compatibility will be 1.8.3.
Closes#3650.
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:]]*$//'
Currently the version is exposed in a 'Server' http headers.
This commit allows to parameterize it in the settings. By defaults it is
not exposed.
Fixes#3423
In this way the only external call to statFile() provides an explicit value for
"dirStatLimit", and thus the initial check on "undefined" at the start of the
function could be removed (just added a comment for now).
All the configuration values can be read from environment variables using the
syntax "${ENV_VAR_NAME}".
This is useful, for example, when running in a Docker container.
EXAMPLE:
"port": "${PORT}"
"minify": "${MINIFY}"
"skinName": "${SKIN_NAME}"
Would read the configuration values for those items from the environment
variables PORT, MINIFY and SKIN_NAME.
REMARKS:
Please note that a variable substitution always needs to be quoted.
"port": 9001, <-- Literal values. When not using substitution,
"minify": false only strings must be quoted: booleans and
"skin": "colibris" numbers must not.
"port": ${PORT} <-- ERROR: this is not valid json
"minify": ${MINIFY}
"skin": ${SKIN_NAME}
"port": "${PORT}" <-- CORRECT: if you want to use a variable
"minify": "${MINIFY}" substitution, put quotes around its name,
"skin": "${SKIN_NAME}" even if the required value is a number or a
boolean.
Etherpad will take care of rewriting it to
the proper type if necessary.
Resolves#3543
Before this commit, when passed a malformed credentials.json the application
crashed with a stack dump. Now we catch the error and fail in a controlled way
(like already done for settings.json).
Example of exception we no longer throw:
MALFORMEDJSON
^
SyntaxError: Unexpected token M in JSON at position 0
at JSON.parse (<anonymous>)
at Object.reloadSettings (<BASEDIR>/src/node/utils/Settings.js:390:24)
at Object.<anonymous> (<BASEDIR>/src/node/utils/Settings.js:543:9)
at Module._compile (module.js:635:30)
at Object.Module._extensions..js (module.js:646:10)
at Module.load (module.js:554:32)
at tryModuleLoad (module.js:497:12)
at Function.Module._load (module.js:489:3)
at Module.require (module.js:579:17)
at require (internal/module.js:11:18)
ueberDB2 can return either undefined or null for a missing key, depending on
which DB driver is used. This patch changes the promise version of the API so
that it will always return null.
some code chunks previously used `async.parallel` but if you
use `await` that forces them to be run serially. Instead,
you can initiate the operation (getting a Promise) and then
_later_ `await` the result of that Promise.
If you use `await` inside a loop it makes the loop inherently serial.
If you omit the `await` however, the tasks will all start but the loop
will finish while the tasks are still being scheduled.
So, to make a set of tasks run in parallel but then have the
code block after the loop once all the tasks have been completed
you have to get an array of Promises (one for each iteration) and
then use `Promise.all()` to wait for those promises to be resolved.
Using `Array#map` is a convenient way to go from an array of inputs
to the require array of Promises.
NB1: needs additional review and testing - no abiword available on my test bed
NB2: in ImportHandler.js, directly delete the file, and handle the eventual
error later: checking before for existence is prone to race conditions,
and does not handle any errors anyway.
- removed possible issue with failing to sanitize `padName` if `padId` was also
supplied
- removed unnecessary `try` block
- simplified API and function name matching tests
Also converted the handler functions that depend on checkAccess() into async
functions too.
NB: this commit needs specific attention to it because it touches a lot of
security related code!
This patch also contains significant refactoring relating to error checking of
arguments supplied to the functions (e.g. rev) facilitated by use of `throw`
instead of nodeback errors.
1. This module was not migrated to Promises, because it is only used via
express-session, which can't actually use promises anyway.
2. all(), clear() and length() depend on the presence of the `db.forEach()`
function, which in ueberdb2 doesn't even exist.
Fortunately those three methods are optional, so I made their existence
conditional on the presence of `db.forEach`.
3. in SessionStore.clear(), replaced a call to db.db.remove() with db.remove()
Changed two occurrences of:
process.nextTick(function() {
if (fn) fn();
});
with
if (fn) {
process.nextTick(fn);
}
i.e. such that no function even gets added to the `nextTick` queue unless
there's actually a function to be called.
Extracted from Ray's work.
`getPadAccess()` (src/node/padaccess.js) is now "promise only", resolving to
`true` or `false` as appropriate, and throwing an exception if there's an
error.
The two call sites (padreadonly.js and importexport.js) updated to match.
Use real `async` instead of async.js where applicable.
The `getPluginTests()` function was never truly async anyway because it only
contains calls to synchronous `fs` modules.
This change is in preparation of the future async refactoring by Ray. It tries
to extract as many changes in boolean conditions as possible, in order to make
more evident identifying eventual logic bugs in the future work.
This proved already useful in at least one case.
BEWARE: this commit exposes an incoherency in the DB API, in which, depending
on the driver used, some functions can return null or undefined. This condition
will be externally fixed by the final commit in this series ("db/DB.js: prevent
DB layer from returning undefined"). Until that commit, the code base may have
some bugs.
This change extracts the grammar correction performed on the async branch,
anticipating them in a single commit. It cannot be folded with the previous
one, as it is not purely cosmetic.
This change is only cosmetic. Its aim is do make it easier to understand the
async changes that are going to be merged later on. It was extracted from the
original work from Ray Bellis.
To verify that nothing has changed, you can run the following command on each
file touched by this commit:
npm install uglify-es
diff --unified <(uglify-js --beautify bracketize <BEFORE.js>) <(uglify-js --beautify bracketize <AFTER.js>)
This is a complete script that does the same automatically (works from a
mercurial clone):
```bash
#!/usr/bin/env bash
set -eu
REVISION=<THIS_REVISION>
PARENT_REV=$(hg identify --rev "${REVISION}" --template '{p1rev}')
FILE_LIST=$(hg status --no-status --change ${REVISION})
UGLIFYJS="node_modules/uglify-es/bin/uglifyjs"
for FILE_NAME in ${FILE_LIST[@]}; do
echo "Checking ${FILE_NAME}"
diff --unified \
<("${UGLIFYJS}" --beautify bracketize <(hg cat --rev "${PARENT_REV}" "${FILE_NAME}")) \
<("${UGLIFYJS}" --beautify bracketize <(hg cat --rev "${REVISION}" "${FILE_NAME}"))
done
```
This is documented to be more performant.
The substitution was made on frontend code, too (i.e., the one in /static),
because Date.now() is supported since IE 9, and we are life supporting only
IE 11.
Commands:
find . -name *.js | xargs sed --in-place "s/new Date().getTime()/Date.now()/g"
find . -name *.js | xargs sed --in-place "s/(new Date()).getTime()/Date.now()/g"
Not done on jQuery.
The guard condition on count being non negative and < 100 used the wrong
boolean operator. In its form it was impossible.
This error was introduced in 2013, in 5592c4b0fe.
Fixes#3499
The HTTP API doesn't ever omit arguments, it always passes `undefined` for a
parameter that wasn't supplied in the request.
The functions that were simplified are:
- getRevisionChangeset()
- getText()
- getHTML()
- saveRevision()
The only function still supporting optional arguments is getPadSafe(), which is
only called from this module.
This function was simulating two overloads:
1. copy(destinationID, force, callback)
2. copy(destinationID, callback), in this case "force" would be assumed false
But all the call sites always used the version with arity 3.
Thus, we can remove that optionality and always assume that the funcion will be
called with three parameters. This will simplify future work.
- path.exists() is no longer part of nodejs
- fs.exists() is deprecated (as of nodejs >= 8)
- checking a file for existence before using it is open to raca condition. It is
preferable to go ahead and use the file, and eventually handle the error
- we can afford two simple synchronous fs operations here
Next version will be Etherpad 1.8. As planned in #3424, we are going to require
NodeJS >=8.9.0 and npm >= 6.4.
This commit implements that change and updates documentation and scripts.
Subsequent changes will get rid of old idioms, dating back to node < 0.7, that
still survive in the code.
Once migrated to NodeJS 8, we will be able to start working on migrating the
code base from callbacks to async/await, greatly simplifying legibility (see
#3540).
Closes#3557
Until Etherpad 1.7.5, process.on('SIGTERM') and process.on('SIGINT') were not
hooked up under Windows, because old nodejs versions did not support them.
This excluded the possibility of doing a graceful shutdown of the database
connection under that platform.
According to nodejs 6.x documentation, it is now safe to do so. This allows to
gracefully close the DB connection when hitting CTRL+C under Windows, for
example.
Source: https://nodejs.org/docs/latest-v6.x/api/process.html#process_signal_events
- SIGTERM is not supported on Windows, it can be listened on.
- SIGINT from the terminal is supported on all platforms, and can usually be
generated with <Ctrl>+C (though this may be configurable). It is not
generated when terminal raw mode is enabled.
A Windows manual install has the same directory layout of a normal Unix one
(e.g. the nice symlink node_modules/ep_etherpad-lite -> ../src).
Only when running from the pre-built Windows package the directory layout is
different (e.g. src is physically copied into node_modules/ep_etherpad-lite).
The previous version of the code wrongly assumed that all Windows installs would
be run from the pre-built pakage.
In this version the path search is the same on all platform. If it fails, and we
are on Windows, there is a fallback for the specific case of the pre-built
package.
Fixes#3550
This commit vastly shortens (and simplifies) the version table within
handler/APIHandler.js by building each version's entry incrementally based off
the previous version.
The resulting table has been validated by comparing the "before" and "after"
output of the following loop on both versions of the code (albeit with an
intermediate "sort" step to account for the different insertion order)
for (let v in version) {
let m = version[v];
for (let [k, a] of Object.entries(m)) {
console.log(v, k, a);
}
}
The patch also fixes a few typos, and removes a duplicate definition of
getChatHistory which in each applicable version was defined with two different
parameter lists, but where only the second would be used.
Since the original comparison compared for truthy and not for "===", and it's
3 AM now, I blindly negated it, in order to show how fragile it was in the first
instance.
No functional changes.
This is the final commit of this refactoring series.
Get rid of an else branch to simplify code layout. No functional changes at all.
==============
This series is an attempt to reduce the control structure depth of the code
base, maintaining at the same time its exact same behaviour, bugs included. It
is, in a sense, an initial attempt at a refactoring in the spirit of its
original definition [0].
The idea beyond this refactoring is that reducing the code depth and, sometimes,
inverting some conditions, bugs and logic errors may become easier to spot, and
the code easier to read.
When looked at ignoring whitespace changes, all of these diffs should appear
trivial.
[0] https://refactoring.com/
It's just synctactic sugar, but it is always better than executing string
concatenations in one's mind.
Do not do this with files in src/static, because we want to keep IE 11
compatibility.
The old "static/custom" directory is replaced by "static/skins/<skinName>",
where <skinName> is taken from settings.json.
When no value is found, a default of "no-skin" is assumed, so that backward
compatibility is maintained.
The most evident security concerns have been addressed.
Closes#3471.
skinName must be a single string (no directory separators in it) pointing to an
existing directory under /src/static/skins.
In case these conditions are not met, its value is rewritten to "no-skin".
Also, the value of skinName if sent to the client via clientVars for allowing
its use it in the browser.
If you edit `src/templates/export_html.html` to remove the
`<meta name="changedby" content="Etherpad">` tag[1], PDF export with
soffice has a bug: the first word of the pad is deleted and a blank page
is inserted as first page (the pad's text begins on the second page).
The `--writer` soffice option avoids that bug.
[1] you may want to delete that tag since it is inserted as a comment in
.doc or .odt soffice export.
This file uses it for robots.txt and favicon.ico.
This makes use of the new stable settings.root introduced with #3466, and will
be modified when introducing support for custom skins.
This should look to consistent locations when looking for relative paths,
without depending on current working directory.
For absolute paths, nothing changes.
This is just a function (with an ugly side effect for caching purposes) that
heuristically tries to compute the Etherpad installation path when running under
Unix and win32 (they have different file system layouts).
This path can be used by Etherpad as a base for all the relative paths, in order
to be deterministic and not depending on cwd.
This is the location that is choosen by default when Etherpad starts with no
settings.json file.
It was different than the one contained into setting.json.template.
Minimum supported Node version is 6.9.0, but Object.values() was introduced in
Node < 7. Let's use a polyfill if needed.
This will be removed when minimum supported Node version is raised to 8.9.0.
Fixes#3459
When installing dependencies, npm informed us that measured had been deprecated,
and renamed to measured-core. Let's follow the advice, and get rid of the
warning.
npm WARN deprecated measured@1.1.0: This package has been renamed to
measured-core, all versions of measured have been re-released under
measured-core, please update your package and consider updating to the newest
version. See https://github.com/yaorg/node-measured for latest updates.
This package is used to expose a single endpoint ("/stats"), whose output does
not change after this commit.
Fixes#3458
The hostname:port of URIs used in Minify are currently bogus and refer
to localhost only for historical reasons; there's no reason to retain
them and omitting them avoids generating an invalid URI when "port" is
not an integer.
Context: settings.port is passed to express's listen; if not numeric, it
is used a filename for a Unix domain socket.
This allows e.g. starting a server to be reverse-proxied on a multi-user
system, using the filesystem to handle access control and avoiding need
to allocate port numbers.
Before this change, etherpad-lite starts without error when configured
to listen on a Unix domain socket in this manner. However, `pad.js` and
`ace2_common.js` are generated incorrecting, causing an error
"Uncaught Error: The module at "ep_etherpad-lite/static/js/rjquery" does not exist."
when loading the editor:
When settings.port is a non-numeric string, e.g. `etherpad.sock`, a URI
of the form `http://localhost:etherpad.sock/static/js/rjquery.js` is
generated and parsed to find the file needed. In this case, the file
searched for is `:etherpad.sock/static/js/rjquery.js`, rather than the
expected `static/js/rjquery.js`. No such file exists, and the required
code is silently omitted from the bundle.
As a workaround, hard-code a (meaningless) hostname which can be parsed
correctly, since the current code makes no use of it anyway.
Etherpad 1.6.6 does not run on node <= 5 already.
Node 6.9 is the first LTS release in the 6 series, and comes with npm 3.10.8.
Declarations in package.json are advisory unless the user has set
`engine-strict` config flag.
Updated the docs accordingly.
When using LibreOffice to convert pads to doc, we got `Error: no export filter for /tmp/xxxx.doc` (tested with LO 5 and 6). Maybe it's a regression from LO. Anyway, converting HTML to odt, then to doc works.
Thx to lpagliari for her review!
* Add scroll when it edits a line out of viewport
By default, when there is an edition of a line, which is out of the
viewport, Etherpad scrolls the minimum necessary to make this line
visible. This makes that the line stays either on the top or the bottom
of the viewport. With this commit, we add a setting to make possible to
scroll to a position x% pixels from the viewport. Besides of that, we
add a setting to make an animation of this scroll.
If nothing is changed on settings.json the Etherpad default behavior is
kept
Shut down database connection and exit the node process
when SIGTERM is encountered. This is especially important
when nodejs is run as PID1, e.g. in a docker container.
Shutting down connections to clients (browsers) is beyond
this patche's scope.
Resolves#3265
Users still cannot choose which shortcuts they want to enable/disable,
so it does not make sense (yet) to have a cookie with that preference.
This can be reverted once we create an UI to change shortcuts, but
PLEASE PLEASE PLEASE do not read the cookie every time handleKeyEvent is
called!!!
This is an adjustment to #2891.
On some erros that display a modal with "Force reconnect" button, allow
Etherpad to automatically reload pad after a few seconds. Amount of
seconds is defined on settings.json.
Still need to create tests for this feature, and implement i18n.
This meant plumbing a callback through to `compressCSS()`, which meant that
I had to alter the innards of `getFileCompressed()`. I tried to leave that
function looking more understandable than when I found it; for example, I
flattened out the nested `if`.
I went ahead and upgraded the version of `clean-css` while I was in the
territory.
The semantics of all the substitutions are identical to what they were before.
I _did_ take the liberty of formatting the CSS to be a bit more readable
(at the cost of adding a little bit of whitespace).
