# Engineering Roadmap Non-test engineering work tracked outside `TESTING-BACKLOG.md`. Ordered by priority, not necessarily by when it'll get picked up. --- ## Security ### Add authorization check to `CommentController::deleteComment` **Status:** Not started. `DELETE /api/comment/{id}` (`CommentController::deleteComment`) deletes the comment unconditionally as soon as one is found by ID — the ownership check is commented out: ```php // if ($userId && $comment) { // todo - User should be able to delete only his comments / admin all if ($comment) { $comment->delete(); ``` This route sits in the public `['hashids', 'logger']` middleware group (no `auth:sanctum`), the same public-poll-link surface used to answer polls without login. The frontend (`Comments.vue`'s `canDelete()`) only hides/shows the delete icon client-side based on `isOwner` / `userId === comment.user_id` — it's a UI convenience, not access control. Net effect: anyone who knows or guesses a comment's (hashid-encoded) ID can delete any comment on any poll, regardless of who posted it. **Real fix:** restore and enforce the ownership check server-side — allow deletion only when the requester is the comment's author (matched via the same `user_id`/participant identity used in `addComment`) or the poll's owner/admin. Return 403 otherwise instead of silently deleting. **Why:** Found while reviewing how comments are added/deleted end-to-end (see comment-handling summary in session notes). Low complexity/blast radius per incident (comment deletion, not data exfiltration or account takeover) but a clear, easy-to-exploit authorization gap on an unauthenticated endpoint. **How to apply:** Small, self-contained change to `CommentController::deleteComment`; no frontend change required since `canDelete()` already reflects the intended rule, just not enforced backend-side. ### Replace the support login backdoor with a real admin login-as-user feature **Status:** Stopgap applied, real fix pending until the testing backlog is done. `app/Http/Controllers/Auth/LoginController.php`'s `postLogin()` had a hardcoded `'Engel42'` password that bypassed the real password check for **any** user account by email — a full account-takeover vulnerability, live since `bb6610ab` (Jan 2022). Found while reviewing the auth-flows testing backlog item. **Stopgap already applied** (see `supportLoginBackdoorMatches()`): the bypass now requires an explicit configured password (disabled by default — empty never matches) and logs a warning whenever it's used. **The ROLE_ADMIN check on the target account was removed on 2026-09-19** (`6ab39355`). It was aimed at the wrong party: support uses this to open a **customer's** account and see what the customer sees, so requiring the target to be an admin permitted only the one case the feature is not for. It was never the safety property it read as. What that leaves is worth stating plainly rather than softening: **anyone who knows `SUPPORT_LOGIN_BACKDOOR_PASSWORD` can log in as any account by email.** The audit-trail exposure is closed (every use is logged) and it is off unless deliberately configured, but the blast radius is now every user, not just team accounts. That makes the real fix below more urgent, not less. The password also moved from `env()` in the controller to `config('auth.support_login_backdoor_password')`. Two reasons: `env()` returns its default once `config:cache` has run, so a cached production config would disable the backdoor with no indication; and `$_ENV` beats `putenv()` in Laravel's adapter order, which had quietly made the tests inert — they set the password with `putenv()` and the developer's own `.env` value won, so the backdoor never matched and one test had been passing for entirely the wrong reason. **Real fix:** a proper "log in as user" admin feature — - Gated by an actual authenticated admin session (not a shared password), ideally with a role/permission check rather than just "is a User row". - An explicit action (e.g. an admin-only endpoint/button), not a side effect of the normal login form accepting a magic string. - An audit log entry per use: who impersonated whom, when, and ideally why. - Once built, remove `supportLoginBackdoorMatches()`, its config key and `SUPPORT_LOGIN_BACKDOOR_PASSWORD` entirely. **Why:** The stopgap reduces blast radius (scoped accounts, requires a non-empty configured secret, logged) but doesn't fix the underlying pattern — a shared secret is still a shared secret. Full replacement was deferred until after the testing backlog to avoid context-switching away from the migration/testing work already in progress; the env-based allowlist keeps it acceptably contained until then. **How to apply:** Don't extend or "productionize" the stopgap further — if it needs more capability, that's the signal to build the real feature instead. Pick this up once `TESTING-BACKLOG.md`'s P0/P1 items are done. ### Install the Laravel scheduler on the production server **Status:** P0, not done — cron has never run on the current disk. `sudo crontab -u www-data -l | grep schedule:run` returns nothing on `kulibri-prod` (195.201.32.252). Production was rebuilt in place on **2026-09-16** for the Ubuntu 24.04 upgrade — same server, same IP, new disk — and a rebuilt disk has no crontabs. `setup-kulibri-backup.yml` either was not applied in step 10 of that runbook or did not take. Everything on the schedule has therefore not run since the rebuild: `auto:reminder` (poll deadline mail), `process:subscriptions` (Mollie cancellations, downgrades, downgrade mail), `process:task`, `backup:database`/`backup:cleanup`, and `health:check --alert` — which is why nobody was told. The external uptime monitor polls `GET /api/health` over HTTP and kept passing the whole time, because Apache and PHP-FPM were healthy; only the cron half was missing. **Real fix** — from the `ansible-thinkpi` repo, *not* this one: ```bash cd ~/ansible-thinkpi ansible-playbook playbooks/setup-kulibri-backup.yml --ask-vault-pass -e target_group=prod ansible-playbook playbooks/authorize-kulibri-backup-key.yml --ask-vault-pass -e target_group=prod ``` - Preserve the existing dumps first. The cleanup guard added on 2026-09-19 keeps the newest one, but the other 16 are all past the 14-day window, and the offsite rsync runs with `--delete`. The runbook's hardlink trick is the cheap way: `ssh mitko@65.21.184.46 'cp -al ~/db-backups ~/db-backups-precutover'` - `process:subscriptions` has days of unprocessed Mollie cancellations queued behind it and it sends downgrade mail and writes invoices. Run it by hand once and read the output before it fires unattended. **Why:** Moving these jobs into the Laravel scheduler was meant to make them travel with the repo instead of living only in server config. That only helps if the one line driving the scheduler is installed on the box, and a rebuild silently removes it. **How to apply:** Before anything else on this list. There is no fresh restore point and no reminder mail going out until it is done. ### Why the backup history ends on 2026-09-01 **Status:** Open question — needs one command to answer. The dumps seeded onto the rebuilt server run 2026-08-18 to 2026-09-01 and stop there, fifteen days before the 09-16 rebuild. Step 10 of the upgrade runbook seeds that directory *from the offsite host*, so the offsite history itself already ended on 09-01. Two possibilities, with very different consequences: - The old box stopped producing dumps on 09-01, in which case the scheduler was already broken there and the gap is ~3 weeks, not 3 days. - The old box kept dumping but its 01:01 offsite rsync stopped, in which case the dumps existed locally until the old disk went away. **How to find out:** ```bash ssh mitko@65.21.184.46 'ls -la ~/db-backups/ ~/db-backups-precutover/ | head -40' ``` If `~/db-backups-precutover/` (the hardlink copy the runbook takes before cutover) also ends on 09-01, the first explanation is the right one. It also matters for how far back a restore can actually go. ### `mysqldump` omits routines, triggers and events **Status:** Not started — deliberately left alone during the 2026-09-19 backup hardening rather than changed silently. `backup:database` dumps with `--single-transaction` only. Stored routines, triggers and events are **not** included, so a restore from these dumps rebuilds the tables and data but not that schema-adjacent logic. If the schema has none of those objects this is a non-issue; nobody has verified which is the case. **Real fix:** check with `SELECT routine_name FROM information_schema.routines WHERE routine_schema='kulibri'` (and the equivalent for `triggers`/`events`). If any exist, add `--routines --triggers --events` to the command. It changes what the dumps contain, so it wants a deliberate decision and a test restore, not a drive-by flag. **How to apply:** One query to find out whether it matters at all. ### Use a dedicated least-privilege MySQL user for `backup:database` **Status:** Not started — command supports it, DB user doesn't exist yet. Still reads the app's own credentials on every nightly dump. `app/Console/Commands/BackupDatabase.php` (added while moving the DB backup/cleanup cron jobs into the Laravel scheduler, see `Hosting.md`) falls back to the app's normal `DB_USERNAME`/`DB_PASSWORD` when `DB_BACKUP_USERNAME`/`DB_BACKUP_PASSWORD` aren't set. That means the nightly `mysqldump` currently runs with the same full read/write credential the app itself uses, purely to do a read-only dump. **Real fix:** - On the server, create a MySQL user scoped to just what `mysqldump --single-transaction` needs: `SELECT`, `SHOW VIEW`, and `PROCESS` (only if using `--single-transaction` with other connections active) on the `kulibri` database — no `INSERT`/`UPDATE`/`DELETE`/`DROP`. - Set `DB_BACKUP_USERNAME`/`DB_BACKUP_PASSWORD` in `.env` to that user (already wired up in `.env.example` and read by the command — no code change needed, just the DB-side `CREATE USER`/`GRANT` and the env values). **Why:** This whole cron migration was motivated by finding the MySQL root password sitting in plaintext in root's crontab (see `Hosting.md`). Moving the dump into Laravel fixed the plaintext-in-crontab part, but defaulting to the app's own DB credentials still means a leaked backup credential (env file, log line, etc.) would grant full write access, not just read access to take a backup. **How to apply:** One-time server-side `CREATE USER`/`GRANT` plus setting two env vars — no further code changes required. Do this before or during the new Ubuntu server cutover, not urgent enough to block it. --- ## Features ### Notify a participant when they are promoted off a waitlist **Status:** Not started — deliberately deferred, P1. Decided by Andreas, 24.09.2026, after being raised as a risk in the 23.09.2026 implementation report. `ParticipantController::promoteFromWaitlist` sets the longest-waiting `preferences.value = 3` entry to `1` when a seat frees up on a full choice. Nobody is told. There is no participant-facing notification mechanism at all to reuse: `options.notifyOnNewAnswer` and `notifyOnNewUser` mail the poll admin, not the respondents. This is the one place in the codebase where a request writes to a *different* participant's row, and it does so silently. The practical consequence is that a wrong promotion has no detection path — neither the promoted person nor the admin learns anything until someone turns up to a course they were not booked into, or fails to turn up to one they were. Accepted for launch on purpose so the rest of the Kursbuchung work can ship, explicitly not because the risk is thought to be small. Tracked here as its own named item rather than being folded into "waitlist works", which is what the decision asked for. Needs a new participant-side mail trigger fired from the promotion, plus a decision on whether the promoted person can decline the seat they were given without asking for it. --- ## Code cleanup ### Misleading route name/method for the design-usage-check endpoint **Status:** Not started — functionally correct, naming only. `routes/api.php` defines `DELETE /designs/{pollId}/{id}` with the route name `deleteDesign`, mapped to `DesignController@findOther`. Despite the name, this endpoint deletes nothing — it counts how many *other* polls still reference a given design (`Poll::where('id', '!=', $pollId) ->where('design_id', $id)->count()`) and returns `{count: n}`. The actual deletion happens through a separate route, the implicit `DELETE /designs/{id}` from `Route::resource('/designs', ...)`, which calls `destroy()`. The frontend (`frontend/src/components/Edit/CustomBranding.vue`) uses this correctly: `deleteTemplateConfirmation()` calls the `{pollId}/{id}` route first as a "still in use elsewhere?" check to decide whether to show a warning, then a separate confirmed action calls the real `DELETE /designs/{id}` to delete. So current behavior is not a bug. **Real fix:** - Rename the route (e.g. `checkDesignUsage` or `designs.usageCount`) and/or the controller method to match what it actually does. - Consider modeling it as a `GET` instead of `DELETE`, since it's a read-only check, not a mutation. **Why:** Found while investigating a report that a delete-named endpoint called a `findOther` method. No functional bug, but the mismatch between HTTP verb, route name, and method name is exactly the kind of thing that causes someone to misuse the endpoint expecting it to delete. **How to apply:** Rename route/method and update the one frontend call site; low-risk, self-contained change. ### Leftovers from the participant-attribute `name` → foreign-key refactor **Status:** Not started — dead code, no user-visible bug. Migrations `2026_05_21_090803` and `2026_05_21_160717` replaced the string `name` columns on `participant_values` / `participant_attributes` with the `participant_attribute_id` foreign key. The live code paths were converted, but two blocks that still match on `name` stayed behind. Both are currently unreachable, which is why nothing broke: 1. `PollExportService::buildExportHeaders()` reads `$poll->participant_attributes` (snake case is only produced when Eloquent *serialises* a relation, it does not resolve on access — the relation method is `participantAttributes()`), so the value is always `null` and the attribute-header loop never runs. Inside that loop it also reads `$attribute['name']`, a column that no longer exists. The method's result is assigned to `$headers` in `exportPollDetails()` and never used; its companion `writeHeaderRow()` has no call site at all. The actual export writes the attribute headers inline from `$attribute->label` (around line 208) and the values by `participant_attribute_id`, both correct. 2. `RenderMixins.hasUncompleteAttributes()` matches values to definitions with `a.name == attribute.name` — both `undefined` since the refactor, so it would match the first value row for every field. It has no caller; the mandatory check that actually runs is in `ModalParticipant.vue`'s submit handler and maps by `participant_attribute_id`. **Real fix:** delete `buildExportHeaders()`, `writeHeaderRow()` and the `$headers` assignment, or repair them (`participantAttributes`, `label`) and wire `writeHeaderRow()` up in place of the inline header writing — whichever matches the intent of the half-finished refactor. Delete `hasUncompleteAttributes()`. **Why:** Found while summarising how extended participant attributes work (2026-09-18). The risk is not the current behaviour but the next person to touch the export or the poll rendering: both blocks look like the live implementation and would be copied or "fixed" into use. **How to apply:** Pure deletion in two files, plus a run of the Excel export to confirm the header row is unchanged. Do it together with, or after, SEC-008, which touches the same attribute-rendering code. ### Mandatory participant attributes are only enforced client-side **Status:** Not started. `ParticipantAttribute.mandatory` is checked in `ModalParticipant.vue`'s submit handler (`attribute.mandatory && attribute.value == null`). The endpoints that store values — `POST /api/poll/{id}` (`addNewParticipants`) and `PUT /api/participant/{id}` — accept any subset of attributes without checking which fields the poll requires, and `PollController::addParticipantAttributes()` silently skips falsy values. An empty string also passes the client check. **Real fix:** validate on the server against the poll's attribute definitions, rejecting a participant whose mandatory fields are empty (and treating `''` as empty). Decide what happens to participants created *before* a field was made mandatory — `ParticipantAttributeController` backfills them with `value: null`, so they would all count as incomplete. **Why:** Data quality rather than security — the field is the poll owner's own form. Noted with the SEC-008 work since both concern the same validation path.