Skip to content

FINDINGS.md

Latest commit

 

History

History
182 lines (113 loc) · 9.47 KB

FINDINGS.md

File metadata and controls

182 lines (113 loc) · 9.47 KB

Production Hardening — Findings Report

Date: April 20, 2026
Commit: b8445224
Deployment: Apps Script @38

1. Architecture Summary

Two-layer system:

Layer Purpose
lib/*.jsx + Dashboard.html Static browser prototype / local development
appsscript/*.gs + appsscript/Index.html Production Apps Script web app (Google Workspace)
tests/*.mjs Node.js unit tests (no framework)

Client–server boundary: all data calls go through lib/api.jsx (API adapter), which wraps google.script.run in production or uses an in-memory store locally. AI calls go through lib/ai.jsx, which calls ai_* Apps Script shims or falls back to a pre-authored deterministic response library.

2. High-Risk Defects Found

CRITICAL — actorEmail_() effective-user fallback

File: appsscript/Code.gs
Risk level: Critical (privilege escalation for all anonymous visitors)

When a Google Apps Script web app is deployed as "execute as: me", Session.getEffectiveUser() returns the script owner's email, not the visiting user. The old fallback:

return Session.getEffectiveUser().getEmail() || 'anonymous@local';

meant that any anonymous public visitor (e.g. a competition judge opening the URL) was silently granted the script owner's identity. If the owner had admin role in the Users sheet, every public visitor would see the full admin panel and have write access to all records.

Fix: Remove getEffectiveUser() entirely. Return 'anonymous@public' sentinel when getActiveUser() returns empty.

HIGH — listUsers() called Auth_listUsers() with no actor

File: appsscript/Code.gs
Risk level: High (all Admin user-listing requests permanently broken)

Auth_listUsers(actor) calls Auth_requireRole_(actor, 'admin') at the top. The old shim was:

function listUsers() { return Auth_listUsers(); }

With no actor, Auth_resolveRole(undefined) returns 'student', so the role check always threw 'Forbidden: undefined has role "student"'. The admin Users screen was permanently broken for everyone — including actual admins.

Fix: function listUsers() { return Auth_listUsers(actorEmail_()); }

HIGH — Jobs_list() returned all rows with no role filtering

File: appsscript/Jobs.gs / appsscript/Code.gs
Risk level: High (any signed-in user, including students, could read all jobs)

The old listJobs() shim called Jobs_list() with no actor, and Jobs_list() returned every row from the sheet unconditionally. A student could call google.script.run.listJobs() and receive the full job dataset including other students' names, file IDs, and reviewer notes.

Fix: Jobs_list(actor) now enforces:

  • student → only rows where student_email === actor
  • teacher → only rows where class === teacher's class (from Users sheet)
  • technician / admin → all rows
  • anonymous@public → empty array

HIGH — Jobs_get() used unscoped Jobs_list()

File: appsscript/Jobs.gs
Risk level: High (same as above — single job fetch was unscoped)

Jobs_get(id) iterated over all jobs. A student knowing another student's job ID could call getJob('J-XXXX') and retrieve it.

Fix: Jobs_get(id, actor) delegates to Jobs_list(actor), so scoping is inherited.

MEDIUM — Write operations had no role gates

File: appsscript/Jobs.gs
Risk level: Medium (students or anonymous users could call privileged write functions)

Jobs_setIssues and Jobs_setNote (reviewer-only operations) had no role check. Any signed-in user could call setIssues('J-XXXX', [...]) and overwrite the reviewer's issue codes. Jobs_updateState had no check preventing students from calling updateJobState with states other than submitted.

Fix:

  • Jobs_setIssues and Jobs_setNote: Auth_requireRole_(actor, 'technician', 'admin')
  • Jobs_updateState: students may only resubmit (needs_fix → submitted) on their own job; teachers blocked; anonymous blocked

MEDIUM — Audit_read() had no role gate

File: appsscript/Audit.gs
Risk level: Medium (any signed-in user could read the full audit log)

The audit log contains actor emails, state transitions, and AI call records for all users. It should be restricted to operational roles.

Fix: Audit_read(limit, actor) calls Auth_requireRole_(actor, 'technician', 'admin').

MEDIUM — ai.jsx askSidekick() argument shape mismatch

File: lib/ai.jsx
Risk level: Medium (AskPanel AI calls were silently broken in production)

AskPanel called AI.askSidekick({ question: text, yearLevel: year, locale }) — an object. But askSidekick(question, scopedContext, locale) expected a string as the first arg. The OFFTOPIC_RE refusal test ran against [object Object], the fallback received an object as question, and the server received a malformed payload where input.question was itself a nested object.

Fix: askSidekick() now detects object input and extracts question, yearLevel, locale from it. The server call always sends { question, yearLevel, scopedContext, locale } with a clean string question.

LOW — Demo mode write guard only on client, not server

File: appsscript/Code.gs
Risk level: Low (mitigated by auth, but defense-in-depth gap)

Demo mode was enforced by the client API adapter but not by the server. In a tampered or headless request, write shims would still execute against the live sheet.

Fix: All write shims (createJob, updateJobState, setIssues, setNote, uploadFile) check isDemoMode_() on the server and return a {demo:true} stub without touching Sheets.

LOW — appendAudit() accepted writes from anonymous sessions

File: appsscript/Code.gs
Risk level: Low (append-only; no sensitive data exposed; but allows pollution)

Any public visitor could call appendAudit() and write arbitrary records to the Audit sheet.

Fix: appendAudit() now silently returns null for anonymous@public actors.

3. What Was Changed

File Change
appsscript/Code.gs Remove getEffectiveUser() fallback; add isDemoMode_() helper; fix all shims with actor args, demo guards, and role-correct delegation
appsscript/Jobs.gs Jobs_list(actor) role scoping; Jobs_get(id, actor) delegation; role gates on updateJobState, setIssues, setNote
appsscript/Audit.gs Audit_read(limit, actor) — technician/admin only
lib/ai.jsx askSidekick() accepts object input from AskPanel; correct question string extracted; OFFTOPIC_RE now tests real question string
lib/AppBar.jsx Dynamic user avatar from actor email; "Ask guidance" label; "Competition showcase — sample data" demo badge
lib/App.jsx listJobs() error fallback to DEMO_JOBS; pass actor to AppBar
lib/RoleViews.jsx AskPanel header/footer copy updated to GenAI-supported framing
tests/auth-boundaries.test.mjs 19 new unit tests (new file)

4. Remaining Limitations

  1. Mail delivery in demo mode: Mail_notifyStateChange is called inside Jobs_create and Jobs_updateState before the demo guard in the shim layer can intercept. In practice, Jobs_create is blocked by the demo guard in Code.gs, so mail is never triggered from demo sessions. This is correct, but the defense relies on the shim guard — not on a guard inside Mail.gs itself. A belt-and-suspenders guard inside Mail_notifyStateChange would be ideal.

  2. Teacher class assignment: Teachers only see their own class, as defined in the Users sheet class column. If a teacher's class field is empty or not yet set, they see an empty job list (by design — better to show nothing than to show everything). This should be communicated during setup.

  3. Audit log write: The Audit_read gate correctly blocks students from reading the audit log. However, there is no check that prevents a technician from writing arbitrary meta_json to the audit log via appendAudit. This is acceptable for an audit log (audit records are not security boundaries), but should be noted.

  4. Rate limiting on AI ask: ASK_DAILY_LIMIT = 20 is enforced in AI.gs via checkAndIncrementAskLimit_() using PropertiesService.getUserProperties(). In demo mode, AI calls currently go to the server fallback (client fallback), so the limit is not consumed. In production, the limit correctly gates per-user daily usage.

  5. Search bar in AppBar is not yet wired to real data — it's a visual placeholder. Results are empty.

5. Why This Version Is Stronger for Competition

Criterion Before After
Public access identity Inherited owner email via getEffectiveUser Returns anonymous@public sentinel — zero privilege
Student data isolation All students could read all jobs via listJobs() Server-enforced: students see only their own rows
listUsers admin function Always threw Forbidden (broken) Correctly enforces admin role on calling actor
Write surface for unauth users No server-side guard All writes guarded by demo mode + role checks
Audit log visibility Any user could read full audit log Restricted to technician/admin
AI call argument shape AskPanel calls were broken silently Fixed: object input accepted, question string extracted
Test coverage State machine only +19 auth-boundary and scoping tests, all passing
AI/help framing in copy "AI decides" adjacent Consistently "GenAI-supported guidance; teachers decide"