sales-trainer/docs/engineering-log/2026-08-24-training-security-review.md

2026-08-24 — Training flow and security review checkpoint

Goal

Move the regular-user journey to:

Training → สินค้าของฉัน → เลือกสินค้า → ดู Personas → เริ่มฝึก

while preserving the admin flow, restricting demo users, enforcing tenant/owner boundaries, allowing safe retry of draft/failed products, and preventing persona/credential leakage.

Remediation recorded

• Regular users create products through their own route; demo users are blocked in both frontend navigation and backend authorization.
• Personas links are shown to regular users only for ready products.
• Owners retain visibility of their private draft/failed products; admins can retry analysis through the edit flow.
• Cross-tenant group IDs are indistinguishable from missing IDs (404), and API responses use is_owned rather than exposing owner_user_id/creator_id.
• Protected routes require both completed setup (must_setup is False) and explicit Terms consent (accepted_terms is True); missing markers fail closed.
• Session/debrief/persona responses use closed allowlists; provider/storage errors are mapped to stable client-safe messages.
• Owner markers and private visibility now form one fail-closed invariant; normal create/update paths reject unowned-private and malformed-owner records, while legacy malformed records are excluded from access and indexes.
• Non-ready groups remain visible to their owner/admin for recovery, but their persona payloads/counts and direct persona access are empty/blocked until ready, including admin responses.
• Owner products stuck in analyzing remain reopenable and the Training and admin edit UIs expose the recovery action instead of dead-ending.
• Judge-only evaluator reasoning and provider paths are excluded from the persona role-play prompt; only bounded role-play state is retained.
• Transcript serialization keeps seller/customer messages plus an exact allowlist of server-generated ⏳ scene notes, redacts legacy initial openers, and transcript-only debriefing receives seller messages only.
• finishSetup() remains setup-required unless the API returns both must_setup: false and accepted_terms: true.
• JWT lifetimes now use a positive 1–720 hour allowlist and token issuance rejects invalid values before encoding; decode also rejects non-positive exp.
• Group visibility now uses one exact allowlist; malformed/non-string/falsey persisted values are rejected at listing and ID-addressed authorization.

Current verification

Check	Result
cd backend && ./.venv/bin/python -m pytest -q	457 passed
cd frontend && npm run test:unit	18 passed
cd frontend && npm run build	passed
cd frontend && npm run test:e2e	15 passed across desktop, 320×568, and 500×768
cd backend && python3 -m compileall -q app tests	passed
git diff --check	passed
Added-line static security scan	0 shell-injection, eval/exec, pickle, or formatted-SQL matches; hardcoded-secret pattern matched test-only fixture literals only, with 0 production-file matches

The Playwright run emitted Vite proxy connection-refused messages for /api/auth/oauth/config because no backend was running for the fixture server. The tests still passed; this checkpoint does not claim live-provider or production verification.

Independent review gate

deleg_6017c670 was dispatched against the current tree with four bounded scopes. All four reviewers timed out after roughly ten minutes and returned no complete five-key JSON payload. This is no verdict, not a failed review and not approval. Fresh batch deleg_ea020089 then targeted the post-hardening tree, but its auth, tenant/group, and frontend scopes timed out at 600 seconds and its integration task was interrupted. It returned no complete five-key verdict and is also no verdict, not approval. Replacement integration review deleg_513b1aa3 also timed out at 600 seconds with no complete five-key payload; it is no verdict, not approval. The gate remains closed until replacement scopes return complete, schema-valid current-tree verdicts with passed: true: Fresh batch deleg_5c88269e also targeted the post-hardening tree, but its delegation owner exited before recording a terminal result. It is unknown/no-verdict and cannot affect the gate. Bounded batch deleg_fc7b3f38 returned one valid negative auth verdict whose findings were remediated, one valid positive tenant verdict, and no usable frontend/integration verdicts, so it did not approve the batch. Latest batch deleg_73452135 timed out in all four scopes after 600 seconds with no complete five-key payload; it is no verdict, not approval. Replacement batch deleg_191e6862 completed with one valid negative auth verdict, one valid positive frontend verdict, one timeout, and one valid negative lifecycle verdict; its findings were remediated and it did not approve the tree. Fresh batch deleg_71b85ab4 completed with one valid frontend pass and three timeouts (auth, backend visibility/privacy, and lifecycle); the timeouts are no verdicts, so this batch did not approve the tree. Replacement batch deleg_b4bdbacb completed with four valid negative current-tree verdicts; all cited findings were remediated locally and re-verified by focused/full automated tests. Earlier batch deleg_40657086 produced one valid positive auth verdict, but its group, chat/privacy, and lifecycle scopes were interrupted without complete five-key payloads; it is incomplete/no-approval. Interrupted batch deleg_735d9a1f produced no complete five-key payload in any scope, so it is no verdict, not approval and is superseded by the current batch. Fresh batch deleg_4c6a7118 is now running against the exact current tree with eight bounded scopes and an exact five-key schema. It includes a regression gate for the corrected super-admin CSV privacy bypass and malformed active-session recovery. No stage, commit, or push is allowed until every required scope returns a complete passed: true verdict with empty blocking arrays.

