Conversation
…, 8, 10, 11, 12, 13, 14) Round 2 of the security review. Implements the remaining fixes from the original review (PR #149) that the owner asked to be addressed. Authentication fixes (login/index.php): - Issue 1: new users no longer get stuck — OTP code generation and email sending now run for both new and existing users. Previously the code was only generated in the else branch, so freshly created users were shown "check your email" but never received a code. - Issue 2: per-IP rate limiting on auth flows (new func/rate_limit.php): send_code 10 attempts / hour / IP verify_code 5 wrong attempts → invalidate the user's OTP (no IP lockout per spec, but the user must request a fresh code to continue) verify_totp 5 wrong attempts → 15-minute IP block verify_totp_setup 5 wrong attempts → 15-minute IP block - Issue 7: OAuth IdentityProviderException no longer leaks the raw exception message. Renders a generic error page with a collapsible <details>Debug</details> block; full message still logged server-side. - Issue 10: session validity changed from 3600 → 1800 to match session_config.php's gc_maxlifetime. - Issue 11: start_authenticated_session() now calls regenerate_csrf_token() so a token captured pre-login cannot be reused. - Issue 12: TOTP QR code now rendered locally with chillerlan/php-qrcode (added to composer.json). The previous api.qrserver.com call sent the otpauth:// URL — and therefore the TOTP secret — to a third party that could go offline, log the secret, or be MITM'd by a corporate firewall. - Issue 14: server-side filter_var($email, FILTER_VALIDATE_EMAIL) on send_code. CSRF (func/csrf.php): - Added regenerate_csrf_token() helper. Race condition (src/db.php, login/index.php): - Issue 13: first-user-admin assignment is now race-safe via INSERT ... ON DUPLICATE KEY UPDATE on a config row. The first thread to insert wins; concurrent threads update the existing row and lose. A new default config key 'first_user_admin_id' is added. - IS_FIRST_RUN constant updated to consider whether the admin slot has been claimed. GET → POST conversions (Issue 3): - admin/pedidos.php: aprovar and rejeitar now require POST + CSRF. The PHP handler reads from POST/GET via a unified $src array. JS submits a hidden POST form with the action and parameters. bulk_approve / bulk_reject were already POST and unchanged. - reservar/manage.php: apagar now requires POST + CSRF. - reservas/index.php: delete button submits a hidden POST form with a CSRF token (the page now includes func/csrf.php and exposes the token to JS via window.__csrfToken). Session security (Issue 6): - 17 files now require_once func/session_config.php before session_start(): index.php, login/index.php, reservar/*, reservas/index.php, admin/index.php, admin/relatorios.php, and all 9 admin/api/* files. Applies the secure cookie/session settings (httponly, samesite=Lax, secure-if-HTTPS, strict mode, 48-char session ID, 30-min GC maxlifetime) to every entry point. TOTP secret exposure (Issue 8): - admin/api/users_search.php: replaced the raw totp_secret column in the SELECT with a computed (totp_secret IS NOT NULL AND totp_secret != '') AS hasTotp boolean, so the TOTP secret never leaves the database. New database table: - rate_limits (ip, action, attempts, window_start, blocked_until) with composite primary key (ip, action). Not addressed in this commit (per owner direction): - Issue 4 (audit log POST/GET data) — kept as expected - Issue 5 (proxy IP headers) — server-side concern - Issue 9 (cookie not cleared on session_destroy) — not relevant - Issue 15 (sidebarLink escaping) - Issue 16 (config validation) - Issue 17 (explicit $db->close()) - Issue 18 (unescaped $_GET in logs) - Issue 19 (recipients_preview type coercion)
…e.php The recent commit converted the 'apagar' action to POST+CSRF, but the old GET-based switch case at line 607 (lines 607-669) was left in place. A GET request to /reservar/manage.php?subaction=apagar&... bypassed CSRF entirely and still deleted reservations. The POST handler at the top of the file (lines 227-242) is the authoritative code path. The switch case is now a deliberate no-op that falls through to the detail-view case, so a GET request shows the reservation detail instead of deleting it. Fixes finding from second security review (C2).
… broke the race claim) The race-safety fix from the previous commit relied on INSERT ... ON DUPLICATE KEY UPDATE config_value = config_value returning affected_rows === 1 for the winning thread. However, src/db.php pre-seeded 'first_user_admin_id' with an empty value in \, so the row always existed and the 'ON DUPLICATE KEY UPDATE' branch was a no-op. MySQL reports affected_rows = 0 for a no-op update, so \ was always false and the first user was never made admin. This commit removes 'first_user_admin_id' from the default seed list. The row is now only inserted when create_pending_user actually claims it, restoring the intended race-safe behavior: the first thread to insert gets affected_rows = 1, subsequent threads get affected_rows = 2 (or 0) and lose. Fixes finding C3 from the second security review.
…ion only in URL) The recent commit converted aprovar/rejeitar to POST+CSRF and read \ from \ (= \ for POSTs). The submitBulkAction() JS helper was unchanged: it built a POST form with the subaction in the URL (?subaction=bulk_approve) and the reservations array in the body. The PHP handler therefore saw \['subaction']=null and the switch matched nothing, so the bulk actions silently did nothing — the user saw a success modal and the page re-rendered with the same pending list. Two changes fix this: 1. submitBulkAction() now puts subaction in the body via a hidden input, so it round-trips in \ as expected. 2. The PHP handler also falls back to \['subaction'] when \['subaction'] is missing, so existing callers that put it in the URL still work. Fixes finding C2 (second review) — bulk actions via the UI.
… TOTP lockout The record_attempt() function used a hard-coded 'INTERVAL 1 HOUR' when deciding whether to reset the attempts counter. check_rate_limit() correctly used the caller-supplied \ (900 for TOTP, 3600 for send_code), but record_attempt() did not. This created a 45-minute window of unlimited TOTP attempts after a 15-min block expired: the 15-min window in check_rate_limit() expired and the function returned true, while record_attempt() never reset the counter (because 15 < 60 min). After the block expired, the user could try TOTP codes indefinitely until the actual 1-hour mark. Fix: pass \ to record_attempt() and use INTERVAL ? SECOND in the SQL, so the reset threshold matches the configured window. All four callers in login/index.php (send_code, verify_code, verify_totp, verify_totp_setup) now pass their actual window. Fixes Bug #1 from the second review.
…to POST+CSRF admin/users.php, admin/salas.php, and admin/tempos.php each had a destructive 'apagar' case (and users.php also had 'removetotp') reachable via a plain GET request, with the target id in the URL query string. The global CSRF check in admin/index.php only applies to POSTs, so a top-level navigation to e.g. /admin/users.php?action=apagar&id=<victim> would silently delete the user (or strip their TOTP, enabling account takeover via email-OTP login) without any CSRF protection. For each file: 1. A guard at the top of the switch rejects non-POST requests for destructive actions with HTTP 405. 2. The action handler now reads the id from \ (the form body) instead of \. 3. The HTML and JS delete/removetotp links are replaced with inline POST forms. The global CSRF injector in admin/index.php adds a csrf_token hidden input on submit. The chained 'removetotp' + email-OTP takeover vector on another admin is now closed: an attacker who can click a link as the admin still cannot bypass the global POST+CSRF check. Fixes C1 from the second security review.
\$_SERVER['REQUEST_URI'] echoed raw into an HTML attribute lets an attacker craft a URL that injects a script tag into the page when the victim clicks the link while authenticated. Escape the value. Fixes H1 from the second review.
…users.php) addslashes() does not escape '</script>'. An admin (or attacker with admin session) who sets the internal_email_domain config value to '</script><script>alert(1)</script>' would inject arbitrary JS that runs in every admin viewing the user list. Use json_encode() with JSON_HEX_TAG | JSON_HEX_AMP | JSON_HEX_APOS | JSON_HEX_QUOT instead. The resulting value is a properly-quoted JS string literal that cannot break out of the script tag. Fixes H3 from the second review.
…lues Several admin/user pages echoed database-fetched sala and tempo ids and names directly into HTML attributes and element content without htmlspecialchars. Admin-controlled names (e.g. an admin sets sala nome to '><script>alert(1)</script> via admin/salas.php) would execute JS in every user visiting a page that lists the affected sala. Wrap id and nome interpolations with htmlspecialchars(..., ENT_QUOTES, 'UTF-8') in: - reservar/index.php (sala select) - admin/materiais.php (sala select, two places) - admin/reservaemmassa.php (tempo checkbox value/id) Fixes H2 from the second review.
The blocked-email check in the email-OTP path did not guard
against an empty regex. preg_match('', '...') returns false
with a deprecation/warning in modern PHP, and admin-controlled
regex values that are syntactically invalid (or ReDoS-prone)
would error out at runtime. The OAuth path (line 771) already
has the !empty() check; this aligns the email-OTP path with it.
Use @preg_match() and explicit === 1 comparison so an invalid
pattern silently disables the check instead of leaking warnings
or being bypassed by a return value of false (which would
have logged the user out unexpectedly). Fixes H7.
Two related bugs in the send_code flow:
1. The return value of send_login_code_email() was discarded. SMTP
failures (server misconfig, quota, network) left the user
looking at 'check your email' forever, with no code ever
delivered. Now we surface a generic error and reset the
stage to 'email' so the user can retry.
2. record_attempt('send_code') ran unconditionally, even when
create_pending_user() returned null (transient DB error, race
lost) or the email failed. A handful of transient errors
would lock the IP out for an hour with no email ever sent.
Now we only count the attempt when the email was successfully
queued (or, more precisely, when sendStyledEmail did not
return a failure).
Fixes M5 and M6 from the second review.
…d XSS) post_reservation_content was stored verbatim from POST and rendered as raw HTML in every user-facing reservation page. CKEditor's sanitization runs client-side only, so a raw POST (or a compromised admin session) could store <script> that then executes in every regular user viewing the affected sala's post-reservation page. Two layers of defense: 1. Sanitize on save in admin/salas_postreserva.php with HTMLPurifier. The allowlist permits the markup that CKEditor produces (headings, lists, tables, links with http/https/mailto/tel schemes only) but strips scripts, iframes, javascript: URLs, and event-handler attributes. 2. Re-purify on render in reservar/manage.php (3 sites). Defense in depth for any legacy rows that predate the input-side sanitization. ezyang/htmlpurifier v4.19 is added to composer.json/lock.
The OAuth callback merged any user with a matching email (other
than the current OAuth id) into the new OAuth id, including all
reservas, logs, AND the admin flag. That meant:
- A different IdP / changed tenant returning a different 'sub'
for the same email silently absorbed the original account.
- A long-standing real OAuth user could be replaced by a new
authenticator with the same email.
- The new account inherited the original's admin flag, so
this was a privilege-escalation vector.
The merge now requires the source row to have a pre-registered
prefix (pre_, pending_, admin_first_). Without one of those
prefixes the row is left alone and the OAuth user is created
fresh (or co-exists, depending on INSERT IGNORE semantics).
Fixes C4 from the second review.
The OAuth flow set \[id/nome/email/admin/validity] from the IdP response (line ~681-685) before checking whether TOTP was required. If TOTP was required, the user was redirected to the TOTP step while those session keys were still set. Most pages check isset(\['pending_totp_user']) and redirect away, but a new page (or a future page that forgets the guard) would treat the partially-authenticated session as fully authenticated and grant admin access. The email-OTP path already does the unset() (matches the symmetric case earlier in this file); this commit mirrors that in the OAuth path. Fixes H6 from the second review.
Add a trusted referer helper and use it before rendering back links or deciding whether to show the admin pedidos return button. External or malformed referers now fall back to safe local paths. Track email-code verification failures per user by deriving a stable namespaced action key from the user id. This prevents one user's failed OTP attempts from consuming another user's budget on the same IP while still fitting the existing rate_limits.action column. Add a small PHP helper test covering trusted referer handling and the per-user verify_code action key.
Validate the selected room before rendering reservation week links, showing an invalid classroom error when the sala id does not exist. Week navigation links are now built with http_build_query and escaped before output. Redact sensitive keys recursively from admin action POST/GET logs while preserving non-sensitive request context for auditing. Fix record_attempt's UPDATE bind types so the action parameter is bound as a string, preventing auth rate-limit rows from being matched or updated incorrectly.
|
📝 Manual Update Required? It looks like you've modified core logic. Please check if the user manual needs a refresh! @marpisco |
|
@codex please review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
Pull request overview
This PR implements a broad set of security hardening follow-ups across the ClassLink PHP app, focusing on CSRF protection for destructive actions, XSS/HTML sanitization, OAuth/TOTP flow tightening, request log redaction, and new rate-limiting helpers for authentication.
Changes:
- Added new security helper modules (trusted referer parsing, request data redaction, and DB-backed rate limiting) plus a small CLI-style test for them.
- Hardened auth flows (email code validation + throttling, OTP invalidation/reset behavior, TOTP lockouts, safer OAuth error handling, local QR rendering).
- Converted multiple destructive/admin flows from GET to POST with CSRF enforcement, and expanded output escaping/sanitization in reservation/admin UIs.
Reviewed changes
Copilot reviewed 31 out of 32 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/security_helpers_test.php | Adds a lightweight test script covering new helper functions (trusted referer, redaction, rate-limit internals). |
| src/db.php | Boots session hardening earlier and introduces the rate_limits table + tighter first-run detection. |
| reservas/index.php | Switches reservation deletion UX to POST + CSRF (modal-driven). |
| reservar/manage.php | Enforces CSRF on POST and moves reservation deletion behind POST-only handling. |
| reservar/index.php | Adds session hardening include and strengthens output escaping in room selection/navigation. |
| login/index.php | Adds rate limiting + validation, improves OTP/TOTP handling, renders QR locally, and hardens OAuth error handling/account merge behavior. |
| index.php | Ensures session hardening settings are applied before session start. |
| func/trusted_referer.php | Introduces helper to safely derive same-origin “back” paths from Referer. |
| func/session_config.php | Minor tweak to session config file comments/usage expectations. |
| func/request_redaction.php | Introduces recursive redaction for sensitive request keys before logging. |
| func/rate_limit.php | Adds DB-backed per-IP rate limiting/attempt tracking utilities. |
| func/csrf.php | Adds CSRF token regeneration for auth state changes. |
| composer.json | Adds chillerlan/php-qrcode and ezyang/htmlpurifier dependencies. |
| composer.lock | Locks new dependencies and updates lock metadata accordingly. |
| admin/users.php | Moves destructive actions to POST and updates client rendering to use POST forms. |
| admin/tempos.php | Moves destructive actions to POST (UI template updated accordingly). |
| admin/salas.php | Moves destructive actions to POST (UI template updated accordingly). |
| admin/salas_postreserva.php | Sanitizes post-reservation HTML before storing to DB. |
| admin/reservaemmassa.php | Improves escaping for tempo checkbox attributes. |
| admin/relatorios.php | Ensures session hardening include is applied. |
| admin/pedidos.php | Moves approve/reject/bulk actions to POST handling and updates UI JS submission. |
| admin/materiais.php | Escapes room option values/names in selects. |
| admin/index.php | Adds session hardening include, enforces CSRF for POSTs, and redacts logged request payloads. |
| admin/api/users_search.php | Avoids returning secrets by exposing hasTotp boolean instead of totp_secret. |
| admin/api/tempos_search.php | Applies session hardening include. |
| admin/api/tempo_lookup.php | Applies session hardening include. |
| admin/api/salas_search.php | Applies session hardening include. |
| admin/api/sala_lookup.php | Applies session hardening include. |
| admin/api/requisitor_lookup.php | Applies session hardening include. |
| admin/api/recipients_preview.php | Applies session hardening include. |
| admin/api/dashboard_stats.php | Applies session hardening include. |
| admin/api/api_registos.php | Applies session hardening include. |
Comments suppressed due to low confidence (1)
admin/pedidos.php:1395
submitBulkAction()usesform.submit()on a dynamically-created form. This bypasses the submit event, so the CSRF token auto-injection in admin/index.php won't run and the POST will be blocked. UserequestSubmit()(with fallback) instead.
form.appendChild(input);
document.body.appendChild(form);
form.submit();
}
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bc2abb1403
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The newly-generated delete and remove-TOTP forms post action in the request body, but the switch in users/salas/tempos dispatched only from \['action']. POST submissions fell through to the null case and silently no-op'd. Resolve the action from \ first, falling back to \ so existing link-based GET flows still work, and use the resolved value in both the 405 guard and the switch.
The delete modal posted tempo/sala/data in the body, but the server-side handler required \['subaction'] === 'apagar' and subaction was only in the query string. Either side could be tightened but never both at once: the modal could include subaction in the body, or the handler could accept it from the query string on a POST request. We do both. - modal: also include subaction=apagar in the POST body so the form is self-describing - handler: accept subaction from POST or GET when the request method is POST, so the query-string form used by callers also works The POST + CSRF guard at the top of manage.php is the real safety property; subaction routing is convenience.
admin/pedidos.php builds three forms dynamically (per-row approve, per-row reject, bulk action) and submitted them with form.submit(). Programmatic form.submit() does not fire the submit event, so the global CSRF injector in admin/index.php never ran and every dynamically-built POST was rejected with 403 by the global CSRF guard. Replace form.submit() with a new window.__submitFormWithCsrf helper that uses requestSubmit() (which fires the submit event) and falls back to manual ensureCsrf() + submit() on older browsers. Affects the per-row approve, per-row reject, and bulk submit paths.
parse_url referer host returns just the hostname, while HTTP_HOST typically includes the port (example.test:8080). A direct string compare therefore incorrectly rejected genuinely same-origin absolute referers on non-default ports - the reservation back link then always fell back to /reservar and the manage page no longer recognized navigation from /admin/pedidos.php. Parse both sides into host + port and compare the structured values. Add a small parse_host_and_port helper that handles IPv6 literals and credentials.
The checkbox id was HTML-escaped but the corresponding <label for=...> still received the raw tempo id. Use a single escaped value for both attributes so the association is consistent and there is no path for an injected id to break out of the attribute.
Two P1 issues with the authentication rate limiter are addressed together because they share the same code path. 1. Source IP trust: get_client_ip() previously honoured HTTP_CLIENT_IP and HTTP_X_FORWARDED_FOR on every request, so an attacker able to reach the app directly (or behind a proxy that preserves those headers) could rotate the value on each request to bypass the per-IP rate limit on send_code, verify_code and verify_totp. Both headers are now only consulted when the immediate peer (REMOTE_ADDR) is on a new trusted_proxies allowlist stored in the config table. When no proxies are configured (the default), the application always uses REMOTE_ADDR. 2. Atomic check-and-reserve: the previous check_rate_limit + record_attempt pair had a TOCTOU window: concurrent bursts could all observe a counter below the threshold and proceed; duplicate-key insert failures on the first attempt were silently swallowed, so only one attempt was actually counted for N concurrent ones. Add reserve_rate_limit_attempt() which uses SELECT ... FOR UPDATE inside a transaction to atomically decide and increment. Migrate the send_code, verify_code and verify_totp call sites. clear_attempts() is invoked when a code is reserved but never delivered (transient SMTP/DB failures), preserving the previous 'don't consume rate on SMTP failure' semantics.
There was a problem hiding this comment.
💡 Codex Review
Lines 249 to 254 in a4cf688
Preserve deletion audit logs and notifications
Every reservation deletion now returns immediately after this new POST-path DELETE, while the prior deletion branch that logged who removed the reservation and called sendReservationDeletedEmail() was replaced with a no-op. Consequently successful user and administrator deletions are no longer recorded in the audit log, and affected requisitors receive no deletion notification.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…rvation on SMTP failure
Two follow-up findings from the second review pass.
1. Rightmost untrusted XFF (P1): the previous trusted-proxy check returned the leftmost XFF entry, which is attacker-controlled. Walk X-Forwarded-For from the right, skip any proxies in the trusted_proxies allowlist, and return the first untrusted address. Drop the non-standard HTTP_CLIENT_IP header entirely; no real reverse proxy populates it. The pure resolver is factored into resolve_client_ip() so it can be unit-tested without globals or a database.
2. send_code reservation on SMTP failure (P2): clear_attempts('send_code') on SMTP failure deleted the entire rate-limit row, letting an attacker submit undeliverable addresses to reset the shared IP counter and exceed the 10/hour limit. The reservation now stands on SMTP failure; the user is rate-limited as if the request had succeeded, and the lockout expires in one hour. A per-reservation release would race with concurrent reservations.
|
📝 Manual Update Required? It looks like you've modified core logic. Please check if the user manual needs a refresh! @marpisco |
|
Two follow-up findings from the second review pass are addressed in
Tests in |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8bd0037e93
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…orm values The new delete-user, delete-room, delete-time-slot and remove-TOTP forms used urlencode(\['id']) for both the query-string edit link and the form value. Browsers do not URL-decode form input values, so the server received the percent-encoded ID and compared it against the raw value stored in the database, mismatching for any ID containing URL-special characters (notably OAuth/OIDC 'sub' values used as cache.id, which can contain '/', '+' or '='). Use the URL-encoded value for query strings (where it is required by the URL) and a separate HTML-escaped raw value for form value attributes. Affects the PHP-rendered forms in admin/users.php and admin/materiais.php, and the JS-generated forms in admin/salas.php and admin/tempos.php.
… attempts' semantics The reserve_rate_limit_attempt helper denies when newCount > maxAttempts, so with maxAttempts=5 the 5th wrong attempt is allowed and the 6th is the one that triggers invalidation/block. The original check_rate_limit denied on attempts >= 5, so the 5th attempt was the one that triggered the lockout. Drop maxAttempts from 5 to 4 in the verify_code, verify_totp and verify_totp_setup call sites. With the new threshold the 5th wrong attempt is the one that triggers invalidation/block, matching the documented 'after 5 wrong attempts' budget. send_code keeps maxAttempts=10 because the original used check_rate_limit strictly less-than, and the same numeric budget is preserved.
…dows The reserve function wrote window_start via PHP's date() but read it back via MySQL's UNIX_TIMESTAMP(). The two use independent time-zone settings, so a row inserted by PHP could read back as an epoch several hours off depending on the configured offsets. The window could appear already expired (letting an attacker submit unlimited requests) or remain active hours too long (locking legitimate users out). Use MySQL's NOW() exclusively for both the row write and the window comparison. The window filter in the SELECT is window_start >= DATE_SUB(NOW(), INTERVAL ? SECOND), evaluated by MySQL; the write is INSERT/UPDATE ... window_start = NOW(), also evaluated by MySQL. No PHP/MySQL clock mixing. Also update the legacy record_attempt() helper to the same convention and drop the now-unused rl_record_attempt_update_bind_types() helper. record_attempt() is no longer called by production code; reserve_rate_limit_attempt() is the supported path. The test that referenced the deleted helper is removed.
|
📝 Manual Update Required? It looks like you've modified core logic. Please check if the user manual needs a refresh! @marpisco |
|
Three follow-up findings from the third review pass are addressed in three new commits:
Lint clean, |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 60315a4a8f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…irst admin through profile setup Two follow-up findings from the fourth review pass, addressed together because they share the same code path (the local-auth account creation flow). 1. setup_name bypassed blocked_emails_regex (P1): the setup_name handler called start_authenticated_session and redirected without applying the blocked-email policy. A user whose address matches blocked_emails_regex could request a code, verify it, and set a name to log in, bypassing the policy that applies to existing local and OAuth users in the verify_code success path. Apply the same check (skipped for admins) before redirecting to '/'. 2. First admin skipped profile setup (P2): create_pending_user() assigns the first user an admin_first_* ID (with placeholder name 'Pendente') and a pending_* ID for everyone else, but the verify_code success branch only routes pending_* to the profile-setup step. The first admin therefore logged in permanently named 'Pendente'. Treat admin_first_* like pending_* in the routing check so the first admin also goes through the name-entry step before the authenticated session starts.
…ocking The previous pass replaced check-verify-record with reserve_rate_limit_attempt for verify_code, verify_totp and verify_totp_setup, on the theory that the rate limit should be atomic. But the limit is on wrong codes, not on attempts, and the atomic helper denied the 5th submission before verifyKey ran, so even a correct 5th code was rejected for 15 minutes. The documented 'after 5 wrong attempts' budget effectively allowed only 4 verification attempts. Revert the three handlers to check-verify-record: a wrong code records an attempt and only invalidates/blocks if the count is now at the cap; a correct code is always accepted. The reserve helper remains the right choice for send_code, where the limit is on requests rather than on wrong outcomes. The TOCTOU window (concurrent wrong guesses from the same IP/user passing the same check) is acceptable here because the lockout applies per-user (verify_code) or per-IP (verify_totp) and the user is the only legitimate wrong-codes source for their own session.
|
📝 Manual Update Required? It looks like you've modified core logic. Please check if the user manual needs a refresh! @marpisco |
|
Three follow-up findings from the fourth review pass are addressed in two new commits:
Lint clean, |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f1a9f12af6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The previous fix added admin_first_* to the profile-setup routing branch in verify_code, but that branch redirects to setup before the admin TOTP check runs, and setup_name calls start_authenticated_session directly. The first administrator therefore received a fully authenticated admin session without configuring or verifying TOTP, bypassing the admin_requires_totp policy. After name setup, check whether the user is an admin who needs TOTP. If they have no secret, redirect to totp_setup; if they have a secret, redirect to totp. This mirrors the regular verify_code TOTP enforcement at line 294.
|
Addressed in P1: route first admin from setup_name back into TOTP flow — the previous fix added Lint clean, tests pass. Resolving the thread. |
|
📝 Manual Update Required? It looks like you've modified core logic. Please check if the user manual needs a refresh! @marpisco |
There was a problem hiding this comment.
💡 Codex Review
Lines 53 to 57 in 80d056a
Normalize dot segments before exempting API paths
When the web server resolves dot segments while preserving the original REQUEST_URI, a POST to /admin/api/../users.php is routed to the state-changing admin page but still matches this /admin/api/ prefix and skips CSRF validation. This permits CSRF from an attacker-controlled same-site origin, where the SameSite=Lax session cookie is still attached; resolve or reject ./.. segments before deciding whether to exempt the request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…tempt The UPDATE statement in record_attempt has four ? placeholders (two INTERVAL window lengths, ip, and action) but bind_param received only three values (iss binding windowSeconds, ip, action). mysqli does not throw by default, so every call to record_attempt after the INSERT path (i.e., the second wrong code and onward) silently failed with a bind-param argument-count error, never incrementing the attempts counter past 1. The verify_code, verify_totp and verify_totp_setup rate limits were completely ineffective after the first wrong attempt. Fix the bind to supply windowSeconds twice (iiss), matching the two DATE_SUB placeholders. The reserve_rate_limit_attempt path was unaffected: all its statements have matching placeholder counts.
|
Fixed in P1: bind both rate-window placeholders in record_attempt — the UPDATE statement has four |
|
📝 Manual Update Required? It looks like you've modified core logic. Please check if the user manual needs a refresh! @marpisco |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Lines 249 to 253 in eae9b50
Preserve deletion notifications and audit logging
When any reservation is deleted through the new POST handler, this path immediately deletes the row and redirects without calling sendReservationDeletedEmail() or logAction(). Those calls existed in the removed deletion path, so deletions initiated from the reservations modal now silently stop notifying the requisitor and disappear from the audit log.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
1. reservar/manage.php delete button was still a GET link after the POST-only handler change, so confirming the delete merely reloaded the detail page. Converted to a POST form with CSRF token and hidden subaction field. 2. admin/users.php JS-generated user cards used encodeURIComponent for form values (like the PHP-rendered forms did earlier). Browsers do not URL-decode form input values, so POSTing the percent-encoded ID mismatched the raw cache.id. Use a separate attr-safe idAttr variable with escapeHtml plus single-quote escaping. 3. verify_code rate-limit keys were per-user-per-IP because every helper called rl_get_client_ip(). An attacker could rotate IPs for the same victim and get 5 guesses from each address without invalidating the OTP. Added an optional \ parameter to record_attempt, check_rate_limit and clear_attempts, and all verify_code callers pass '0.0.0.0' so the rate limit is user-global. send_code, verify_totp and verify_totp_setup continue to use the actual IP. Full audit of all 10 bind_param calls across the rate_limit module: all placeholder counts match their bind types (verified in eae9b50 and confirmed unchanged).
|
📝 Manual Update Required? It looks like you've modified core logic. Please check if the user manual needs a refresh! @marpisco |
|
Three follow-up findings from the fifth review pass, addressed in
Full audit: all 10 Lint clean, tests pass. Resolving the threads. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a2848d9677
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| record_attempt($verifyCodeAction, 3600, '0.0.0.0'); | ||
| if (!check_rate_limit($verifyCodeAction, 5, 3600, '0.0.0.0')) { |
There was a problem hiding this comment.
When an attacker submits concurrent wrong codes for the same user, each request verifies the code before reaching this non-atomic record_attempt() call, whose missing-row inserts and read-then-update path can collapse many failures into one recorded attempt. The OTP therefore remains valid after far more than five guesses, allowing a later correct concurrent guess to authenticate. Fresh evidence beyond the existing atomic-rate-limit comment is that the follow-up now deliberately routes the user-global 0.0.0.0 counter through the documented TOCTOU helper instead of an atomic failed-attempt increment.
Useful? React with 👍 / 👎.
| $claimId = 'first_user_claim_' . bin2hex(random_bytes(8)); | ||
| $stmt = $db->prepare("INSERT INTO config (config_key, config_value) VALUES ('first_user_admin_id', ?) ON DUPLICATE KEY UPDATE config_value = config_value"); | ||
| if ($stmt) { | ||
| $stmt->bind_param("s", $claimId); | ||
| $stmt->execute(); | ||
| $isFirstUser = $stmt->affected_rows === 1; // 1 = inserted (we won the race), 0 = updated (someone else got there first) |
There was a problem hiding this comment.
On a fresh installation, this claim is committed before the subsequent cache insert and is never rolled back if that insert fails. An unauthenticated first request can therefore permanently consume the admin claim—for example, a syntactically valid email longer than the cache.email VARCHAR(99) column causes the later insert to fail under MySQL strict mode—leaving cache empty while every later account is created as a non-admin. Put the claim and user insert in one transaction and only retain the claim after the user row succeeds.
Useful? React with 👍 / 👎.
| $content .= '<details style="margin-top: 1rem; text-align: left; font-size: 0.85rem;">'; | ||
| $content .= '<summary style="cursor: pointer; opacity: 0.7;">Debug</summary>'; | ||
| $content .= '<pre style="margin-top: 0.5rem; padding: 0.5rem; background: rgba(0,0,0,0.05); border-radius: 6px; white-space: pre-wrap; word-break: break-word;">' . $debugDetails . '</pre>'; |
There was a problem hiding this comment.
When the identity provider returns an error other than invalid_grant, this renders the raw IdentityProviderException message to the unauthenticated requester inside the Debug section. Provider exception messages can include response bodies and diagnostic details that were intended only for logs, so the new generic-error handling still exposes the sensitive value it logs at line 933; render only the generic message and keep $e->getMessage() server-side.
Useful? React with 👍 / 👎.
| } elseif (!reserve_rate_limit_attempt('send_code', 10, 3600)) { | ||
| // Issue 2: rate limit send_code per IP (10/hour). The reserve | ||
| // is atomic so concurrent bursts cannot exceed the limit; | ||
| // the rejected request is also declined before we spend | ||
| // any SMTP budget. | ||
| $localAuthError = 'Demasiados pedidos. Por favor aguarde antes de tentar novamente.'; |
There was a problem hiding this comment.
When an attacker can rotate source IPs, this per-IP-only reservation gives every address another ten requests and never limits requests for the same recipient. Each accepted request sends another email and overwrites that user's current OTP, so a distributed attacker can continuously email-bomb a victim and prevent their valid code from working despite the new send-code limiter. Add a recipient-scoped limit alongside the IP-scoped budget.
Useful? React with 👍 / 👎.
Summary
Test Plan
php tests/security_helpers_test.phpphp -lNotes
src/config.phpis ignored/untracked local config and intentionally not part of this PR.