[FINAL REVIEW] Complete security review — CSRF, prepared statements, transactions, merge with main

[Copilot]

This pass implements the security fixes identified in the original review, the follow-up items from PR #152 (CSV import), and a comprehensive re-review of all overlapping files after merging with main.

---

Changes implemented

OAuth & CSRF (original scope)

• OAuth callback state validation enforced — validates $_GET['state'] against $_SESSION['oauth2state'] with hash_equals(); rejected attempts logged with audit context
• CSRF protection wired into mutating POST flows — admin-wide CSRF check on all POST (with API-route bypass); JS auto-injection of CSRF tokens into all POST forms; explicit csrf_token_field() on all forms
• DB connection error output hardened — connection failures log to error_log() server-side, return generic user-facing message

Post-merge review & new features (follow-up from #152)

• materiais.php apagar action: GET → POST with CSRF — destructive deletion was a plain GET link; converted to POST form with csrf_token_field()
• Explicit CSRF tokens on forms that relied solely on JS injection — criar_completo, update, and mass reservation forms now have explicit csrf_token_field() for defense-in-depth
• Transaction wrappers for CSV import & mass reservation — $db->begin_transaction() / commit / rollback on both loops; separate $insertErrorCount avoids rolling back on validation-only errors
• All $db->query() with real_escape_string() → prepared statements — 6 occurrences in reservar/manage.php replaced with $db->prepare() + bind_param() per coding rules

Main branch merge

• Merged main into branch, resolved conflict in admin/relatorios.php (kept both CSRF validation and pending TOTP/setup flow guards)

Already verified (no changes needed)

• salas_search.php & tempos_search.php — LIKE wildcard escaping already present (fixed in Adicionar importação de reservas em massa via CSV com modal de pesquisa de IDs no painel admin #152)
• New API endpoints (requisitor_lookup, sala_lookup, tempo_lookup) — properly secured with auth guards, wildcard escaping, prepared statements
• post_reservation_content from CKEditor intentionally kept as raw HTML (admin-controlled rich content)

Merge status

• Base: main (b748299)
• Mergeable: ✅ clean (no conflicts)
• Files changed: 8 | Commits: 10 | Lint: all files pass php -l

[marpisco]

@copilot
OAuth callback is missing state validation -> oauth2state is generated, but the callback path does not verify it before exchanging the code. ----> Go ahead, and implement it.
Token expiry is handled incorrectly -> Session validity is derived from time() + $accessToken->getExpires(), which treats an absolute timestamp as a duration. -----> Pretty sure that because of Microsoft's short duration time, I've done multiple checks + adds on the expiration time of a session on purpose to add more time to that expiry time so it doesn't expire both so soon and also so it doesn't expire when you're actively using it (for example, for multiple hours at a time)
CSRF protection is not wired into mutating forms -> The project has a CSRF helper, but POST handlers do not enforce it. -------> implement it on all forms
Admin bootstrap emits HTML before authorization checks -> Some admin entry points render output before confirming session/admin state. -------> do not mess with this yet, we can look into it after the other points have been generated
DB connection errors can leak internals -> Connection failure handling exposes raw connector errors. ------> it won't, as PHP's mysqli is protected against this, however if you have any idea of how to hide even more details (like the folder it is on, etc when display_errors are enabled [which on prod they aren't]), then go ahead and implement it.

[Copilot]

@copilot
OAuth callback is missing state validation -> oauth2state is generated, but the callback path does not verify it before exchanging the code. ----> Go ahead, and implement it.
Token expiry ...

Implementei os pontos pedidos: validação de oauth2state no callback OAuth, proteção CSRF nos formulários/handlers POST (login/admin/reservar), e hardening da mensagem de erro de ligação à BD (sem expor detalhes internos ao utilizador). Mantive a lógica de expiração de sessão como estava e não mexi no ponto do bootstrap admin. Commit de referência: c10b25a.

[marpisco]

As per new feature that was implemented (Importação Massiva de Reservas por CSV #152),

"Post-merge follow-ups (not blocking, but recommended):

Add CSRF protection to remaining materiais.php actions (criar, criar_completo, update, apagar) — currently unprotected
Fix LIKE wildcard escaping in existing admin/api/salas_search.php and admin/api/tempos_search.php — currently unescaped, inconsistent with the new lookup endpoints
Consider adding a transaction wrapper around the CSV import loop for atomicity (partial import rollback on error)"

Remember to implement all of this later on.

[github-actions]

📝 Manual Update Required? It looks like you've modified core logic. Please check if the user manual needs a refresh! @marpisco

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[github-actions]

📝 Manual Update Required? It looks like you've modified core logic. Please check if the user manual needs a refresh! @marpisco

[marpisco]

Security review completed — all items addressed

From @marpisco's follow-up (post #152 merge):

1. ✅ CSRF on remaining materiais.php actions — apagar converted from GET to POST with CSRF token; explicit csrf_token_field() on criar_completo and update forms (defense-in-depth, the admin-wide CSRF check already covered them)

2. ✅ LIKE wildcard escaping — Already present in salas_search.php and tempos_search.php (fixed by Adicionar importação de reservas em massa via CSV com modal de pesquisa de IDs no painel admin #152). Verified — both use str_replace(['%', '_'], ['\\%', '\\_'], $search) with ESCAPE '\\' in queries.

3. ✅ Transaction wrapper for CSV import loop — $db->begin_transaction() / commit / rollback added to both admin/reservaemmassa.php CSV import and mass reservation creation loops. Uses separate $insertErrorCount to avoid rolling back on validation-only errors.

Additional findings & fixes from re-review:

Severity	File	Issue	Fix
Medium	reservar/manage.php	6x $db->query() with real_escape_string()	Replaced all with $db->prepare() + bind_param()
Low	admin/reservaemmassa.php	Mass reservation form no explicit CSRF	Added csrf_token_field()

Merge status

• ✅ Merged with latest main (conflict in relatorios.php resolved)
• ✅ All 8 changed files pass php -l
• ✅ Git reports clean merge with main (no conflicts)

Ready for final review.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.