Older batch deleg_3fdefb96 completed before the latest hardening and is superseded; its findings are not approval or a current-tree verdict. Batch deleg_b25d8482 likewise completed from a pre-hardening snapshot and is superseded; only a fresh current-tree verdict can affect this gate. Batch deleg_f0a2aa91 also predates the latest hardening and is superseded; its findings do not close or replace the current-tree gate. Batch deleg_08c54d5a was also dispatched before the latest hardening. Its auth task was interrupted with no verdict; its tenant and frontend tasks are superseded and cannot affect the current-tree gate. Batch deleg_cd09ba23 exited before recording a terminal result. It is an unknown/no-verdict batch, predates the current hardening, and cannot affect the current-tree gate. Batch deleg_abeccba2 was dispatched before the latest hardening; all three tasks were interrupted without complete JSON verdicts and cannot affect the current-tree gate. Batch deleg_bfac6f8b exited before recording a terminal result. It is an unknown/no-verdict pre-hardening batch and cannot affect the current-tree gate. Batch deleg_5f3f325f exited before recording a terminal result. It is an unknown/no-verdict pre-hardening batch and cannot affect the current-tree gate.

{
  "passed": true,
  "security_concerns": [],
  "logic_errors": [],
  "suggestions": [],
  "summary": "..."
}

Git and safety state

• Branch: main
• HEAD: 8a632b5e6a3b67c9acd2787e1a686ac17ae69486
• Index: clean; no staged paths
• Existing user WIP remains untouched
• No secrets were read, stored, staged, committed, pushed, or deployed

Next action

Validate exact-current re-check deleg_0456c47d. If both exact five-key verdicts pass with empty blocking arrays, dispatch the remaining six scopes as three queued waves of at most two heavy reviewers. If either fails, stop the queue and repeat test-first remediation only for that scope. Commit/push/deploy still require explicit operator direction.

2026-08-25 Wave 2 Chat/session and auth remediation

Independent batch deleg_ead5db71 returned two complete negative verdicts. The Chat/session reviewer reproduced three current-tree failures: a second pre-seller customer opener leaked through public serialization, malformed prompt-facing Persona fields could be published as ready, and a corrupt same-scope session status allowed creation of a second session. The auth reviewer reproduced repeatable public super-admin promotion after deleting all users, unbounded forged JWT lifetimes, trivially repeated production secrets, unstable OAuth subject/email identity, implicit OAuth tenant creation, and malformed consent state accepted by /auth/me.

The remediation was test-first. backend/tests/test_wave2_review_regressions.py failed all 12 new regressions before production changes and then passed all 12. The current implementation now:

• redacts every customer message before the first seller turn;
• validates bounded, finite, structurally canonical prompt-facing Persona data;
• rejects malformed same-scope session mode/status/outcome before resume/create;
• keeps public registration permanently at role user; factory bootstrap is the only super-admin initialization path;
• bounds decoded JWT lifetime to configured JWT_EXPIRES_HOURS and requires positive finite iat/exp;
• rejects trivially repeated production JWT/bootstrap secrets;
• persists atomic provider+subject OAuth bindings, rejects email-only auto-link, and requires a pre-provisioned active default organization; and
• rejects inconsistent consent/setup state and malformed consent timestamps.

Compatibility fixes narrowed Persona validation to bounded recursive JSON while retaining strict scalar revealable fields, updated stale tests that encoded the retired email auto-link/first-user promotion behavior, and kept malformed state fail-closed. Current automated evidence is 496 backend tests passed in 71.80s, compileall passed, git diff --check passed, and the added-line secret/dangerous- execution scan returned 0 findings. No live OAuth provider verification is claimed.

