Conversation
fix(oauth): preserve consent failures
fix(oauth): preserve failed consent logout
…ble container lifecycle and teardown
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_442163c1-0135-485d-a4e0-3bafa97b1fc2) |
There was a problem hiding this comment.
Code Review
This pull request introduces helper functions to manage OAuth consent unauthorized responses and global logout logic, integrating them into the layout context, along with a new PowerShell script for automated integration testing using Docker. The review feedback highlights potential TypeError crashes in the layout context due to unsafe access to options.method when options is undefined, suggesting the use of optional chaining. Additionally, it recommends checking $LASTEXITCODE after starting Docker containers in the PowerShell script to ensure fast failures if container initialization fails.
| if ( | ||
| shouldPreserveOAuthConsentUnauthorized( | ||
| url, | ||
| options.method, | ||
| response.status | ||
| ) | ||
| ) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
The options parameter in afterRequest can be undefined or null if the fetch wrapper is called without options (e.g., for simple GET requests). Accessing options.method directly will throw a TypeError: Cannot read properties of undefined (reading 'method') and crash the application layout. Use optional chaining (options?.method) to safely access the method.
| if ( | |
| shouldPreserveOAuthConsentUnauthorized( | |
| url, | |
| options.method, | |
| response.status | |
| ) | |
| ) { | |
| return true; | |
| } | |
| if ( | |
| shouldPreserveOAuthConsentUnauthorized( | |
| url, | |
| options?.method, | |
| response.status | |
| ) | |
| ) { | |
| return true; | |
| } |
| if ( | ||
| shouldHandleGlobalLogout( | ||
| url, | ||
| options.method, | ||
| response.status, | ||
| Boolean(logout) | ||
| ) && | ||
| !isSecured | ||
| ) { |
There was a problem hiding this comment.
Accessing options.method directly can throw a TypeError if options is undefined. Use optional chaining (options?.method) to ensure safe property access.
| if ( | |
| shouldHandleGlobalLogout( | |
| url, | |
| options.method, | |
| response.status, | |
| Boolean(logout) | |
| ) && | |
| !isSecured | |
| ) { | |
| if ( | |
| shouldHandleGlobalLogout( | |
| url, | |
| options?.method, | |
| response.status, | |
| Boolean(logout) | |
| ) && | |
| !isSecured | |
| ) { |
| if ( | ||
| response.status === 401 || | ||
| shouldHandleGlobalLogout( | ||
| url, | ||
| options.method, | ||
| response.status, | ||
| Boolean(logout) | ||
| ) | ||
| ) { |
There was a problem hiding this comment.
Accessing options.method directly can throw a TypeError if options is undefined. Use optional chaining (options?.method) to ensure safe property access.
| if ( | |
| response.status === 401 || | |
| shouldHandleGlobalLogout( | |
| url, | |
| options.method, | |
| response.status, | |
| Boolean(logout) | |
| ) | |
| ) { | |
| if ( | |
| response.status === 401 || | |
| shouldHandleGlobalLogout( | |
| url, | |
| options?.method, | |
| response.status, | |
| Boolean(logout) | |
| ) | |
| ) { |
| docker run -d --name $PgContainer -p "127.0.0.1:${PgPort}:5432" -e POSTGRES_PASSWORD=postiz-password -e POSTGRES_USER=postiz-user -e POSTGRES_DB=postiz-db-local postgres:17-alpine | Out-Null | ||
| docker run -d --name $RedisContainer -p "127.0.0.1:${RedisPort}:6379" redis:7.2 | Out-Null |
There was a problem hiding this comment.
In PowerShell, external commands (like docker run) do not throw script-terminating errors when they fail, even with $ErrorActionPreference = "Stop". If docker run fails (e.g., due to port conflicts or Docker daemon not running), the script will continue and wait 30 seconds for PostgreSQL to become ready, leading to a slow and confusing failure. Check $LASTEXITCODE immediately after each docker run command to fail fast.
docker run -d --name $PgContainer -p "127.0.0.1:${PgPort}:5432" -e POSTGRES_PASSWORD=postiz-password -e POSTGRES_USER=postiz-user -e POSTGRES_DB=postiz-db-local postgres:17-alpine | Out-Null
if ($LASTEXITCODE -ne 0) { throw "Failed to start PostgreSQL container." }
docker run -d --name $RedisContainer -p "127.0.0.1:${RedisPort}:6379" redis:7.2 | Out-Null
if ($LASTEXITCODE -ne 0) { throw "Failed to start Redis container." }
What kind of change does this PR introduce?
Release & Reliability Improvements
Why was this change needed?
Promotes verified
devbranch fixes and tooling improvements tomain:scripts/test-integration.ps1with disposable container lifecycle management (starts isolated PostgreSQL & Redis test fixtures, executes Jest bootstrap suite, and guarantees automatic teardown viafinallyblock).Technical Details & Scope
apps/frontend/src/components/layout/: Handled unauthorized consent states and error messaging.tests/: Added integration tests for consent error scenarios.scripts/test-integration.ps1: Automated isolated test runner with auto-teardown.Verification & Testing
scripts/validate-beta-compose.mjspassed.QA
mainand runpnpm installandpnpm run build.GET https://post.crove.com/api/healthandGET https://beta-post.crove.com/api/healthreturnHTTP 200 OK..\scripts\test-integration.ps1and verify all tests pass and test containers are cleanly removed.Checklist:
pnpm run build).pnpm dlx tsx scripts/branding-guard.ts).Note
Medium Risk
Changes global fetch response handling for 401 and logout headers; scope is narrow (OAuth authorize POST) but touches session cookie clearing and redirects.
Overview
OAuth consent flows no longer get swept into the app-wide 401/logout redirect when a POST to
/oauth/authorizereturns 401. New helpers inoauth-consent-unauthorized.tsdrive an early exit in the layout fetchafterRequesthook and gate logout-header handling so the consent UI can show errors instead of clearing cookies and sending users to/or launches.Tests add unit coverage for those URL/method/status rules alongside existing consent error UI tests.
Dev tooling adds
scripts/test-integration.ps1, which spins up disposable Postgres/Redis containers, runs Prismadb pushand the Jest bootstrap suite, and tears containers down in afinallyblock (optional-KeepContainers).Reviewed by Cursor Bugbot for commit 5a0b5bc. Configure here.