Feature/backend#15
Open
sahle8141 wants to merge 2 commits into
Open
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR implements a complete PHP REST API backend with database models, controllers, authentication, routing, and comprehensive documentation. It establishes environment configuration, database schema with seed data, helper utilities for slug generation, and provides full CRUD operations across six API modules: articles, authentication, comments, community/forums, events, and resources with learning paths. ChangesBackend Core Implementation
Data Models and API Layer
Controllers and API Endpoints
API Routes and Mail Service
Documentation and Testing
Sequence Diagram(s)sequenceDiagram
participant Client as Client/Browser
participant Router as router.php
participant PublicIndex as public/index.php
participant Controller
participant Model as Database Model
participant DB as Database
Client->>Router: GET /api/articles
Router->>PublicIndex: Route to controller
PublicIndex->>PublicIndex: Load .env variables
PublicIndex->>Controller: Instantiate ArticleController
Controller->>Controller: requireUser() or optionalUser()
Controller->>Model: paginate(filters, viewerId)
Model->>DB: SELECT with JOIN author/category
DB-->>Model: Article rows
Model-->>Controller: Formatted articles + pagination
Controller-->>PublicIndex: {success: true, data: {...}}
PublicIndex-->>Client: JSON 200 OK
sequenceDiagram
participant Client
participant Controller as EventController
participant AuthTrait as AuthenticatesRequests
participant Model as Event Model
participant DB
Client->>Controller: POST /api/events (with auth header)
Controller->>AuthTrait: requireUser()
AuthTrait->>AuthTrait: Decode bearer token
AuthTrait->>DB: SELECT user by ID
DB-->>AuthTrait: User record
AuthTrait-->>Controller: User array
Controller->>Controller: validateEventPayload()
Controller->>Model: create(payload + organizer_id)
Model->>Model: Str::uniqueSlug() for slug
Model->>DB: INSERT event row
DB-->>Model: lastInsertId
Model->>DB: SELECT full event details
DB-->>Model: Event with organizer/attendees
Model-->>Controller: Formatted event
Controller-->>Client: {success: true, data: event, status: 201}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/src/Models/User.php (1)
88-116:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCheck username conflicts during profile updates too.
This path rejects duplicate emails but allows duplicate usernames. If the table enforces uniqueness, the update will fail at write time; if it doesn't, public profile/search endpoints can return ambiguous identities.
Suggested fix
$email = strtolower(trim((string) ($data['email'] ?? $current['email']))); $emailOwner = $this->findByEmail($email); if ($emailOwner !== null && (int) $emailOwner['id'] !== $userId) { return null; } + + $username = trim((string) ($data['username'] ?? $current['username'])); + if ($username !== (string) $current['username'] && $this->existsByUsername($username)) { + return null; + } $statement = $this->db->prepare( 'UPDATE users SET username = :username,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/Models/User.php` around lines 88 - 116, The update path only checks for email conflicts but not username collisions; before executing the UPDATE in the method that builds $statement, normalize the incoming username (e.g., $username = strtolower(trim((string) ($data['username'] ?? $current['username'])))), call a lookup like $this->findByUsername($username) (or implement findByUsername if missing), and if a record exists with a different id (similar to the existing email check) return null to prevent duplicate usernames; then use that normalized $username in the $statement->execute payload so the same value is written to the users table.
🟠 Major comments (19)
backend/README.md-62-63 (1)
62-63:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd an explicit production-security warning for default credentials/secrets.
README currently exposes default admin credentials and a concrete default
JWT_SECRETbut does not clearly mark them as local-dev only. Please add a hard warning to rotate/change these before any non-local deployment to avoid weak-auth deployments.Suggested doc patch
Default admin: `admin@etc.com` / `admin123` Sample users: `lelisa@etc.com`, `ruth@etc.com`, `sami@etc.com` — all with password `admin123` + +> ⚠️ **Security notice (production):** +> These credentials and example secrets are for local development only. +> Before deploying, set strong unique values for `JWT_SECRET`, `ADMIN_EMAIL` credentials, DB credentials, and mail settings.| `JWT_SECRET` | HMAC signing key | `1234567ABCDAASTUIPPROJECT` | @@ | `APP_URL` | Base URL used in download links | `http://localhost:8000` | + +**Production requirement:** never use README example secrets/credentials in deployed environments.Also applies to: 171-181
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/README.md` around lines 62 - 63, The README exposes default credentials and a concrete JWT_SECRET without marking them as local-dev only; update the text around the default admin credentials (the block showing "Default admin: `admin@etc.com` / `admin123`" and the sample users) and the section that lists the default JWT_SECRET (lines ~171-181) to add a prominent production-security warning: clearly label these values as "LOCAL DEVELOPMENT ONLY", remove any suggestion to use the concrete JWT_SECRET in production, instruct users to set JWT_SECRET via environment variable and rotate/change all default passwords and secrets before any non-local deployment, and add a short example of how to generate/store a secure secret (e.g., use a strong random value or a secrets manager).backend/src/Controllers/Concerns/AuthenticatesRequests.php-74-83 (1)
74-83:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove email from the shared public serializer.
publicUser()is a public-facing payload, but Line 77 exposes the raw email address. That leaks PII anywhere this helper is reused for listings or profile responses; keep email in self/admin-specific responses instead.Suggested fix
return [ 'id' => (int) $user['id'], 'username' => (string) $user['username'], - 'email' => (string) $user['email'], 'full_name' => $fullName !== '' ? $fullName : (string) $user['username'], 'avatar' => (string) ($user['avatar'] ?? 'default-avatar.png'), 'bio' => (string) ($user['bio'] ?? ''), 'location' => (string) ($user['location'] ?? ''), 'skill_level' => (string) ($user['skill_level'] ?? 'beginner'),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/Controllers/Concerns/AuthenticatesRequests.php` around lines 74 - 83, In the publicUser() payload returned from AuthenticatesRequests (the array that currently includes keys like 'id', 'username', 'full_name', 'avatar', etc.), remove the 'email' entry so the public/shared serializer no longer exposes raw email PII; update the returned array in the publicUser helper (and any similarly named public serializer in this file) to omit 'email' and ensure email is only included in self/admin-specific responses or a separate privateUser/privateSerializer used for authenticated/admin endpoints.backend/src/Controllers/Concerns/AuthenticatesRequests.php-64-68 (1)
64-68:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail closed when
ADMIN_EMAILis missing.Line 66 falls back to
admin@etc.com, so a deployment with noADMIN_EMAILconfigured silently grants admin access to any account using that address. Missing admin config should deny admin access or fail during bootstrap.Suggested fix
protected function isAdmin(array $user): bool { - $adminEmail = strtolower((string) (getenv('ADMIN_EMAIL') ?: 'admin@etc.com')); - return strtolower((string) $user['email']) === $adminEmail; + $adminEmail = trim((string) getenv('ADMIN_EMAIL')); + if ($adminEmail === '') { + return false; + } + + return strtolower((string) ($user['email'] ?? '')) === strtolower($adminEmail); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/Controllers/Concerns/AuthenticatesRequests.php` around lines 64 - 68, The isAdmin function currently falls back to 'admin@etc.com' when getenv('ADMIN_EMAIL') is missing; change it to fail closed by removing that default and treating a missing/empty ADMIN_EMAIL as "no admin configured" (i.e., return false from isAdmin). Concretely, update the isAdmin(array $user): bool implementation to fetch $adminEmail = getenv('ADMIN_EMAIL'), trim and lowercase it, and if it's null/empty return false; otherwise compare strtolower((string)$user['email']) to the configured $adminEmail. Optionally, add a bootstrap-time check elsewhere to throw or log a fatal error if your deployment requires ADMIN_EMAIL to be set.backend/src/Helpers/Str.php-32-49 (1)
32-49:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject unsupported slug targets instead of interpolating them.
Line 39 parameterizes the slug value but not
$column, and Lines 35-36 silently returnfalsefor unsupported tables, which skips the uniqueness check entirely. Allowlist both table and column and throw on invalid input so this helper can't create duplicate slugs or become an SQL-injection footgun later.Suggested fix
private static function slugExists(\PDO $db, string $table, string $column, string $slug, ?int $ignoreId): bool { - $allowedTables = ['articles', 'forum_topics', 'events']; - if (!in_array($table, $allowedTables, true)) { - return false; - } + $allowedTargets = [ + 'articles' => ['slug'], + 'forum_topics' => ['slug'], + 'events' => ['slug'], + ]; + if (!isset($allowedTargets[$table]) || !in_array($column, $allowedTargets[$table], true)) { + throw new \InvalidArgumentException('Unsupported slug target.'); + } $sql = "SELECT id FROM {$table} WHERE {$column} = :slug"; $params = ['slug' => $slug];🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/Helpers/Str.php` around lines 32 - 49, The slugExists helper currently only allowlists tables and silently returns false for invalid tables and interpolates the column name; update slugExists to validate both the $table and $column against explicit allowlists (e.g., a map like allowedColumnsPerTable) and throw an InvalidArgumentException on any invalid table/column instead of returning false; once validated, use the validated table and column names directly in the SQL string (still parameterize the slug and ignore_id values) when preparing the statement in slugExists so you eliminate silent skips and prevent SQL injection via column/table names.backend/src/Models/Article.php-123-128 (1)
123-128:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid returning author email in article detail payloads.
formatDetail()exposesauthor_emailfor every article fetch. If this model backs public article pages, that leaks PII with no authorization check here. Gate it to self/admin flows or remove it from the public shape.Also applies to: 464-467
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/Models/Article.php` around lines 123 - 128, The Article model is exposing author_email in public article payloads via the SELECT and formatDetail(); remove author_email from the public shape — either strip it out of the SQL projection and from formatDetail(), or add an explicit flag/parameter to formatDetail(article, includePrivate=false) (and any callers) that only includes author_email when includePrivate is true and the caller has verified self/admin rights; update any code paths calling formatDetail() (and the SELECT that currently selects author_email) to pass true only for authenticated self/admin flows and false for public fetches.backend/src/Models/Resource.php-98-114 (1)
98-114:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate the rating range before saving it.
rate()accepts any integer. Values outside the supported scale will skewaverage_ratingand can break clients that assume a bounded rating system.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/Models/Resource.php` around lines 98 - 114, In rate(), validate the incoming $rating before preparing/executing the INSERT: enforce the allowed rating bounds (e.g. 1..5) using either a class constant or config (e.g. self::MIN_RATING / self::MAX_RATING), and if $rating is outside the range, throw an InvalidArgumentException (or return a clear error result) instead of writing to the DB; keep the calls to refreshAverageRating($resourceId) and userRating($resourceId, $userId) unchanged and only run them when the rating is valid.backend/src/Models/Forum.php-56-75 (1)
56-75:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep
findTopic()side-effect free.
findTopic()increments views on every lookup. That meanscreateTopic()immediately records a view via Line 95, any internal fetch inflates analytics, and the returned payload is still one view behind because it formats the pre-increment row.Also applies to: 95-95
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/Models/Forum.php` around lines 56 - 75, findTopic currently has a side-effect: it calls incrementTopicViews($id) which causes unintended view increments and makes the returned row out-of-sync; remove the incrementTopicViews($id) call from findTopic so it becomes side-effect-free. Update call sites that should record a view (e.g., createTopic and any controller/view handlers that represent a real user view) to call incrementTopicViews($id) explicitly at the correct time and, if the incremented count must be reflected in the returned payload, call incrementTopicViews before reloading/formatting via formatTopic or fetch the row after incrementing.backend/src/Models/Comment.php-66-79 (1)
66-79:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate reply parents against the target article.
create()stores anyparentIdit receives. That allows a comment on article A to reference a parent from article B, which breaksforArticle()tree construction and mixes threads across articles.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/Models/Comment.php` around lines 66 - 79, In create(), validate that a provided $parentId belongs to the same $articleId before inserting: fetch the parent comment (use the existing findById or a short SELECT on comments by id) and if it exists ensure its article_id === $articleId; if it does not match (or the parent doesn't exist) either reject the request (throw/return an error) or treat parent as null, then proceed to execute the INSERT and return findById(lastInsertId()). Update create() to perform this check so cross-article parent references cannot be stored and break forArticle() tree construction.backend/src/Models/Article.php-106-107 (1)
106-107:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
paginate()fans out into per-article queries.Each list item triggers one tags query, plus two more viewer-flag queries when
viewerIdis set. At the current 50-row cap, one page can jump from 2 queries to 152 queries. Preload tags and viewer flags in bulk before formatting.Also applies to: 381-421, 423-446
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/Models/Article.php` around lines 106 - 107, The paginate() implementation is causing N+1 queries because formatListItem($row, $viewerId) issues per-article tag and viewer-flag queries; instead, before the foreach over $rows, bulk-load tags and viewer flags for all article IDs in $rows (e.g., collect ids from $rows), hydrate a tags map and a viewerFlags map, and then pass those preloaded maps or inject them into formatListItem so formatListItem($row, $viewerId) reads from the in-memory maps rather than issuing new DB queries; apply the same preloading approach to the other similar blocks referenced (around lines 381-421 and 423-446) that call formatListItem or similar per-item loaders.backend/src/Models/Event.php-44-51 (1)
44-51:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse overlap predicates for date-range queries.
Both filters only return events fully contained inside the window. Any multi-day event that starts before
frombut is still active during the range disappears frompaginate()andcalendar().Suggested fix
- if (!empty($filters['from'])) { - $where[] = 'e.start_date >= :from_date'; - $params['from_date'] = (string) $filters['from']; - } - - if (!empty($filters['to'])) { - $where[] = 'e.end_date <= :to_date'; - $params['to_date'] = (string) $filters['to']; - } + if (!empty($filters['from'])) { + $where[] = 'e.end_date >= :from_date'; + $params['from_date'] = (string) $filters['from']; + } + + if (!empty($filters['to'])) { + $where[] = 'e.start_date <= :to_date'; + $params['to_date'] = (string) $filters['to']; + }- WHERE e.start_date >= :from_date AND e.start_date <= :to_date + WHERE e.end_date >= :from_date AND e.start_date <= :to_dateAlso applies to: 97-100
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/Models/Event.php` around lines 44 - 51, The current date filters in paginate() and calendar() use e.start_date >= :from_date and e.end_date <= :to_date which only returns events fully inside the window; change to use overlap logic so events that intersect the range are included: when filters['from'] is set require e.end_date >= :from_date (not e.start_date) and when filters['to'] is set require e.start_date <= :to_date (not e.end_date), or equivalently add a combined overlap predicate like NOT (e.end_date < :from_date OR e.start_date > :to_date); update the parameter bindings for :from_date and :to_date accordingly in the methods paginate() and calendar() (the blocks referencing e.start_date/e.end_date for filters 'from' and 'to', including the second occurrence around lines 97-100).backend/src/Models/Article.php-173-176 (1)
173-176:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDeduplicate caller-provided slugs before insert.
create()only guarantees uniqueness whenslugis omitted. If a request sends an existing slug, this path inserts it unchanged and will fail on the first collision.Suggested fix
- $slug = !empty($data['slug']) - ? Str::slug((string) $data['slug']) - : Str::uniqueSlug($this->db, 'articles', 'slug', $title); + $slugBase = trim((string) ($data['slug'] ?? '')); + $slug = Str::uniqueSlug( + $this->db, + 'articles', + 'slug', + $slugBase !== '' ? $slugBase : $title + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/Models/Article.php` around lines 173 - 176, The create() path accepts a caller-provided slug but currently only slugifies it with Str::slug and doesn’t ensure uniqueness, causing collisions; change the logic around the $slug assignment in Article::create() so that when a slug is provided you pass the slugified value into Str::uniqueSlug($this->db, 'articles', 'slug', $providedSlug) (instead of using Str::slug alone), ensuring deduplication for both caller-provided and autogenerated slugs; update the $slug variable assignment to always call Str::uniqueSlug for the final stored slug.backend/src/Models/Event.php-229-240 (1)
229-240:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject unknown RSVP states before writing them.
This method persists any string, but the rest of the model only understands known values like
going,interested, andcancelled. A stray value such as"yes"will silently create inconsistent attendee data.Suggested fix
public function rsvp(int $eventId, int $userId, string $status = 'going'): array { + $allowed = ['going', 'interested', 'cancelled']; + if (!in_array($status, $allowed, true)) { + throw new \InvalidArgumentException('Invalid RSVP status.'); + } + $statement = $this->db->prepare( 'INSERT INTO event_attendees (event_id, user_id, status) VALUES (:event_id, :user_id, :status)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/Models/Event.php` around lines 229 - 240, The rsvp method currently accepts any string for $status and writes it to the event_attendees table; add validation in rsvp to reject anything not in the allowed set (e.g. ['going','interested','cancelled']) before preparing/executing the INSERT/ON DUPLICATE query, throwing an InvalidArgumentException (or returning a clear error) when $status is invalid; implement the whitelist check near the start of rsvp (before $this->db->prepare) so only known states are persisted.backend/src/Controllers/ArticleController.php-143-144 (1)
143-144:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate update payloads before persisting them.
store()rejects empty titles/contents and invalid statuses, butupdate()writes the raw body straight through. That makes the update path able to save states the create path explicitly forbids.💡 Suggested fix
- $payload = Request::body(); - $article = $this->model()->update($id, $payload); + $payload = Request::body(); + $errors = $this->validateArticlePayload($payload, false); + if ($errors !== []) { + return Response::error('Article validation failed.', 422, $errors); + } + + $article = $this->model()->update($id, $payload);- if (trim((string) ($payload['title'] ?? '')) === '') { + if (array_key_exists('title', $payload) && trim((string) $payload['title']) === '') { $errors['title'][] = 'Title is required.'; } - if ($requireContent && trim((string) ($payload['content'] ?? '')) === '') { + if (($requireContent || array_key_exists('content', $payload)) + && trim((string) ($payload['content'] ?? '')) === '') { $errors['content'][] = 'Content is required.'; } - if (!empty($payload['status']) && !in_array($payload['status'], ['draft', 'published', 'archived'], true)) { + if (array_key_exists('status', $payload) + && !in_array($payload['status'], ['draft', 'published', 'archived'], true)) { $errors['status'][] = 'Invalid status.'; }Also applies to: 190-206
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/Controllers/ArticleController.php` around lines 143 - 144, The update path in ArticleController currently writes Request::body() directly via $this->model()->update($id, $payload); replicate the same validation used in store() for titles, contents and allowed statuses before calling model()->update: extract and validate $payload (non-empty title/content, status in allowed list), return/throw the same validation error responses as store() on failure, and only pass the sanitized/validated payload to $this->model()->update($id, $payload); apply the same validation fix to the other update-related block referenced around lines 190-206.backend/src/Controllers/ResourceController.php-69-75 (1)
69-75:⚠️ Potential issue | 🟠 Major | ⚡ Quick winOnly count downloads after confirming the asset is actually available.
resolveDownload()can returnmissing/unavailablewith a null URL, but the controller increments the counter first and still replies with “Download ready.” That breaks download analytics and gives clients a false-success response.💡 Suggested fix
- $updated = $this->model()->incrementDownloadCount($id); - $download = $this->resolveDownload($updated ?? $resource); + $download = $this->resolveDownload($resource); + if (empty($download['url'])) { + return Response::error('Download is not available.', 404); + } + + $updated = $this->model()->incrementDownloadCount($id); + $download = $this->resolveDownload($updated ?? $resource);Also applies to: 268-303
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/Controllers/ResourceController.php` around lines 69 - 75, The controller currently calls model()->incrementDownloadCount($id) before checking availability, causing false positives; change the flow in the method that uses incrementDownloadCount, resolveDownload, $updated and $resource so that you first call resolveDownload($resource) (or resolveDownload($updated ?? $resource)) and verify the returned download is available (e.g., not 'missing' and has a non-null URL) and only then call incrementDownloadCount($id) and refresh $updated; finally return Response::success with the updated resource and download. Apply the same adjustment to the other similar block that uses incrementDownloadCount and resolveDownload.backend/src/Controllers/EventController.php-103-106 (1)
103-106:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate event updates before sending them to the model.
store()guardstitle,event_type, and dates, butupdate()writes the raw request body. That makes it possible to save invalid event data through the update path even though create blocks it.Also applies to: 210-227
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/Controllers/EventController.php` around lines 103 - 106, The update path is writing raw Request::body() via $this->model()->update($id, Request::body()) without the same validation performed in store(), allowing invalid event data (title, event_type, start/end dates) to be saved; modify EventController::update (and the other update path around the 210-227 region) to validate and sanitize the incoming payload the same way store() does (check required fields, allowed event_type values, and that start/end dates are valid and consistent) before calling $this->model()->update; reuse the existing store() validation logic or extract it into a shared validateEventPayload() helper and call that from update(), returning a 400 response with validation errors instead of passing raw Request::body() to the model.backend/src/Controllers/AuthController.php-205-218 (1)
205-218:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCheck the PHP upload error before reading the temp file.
This path calls
mime_content_type()ontmp_namewithout validating$_FILES['avatar']['error']. Failed uploads (UPLOAD_ERR_INI_SIZE, partial uploads, missing tmp file) will fall through here and can produce warnings or a misleading 500 instead of a clean 422.💡 Suggested fix
- if (empty($_FILES['avatar'])) { + if (empty($_FILES['avatar']) || !is_array($_FILES['avatar'])) { return Response::error('No avatar file uploaded.', 422); } $file = $_FILES['avatar']; + if (($file['error'] ?? UPLOAD_ERR_NO_FILE) !== UPLOAD_ERR_OK) { + return Response::error('Avatar upload failed.', 422); + } + $allowedMimes = ['image/jpeg', 'image/png', 'image/gif', 'image/webp']; - $mimeType = mime_content_type($file['tmp_name']); + $mimeType = mime_content_type($file['tmp_name']) ?: '';🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/Controllers/AuthController.php` around lines 205 - 218, AuthController currently calls mime_content_type() on $_FILES['avatar']['tmp_name'] without checking the upload status; first validate $_FILES['avatar']['error'] is UPLOAD_ERR_OK (and that tmp_name exists and is readable) before calling mime_content_type(), and return Response::error(...) with a 422 for other upload error codes (e.g., UPLOAD_ERR_INI_SIZE, UPLOAD_ERR_PARTIAL, UPLOAD_ERR_NO_FILE) so failed uploads produce a clean 422 instead of warnings/500.backend/src/Controllers/CommunityController.php-211-215 (1)
211-215:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve the existing solution flag when it isn’t being edited.
!empty($payload['is_solution'])forcesfalsewhenever the client omits that field, so a normal text edit will silently unmark an already accepted solution.💡 Suggested fix
$payload = Request::body(); $content = trim((string) ($payload['content'] ?? $reply['content'])); - $isSolution = !empty($payload['is_solution']); + $isSolution = array_key_exists('is_solution', $payload) + ? (bool) $payload['is_solution'] + : (bool) ($reply['is_solution'] ?? false); $updated = $this->resolveForumRepository()->updateReply($id, $content, $isSolution);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/Controllers/CommunityController.php` around lines 211 - 215, The update currently sets $isSolution to !empty($payload['is_solution']) which clears the flag when the client omits that field; change the logic so if 'is_solution' is present in Request::body() you use its boolean value, otherwise preserve the existing value from $reply['is_solution'] before calling resolveForumRepository()->updateReply($id, $content, $isSolution); use Request::body()/$payload, $reply and updateReply(...) to locate and implement this conditional assignment.backend/src/Controllers/ResourceController.php-177-189 (1)
177-189:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClamp progress to the valid 0–100 range before saving.
incrementcan drive progress below zero, and directprogress_percentcurrently accepts values outside 0–100. That lets invalid progress leak into storage.💡 Suggested fix
if ($increment !== null) { $current = $this->model()->progressForUser((int) $user['id'], $resourceId); - $progressPercent = min(100, (int) $current['progress_percent'] + $increment); + $progressPercent = max(0, min(100, (int) $current['progress_percent'] + $increment)); } if ($progressPercent === null) { return Response::error('progress_percent or increment is required.', 422); } + $progressPercent = max(0, min(100, $progressPercent)); $saved = $this->model()->saveProgress((int) $user['id'], $resourceId, $progressPercent);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/Controllers/ResourceController.php` around lines 177 - 189, The progress value isn't clamped to 0–100 before persisting, so update the logic around $increment and $progressPercent (in ResourceController methods using $this->model()->progressForUser and $this->model()->saveProgress) to coerce inputs to ints and clamp the resulting $progressPercent using max(0, min(100, $value)) both when computing from $increment (replace the current min(100, ...)+increment logic with a clamp that also prevents <0) and when a direct progress_percent is provided, then pass the clamped $progressPercent to saveProgress.backend/src/Controllers/AuthController.php-229-243 (1)
229-243:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t return success until the avatar path is actually persisted.
realpath()can returnfalsehere whenstorage/uploadsis missing, andupdateProfile()can also fail after the file has already been moved. In both cases this endpoint can leave an orphaned file or build an invalid destination while still returning a successful avatar path to the client.💡 Suggested fix
- $uploadDir = realpath(__DIR__ . '/../../storage/uploads') . DIRECTORY_SEPARATOR; + $uploadDir = __DIR__ . '/../../storage/uploads'; + if (!is_dir($uploadDir) && !mkdir($uploadDir, 0775, true) && !is_dir($uploadDir)) { + return Response::error('Avatar storage is not available.', 500); + } + + $uploadDir = realpath($uploadDir); + if ($uploadDir === false) { + return Response::error('Avatar storage is not available.', 500); + } + + $uploadDir .= DIRECTORY_SEPARATOR; $filename = 'avatar_' . (int) $user['id'] . '_' . time() . '.' . $ext; $destination = $uploadDir . $filename; if (!move_uploaded_file($file['tmp_name'], $destination)) { return Response::error('Failed to save avatar.', 500); } $avatarPath = 'storage/uploads/' . $filename; $updated = $this->users()->updateProfile((int) $user['id'], ['avatar' => $avatarPath]); + if ($updated === null) { + `@unlink`($destination); + return Response::error('Avatar could not be saved.', 500); + } return Response::success([ - 'user' => $this->publicUser($updated ?? $user), + 'user' => $this->publicUser($updated), 'avatar' => $avatarPath, ], 'Avatar uploaded successfully.');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/Controllers/AuthController.php` around lines 229 - 243, realpath() can return false and updateProfile() may fail after move_uploaded_file, so ensure you only return success once the avatar path is persisted: verify realpath(__DIR__ . '/../../storage/uploads') !== false (or create the directory if missing and writable) before building $destination, ensure move_uploaded_file($file['tmp_name'], $destination) succeeds, then call $this->users()->updateProfile((int)$user['id'], ['avatar' => $avatarPath]) and check its return value; if updateProfile fails, delete the moved file (unlink($destination)) and return an error via Response::error, otherwise return Response::success with $this->publicUser($updated) and the avatar path; reference functions/methods: realpath, move_uploaded_file, $this->users()->updateProfile, unlink, publicUser, Response::success/Response::error.
🟡 Minor comments (2)
backend/tests/Test.php-44-47 (1)
44-47:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard route-map type before indexing keys.
If
routes/api.phpever returns a non-array (or throws/returns scalar), these checks can crash instead of reporting a clean failed assertion.Suggested fix
$routes = require $base . '/routes/api.php'; -assertTrue(isset($routes['GET /api/articles/search']), 'articles search route exists'); -assertTrue(isset($routes['GET /api/events/calendar']), 'events calendar route exists'); -assertTrue(isset($routes['GET /api/users/search']), 'users search route exists'); +assertTrue(is_array($routes), 'routes/api.php returns a route map array'); +if (is_array($routes)) { + assertTrue(isset($routes['GET /api/articles/search']), 'articles search route exists'); + assertTrue(isset($routes['GET /api/events/calendar']), 'events calendar route exists'); + assertTrue(isset($routes['GET /api/users/search']), 'users search route exists'); +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/tests/Test.php` around lines 44 - 47, The test assumes $routes (from require $base . '/routes/api.php') is an array before using isset on keys; guard the type first by asserting $routes is an array (e.g., use assertTrue(is_array($routes), 'routes must be an array') or assertIsArray($routes)) before any assertTrue(isset($routes['GET /api/articles/search'])) checks, then perform the existing isset assertions (for keys 'GET /api/articles/search', 'GET /api/events/calendar', 'GET /api/users/search') so a non-array return yields a clear test failure instead of a crash.backend/tests/Test.php-60-62 (1)
60-62:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSkip file reads when controller file is missing.
At Line 61,
file_get_contentsruns even after a failed existence assertion at Line 60, which can emit warnings and muddy smoke-test output.Suggested fix
foreach ($controllerFiles as $file) { $path = $base . '/src/Controllers/' . $file; assertTrue(is_file($path), "{$file} exists"); + if (!is_file($path)) { + continue; + } $source = (string) file_get_contents($path); assertTrue(!str_contains($source, 'Starter endpoint is ready'), "{$file} is implemented"); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/tests/Test.php` around lines 60 - 62, The test currently calls file_get_contents($path) unconditionally after assertTrue(is_file($path)), which can still trigger warnings when the file is missing; change the test to guard the read so it only executes if the file exists—e.g., replace the unconditional read with a conditional/guard that checks is_file($path) (or immediately return/fail on false) before calling file_get_contents, keeping the existing assertions (assertTrue/is_file and the subsequent assertTrue(!str_contains($source,...))) and referencing $path, $source, assertTrue, is_file, and file_get_contents in your changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/migrations/database.sql`:
- Around line 341-343: The migration seeds an active admin user with a hardcoded
bcrypt password hash (INSERT INTO users ... ('admin', ..., '$2y$10$92IXUNpk...',
'Administrator', TRUE)), which creates predictable credentials; remove the fixed
password hash from the migration and change seeding so credentials are created
at runtime or the account is disabled by default: either (a) stop inserting a
usable password_hash in the migration (set password_hash to NULL) and set
is_active = FALSE for seeded users, or (b) move password generation to your
setup script (setup.php) to insert a unique bcrypt hash for username 'admin'
(and other seeded users) at install time; update any code that expects an active
admin in the migration to rely on the setup step instead.
In `@backend/router.php`:
- Around line 15-21: The current passthrough checks (str_starts_with($uri,
'/storage/') and the backend-root is_file(__DIR__ . $uri) fallback) allow path
traversal and exposure of arbitrary files; change the logic to resolve and
validate canonical paths: for storage paths, compute the realpath of the
requested file (use $storagePath or realpath(__DIR__ . '/storage/' .
substr($uri, strlen('/storage/')))) and only return false if realpath succeeds
and the resolved path starts with the known storage root directory (preventing
../../ escapes); remove the backend-root fallback that checks is_file(__DIR__ .
$uri) so no arbitrary backend-root files (like .env) are served. Ensure you
handle leading slashes consistently and treat missing realpath as non-existent.
---
Outside diff comments:
In `@backend/src/Models/User.php`:
- Around line 88-116: The update path only checks for email conflicts but not
username collisions; before executing the UPDATE in the method that builds
$statement, normalize the incoming username (e.g., $username =
strtolower(trim((string) ($data['username'] ?? $current['username'])))), call a
lookup like $this->findByUsername($username) (or implement findByUsername if
missing), and if a record exists with a different id (similar to the existing
email check) return null to prevent duplicate usernames; then use that
normalized $username in the $statement->execute payload so the same value is
written to the users table.
---
Major comments:
In `@backend/README.md`:
- Around line 62-63: The README exposes default credentials and a concrete
JWT_SECRET without marking them as local-dev only; update the text around the
default admin credentials (the block showing "Default admin: `admin@etc.com` /
`admin123`" and the sample users) and the section that lists the default
JWT_SECRET (lines ~171-181) to add a prominent production-security warning:
clearly label these values as "LOCAL DEVELOPMENT ONLY", remove any suggestion to
use the concrete JWT_SECRET in production, instruct users to set JWT_SECRET via
environment variable and rotate/change all default passwords and secrets before
any non-local deployment, and add a short example of how to generate/store a
secure secret (e.g., use a strong random value or a secrets manager).
In `@backend/src/Controllers/ArticleController.php`:
- Around line 143-144: The update path in ArticleController currently writes
Request::body() directly via $this->model()->update($id, $payload); replicate
the same validation used in store() for titles, contents and allowed statuses
before calling model()->update: extract and validate $payload (non-empty
title/content, status in allowed list), return/throw the same validation error
responses as store() on failure, and only pass the sanitized/validated payload
to $this->model()->update($id, $payload); apply the same validation fix to the
other update-related block referenced around lines 190-206.
In `@backend/src/Controllers/AuthController.php`:
- Around line 205-218: AuthController currently calls mime_content_type() on
$_FILES['avatar']['tmp_name'] without checking the upload status; first validate
$_FILES['avatar']['error'] is UPLOAD_ERR_OK (and that tmp_name exists and is
readable) before calling mime_content_type(), and return Response::error(...)
with a 422 for other upload error codes (e.g., UPLOAD_ERR_INI_SIZE,
UPLOAD_ERR_PARTIAL, UPLOAD_ERR_NO_FILE) so failed uploads produce a clean 422
instead of warnings/500.
- Around line 229-243: realpath() can return false and updateProfile() may fail
after move_uploaded_file, so ensure you only return success once the avatar path
is persisted: verify realpath(__DIR__ . '/../../storage/uploads') !== false (or
create the directory if missing and writable) before building $destination,
ensure move_uploaded_file($file['tmp_name'], $destination) succeeds, then call
$this->users()->updateProfile((int)$user['id'], ['avatar' => $avatarPath]) and
check its return value; if updateProfile fails, delete the moved file
(unlink($destination)) and return an error via Response::error, otherwise return
Response::success with $this->publicUser($updated) and the avatar path;
reference functions/methods: realpath, move_uploaded_file,
$this->users()->updateProfile, unlink, publicUser,
Response::success/Response::error.
In `@backend/src/Controllers/CommunityController.php`:
- Around line 211-215: The update currently sets $isSolution to
!empty($payload['is_solution']) which clears the flag when the client omits that
field; change the logic so if 'is_solution' is present in Request::body() you
use its boolean value, otherwise preserve the existing value from
$reply['is_solution'] before calling resolveForumRepository()->updateReply($id,
$content, $isSolution); use Request::body()/$payload, $reply and
updateReply(...) to locate and implement this conditional assignment.
In `@backend/src/Controllers/Concerns/AuthenticatesRequests.php`:
- Around line 74-83: In the publicUser() payload returned from
AuthenticatesRequests (the array that currently includes keys like 'id',
'username', 'full_name', 'avatar', etc.), remove the 'email' entry so the
public/shared serializer no longer exposes raw email PII; update the returned
array in the publicUser helper (and any similarly named public serializer in
this file) to omit 'email' and ensure email is only included in
self/admin-specific responses or a separate privateUser/privateSerializer used
for authenticated/admin endpoints.
- Around line 64-68: The isAdmin function currently falls back to
'admin@etc.com' when getenv('ADMIN_EMAIL') is missing; change it to fail closed
by removing that default and treating a missing/empty ADMIN_EMAIL as "no admin
configured" (i.e., return false from isAdmin). Concretely, update the
isAdmin(array $user): bool implementation to fetch $adminEmail =
getenv('ADMIN_EMAIL'), trim and lowercase it, and if it's null/empty return
false; otherwise compare strtolower((string)$user['email']) to the configured
$adminEmail. Optionally, add a bootstrap-time check elsewhere to throw or log a
fatal error if your deployment requires ADMIN_EMAIL to be set.
In `@backend/src/Controllers/EventController.php`:
- Around line 103-106: The update path is writing raw Request::body() via
$this->model()->update($id, Request::body()) without the same validation
performed in store(), allowing invalid event data (title, event_type, start/end
dates) to be saved; modify EventController::update (and the other update path
around the 210-227 region) to validate and sanitize the incoming payload the
same way store() does (check required fields, allowed event_type values, and
that start/end dates are valid and consistent) before calling
$this->model()->update; reuse the existing store() validation logic or extract
it into a shared validateEventPayload() helper and call that from update(),
returning a 400 response with validation errors instead of passing raw
Request::body() to the model.
In `@backend/src/Controllers/ResourceController.php`:
- Around line 69-75: The controller currently calls
model()->incrementDownloadCount($id) before checking availability, causing false
positives; change the flow in the method that uses incrementDownloadCount,
resolveDownload, $updated and $resource so that you first call
resolveDownload($resource) (or resolveDownload($updated ?? $resource)) and
verify the returned download is available (e.g., not 'missing' and has a
non-null URL) and only then call incrementDownloadCount($id) and refresh
$updated; finally return Response::success with the updated resource and
download. Apply the same adjustment to the other similar block that uses
incrementDownloadCount and resolveDownload.
- Around line 177-189: The progress value isn't clamped to 0–100 before
persisting, so update the logic around $increment and $progressPercent (in
ResourceController methods using $this->model()->progressForUser and
$this->model()->saveProgress) to coerce inputs to ints and clamp the resulting
$progressPercent using max(0, min(100, $value)) both when computing from
$increment (replace the current min(100, ...)+increment logic with a clamp that
also prevents <0) and when a direct progress_percent is provided, then pass the
clamped $progressPercent to saveProgress.
In `@backend/src/Helpers/Str.php`:
- Around line 32-49: The slugExists helper currently only allowlists tables and
silently returns false for invalid tables and interpolates the column name;
update slugExists to validate both the $table and $column against explicit
allowlists (e.g., a map like allowedColumnsPerTable) and throw an
InvalidArgumentException on any invalid table/column instead of returning false;
once validated, use the validated table and column names directly in the SQL
string (still parameterize the slug and ignore_id values) when preparing the
statement in slugExists so you eliminate silent skips and prevent SQL injection
via column/table names.
In `@backend/src/Models/Article.php`:
- Around line 123-128: The Article model is exposing author_email in public
article payloads via the SELECT and formatDetail(); remove author_email from the
public shape — either strip it out of the SQL projection and from
formatDetail(), or add an explicit flag/parameter to formatDetail(article,
includePrivate=false) (and any callers) that only includes author_email when
includePrivate is true and the caller has verified self/admin rights; update any
code paths calling formatDetail() (and the SELECT that currently selects
author_email) to pass true only for authenticated self/admin flows and false for
public fetches.
- Around line 106-107: The paginate() implementation is causing N+1 queries
because formatListItem($row, $viewerId) issues per-article tag and viewer-flag
queries; instead, before the foreach over $rows, bulk-load tags and viewer flags
for all article IDs in $rows (e.g., collect ids from $rows), hydrate a tags map
and a viewerFlags map, and then pass those preloaded maps or inject them into
formatListItem so formatListItem($row, $viewerId) reads from the in-memory maps
rather than issuing new DB queries; apply the same preloading approach to the
other similar blocks referenced (around lines 381-421 and 423-446) that call
formatListItem or similar per-item loaders.
- Around line 173-176: The create() path accepts a caller-provided slug but
currently only slugifies it with Str::slug and doesn’t ensure uniqueness,
causing collisions; change the logic around the $slug assignment in
Article::create() so that when a slug is provided you pass the slugified value
into Str::uniqueSlug($this->db, 'articles', 'slug', $providedSlug) (instead of
using Str::slug alone), ensuring deduplication for both caller-provided and
autogenerated slugs; update the $slug variable assignment to always call
Str::uniqueSlug for the final stored slug.
In `@backend/src/Models/Comment.php`:
- Around line 66-79: In create(), validate that a provided $parentId belongs to
the same $articleId before inserting: fetch the parent comment (use the existing
findById or a short SELECT on comments by id) and if it exists ensure its
article_id === $articleId; if it does not match (or the parent doesn't exist)
either reject the request (throw/return an error) or treat parent as null, then
proceed to execute the INSERT and return findById(lastInsertId()). Update
create() to perform this check so cross-article parent references cannot be
stored and break forArticle() tree construction.
In `@backend/src/Models/Event.php`:
- Around line 44-51: The current date filters in paginate() and calendar() use
e.start_date >= :from_date and e.end_date <= :to_date which only returns events
fully inside the window; change to use overlap logic so events that intersect
the range are included: when filters['from'] is set require e.end_date >=
:from_date (not e.start_date) and when filters['to'] is set require e.start_date
<= :to_date (not e.end_date), or equivalently add a combined overlap predicate
like NOT (e.end_date < :from_date OR e.start_date > :to_date); update the
parameter bindings for :from_date and :to_date accordingly in the methods
paginate() and calendar() (the blocks referencing e.start_date/e.end_date for
filters 'from' and 'to', including the second occurrence around lines 97-100).
- Around line 229-240: The rsvp method currently accepts any string for $status
and writes it to the event_attendees table; add validation in rsvp to reject
anything not in the allowed set (e.g. ['going','interested','cancelled']) before
preparing/executing the INSERT/ON DUPLICATE query, throwing an
InvalidArgumentException (or returning a clear error) when $status is invalid;
implement the whitelist check near the start of rsvp (before $this->db->prepare)
so only known states are persisted.
In `@backend/src/Models/Forum.php`:
- Around line 56-75: findTopic currently has a side-effect: it calls
incrementTopicViews($id) which causes unintended view increments and makes the
returned row out-of-sync; remove the incrementTopicViews($id) call from
findTopic so it becomes side-effect-free. Update call sites that should record a
view (e.g., createTopic and any controller/view handlers that represent a real
user view) to call incrementTopicViews($id) explicitly at the correct time and,
if the incremented count must be reflected in the returned payload, call
incrementTopicViews before reloading/formatting via formatTopic or fetch the row
after incrementing.
In `@backend/src/Models/Resource.php`:
- Around line 98-114: In rate(), validate the incoming $rating before
preparing/executing the INSERT: enforce the allowed rating bounds (e.g. 1..5)
using either a class constant or config (e.g. self::MIN_RATING /
self::MAX_RATING), and if $rating is outside the range, throw an
InvalidArgumentException (or return a clear error result) instead of writing to
the DB; keep the calls to refreshAverageRating($resourceId) and
userRating($resourceId, $userId) unchanged and only run them when the rating is
valid.
---
Minor comments:
In `@backend/tests/Test.php`:
- Around line 44-47: The test assumes $routes (from require $base .
'/routes/api.php') is an array before using isset on keys; guard the type first
by asserting $routes is an array (e.g., use assertTrue(is_array($routes),
'routes must be an array') or assertIsArray($routes)) before any
assertTrue(isset($routes['GET /api/articles/search'])) checks, then perform the
existing isset assertions (for keys 'GET /api/articles/search', 'GET
/api/events/calendar', 'GET /api/users/search') so a non-array return yields a
clear test failure instead of a crash.
- Around line 60-62: The test currently calls file_get_contents($path)
unconditionally after assertTrue(is_file($path)), which can still trigger
warnings when the file is missing; change the test to guard the read so it only
executes if the file exists—e.g., replace the unconditional read with a
conditional/guard that checks is_file($path) (or immediately return/fail on
false) before calling file_get_contents, keeping the existing assertions
(assertTrue/is_file and the subsequent assertTrue(!str_contains($source,...)))
and referencing $path, $source, assertTrue, is_file, and file_get_contents in
your changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 94dabb38-9908-4cbb-85d4-66262600ec0d
📒 Files selected for processing (37)
backend/.envbackend/.htaccessbackend/README.mdbackend/composer.jsonbackend/config/learning_paths.phpbackend/migrations/database.sqlbackend/php.inibackend/public/.htaccessbackend/public/index.phpbackend/router.phpbackend/routes/api.phpbackend/serve.cmdbackend/serve.ps1backend/serve.shbackend/setup.cmdbackend/setup.ps1backend/setup.shbackend/src/Controllers/ArticleController.phpbackend/src/Controllers/AuthController.phpbackend/src/Controllers/CommentController.phpbackend/src/Controllers/CommunityController.phpbackend/src/Controllers/Concerns/AuthenticatesRequests.phpbackend/src/Controllers/EventController.phpbackend/src/Controllers/ResourceController.phpbackend/src/Helpers/Str.phpbackend/src/Models/Article.phpbackend/src/Models/Comment.phpbackend/src/Models/Event.phpbackend/src/Models/Forum.phpbackend/src/Models/Resource.phpbackend/src/Models/User.phpbackend/src/Services/MailService.phpbackend/storage/downloads/ethiopian-tech-templates.htmlbackend/storage/downloads/localized-cheatsheets.htmlbackend/storage/downloads/offline-tutorials.htmlbackend/storage/uploads/.gitkeepbackend/tests/Test.php
Comment on lines
+341
to
+343
| -- Hash below is bcrypt of 'admin123' (cost=10). Use setup.php which auto-generates a fresh hash. | ||
| INSERT INTO users (username, email, password_hash, full_name, is_active) VALUES | ||
| ('admin', 'admin@etc.com', '$2y$10$YourHashHere', 'Administrator', TRUE); | ||
| ('admin', 'admin@etc.com', '$2y$10$92IXUNpkjO0rOQ5byMi.Ye4oKoEa3Ro9llC/.og/at2.uheWG/igi', 'Administrator', TRUE); |
There was a problem hiding this comment.
Avoid shipping shared hardcoded credential hashes for active seeded accounts.
Line 343 and Lines 406–408 seed active users (including admin) with the same fixed hash, which creates predictable default credentials and a direct account-takeover risk if this seed is used beyond isolated local setups. Move credential creation to runtime setup with unique generated passwords (or disable these users by default until reset).
Also applies to: 404-408
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/migrations/database.sql` around lines 341 - 343, The migration seeds
an active admin user with a hardcoded bcrypt password hash (INSERT INTO users
... ('admin', ..., '$2y$10$92IXUNpk...', 'Administrator', TRUE)), which creates
predictable credentials; remove the fixed password hash from the migration and
change seeding so credentials are created at runtime or the account is disabled
by default: either (a) stop inserting a usable password_hash in the migration
(set password_hash to NULL) and set is_active = FALSE for seeded users, or (b)
move password generation to your setup script (setup.php) to insert a unique
bcrypt hash for username 'admin' (and other seeded users) at install time;
update any code that expects an active admin in the migration to rely on the
setup step instead.
Comment on lines
+15
to
+21
| if (str_starts_with($uri, '/storage/') && is_file($storagePath)) { | ||
| return false; | ||
| } | ||
|
|
||
| // Serve backend root static files (assets, etc.) if they exist | ||
| if ($uri !== '/' && is_file(__DIR__ . $uri)) { | ||
| return false; |
There was a problem hiding this comment.
Block path traversal and secret-file exposure in static file passthrough.
Line 20 serves arbitrary backend-root files (for example /.env), and Line 15 trusts raw /storage/... paths that can resolve outside storage (for example /storage/../../.env). Restrict passthrough to canonical paths inside allowed roots and remove the backend-root fallback.
🔒 Proposed fix
$uri = parse_url($_SERVER['REQUEST_URI'] ?? '/', PHP_URL_PATH);
$publicPath = __DIR__ . '/public' . $uri;
-$storagePath = __DIR__ . $uri;
+$storageRoot = realpath(__DIR__ . '/storage');
// Serve real static files from the public/ directory
if ($uri !== '/' && is_file($publicPath)) {
return false;
}
// Serve storage/ files directly (e.g. storage/downloads/...)
-if (str_starts_with($uri, '/storage/') && is_file($storagePath)) {
- return false;
-}
-
-// Serve backend root static files (assets, etc.) if they exist
-if ($uri !== '/' && is_file(__DIR__ . $uri)) {
- return false;
+if (str_starts_with($uri, '/storage/')) {
+ $resolved = realpath(__DIR__ . $uri);
+ if (
+ $resolved !== false
+ && $storageRoot !== false
+ && is_file($resolved)
+ && str_starts_with($resolved, $storageRoot . DIRECTORY_SEPARATOR)
+ ) {
+ return false;
+ }
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/router.php` around lines 15 - 21, The current passthrough checks
(str_starts_with($uri, '/storage/') and the backend-root is_file(__DIR__ . $uri)
fallback) allow path traversal and exposure of arbitrary files; change the logic
to resolve and validate canonical paths: for storage paths, compute the realpath
of the requested file (use $storagePath or realpath(__DIR__ . '/storage/' .
substr($uri, strlen('/storage/')))) and only return false if realpath succeeds
and the resolved path starts with the known storage root directory (preventing
../../ escapes); remove the backend-root fallback that checks is_file(__DIR__ .
$uri) so no arbitrary backend-root files (like .env) are served. Ensure you
handle leading slashes consistently and treat missing realpath as non-existent.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fixed backedn
Summary by CodeRabbit
Release Notes
New Features
Documentation