Re-review deleg_6155669d returned two complete negative verdicts. Chat/session still silently normalized malformed explicit tier and recontact; auth still accepted duplicate same-user OAuth bindings, low-period repeated secrets, and a non-null malformed consent timestamp while Terms were unaccepted. Six additional RED assertions reproduced all four classes. The remediation validates explicit Persona behavior fields before defaults/coercion, rejects every repeated OAuth identity pair, detects exact repeated secret periods, and enforces a canonical unaccepted-consent timestamp of null/absent.

Re-review deleg_1e21e60c approved the auth/setup/consent/JWT/OAuth/config scope with an exact five-key pass after 226 focused and 502 full tests. It found one remaining Chat-only alias-validation defect: malformed explicit intent_tier was skipped when a valid tier coexisted. A dedicated RED test reproduced the acceptance; the guard now validates every explicitly present alias before normalization.

Exact-current evidence after that narrow patch: focused Wave-2 file 19 passed in 1.39s, full backend 503 passed in 71.89s, compileall and git diff --check passed, and the added-line scan returned 0 findings. Incremental current-tree Chat re-review deleg_8c71eff4 returned an exact five-key pass with empty blocking arrays. Its live probes confirmed malformed explicit intent_tier fails closed alone and beside a valid tier, while canonical nested Persona data remains accepted. The reviewer independently ran 136/136 bounded Chat/session/privacy tests, 19/19 Wave-2 regressions, four targeted tests, compileall, and diff checks. At that historical checkpoint, Wave 2 was closed and the overall gate was 4/8; Wave 3 batch deleg_1733a8bf then reviewed analytics/privacy and frontend with exactly two read-only reviewers while Wave 4 remained queued. The final state is recorded in the Wave 3–4 completion section below.

Wave 1 current-tree review and remediation

deleg_4d48caf2 returned two schema-valid negative verdicts rather than timing out. Ownership review proved that super_admin could cross the owner/private boundary in group listings, ID-addressed reads, report/Persona reads, and chat. Lifecycle review proved three independent gaps: privileged malformed-ready reads, unvalidated status/transitions, and stale admin variant publication after source replacement during generation.

The fixes were test-driven:

• Added backend/tests/test_review_wave1_regressions.py; its initial run failed.
• Excluded owner-private records from super-admin listing and authorization in both group and chat route policy implementations.
• Added GROUP_STATUSES and explicit transition validation to GroupStore.
• Validated structurally ready artifacts before any role receives a persisted ready group.
• Revalidated the source persona identity under the record lock before appending an admin variant.
• Updated stale compatibility tests that previously treated malformed-ready records as readable.

Verification after compatibility remediation:

Check	Result
focused Wave 1 + related suites	125 passed
backend full suite	471 passed in 68.53s
frontend unit	20 passed in 6 files
frontend production build	passed; 1775 modules transformed
Playwright desktop + 320×568 + 500×768	15 passed in 3.6s
compileall / git diff --check	passed / passed
added-line security scan	0 findings in checked secret, shell, SQL, and debug patterns

Remediation re-check deleg_8d258a4f was dispatched with exactly two heavy reviewers. The six untouched scopes remain queued. No stage, commit, push, deploy, reset, stash, credential, or permission action was performed.

2026-08-25 second lifecycle remediation

deleg_8d258a4f returned two complete five-key verdicts. Ownership/tenant isolation passed with empty blocking arrays. Lifecycle/races/readiness/variants failed on five confirmed classes: malformed-ready privileged indexes, publish_analysis() transition bypass, duplicate Persona IDs, stale same-ID admin variant publication, and missing trainee source revalidation.

The second remediation was test-first. Six new assertions failed before the patch. The service now excludes malformed lifecycle records from role indexes, publishes analysis only from analyzing, and rejects duplicate Persona IDs. Both admin and trainee variant paths now re-authorize and compare the complete source Persona, sales kit, and safe input snapshot under the source-group lock before publication. A stale board expectation was updated to match the canonical fail-closed contract.

Current evidence:

Check	Result
focused lifecycle/remediation set	69 passed in 10.90s
backend full suite	477 passed in 69.17s
compileall (app tests) / git diff --check	passed / passed
added-line static scan	0 production findings; 3 password matches are test fixtures

