bug: require login, ownership and CSRF token on todo routes (sfind-ff2cbbbc)

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);

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);

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;

routes/index.js

@@ -10,6 +10,7 @@
 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 @@
 
 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 @@
   }
 }
 
+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 @@
   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.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 @@
 
 // ** 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 @@
       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);

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);
+});

views/edit.ejs

@@ -3,6 +3,7 @@
 <h1 id="page-title"><%= title %></h1>
 <div id="list">
   <form action="/create" method="post" accept-charset="utf-8">
+    <input type="hidden" name="_csrf" value="<%= csrfToken %>" />
     <div class="item-new">
       <input class="input" type="text" name="content" />
     </div>
@@ -17,16 +18,20 @@
 
     <% if( todo._id == current ){ %>
     <form class="update-form" action="/update/<%= todo._id %>" method="post" accept-charset="utf-8">
+      <input type="hidden" name="_csrf" value="<%= csrfToken %>" />
       <input class="update-input" type="text" name="content" value="<%= todo.content %>" />
     </form>
-    <% }else{ %>
+    <% }else if( todo.owner === currentUser ){ %>
     <a class="update-link" href="/edit/<%= todo._id %>" title="Update this todo item"><%= todo.content %></a>
+    <% }else{ %>
+    <span class="update-link"><%= todo.content %></span>
     <% } %>
 
-    <% if( todo._id == current ){ %>
-    <a class="del-btn-edit" href="/destroy/<%= todo._id %>" title="Delete this todo item">Delete</a>
-    <% }else{ %>
-    <a class="del-btn" href="/destroy/<%= todo._id %>" title="Delete this todo item">Delete</a>
+    <% if( todo.owner === currentUser ){ %>
+    <form class="del-form" action="/destroy/<%= todo._id %>" method="post">
+      <input type="hidden" name="_csrf" value="<%= csrfToken %>" />
+      <button class="<%= todo._id == current ? 'del-btn-edit' : 'del-btn' %>" type="submit" title="Delete this todo item">Delete</button>
+    </form>
     <% } %>
   </div>
 <% }); %>

views/index.ejs

@@ -9,16 +9,28 @@ strong {font-weight: bold}
 </div>
 
 <div id="list">
+<% if( currentUser ){ %>
   <form action="/create" method="post" accept-charset="utf-8">
+    <input type="hidden" name="_csrf" value="<%= csrfToken %>" />
     <div class="item-new">
       <input class="input" type="text" name="content" />
     </div>
   </form>
+<% }else{ %>
+  <div class="item-new"><a href="/login?redirectPage=/">Log in</a> to add or change todos.</div>
+<% } %>
 
 <% todos.forEach( function ( todo ){ %>
   <div class="item">
+  <% if( currentUser && todo.owner === currentUser ){ %>
     <a class="update-link" href="/edit/<%= todo._id %>" title="Update this todo item"><%- marked(new String(todo.content)) %></a>
-    <a class="del-btn" href="/destroy/<%= todo._id %>" title="Delete this todo item">Delete</a>
+    <form class="del-form" action="/destroy/<%= todo._id %>" method="post">
+      <input type="hidden" name="_csrf" value="<%= csrfToken %>" />
+      <button class="del-btn" type="submit" title="Delete this todo item">Delete</button>
+    </form>
+  <% }else{ %>
+    <span class="update-link"><%- marked(new String(todo.content)) %></span>
+  <% } %>
   </div>
 <% }); %>
 </div>

views/layout.ejs

@@ -14,14 +14,17 @@
     </div>
     <div id="footer-wrap">
 
+        <% if( currentUser ){ %>
         <form ref='uploadForm' 
             id='uploadForm' 
             action='/import' 
             method='post' 
             encType="multipart/form-data">
+                <input type="hidden" name="_csrf" value="<%= csrfToken %>" />
                 <input type="file" name="importFile" />
                 <input type='submit' value='Import!' />
-        </form>		
+        </form>
+        <% } %>
         <button><a href="/public/about.html">About</a></button>  
     </div>
   </body>