Security hardening: fail-closed auth, POST-only reactivation, tag endpoint fixes

.github/workflows/tests.yml

@@ -40,7 +40,7 @@ jobs:
       - name: Set up database and configuration
         run: |
           mysql -h127.0.0.1 -uroot -proot todo < sql/install.sql
-          printf '<?php\n$db_host = "127.0.0.1"; $db_user = "root"; $db_password = "root"; $db_database = "todo";\n$language = "en-US";\n' > config.php
+          printf '<?php\n$db_host = "127.0.0.1"; $db_user = "root"; $db_password = "root"; $db_database = "todo";\n$language = "en-US";\n$require_http_auth = false;\n' > config.php
       - name: Start PHP server
         run: |
           php -S 127.0.0.1:8000 > php-server.log 2>&1 &

.htaccess

@@ -6,20 +6,29 @@ AuthType Basic
 AuthName "Todo"
 # create with: htpasswd -c /path/outside/webroot/.htpasswd <user>
 AuthUserFile /path/outside/webroot/.htpasswd
-Require valid-user
+# HTTPS is checked first, so that browsers are never asked for the password
+# over plain HTTP (X-Forwarded-Proto: for a proxy which terminates TLS)
+<RequireAll>
+    Require expr "%{HTTPS} == 'on' || %{HTTP:X-Forwarded-Proto} == 'https'"
+    Require valid-user
+</RequireAll>
 
 # never serve internal files directly
 RedirectMatch 404 ^.*/(sql|scripts|tests|node_modules|lang|\.git|\.github)(/|$)
+# (FilesMatch also applies to requests with a trailing path like db.php/x, unlike
+# a match on the request URI)
 <FilesMatch "(^\.|~$|\.(sql|sh|ini|bak|swp|orig|md|yml|lock|sample)$|^config.*\.php$|^package(-lock)?\.json$|^session\.php$|^todo-core\.php$|^lang\.php$)">
     Require all denied
 </FilesMatch>
 # helper scripts which are only included by the query endpoints
-<If "%{REQUEST_URI} =~ m#/queries/(db|date|tags|todo-list-query|reactivate|reactivate-temp)\.php$#">
+<FilesMatch "^(db|date|tags|todo-list-query|reactivate|reactivate-temp|recurrence)\.php$">
     Require all denied
-</If>
+</FilesMatch>
 
 <IfModule mod_headers.c>
     Header always set X-Content-Type-Options "nosniff"
     Header always set X-Frame-Options "DENY"
     Header always set Referrer-Policy "same-origin"
+    # ignored by browsers over plain HTTP
+    Header always set Strict-Transport-Security "max-age=31536000"
 </IfModule>

config.sample.php

@@ -17,6 +17,11 @@
 # its own. Only deploy it behind HTTP
 # authentication and over HTTPS, see
 # .htaccess (Apache) or nginx.conf.sample.
+# Requests the web server did not authenticate
+# (no REMOTE_USER) are rejected, so a missing
+# or ignored web server configuration does not
+# leave your data open. Keep this file outside
+# of the web root if your setup allows it.
 #
 ########################################
 # database connection settings:
@@ -37,3 +42,10 @@
 # note: the file with the name $language.ini from lang
 # folder will be used to translate all strings
 
+########################################
+# authentication check:
+# only set this to false if the application is
+# protected in some other way than by HTTP
+# authentication of the web server (e.g. network
+# access control), and never on a public server.
+# $require_http_auth = false;

lang/de-DE.ini

@@ -119,3 +119,5 @@ INVALID_RECURRENCE_ANCHOR="Ungültiger Wiederholungs-Bezugspunkt!"
 DATABASE_ERROR="Datenbankfehler!"
 METHOD_NOT_ALLOWED="Methode nicht erlaubt!"
 INVALID_CSRF_TOKEN="Ungültiges oder fehlendes CSRF-Token, bitte lade die Seite neu!"
+AUTH_REQUIRED="Zugriff verweigert: der Webserver hat dich nicht authentifiziert. Schütze die Anwendung mit HTTP-Authentifizierung (siehe config.sample.php)!"
+CANNOT_MERGE_TAG_INTO_ITSELF="Ein Tag kann nicht mit sich selbst zusammengeführt werden!"

lang/en-US.ini

@@ -119,3 +119,5 @@ INVALID_RECURRENCE_ANCHOR="Invalid recurrence anchor!"
 DATABASE_ERROR="Database error!"
 METHOD_NOT_ALLOWED="Method not allowed!"
 INVALID_CSRF_TOKEN="Invalid or missing CSRF token, please reload the page!"