Because the patch touched the shared visibility boundary, both Wave 1 scopes were re-dispatched against the exact current tree as deleg_0456c47d, with two read-only reviewers and no overlapping review wave. The other six scopes remain queued. No stage, commit, push, deploy, reset, stash, credential, or permission action was performed.

2026-08-25 third Wave 1 remediation

deleg_0456c47d returned two complete five-key negative verdicts. Ownership failed because role-first authorization denied a private product to its own user after promotion to admin or super-admin. Lifecycle failed because the variant snapshot omitted visibility and had no monotonic signal for a structurally identical reanalysis.

Five RED regression instances reproduced the exact gaps. The current policy now recognizes a valid same-tenant owner before applying privileged non-owner denial, while malformed owner records retain their prior fail-closed 403/404 contracts. Privileged-owner listing uses the closed owner envelope (is_owned) rather than exposing owner_user_id. Every successful analysis publication increments an analysis_revision; variant generation snapshots and revalidates both that revision and visibility under the source lock for admin and trainee paths.

Current evidence:

Check	Result
RED Wave 1 regression file	5 expected failures; 17 passed
focused lifecycle/ownership/race set	74 passed in 12.12s
malformed-owner contract + Wave 1 set	24 passed in 3.55s
backend full suite	482 passed in 70.14s
compileall (app tests) / git diff --check	passed / passed
added-line static scan	0 findings

Fresh exact-current re-review deleg_a5931b0d is pending with exactly two read-only reviewers. The six untouched scopes remain queued; no overlapping wave was launched. No stage, commit, push, deploy, reset, stash, credential, or permission action was performed.

2026-08-25 promoted-owner response and frontend remediation

deleg_a5931b0d returned two complete five-key verdicts. Lifecycle/races passed with empty blocking arrays. Ownership failed because promoted owners received is_owned=false from detail/analyze serialization, their private products were hidden by Training.vue, and the router denied an admin owner from Personas.

The ownership gaps were reproduced before the patch: backend Wave 1 had 2 expected failures, and the frontend Training/router slice had 3 expected failures. Serialization now derives ownership from the canonical valid owner-private predicate rather than current role. Training renders a privileged owner's own-product section when it contains products, and the router admits a non-demo canonical owner before applying privileged shared-product policy.

Current-tree evidence after remediation:

Check	Result
backend promoted-owner/Wave 1 regressions	22 passed in 3.17s
frontend Training + router regressions	17 passed in 2 files
backend full suite	482 passed in 70.11s
frontend full unit suite	24 passed in 6 files
frontend production build	passed; 1775 modules transformed
isolated Sales Trainer E2E	15 passed in 3.8s; desktop, 320×568, 500×768
compileall / git diff --check / added-line scan	passed / passed / 0 findings

The initial E2E attempt reused port 3000 and tested an unrelated CrowdSight Vite server; the served title proved the collision. No product change was made for that infrastructure failure. An isolated Sales Trainer server on port 3001 passed all 15 cases and was then stopped. Fresh ownership-only reviewer deleg_457c2d44 is pending with one read-only agent. Lifecycle remains approved; the remaining six scopes stay queued. No stage, commit, push, deploy, reset, stash, credential, or permission action was performed.

2026-08-25 cross-tenant promoted-owner remediation

Fresh ownership reviewer deleg_457c2d44 reproduced a blocking stale-record path: a promoted super-admin could list, read, and start chat against a private record in another tenant when that record reused the actor's user ID. The prior owner predicate validated identity and private visibility but allowed the super-admin role to bypass tenant equality.

Two parameterized regressions covered a different valid tenant and an invalid empty tenant. Both failed before production changes. A single canonical helper now requires a valid matching actor/group tenant together with owner ID and private visibility. List serialization, group authorization, is_owned, and chat authorization use that policy. Cross-tenant super-admin access remains available only for ownerless shared records.

Check	Result
RED stale/invalid-tenant owner regressions	2 expected failures
canonical + stale-tenant focused slice	4 passed in 1.43s
ownership/security focused set	125 passed in 11.52s
backend full suite	484 passed in 70.52s
compileall / git diff --check / added-line scan	passed / passed / 0 findings

The attempted focused command naming nonexistent test_training_flow.py ran zero tests and is not evidence; it was replaced by the verified existing-file command above. deleg_457c2d44 is superseded by this code patch. Fresh ownership-only reviewer deleg_85d91881 is pending with one read-only agent; the six remaining scopes are still queued. No stage, commit, push, deploy, reset, stash, credential, or permission action was performed.

