REVIVAL-01: Gate registration by deployment mode - #1334
Conversation
Self-review findingsI reviewed the complete diff against #1297, including the auth boundary, migration/bootstrap behavior, container packaging, deployment defaults, tests, and all current PR comments/reviews (none existed when this review began). MEDIUM — fixed: container invite command could change SQLite file ownershipThe initially documented container command ran the packaged CLI as root. On SQLite deployments that can leave WAL/journal sidecars owned by root, preventing the non-root API process from reopening the database. Fix evidence: commit Verification: the packaged CLI minted an invite successfully as UID 10001 in No other actionable findings were identified in this self-review. The PR remains blocked from merge pending required CI, a fresh independent adversarial review, and maintainer approval for the auth-policy convention/migration. The full-suite failures listed in the PR body also remain explicitly unresolved and are not being treated as green. |
There was a problem hiding this comment.
Code Review
This pull request introduces registration gating to Taskdeck, allowing operators to configure the registration mode as Open, InviteOnly, or Closed. It adds a RegistrationPolicyService to authorize new users, supports atomic first-user bootstrapping, and implements CLI commands to generate one-time registration invites. The feedback highlights opportunities to optimize performance and resource utilization by moving CPU-intensive password hashing operations outside of active database transactions in AuthenticationService. Additionally, it is recommended to cache the claimed state of the first-user bootstrap in-memory within RegistrationPolicyService to avoid redundant database write attempts on subsequent registrations.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
Out-of-scope full-suite failures are now durably tracked rather than dismissed:
The exact tests passing narrowly does not make either full run green. This PR remains never-merge and still requires a fresh independent FULL-tier review, final CI, and maintainer approval. |
Chris0Jeky
left a comment
There was a problem hiding this comment.
Fresh FULL-tier adversarial review — changes requested
I reviewed #1297's acceptance criteria, the complete 49-file diff at 37253d45, auth/controller/service/SQLite transaction boundaries, migration upgrade/fresh-install behavior, CLI and container packaging, deployment guidance, targeted/full-suite evidence, and every current top-level/review/bot comment.
CRITICAL
No critical finding.
HIGH
-
Restrictive modes expose an account-enumeration oracle.
AuthenticationService.RegisterAsyncchecksUsers.ExistsAsyncatbackend/src/Taskdeck.Application/Services/AuthenticationService.cs:101-103before the registration policy at lines 105-115. After bootstrap, a caller to a Closed instance (or InviteOnly without a valid invite) gets409plus “account ... already exists” for a registered username/email but the stable403for an unused identity. That defeats the generic login contract and lets an unauthenticated remote caller enumerate accounts. Expected fix: authorize the restrictive policy before returning uniqueness information, while keeping invite consumption and the uniqueness check in the same transaction so a duplicate attempt rolls the invite back; add API regressions proving known and unknown identities receive the same denial without a valid authorization. -
The documented “safe public-deployment default” permits remote first-user seizure.
RegistrationPolicyService.AuthorizeNewUserAsyncunconditionally authorizes whichever request first inserts the bootstrap singleton (RegistrationPolicyService.cs:38-42), including a remote request. The Railway/Render guidance then exposes a fresh instance at a public URL and labelsClosedsafe (deploy/railway.toml:8-15,CLOUD_DEPLOYMENT_GUIDE.md:77-110). A scanner that reaches the service before the operator can become the only user; Closed then locks the legitimate operator out. The literal no-lockout AC is satisfied, but the resulting public-bootstrap ceremony is not safe. Expected fix: require a bootstrap secret/local operator action, or remove the public-safety claim and require private access controls until the owner account is created. Cover the authorized-vs-unauthorized bootstrap path. -
BCrypt work is performed while a SQLite write transaction is open. The registration policy executes a write before
BCrypt.HashPasswordatAuthenticationService.cs:118; the new-external-user path also hashes at line 230 inside its transaction. That lengthens the single-writer lock and can turn registration/OAuth concurrency into database-wide timeouts. Gemini's two HIGH threads (3567833413,3567833416) are valid and this finding adopts them. Expected fix: redesign the short transaction so expensive hashing occurs outside the write lock without weakening atomic bootstrap/invite consumption, and add a contention/rollback regression.
MEDIUM
- The explicit FULL-tier integration coverage is incomplete.
RegistrationGatingApiTestsintegrates password registration modes/bootstrap and only sequential invite reuse. The external-login gate exists only as a mocked application test, and no integration test proves a new OAuth/OIDC identity is denied while an existing linked identity still signs in against the real policy/store. Likewise, single-use invite redemption is not raced, so the atomic SQL guarantee is unproved at the API boundary. Expected fix: add real-store integration coverage for new-vs-existing external identities in restrictive modes and concurrent redemption of one invite (exactly one success, one stable403, no orphan user/link).
LOW
No low finding.
Existing comments and bot triage
- The self-review's container ownership finding is fixed by
37253d45; the non-root command evidence is adequate. - The out-of-scope full-suite failures are durably tracked in #1282 and #1335; they are not treated as green.
- Gemini's two BCrypt threads are valid and covered by HIGH #3.
- Gemini's proposed process-global static
_bootstrapClaimedcache (3567833418,3567833422) is invalid. One process can host multiple database paths/test factories, a database can be restored/replaced, and separate API/CLI processes share SQLite. A static flag would let one database's state suppress bootstrap checks for another and would make the process cache—not the transactionally persisted singleton—the authority. Do not implement that suggestion; optimize only with database-scoped, rollback-safe state if measurement justifies it. - The current frontend cannot submit
inviteCode, but #1301 explicitly owns and sequences that UI work after #1297; treat it as a merge/deployment dependency rather than silently claiming this PR is independently user-complete.
Verification
- Fresh reviewer run: application auth/registration/external/OIDC/account-linking filter 90/90 passed.
- Fresh reviewer run: registration-gating + migration API filter 14/14 passed.
- Fresh reviewer run: invite CLI filter 4/4 passed.
- GitHub checks at submission: all completed jobs green; Windows API Integration still pending.
- The PR body correctly records two non-green full backend runs. This review does not approve merge; the PR remains maintainer-only and changes are requested.
FULL-tier review fix evidence — final head
|
|
Final CI update for head
Therefore the required suite is green, but the PR is not described as fully green under the zero-failure policy and remains maintainer-only / never merge tonight. |
Fresh FULL-tier review result for PR #1334 (
|
Fresh FULL-tier adversarial re-review — changes requestedI independently reviewed issue #1297, the complete 49-file diff at exact head CRITICALNo critical finding. HIGH
MEDIUM
LOW
Existing comments, links, and CI
This review does not approve or merge the PR. It remains draft, maintainer-only, and changes are requested for all findings above. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6156022bb6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
FULL-tier re-review fix evidence — final head
|
|
@codex review — fresh independent FULL-tier pass requested on exact final head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 594f7de1de
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
FULL-tier connector fix evidence — final head
|
|
@codex review — fresh independent FULL-tier pass requested on exact final head |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Required E2E failure investigationThe failed Evidence gathered:
PR #1334 changes registration/auth only, so no frontend race fix is being smuggled into this never-merge auth PR. I am rerunning the failed GitHub job now after the focused investigation. #1334 remains draft and independently blocked by #1330 and maintainer-owned auth/migration review. |
Required E2E rerun resultThe investigated failed job reran green at the unchanged auth head
The original failure remains documented rather than erased: exact local Playwright passed 6/6 and the underlying stale-summary ordering seam is tracked in #1343 with deterministic reproduction/repair criteria. PR #1334 remains draft, never-merge tonight because it changes auth policy/migrations, and independently red in CI Extended under #1330. |
…on-gating # Conflicts: # backend/src/Taskdeck.Infrastructure/DependencyInjection.cs # backend/src/Taskdeck.Infrastructure/Migrations/TaskdeckDbContextModelSnapshot.cs # backend/src/Taskdeck.Infrastructure/Persistence/TaskdeckDbContext.cs # docs/platform/CLOUD_DEPLOYMENT_GUIDE.md
Do not merge tonight
This PR changes authentication policy conventions and adds a migration. Per the overnight authority, it requires maintainer review and is not eligible for autonomous merge.
Summary
Auth:Registration:ModewithOpen(application default),InviteOnly, andClosed.Owneronly to the transaction that wins the persisted first-user bootstrap claim; later accounts remain server-sanitizedEditor.taskdeck invite create --expires N.POST /api/userscreation bypass in restrictive modes;/api/auth/registeris the single policy-enforced creation surface there.--expireswithout minting an invite.Assumption: v0.1 invite minting is local-operator CLI-only, centralized behind
IRegistrationPolicyService. Reason: #1297 explicitly permits CLI-only and this is smaller than a new remote admin endpoint. Reversible by adding an authenticated admin transport over the same service.Verification
Passed on final local commit
5655f36b:node scripts/check-docs-governance.mjsgit diff --checkdocker compose ... config --quietwith required test secretsThe first full run of the final review batch exposed an order-dependent API test fixture that assumed the first account was Editor. The fixture was made bootstrap-aware before commit; the focused 3/3 regression and final full suite are green. Two earlier unrelated full-suite failure families remain tracked in #1282 and #1335 and are not dismissed.
GitHub status after this push:
5655f36b; the earlier head6156022bwas 20/20 green, including Linux/Windows API integration and E2E Smoke.Review fixes
1b2a7f13: fixes the first four fresh FULL-tier findings—account enumeration, remote bootstrap takeover, BCrypt transaction duration, and missing real-store/concurrency coverage.6156022b: updates operator, container, cloud, authentication, configuration, and data-model guidance.6699a9d9: fixes the production CLI-actor migration identity and proves a CLI-only upgrade can redeem the first-owner invite.594f7de1: adds cheap restrictive-policy eligibility before hashing, retains atomic transactional authorization, and enforces/tests invite lifetime boundaries.5655f36b: closes the authenticated/api/usersbypass, derives the firstOwnerfrom the transactional bootstrap winner, rejects bare--expires, and documents the REVIVAL-05: Paper onboarding - guided first-board path + Login/Register in the default theme #1301 browser dependency with an explicit API redemption path.Review notes
inviteCode; REVIVAL-05: Paper onboarding - guided first-board path + Login/Register in the default theme #1301 explicitly owns the Paper Register invite field. Until it lands, the auth/configuration docs provide the exactPOST /api/auth/registerredemption path. This PR does not claim browser completeness.docs/STATUS.md,docs/IMPLEMENTATION_MASTERPLAN.md, and.codex/memories/00_ACTIVE.mdwere intentionally not changed: their direction sections are superseded and land through docs: revival pivot (ADR-0044) - open-beta direction, REVIVAL_PLAN spine, canonical doc sync #1296/docs: generalist expansion wave (ADR-0046) - REVIVAL_PLAN Phase 4, artefact intake + dossiers + generalist reach, twin-app deferral #1328; canonical batch docs remain coordinator-owned.Checklist
Closes #1297