Close the remaining fixable risks from the handover - #3662
Merged
Merged
Conversation
queue:monitor was not scheduled and no listener for Laravel's QueueBusy event existed, so a stalled worker during October was noticed by a human wondering why activities had stopped appearing. All three pieces are needed: the scheduled check, the event, and a listener. The threshold is QUEUE_BUSY_THRESHOLD so a noisy alert can be tuned rather than switched off. Co-authored-by: Cursor <cursoragent@cursor.com>
Access to /admin/certificate-backend/* was gated on one hardcoded personal email address, in a public repository, with no role granting access. A new super admin got a 403 and could not operate certificates at all. It now reads a comma-separated CERTIFICATE_ADMIN_EMAILS allowlist and fails closed, saying so in the 403 so the cause is obvious. The profile dropdown link uses the same list rather than its own copy of the address. Co-authored-by: Cursor <cursoragent@cursor.com>
EventController@reject called $this->authorize() inside a try with an empty catch, so the country check was defeated and any ambassador could reject any country's activities - while the identical check on approve was enforced. Also gives the Nova reject action a required reason field. It called reject() with no argument, writing an empty moderation message and emailing the organiser a rejection with no explanation, unlike /pending and /review. Co-authored-by: Cursor <cursoragent@cursor.com>
approved controls whether a leading teacher is listed publicly, so it was the one genuine escalation path through the wide-open $guarded. The only place that mass-assigned it now sets it explicitly. Note that id must stay assignable: the legacy placeholder user relies on mass-assigning 1000000. That same legacy-user creation used a hardcoded remember_token and a time-seeded password; both are random now. Also takes a personal address out of two support test fixtures. Co-authored-by: Cursor <cursoragent@cursor.com>
The community map groups teachers by city_id and skips any group whose city has no coordinates, so a teacher without a city was rendered nowhere at all - present in the query, absent from the page, with nothing explaining why. This is the most common support question about the community page. Their profile now warns them, and the leading-teachers admin list gains a City column and a 'Not set' filter so an admin can find and chase them. Also makes country_iso required on profile update, which the form already marked as required, and fixes the error block beneath it: it was bound to 'country' rather than 'country_iso', so the message could never appear. Co-authored-by: Cursor <cursoragent@cursor.com>
Two failures, one flake, and five files PHPUnit never collected: - CityFactory was still a pre-Laravel-8 closure factory, so App\City had no discoverable factory. - UserRestoreServiceTest never disabled the support_gmail.dry_run guard that refuses writes, so it exercised the refusal rather than the restore. - OnlineEventsWorkflowTest created its fixture at exactly the query's 15-day boundary. The two now() calls are milliseconds apart, so it failed whenever the clock ticked a second in between - about one run in four. - Five files in tests/Feature/ lacked the Test.php suffix and had drifted against the factory API: ->create($attrs, N) instead of ->count(N)->create(), and User::factory() with no App\User import. Repaired and renamed. GermanImports asserted three German cities that ImporterHelper no longer lists; rather than change what Event::imported() matches, it now iterates the helper so the list stays the single source of truth. Also makes ResourceEditorRoleSeeder idempotent - it used Role::create for a role the main seeder already creates, so any re-run threw. 449 passing, up from 431 passing with 1 failing. Co-authored-by: Cursor <cursoragent@cursor.com>
Chapter 12 is reorganised around what the incoming team must act on. It no longer lists problems that are now fixed, and the security items that remain are the ones needing credentials we cannot use on their behalf: rotating the forge SSH keys and APP_KEY, setting CERTIFICATE_ADMIN_EMAILS, and proving the Turnstile secret is valid. Adds chapter 14 on accounts, profiles, roles and moderation, built around the recurring 'why am I not on the community page?' question - the ambassador and leading-teacher halves of that page hide people for entirely different reasons, and both fail silently. Also corrects a claim that was simply wrong: relocate and relocate:country are different commands, not a name collision resolving to one class. Co-authored-by: Cursor <cursoragent@cursor.com>
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
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.
Second pass on the handover. The first PR fixed the risks that were safe to fix without a decision; this one closes the rest, including the items that were previously left for the incoming team.
Security and access
CERTIFICATE_ADMIN_EMAILSallowlist and fails closed, saying so in the 403. The profile dropdown link uses the same list rather than its own copy of the address.EventController@rejectcalled$this->authorize()inside atrywith an emptycatch, so the country check was defeated while the identical check onapprovewas enforced.users.approvedwas mass-assignable, so any futureupdate($request->all())could publish or hide a leading teacher. Now guarded.iddeliberately stays assignable — the legacy placeholder user relies on mass-assigning1000000.remember_tokenand a time-seeded password were used when creating that legacy user. Both are random now.Correctness
reject()with no argument, writing an empty moderation message and emailing a rejection with no reason, while/pendingand/reviewcaptured one. The action now requires a reason.ResourceEditorRoleSeederthrew on any re-run — it usedRole::createfor a role the main seeder already creates.Visibility and operations
/communitywith nothing to tell them why. The map groups teachers bycity_idand skips any group whose city has no coordinates, so they were present in the query and absent from the page. Their profile now warns them, and the admin list gains a City column and a Not set filter so someone can find and chase them. This is the most common support question about that page.queue:monitorwas not scheduled and noQueueBusylistener existed, so a stalled worker in October was noticed by a human wondering why activities had stopped appearing. All three pieces are now in place, with the threshold inQUEUE_BUSY_THRESHOLDso a noisy alert can be tuned rather than switched off.Tests
449 passing, up from 431 passing with 1 failing.
CityFactorywas still a pre-Laravel-8 closure factory, soApp\Cityhad no discoverable factory.UserRestoreServiceTestnever disabled thesupport_gmail.dry_runguard, so it exercised the refusal rather than the restore.OnlineEventsWorkflowTestcreated its fixture at exactly the query's 15-day boundary. The twonow()calls are milliseconds apart, so it failed whenever the clock ticked a second in between — roughly one run in four.tests/Feature/lacked theTest.phpsuffix and had drifted against the factory API. Repaired and renamed, so they run.GermanImportsasserted three German cities thatImporterHelperno longer lists. Rather than change whatEvent::imported()matches, the test now iterates that helper so the list stays the single source of truth.Docs
Chapter 12 is reorganised around what the incoming team must act on, and no longer lists problems that are fixed. What remains needs credentials we cannot use on their behalf. Adds chapter 14 on accounts, profiles, roles and moderation.
Also corrects a claim that was simply wrong:
relocateandrelocate:countryare different commands, not a name collision resolving to one class.Before merging
Two changes alter live behaviour and want a look on dev first:
CERTIFICATE_ADMIN_EMAILSin Forge for dev and live. It fails closed, so leaving it blank locks everyone out of the certificate backend.Made with Cursor