Initial project

[jeppekroghitk]

Link to ticket

#7792

Description

This is the recreation of project-database - a drupal 9 project, that served to house project/initiatives across ITK.

In later PRs, the core functionality of project-database-app will be implemented into a dashboard in this project.

The goal is to create a home for projects across ITK — one shared, searchable place where every initiative is registered and described in a consistent way, so the whole organisation can get an overview of what's underway, who owns it, where it stands and how it's funded, surface connections and synergies between teams, and avoid work being duplicated or siloed.

Screenshot of the result

Checklist

• My code is covered by test cases.
• My code passes our test (all our tests).
• My code passes our static analysis suite.
• My code passes our continuous integration process.

[tuj]

You should implement 100% PHP test coverage from the start and a coverage gate in GitHub actions. See economics for examples:
https://github.com/itk-dev/economics/blob/develop/.github/workflows/pr.yml#L49

---

AI review: (I don't know why it considers it a symfony 7 project...)

Code Review — PR #1: Initial project database (Symfony 7)

Context

This branch (feature/7792-project-database-initial-project) is the initial Symfony 7
build of the itk-project-database — ~19k lines: controllers, Doctrine entities, forms,
repositories, Twig templates, fixtures, Docker config, and CI workflows. The user asked
for a code review. This file records the findings (verified against the code) and a
recommended fix plan. No code has been changed.

Overall: a clean, modern, type-safe build. declare(strict_types=1) throughout, backed
enums, parameterized Doctrine queries everywhere, whitelisted sort columns, Vich uploads
served behind an authenticated controller, CSRF on login + delete, login throttling.
Verdict: solid initial build, one fix needed before merge + one decision to document.

---

Findings by severity

🔴 Critical — must fix before merge

1. Stored XSS via initiative link URLs — templates/initiative/show.html.twig:42
   {% for link in initiative.links %}
2. <a href="{{ link }}" ...>{{ link }}
{% endfor %}
The href value is not protocol-validated. A user can store javascript:alert(...) as a
link; Twig's HTML-escaping does not neutralize the javascript: scheme in an href.
InitiativeType uses UrlType with default_protocol => 'https' (InitiativeType.php:134-148),
but default_protocol only prepends a scheme when none is present — it does not reject a
value that already carries javascript:. Initiative::setLinks() only filters empty strings.
Since every ROLE_USER sees every initiative, this is stored XSS against other authenticated
users (incl. admins). Verified.

🟠 High

2. Flat authorization model — no per-resource / per-action access control.
   InitiativeController, ContactController, MediaController carry no #[IsGranted] and rely
   solely on the firewall ^/ → ROLE_USER. Any ROLE_USER can edit/delete any initiative or
   contact, and download any upload by enumerating /media/image/{id} (MediaController.php:24-34).
   The MediaController docblock claims files are "never publicly reachable" — true, but they are
   reachable by every logged-in user. This may be the intended trust model for an internal tool.
   Needs an explicit decision, documented (README/ADR) — or a Voter / role split if restriction
   is wanted. (UserController is correctly gated with #[IsGranted('ROLE_ADMIN')].)

3. CSV export loads the full result set + lazy collections into memory — InitiativeController.php:56.
   ->getQuery()->getResult() hydrates every matching Initiative and then, per row in the stream
   closure, lazy-loads contacts/tags/stakeholders/strategies (N+1). Fine now, a scaling risk.
   Use toIterable() with batched clear(), or eager-join the serialized collections.

🟡 Medium

4. countByStatus() dead branch — InitiativeRepository.php:94. getScalarResult() returns
   raw scalars, so $status instanceof Status is always false; only (string) $status ever runs.
   Works by accident — simplify to the string cast. Verified.

5. Paginator clamps the page number after running the query with the unclamped offset —
   Paginator.php:29-35. ?page=999 runs with a huge offset (0 rows) then shows "page N of M"
   with an empty list. Clamp $page against page count before setFirstResult.

6. TermRepository::findOrCreate race — TermRepository.php:39-60. Concurrent creation of the
   same tag can both pass findOneBy then hit the unique constraint on flush (one request 500s).
   Catch UniqueConstraintViolationException and re-fetch, or accept as known low-risk.

7. Migration boilerplate — Version20260623093956.php. Still has "Please modify to your
   needs!", empty getDescription(), and generated comments. Give it a real description.

🟢 Low / suggestions

• AppFixtures.php:38,45 — hardcoded 'password'; add a dev-only comment (fixtures are dev/test).
• security.yaml test hasher sets both bcrypt cost and argon time/memory_cost; only one
  applies under algorithm: auto — keep the relevant knobs.
• LocaleController open-redirect guard is good; also reject \ (backslash).
• User::getName() silently falls back to email — not a pure accessor; document or move fallback
  to the template.
• Conventions: branch name lacks the issue- segment; commits aren't Conventional Commits;
  CHANGELOG says "Symfony 8" while this is Symfony 7 — reconcile wording.

✅ Verified good

Parameterized queries + whitelisted sort (InitiativeRepository), uploads outside web root behind
auth with Image/File MIME+size constraints + SmartUniqueNamer, auto password hasher + CSRF login +
throttling + self-delete guard, clean enum domain model, open-redirect-safe locale switcher.

---

Recommended fix plan (if the user wants fixes applied)

1. XSS (critical): add new Assert\Url(protocols: ['http', 'https']) to the links entry
   options in InitiativeType.php; optionally harden Initiative::setLinks() to drop non-http(s)
   values and add a starts with 'http' guard in show.html.twig:42.
2. Authorization decision (high): confirm "all ROLE_USER equal" is intended; document it in the
   README, and tighten the MediaController docblock. Add a Voter only if restriction is wanted.
3. Quick cleanups: simplify countByStatus() (#4); clamp page before query in Paginator (#5);
   write a real migration description (#7).
4. Optional/defer: CSV streaming (#3), findOrCreate race (#6), low-priority items.

[tuj]

Consider making this configurable from .env

[tuj]

Consider making this configurable