From 62cb5e5268bb08709645539fa2659535d36b10da Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 5 Oct 2026 12:16:45 +0000 Subject: [PATCH] Fix findings of the security review Authentication - Refuse requests the web server did not authenticate (no REMOTE_USER), so an ignored .htaccess or a missing nginx auth block fails closed instead of leaving all data open. Opt out with $require_http_auth = false in config.php. - .htaccess/nginx: demand HTTPS before any password prompt, send HSTS, pass REMOTE_USER to PHP-FPM, and block helper scripts with FilesMatch, which also covers requests like queries/db.php/x (the on REQUEST_URI did not). - Session cookie gets the Secure flag behind a TLS-terminating proxy. Data changes - Reactivation of due recurring entries no longer happens as a side effect of GET query-todos.php (no CSRF protection, ran for all users): it is now the POST endpoint reactivate-due.php, limited to the lists of the current user, and claims each entry in recurringCopied first so concurrent requests cannot create duplicates. - Merging a tag into itself deleted the tag and all its assignments: rejected on the server and no longer offered in the UI. Tag edit/delete/merge run in a transaction. - Tag endpoints check ownership (tag used by the current user's todos and by nobody else) and validate their parameters; a missing tag_name no longer blanks the tag name. - Data endpoints answer with application/json. Tests: regression tests for each of the above, tests/auth.test.js for the authentication check, extended tests/apache-access.sh. Co-Authored-By: Claude Claude-Session: https://claude.ai/code/session_01LeVfWCjSSQSgKKHpGbjKWK --- .github/workflows/tests.yml | 2 +- .htaccess | 15 ++++- config.sample.php | 12 ++++ lang/de-DE.ini | 2 + lang/en-US.ini | 2 + nginx.conf.sample | 9 ++- queries/complete.php | 6 +- queries/db.php | 21 +++++++ queries/delete-tag.php | 6 +- queries/edit-tag.php | 9 ++- queries/empty-trash.php | 2 +- queries/merge-tag.php | 15 ++++- queries/query-lists.php | 2 +- queries/query-tags.php | 2 +- queries/query-todos.php | 3 +- queries/reactivate-due.php | 7 +++ queries/reactivate-temp.php | 13 +++- queries/reactivate.php | 11 ++-- queries/trash.php | 6 +- session.php | 4 +- tests/README.md | 8 ++- tests/apache-access.sh | 17 +++-- tests/api.test.js | 122 +++++++++++++++++++++++++++++++++--- tests/auth.test.js | 86 +++++++++++++++++++++++++ todo-core.php | 23 +++++++ todo.js | 42 ++++++++----- 26 files changed, 385 insertions(+), 62 deletions(-) create mode 100644 queries/reactivate-due.php create mode 100644 tests/auth.test.js diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 08e7b86..b49ad46 100644 --- a/.github/workflows/tests.yml +++ b/.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 ' config.php + printf ' config.php - name: Start PHP server run: | php -S 127.0.0.1:8000 > php-server.log 2>&1 & diff --git a/.htaccess b/.htaccess index cfdf177..7ffd6a7 100644 --- a/.htaccess +++ b/.htaccess @@ -6,20 +6,29 @@ AuthType Basic AuthName "Todo" # create with: htpasswd -c /path/outside/webroot/.htpasswd 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) + + Require expr "%{HTTPS} == 'on' || %{HTTP:X-Forwarded-Proto} == 'https'" + Require valid-user + # 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) Require all denied # helper scripts which are only included by the query endpoints - + Require all denied - + 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" diff --git a/config.sample.php b/config.sample.php index 6e272cc..1d2ca64 100644 --- a/config.sample.php +++ b/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; diff --git a/lang/de-DE.ini b/lang/de-DE.ini index 357c483..cd408d7 100644 --- a/lang/de-DE.ini +++ b/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!" diff --git a/lang/en-US.ini b/lang/en-US.ini index 639460b..33f839d 100644 --- a/lang/en-US.ini +++ b/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!" diff --git a/nginx.conf.sample b/nginx.conf.sample index 73ee957..acf7f60 100644 --- a/nginx.conf.sample +++ b/nginx.conf.sample @@ -2,6 +2,10 @@ # 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 auth_basic "Todo"; auth_basic_user_file /etc/nginx/todo.htpasswd; @@ -9,16 +13,19 @@ location /todo/ { 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; } } diff --git a/queries/complete.php b/queries/complete.php index 14b6793..bcbdd4f 100644 --- a/queries/complete.php +++ b/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)); diff --git a/queries/db.php b/queries/db.php index 1377cd7..35153dc 100644 --- a/queries/db.php +++ b/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; + } +} diff --git a/queries/delete-tag.php b/queries/delete-tag.php index 38641af..4986358 100644 --- a/queries/delete-tag.php +++ b/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) { diff --git a/queries/edit-tag.php b/queries/edit-tag.php index 18f7559..a93588a 100644 --- a/queries/edit-tag.php +++ b/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"); diff --git a/queries/empty-trash.php b/queries/empty-trash.php index 9cebdee..343f84a 100644 --- a/queries/empty-trash.php +++ b/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; diff --git a/queries/merge-tag.php b/queries/merge-tag.php index d1743b5..69e24d3 100644 --- a/queries/merge-tag.php +++ b/queries/merge-tag.php @@ -2,12 +2,20 @@ 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 ". @@ -15,6 +23,7 @@ "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) { diff --git a/queries/query-lists.php b/queries/query-lists.php index 36b6aae..bf00431 100644 --- a/queries/query-lists.php +++ b/queries/query-lists.php @@ -8,4 +8,4 @@ $allResults[] = $stuff; } $db->close(); -echo json_encode($allResults); +sendJson(json_encode($allResults)); diff --git a/queries/query-tags.php b/queries/query-tags.php index e2eb40a..06aa830 100644 --- a/queries/query-tags.php +++ b/queries/query-tags.php @@ -14,4 +14,4 @@ $allResults[] = $stuff; } $db->close(); -echo json_encode($allResults); +sendJson(json_encode($allResults)); diff --git a/queries/query-todos.php b/queries/query-todos.php index 5c8b6a4..933ac9e 100644 --- a/queries/query-todos.php +++ b/queries/query-todos.php @@ -1,6 +1,5 @@ close(); - echo $result; + sendJson($result); diff --git a/queries/reactivate-due.php b/queries/reactivate-due.php new file mode 100644 index 0000000..f681fa9 --- /dev/null +++ b/queries/reactivate-due.php @@ -0,0 +1,7 @@ +close(); diff --git a/queries/reactivate-temp.php b/queries/reactivate-temp.php index 7b6e6e4..3dbf417 100644 --- a/queries/reactivate-temp.php +++ b/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(); } diff --git a/queries/reactivate.php b/queries/reactivate.php index 36841c8..e4cea07 100644 --- a/queries/reactivate.php +++ b/queries/reactivate.php @@ -1,11 +1,12 @@ affected_rows; diff --git a/session.php b/session.php index 0de52c2..cadcb98 100644 --- a/session.php +++ b/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, diff --git a/tests/README.md b/tests/README.md index b1025d9..2006983 100644 --- a/tests/README.md +++ b/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`). diff --git a/tests/apache-access.sh b/tests/apache-access.sh index ae8954a..4b436e1 100644 --- a/tests/apache-access.sh +++ b/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 diff --git a/tests/api.test.js b/tests/api.test.js index 506f7ec..d75c200 100644 --- a/tests/api.test.js +++ b/tests/api.test.js @@ -1,6 +1,6 @@ const test = require('node:test'); const assert = require('node:assert/strict'); -const { sql, sqlRows, resetDb, session, post, get } = require('./lib'); +const { BASE, sql, sqlRows, resetDb, session, post, get } = require('./lib'); const todoFields = (over) => Object.assign({ todo: 'item', due: '', start: '', effort: 1, notes: '', tags: '', @@ -12,7 +12,7 @@ test.beforeEach(resetDb); test('modifying endpoints reject requests without valid CSRF token', async () => { const sess = await session(); const endpoints = ['enter.php', 'update.php', 'complete.php', 'trash.php', 'empty-trash.php', - 'reactivate-one.php', 'edit-tag.php', 'delete-tag.php', 'merge-tag.php']; + 'reactivate-one.php', 'reactivate-due.php', 'edit-tag.php', 'delete-tag.php', 'merge-tag.php']; for (const ep of endpoints) { assert.equal((await post(ep, { id: 1 })).status, 403, ep + ' without token'); assert.equal((await post(ep, { id: 1 }, sess, 'wrong')).status, 403, ep + ' with wrong token'); @@ -51,14 +51,20 @@ test('reactivating a recurring entry copies stored values verbatim (no second-or assert.equal(sql(`SELECT COUNT(*) FROM todo_tags WHERE todo_id=${Number(id) + 1}`).trim(), '1'); }); -test('due recurring entries are reactivated when loading the list', async () => { - const sess = await session(); - const id = (await post('enter.php', { todo: 'weekly', due: '2026-01-10', start: '2026-01-08', tags: 'w', list_id: 0 }, sess)).text; +// a completed weekly entry which is due for reactivation, in the given list +async function completedDueEntry(sess, list_id) { + const id = (await post('enter.php', { todo: 'weekly', due: '2026-01-10', start: '2026-01-08', tags: 'w', list_id }, sess)).text; await post('update.php', todoFields({ id, version: 1, todo: 'weekly', due: '2026-01-10', start: '2026-01-08', - recurrenceMode: 2, recurrenceInterval: 1, recurrenceAnchor: 1, tags: 'w' }), sess); + recurrenceMode: 2, recurrenceInterval: 1, recurrenceAnchor: 1, tags: 'w', list_id }), sess); await post('complete.php', { id, completed: 1, version: 2 }, sess); - const res = await get('query-todos.php?list_id=0&age=10000&incomplete=true'); - const items = JSON.parse(res.text); + return id; +} + +test('due recurring entries are reactivated by reactivate-due.php', async () => { + const sess = await session(); + await completedDueEntry(sess, 0); + assert.equal((await post('reactivate-due.php', {}, sess)).text, '1'); + const items = JSON.parse((await get('query-todos.php?list_id=0&age=10000&incomplete=true')).text); const copy = items.find((i) => i.completed == 0); assert.equal(copy.todo, 'weekly'); assert.equal(copy.due, '2026-01-17 00:00:00'); @@ -66,6 +72,36 @@ test('due recurring entries are reactivated when loading the list', async () => assert.equal(copy.tags, 'w'); }); +test('loading the list does not modify any data', async () => { + const sess = await session(); + await completedDueEntry(sess, 0); + const before = sql('SELECT COUNT(*) FROM todo').trim(); + for (let i = 0; i < 2; i++) { + assert.equal((await get('query-todos.php?list_id=0&age=10000&incomplete=true')).status, 200); + } + assert.equal(sql('SELECT COUNT(*) FROM todo').trim(), before); + assert.equal(sql('SELECT COUNT(*) FROM recurringCopied').trim(), '0'); +}); + +test('reactivate-due.php only touches the lists of the current user', async () => { + const sess = await session(); + sql("INSERT INTO todo (id, creationDate, description, startDate, dueDate, completed, completionDate, " + + "recurrenceMode, recurrenceInterval, recurrenceAnchor, list_id) VALUES " + + "(100, UTC_TIMESTAMP(), 'foreign', '2026-01-08', '2026-01-10', 1, '2026-01-10 10:00:00', 2, 1, 1, 2)"); + await completedDueEntry(sess, 0); + await post('reactivate-due.php', {}, sess); + assert.equal(sql("SELECT COUNT(*) FROM todo WHERE description='foreign'").trim(), '1'); + assert.equal(sql("SELECT COUNT(*) FROM todo WHERE description='weekly'").trim(), '2'); +}); + +test('concurrent reactivation creates only one copy', async () => { + const sess = await session(); + await completedDueEntry(sess, 0); + await Promise.all(Array.from({ length: 6 }, () => post('reactivate-due.php', {}, sess))); + assert.equal(sql("SELECT COUNT(*) FROM todo WHERE description='weekly'").trim(), '2'); + assert.equal(sql('SELECT COUNT(*) FROM recurringCopied').trim(), '1'); +}); + test('recurrence supports arbitrary intervals in days, weeks, months and years', async () => { const sess = await session(); const cases = [[1, 10, '2026-01-20'], [2, 3, '2026-01-31'], [3, 2, '2026-03-10'], [4, 10, '2036-01-10']]; @@ -103,6 +139,66 @@ test('lists and todos of other users can neither be read nor changed', async () [['100', 'foreign', '0', '0', '2'], [own, 'own', '0', '0', '0']]); }); +// two todos in list 0 with the given tags (comma separated), returns the tag ids by name +async function taggedTodos(sess, tagsOfTodo1, tagsOfTodo2) { + await post('enter.php', { todo: 'one', due: '', start: '', tags: tagsOfTodo1, list_id: 0 }, sess); + await post('enter.php', { todo: 'two', due: '', start: '', tags: tagsOfTodo2, list_id: 0 }, sess); + return Object.fromEntries(sqlRows('SELECT name, id FROM tags')); +} +const assignments = () => sqlRows('SELECT t.description, g.name FROM todo_tags r JOIN todo t ON t.id=r.todo_id ' + + 'JOIN tags g ON g.id=r.tag_id ORDER BY 1, 2').map((r) => r.join(':')); + +test('a tag cannot be merged into itself', async () => { + const sess = await session(); + const tag = await taggedTodos(sess, 'a,b', 'a'); + assert.equal((await post('merge-tag.php', { id: tag.a, merge_id: tag.a }, sess)).text, + 'A tag cannot be merged into itself!'); + assert.deepEqual(assignments(), ['one:a', 'one:b', 'two:a']); + assert.equal(sql('SELECT COUNT(*) FROM tags').trim(), '2'); +}); + +test('merging tags moves the entries and keeps each only once', async () => { + const sess = await session(); + const tag = await taggedTodos(sess, 'a,b', 'a'); + assert.equal((await post('merge-tag.php', { id: tag.a, merge_id: tag.b }, sess)).text, '1'); + assert.deepEqual(assignments(), ['one:b', 'two:b']); +}); + +test('tag endpoints validate their parameters', async () => { + const sess = await session(); + const tag = await taggedTodos(sess, 'keep', ''); + assert.equal((await post('edit-tag.php', { id: tag.keep }, sess)).text, 'Invalid parameters!'); + assert.equal((await post('edit-tag.php', { id: tag.keep, tag_name: ' ' }, sess)).text, 'Invalid parameters!'); + assert.equal((await post('merge-tag.php', { id: tag.keep }, sess)).text, 'Invalid parameters!'); + assert.equal(sql('SELECT name FROM tags').trim(), 'keep'); + assert.equal((await post('edit-tag.php', { id: tag.keep, tag_name: ' ' }, sess)).text, '1'); + assert.equal(sql('SELECT name FROM tags').trim(), '<b>'); +}); + +test('tags of other users, or shared with them, can not be changed', async () => { + const sess = await session(); + const denied = 'Access denied: this list or entry does not belong to you!'; + // 50 is only used by another user, 51 by both, 52 by nobody + sql("INSERT INTO tags (id, name) VALUES (50, 'foreign'), (51, 'shared'), (52, 'unused'); " + + "INSERT INTO todo (id, creationDate, description, startDate, list_id) VALUES " + + "(100, UTC_TIMESTAMP(), 'foreign', UTC_DATE(), 2); " + + "INSERT INTO todo_tags (todo_id, tag_id) VALUES (100, 50), (100, 51)"); + await post('enter.php', { todo: 'own', due: '', start: '', tags: 'shared,mine', list_id: 0 }, sess); + const mine = sqlRows("SELECT id FROM tags WHERE name='mine'")[0][0]; + const tagsBefore = sqlRows('SELECT id, name FROM tags ORDER BY id'); + const assignmentsBefore = sql('SELECT todo_id, tag_id FROM todo_tags ORDER BY 1, 2'); + for (const tag of [50, 51, 52]) { + assert.equal((await post('edit-tag.php', { id: tag, tag_name: 'x' }, sess)).text, denied, 'edit ' + tag); + assert.equal((await post('delete-tag.php', { id: tag }, sess)).text, denied, 'delete ' + tag); + assert.equal((await post('merge-tag.php', { id: tag, merge_id: mine }, sess)).text, denied, 'merge ' + tag); + assert.equal((await post('merge-tag.php', { id: mine, merge_id: tag }, sess)).text, denied, 'merge into ' + tag); + } + assert.deepEqual(sqlRows('SELECT id, name FROM tags ORDER BY id'), tagsBefore); + assert.equal(sql('SELECT todo_id, tag_id FROM todo_tags ORDER BY 1, 2'), assignmentsBefore); + // an own tag can still be edited: + assert.equal((await post('edit-tag.php', { id: mine, tag_name: 'renamed' }, sess)).text, '1'); +}); + test('invalid input is rejected', async () => { const sess = await session(); assert.equal((await post('enter.php', { todo: 'a', due: '2026-01-10x', start: '', list_id: 0 }, sess)).text, 'Invalid due date!'); @@ -135,8 +231,16 @@ test('database errors are not shown to the client', async () => { } }); +test('data endpoints answer with a JSON content type', async () => { + for (const path of ['query-lists.php', 'query-tags.php?list_id=0', 'query-todos.php?list_id=0']) { + const res = await fetch(BASE + '/queries/' + path); + assert.match(res.headers.get('content-type'), /^application\/json/, path); + JSON.parse(await res.text()); + } +}); + test('pages send security headers', async () => { - const res = await fetch(require('./lib').BASE + '/index.php'); + const res = await fetch(BASE + '/index.php'); assert.match(res.headers.get('content-security-policy'), /default-src 'self'/); assert.match(res.headers.get('content-security-policy'), /frame-ancestors 'none'/); assert.equal(res.headers.get('x-content-type-options'), 'nosniff'); diff --git a/tests/auth.test.js b/tests/auth.test.js new file mode 100644 index 0000000..1b1d809 --- /dev/null +++ b/tests/auth.test.js @@ -0,0 +1,86 @@ +const test = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const os = require('node:os'); +const net = require('node:net'); +const path = require('node:path'); +const { spawn } = require('node:child_process'); + +// The application relies on the web server's HTTP authentication and refuses to run without it. +// These tests start their own PHP servers on a copy of the application: the guard rejects +// requests before the database is used, so no database is needed here. +const ROOT = path.join(__dirname, '..'); + +function freePort() { + return new Promise((resolve) => { + const srv = net.createServer().listen(0, '127.0.0.1', () => { + const { port } = srv.address(); + srv.close(() => resolve(port)); + }); + }); +} + +async function startServer(dir, args) { + const port = await freePort(); + const proc = spawn('php', ['-S', '127.0.0.1:' + port, '-t', dir, ...args], { stdio: 'ignore' }); + const base = 'http://127.0.0.1:' + port; + for (let i = 0; i < 50; i++) { + try { await fetch(base + '/lang-js.php'); return { base, proc }; } catch (e) { /* not up yet */ } + await new Promise((r) => setTimeout(r, 100)); + } + proc.kill(); + throw new Error('PHP server did not start'); +} + +let dir; +const servers = []; +test.before(() => { + dir = fs.mkdtempSync(path.join(os.tmpdir(), 'todo-auth-')); + fs.cpSync(ROOT, dir, { recursive: true, filter: (src) => !/[\\/](node_modules|\.git)([\\/]|$)/.test(src) }); + fs.writeFileSync(path.join(dir, 'config.php'), + ' { + servers.forEach((s) => s.proc.kill()); + fs.rmSync(dir, { recursive: true, force: true }); +}); + +const pages = ['index.php', 'statistik.php', 'log.js.php', 'queries/query-lists.php', 'queries/query-todos.php?list_id=0']; + +test('requests the web server did not authenticate are rejected', async () => { + const srv = await startServer(dir, []); + servers.push(srv); + for (const page of pages) { + for (const headers of [{}, { Authorization: 'Basic ' + Buffer.from('alice:secret').toString('base64') }]) { + const res = await fetch(srv.base + '/' + page, { headers }); + assert.equal(res.status, 403, page); + assert.match(await res.text(), /HTTP authentication/, page); + } + } + const post = await fetch(srv.base + '/queries/trash.php', { method: 'POST' }); + assert.equal(post.status, 403); +}); + +test('requests authenticated by the web server (REMOTE_USER) are served', async () => { + const srv = await startServer(dir, [path.join(dir, 'router-auth.php')]); + servers.push(srv); + const res = await fetch(srv.base + '/index.php'); + assert.equal(res.status, 200); + assert.match(await res.text(), /name="csrf-token"/); +}); + +test('the check can be switched off explicitly in config.php', async () => { + const off = fs.mkdtempSync(path.join(os.tmpdir(), 'todo-auth-off-')); + try { + fs.cpSync(dir, off, { recursive: true }); + fs.appendFileSync(path.join(off, 'config.php'), '$require_http_auth = false;\n'); + const srv = await startServer(off, []); + servers.push(srv); + assert.equal((await fetch(srv.base + '/index.php')).status, 200); + } finally { + servers.forEach((s) => s.proc.kill()); + fs.rmSync(off, { recursive: true, force: true }); + } +}); diff --git a/todo-core.php b/todo-core.php index fa7578d..c6dfe56 100644 --- a/todo-core.php +++ b/todo-core.php @@ -14,9 +14,32 @@ function assetUrl($file) { // TODO: get that from the current user account $curUserID = TodoConstants::DefaultUserID; +// The application has no login of its own, it relies on HTTP authentication of +// the web server. Refuse to run if the web server did not authenticate the +// request, so that a missing or ignored .htaccess / nginx setting does not +// leave all data open. Only REMOTE_USER is trusted: the web server sets it after +// checking the credentials, a client cannot forge it with the Authorization header. +function requireHttpAuth() { + global $require_http_auth; + if (isset($require_http_auth) && $require_http_auth === false) { + return; + } + foreach (array('REMOTE_USER', 'REDIRECT_REMOTE_USER') as $key) { + if (!empty($_SERVER[$key])) { + return; + } + } + error_log("todo: request without authenticated user rejected, see \$require_http_auth in config.sample.php"); + http_response_code(403); + echo TodoLang::_("AUTH_REQUIRED"); + exit; +} + // inline styles are still used (statistics bars, jQuery UI), inline scripts are not header("Content-Security-Policy: default-src 'self'; style-src 'self' 'unsafe-inline'; ". "img-src 'self' data:; object-src 'none'; base-uri 'self'; form-action 'self'; frame-ancestors 'none'"); header("X-Content-Type-Options: nosniff"); header("X-Frame-Options: DENY"); header("Referrer-Policy: same-origin"); + +requireHttpAuth(); diff --git a/todo.js b/todo.js index a18bcd2..9e17489 100644 --- a/todo.js +++ b/todo.js @@ -678,9 +678,34 @@ function toggleWorking(show) { function reload() { log($T('LOADING_TODO_LIST')); + // recurring entries which are due are copied first; that modifies data, + // so it is a POST request (with CSRF token) and not part of loading the list + $.ajax({ + url: 'queries/reactivate-due.php', + type: 'POST' + }).always(loadTodos); + + $.ajax({ + url: 'queries/query-lists.php', + type: 'GET', + dataType: 'text', + data: listsData + }).done( function(responseText) { + try { + lists = JSON.parse(responseText); + } catch (e) { + alert($T('SERVER_DELIVERED_INVALID_DATA')+': "'+responseText+ + '";'+$T('JSON_PARSER_MESSAGE')+' : '+e); + } + renderLists(); + }); +} + +function loadTodos() { $.ajax({ url: 'queries/query-todos.php', type: 'GET', + dataType: 'text', data: reloadData }).done(function(responseText) { try { @@ -703,20 +728,6 @@ function reload() { updateProgress(); log($T('LOADING_FINISHED')); }); - - $.ajax({ - url: 'queries/query-lists.php', - type: 'GET', - data: listsData - }).done( function(responseText) { - try { - lists = JSON.parse(responseText); - } catch (e) { - alert($T('SERVER_DELIVERED_INVALID_DATA')+': "'+responseText+ - '";'+$T('JSON_PARSER_MESSAGE')+' : '+e); - } - renderLists(); - }); } function getUTCDate() { @@ -1023,7 +1034,8 @@ function openTagDialog(tagname) }); var tagify = $('#merge_tag_edit')[0].__tagify; tagify.removeAllTags(); - tagify.settings.whitelist = tagList.map( (x) => x.name ); + // a tag cannot be merged into itself: + tagify.settings.whitelist = tagList.filter( (x) => x.id != result[0].id ).map( (x) => x.name ); $('#tag_todo_table').empty(); var filtered = getTodoWithTag(new Array(tagname)); filtered.sort(ItemSort);