2026-08-25 Wave 3–4 completion — eight-scope code gate passed

Wave 3 initially returned two complete negative verdicts in deleg_1733a8bf. The analytics scope found unbounded personal/admin store scans and a missing session-user-tenant consistency check. The frontend scope found duplicate analysis actions while status was analyzing and a completed regular-user chat link that routed through / instead of /my/board. All four findings were reproduced and remediated with focused regressions, bounded shared scan accounting, tenant-safe joins, disabled in-progress actions, and role-aware result navigation.

Fresh Wave 3 batch deleg_1fed2524 approved the frontend scope but found one remaining analytics export leak: arbitrary nested persisted values were passed to str() and emitted into CSV. A RED sentinel regression reproduced leakage from persona_name, scenario, created_at, and a malformed username. _csv_cell now exports only bounded strings and finite integer/float scalars; booleans, composites, non-finite numbers, and unsupported values become empty cells.

Post-fix evidence:

Check	Result
nested-value CSV regression	1 passed in 0.63s
focused analytics/export/privacy slice	39 passed, 146 deselected in 7.88s
backend full suite	509 passed in 73.27s
frontend full unit suite	26 passed
frontend production build	passed; 1775 modules transformed
isolated Playwright desktop + 320×568 + 500×768	15 passed
compileall / git diff --check / source security scan	passed / passed / 0 findings

Analytics re-review deleg_bcb1510f returned an exact five-key pass with empty security and logic arrays. It independently confirmed scalar-only CSV handling, shared scan budgets, and tenant-consistent session/user/group joins. Wave 3 then closed at 6/8 approved scopes.

Final Wave 4 batch deleg_97eb385c used exactly two concurrent read-only agents:

• Scope 7/8 frontend-backend API contracts: passed, empty blocking arrays; reviewer verification included 26/26 frontend tests and 81/81 focused backend contract/security tests.
• Scope 8/8 bounded cross-cutting security/correctness: passed, empty blocking arrays; backend, frontend, build, compileall, diff check, and isolated three-viewport E2E all passed.

The exact-current local code/security gate is therefore 8/8 independently approved. Non-blocking follow-ups are to add direct _csv_cell parameterization, expand malformed frontend-envelope tests, and prevent Playwright from reusing an unrelated server on port 3000. These do not reopen the gate. No live OAuth-provider verification, production runtime check, stage, commit, push, deploy, reset, stash, credential, or permission action is claimed. Branch remains main; HEAD remains 8a632b5e6a3b67c9acd2787e1a686ac17ae69486; the index is empty and the broad existing working tree remains intentionally dirty.

2026-08-25 legacy-visibility migrate-on-read remediation

After the eight-scope security gate passed and the work was pushed, the operator reported that pre-existing training data appeared to have disappeared from the UI while the underlying records were still present. Root cause was a backward-incompatible visibility schema introduction: records written before the visibility field existed carry none, and the new fail-closed authorization treated a missing visibility as invalid, so every legacy record became unlistable and unreadable.

The old schema encoded sharing entirely through owner_user_id (owner present => own private group; absent => shared public group). The fix is a migrate-on-read helper resolved_visibility(group) that derives the effective visibility for such legacy records only (missing visibility): owner present => private, absent => public. Records with an explicit but malformed visibility stay fail-closed (None); demo/hidden are never derived from legacy records. No persisted data is rewritten.

The resolved visibility is now used at every authorization/list boundary that previously blocked legacy records — GroupStore.list_visible_to (user, admin, super-admin shared branches), group_routes._authorize_group, group_routes._get_owned_group, group_routes.list_groups serialization, chat_routes._get_ready_group (non-demo), and analytics_routes._is_shared_group — while demo and hidden-preview paths keep using the raw visibility so legacy records are never promoted to demo/hidden.

Evidence after remediation:

Check	Result
legacy/resolved_visibility regression suite	13 passed
blocker test file	55 passed in 10.10s
backend full suite	517 passed in 80.97s
frontend unit suite	26 passed (6 files)
frontend production build	passed
compileall / git diff --check	passed / passed

Returning to migration-on-read behaves purely at runtime; no data migration, backfill, or permission change was performed. Stage/commit/push requires operator direction as before.