Megiddo: frontend domain refactor + development tooling

[esdraelon]

Summary

• Frontend refactor: Move business logic and database access out of orkui/ into domain services / models (Megiddo R-* stack through residual lib work, idiom alignment, Phase 3 gate fixes, and post-refactor rebases onto master).
• Development tooling: Test sandbox / ork-db, fuzzy-validator (including v2 classified drift overlays and agent skills), characterization + Playwright coverage, Infection / php-cs-fixer under tools/, and Megiddo docs/skills for the process.

Test plan

• PHPUnit + relevant e2e for touched domains
• bin/ork-db deploy-sandbox / dual-profile fuzzy validate as needed
• cd tools/fuzzy-validator/python && pytest tests/ --cov-fail-under=95
• Spot-check no new $DB / domain Ork3::$Lib in orkui/ for migrated surfaces

[baltinerdist]

(This is a Claude-generated finding, so take with a grain of salt. --TofH)

Automated review — frontend domain refactor

Focused on the ~123 application-code files (the refactor half; the fuzzy-validator / ork-db tooling, tests, and docs weren't the focus of this pass). Overall the refactor is clean and in the right direction: the changed controllers are genuinely thin (no leftover raw $DB-> or Ork3::$Lib domain logic), the SOAP defs/registration line up, DB idioms (Clear() / Next() / int-casts) are consistent, auth checks were moved rather than dropped, and templates stayed plain PHP. Three inline comments below flag specific code issues. A few things that don't anchor to one line:

Blocker — Event module is broken at runtime (see inline on model.Event.php)

Splitting Event into Event / EventPlanning / EventEmbed left the orkui models calling the planning/embed methods through new APIModel('Event'), which binds to a plain Event instance that has no __call. ~21 call sites across model.Event.php and model.EventPlanning.php fatal at runtime — Event detail/index/create/occurrence pages, the EventAjax set_status / preview / staff / schedule-CRUD / copy / create-with-copy endpoints, and the published-schedule embed. The unit tests passed because they exercise new EventPlanning() directly rather than the APIModel('Event') path the app actually uses. The fix pattern already exists at model.EventPlanning.php:138 (_planning() helper) — model.Event.php needs an equivalent EventPlanning/EventEmbed handle.

Behavior changes worth confirming are intentional

• Report::GetLadderAwardGrid — park reports now use the kingdom's kingdomaward column set instead of the global is_ladder=1 list, and master-marking uses a 14-entry map that marks ladders (Jovius/Mask/Zodiac/Hydra/Griffin) master didn't.
• Report::GetVotingEligible — merges VotingRules::rulesForKingdom() when KingdomId > 0 and no explicit mode, changing eligibility defaults for kingdoms with stored rules.
• SearchService::ScopedPlayerSearch — on the KD: / KD:PK abbreviation-prefix path, the suspended/inactive-to-bottom ordering CASE is lost (falls back to m.persona order).
• Award::GetAwardOptionListHtml — custom award/title options now render above the Ladder Awards optgroup (previously below).
• Weather::coords_for_calendar_detail — rejects coords only when both lat/lng are 0.0 (master rejected if either was 0/null); near-nil impact for US parks.

Process

This bundles a live-product refactor with ~1150 files of dev tooling / tests / docs. Consider splitting the tooling into its own PR so the refactor can be reviewed and landed on its own once the Event blocker is fixed and it's been exercised through the real app path.

[baltinerdist]

(This is a Claude-generated finding, so take with a grain of salt. --TofH)

Runtime-fatal delegation. $this->Event is new APIModel('Event'), which binds to a plain Event instance. GetSchedule lives only on EventPlanning (class.EventPlanning.php:292), and neither Event nor its base Ork3 defines __call — so APIModel::__call does call_user_func_array([new Event(), 'GetSchedule'], ...), throwing Error: Call to undefined method Event::GetSchedule().

Same issue hits ~21 planning/embed calls across this file and model.EventPlanning.php (GetOccurrencePageData, GetPublishedScheduleEmbed → EventEmbed, SetEventStatus, AddEventStaff, schedule CRUD, CreateEventWithCopy, ...). The working pattern is already in model.EventPlanning.php:138 (_planning()); model.Event.php needs an EventPlanning/EventEmbed handle. Worth adding a test that goes through the APIModel('Event') path — the current tests call new EventPlanning() directly and miss this.

[esdraelon]

Fixed in 656480ae88129c1ca9c0b0742ce988b8ebcf5bf7.

APIModel('Event') only instantiates plain Event, so planning/embed methods (GetSchedule, GetPublishedScheduleEmbed, GetOccurrencePageData, schedule CRUD, etc.) fatalled at runtime. Model_Event now routes those through _planning() / _embed() helpers (EventPlanning / EventEmbed); Model_EventPlanning routes its planning APIs the same way. Added ModelEventDelegationTest (integration) proving the UI path no longer fatals; full PHPUnit suite green.

[baltinerdist]

(This is a Claude-generated finding, so take with a grain of salt. --TofH)

Missing authorization. Unlike the sibling banner mutators, CopyBanner performs no IsAuthorized / canEditBanner check — but it's registered as BannerCopyBanner (BannerService.registration.php / BannerService.function.php:21) and reachable via the JSON endpoint (orkservice/Json/index.php). It calls deleteBannerFiles($targetBase) and then overwrites has_banner/config, so an unauthenticated caller could wipe or replace any same-type entity's banner. Suggest requiring a token + canEditBanner on the target (ideally the source too), or removing it from service registration.

[esdraelon]

Fixed in 4c13c14f76d40f47fbfb3a782f7d2a6728162eda.

CopyBanner now requires Token and canEditBanner on both source and target (same gate as SetBanner). WSDL CopyBannerRequest includes Token; CreateEventWithCopy passes its Token through. Added testCopyBannerRequiresTokenAndAuth and banner module coverage on create-with-copy; full PHPUnit suite green.

[baltinerdist]

(This is a Claude-generated finding, so take with a grain of salt. --TofH)

SQL injection. mysql_real_escape_string() is a no-op shim in this codebase, so $methodFilter (from $filters['MethodCall'], line 65) is interpolated unescaped into the WHERE clause here. The date and entity-type filters are safe (regex/whitelist-validated above), but MethodCall has no other guard, so a crafted request bypasses the UI dropdown. It's admin-gated, but still injectable. Suggest whitelisting against ListAuditMethods() or regex-validating the value.

[esdraelon]

Fixed in b1875f6332bd82741cf3e28446ead6954d1a780c.

ListAuditLog now whitelists MethodCall against ListAuditMethods(). Unknown/injection payloads are rejected (AND 1=0) instead of being interpolated — so they cannot broaden results. Legitimate method filters still work. Added testMethodCallWhitelistRejectsInjection; full PHPUnit suite green.

[baltinerdist]

(This is a Claude-generated finding, so take with a grain of salt. --TofH)

Follow-up review — access control on the domain/service migration

A deeper pass on the refactor surfaced a consistent access-control gap: logic moved out of session-gated controllers into system/lib/ork3 / orkservice classes that are exposed on the unauthenticated JSON/SOAP dispatcher, where several new methods skip the per-method Token gate their existing siblings enforce. Four inline comments below. (I checked each against master to confirm the branch introduces them — a batch of other real-but-pre-existing issues, e.g. the StartDate mysql_real_escape_string pattern in Report.php and a RemoveParkDay precedence bug, were left out because they already exist on master and aren't introduced by this refactor.)

[baltinerdist]

(This is a Claude-generated finding, so take with a grain of salt. --TofH)

Security theme — unauthenticated access to methods moved onto the JSON/SOAP layer. The JsonServer dispatcher (system/lib/system/class.JsonServer.php — RunServer/run_call/validate_method) does no session/auth check, so ORK's access-control model is per-method Token gating: e.g. Administration::PurgeLogs($Token)/OptimizeTable($Token) call Ork3::$Lib->authorization->IsAuthorized($Token) + HasAuthority($mundane_id, AUTH_ADMIN, 0, AUTH_CREATE). The methods/classes below are new in this branch, sit on the JsonServer allowlist (orkservice/Json/index.php), and skip that gate.

GetGlobalAdminGrants() — unauthenticated PII disclosure. New in this branch, on the already-exposed Administration class, takes no $Token and calls no IsAuthorized/HasAuthority. orkservice/Json/index.php?call=Administration/GetGlobalAdminGrants therefore returns every ORK-admin's identity to any unauthenticated caller. Add a $Token parameter and gate on IsAuthorized($Token) + HasAuthority($mundane_id, AUTH_ADMIN, 0, AUTH_CREATE), matching PurgeLogs/OptimizeTable.

[esdraelon]

Fixed in b71d8fd8942fb05fbc1ef9d5b8bcb759f51e9b7c.

GetGlobalAdminGrants now requires Token + global AUTH_ADMIN (same gate as PurgeLogs). Model_AdminDashboard / Controller_Admin pass the session token. Non-admin/empty Token returns NoAuthorization. Full PHPUnit suite green.

[baltinerdist]

(This is a Claude-generated finding, so take with a grain of salt. --TofH)

GetScopedAuths() / GetKingdomParkAuths() (line 346) / GetEventInheritedPermissions() (line 393) — unauthenticated IDOR. Same exposure as GetGlobalAdminGrants: three new, ungated methods on the JsonServer-exposed Administration class. They back Controller_Admin::permissions(), which correctly gates on has_authority($uid, $type, $id, AUTH_CREATE) at the controller — but that check is bypassed by calling the class directly, e.g. ?call=Administration/GetScopedAuths&type=Kingdom&id=<any>, disclosing who holds authority over any kingdom/park/event. Move the authority check into the domain methods: require + validate a Token, then HasAuthority(..., AUTH_CREATE) before returning.

[esdraelon]

Fixed in 302fb5dedd5419c2004a56305f3319fb6fac2dfc.

GetScopedAuths, GetKingdomParkAuths, and GetEventInheritedPermissions now require Token + scoped AUTH_CREATE (or global admin). AdminDashboard/controller pass the session token. Cross-tenant Tokens are denied. Full PHPUnit suite green.

[baltinerdist]

(This is a Claude-generated finding, so take with a grain of salt. --TofH)

GetServerHealthProcesses() / GetServerHealthDbStatus() (line 81) — unauthenticated server internals. Two new, ungated methods on the JSON-exposed Administration class (no $Token/IsAuthorized). They back the admin-only Server Health page but are reachable at ?call=Administration/GetServerHealthProcesses, exposing the live DB process list and server/DB status to anyone. Gate with the same Token + HasAuthority(AUTH_ADMIN, 0, AUTH_CREATE) check used by PurgeLogs/OptimizeTable.

[esdraelon]

Fixed in 8a9e41fcbf0a906af10541dce744aabec0356dee.

GetServerHealthDbStatus / GetServerHealthProcesses now require Token + global AUTH_ADMIN (PurgeLogs gate). Weather summary left ungated per plan. AdminDashboard/controller pass session token. Full PHPUnit suite green.

[baltinerdist]

(This is a Claude-generated finding, so take with a grain of salt. --TofH)

AuthorizationGate::HasAuthority() — unauthenticated privilege oracle. This branch adds AuthorizationGate to the JsonServer allowlist (orkservice/Json/index.php) and registers Authorization.HasAuthority (orkservice/Authorization/AuthorizationService.registration.php). HasAuthority(array $request) reads MundaneId straight from the request (line 20) with no Token/IsAuthorized gate — so ?call=AuthorizationGate/HasAuthority&MundaneId=<any>&Type=...&Id=... lets an unauthenticated caller probe any player's authority over any entity. Resolve the acting identity from IsAuthorized($request['Token']) (as AddAuthorization/RemoveAuthorization do) instead of trusting a client-supplied MundaneId, or drop it from the public allowlist and keep it in-process only.

[esdraelon]

Fixed in 7bb32f59a7de07d3c20ee24896be85aeb0c27ca6.

AuthorizationGate::HasAuthority now resolves the actor from Token via IsAuthorized and ignores client MundaneId. Missing Token → BadToken; stranger Token + admin MundaneId no longer escalates. Full PHPUnit suite green.

[baltinerdist]

(This is a Claude-generated finding, so take with a grain of salt. --TofH)

Follow-up review (batch 3) — access control on the migrated domain/service layer

This batch focuses on the dominant theme from a deeper pass: the new orkservice/domain layer exposes SOAP/JsonServer methods with no internal Token/IsAuthorized/HasAuthority gate, so controller-only session gates are bypassable by calling the domain method directly. Also included: a couple of correctness defects (a :array return type over a nullable delegate; an undefined breadcrumb variable) and a missing active-unit filter. Every item was checked against master to confirm this branch introduces it. Nine inline comments follow.

Method note: these were surfaced by a multi-agent review pass and each independently re-verified for scope/reachability; still worth a maintainer's eye, hence the per-comment disclaimer.

[baltinerdist]

(This is a Claude-generated finding, so take with a grain of salt. --TofH)

Unauthenticated exposure of the Live dashboard - bypasses Controller_Live's anti-scrape session gate

LiveService::GetStats()/GetRecent() (class.LiveService.php:8) wrap Live::stats()/recent() with no authorization:

public function GetStats(): array { $live = new Live(); return $live->stats(); }
public function GetRecent(): array { $live = new Live(); return $live->recent(); }

Neither method takes a Token or calls IsAuthorized/HasAuthority. The class is new on this branch, added to the JsonServer allowlist (orkservice/Json/index.php, 'LiveService') and SOAP-registered (orkservice/Live/LiveService.registration.php -> Live/GetStats, Live/GetRecent). The JsonServer/SOAP dispatcher performs no session or auth check (call_endpoint() just does new $class() + invoke; validate_method() returns true under the default METHOD_ANY), so both are reachable unauthenticated, e.g.:

GET orkservice/Json/index.php?call=LiveService/GetRecent
GET orkservice/Json/index.php?call=LiveService/GetStats

This directly defeats the gate Controller_Live::stats()/recent() documents and enforces - if (!isset($this->session->user_id)) { ...status 5, Not logged in... }, docblock: "Both JSON endpoints are session-gated ... so bots can't scrape the aggregated view." An anonymous caller can now scrape the full rolling-24h aggregated view: per-park/per-event counts across three time windows, park/event coordinates, kingdom names, weather, and the recent sign-in ticker (timestamps + park/event IDs) - i.e. real-time disclosure of where people are physically gathering. (recent() deliberately strips mundane_id on the wire as "private," so there is no direct PII leak; the harm is the scraping/real-time-location exposure the gate was built to prevent.)

Fix: self-gate both methods before returning data, mirroring the controller and the other domain services - require a Token and reject when Ork3::$Lib->authorization->IsAuthorized($request['Token']) <= 0 (return the 5/Not logged in shape). A controller-only gate does not protect the directly-callable domain endpoint. If the aggregated view is genuinely public, drop the controller's session gate instead so the two paths are consistent - but the documented anti-scrape intent argues for gating the webservice.

[esdraelon]

Fixed in b1bb4fca7f5ee4ae6d955a8815f32b01e63fd27e.

LiveService::GetStats / GetRecent now require a valid Token (BadToken otherwise). Registration accepts Token; Model_Live / Controller_Live pass the session token while keeping the controller session gate. Full PHPUnit suite green.

[baltinerdist]

(This is a Claude-generated finding, so take with a grain of salt. --TofH)

Unauthenticated SOAP read of draft-event details + attendee dietary/allergen PII

EventPlanning::GetOccurrencePageData() (class.EventPlanning.php:233) is exposed as the SOAP op Event.GetOccurrencePageData (EventService.registration.php:235 -> EventService.function.php:141), and the wrapper calls it with no auth:

function GetOccurrencePageData($request) {
    $EP = new EventPlanning();
    return $EP->GetOccurrencePageData($request);   // no Token / IsAuthorized / HasAuthority
}

Any registered SOAP op is reachable unauthenticated at the service endpoint. The domain method only validates ids (valid_id) and detailBelongsToEvent() - an id-pairing check, not an authorization check. So an unauthenticated caller who supplies any valid EventId/EventCalendarDetailId pair (trivially enumerable) receives:

• EventStatus (including draft), CreatorId, KingdomId, ParkId, StaffList, ScheduleList, EventFees, ExternalLinks, AtPark - i.e. full details of not-yet-published events, and
• with IncludeDietary set, DietarySummary - per-attendee Persona + MundaneId + the complete dietary/allergen/medical profile from mundane_dietary (vegan/halal/kosher, milk/egg/fish/shellfish/treenut/peanut/wheat/soy allergens, etc.) for everyone RSVP'd or checked in (fetchDietarySummary, line 822).

This is health-adjacent PII plus pre-publication event data, leaking to anonymous callers. All of this is new on this branch (method, SOAP registration, and wrapper are absent on master).

Fix: gate in the domain layer before returning anything - require a valid Token via Ork3::$Lib->authorization->IsAuthorized($request['Token']), and require staff/organizer authority (HasAuthority over the event's kingdom/park, or a staff-cap match) for non-published events and especially before returning DietarySummary. Public callers should at most get published, non-PII fields.

[esdraelon]

Fixed in d4a4922.

GetOccurrencePageData now requires a valid Token (and manage/staff/creator/admin authority) for non-published events. IncludeDietary is ignored unless the Token actor has feast capability via CanManageEventDetail(..., 'feast'); anonymous published reads still work with an empty DietarySummary. Token is on the WSDL request and threaded from Model_Event / Controller_Event. Tests cover anonymous draft denial, stranger draft denial, anonymous IncludeDietary → empty summary, and feast Token → summary.

[baltinerdist]

(This is a Claude-generated finding, so take with a grain of salt. --TofH)

Unauthenticated writes: SetCalendarDetailFeesAndLinks / SetCalendarDetailEventType have no Token/authority gate

Both methods are new on this branch, SOAP-registered (Event.SetCalendarDetailFeesAndLinks at EventService.registration.php:256; Event.SetCalendarDetailEventType at :263) with passthrough wrappers (EventService.function.php:159, :165) that instantiate EventPlanning and forward the raw request. The JsonServer/SOAP dispatcher does no session/auth check, so both are reachable unauthenticated.

SetCalendarDetailFeesAndLinks (class.EventPlanning.php:305) validates only valid_id($detailId) then calls syncCalendarDetailFeesAndLinks(), which for any caller-supplied EventCalendarDetailId:

• DELETE FROM event_fees WHERE event_calendardetail_id = $detailId and re-inserts attacker-supplied fee rows
• DELETE FROM event_links WHERE event_calendardetail_id = $detailId and re-inserts attacker-supplied link rows (titles/URLs that render on the public event page)

SetCalendarDetailEventType (class.EventPlanning.php:332) likewise reads no Token and UPDATEs event_calendardetail.event_type for an arbitrary detail id (the value is whitelisted against CALENDAR_DETAIL_EVENT_TYPES and sq-escaped, so this is an authorization/tamper issue, not injection).

Neither path calls IsAuthorized/HasAuthority. An anonymous caller can wipe and rewrite the fee schedule and external links of any occurrence (IDOR + stored-content injection into public pages) and retype any occurrence's calendar detail. Sibling methods in the same class show the intended pattern - SetEventStatus/ReconcilePastAttendance do $mid = IsAuthorized($request['Token']) then HasAuthority(...)/CanManageEventDetail(...).

Fix: in both methods resolve the actor and gate before mutating:

$mundaneId = Ork3::$Lib->authorization->IsAuthorized($request['Token'] ?? '');
if ($mundaneId <= 0) { return BadToken(); }
$eventId = (int)($request['EventId'] ?? 0);
if (!valid_id($detailId) || !valid_id($eventId) || !$this->detailBelongsToEvent($detailId, $eventId)) {
    return InvalidParameter('Invalid event or detail id');
}
if (!$this->CanManageEventDetail($mundaneId, $eventId, $detailId, 'manage')) {
    return NoAuthorization();
}

[esdraelon]

Fixed in 15cba9a68f80dc5913ef7f51479127420723d9d1.

SetCalendarDetailFeesAndLinks / SetCalendarDetailEventType now require Token, detail ownership, and manage authority (same pattern as SetEventStatus). WSDL + Model_Event / Controller_Event pass Token. No/bad Token leaves DB unchanged. Full PHPUnit suite green.

[baltinerdist]

(This is a Claude-generated finding, so take with a grain of salt. --TofH)

Unauthenticated PII disclosure: EventPlanning::GetDietarySummaryForOccurrence leaks every attendee's dietary/allergen data

GetDietarySummaryForOccurrence() (class.EventPlanning.php:479) reads only EventCalendarDetailId and returns fetchDietarySummary() with no auth check:

public function GetDietarySummaryForOccurrence($request)
{
    $detailId = (int) ($request['EventCalendarDetailId'] ?? 0);
    ...
    return ['Status' => Success(), 'Items' => $this->fetchDietarySummary($detailId)];
}

This is registered as the SOAP op Event.GetDietarySummaryForOccurrence (EventService.registration.php:277) whose request type OccurrenceDetailRequest carries only EventCalendarDetailId - no Token field - and the wrapper (EventService.function.php:177) forwards $request verbatim. Since the JsonServer/SOAP dispatchers perform no session/auth check, any anonymous caller can supply an arbitrary detail id and harvest, for every RSVP'd/checked-in attendee: mundane_id, persona, and the full dietary/allergen/medical profile (halal/kosher, allergen_peanuts, allergen_gluten, etc.). The row's IsAnonymous flag is returned as data but never enforced, so opted-out attendees are exposed too.

Every other mutating/reading method in this class gates via Ork3::$Lib->authorization->IsAuthorized($request['Token'] ?? '') + CanManageEventDetail(..., 'feast'), and the UI already restricts this data behind CanManageFeast (controller.Event.php:520). The domain method must self-gate identically:

$mundaneId = Ork3::$Lib->authorization->IsAuthorized($request['Token'] ?? '');
$detailId  = (int) ($request['EventCalendarDetailId'] ?? 0);
$eventId   = /* resolve from $detailId */;
if (!$this->CanManageEventDetail($mundaneId, $eventId, $detailId, 'feast')) {
    return Denied();
}

Add Token to OccurrenceDetailRequest in EventService.definitions.php so callers can pass it.

[esdraelon]

Fixed in $(git rev-parse HEAD).

GetDietarySummaryForOccurrence now requires a valid Token and CanManageEventDetail(..., 'feast') after resolving the occurrence's event id. Token was added to OccurrenceDetailRequest. Tests cover anonymous → BadToken, non-feast → NoAuthorization, and feast Token → Items.

[baltinerdist]

(This is a Claude-generated finding, so take with a grain of salt. --TofH)

Unauthenticated write: Event.SetRsvp / Event.WithdrawRsvp trust MundaneId with no Token - anyone can set or clear any player's RSVP for any event

Both methods are new in this branch and newly registered as SOAP operations (Event.SetRsvp, Event.WithdrawRsvp in orkservice/Event/EventService.registration.php), making them directly reachable through the JsonServer/SOAP dispatcher, which performs no session/auth check.

class.Event.php:94 SetRsvp() and :183 WithdrawRsvp() read MundaneId directly from the request and INSERT/UPDATE/DELETE event_rsvp rows with no Token/IsAuthorized/HasAuthority gate:

public function SetRsvp($request)
{
    $detailId  = (int)($request['EventCalendarDetailId'] ?? 0);
    $mundaneId = (int)($request['MundaneId'] ?? 0);   // trusted, no caller check
    ...
    // INSERT/UPDATE ork_event_rsvp for that mundane_id
}

An unauthenticated caller can therefore set, change, or (via WithdrawRsvp) delete any player's RSVP on any event. The sibling RemoveRsvp() in the same file already gates correctly (AuthorizedByController -> Token -> IsAuthorized($token) -> BadToken()), so the pattern to follow is right there.

Fix: in both SetRsvp and WithdrawRsvp, resolve the actor via $actorId = Ork3::$Lib->authorization->IsAuthorized((string)($request['Token'] ?? ''));, return BadToken() if !valid_id($actorId), and use $actorId as the RSVP mundane_id - only permitting a differing MundaneId when the actor has authority over that player (mirroring RemoveRsvp's AuthorizedByController/authority path).

[esdraelon]

Fixed in 4698103.

SetRsvp / WithdrawRsvp now require a valid Token and resolve the actor via IsAuthorized. Own MundaneId may mutate self; other players require RemoveRsvp-equivalent authority (AUTH_EVENT EDIT or event_staff can_attendance). Token is threaded from WSDL, Model_Event, and Event/EventRsvpAjax controllers. Integration tests cover no Token → BadToken, cross-player IDOR denial, self success, and staff/editor success.

[baltinerdist]

(This is a Claude-generated finding, so take with a grain of salt. --TofH)

Unauthenticated exposure of the Weather domain service - controller session gate is bypassable, plus third-party quota abuse

Controller_Weather::index/day gate behind $this->session->user_id (lines 28/67), but the logic now lives in WeatherService (system/lib/ork3/class.WeatherService.php), which has no internal authorization. The class is new on this branch and reachable unauthenticated two ways:

• JsonServer allowlist: 'WeatherService' in orkservice/Json/index.php:43 -> orkservice/Json/index.php?call=WeatherService/GetDailySummary
• SOAP registration: orkservice/Weather/WeatherService.registration.php registers Weather/GetDailySummary, GetPlayForDate, GetUpcomingEventsWithForecast, GetFreshnessPhrase, GetStripSeverities, GetArchiveForPark.

The JsonServer/SOAP dispatcher does no session/auth check, and neither WeatherService nor Weather performs any IsAuthorized/HasAuthority/Token check, so the controller gate is moot - the domain methods are directly callable.

Two concrete impacts:

1. Data disclosure - GetPlayForDate/GetUpcomingEventsWithForecast return park & event coordinates (lat/lng), park-day schedules, descriptions, event names/dates and kingdom rollups; GetDailySummary/GetArchiveForPark return per-park daily summaries and the historical archive - all to anonymous callers.
2. Third-party quota abuse - several paths issue outbound Open-Meteo HTTP calls on cache miss: GetArchiveForPark(parkId, date) -> archive_for_date -> archive_for_coords -> http_get, and GetDailySummary/GetUpcomingEventsWithForecast -> for_park -> refresh_all_active_parks. The class itself documents the Open-Meteo rate-limit/credit budget (429 handling, BATCH_LIMIT_PARKS/BATCH_LIMIT_VENUES chunking). An unauthenticated attacker can walk distinct parkId/date pairs (dates valid back to 1940) to force uncached fetches and exhaust that shared budget -> 429s break weather for legitimate cron refreshes and users.

Fix: self-gate each WeatherService method with the standard token check (($mid = Ork3::$Lib->authorization->IsAuthorized($request['Token'])) > 0, else reject) before dispatching, matching the other domain services, and add a Token field to each entry in WeatherService.registration.php. Gating only the orkservice entry file is insufficient since the class is on the JsonServer allowlist independently.

[esdraelon]

Fixed in ee24353cbbb78010b5b9a029f1aa4cfcc178c537.

Every WeatherService public method now requires a valid Token. Registration + Model_Weather / Controller_Weather pass the session token; controller session gate kept. Invalid Token fails; valid Token returns existing shapes. Full PHPUnit suite green.

[baltinerdist]

(This is a Claude-generated finding, so take with a grain of salt. --TofH)

GetArchiveForPark() return-type contract broken - : array but the delegate returns null -> fatal TypeError

WeatherService::GetArchiveForPark() (class.WeatherService.php:49) is declared with a : array return type but returns the value of its Weather delegate (archive_for_park/archive_for_date), which routinely yields null - on a cache miss with a failed/empty Open-Meteo fetch, missing park coordinates, or an out-of-range date. When the delegate returns null, PHP raises TypeError: Return value must be of type array, null returned, fataling the request instead of degrading gracefully. This is directly reachable: the method is on the JsonServer allowlist and SOAP-registered as Weather/GetArchiveForPark, and the archive path is exactly the one that most often has no cached row to return.

Fix: coalesce the delegate result before returning, e.g. return $result ?? []; (or relax the signature to : ?array and have callers handle null). Audit the sibling WeatherService methods that declare : array while forwarding a Weather delegate that can return null for the same defect.

[esdraelon]

Fixed in 7f8b3dbec45632ce72df40c1f8f7709f45fc0862.

WeatherService::GetArchiveForPark(): array now returns $this->weather->archive_for_date(...) ?? [], so invalid park / lag-window / bad-date misses no longer TypeError. Added testWeatherServiceGetArchiveForParkMissReturnsEmptyArray; full PHPUnit suite green.

[baltinerdist]

(This is a Claude-generated finding, so take with a grain of salt. --TofH)

Event permissions breadcrumbs silently dropped - $evKingdomId/$evParkId are undefined here.

The refactor that moved the event-inherited logic into AdminDashboard->event_inherited_permissions() -> Administration::GetEventInheritedPermissions() lost the id variables. That method computes $evParkId/$evKingdomId internally (class.Administration.php:416-417) but its return array (lines 482-487) exposes only creator/parkAuths/kingdomAuths/parkName/kingdomName. The controller reads back just the names into $inheritedParkName/$inheritedKingdomName (lines 836-838), yet the breadcrumb block still references $evKingdomId/$evParkId:

if (!empty($evKingdomId)) {
    $this->data['menu']['kingdom'] = ['url' => UIR . 'Kingdom/profile/' . $evKingdomId, 'display' => $inheritedKingdomName];
}
if (!empty($evParkId)) {
    $this->data['menu']['park'] = ['url' => UIR . 'Park/profile/' . $evParkId, 'display' => $inheritedParkName];
}

These locals are never assigned in permissions() (they were on master, inline). Under PHP 8 both !empty() checks evaluate false (with an Undefined-variable warning), so the Kingdom and Park breadcrumbs on the Event permissions page never render - a regression from master, which showed them.

Fix: add parkId/kingdomId to the GetEventInheritedPermissions() return array and read them into $evKingdomId/$evParkId alongside the name assignments before the breadcrumb block.

[esdraelon]

Fixed in 5b5d794e94256bd601977caeec6506255ff4c34f.

GetEventInheritedPermissions now returns parkId / kingdomId; Controller_Admin assigns $evParkId / $evKingdomId from that payload so event permission breadcrumbs work again. Assertions added in AdminPermissionsTest; full PHPUnit suite green.

[baltinerdist]

(This is a Claude-generated finding, so take with a grain of salt. --TofH)

UniversalSearch builds the unit query without an active-status filter, so retired units leak into the global search dropdown:

$unitWhere = $fold('name') . " LIKE '%{$term}%'";
... FROM ork_unit WHERE {$unitWhere} ORDER BY name LIMIT {$unitBudget}

Every other listing path scopes units to active ones (class.Report.php:951 / :1017 use u.active = 'Active'), and the sibling park branch right here already filters p.active = 'Active' (line 633). Units get set to active='Retired' via WaffleUnit() (class.Unit.php:481), so this query surfaces retired units that shouldn't appear.

Suggest matching the park branch:

$unitWhere = "active = 'Active' AND " . $fold('name') . " LIKE '%{$term}%'";

Minor (retired units aren't sensitive), but it's an inconsistency introduced with this new method.

[esdraelon]

Fixed in 67249fe13b22102e59571edb6fe72ac08efeb373.

UniversalSearch unit query now prefixes active = 'Active' AND … (matching parks). Added testUniversalSearchExcludesRetiredUnits; full PHPUnit suite green.

[baltinerdist]

(This is a Claude-generated finding, so take with a grain of salt. --TofH)

Follow-up (batch 4) — two remaining verified findings

Hand-verified the last unassessed candidates. Two hold up as branch-introduced (below); a third — GetAttendanceLinks returning whole-event sign-in links to single-session staff — was dropped as pre-existing on master (the event_staff fallback and broad al.event_id WHERE are byte-identical to master, only reformatted here).

[baltinerdist]

(This is a Claude-generated finding, so take with a grain of salt. --TofH)

Behavior change to confirm: attendance now silently reactivates inactive players on the officer and public sign-in-link paths. On master, the "adding attendance reactivates an inactive profile" write (UPDATE mundane SET active = 1) lived only in Controller_AttendanceAjax::add (the AJAX park sign-in). This branch centralizes it into reactivateInactiveMundane() and now calls it from two paths that did not reactivate on master:

• AddAttendance() — line 154 (reached by the officer park/kingdom "Add Attendance" form), and
• UseAttendanceLink() — line 773 (the public signin_link self sign-in path).

Net effect: a player an officer deliberately deactivated is now silently reactivated the moment attendance is added for them — including via a public sign-in link, with no officer action. If that's intended (single source of truth for reactivation), no change needed. If the public/officer paths should not auto-reactivate, gate reactivateInactiveMundane() so it only fires on the path that did so on master. Flagging because it's a silent, branch-introduced broadening of a state-changing write.

[esdraelon]

Fixed in cf50d7a.

Policy decision applied: AddAttendance still reactivates inactive mundanes (officer-authorized credits). UseAttendanceLink no longer calls reactivateInactiveMundane, so public self-service sign-in cannot undo officer deactivation. Tests keep AddAttendance reactivation coverage and assert UseAttendanceLink leaves the player inactive.

[baltinerdist]

(This is a Claude-generated finding, so take with a grain of salt. --TofH)

GetRevokedAwardsForPlayer misclassifies alias-backed revoked titles into the Revoked Awards tab. This new method (backing the profile's Revoked Awards / Revoked Titles split) decides title-vs-award using only aw.officer_role (joined via a.award_id) and ka.is_title (via a.kingdomaward_id):

$titlesSql: ... AND (aw.officer_role != 'none' OR ka.is_title = 1)
$awardsSql: ... AND (aw.officer_role = 'none' OR aw.officer_role IS NULL)
             AND (ka.is_title IS NULL OR ka.is_title = 0)

It never joins award alias ON alias.award_id = a.alias_award_id. But everywhere else in the codebase, an aliased award's title-ness comes from the alias, not the base row — e.g. the active-grant display query uses GREATEST(IFNULL(a.is_title,0), IFNULL(ka.is_title,0), IFNULL(alias.is_title,0)) with left join award alias on alias.award_id = awards.alias_award_id (class.Player.php ~495–507), and revoke_award() itself determines title-ness from the alias's is_title/peerage/officer_role (~2235–2243). So a revoked award whose title semantics live on its alias (e.g. a custom award aliased to a knighthood/noble title) is placed in Revoked Awards instead of Revoked Titles.

Fix: mirror the established pattern — LEFT JOIN award alias ON alias.award_id = a.alias_award_id, and classify with COALESCE(alias.officer_role, aw.officer_role) and GREATEST(IFNULL(ka.is_title,0), IFNULL(alias.is_title,0)) (plus alias peerage where relevant).

[esdraelon]

Fixed in 31a37d8802ad2f7ded4cbc89dbf15b29d7ba94a7.

GetRevokedAwardsForPlayer now LEFT JOINs the alias award and classifies with COALESCE(officer_role) / GREATEST(is_title) (mirroring active grants), so alias-backed revoked titles land in RevokedTitles. Added testGetRevokedAwardsClassifiesAliasTitles; full PHPUnit suite green.

[esdraelon]

Revision 2

Pushed review-fix commits onto megiddo/fuzzy-validator-v2 (also mirrored on fix-pr-492).

Addresses all 18 inline review threads from @baltinerdist (auth gates, Event APIModel delegation, SQLi whitelist, breadcrumbs, search/units, revoked-title alias classification, attendance reactivation policy, etc.). Each item has characterization/unit coverage and a thread reply with the commit SHA.

Plan/checklist: docs/megiddo/refactor/pr-492-review-fixes/

Out of scope for this revision (triaged from the batch-1 summary only): ladder-grid / voting-rules / ScopedPlayerSearch KD ordering / award-option HTML order / weather coords sentinel — follow-ups if desired.

[esdraelon]

Revision 3

Clears the 5 hard HTTP fails found by the expanded page-registry reachability pass. No new reviewer comments were present on Rev2 before this push (only prior esdraelon replies + the Revision 2 issue comment). Original review threads remain unresolved in the UI but were already addressed in Rev2.

HTTP fail fixes

Fail route	Root cause	Fix	Commit
Admin/tournament	Missing Controller_Admin::tournament → ReflectionException/500	Redirect to Tournament / Tournament/worksheet/{id}	a7564b35
Authorization/add_auth/{id}	Path segment string forwarded as AddAuthorization request → TypeError/500	Non-array → redirect Admin/authorization	a2fe83c0
Authorization/del_auth/{id}	Same for RemoveAuthorization	Same guard	a2fe83c0
Kingdom/map/{id}	GetParks miss omitted Parks; array_filter(null) → 500	Always return Parks=[] on miss; map() defensive	2cdd0772
Kingdom/recommendations_panel	Bare action intentionally 400	Redirect home like profile() (id required; use …/{id})	2cdd0772

Registry/tooling: expanded pages.json5, validate_page_http.py, analyze_route_coverage.py, and analysis docs in 20b0f61f.

Tests / validation

• PHPUnit: 288 tests, 0 failures (2 skipped)
• HTTP reachability (validate_page_http.py --profile test): checked=278, ok=190, redirect=88, fail_hard=0, 4xx=0
• Report: tools/fuzzy-validator/analysis/http-reachability-test.md

Remaining known noise

• Many active pages still redirect to home (Route=) after follow — treated as reachable, not hard fails. Often auth/session context or bare-action gates; not blocking Rev3.
• Intentional redirects from the five fixes: tournament → Tournament; auth mutations → Admin/authorization; bare recommendations_panel → home.