+AUTH_REQUIRED="Access denied: the web server did not authenticate you. Protect the application with HTTP authentication (see config.sample.php)!"
+CANNOT_MERGE_TAG_INTO_ITSELF="A tag cannot be merged into itself!"

nginx.conf.sample

@@ -2,23 +2,30 @@
 # The application has no login of its own: protect it with HTTP authentication,
 # and only serve it over HTTPS.
 location /todo/ {
+    # only HTTPS, so that the password is never sent in cleartext
+    # (remove this if TLS is terminated in front of nginx)
+    if ($scheme != "https") { return 301 https://$host$request_uri; }
+
     # create with: htpasswd -c /etc/nginx/todo.htpasswd <user>
     auth_basic "Todo";
     auth_basic_user_file /etc/nginx/todo.htpasswd;
 
     add_header X-Content-Type-Options "nosniff" always;
     add_header X-Frame-Options "DENY" always;
     add_header Referrer-Policy "same-origin" always;
+    add_header Strict-Transport-Security "max-age=31536000" always;
 
     # never serve internal files directly
     location ~ ^/todo/(sql|scripts|tests|node_modules|lang|\.git|\.github)/ { return 404; }
     location ~ (/\.|~$|\.(sql|sh|ini|bak|swp|orig|md|yml|lock|sample)$|/config[^/]*\.php$|/package(-lock)?\.json$) { return 404; }
     location ~ ^/todo/(session|todo-core|lang)\.php$ { return 404; }
-    location ~ ^/todo/queries/(db|date|tags|todo-list-query|reactivate|reactivate-temp)\.php$ { return 404; }
+    location ~ ^/todo/queries/(db|date|tags|todo-list-query|reactivate|reactivate-temp|recurrence)\.php$ { return 404; }
 
     location ~ \.php$ {
         include fastcgi_params;
         fastcgi_param SCRIPT_FILENAME $request_filename;
+        # the application rejects requests without an authenticated user
+        fastcgi_param REMOTE_USER $remote_user;
         fastcgi_pass unix:/run/php/php-fpm.sock;
     }
 }

queries/complete.php

@@ -2,10 +2,10 @@
     require(__DIR__."/../session.php");
     requirePostWithCsrf();
     require("db.php");
-    $id = (int)$_POST['id'];
+    $id = (int)postParam('id');
     requireOwnTodo($db, $id);
-    $completed = ((int)$_POST['completed'] == 1) ? 1 : 0;
-    $version   = (int)$_POST['version'];
+    $completed = ((int)postParam('completed') == 1) ? 1 : 0;
+    $version   = (int)postParam('version');
     $stmt = dbExec($db, "UPDATE todo SET completed=?, ".
         "completionDate=IF(?=1, UTC_TIMESTAMP(), NULL), version=?+1 WHERE id=? AND version=?",
         array($completed, $completed, $version, $id, $version));

queries/db.php

@@ -46,6 +46,12 @@ function dbQueryOrDie($db, $sql) {
     return $db->query($sql);
 }
 
+// answer with JSON (not as text/html, the default)
+function sendJson($json) {
+    header('Content-Type: application/json; charset=utf-8');
+    echo $json;
+}
+
 function jsonQueryResults($db, $sql, $params = array())
 {
     $qResult = dbExec($db, $sql, $params)->get_result();
@@ -78,3 +84,18 @@ function requireOwnTodo($db, $todo_id) {
         exit;
     }
 }
+
+// stop with an error message unless the given tag is used by todos of the current
+// user and by no todos of other users: tags are shared by name, so changing or
+// deleting a tag which is also used by others would change their data
+function requireOwnTag($db, $tag_id) {
+    global $curUserID;
+    $qResult = dbExec($db, "SELECT COALESCE(SUM(l.user_id=?), 0) AS own, COALESCE(SUM(l.user_id<>?), 0) AS others ".
+        "FROM todo_tags r JOIN todo t ON t.id=r.todo_id JOIN list l ON l.id=t.list_id WHERE r.tag_id=?",
+        array((int)$curUserID, (int)$curUserID, (int)$tag_id))->get_result();
+    $row = $qResult->fetch_object();
+    if ((int)$row->own < 1 || (int)$row->others > 0) {
+        echo TodoLang::_("ACCESS_DENIED");
+        exit;
+    }
+}

queries/delete-tag.php

@@ -2,11 +2,13 @@
     require(__DIR__."/../session.php");
     requirePostWithCsrf();
     require("db.php");
-    $id = (int)$_POST['id'];
-	// TODO: restrict to current list!
+    $id = (int)postParam('id');
+    requireOwnTag($db, $id);
+    $db->begin_transaction();
     $deletedAssignments = dbExec($db, "DELETE FROM todo_tags WHERE tag_id=?", array($id))->affected_rows;
 	// TODO: only delete if no tags left!
     $affectedRows = dbExec($db, "DELETE FROM tags WHERE id=?", array($id))->affected_rows;
+    $db->commit();
     if ($affectedRows < 1) {
         echo TodoLang::_("NO_ROWS_AFFECTED");
     } else if ($affectedRows > 1) {

queries/edit-tag.php

@@ -2,8 +2,13 @@
     require(__DIR__."/../session.php");
     requirePostWithCsrf();
     require("db.php");
-    $id   = (int)$_POST['id'];
-    $name = encodeInput($_POST['tag_name']);
+    $id   = (int)postParam('id');
+    $name = encodeInput(trim(postParam('tag_name')));
+    if ($name == '') {
+        echo TodoLang::_("INVALID_PARAMETERS");
+        die;
+    }
+    requireOwnTag($db, $id);
     $affectedRows = dbExec($db, "UPDATE `tags` SET `name`=? WHERE id=?", array($name, $id))->affected_rows;
     if ($affectedRows < 1) {
         echo TodoLang::_("NO_ROWS_AFFECTED");

queries/empty-trash.php

@@ -2,7 +2,7 @@
     require(__DIR__."/../session.php");
     requirePostWithCsrf();
     require("db.php");
-    $list_id = (int)$_POST["list_id"];
+    $list_id = (int)postParam('list_id');
     requireOwnList($db, $list_id);
     dbExec($db, "DELETE FROM todo WHERE deleted=1 AND list_id=?", array($list_id));
     echo 1;

queries/merge-tag.php

@@ -2,19 +2,28 @@
     require(__DIR__."/../session.php");
     requirePostWithCsrf();
     require("db.php");
-    if (!isset($_POST['id']) || !isset($_POST['merge_id']))
+    $id   = (int)postParam('id');
+    $merge_id = (int)postParam('merge_id');
+    if ($id < 1 || $merge_id < 1)
     {
         die(TodoLang::_("INVALID_PARAMETERS"));
     }
-    $id   = (int)$_POST['id'];
-    $merge_id = (int)$_POST['merge_id'];
+    // the join below would match every entry with itself and delete them all:
+    if ($id == $merge_id)
+    {
+        die(TodoLang::_("CANNOT_MERGE_TAG_INTO_ITSELF"));
+    }
+    requireOwnTag($db, $id);
+    requireOwnTag($db, $merge_id);
+    $db->begin_transaction();
 	// delete entries which already have the merge tag:
 	dbExec($db, "DELETE t1 FROM `todo_tags` AS t1 ".
 		"INNER JOIN `todo_tags` t2 ".
 			"ON t1.`todo_id` = t2.`todo_id` ".
 		"WHERE t1.`tag_id`=? AND t2.`tag_id`=?", array($id, $merge_id));
     dbExec($db, "UPDATE `todo_tags` SET `tag_id`=? WHERE `tag_id`=?", array($merge_id, $id));
     $affectedRows = dbExec($db, "DELETE FROM `tags` WHERE id=?", array($id))->affected_rows;
+    $db->commit();
     if ($affectedRows < 1) {
         echo TodoLang::_("NO_ROWS_AFFECTED");
     } else if ($affectedRows > 1) {

queries/query-lists.php

@@ -8,4 +8,4 @@
     $allResults[] = $stuff;
 }
 $db->close();
-echo json_encode($allResults);
+sendJson(json_encode($allResults));

queries/query-tags.php

@@ -14,4 +14,4 @@
     $allResults[] = $stuff;
 }
 $db->close();
-echo json_encode($allResults);
+sendJson(json_encode($allResults));

queries/query-todos.php

@@ -1,6 +1,5 @@
 <?php
     require("db.php");
-    require("reactivate.php");
     require("todo-list-query.php");
 
     $list_id = (int)$_GET["list_id"];
@@ -10,4 +9,4 @@
     $sql = todoListQuery($incomplete);
     $result = jsonQueryResults($db, $sql, array($list_id, $age));
     $db->close();
-    echo $result;
+    sendJson($result);

queries/reactivate-due.php

@@ -0,0 +1,7 @@
+<?php
+    require(__DIR__."/../session.php");
+    requirePostWithCsrf();
+    require("db.php");
+    require("reactivate.php");
+    echo 1;
+    $db->close();

queries/reactivate-temp.php

@@ -6,6 +6,15 @@
 $qResult = dbQueryOrDie($db, "SELECT id FROM reviving");
 while ($toReactivate = $qResult->fetch_object())
 {
+    $db->begin_transaction();
+    // claim the entry first: if several requests run at the same time,
+    // only one of them gets to copy it
+    $claimed = dbExec($db, "INSERT IGNORE INTO recurringCopied (todo_id, copiedDate) VALUES (?, ?)",
+        array($toReactivate->id, $creationDate))->affected_rows;
+    if ($claimed < 1) {
+        $db->rollback();
+        continue;
+    }
     // new due date: recurrence interval after completion (anchor 0) or after due date (anchor 1);
     // start date keeps its distance to the due date (or equals the due date if there is none)
     $nextDue = recurrenceNextSql("IF(recurrenceAnchor=0, completionDate, dueDate)");
@@ -26,7 +35,5 @@
     dbExec($db, "INSERT INTO todo_tags(todo_id, tag_id) ".
             "SELECT ?, tag_id FROM todo_tags WHERE todo_id=?",
         array($newId, $toReactivate->id));
-
-    dbExec($db, "INSERT INTO recurringCopied (todo_id, copiedDate) VALUES (?, ?)",
-        array($toReactivate->id, $creationDate));
+    $db->commit();
 }

queries/reactivate.php

@@ -1,18 +1,19 @@
 <?php
-    // recurring events reactivation
-    // currently called from query.php
+    // recurring events reactivation, for the lists of the current user;
+    // called by reactivate-due.php (a POST request, as it modifies data),
     // would theoretically be enough to do this once per day or so:
     require_once(__DIR__."/recurrence.php");
     $sql = "CREATE TEMPORARY TABLE reviving AS ".
             "SELECT * FROM todo t WHERE completed=1 and ".
-            "recurrenceMode != 0 AND ((".
+            "recurrenceMode != 0 AND ".
+            "t.list_id IN (SELECT id FROM list WHERE user_id=?) AND ((".
             "recurrenceAnchor = 0 AND ".
             "DATEDIFF(".recurrenceNextSql("completionDate").", UTC_DATE()) ".
                 "< GREATEST(DATEDIFF(dueDate, startDate), 4) ".
             ") OR (".
             "recurrenceAnchor = 1 AND ".
             "DATEDIFF(".recurrenceNextSql("dueDate").", UTC_DATE()) ".
                 "< GREATEST(DATEDIFF(dueDate, startDate), 4) ".
-            ")) AND NOT EXISTS (SELECT 1 FROM recurringCopied r WHERE r.todo_id=t.id);";
-    $qResult = dbQueryOrDie($db, $sql);
+            ")) AND NOT EXISTS (SELECT 1 FROM recurringCopied r WHERE r.todo_id=t.id)";
+    dbExec($db, $sql, array((int)$curUserID));
     require("reactivate-temp.php");

queries/trash.php

@@ -2,10 +2,10 @@
     require(__DIR__."/../session.php");
     requirePostWithCsrf();
     require("db.php");
-    $id = (int)$_POST['id'];
+    $id = (int)postParam('id');
     requireOwnTodo($db, $id);
-    $trash = ((int)$_POST['trash'] == 1) ? 1 : 0;
-    $version = (int)$_POST['version'];
+    $trash = ((int)postParam('trash') == 1) ? 1 : 0;
+    $version = (int)postParam('version');
     $stmt = dbExec($db, "UPDATE todo SET deleted=? WHERE id=? AND version=?",
         array($trash, $id, $version));
     $affectedRows = $stmt->affected_rows;

session.php

@@ -5,7 +5,9 @@ function todoStartSession($readOnly) {
     if (session_status() === PHP_SESSION_ACTIVE) {
         return;
     }
-    $https = !empty($_SERVER['HTTPS']) && $_SERVER['HTTPS'] !== 'off';
+    // also behind a proxy which terminates TLS:
+    $https = (!empty($_SERVER['HTTPS']) && $_SERVER['HTTPS'] !== 'off') ||
+        (isset($_SERVER['HTTP_X_FORWARDED_PROTO']) && $_SERVER['HTTP_X_FORWARDED_PROTO'] === 'https');
     session_name("todo_session");
     session_set_cookie_params(array(
         'lifetime' => 0,

tests/README.md

@@ -2,13 +2,17 @@
 
 Run by `.github/workflows/tests.yml`. Locally:
 
-1. Create a database from `sql/install.sql`, write a matching `config.php`,
-   and start the application, e.g. `php -S 127.0.0.1:8000`.
+1. Create a database from `sql/install.sql`, write a matching `config.php`
+   (with `$require_http_auth = false;`, as the PHP development server does no HTTP
+   authentication), and start the application, e.g. `php -S 127.0.0.1:8000`.
    The tests truncate all tables, so use a separate database.
 2. `npm ci` and `npx playwright install chromium`
    (or set `CHROMIUM_PATH` to an installed Chromium).
 3. `TODO_URL=http://127.0.0.1:8000 TODO_MYSQL="mysql -h127.0.0.1 -uroot -proot todo" npm test`
    (`TODO_MYSQL` is the client command line used to prepare and check the database).
 
+`tests/auth.test.js` starts its own PHP servers (no database needed) to test the check for
+an authenticated user.
+
 `sh tests/apache-access.sh` checks the access rules of `.htaccess`
 with a temporary Apache instance (run as root, needs `apache2` and `htpasswd`).

tests/apache-access.sh

@@ -44,9 +44,10 @@ sleep 1
 URL=http://127.0.0.1:$PORT/todo
 FAIL=0
 # path, expected status without credentials, expected status with credentials
+# (X-Forwarded-Proto: https, as sent by a proxy which terminates TLS, as the test server only speaks HTTP)
 check() {
-    noauth=$(curl -s -o /dev/null -w '%{http_code}' "$URL/$1")
-    auth=$(curl -s -u alice:secret -o /dev/null -w '%{http_code}' "$URL/$1")
+    noauth=$(curl -s -H 'X-Forwarded-Proto: https' -o /dev/null -w '%{http_code}' "$URL/$1")
+    auth=$(curl -s -H 'X-Forwarded-Proto: https' -u alice:secret -o /dev/null -w '%{http_code}' "$URL/$1")
     if [ "$noauth" = "$2" ] && [ "$auth" = "$3" ]; then
         echo "ok   $1 ($noauth/$auth)"
     else
@@ -60,13 +61,21 @@ check vendor/jquery/jquery.min.js 401 200
 check todo.css 401 200
 for denied in sql/install.sql lang/en-US.ini config.php config.sample.php package.json \
         package-lock.json scripts/vendor.sh session.php todo-core.php lang.php queries/db.php \
-        queries/tags.php queries/reactivate-temp.php .htaccess nginx.conf.sample; do
+        queries/tags.php queries/reactivate-temp.php queries/reactivate.php queries/recurrence.php \
+        queries/db.php/x queries/reactivate.php/x queries/reactivate-temp.php/x session.php/x config.php/x \
+        .htaccess nginx.conf.sample; do
     check "$denied" 403 403
 done
 for hidden in .git/HEAD node_modules/jquery/dist/jquery.js tests/lib.js; do
     check "$hidden" 401 404
 done
-headers=$(curl -s -u alice:secret -D - -o /dev/null "$URL/todo.css")
+# plain HTTP: no password prompt (403, not 401), with or without credentials
+for creds in "" "-u alice:secret"; do
+    code=$(curl -s $creds -o /dev/null -w '%{http_code}' "$URL/index.php")
+    if [ "$code" = 403 ]; then echo "ok   plain HTTP index.php ($code) $creds"; else echo "FAIL plain HTTP index.php: got $code, expected 403 ($creds)"; FAIL=1; fi
+done
+headers=$(curl -s -H 'X-Forwarded-Proto: https' -u alice:secret -D - -o /dev/null "$URL/todo.css")
 echo "$headers" | grep -qi '^X-Frame-Options: DENY' || { echo "FAIL missing X-Frame-Options"; FAIL=1; }
 echo "$headers" | grep -qi '^X-Content-Type-Options: nosniff' || { echo "FAIL missing nosniff"; FAIL=1; }
+echo "$headers" | grep -qi '^Strict-Transport-Security: ' || { echo "FAIL missing Strict-Transport-Security"; FAIL=1; }
 exit $FAIL