Harden tenant authorization and repair password-reset delivery - #272
Merged
Merged
Conversation
Summary: - Align make and pytest database paths without resetting a shared global file. Changed files: - Makefile, api/database.py, tests/conftest.py: isolate test state and preserve development database configuration. - tests/unit/test_make_test_db_path.py: verify distinct run paths and preserved non-test configuration. Validation: - make test-all passed: unit, API, CLI, and 324 integration tests. - make test-unit-fast passed after final changes: 350 passed, 21 skipped, 1 deselected. - Black, Ruff, and diff checks passed. Follow-ups: - Refs #260; PostgreSQL runtime and worker-level isolation remain.
Summary: - Replace caller-supplied billing identities with JWT admin authentication. - Remove super_admin privilege bypass and scope invoice/payment-method access. Changed files: - api/dependencies.py, api/routers/billing.py: require authenticated admin identity. - api/services/stripe_service.py: verify customer ownership before payment-method mutations. - tests/api/test_billing_authorization.py, tests/unit/test_dependencies.py: cover authorization regressions. - tests/contract/openapi.snapshot.json: reflect bearer authentication requirements. Validation: - 81 new billing tests passed; full API suite 413 passed. - make test-all passed; final fast unit rerun 350 passed. - Web and contract suites 218 passed; Black and Ruff passed. - Required mypy command reports 90 pre-existing errors, reproduced on untouched main. Follow-ups: - Refs #252, #255, #256. Checkout verification, full billing audit coverage, and membership security remain. - Keep draft pending CI and independent review; do not merge.
This was referenced Sep 9, 2026
Owner
Author
|
Final validation: GitHub CI run https://github.com/tomqwu/SignUpFlow/actions/runs/34361798550 completed successfully. Both The Codex precondition log explicitly reports |
Summary: - Restrict organization reads to members and mutations to owning admins. - Commit lifecycle mutations and actor audit records atomically. Changed files: - Organization router and audit logger: enforce tenant scope and transaction rollback. - Web settings callers: pass authenticated actors explicitly. - API, integration, and unit tests: replace public-access assumptions and cover denial cases. - CLAUDE.md and OpenAPI snapshot: document the public onboarding exception. Validation: - 350 fast unit tests passed; 21 skipped and 1 deselected. - 56 focused authorization, lifecycle, search, and settings tests passed. - 16 organization integration tests passed; 221 web and contract tests passed. - Black, Ruff, strict mypy, and diff whitespace checks passed. Follow-ups: - Full suite and GitHub CI must pass before shipping. - PostgreSQL cascade validation and independent review remain required. - Public membership onboarding hardening remains in #255. Refs #253, #252
Summary: - Attach reset email background tasks to browser responses. - Point email fallback links at the registered web reset route. - Record delivery failures while preserving generic user responses. Changed files: - web/auth.py: retain and attach FastAPI background tasks. - api/services/email_service.py: correct web link and normalize trailing slash. - api/routers/password_reset.py: observe false/exception delivery outcomes without recipient or token logging. - tests/web/test_password_reset.py: capture actual email content and exercise link redemption, single use, and safe re-request. - docs/features/password-reset.md: document current delivery contract and remaining production verification. Validation: - New tests reproduced three failures before implementation. - 23 focused API/web reset tests passed; 221 web and contract tests passed. - 350 fast unit tests passed; Black and Ruff passed. Follow-ups: - Wait for full local suite and GitHub CI; retain draft pending independent review. - Durable queuing, browser mail-sink journey, invitation links, and approved staging-provider delivery remain in #262 and #266. Refs #262, #252
tomqwu
changed the title
This was referenced Sep 9, 2026
Summary: - Replace the OpenAI Codex action with direct Ollama Cloud chat requests. - Default to glm-5.3-flash and read OLLAMA_API_KEY from GitHub secrets. - Fail the stable codex-pr-review-gate check on unavailable, incomplete, stale, or blocking review. Changed files: - .github/workflows/codex-review.yml: bounded diff-only review without executing PR code; validate JSON and publish SHA-bound feedback. - tests/unit/test_ollama_review_workflow.py: execute real workflow JavaScript against mocked APIs in 25 cases. - docs/ai-pr-review.md: configuration, rollout order, provider limits, and trusted-review caveats. - AGENTS.md, CLAUDE.md, .github/copilot-instructions.md: align provider and builder/reviewer merge rules. Validation: - 25 workflow cases passed after reproducing failures before the conversion. - Full unit suite: 376 passed, 21 skipped; web/contract: 221 passed. - Black, Ruff, actionlint, strict mypy, and staged whitespace/secret review passed. - Complete make test-all and GitHub CI must finish before declaring shippable. Follow-ups: - Configure OLLAMA_API_KEY separately and validate real cloud inference. - Require the check in GitHub protection only after the workflow lands and is observed on a PR. - No GitHub settings changes, merges, or external inference performed locally. Refs #259, #252
Summary: - Distinguish HTTP failures from pre-response network errors without exposing provider data. Changed files: - .github/workflows/codex-review.yml: report numeric HTTP status with fixed guidance. - tests/unit/test_ollama_review_workflow.py: cover nine HTTP statuses and transport failure privacy. - docs/ai-pr-review.md: document safe troubleshooting. Validation: - TDD: 10 new cases failed before implementation; all workflow cases now pass. - make test-unit-fast: 385 passed, 21 skipped, 1 deselected. - make test-all: 386 unit, 442 API, 16 CLI, 325 integration passed; 21 unit skipped. - Black, Ruff, actionlint, and staged diff checks passed. - mypy api: existing typing debt remains (835 errors in 40 untouched files). Follow-ups: - Verify the live Ollama response on GitHub Actions; keep the PR unmerged until its gates pass.
Summary: - Read native Ollama NDJSON streams instead of buffering a long reasoning response. - Allow an eight-minute request within a ten-minute job and distinguish deadline failures from HTTP rejection. Changed files: - .github/workflows/codex-review.yml: bounded UTF-8 streaming, final-frame validation, safe status logging. - tests/unit/test_ollama_review_workflow.py: streaming, truncation, size-limit, and timeout regressions. - docs/ai-pr-review.md: document response bounds and deadline behavior. Validation: - TDD: 11 failures before implementation, all 45 workflow cases pass after implementation. - make test-unit-fast: 395 passed, 21 skipped, 1 deselected. - make test-all completed after resume: 396 unit, 442 API, 16 CLI, 325 integration passed; 21 unit skipped. - Black, Ruff, actionlint, strict mypy (61 source files), staged whitespace and secret review passed. Follow-ups: - Verify live Ollama streaming and regular CI on this commit. - Preserve existing advisory API typing debt and keep PR unmerged until review and required checks pass.
AI review (Ollama: glm-5.3-flash){
"verdict": "SAFE TO MERGE",
"summary": "This PR is a net security improvement: billing routes move from spoofable person_id query auth to JWT-based get_current_admin_user, invoice lookups are scoped by org_id, Stripe payment-method mutations verify customer ownership, organization read/list/mutations are tenant-scoped with atomic lifecycle+audit commits, the undocumented super_admin bypass is removed, test runs get disposable databases, and the AI review workflow is converted to a fail-closed, credential-bearing-but-no-checkout design with stale-head protection, inert output, key redaction, and bounded provider I/O. The new real-JWT authorization suites (billing + organization) demonstrate denial before service invocation, tenant isolation, audit atomicity/rollback, and single-use password-reset redemption. No P0/P1 issues found in the diff. Residual risks are explicitly tracked by the author and limited: checkout-session ownership/completion is still unverified (#256), public unauthenticated organization creation remains as a documented onboarding exception (#255), and PostgreSQL cascade/FK behavior for org hard-delete plus audit rows is unvalidated (#260). Remaining P2/P3 items below are nonblocking: the unexercised comment-publish path (permission scope + 10-minute budget vs 480s request), a possible false 'delivery failed' log if the email service returns None, contract narrowing for non-admin members and potentially unauthenticated mobile organization reads, and minor hygiene (mktemp litter, Node test dependency, mutable action tags).",
"findings": [
{
"priority": "P2",
"path": "api/routers/billing.py",
"line": 197,
"detail": "handle_checkout_success applies checkout results for an arbitrary session_id with only admin authentication; the session-to-organization mapping/ownership is not verified in this PR (acknowledged #256). A foreign admin could supply another org's session id. Proposed fix: retrieve the Stripe session, verify session.metadata.org_id (or client_reference_id) matches the caller's org_id before applying completion; add a foreign-admin denial case to test_billing_authorization.py for /subscription/checkout-success."
},
{
"priority": "P2",
"path": "api/routers/organizations.py",
"line": 190,
"detail": "Hard-delete commits an AuditLog row (organization_id/resource_id referencing the deleted org) in the same transaction as the org and member deletes. Verified only on SQLite; PostgreSQL FK constraints, cascade behavior, and orphaned audit rows are unvalidated (#260). Proposed fix: run delete/cancel/restore + audit flow against PostgreSQL, or record audit rows with a snapshot field that survives the delete and add a migration test for FK ordering."
},
{
"priority": "P2",
"path": ".github/workflows/codex-review.yml",
"line": 160,
"detail": "The publish step calls github.rest.issues.createComment with only contents:read + pull-requests:write; PR conversation comments are issue comments and some GitHub permission configurations require issues:write. The live path has never reached publication (HTTP 401 at the provider). Proposed fix: add issues:write to job permissions, and keep the gate non-required until a successful end-to-end publish is observed (the PR already commits to this rollout order)."
},
{
"priority": "P2",
"path": "api/routers/billing.py",
"line": 25,
"detail": "Contract narrowing: /billing/subscription, /payment-methods, /history, and /invoices/{id}/pdf changed from org-member to admin-only. OpenAPI snapshot is updated, but confirm the web UI and any scripts that render these pages as volunteers handle the new 403 (UI should hide or degrade gracefully), and confirm no mobile organization-read calls exist outside billing (only billing is addressed in the description)."
},
{
"priority": "P2",
"path": "api/routers/organizations.py",
"line": 100,
"detail": "POST /organizations/ remains unauthenticated with an attacker-controlled id (rate limited to 2/hour/IP). Documented as an onboarding exception and tracked in #255, but combined with signup it still allows unbounded org/tenant creation from rotating IPs. Proposed fix: add per-IP + per-payload abuse metrics and an admin kill switch, or require a signed onboarding token until #255 lands."
},
{
"priority": "P2",
"path": "api/routers/password_reset.py",
"line": 68,
"detail": "_send_reset_email_quiet treats any falsy return from send_password_reset_email as failure. If that service returns None on success (its return contract is not visible in this diff), every successful send logs a false 'delivery failed' error. Proposed fix: confirm the service returns a real bool (and update its docstring), or make the web/API call sites check a documented result object."
},
{
"priority": "P3",
"path": "Makefile",
"line": 13,
"detail": "TEST_DB_PATH evaluates $(shell mktemp -d ...) on every make invocation, including run/dev/help/clean, leaking an empty directory per invocation that clean never removes. Proposed fix: guard the mktemp behind the same MAKECMDGOALS filter used for the exports, and have the clean recipe rmdir the parent directory."
},
{
"priority": "P3",
"path": "api/database.py",
"line": 16,
"detail": "TESTING_FORCE_MEMORY support was removed, but any remaining scripts, CI steps, or docs that still set or document it will now silently behave differently (binding to DATABASE_URL). Proposed fix: grep the repo for TESTING_FORCE_MEMORY and remove stale references so environments fail loudly instead of silently using a different database."
},
{
"priority": "P3",
"path": "tests/unit/test_ollama_review_workflow.py",
"line": 40,
"detail": "These unit tests spawn Node without a skip guard; on machines without Node >= 20 they raise FileNotFoundError and fail the whole fast unit tier. Proposed fix: add a pytest.importorskip-style Node availability check (with a clear skip message) or mark them so contributors without Node can still run make test-unit-fast."
},
{
"priority": "P3",
"path": "api/services/email_service.py",
"line": 1053,
"detail": "Reset token is embedded in the URL path (previously a query param); either form lands in server access logs, proxy logs, and Referer headers. No regression, but consider single-use tokens with short TTL (already done) plus log-scrubbing of /auth/reset/ paths, and document the residual exposure in docs/features/password-reset.md."
},
{
"priority": "P3",
"path": "api/dependencies.py",
"line": 129,
"detail": "verify_admin_access changed from (person_id: str, db: Session) to an async dependency on get_current_user. Any non-FastAPI caller using the old positional signature (person_id, db) would break silently at runtime; full mypy on api is advisory so it may not catch this. Proposed fix: grep for non-Depends call sites of verify_admin_access and update or delete them."
},
{
"priority": "P3",
"path": ".github/workflows/codex-review.yml",
"line": 150,
"detail": "480000 ms request deadline plus GitHub API pagination and comment publication leaves little headroom under timeout-minutes: 10; a slow model run can be cancelled after the comment is posted but before the verdict is processed. Fail-closed, but noisy. Proposed fix: raise timeout-minutes to 12 or lower the request deadline to 420000 ms so cancellation cannot clip the verdict processing."
},
{
"priority": "P3",
"path": ".github/workflows/codex-review.yml",
"line": 1,
"detail": "actions/github-script\u0040v7 is pinned by mutable major tag, not commit SHA. For a credential-bearing job, pin to a full SHA (and document the pin in docs/ai-pr-review.md) to reduce supply-chain risk; the workflow already correctly avoids checking out PR code."
}
]
}Diff-only advisory review; CI and GitHub mergeability remain mandatory. |
Summary: - Keep onboarding copy readable beside compact actions. - Stack full-width actions below copy on narrow screens. - Rev the stylesheet URL so existing sessions load the fix. Changed files: - web/static/css/styles.css: scope responsive onboarding action sizing. - web/templates/base.html: add the stylesheet cache revision. - tests/e2e/test_onboarding_wizard.py: cover desktop and narrow layout geometry. - tests/web/test_onboarding_wizard.py: assert the stylesheet revision contract. Validation: - Focused onboarding web and Playwright tests pass. - make test-unit-fast passes: 395 passed, 21 skipped, 1 deselected. - make test-all passes: 442 API, 16 CLI, and 325 integration tests. - Black, Ruff, mypy, and git diff checks pass. - Live onboarding page verified in the in-app browser. Follow-ups: - Confirm GitHub CI and codex-pr-review-gate on this commit.
AI review (Ollama: glm-5.3-flash){
"verdict": "SAFE TO MERGE",
"summary": "This is a genuine security-hardening increment: all billing routes now require JWT-authenticated tenant admins, the spoofable person_id query-parameter authorization and the undocumented super_admin bypass are removed, invoice reads are tenant-scoped, and Stripe payment-method detach/default operations verify customer ownership before provider mutations. Organization read/list/update/delete/cancel/restore now require authenticated same-tenant membership/admin, with lifecycle mutations and audit records committed atomically and rollback verified by tests. The password-reset web flow fix (attaching the email task to the real response and generating /auth/reset/{token} links) repairs a previously non-functional delivery path while preserving anti-enumeration responses and avoiding recipient/token logging. Test isolation changes give each make invocation a disposable database and remove the shared /tmp file contract. The Ollama review workflow is fail-closed (missing creds, oversized/missing patches, malformed streams, stale head/base all fail), validates JSON reports, forces NEEDS FIX on P0/P1 regardless of model verdict, redacts the key, renders output inert, and is covered by 45 tests that execute the real inline JS. Remaining known gaps (checkout-session ownership verification #256, public org creation #255, PostgreSQL runtime coverage #260) are explicitly documented as follow-ups and are not regressions. Findings below are P2/P3: a possible PostgreSQL FK interaction on hard-delete auditing, contract changes for member-level billing reads and existing super_admin accounts, the checkout-session ownership gap, an email return-contract assumption, and test-portability issues. None block merge given the PR's stated scope and passing CI.",
"findings": [
{
"priority": "P2",
"path": "api/routers/organizations.py",
"line": 178,
"detail": "delete_organization writes an AuditLog row (organization_id=org_id, user_id=owner) in the same transaction that hard-deletes the Organization and its Persons. If audit_logs.organization_id/user_id are ForeignKey columns without ON DELETE CASCADE/SET NULL, the commit will fail with IntegrityError on PostgreSQL (SQLite tests won't catch it since FK enforcement is off by default). Verify the AuditLog model constraints or add ON DELETE behavior before relying on hard-delete in production."
},
{
"priority": "P2",
"path": "api/routers/billing.py",
"line": 154,
"detail": "handle_checkout_success still finalizes any client-supplied Stripe checkout session_id without verifying the session belongs to the caller's org/customer (acknowledged as open #256). A foreign admin could replay another tenant's session id. Proposed fix: fetch the session from Stripe and compare its customer/metadata org id to the authenticated admin's org before applying the upgrade."
},
{
"priority": "P2",
"path": "api/routers/billing.py",
"line": 29,
"detail": "GET /subscription, /payment-methods, /history, and /invoices/{id}/pdf changed from org-member to admin-only. This is an intentional tightening but is a breaking API contract for any existing volunteer-facing client that read these endpoints; verify no web/mobile consumer depends on member-level reads before release, or add a documented migration note."
},
{
"priority": "P2",
"path": "api/dependencies.py",
"line": 16,
"detail": "Removing super_admin from check_admin_permission silently downgrades any existing accounts carrying that role (seed data, bootstrap admins, production users). Confirm the seed/bootstrap path grants 'admin' and communicate a role migration; otherwise previously-working super_admin logins will start receiving 403 with no diagnostics (the debug prints were removed too)."
},
{
"priority": "P3",
"path": "api/routers/password_reset.py",
"line": 77,
"detail": "\u0060sent = email_service.send_password_reset_email(...)\u0060 assumes a boolean return. If the service returns None on success, every successful send will log a spurious 'delivery failed' error. Confirm send_password_reset_email's return contract or treat None/falsy-undefined distinctly from an explicit False."
},
{
"priority": "P3",
"path": "api/routers/organizations.py",
"line": 180,
"detail": "delete_organization reuses AuditAction.BULK_DELETE for organization deletion, which is semantically misleading in audit reports. Propose adding a dedicated ORG_DELETED action so audit consumers can distinguish org deletion from bulk data operations."
},
{
"priority": "P3",
"path": "tests/unit/test_ollama_review_workflow.py",
"line": 40,
"detail": "The unit suite now hard-depends on a \u0060node\u0060 binary (subprocess.run(['node',...], check=True)); make test-unit-fast fails with FileNotFoundError on machines without Node. Consider a pytest.importorskip/shutil.which('node') skip marker or move these to a dedicated marker so core unit tests stay portable."
},
{
"priority": "P3",
"path": "api/routers/organizations.py",
"line": 90,
"detail": "POST /organizations/ remains public (documented onboarding exception, rate limited 2/hour/IP). Before release, consider requiring authentication or stronger abuse controls since anyone can pre-claim org ids and consume subscription rows; tracked by #255 but worth surfacing at review time."
}
]
}Diff-only advisory review; CI and GitHub mergeability remain mandatory. |
Summary: - Drive onboarding from the dashboard banner through its visible step actions. - Verify completed setup removes the unfinished dashboard banner. Changed files: - tests/e2e/test_onboarding_wizard.py: replace direct route jumps with user-facing navigation assertions. Validation: - Focused onboarding, smoke-loop, and authentication tests pass: 6 tests. - Full browser suite passes: 29 tests. - make test-all passes: 396 unit, 442 API, 16 CLI, and 325 integration tests. - Black, Ruff, strict mypy, whitespace, and staged secret review pass. Follow-ups: - Confirm GitHub CI and codex-pr-review-gate on this commit.
AI review (Ollama: glm-5.3-flash){
"verdict": "SAFE TO MERGE",
"summary": "This multi-increment PR makes genuine security improvements with no P0/P1 issues found in the diff. Billing routes now require JWT-authenticated tenant admins (removing the spoofable person_id query-parameter authorization), the undocumented super_admin bypass is removed, invoice reads are scoped by org_id, and Stripe detach/set-default verify the payment method belongs to the org's customer before mutation. Organization read/list/update/delete/cancel/restore are now authentication- and membership-scoped, with lifecycle mutations and audit records committed atomically (rollback verified by tests). The web forgot-password flow fixes a real delivery bug (throwaway BackgroundTasks meant email was never sent) by attaching tasks to the TemplateResponse, and reset links now target the served /auth/reset/{token} route; failed deliveries are logged without recipient/token leakage. The Ollama workflow conversion is fail-closed: bounded diff (400KB), stream/size/deadline bounds, strict JSON validation, P0/P1 override of SAFE verdicts, key redaction and inert rendering, head/base SHA checks before and after publication, no checkout or execution of PR code, and comprehensive negative-path tests exercising the real inline JavaScript. Test isolation changes give each make/pytest invocation a disposable database while preserving DATABASE_URL for non-test targets. Remaining gaps (attachment/checkout-session ownership, PostgreSQL runtime cascade behavior, durable email delivery, branch-protection rollout) are explicitly acknowledged and tracked in #256/#262/#253/#259 and are pre-existing rather than regressions introduced here. Findings below are P2 verification items and P3 hygiene items only; none block merge.",
"findings": [
{
"priority": "P2",
"path": "api/routers/organizations.py",
"line": 186,
"detail": "delete_organization hard-deletes the Organization and inserts an AuditLog row referencing organization_id=org_id in the same transaction. If AuditLog.organization_id has a ForeignKey to organizations.id with ondelete restrict (and PostgreSQL enforces it, unlike the SQLite test DB), the DELETE will raise an IntegrityError at commit and hard delete will fail in production. The PR itself defers PostgreSQL runtime validation. Fix: verify the schema; if the FK exists, make it deferrable/ondelete=SET NULL, or drop the FK on audit organization_id and keep the dangling reference intentionally."
},
{
"priority": "P2",
"path": "api/services/stripe_service.py",
"line": 679,
"detail": "Ownership verification was added for detach and set-default, but attach_payment_method (POST /billing/payment-methods) and handle_checkout_success still accept a payment-method/session id without verifying it belongs to the caller's Stripe customer. An admin who learns another org's pm_* id could attach it to their own customer and charge another tenant's card. This is acknowledged as remaining in #256, but since the identical check (retrieve then compare 'customer' to the org's subscription.stripe_customer_id) is now implemented twice in this file, applying it to attach in this same change would be cheap and would close the last cross-tenant financial mutation."
},
{
"priority": "P3",
"path": "Makefile",
"line": 14,
"detail": "TEST_DB_PATH := $(shell mktemp -d /tmp/signupflow-tests.XXXXXX)/signupflow_test.db is evaluated at Makefile parse time for every make invocation, including non-test targets (run, dev, help), leaving an orphan temp directory each time. Fix: only run mktemp inside the test-goal conditional (defer with = instead of := and evaluate under the ifneq), or add these dirs to the clean target's sweep."
},
{
"priority": "P3",
"path": "tests/conftest.py",
"line": 16,
"detail": "tempfile.mkdtemp(prefix=\"signupflow-tests-\") creates a per-run test database directory that is never removed, accumulating directories in /tmp across runs (same class of leak the Makefile change fixes for shared state). Fix: register an atexit/shutil.rmtree cleanup or reuse a single session-scoped temp dir removed at session end."
},
{
"priority": "P3",
"path": "api/routers/password_reset.py",
"line": 81,
"detail": "The except branch now uses logger.error without exc_info, so the exception traceback (root cause of delivery failure) is lost even though recipient/token were deliberately removed from the message. Fix: use logger.exception(\"Password-reset email delivery failed; a fresh request may be retried\") — the stack trace contains no recipient data, preserving the privacy goal while restoring diagnosability."
},
{
"priority": "P3",
"path": "api/dependencies.py",
"line": 16,
"detail": "check_admin_permission no longer honors super_admin. This is the intended #255 change, but any production or seed account whose only role is super_admin will silently lose all admin access on deploy (403s on billing/organization admin routes). Fix: before release, audit seed scripts and production data for super_admin-only accounts and migrate them to admin, or document the migration step in the release notes."
},
{
"priority": "P3",
"path": ".github/workflows/codex-review.yml",
"line": 157,
"detail": "If the PR head changes between comment publication and the final assertCurrent, a SAFE TO MERGE comment remains on the PR while the gate check fails. The comment prints the (now stale) head SHA so it is traceable, but it can mislead readers. Fix: on stale detection after publication, either update the comment with a stale banner (requires an additional API call storing the comment id) or delete it before failing."
},
{
"priority": "P3",
"path": "api/routers/organizations.py",
"line": 173,
"detail": "update_organization and delete_organization call _commit_organization_change without a Request, so their audit records omit ip_address/user_agent that cancel/restore capture. For consistent actor evidence on all lifecycle mutations, pass the http_request argument (add Request parameter to those handlers) or document why update/delete intentionally omit request context."
}
]
}Diff-only advisory review; CI and GitHub mergeability remain mandatory. |
Summary: - Execute browser invitation email tasks and generate working browser acceptance links. - Count saved assignments so the dashboard works after generating a schedule. - Open the latest tenant-owned solution from the onboarding Publish action. Changed files: - API analytics and email service: repair assignment count and invitation URL. - Web page and partial handlers: preserve background tasks and resume publication. - Web and browser tests: cover email acceptance, dashboard data, and resumed onboarding. Validation: - Full backend suite passed; final fast unit rerun passed (395 tests). - Full browser suite and focused web tests passed (47 tests). - Final invitation, onboarding, dashboard, and API email checks passed (21 tests). - Black, Ruff, strict mypy subset, and staged diff review passed. - Full API mypy retains 835 existing errors. Follow-ups: - Validate GitHub CI and Ollama review on this commit. - Live provider delivery remains a staging check.
Owner
Author
|
Review fixes in 9f94d56:
Validation:
Live provider delivery remains a staging check. PR remains draft and unmerged. |
AI review (Ollama: glm-5.3-flash){
"verdict": "SAFE TO MERGE",
"summary": "This is a genuine security-hardening increment: billing routes now require JWT-authenticated admins instead of the spoofable person_id query parameter, organization reads/mutations are tenant-scoped with atomic audit commits, invoice lookups are scoped by org, Stripe payment-method mutations verify customer ownership, the super_admin bypass is removed, and test databases are made disposable per run. The bundled Ollama review workflow is fail-closed (no checkout of PR code, validated JSON, head/base staleness checks, inert rendering, key redaction) and is advisory only. The 81 billing and 29 organization regression tests cover the critical denial paths (anonymous, invalid, volunteer, foreign-admin, super_admin), and the shown tests plus reported CI give reasonable confidence that unchanged handler bodies retain org-membership verification. No P0/P1 issues were introduced by the diff. Remaining concerns are nonblocking: the checkout-success endpoint still lacks session-ownership verification (explicitly tracked in #256), the super_admin removal has no migration path for existing accounts, public organization creation/membership gaps remain tracked in #255, and minor operational issues (temp-dir leakage, provider error text in client responses, Postgres test coverage deferred per #260) exist. All are documented by the author as follow-ups; none blocks this partial increment.",
"findings": [
{
"priority": "P2",
"path": "api/routers/billing.py",
"line": 152,
"detail": "POST /billing/subscription/checkout-success accepts an arbitrary Stripe session_id and (per the PR's own #256 follow-up) does not verify the checkout session belongs to the caller's org/customer. An authenticated admin could redeem another org's checkout session. Fix before #256 closes: fetch the session, verify customer/org metadata matches the caller's org before applying any state change, and add a foreign-session regression test alongside the existing auth-denial cases."
},
{
"priority": "P2",
"path": "api/dependencies.py",
"line": 15,
"detail": "check_admin_permission now returns True only for the 'admin' role. Any existing account whose roles contain only 'super_admin' silently loses all administrative access with no migration or backfill in this diff. Intentional hardening, but deploy risk: add a one-time role migration/backfill (or document that operators must update roles) before this ships to an environment with super_admin users."
},
{
"priority": "P3",
"path": "api/routers/organizations.py",
"line": 87,
"detail": "POST /organizations/ remains public by design (documented empty-onboarding exception, rate-limited 2/h/IP). Combined with the acknowledged public-membership hardening gap (#255), tenant isolation added in this PR can be undermined if signup can join arbitrary org IDs. Confirm #255 covers restricting membership joins to the just-created empty org."
},
{
"priority": "P3",
"path": "Makefile",
"line": 13,
"detail": "TEST_DB_PATH uses $(shell mktemp -d ...) which is evaluated at parse time for every make invocation, including non-test goals (run, help, clean), creating a never-cleaned temp directory each run; test-run DB files also accumulate. Fix: gate the mktemp behind the same MAKECMDGOALS filter used for the exports, or add cleanup to the test recipes."
},
{
"priority": "P3",
"path": "api/services/stripe_service.py",
"line": 715,
"detail": "The generic except handlers return f\"...: {str(e)}\" which can surface raw Stripe SDK/provider error text to clients via the 400 detail (e.g., remove_payment_method). Log the detail server-side and return a fixed client-facing message to avoid leaking provider internals."
},
{
"priority": "P3",
"path": ".github/workflows/codex-review.yml",
"line": 100,
"detail": "PR title/body/patches are attacker-controlled model input; a crafted patch could attempt prompt injection to elicit a 'SAFE TO MERGE' verdict with no findings. Output validation, P0/P1 override, and advisory-only status mitigate this, and docs acknowledge fallibility. Consider additionally stripping or flagging PR description content, and never letting this check substitute for human review on security-sensitive PRs."
},
{
"priority": "P3",
"path": "tests/conftest.py",
"line": 19,
"detail": "conftest unconditionally overwrites DATABASE_URL with the SQLite test URL, so pytest runs always bind SQLite even when DATABASE_URL points at PostgreSQL; Postgres runtime/concurrency test coverage remains deferred (#260). Also tempfile.mkdtemp dirs leak when pytest is invoked outside make. Acceptable now; track the Postgres lane explicitly."
},
{
"priority": "P3",
"path": "api/routers/organizations.py",
"line": 183,
"detail": "delete_organization's audit record omits request IP/user-agent (no Request parameter passed), unlike cancel/restore/update. Pass the request through _commit_organization_change for consistent actor evidence on destructive operations."
}
]
}Diff-only advisory review; CI and GitHub mergeability remain mandatory. |
Summary: - Add reusable six-week church and basketball acceptance playbooks and tests. - Preserve assigned roles, count actual slots, and reject overlapping assignments. - Hide unpublished schedules from member lists and self-service actions. Changed files: - docs/playbooks and README.md: operational scenarios and recorded evidence. - api/core, api/routers, api/services, web/routers: roster and publication fixes. - tests: domain journeys, regression coverage, and updated API contract. Validation: - make test-all: 1184 passed, 21 skipped across backend tiers. - Web: 225 passed; browser: 33 passed; contract: 1 passed. - Black and Ruff pass; strict mypy scope passes. - Full API mypy retains 835 pre-existing errors in 40 files. Follow-ups: - Keep documented delivery, tenant security, custom constraints and venue gaps visible. - Leave PR unmerged pending CI and independent review.
Owner
Author
|
Summary:
Changed files:
Validation for cccc6f7:
Follow-ups:
|
AI review (Ollama: glm-5.3-flash){
"verdict": "SAFE TO MERGE",
"summary": "Diff-only advisory review, not a merge approval. The security intent is coherent and well-covered by real-JWT regressions: (1) billing routes now require JWT admin identity and the spoofable person_id query authorization is removed; invoice reads are scoped by BillingHistory.org_id and Stripe payment-method detach/default now verify customer ownership before provider mutation. (2) Organization reads/lists are membership-scoped, lifecycle mutations require same-tenant admin, and audit records commit atomically with the mutation (rollback verified by tests). (3) Password-reset browser links now point at the served /auth/reset/{token} route and background email tasks attach to the real response, fixing silent non-delivery; failure logging drops recipient/token data. (4) Test databases are per-invocation temp dirs with DATABASE_URL preserved for non-test targets. (5) The Ollama workflow keeps credentials inline, never checks out or executes PR code, validates patches/JSON/head-base freshness, fails closed on every error path, and renders model output inert. Remaining risks are known, pre-existing, and explicitly tracked (#255 public org creation/admin signup, #256 checkout-session ownership), so they are nonblocking P2/P3 items below. I could not verify repo-wide internal callers of get_organization or the changed detach_payment_method signature from the diff alone; passing API/integration suites reduce but do not eliminate that risk — flagged as P3 verification items.",
"findings": [
{
"priority": "P2",
"path": "api/routers/billing.py",
"line": 154,
"detail": "POST /billing/subscription/checkout-success now requires an authenticated admin, but the session_id query parameter is still trusted without verifying the Stripe checkout session belongs to the caller's tenant (org attribution appears to come from session metadata). A foreign admin could complete another org's checkout session. Acknowledged as open #256; propose verifying session.metadata.org_id/client_reference_id == current_user.org_id and payment_status before applying the upgrade."
},
{
"priority": "P2",
"path": "api/routers/organizations.py",
"line": 96,
"detail": "POST /organizations/ remains public and, combined with /auth/signup, lets anyone become admin of a fresh organization (the playbook itself relies on this). It is the documented empty-onboarding exception tracked in #255, but until invitation-only membership lands, ensure the create_org rate limit holds under distributed IPs and that signup cannot join pre-existing orgs."
},
{
"priority": "P3",
"path": "api/dependencies.py",
"line": 16,
"detail": "check_admin_permission no longer honors super_admin. Intentional privilege-bypass removal, but any existing accounts/seed data holding only super_admin lose admin access on deploy. Provide a migration or documented rename to admin before rollout."
},
{
"priority": "P3",
"path": "api/routers/organizations.py",
"line": 170,
"detail": "delete_organization records AuditAction.BULK_DELETE with resource_type=organization. A dedicated ORG_DELETED action would make audit semantics unambiguous and avoid consumers misreading org deletion as bulk data deletion."
},
{
"priority": "P3",
"path": "api/services/stripe_service.py",
"line": 682,
"detail": "detach_payment_method signature changed to (org_id, payment_method_id). Diff-only review cannot confirm all call sites were updated; any remaining old-signature caller (e.g., webhook or cancel flow) would now hit the 'No Stripe customer found' branch instead of detaching. Grep the repo for detach_payment_method callers before merge."
},
{
"priority": "P3",
"path": "api/routers/organizations.py",
"line": 40,
"detail": "get_organization now requires an explicit authenticated Person; diff-only review cannot confirm every internal caller (beyond the updated web/routers/pages.py and partials.py) passes current_user. A stale internal caller would pass the Depends sentinel object and fail with a 500. Grep for get_organization( call sites."
},
{
"priority": "P3",
"path": "api/routers/password_reset.py",
"line": 71,
"detail": "_send_reset_email_quiet treats a falsy return as delivery failure. If send_password_reset_email returns None on success rather than True, every successful send logs a spurious failure and the retry contract becomes confusing. Confirm the service's documented bool return, or normalize (sent is not False)."
},
{
"priority": "P3",
"path": "Makefile",
"line": 12,
"detail": "TEST_DB_PATH uses $(shell mktemp -d ...) with immediate assignment, so an empty temp dir is created on every make invocation, including run/dev/clean, littering /tmp. Consider a deferred assignment (or compute lazily only under the test-goal branch) and optionally clean the directory in the test recipes."
}
]
}Diff-only advisory review; CI and GitHub mergeability remain mandatory. |
Summary: - Discover validated domain definitions through a shared pytest plugin. - Reuse six-week API and browser workflows without hard-coded domains. Changed files: - tests/playbooks: registry, plugin, reusable runtime and workflow, external example. - tests/unit/test_playbook_plugin.py: discovery, validation and collection regressions. - tests/api, tests/e2e, tests/conftest.py: integrate the shared fixture. - docs/playbooks: versioned definitions, extension guide and validation evidence. Validation: - Full backend, web, contract and browser suites: 1464 passed, 21 skipped. - External third-domain proof: 3 API and 6 browser cases passed. - Black, Ruff, strict mypy scope passed; full mypy retains existing debt. - Reviewed staged diff for whitespace and secrets; only test auth references present. Follow-ups: - Preserve documented production acceptance gaps; no production-readiness claim.
Owner
Author
|
Implemented pluggable pytest playbooks; final commit 82b633b. Summary:
Validation:
Failure and correction:
Follow-ups:
|
AI review (Ollama: glm-5.3-flash){
"verdict": "SAFE TO MERGE",
"summary": "Diff-only review of a large multi-increment PR: tenant authorization hardening (billing, organizations), Stripe payment-method ownership checks, organization lifecycle audit atomicity, password-reset delivery fixes, solver role/overlap fixes plus a publication-visibility boundary, per-run test DB isolation, an Ollama Cloud fail-closed review workflow, and a pytest playbook plugin. No P0/P1 defects found in the changed code. The core security changes are sound: legacy query-parameter identity authorization is replaced with JWT-based admin auth; billing reads/writes require get_current_admin_user with membership checks asserted by new real-JWT regression tests (foreign admins denied before any provider call, services asserted un-called); invoice lookups are org-scoped; detach/set-default verify PaymentMethod.customer against the org's Stripe customer before mutation; organization reads are membership-scoped and lifecycle mutations commit with their audit records and roll back on failure; member schedules hide unpublished solver assignments while manual assignments stay visible; the review workflow never checks out or executes PR code, validates head/base freshness, bounds input/stream/output, fails closed on every error path, forces NEEDS FIX on P0/P1 findings, and renders model output inert with key redaction. Intentional contract changes (organizations GET/list now require auth with 403 instead of 404 for unknown tenants; billing endpoints admin-only; new optional role field on solution assignees) are consistently reflected in the OpenAPI snapshot and updated tests; the PR itself documents the mobile impact as nil and keeps public org creation as a documented, rate-limited onboarding exception. Residual risks are pre-existing or explicitly deferred and tracked: payment-method attach and checkout-session completion verification (issue #256), unauthenticated solution-read routes, and PostgreSQL FK/cascade behavior for the new delete-plus-audit path. The attached findings are nonblocking follow-ups to complete before production rollout.",
"findings": [
{
"priority": "P2",
"path": "api/services/stripe_service.py",
"line": 640,
"detail": "attach_payment_method (and handle_checkout_success) still lack customer-ownership/completion verification, so an org admin who learns another tenant's pm_ ID may be able to move that PaymentMethod to their own customer (Stripe re-attachment detaches it from the prior customer) and bill with it. The PR scopes this to #256; complete the attach-side ownership check before enabling live Stripe billing."
},
{
"priority": "P2",
"path": "api/routers/organizations.py",
"line": 176,
"detail": "delete_organization hard-deletes the Organization and inserts its BULK_DELETE audit record in one transaction. If AuditLog.organization_id carries a Postgres FK with RESTRICT the delete returns 500, and with CASCADE the deletion audit evidence is destroyed; SQLite tests do not enforce FKs so both fail silently. Confirm the audit table has no enforcing FK (or use SET NULL) during the deferred PostgreSQL cascade validation."
},
{
"priority": "P2",
"path": "api/routers/solutions.py",
"line": 196,
"detail": "GET /solutions/{id}/assignments (and related solution-read routes) still have no authentication dependency while this diff adds the new role field to that public response. Pre-existing and acknowledged, but since this file is touched, add member/admin scoping here or confirm #256/#191 tracks it."
},
{
"priority": "P3",
"path": "Makefile",
"line": 11,
"detail": "TEST_DB_PATH uses $(shell mktemp -d ...) which executes at parse time for every make invocation, including non-test targets (make run, make migrate), leaking an empty directory per invocation; recipes only rm the db file. Create the directory lazily in test recipes or clean it in the clean target."
},
{
"priority": "P3",
"path": "api/routers/password_reset.py",
"line": 87,
"detail": "The exception branch now logs only a fixed message, dropping the exception type/traceback that the previous .exception() call captured, which will make SMTP delivery failures hard to diagnose. Log the exception class name (no recipient, token, or provider body) or keep exception logging behind a scrubbing formatter."
},
{
"priority": "P3",
"path": "tests/conftest.py",
"line": 17,
"detail": "DATABASE_URL is now unconditionally overwritten with SIGNUPFLOW_TEST_DATABASE_URL (previously setdefault), so running pytest with an operator-provided DATABASE_URL (e.g., a Postgres service) silently targets disposable SQLite. Document the override or gate it on an explicit env flag."
},
{
"priority": "P3",
"path": "api/routers/organizations.py",
"line": 168,
"detail": "update_organization and delete_organization audit without request IP/user-agent (cancel/restore capture both) and update records no changed-field details. Pass the Request through _commit_organization_change and include a diff of updated fields for consistent audit evidence."
},
{
"priority": "P3",
"path": "api/dependencies.py",
"line": 16,
"detail": "Removing super_admin from check_admin_permission is a breaking change for any existing super_admin principal in production data (intentional per #255). Confirm no seeded/deployed accounts rely on the role and update seed scripts/docs so legitimate operators are not locked out."
},
{
"priority": "P3",
"path": "api/routers/billing.py",
"line": 545,
"detail": "get_subscription, get_payment_methods, and get_billing_history now require admin where members previously could read; any volunteer-facing page or integration calling these will break. The OpenAPI snapshot is updated, but verify web/mobile clients do not call them for non-admins before deploy."
}
]
}Diff-only advisory review; CI and GitHub mergeability remain mandatory. |
Summary: - Collect the API runner in plugin unit probes; leave browser execution in its own tier. Changed files: - tests/unit/test_playbook_plugin.py: remove Playwright dependency from backend unit collection. - docs/playbooks/validation.md: record the hosted failure and correction. Validation: - Plugin tests: 21 passed. - Full backend rerun: 1205 passed, 21 skipped. - Black, Ruff and diff whitespace checks passed. Follow-ups: - Revalidate the corrected commit in existing CI and AI review jobs.
AI review (Ollama: glm-5.3-flash){
"verdict": "SAFE TO MERGE",
"summary": "This multi-increment PR materially improves tenant isolation and fails closed on most new failure paths. Billing routes now require JWT-authenticated admins, the spoofable person_id query-parameter authorization and the undocumented super_admin bypass are removed, invoice lookups are org-scoped, and Stripe payment-method mutations verify customer ownership before detaching/modifying (verified by mocked-provider regressions including the foreign-customer denial). Organization reads/lists are membership-scoped, lifecycle mutations commit with their audit records atomically, and denied requests leave state unchanged. Member-facing schedules correctly hide unpublished solver assignments across API and web paths. Password-reset delivery fixes are real: browser tasks are attached to the response, links target /auth/reset/{token}, and delivery failures are observed and retryable with token rotation. Test databases are per-invocation disposable, closing the shared /tmp collision. The Ollama workflow is a sound diff-only reviewer: no PR code execution, HTTPS/redirect/userinfo endpoint validation, strict JSON report validation, P0/P1 forcing NEEDS FIX, head/base freshness checks before and after publication, credential redaction and inert rendering, and fail-closed behavior on missing patches, truncation, timeouts, and malformed streams; 45 tests execute the real inline JavaScript. Remaining known gaps (checkout-session ownership under #256, public org creation under #255, Postgres coverage under #260) are pre-existing, explicitly deferred, and tracked, so they are nonblocking for this incremental PR. Findings below are P2/P3 hardening suggestions.",
"findings": [
{
"priority": "P2",
"path": "api/routers/billing.py",
"line": 198,
"detail": "handle_checkout_success authenticates the admin but (per the PR's own #256 follow-up) still does not verify the Stripe checkout session belongs to the caller's organization/customer before applying the upgrade; a caller-supplied session_id from another tenant could drive subscription state. Pre-existing and tracked, but the next increment should verify session.customer/metadata.org matches admin.org_id before mutation."
},
{
"priority": "P2",
"path": "api/routers/organizations.py",
"line": 92,
"detail": "POST /organizations/ remains public (documented onboarding exception, #255). Combined with knowledge of an org id, anonymous callers can still probe id existence via 409 vs 201. Acceptable for this increment given rate limiting and tracking, but the follow-up should move creation behind signup/invitation flow."
},
{
"priority": "P2",
"path": ".github/workflows/codex-review.yml",
"line": 152,
"detail": "The report validator permits up to ~155k characters (3000 summary + 50 findings x 3000 detail), exceeding GitHub's ~65k comment body limit; createComment would fail and the valid review is discarded (fail-closed but the verdict is lost). Cap total rendered report length (e.g., truncate findings or enforce aggregate size) before publishing."
},
{
"priority": "P2",
"path": "api/dependencies.py",
"line": 16,
"detail": "Removing super_admin from check_admin_permission is the intended #255 fix, but any existing account whose only role is super_admin silently loses all admin access (billing, org lifecycle, invitations). Verify no production/seeded accounts rely on it before release, or migrate such roles to admin."
},
{
"priority": "P3",
"path": ".github/workflows/codex-review.yml",
"line": 36,
"detail": "OLLAMA_ENDPOINT is a repo variable validated only as HTTPS + /api/chat path; any actor who can edit repo variables can point it at their own host and capture the bearer key and PR data. Consider restricting to an approved-host allowlist or documenting that variable write access is equivalent to secret access."
},
{
"priority": "P3",
"path": ".github/workflows/codex-review.yml",
"line": 55,
"detail": "assertCurrent compares pulls.get().base.sha (live base branch head) to the event's base.sha, so any push to the base branch during the review window marks a valid review stale and fails the job. Consider comparing the merge-base commit instead to avoid spurious staleness failures."
},
{
"priority": "P3",
"path": "Makefile",
"line": 14,
"detail": "TEST_DB_PATH := $(shell mktemp -d ...) is evaluated on every make invocation (including run/dev), creating stray /tmp/signupflow-tests.* directories unrelated to testing. Use a deferred assignment or gate mktemp behind the same MAKECMDGOALS filter as the exports."
},
{
"priority": "P3",
"path": "tests/conftest.py",
"line": 17,
"detail": "conftest unconditionally overwrites DATABASE_URL with SIGNUPFLOW_TEST_DATABASE_URL, so future Postgres-targeted test runs (#260) would be silently redirected to SQLite. Consider honoring an explicit escape hatch (e.g., SIGNUPFLOW_TEST_DATABASE_URL=postgres://) with a warning, or documenting the override."
},
{
"priority": "P3",
"path": "api/routers/organizations.py",
"line": 183,
"detail": "delete_organization records AuditAction.BULK_DELETE for a single-organization hard delete; the audit trail action is misleading. Add a dedicated org.deleted action (or include the reason in details) so lifecycle audits remain interpretable."
},
{
"priority": "P3",
"path": "api/routers/password_reset.py",
"line": 80,
"detail": "The failure handler now logs a fixed message and drops the exception stack (previously .exception), reducing diagnosability of SMTP/provider failures while keeping PII out. Consider logging exception type/class (never message) to retain debuggability without leaking recipient data."
}
]
}Diff-only advisory review; CI and GitHub mergeability remain mandatory. |
tomqwu
marked this pull request as ready for review
September 12, 2026 18:40
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary:
Changed files:
Validation (initial billing/test-isolation increment):
Follow-ups:
Organization and Password-Reset Increment
Summary:
Changed files:
Validation on c736236:
Follow-ups:
Ollama Cloud Conversion (785c894)
Summary:
Changed files:
Validation on 785c894:
Follow-ups:
Ollama HTTP Diagnostics (4530724)
Summary:
Validation:
Follow-ups:
Successful Ollama Streaming Review (3816f7c)
Summary:
Changed files:
Validation:
Follow-ups:
Responsive Onboarding Layout (8beb32c)
Summary:
Changed files:
Validation on 8beb32c:
Follow-ups:
Visible Workflow Coverage (299bce2)
Summary:
Validation on 299bce2:
Follow-ups: