---
name: portal-timesheet-sync-audit
description: "Timesheet entry system security/sync audit in portal repo — fixes 1/3/4/7/9-15 shipped, 2/5/6/16 design doc open, 8 (CSRF) untouched. #16 = jobid 0 reaching prod"
metadata: 
  node_type: memory
  type: project
  originSessionId: 769e73c4-0055-4209-91b5-5c773aae5f66
  modified: 2026-07-22T02:08:23.839Z
---

Audited the portal repo's timesheet entry system (offline PWA sync + legacy/WIP dual stacks) for security, bugs, and sync issues (2026-07-21/22). Full original 15-point list produced in one session; items fixed across two follow-up passes ("1-7" then "9-15", per user's own numbering — #8 CSRF explicitly excluded both times, described by the user as "a known issue").

Shipped fixes (mechanical, no schema/protocol change):
- #1 SQLi + dead permission check in `api/api-timesheets.php` (recids/sid raw concat; re-enabled staff scoping via `$_SESSION['administrator']`/`mod_timesheet`).
- #3 IDOR on timesheet update — both legacy (`api/api-timesheets-update.php`) and new stack (`library/Timesheet.php::update()` now takes `$ownerStaffId`, scopes `WHERE ... AND srcid = ?`; `TimesheetManager::save()` treats 0-rows-affected as failure).
- #4 IDOR on `api/api-timesheets-update-evo-ts.php` push-flag clearer (added `AND srcid = ?`).
- #7 SQLi in `library/Timesheet.php::findEntriesByStaff()` (sprintf→bound params) and `manual_time_sheet_entry.inc:8` job_id (int cast).
- #9 Uncaught PDOException on any DB error mid-batch in `api/api-timesheets-update.php` — wrapped the whole update loop in try/catch → clean 500 JSON instead of a broken response. NOT true atomicity: `DB::getPDO()` opens a fresh connection per call (see `evolution/library/db.php`), so there's no shared handle to wrap in a real transaction — documented inline so this isn't mistaken for the underlying fix.
- #10 Fire-and-forget push-flag-clear call in `js/backgroundDataFetch.6.nomin.js:685` (+ `.min.js` twin) had no `.catch` — added one (logs via `debugLog`). Natural retry already exists (failure leaves server `push=1`, next sync cycle's `exists` branch re-fires it) — this just makes persistent failures visible.
- #11 `timesheets.inc` was serving `timesheets.nomin.js` instead of `timesheets.min.js` (regression from commit `5ee0030`). Verified no drift first (every commit touching one touched the other since the `.nomin.js` rename; both define the identical 93 top-level functions) — switched the `<script src>` back to `.min.js`.
- #12 No validation on duration/start/end in the legacy update path (`api/api-timesheets-update.php`) — added the same `$start===false||$end===false||$end<=$start` guard `TimesheetManager::save()` already had, returns `status: 'invalid'` per row instead of writing a corrupt row.
- #13 Raw PDO exception message echoed to the client on DB connection failure, in all three of `DB::getPDO()`, `EVO::getPDO()`, `ESS_DB::getPDO()` (`evolution/library/db.php`) — now `error_log()`'d server-side and rethrown (previously the swallow-and-continue left `$dbo` undefined, so callers fataled on `->prepare()` of null anyway).
- #14 Static (non-DST-aware) tzgmt offset in `indexDB.nomin.js`'s date conversion — `tzgmt` was stamped once server-side at initial page load (`$timeZoneGMT = date('P', time())`, itself DST-aware) and cached client-side for the life of the page/background worker, so a device left open across a DST boundary drifts an hour. Fixed by having `api/api-timesheets.php`'s pull response include the live `tzgmt`, and having the client (`backgroundDataFetch.6.nomin.js` + `.min.js` twin) refresh its cached value from every timesheets pull cycle, even when there's nothing new to sync.
- #15 `Accessor::getUpdatedFieldsAndValues()` (`portal/library/Accessor.php`) referenced an undefined `$updatedFields` (fixed to `$properties`) and built raw unescaped SQL string concatenation (plus a `.`-vs-`?:` precedence bug making it worse than it looked). Rewrote to return `?`-placeholder fragments matching the class's existing `getFields()`/`getValues()` pattern, and added a companion `getUpdatedValues()` for the bound values. Still unused/dead code — this was a landmine defusal, not a live bug.

Open (design doc written, NOT implemented — awaiting review, see `portal/audits/TIMESHEET_SYNC_DESIGN.md`):
- #2 Conflict detection never fires for portal-vs-portal edits — only Evo-side `push=1` flag triggers the existing conflict UI. Real fix needs a NEW client-only `baseline_last_updated_at` field (stamped at pull time, untouched by edits — distinct from `last_updated_at` which the device always stamps fresh to "now" on every save) sent up with pushes and compared server-side. Backward-compatible (falls back to today's behavior if the field is absent).
- #5 No idempotency on `timesheets_items`/`log` rows on retry — confirmed via schema files neither table has a natural/local key column. Needs an actual `ALTER TABLE` (new `local_key` column + unique constraint) across every tenant DB — bigger migration than the ad-hoc prod `claude` SQL user can do (see [[prod_mysql_access]], no ALTER privilege).
- #6 Race: line item added mid-push gets permanently orphaned — root cause confirmed in `timesheets.nomin.js`: the post-push re-key loop (~line 4199-4209) walks a deep-copied in-memory snapshot (`tsTimesheet_Items`, captured when the edit modal opened) instead of live IndexedDB, so items written after the snapshot but before the push response is processed never get re-keyed to the new server id. Pure client-side fix, no schema change.

**Why:** this is live payroll-linked data (timesheet hours feed billing/payroll), so the higher-risk fixes (schema migration + sync protocol changes) got split out for explicit review rather than shipped unilaterally alongside the mechanical SQLi/IDOR fixes.

**How to apply:** when asked to continue this work, read `portal/audits/TIMESHEET_SYNC_DESIGN.md` first — it has the concrete field names, server-side conditionals, and migration DDL already drafted. Recommended build order in the doc: #16 → #6 → #2 → #5 (ascending risk, #16 prioritized first as actively-writing-bad-data).

**2026-07-22 addendum — finding #16, jobid=0 reaching the server:** separate ticket (Beth Manners, tenant `85-1539867819`, timesheets rows 80/81/82/85/86) found `jobid=0 AND opportunityid=0` timesheets reaching prod — a standard 9-5 Basic-type shift, entered the night before, ~weekly, with neither job nor opportunity ever recorded. Root cause confirmed: **no server-side check anywhere** (`api/api-timesheets-update.php`, `TimesheetManager::save()`) requires jobid/opportunityid to be non-zero — client sends whatever, server writes it. A client-side gate exists (`validateTimeEntryForm()` at `timesheets.nomin.js:2582`) but is undermined by a confirmed logic bug in `resetJobItems()` (`timesheets.nomin.js:3644`) that mishandles the string `"0"` via loose `==` equality, plus `mobaddTimesheet()` (line 1263) resetting the job field without dispatching `change` on new entries. Not yet reproduced live in a browser — diagnosis is from static code read + prod data pattern. Confirmed NOT covered by the same-day `b2e35d3` "AI timesheet system fixes" commit — fully open. Full writeup is section 5/7 of the design doc.