Skip to content

Commit

Permalink
undomodule: disallow undoing "clear authorship colors"
Browse files Browse the repository at this point in the history
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.

This is a change of behaviour, and is documented in the changelog.

Fixes #2802 (sidestepping it).
  • Loading branch information
JohnMcLear committed Apr 6, 2020
1 parent 16daaa5 commit 301e65b
Show file tree
Hide file tree
Showing 7 changed files with 19 additions and 4 deletions.
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
# 1.8.3
*BREAKING CHANGE*: undoing the "clear authorship colors" command is no longer supported (see https://github.com/ether/etherpad-lite/issues/2802)

# 1.8
* SECURITY: change referrer policy so that Etherpad addresses aren't leaked when links are clicked (discussion: https://github.com/ether/etherpad-lite/pull/3636)
* SECURITY: set the "secure" flag for the session cookies when served over SSL. From now on it will not be possible to serve the same instance both in cleartext and over SSL
Expand Down
2 changes: 1 addition & 1 deletion src/locales/en.json
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@
"pad.userlist.guest": "Guest",
"pad.userlist.deny": "Deny",
"pad.userlist.approve": "Approve",
"pad.editbar.clearcolors": "Clear authorship colors on entire document?",
"pad.editbar.clearcolors": "Clear authorship colors on entire document? This cannot be undone",

"pad.impexp.importbutton": "Import Now",
"pad.impexp.importing": "Importing...",
Expand Down
2 changes: 1 addition & 1 deletion src/locales/qqq.json
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@
"pad.userlist.guest": "Preceded by the link text which is labeled {{msg-etherpadlite|Pad.userlist.approve}}.\n{{Identical|Guest}}",
"pad.userlist.deny": "Used as link text.\n\nFollowed by the link which is labeled {{msg-etherpadlite|Pad.userlist.approve}}.",
"pad.userlist.approve": "Used as link text.\n\nPreceded by the link which is labeled {{msg-etherpadlite|Pad.userlist.deny}}.\n\nFollowed by the message {{msg-etherpadlite|Pad.userlist.guest}}.\n{{Identical|Approve}}",
"pad.editbar.clearcolors": "Used as confirmation message (JavaScript <code>confirm()</code> function).\n\nThis message means \"Are you sure you want to clear authorship colors on entire document?\".",
"pad.editbar.clearcolors": "Used as confirmation message (JavaScript <code>confirm()</code> function).\n\nThis message means \"Are you sure you want to clear authorship colors on entire document? This cannot be undone\".",
"pad.impexp.importbutton": "Used as label for the Submit button.",
"pad.impexp.importing": "Used to indicate that the file is being imported.\n{{Identical|Importing}}",
"pad.impexp.confirmimport": "Used as confirmation message (JavaScript <code>confirm()</code> function).",
Expand Down
2 changes: 1 addition & 1 deletion src/node/handler/PadMessageHandler.js
Original file line number Diff line number Diff line change
Expand Up @@ -932,7 +932,7 @@ async function handleClientReady(client, message)
await Promise.all(authors.map(authorId => {
return authorManager.getAuthor(authorId).then(author => {
if (!author) {
messageLogger.error("There is no author for authorId:", authorId);
messageLogger.error("There is no author for authorId: ", authorId, ". This is possibly related to https://github.com/ether/etherpad-lite/issues/2802");
} else {
historicalAuthorData[authorId] = { name: author.name, colorId: author.colorId }; // Filter author attribs (e.g. don't send author's pads to all clients)
}
Expand Down
3 changes: 3 additions & 0 deletions src/static/js/Changeset.js
Original file line number Diff line number Diff line change
Expand Up @@ -791,6 +791,9 @@ exports.textLinesMutator = function (lines) {
}
} else {
var sline = putCurLineInSplice();
if (!curSplice[sline]) {
console.error("curSplice[sline] not populated, actual curSplice contents is ", curSplice, ". Possibly related to https://github.com/ether/etherpad-lite/issues/2802");
}
curSplice[sline] = curSplice[sline].substring(0, curCol) + text + curSplice[sline].substring(curCol);
curCol += text.length;
}
Expand Down
1 change: 1 addition & 0 deletions src/static/js/ace2_inner.js
Original file line number Diff line number Diff line change
Expand Up @@ -292,6 +292,7 @@ function Ace2Inner(){
{
if ((typeof author) != "string")
{
top.console.error("Going to throw new error, potentially caused by: https://github.com/ether/etherpad-lite/issues/2802");
throw new Error("setAuthorInfo: author (" + author + ") is not a string");
}
if (!info)
Expand Down
10 changes: 9 additions & 1 deletion src/static/js/undomodule.js
Original file line number Diff line number Diff line change
Expand Up @@ -253,7 +253,15 @@ var undoModule = (function()
}
if (!merged)
{
stack.pushEvent(event);
/*
* Push the event on the undo stack only if it exists, and if it's
* not a "clearauthorship". This disallows undoing the removal of the
* authorship colors, but is a necessary stopgap measure against
* https://github.com/ether/etherpad-lite/issues/2802
*/
if (event && (event.eventType !== "clearauthorship")) {
stack.pushEvent(event);
}
}
undoPtr = 0;
}
Expand Down

0 comments on commit 301e65b

Please sign in to comment.