diff --git a/app.js b/app.js index e7dfa39ffde..cd351067b0b 100644 --- a/app.js +++ b/app.js @@ -57,11 +57,11 @@ app.get('/admin', routes.isLoggedIn, routes.admin); app.get('/account_details', routes.isLoggedIn, routes.get_account_details); app.post('/account_details', routes.isLoggedIn, routes.save_account_details); app.get('/logout', routes.logout); -app.post('/create', routes.create); -app.get('/destroy/:id', routes.destroy); -app.get('/edit/:id', routes.edit); -app.post('/update/:id', routes.update); -app.post('/import', routes.import); +app.post('/create', routes.isLoggedIn, routes.verifyCsrf, routes.create); +app.post('/destroy/:id', routes.isLoggedIn, routes.verifyCsrf, routes.destroy); +app.get('/edit/:id', routes.isLoggedIn, routes.edit); +app.post('/update/:id', routes.isLoggedIn, routes.verifyCsrf, routes.update); +app.post('/import', routes.isLoggedIn, routes.verifyCsrf, routes.import); app.get('/about_new', routes.about_new); app.get('/chat', routes.chat.get); app.put('/chat', routes.chat.add); diff --git a/mongoose-db.js b/mongoose-db.js index 3c0966eeb25..cae23956eb8 100644 --- a/mongoose-db.js +++ b/mongoose-db.js @@ -5,6 +5,7 @@ var Schema = mongoose.Schema; var Todo = new Schema({ content: Buffer, updated_at: Date, + owner: String, }); mongoose.model('Todo', Todo); diff --git a/public/css/screen.css b/public/css/screen.css index bc625cc470e..a205370a3a3 100644 --- a/public/css/screen.css +++ b/public/css/screen.css @@ -91,6 +91,19 @@ body { height: 18px; } +.del-form { + display: inline; + float: right; + margin: 0; +} + +button.del-btn, button.del-btn-edit { + border: 0; + padding: 0; + cursor: pointer; + background-color: transparent; +} + /* line 28, ../sass/screen.sass */ .del-btn-edit { width: 20px; diff --git a/routes/index.js b/routes/index.js index 6b5455f03e4..0ce1d123f0f 100644 --- a/routes/index.js +++ b/routes/index.js @@ -10,6 +10,7 @@ var readline = require('readline'); var moment = require('moment'); var exec = require('child_process').exec; var validator = require('validator'); +var crypto = require('crypto'); // zip-slip var fileType = require('file-type'); @@ -53,6 +54,7 @@ exports.loginHandler = function (req, res, next) { function adminLoginSuccess(redirectPage, session, username, res) { session.loggedIn = 1 + session.username = username // Log the login action for audit console.log(`User logged in: ${username}`) @@ -112,6 +114,30 @@ exports.save_account_details = function(req, res, next) { } } +function currentUsername(req) { + var username = req.session && req.session.username; + return req.session && req.session.loggedIn === 1 && typeof username === 'string' && username ? username : null; +} + +exports.verifyCsrf = function (req, res, next) { + var expected = req.session && req.session.csrfToken; + var token = (req.body && req.body._csrf) || req.get('x-csrf-token'); + if (typeof expected === 'string' && typeof token === 'string' && + Buffer.byteLength(token) === Buffer.byteLength(expected) && + crypto.timingSafeEqual(Buffer.from(token), Buffer.from(expected))) { + return next(); + } + return res.status(403).send('Invalid CSRF token'); +}; + +function findOwnedTodo(req, callback) { + var owner = currentUsername(req); + if (!owner || !mongoose.Types.ObjectId.isValid(req.params.id)) { + return callback(null, null); + } + Todo.findOne({ _id: req.params.id, owner: owner }, callback); +} + exports.isLoggedIn = function (req, res, next) { if (req.session.loggedIn === 1) { return next() @@ -172,6 +198,7 @@ exports.create = function (req, res, next) { new Todo({ content: item, updated_at: Date.now(), + owner: currentUsername(req), }).save(function (err, todo, count) { if (err) return next(err); @@ -188,35 +215,41 @@ exports.create = function (req, res, next) { }; exports.destroy = function (req, res, next) { - Todo.findById(req.params.id, function (err, todo) { + findOwnedTodo(req, function (err, todo) { + if (err) return next(err); + if (!todo) return res.status(404).send('Not found'); - try { - todo.remove(function (err, todo) { - if (err) return next(err); - res.redirect('/'); - }); - } catch (e) { - } + todo.remove(function (err, todo) { + if (err) return next(err); + res.redirect('/'); + }); }); }; exports.edit = function (req, res, next) { - Todo. - find({}). - sort('-updated_at'). - exec(function (err, todos) { - if (err) return next(err); + findOwnedTodo(req, function (err, owned) { + if (err) return next(err); + if (!owned) return res.status(404).send('Not found'); - res.render('edit', { - title: 'TODO', - todos: todos, - current: req.params.id + Todo. + find({}). + sort('-updated_at'). + exec(function (err, todos) { + if (err) return next(err); + + res.render('edit', { + title: 'TODO', + todos: todos, + current: req.params.id + }); }); - }); + }); }; exports.update = function (req, res, next) { - Todo.findById(req.params.id, function (err, todo) { + findOwnedTodo(req, function (err, todo) { + if (err) return next(err); + if (!todo) return res.status(404).send('Not found'); todo.content = req.body.content; todo.updated_at = Date.now(); @@ -230,7 +263,12 @@ exports.update = function (req, res, next) { // ** express turns the cookie key to lowercase ** exports.current_user = function (req, res, next) { - + var username = currentUsername(req); + if (username && !req.session.csrfToken) { + req.session.csrfToken = crypto.randomBytes(32).toString('hex'); + } + res.locals.currentUser = username; + res.locals.csrfToken = username ? req.session.csrfToken : ''; next(); }; @@ -285,6 +323,7 @@ exports.import = function (req, res, next) { new Todo({ content: item, updated_at: Date.now(), + owner: currentUsername(req), }).save(function (err, todo, count) { if (err) return next(err); console.log('added ' + todo); diff --git a/tests/todo-authz.test.js b/tests/todo-authz.test.js new file mode 100644 index 00000000000..2c320d04ddf --- /dev/null +++ b/tests/todo-authz.test.js @@ -0,0 +1,104 @@ +const test = require('node:test'); +const assert = require('node:assert'); +const mongoose = require('mongoose'); + +const Todo = mongoose.model('Todo', new mongoose.Schema({ content: Buffer, updated_at: Date, owner: String })); +mongoose.model('User', new mongoose.Schema({ username: String, password: String })); +const routes = require('../routes'); + +const ALICE_ID = '5f0c3b2a9d1e8a0012345678'; +let store; + +Todo.findOne = function (query, cb) { + const doc = store.find((d) => d._id === String(query._id) && d.owner === query.owner) || null; + setImmediate(cb, null, doc); +}; + +function makeTodo(owner) { + const doc = { _id: ALICE_ID, owner, content: 'alice todo' }; + doc.remove = (cb) => { store = store.filter((d) => d !== doc); setImmediate(cb, null, doc); }; + doc.save = (cb) => setImmediate(cb, null, doc, 1); + return doc; +} + +function call(handler, req) { + return new Promise((resolve) => { + const res = { + locals: {}, + status(code) { this.statusCode = code; return this; }, + send() { resolve({ status: this.statusCode }); }, + redirect(location) { resolve({ status: 302, location }); }, + }; + req.get = () => undefined; + handler(req, res, (err) => resolve({ next: true, err })); + }); +} + +const session = (username) => ({ loggedIn: 1, username, csrfToken: 'a'.repeat(64) }); + +test.beforeEach(() => { store = [makeTodo('alice@example.com')]; }); + +test('anonymous requests are redirected by isLoggedIn', async () => { + const r = await call(routes.isLoggedIn, { session: {} }); + assert.deepStrictEqual(r, { status: 302, location: '/' }); +}); + +test('verifyCsrf rejects missing and wrong tokens', async () => { + assert.strictEqual((await call(routes.verifyCsrf, { session: session('alice@example.com'), body: {} })).status, 403); + assert.strictEqual((await call(routes.verifyCsrf, { session: session('alice@example.com'), body: { _csrf: 'b'.repeat(64) } })).status, 403); + assert.strictEqual((await call(routes.verifyCsrf, { session: {}, body: { _csrf: 'x' } })).status, 403); +}); + +test('verifyCsrf rejects same-length non-ASCII tokens with 403', async () => { + const r = await call(routes.verifyCsrf, { session: session('alice@example.com'), body: { _csrf: '\u00e9'.repeat(64) } }); + assert.strictEqual(r.status, 403); +}); + +test('current_user does not create a CSRF token for anonymous sessions', async () => { + const anon = {}; + assert.strictEqual((await call(routes.current_user, { session: anon })).next, true); + assert.strictEqual(anon.csrfToken, undefined); + const alice = { loggedIn: 1, username: 'alice@example.com' }; + await call(routes.current_user, { session: alice }); + assert.match(alice.csrfToken, /^[0-9a-f]{64}$/); +}); + +test('verifyCsrf accepts the session token', async () => { + const r = await call(routes.verifyCsrf, { session: session('alice@example.com'), body: { _csrf: 'a'.repeat(64) } }); + assert.strictEqual(r.next, true); +}); + +test('destroy by a different user returns 404 and keeps the todo', async () => { + const r = await call(routes.destroy, { session: session('bob@example.com'), params: { id: ALICE_ID } }); + assert.strictEqual(r.status, 404); + assert.strictEqual(store.length, 1); +}); + +test('destroy without a session username does not match ownerless lookups', async () => { + const r = await call(routes.destroy, { session: { loggedIn: 1 }, params: { id: ALICE_ID } }); + assert.strictEqual(r.status, 404); + assert.strictEqual(store.length, 1); +}); + +test('destroy by the owner removes the todo', async () => { + const r = await call(routes.destroy, { session: session('alice@example.com'), params: { id: ALICE_ID } }); + assert.deepStrictEqual(r, { status: 302, location: '/' }); + assert.strictEqual(store.length, 0); +}); + +test('update by a different user returns 404 and leaves content unchanged', async () => { + const r = await call(routes.update, { session: session('bob@example.com'), params: { id: ALICE_ID }, body: { content: 'defaced' } }); + assert.strictEqual(r.status, 404); + assert.strictEqual(store[0].content, 'alice todo'); +}); + +test('update by the owner changes content', async () => { + const r = await call(routes.update, { session: session('alice@example.com'), params: { id: ALICE_ID }, body: { content: 'edited' } }); + assert.strictEqual(r.status, 302); + assert.strictEqual(store[0].content, 'edited'); +}); + +test('malformed ids return 404', async () => { + const r = await call(routes.destroy, { session: session('alice@example.com'), params: { id: 'not-an-id' } }); + assert.strictEqual(r.status, 404); +}); diff --git a/views/edit.ejs b/views/edit.ejs index 22b15a8bdbd..57d69bcab7c 100644 --- a/views/edit.ejs +++ b/views/edit.ejs @@ -3,6 +3,7 @@

<%= title %>

+
@@ -17,16 +18,20 @@ <% if( todo._id == current ){ %> +
- <% }else{ %> + <% }else if( todo.owner === currentUser ){ %> <%= todo.content %> + <% }else{ %> + <%= todo.content %> <% } %> - <% if( todo._id == current ){ %> - Delete - <% }else{ %> - Delete + <% if( todo.owner === currentUser ){ %> +
+ + +
<% } %>
<% }); %> diff --git a/views/index.ejs b/views/index.ejs index fef7c6c3ad4..15a77567c5d 100644 --- a/views/index.ejs +++ b/views/index.ejs @@ -9,16 +9,28 @@ strong {font-weight: bold}
+<% if( currentUser ){ %>
+
+<% }else{ %> +
Log in to add or change todos.
+<% } %> <% todos.forEach( function ( todo ){ %>
+ <% if( currentUser && todo.owner === currentUser ){ %> <%- marked(new String(todo.content)) %> - Delete +
+ + +
+ <% }else{ %> + <%- marked(new String(todo.content)) %> + <% } %>
<% }); %>
diff --git a/views/layout.ejs b/views/layout.ejs index eea8491b05d..8fdae6e5dfa 100644 --- a/views/layout.ejs +++ b/views/layout.ejs @@ -14,14 +14,17 @@