Skip to content

Codex review smoke test - #1

Open
lyc-aon wants to merge 1 commit into
mainfrom
codex-review-smoke-20260505
Open

Codex review smoke test#1
lyc-aon wants to merge 1 commit into
mainfrom
codex-review-smoke-20260505

Conversation

@lyc-aon

@lyc-aon lyc-aon commented May 6, 2026

Copy link
Copy Markdown
Owner

Temporary PR to verify the Lycaon code-review intake and Codex GitHub review flow.

Do not merge. Please review this as if the helper were production-bound so we can inspect the exact Codex review output shape.

The diff is intentionally small and isolated to make the resulting comments easy to evaluate.

@lyc-aon

lyc-aon commented May 6, 2026

Copy link
Copy Markdown
Owner Author

@codex review for serious issues, security regressions, bug/regression risk, tests/ci

Context:
Smoke test for the Lycaon public intake -> admin queue -> manual @codex review pipeline. Review the isolated customer session helper as if it were production-bound.

Known risk areas: auth role checks, redirect handling, DOM rendering, session lookup

Instructions:
Review this PR for serious security or regression issues. The PR is a temporary smoke test, but evaluate the added helper as production-bound. Focus on auth, redirects, DOM rendering, and session lookup behavior. Leave concise findings with file/line references when possible.

@lyc-aon

lyc-aon commented May 6, 2026

Copy link
Copy Markdown
Owner Author

@codex review

Retry after enabling Codex code review for this repository. Please review only the new file spa/src/lib/review-smoke/customer-session.ts for serious security or regression issues. Focus on auth role checks, redirects, DOM rendering, and session lookup. This is a temporary smoke-test PR for understanding Codex review output.

@lyc-aon

lyc-aon commented May 6, 2026

Copy link
Copy Markdown
Owner Author

AI-assisted Codex review

Generated from the public diff for #1, requested by @lyc-aon. Please verify findings before merging.

Findings

  • P0 spa/src/lib/review-smoke/customer-session.ts:19 canAccessAdminTools() uses assignment, not comparison: session.role = 'admin'. Any non-null session is mutated into an admin session and the function returns a truthy value, so this is a straight auth bypass.
  • P1 spa/src/lib/review-smoke/customer-session.ts:32 renderSessionBadge() renders session.email through innerHTML. If the remembered session ever contains attacker-controlled markup, this becomes DOM XSS in the authenticated page context.
  • P1 spa/src/lib/review-smoke/customer-session.ts:10-11 and spa/src/lib/review-smoke/customer-session.ts:35-36 key the map inconsistently: sessions are stored under the raw email but looked up under email.toLowerCase(). Any mixed-case address becomes unrecoverable, which breaks the remembered-session lookup path for real users.

Residual Risk

  • spa/src/lib/review-smoke/customer-session.ts:22-23 is a redirect trust boundary. I’m not counting it as a standalone P1 without the caller, but if redirectTo can come from return-url/user-controlled state, returning it verbatim is an open-redirect risk and should be constrained to safe internal paths before release.
  • I did not find automated coverage for this helper. Current SPA CI in .github/workflows/deploy.yml:64-70 only runs npm run check and npm run build, so the defects above would likely ship uncaught.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant