CLD-03 follow-up: DB-backed auth codes, PKCE, account linking#812
CLD-03 follow-up: DB-backed auth codes, PKCE, account linking#812Chris0Jeky merged 19 commits intomainfrom
Conversation
Replaces the in-memory ConcurrentDictionary approach with a persistent entity that tracks code, userId, token, expiry, and consumption state. Supports single-use semantics via TryConsume() with TTL enforcement. Part of #676.
Defines the application-layer contract for auth code persistence with GetByCodeAsync and DeleteExpiredAsync for TTL cleanup. Part of #676.
Registers OAuthAuthCodes table with unique index on Code and index on ExpiresAt for TTL cleanup. Wires repository through DI and UnitOfWork. Part of #676.
Creates OAuthAuthCodes table with unique Code index, ExpiresAt index for TTL cleanup, and FK cascade to Users. Part of #676.
…ccount linking - OAuthAuthCode entity with Purpose field supporting both login and link flows - DB-backed exchange with TryConsume() for single-use, TTL, replay prevention - AuthController: new IUnitOfWork dependency, async ExchangeCode, CleanupExpiredCodesAsync - Account linking: POST github/link, DELETE github/link, GET linked-accounts endpoints - AuthenticationService: CompleteAccountLinkAsync, UnlinkExternalLoginAsync - LinkedAccountDto for account linking API responses - Updated all test stubs for IUnitOfWork.OAuthAuthCodes - Rewrote OAuthTokenLifecycleTests and AuthControllerEdgeCaseTests for DB-backed store Part of #676.
Sets UsePkce = true on the GitHub OAuth handler. ASP.NET Core 8 automatically generates code_verifier and sends code_challenge in the authorization request, then includes code_verifier in the token exchange. Defense-in-depth against authorization code interception. Part of #676.
- LinkedAccount type and API methods (link, unlink, getLinkedAccounts) - ProfileSettingsView: shows linked GitHub account with avatar and display name, Link/Unlink buttons, handles oauth_link_code from redirect callback - Only shown when GitHub OAuth is configured and not in demo mode Part of #676.
- OAuthAuthCodeTests: 13 tests covering construction validation, TryConsume single-use semantics, expiry, CreateForLinking - AccountLinkingTests: 8 tests covering CompleteAccountLinkAsync and UnlinkExternalLoginAsync with conflict, not-found, and forbidden edge cases Part of #676.
- Add TryConsumeAtomicAsync for race-safe single-use code exchange using raw SQL UPDATE WHERE IsConsumed = 0 - Fix DeleteExpiredAsync to use in-memory filtering since EF Core 8 with SQLite cannot translate DateTimeOffset comparisons in LINQ - Update AuthController to use atomic consume in both exchange and link endpoints - Update tests for new atomic consume behavior Part of #676.
The settings view now uses useRouter/useRoute for OAuth link code handling. Add mocks for vue-router, authApi, and demoMode to prevent test failures. Part of #676.
…scope - Read mode from OAuth state (authenticateResult.Properties.Items) instead of trusting the query string parameter, preventing state manipulation attacks - Fix fire-and-forget cleanup that could run after scope disposal by awaiting it within the request - Remove unused LinkExternalLoginAsync from interface and service Part of #676.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a database-backed storage mechanism for OAuth authorization codes, replacing the previous in-memory ConcurrentDictionary implementation. This change enables support for multi-instance deployments and persistence across application restarts. The PR also adds functionality for account linking, allowing users to associate external providers like GitHub with their accounts, and includes necessary database migrations and repository interfaces. I have provided feedback regarding the potential for ObjectDisposedException in the fire-and-forget cleanup task, the use of parameterized queries for DateTimeOffset values, and the efficiency of the bulk deletion logic in the repository.
| await _unitOfWork.SaveChangesAsync(); | ||
|
|
||
| // Best-effort cleanup of expired codes | ||
| _ = CleanupExpiredCodesAsync(); |
There was a problem hiding this comment.
Executing CleanupExpiredCodesAsync as a fire-and-forget task (_ = ...) will result in an ObjectDisposedException. The AuthController and its injected IUnitOfWork are scoped to the request; when the request completes (which happens immediately upon returning the Redirect), the scope and the underlying DbContext are disposed. Any background task still running will fail when it attempts to access the database. Since this is a fast operation, it should be awaited directly.
| var affected = await _context.Database.ExecuteSqlRawAsync( | ||
| "UPDATE OAuthAuthCodes SET IsConsumed = 1, ConsumedAt = {0}, UpdatedAt = {1} WHERE Code = {2} AND IsConsumed = 0", | ||
| now.ToString("o"), | ||
| now.ToString("o"), | ||
| code); |
There was a problem hiding this comment.
Avoid manually stringifying DateTimeOffset values when using ExecuteSqlRawAsync. EF Core's SQLite provider is designed to handle DateTimeOffset parameters correctly by mapping them to the appropriate storage format. Passing the objects directly is more robust and ensures consistent behavior across different environments or provider configurations.
| // queries. Load all codes and filter in memory for cleanup. | ||
| // Auth codes are short-lived and few in number, so this is acceptable. | ||
| var allCodes = await _context.Set<OAuthAuthCode>() | ||
| .ToListAsync(cancellationToken); | ||
|
|
||
| var expired = allCodes.Where(e => e.ExpiresAt < cutoff).ToList(); | ||
|
|
||
| if (expired.Count == 0) | ||
| return 0; | ||
|
|
||
| _context.Set<OAuthAuthCode>().RemoveRange(expired); | ||
| await _context.SaveChangesAsync(cancellationToken); | ||
| return expired.Count; |
There was a problem hiding this comment.
Loading all authorization codes into memory to perform cleanup is inefficient and will not scale. While LINQ translation for DateTimeOffset comparisons can be problematic in SQLite, you can perform an efficient bulk deletion using a raw SQL DELETE statement. This executes entirely on the database side, avoiding the overhead of fetching records and change tracking.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Implements issue #676 by hardening the GitHub OAuth flow: persist auth codes in the DB for multi-instance/restart safety, enable PKCE on the GitHub OAuth handler, and add account linking/unlinking (backend endpoints + frontend Settings UI).
Changes:
- Replaced in-memory OAuth auth-code storage with an EF Core
OAuthAuthCodeentity + repository with atomic consume semantics. - Enabled PKCE (
UsePkce = true) for GitHub OAuth. - Added authenticated GitHub account linking/unlinking endpoints and a “Linked Accounts” section in profile settings.
Reviewed changes
Copilot reviewed 31 out of 32 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/taskdeck-web/src/views/ProfileSettingsView.vue | Adds UI/state for GitHub link/unlink and consumes oauth_link_code from query params. |
| frontend/taskdeck-web/src/api/authApi.ts | Adds API calls for linked-accounts + link/unlink GitHub. |
| frontend/taskdeck-web/src/types/auth.ts | Introduces LinkedAccount type. |
| frontend/taskdeck-web/src/tests/views/ProfileSettingsView.spec.ts | Mocks router/auth APIs for the new linked-accounts logic. |
| backend/src/Taskdeck.Api/Controllers/AuthController.cs | Adds DB-backed exchange + link/unlink/linked-accounts endpoints and link-mode callback behavior. |
| backend/src/Taskdeck.Api/Extensions/AuthenticationRegistration.cs | Enables PKCE on GitHub OAuth handler. |
| backend/src/Taskdeck.Domain/Entities/OAuthAuthCode.cs | New auth-code domain entity (login/link purposes + consume semantics). |
| backend/src/Taskdeck.Infrastructure/Repositories/OAuthAuthCodeRepository.cs | Repository for auth codes (lookup, atomic consume, cleanup). |
| backend/src/Taskdeck.Infrastructure/Persistence/TaskdeckDbContext.cs | Adds DbSet<OAuthAuthCode>. |
| backend/src/Taskdeck.Infrastructure/Persistence/Configurations/OAuthAuthCodeConfiguration.cs | EF mapping for OAuthAuthCode. |
| backend/src/Taskdeck.Infrastructure/Repositories/UnitOfWork.cs | Wires OAuthAuthCodes into UnitOfWork. |
| backend/src/Taskdeck.Infrastructure/DependencyInjection.cs | Registers IOAuthAuthCodeRepository. |
| backend/src/Taskdeck.Infrastructure/Migrations/AddOAuthAuthCodes (+ snapshot) | Adds the new table + indexes. |
| backend/src/Taskdeck.Application/Interfaces/IUnitOfWork.cs | Adds OAuthAuthCodes repository to UoW contract. |
| backend/src/Taskdeck.Application/Interfaces/IOAuthAuthCodeRepository.cs | New repository interface. |
| backend/src/Taskdeck.Application/DTOs/UserDtos.cs | Adds LinkedAccountDto. |
| backend/src/Taskdeck.Application/Services/IAuthenticationService.cs | Adds link/unlink-related methods. |
| backend/src/Taskdeck.Application/Services/AuthenticationService.cs | Implements account link/unlink behavior. |
| backend/tests/* | Adds/updates tests for the new auth-code store and account linking; updates fake UoW implementations accordingly. |
Files not reviewed (1)
- backend/src/Taskdeck.Infrastructure/Migrations/20260409181436_AddOAuthAuthCodes.Designer.cs: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| function startGitHubLink() { | ||
| const apiBase = import.meta.env.VITE_API_BASE_URL || 'http://localhost:5000/api' | ||
| const returnUrl = '/workspace/settings' | ||
| window.location.href = `${apiBase}/auth/github/login?mode=link&returnUrl=${encodeURIComponent(returnUrl)}` | ||
| } |
There was a problem hiding this comment.
startGitHubLink() hard-codes returnUrl to /workspace/settings, but the actual profile settings route is /workspace/settings/profile (there is no /workspace/settings route). This will redirect users to a non-existent client route after linking; use route.path or /workspace/settings/profile instead.
| await _unitOfWork.OAuthAuthCodes.AddAsync(authCode); | ||
| await _unitOfWork.SaveChangesAsync(); | ||
|
|
||
| // Best-effort cleanup of expired codes | ||
| _ = CleanupExpiredCodesAsync(); | ||
|
|
There was a problem hiding this comment.
_ = CleanupExpiredCodesAsync(); is fire-and-forget but uses the request-scoped _unitOfWork/DbContext. Once the request completes the scope may be disposed, causing cleanup to fail silently or produce unobserved task issues. Either await the cleanup, or move cleanup to a background service / create a new DI scope inside the cleanup routine.
| if (string.IsNullOrWhiteSpace(authCode.ProviderData)) | ||
| return BadRequest(new ApiErrorResponse(ErrorCodes.ValidationError, "Link code contains no provider data")); | ||
|
|
||
| var providerInfo = JsonSerializer.Deserialize<JsonElement>(authCode.ProviderData); | ||
| var provider = providerInfo.GetProperty("provider").GetString() ?? "GitHub"; | ||
| var providerUserId = providerInfo.GetProperty("providerUserId").GetString(); | ||
| var displayName = providerInfo.TryGetProperty("displayName", out var dn) ? dn.GetString() : null; | ||
| var avatarUrl = providerInfo.TryGetProperty("avatarUrl", out var av) ? av.GetString() : null; |
There was a problem hiding this comment.
ProviderData is parsed via JsonSerializer.Deserialize<JsonElement>() and then GetProperty(...) is used; malformed/partial JSON will throw and return a 500. Since the code comes from DB state, harden this by catching JsonException and using TryGetProperty with a 400/401 response when required fields are missing.
| public async Task<bool> TryConsumeAtomicAsync(string code, CancellationToken cancellationToken = default) | ||
| { | ||
| // Atomic UPDATE ensures only one concurrent request can consume a code. | ||
| // The WHERE clause filters on IsConsumed = 0 so the second requester gets 0 affected rows. | ||
| var now = DateTimeOffset.UtcNow; | ||
| var affected = await _context.Database.ExecuteSqlRawAsync( | ||
| "UPDATE OAuthAuthCodes SET IsConsumed = 1, ConsumedAt = {0}, UpdatedAt = {1} WHERE Code = {2} AND IsConsumed = 0", | ||
| now.ToString("o"), | ||
| now.ToString("o"), | ||
| code); | ||
|
|
||
| return affected > 0; |
There was a problem hiding this comment.
TryConsumeAtomicAsync ignores cancellationToken, passes timestamps as strings, and does not include an expiry check in the atomic UPDATE. This allows a code to be consumed after it expires if it expires between the read and the UPDATE. Prefer ExecuteSqlInterpolatedAsync(..., cancellationToken) with DateTimeOffset parameters and add AND ExpiresAt > {now} (and/or purpose) to the WHERE clause.
| public async Task<int> DeleteExpiredAsync(DateTimeOffset cutoff, CancellationToken cancellationToken = default) | ||
| { | ||
| // EF Core 8 with SQLite cannot translate DateTimeOffset comparisons in LINQ | ||
| // queries. Load all codes and filter in memory for cleanup. | ||
| // Auth codes are short-lived and few in number, so this is acceptable. | ||
| var allCodes = await _context.Set<OAuthAuthCode>() | ||
| .ToListAsync(cancellationToken); | ||
|
|
||
| var expired = allCodes.Where(e => e.ExpiresAt < cutoff).ToList(); | ||
|
|
||
| if (expired.Count == 0) | ||
| return 0; | ||
|
|
||
| _context.Set<OAuthAuthCode>().RemoveRange(expired); | ||
| await _context.SaveChangesAsync(cancellationToken); | ||
| return expired.Count; |
There was a problem hiding this comment.
DeleteExpiredAsync loads all auth codes into memory and filters client-side. If cleanup is missed, this can become a scalability/memory issue and keeps expired tokens in-process longer than necessary. Consider a DB-side delete (ExecuteDeleteAsync if translatable, or provider-conditional ExecuteSqlInterpolatedAsync DELETE for SQLite) using the cutoff parameter.
| // Return a placeholder indicating the link flow should proceed via OAuth | ||
| // The actual linking happens in CompleteAccountLinkAsync after OAuth callback | ||
| return Result.Failure<LinkedAccountDto>(ErrorCodes.ValidationError, | ||
| "Account linking requires completing the OAuth flow. Use the GitHub login redirect with mode=link."); | ||
| } |
There was a problem hiding this comment.
LinkExternalLoginAsync always returns a failure (ValidationError) even when inputs are valid. Having a public service API that is intentionally non-functional is confusing for callers and hard to test/maintain; either remove this method from the interface, or make it return a meaningful success payload (e.g., a link initiation response) and have the controller use it.
| vi.mock('vue-router', () => ({ | ||
| useRouter: () => ({ | ||
| replace: vi.fn(), | ||
| push: vi.fn(), | ||
| }), | ||
| useRoute: () => ({ | ||
| query: {}, | ||
| path: '/workspace/settings', | ||
| }), | ||
| })) |
There was a problem hiding this comment.
The mocked useRoute().path is set to /workspace/settings, but the real route for this view is /workspace/settings/profile. Using the real path in the mock would better reflect production behavior and would have caught the incorrect returnUrl used by the linking flow.
| // Store the authorization code in the database instead of in-memory. | ||
| // This survives restarts and works with multi-instance deployments. | ||
| var code = GenerateAuthCode(); | ||
| _authCodes[code] = (result.Value, DateTimeOffset.UtcNow.AddSeconds(60)); | ||
| CleanupExpiredCodes(); | ||
| var authCode = new OAuthAuthCode( | ||
| code: code, | ||
| userId: result.Value.User.Id, | ||
| token: result.Value.Token, | ||
| expiresAt: DateTimeOffset.UtcNow.AddSeconds(60)); | ||
|
|
||
| await _unitOfWork.OAuthAuthCodes.AddAsync(authCode); | ||
| await _unitOfWork.SaveChangesAsync(); |
There was a problem hiding this comment.
The DB-backed auth code currently stores the full JWT bearer token (OAuthAuthCode.Token) in the database. Even though the auth code expires quickly, this still persists a usable access token at rest and could be read if the DB is compromised or if cleanup is delayed. A safer approach is to store only UserId (and maybe minimal claims) and mint a fresh JWT during /exchange instead of persisting the token string.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1ec777f4a3
ℹ️ 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".
| var linkAuthCode = OAuthAuthCode.CreateForLinking( | ||
| code: linkCode, | ||
| providerData: providerData, | ||
| expiresAt: DateTimeOffset.UtcNow.AddSeconds(60)); |
There was a problem hiding this comment.
This link-code creation path stores only GitHub identity data and does not bind the code to the Taskdeck user/session that initiated the flow; later, LinkGitHub applies the code to whichever authenticated caller redeems it. In practice, if an oauth_link_code is leaked or phished within its lifetime, another logged-in user can redeem it to attach the attacker’s GitHub account to the victim account and then sign in as that victim via GitHub, so the code needs a caller binding that is verified during exchange.
Useful? React with 👍 / 👎.
| await _unitOfWork.SaveChangesAsync(); | ||
|
|
||
| // Best-effort cleanup of expired codes | ||
| _ = CleanupExpiredCodesAsync(); |
There was a problem hiding this comment.
This unawaited call starts async DB cleanup work on the request-scoped _unitOfWork/DbContext after the response path continues, so request-scope disposal can race and throw during cleanup; because CleanupExpiredCodesAsync swallows exceptions, failures are silent and expired OAuth codes can accumulate indefinitely. Run cleanup within the request scope (await it) or move it to a background worker that creates its own scope.
Useful? React with 👍 / 👎.
Adversarial Self-ReviewSecurity Findings[FIXED] MEDIUM: Link mode from query string instead of OAuth state [FIXED] MEDIUM: Fire-and-forget cleanup could use disposed scope [FIXED] LOW: Dead code (LinkExternalLoginAsync) LOW: Auth code replay - MITIGATED LOW: PKCE downgrade - NOT APPLICABLE LOW: Account linking CSRF - MITIGATED LOW: Timing attacks on code validation - ACCEPTABLE Migration SafetyThe migration adds a new Test Coverage
|
Follow-up: Fixes AppliedAll MEDIUM findings from the adversarial review have been fixed in commit
All 3,788 backend tests and 1,898 frontend tests pass after these changes. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5457479ee7
ℹ️ 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".
| const returnUrl = '/workspace/settings' | ||
| window.location.href = `${apiBase}/auth/github/login?mode=link&returnUrl=${encodeURIComponent(returnUrl)}` |
There was a problem hiding this comment.
startGitHubLink sends the OAuth callback to '/workspace/settings', but the router only registers settings pages under paths like '/workspace/settings/profile' (there is no '/workspace/settings' route). After GitHub callback, users are redirected to an unmatched URL with oauth_link_code, so ProfileSettingsView never mounts to process the code and account linking fails end-to-end.
Useful? React with 👍 / 👎.
Independent Adversarial Review (Round 2) -- Security-FocusedCRITICAL Findings1. [CRITICAL] Account Linking CSRF: Attacker can link their GitHub account to victim's sessionThe account-linking flow has a CSRF vulnerability. The attack:
Root cause: The link code is not bound to any particular user session. It is a bearer token that can be exchanged by anyone who holds a valid JWT. The Fix: The link code must be bound to the user who initiated the link flow. Either:
File: 2. [CRITICAL] TryConsumeAtomicAsync does NOT check expiry -- expired codes can be consumedThe UPDATE OAuthAuthCodes SET IsConsumed = 1, ConsumedAt = {0}, UpdatedAt = {1} WHERE Code = {2} AND IsConsumed = 0This query does not filter on
In practice the window is narrow (under 1 second), but for auth-critical code the atomic operation should enforce ALL invariants: WHERE Code = {2} AND IsConsumed = 0 AND ExpiresAt > {3}File: 3. [CRITICAL] JWT stored in plaintext in database -- token theft via DB accessThe This is worse than the previous in-memory store because:
Mitigation: Either (a) clear the Token field upon consumption, or (b) do not store the JWT at all -- store only the userId and re-issue a fresh JWT at exchange time, or (c) hash/encrypt the token. File: HIGH Findings4. [HIGH] DeleteExpiredAsync loads ALL auth codes into memory -- DoS vectorvar allCodes = await _context.Set<OAuthAuthCode>().ToListAsync(cancellationToken);This loads every Even without attack, this is a correctness bug -- the comment says "Auth codes are short-lived and few in number" but this is only true if cleanup actually runs regularly, and it only runs during successful OAuth callbacks (best-effort, swallowed exceptions). Fix: Use raw SQL for cleanup like DELETE FROM OAuthAuthCodes WHERE ExpiresAt < {0}File: 5. [HIGH] Link code is not bound to authenticated user -- no session bindingRelated to finding #1, but distinct: the
The code should store the intended user ID (from the JWT at initiation time) and verify it at consumption time. File: 6. [HIGH] No cleanup of consumed auth codes -- unbounded table growth
For a production system, this table will grow indefinitely if OAuth is used frequently. Fix: Also delete consumed codes in cleanup ( File: MEDIUM Findings7. [MEDIUM] ExchangeCode exposes timing side-channel for code existenceThe exchange endpoint returns different error messages depending on whether the code exists, is expired, is consumed, or is for the wrong purpose:
An attacker can use these distinct responses to enumerate valid codes and determine their state. All failure paths should return the same generic error message. File: 8. [MEDIUM] mode query parameter on callback is attacker-controllableThe callback reads var isLinkMode = mode == "link";
if (authenticateResult.Properties?.Items.TryGetValue("mode", out var stateMode) == true)
{
isLinkMode = stateMode == "link";
}While the state parameter takes precedence, the fallback to File: 9. [MEDIUM] Frontend XSS risk via displayName in success messagelinkSuccess.value = `GitHub account linked successfully (${linked.displayName || linked.providerUserId})`While Vue's File: 10. [MEDIUM] avatarUrl rendered as img src without validation<img v-if="gitHubAccount.avatarUrl" :src="gitHubAccount.avatarUrl" ... />The File: LOW Findings11. [LOW] PKCE implementation relies entirely on framework -- no verificationThe PKCE setup is just File: 12. [LOW] ExecuteSqlRawAsync parameter passing formatWhile File: 13. [LOW] Missing CancellationToken forwarding in TryConsumeAtomicAsyncThe File: INFO14. [INFO] No ADR for security-impacting architecture changeMoving from in-memory to DB-backed auth codes, adding account linking, and enabling PKCE are security-posture changes. Per CLAUDE.md: "Do not skip ADRs for decisions that affect architecture, security posture, or cross-cutting conventions." An ADR should document this decision. 15. [INFO] Migration changes ProductVersion from 8.0.14 to 8.0.25The model snapshot bump from Summary
The most urgent issues are #1 (CSRF account linking), #2 (TOCTOU on expiry), and #3 (JWT plaintext storage). Issues #1 and #5 together mean an attacker can link their GitHub account to any victim who visits a crafted URL, then log in as the victim. |
CreateForLinking now requires initiatingUserId parameter, stored in UserId field. This prevents CSRF attacks where an attacker generates a link code and tricks a victim into exchanging it, linking the attacker's GitHub to the victim's account. Addresses adversarial review finding #1 (CRITICAL).
TryConsumeAtomicAsync now includes ExpiresAt > now in the WHERE clause to close the TOCTOU race window between application-level expiry check and SQL execution. DeleteExpiredAsync now uses raw SQL instead of loading all rows into memory (DoS prevention). Also deletes consumed codes to prevent unbounded table growth. Uses EF Core SQLite DateTimeOffset format for correct string comparison. Addresses findings #2 (CRITICAL), #4 (HIGH), #6 (HIGH), #13 (LOW).
- GitHubLogin requires authentication for mode=link and stores caller userId in OAuth state for CSRF protection - GitHubCallback reads mode only from tamper-proof OAuth state, never from query string - ExchangeCode re-issues fresh JWT at exchange time instead of reading stored token from DB (no plaintext JWT in database) - LinkGitHub verifies link code was initiated by the same user who is exchanging it (CSRF protection) - All failure paths use uniform error messages to prevent timing side-channel enumeration Addresses findings #1, #3, #5, #7, #8 (CRITICAL/HIGH/MEDIUM).
ExchangeCode endpoint now re-issues JWTs at exchange time rather than reading stored tokens from the database. GenerateJwtToken must be accessible from the controller layer.
- CreateForLinking tests now pass initiatingUserId parameter - Add test for empty userId validation - Constructor test updated to verify Token is not stored
Security Fixes AppliedFollowing up on the adversarial review, the following CRITICAL and HIGH findings have been fixed in 5 commits pushed to this branch: Fixed: CRITICAL #1 + HIGH #5 -- Account Linking CSRF
Fixed: CRITICAL #2 -- TOCTOU on Expiry Check
Fixed: CRITICAL #3 -- JWT Stored in Plaintext
Fixed: HIGH #4 -- DeleteExpiredAsync DoS
Fixed: HIGH #6 -- Unbounded Table Growth
Fixed: MEDIUM #7 -- Timing Side-Channel
Fixed: MEDIUM #8 -- Mode Parameter Injection
Verification
Remaining (not fixed, lower priority)
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b99ab2fa3
ℹ️ 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".
| function startGitHubLink() { | ||
| const apiBase = import.meta.env.VITE_API_BASE_URL || 'http://localhost:5000/api' | ||
| const returnUrl = '/workspace/settings' | ||
| window.location.href = `${apiBase}/auth/github/login?mode=link&returnUrl=${encodeURIComponent(returnUrl)}` |
There was a problem hiding this comment.
startGitHubLink navigates the browser directly to /auth/github/login?mode=link, but this top-level navigation does not include the JWT that Taskdeck stores for axios requests, so AuthController.GitHubLogin fails its TryGetCurrentUserId check and returns 401 instead of starting OAuth. In practice this makes account linking fail before reaching GitHub for authenticated users unless they manually add auth another way.
Useful? React with 👍 / 👎.
| await _unitOfWork.ExternalLogins.DeleteAsync(loginToRemove); | ||
| await _unitOfWork.SaveChangesAsync(); |
There was a problem hiding this comment.
This path removes the external login unconditionally, but OAuth-created users are given a random unknown password hash and cannot set a new password without the current one, so deleting their only GitHub login can permanently lock them out of the account. Add a guard that blocks unlink when no other sign-in method exists (or require setting a password first).
Useful? React with 👍 / 👎.
…er input
Remove the `mode` query parameter from `GitHubLogin`. The link/login
branch is now determined entirely from `TryGetCurrentUserId` (JWT
authentication state): authenticated callers enter the account-linking
flow; unauthenticated callers enter the normal login flow. This
eliminates the CWE-807 / CodeQL high-severity finding ("user-controlled
bypass of sensitive method") where a user-supplied `mode=link` value
could manipulate which security branch was taken.
Add two regression tests in AuthControllerEdgeCaseTests verifying that
the flow is driven by server-side auth state regardless of any
query-string value.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 31644293f1
ℹ️ 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".
| catch (Exception) | ||
| { | ||
| return Result.Failure<LinkedAccountDto>(ErrorCodes.UnexpectedError, "Account linking failed due to an unexpected error"); |
There was a problem hiding this comment.
If two link completions race for the same (provider, providerUserId), the second insert will hit the ExternalLogins unique constraint at SaveChangesAsync(). This catch (Exception) path currently maps that expected collision to UnexpectedError, so callers get a 500 instead of a 409 conflict even though the account was simply linked first by another request/tab. Catch uniqueness violations explicitly and return ErrorCodes.Conflict so retry/double-submit races don’t surface as server faults.
Useful? React with 👍 / 👎.
…orwarding, returnUrl fix - AuthController: Wrap ProviderData JSON parsing in try-catch to return 400 instead of 500 when link code contains malformed JSON or missing required fields - OAuthAuthCodeRepository: Pass cancellation token to ExecuteSqlRawAsync for proper request cancellation support - ProfileSettingsView: Fix returnUrl from /workspace/settings to /workspace/settings/profile; remove mode=link query param (backend derives mode from auth state to prevent bypass)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4743422e09
ℹ️ 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".
| : "/"; | ||
|
|
||
| var linkSeparator = linkReturnUrl.Contains('?') ? "&" : "?"; | ||
| return Redirect($"{linkReturnUrl}{linkSeparator}oauth_link_code={Uri.EscapeDataString(linkCode)}"); |
There was a problem hiding this comment.
The link branch returns immediately after storing linkAuthCode, so CleanupExpiredCodesAsync() never runs for link flows. Because DeleteExpiredAsync is only invoked in the normal login branch, consumed/expired link codes will accumulate until a later login callback happens, which can cause unbounded OAuthAuthCodes growth in deployments that mainly use account-linking. Invoke cleanup (or a scheduled cleaner) on the link path as well.
Useful? React with 👍 / 👎.
| </section> | ||
|
|
||
| <!-- Linked Accounts Section --> | ||
| <section v-if="githubAvailable && !isDemoMode" class="td-settings__section"> |
There was a problem hiding this comment.
loadProviders() swallows errors and leaves githubAvailable as false, and the template gates the whole Linked Accounts section behind v-if="githubAvailable && !isDemoMode". On transient /auth/providers failures, users lose access to existing link/unlink controls for that session even if accounts are already linked. This also conflicts with the frontend/AGENTS.md requirement to provide explicit loading/empty/error states instead of silently hiding functionality.
Useful? React with 👍 / 👎.
Summary
Implements #676 — three concrete improvements to the GitHub OAuth integration:
In-memory auth code store replaced with SQLite/EF Core: Auth codes now persist across restarts and work in multi-instance deployments. Uses
OAuthAuthCodeentity with atomicTryConsumeAtomicAsyncfor race-safe single-use semantics via raw SQLUPDATE WHERE IsConsumed = 0.PKCE enabled: Sets
UsePkce = trueon the GitHub OAuth handler. ASP.NET Core 8 automatically generates code_verifier/code_challenge — defense-in-depth against authorization code interception.Account linking: Authenticated users can link/unlink GitHub accounts from the Settings page. Uses a dedicated link flow:
GET /api/auth/github/login?mode=linkstores GitHub identity in a link code, thenPOST /api/auth/github/linkexchanges the code while requiring JWT auth. Prevents linking already-linked accounts with proper conflict detection.Key files changed
Backend:
backend/src/Taskdeck.Domain/Entities/OAuthAuthCode.cs— New entity with Purpose (login/link), TryConsume, CreateForLinkingbackend/src/Taskdeck.Api/Controllers/AuthController.cs— DB-backed exchange, link/unlink/linked-accounts endpointsbackend/src/Taskdeck.Api/Extensions/AuthenticationRegistration.cs— PKCE enabledbackend/src/Taskdeck.Application/Services/AuthenticationService.cs— CompleteAccountLinkAsync, UnlinkExternalLoginAsyncFrontend:
frontend/taskdeck-web/src/views/ProfileSettingsView.vue— Linked Accounts section with Link/Unlink GitHubfrontend/taskdeck-web/src/api/authApi.ts— linkGitHub, unlinkGitHub, getLinkedAccountsfrontend/taskdeck-web/src/types/auth.ts— LinkedAccount typeTests:
Test plan
dotnet test backend/Taskdeck.sln -c Release -m:1— all 1181 tests passnpm run typecheck && npx vitest --run— all 1898 tests pass, no errorsCloses #676