Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| $views = array_map(function ($item) { | ||
| return $item['name']; | ||
| }, Schema::getViews()); | ||
| }, Schema::getViews($database)); |
There was a problem hiding this comment.
getViews returns incompatible data structure breaking view logic
High Severity
The getViews() method now returns a numerically-indexed array of view name strings, but consumers expect an associative array keyed by view name with objects having a getSql() method. In shouldCreate(), the check isset($views[$viewName]) will always fail since the array uses numeric keys, causing views to always be recreated unnecessarily. In the up() method's foreach loop, $viewName becomes numeric indices (0, 1, 2...) instead of actual view names, breaking the dropped table detection logic entirely.
Additional Locations (2)
nolanpro
changed the title
| $request->name, | ||
| null, // provider | ||
| false // confidential | ||
| ); |
There was a problem hiding this comment.
OAuth clients not associated with user when created
High Severity
When creating personal access or password grant clients via store(), the new code uses createPersonalAccessGrantClient() and createPasswordGrantClient() which don't associate the client with the authenticated user. The old code passed $request->user()->getKey() to link all client types to the user. Since show(), update(), and destroy() all use findForUser($clientId, $request->user()) to retrieve clients, users can no longer access, modify, or delete personal access and password grant clients they create through this API.
Additional Locations (2)
| public function update(Request $request, $clientId) | ||
| { | ||
| $client = $this->clients->find($clientId); | ||
| $client = $this->clients->findForUser($clientId, $request->user()); |
There was a problem hiding this comment.
OAuth client update/destroy restricted to owner only
Medium Severity
The update and destroy methods changed from $this->clients->find($clientId) to $this->clients->findForUser($clientId, $request->user()). This restricts operations to only clients owned by the requesting user, while the index method still returns ALL clients. Users with edit-auth_clients or delete-auth_clients permissions will see clients in the list but receive 404 errors when attempting to modify clients they don't own, breaking admin management functionality.
Additional Locations (1)
| /** | ||
| * Store a new client. | ||
| * | ||
| * @param \Illuminate\Http\Request $request |
There was a problem hiding this comment.
Duplicated type-extraction logic in store and update methods
Low Severity
The store() and update() methods contain identical code for extracting $personalAccess, $password, and $redirect from $request->types. These three lines are duplicated verbatim between the two methods. This logic could be extracted to a private helper method like parseClientTypes(Request $request) to reduce duplication and make future maintenance easier.
Additional Locations (1)
| 'file_size_check' => ProcessMakerMiddleware\FileSizeCheck::class, | ||
| 'auth.basic' => Illuminate\Auth\Middleware\AuthenticateWithBasicAuth::class, | ||
| 'throttle' => Illuminate\Routing\Middleware\ThrottleRequests::class, | ||
| 'client' => Laravel\Passport\Http\Middleware\CheckToken::class, |
There was a problem hiding this comment.
Wrong Passport middleware class for client alias
High Severity
The client middleware alias was changed from CheckClientCredentials to CheckToken. These serve different purposes: CheckClientCredentials validates OAuth2 client credentials grant flows, while CheckToken validates regular bearer access tokens. This breaks client credentials authentication for any routes using the client middleware.
| protected \Illuminate\Contracts\Validation\Factory $validation, | ||
| ) { | ||
| } | ||
|
|
There was a problem hiding this comment.
Client list not filtered by user
Medium Severity
The index method returns all non-revoked clients without filtering by user, while show, update, and destroy methods use findForUser to restrict access to user-specific clients. This inconsistency allows users to see other users' OAuth clients, which may expose sensitive client information.
| // Authorization code grant | ||
| $client = $this->clients->createAuthorizationCodeGrantClient( | ||
| $request->name, | ||
| $redirect ? explode(',', $redirect) : [], |
There was a problem hiding this comment.
Redirect URL incorrectly split on commas
Medium Severity
The redirect URL is split using explode(',', $redirect), but validation enforces a single URL via the url rule. This breaks if the redirect URL contains commas (valid in URLs, e.g., query parameters like ?items=1,2,3). The URL would be incorrectly split into multiple invalid array elements. Should use [$redirect] to wrap the single URL in an array.
|
|
||
| // Do not retry this job if it fails | ||
| public $tries = 10; | ||
| public $tries = 1; |
There was a problem hiding this comment.
Job retry attempts drastically reduced
Medium Severity
The $tries property changed from 10 to 1, meaning script executor builds will no longer retry on failure. The comment states "Do not retry this job if it fails" which matches the new value, but the previous value of 10 suggests retries were important for handling transient failures. Builds may now fail permanently due to temporary network issues or resource constraints.
|
QA server K8S was successfully deployed https://ci-9fdd0aafa1.engk8s.processmaker.net |
1 similar comment
|
QA server K8S was successfully deployed https://ci-9fdd0aafa1.engk8s.processmaker.net |
| if (!$client) { | ||
| $clientRepository = app('Laravel\Passport\ClientRepository'); | ||
| $client = $clientRepository->create(null, 'devlink', $redirectUri); | ||
| $client = $clientRepository->createAuthorizationCodeGrantClient('devlink', [$redirectUri]); |
There was a problem hiding this comment.
DevLink sends hashed secret instead of plain text
High Severity
In Passport v13, client secrets are always hashed before storage. When a new client is created via createAuthorizationCodeGrantClient, $client->secret contains the hashed value, not the plain text. The plain text is only available via $client->plainSecret at creation time. Sending the hashed secret as client_secret in the redirect URL will completely break the DevLink OAuth flow, since the remote server will store a hash and fail to authenticate when exchanging the authorization code for tokens.
Additional Locations (1)
| ); | ||
| } | ||
|
|
||
| $client->makeVisible('secret'); |
There was a problem hiding this comment.
Client store API returns hashed secret to consumers
High Severity
In the store method, $client->makeVisible('secret') exposes the secret attribute in serialization, but in Passport v13 this attribute now contains the hashed value. API consumers creating OAuth clients will receive a hash instead of the usable plain-text secret. The response (and the AuthClientCreated event via getAttributes()) needs to include $client->plainSecret instead, which is only available at creation time.
Additional Locations (1)
|
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 6 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| if (!$client) { | ||
| $clientRepository = app('Laravel\Passport\ClientRepository'); | ||
| $client = $clientRepository->create(null, 'devlink', $redirectUri); | ||
| $client = $clientRepository->createAuthorizationCodeGrantClient('devlink', [$redirectUri]); |
There was a problem hiding this comment.
Client redirect query may fail with JSON storage
Medium Severity
The query Client::where('redirect', $redirectUri) attempts exact string matching against the redirect column. In Passport 13, the redirect field stores an array of URLs (potentially as JSON), but this query treats it as a single string. After creating a client with [$redirectUri], the subsequent lookup will fail to find it because the stored value is a JSON array while the query searches for a plain string.
| } | ||
|
|
||
| $original_values = $client->getAttributes(); | ||
|
|
There was a problem hiding this comment.
Client update stores string instead of array
Medium Severity
In the update method, redirect is assigned as a plain string via forceFill, while Passport 13 expects an array of redirect URIs. The store method correctly uses explode(',', $redirect) to convert comma-separated URLs to an array, but update lacks this conversion, causing a type mismatch where Passport's Client model expects an array but receives a string.
| AuthClientDeleted::dispatch($client->getAttributes()); | ||
| AuthClientDeleted::dispatch($attributes); | ||
|
|
||
| return response('', 204); |
There was a problem hiding this comment.
Redirect validation incompatible with parsing logic
Low Severity
The validation rule 'redirect' => 'required|url|max:2000' only accepts a single valid URL, but the store method uses explode(',', $redirect) to split comma-separated URLs. If a user provides multiple redirect URLs like https://a.com,https://b.com, validation will reject it as invalid because the combined string isn't a valid URL, even though the code is designed to support multiple redirects.
Additional Locations (1)
| } | ||
|
|
||
| $tokens = $this->tokenRepository->forUser($user->id); | ||
| $tokens = $this->tokenRepository->forUser($user); |
There was a problem hiding this comment.
Missing user ID parameter in token operations
Medium Severity
The TokenRepository methods forUser and findForUser now receive the User model object instead of the user ID. If Passport 13's TokenRepository expects scalar user IDs rather than model instances, these calls will fail with type errors or incorrect queries. The change from $user->getKey() to $user object needs verification against Passport 13's actual API.
Additional Locations (1)
| $tables = array_map(function ($item) { | ||
| return $item['name']; | ||
| }, Schema::getTables()); | ||
| }, Schema::getTables($database)); |
There was a problem hiding this comment.
Missing null check for database name
Medium Severity
The code calls \DB::connection()->getDatabaseName() without checking if it returns null. If the database connection doesn't have a configured database name, Schema::getTables($database) and Schema::getViews($database) will receive null, which could cause these methods to fail or behave unexpectedly when querying schema information.
Additional Locations (1)
|
|
||
| // On occasion, the "auth" property is an empty array | ||
| // and trying to call the viaRemember() method off of | ||
| // it will throw a fatal error, this is a work around |
There was a problem hiding this comment.
Missing password hash session invalidation logic
Medium Severity
The custom password hash verification logic was completely removed. The old implementation checked cookie-based remember tokens and session password hashes, logging users out when mismatches were detected. The new code only delegates to the parent after checking for SessionGuard, but doesn't verify if Laravel's base class performs these security checks. If it doesn't, users won't be logged out when their password changes, allowing hijacked sessions to remain active.
2 participants
ci:k8s-branch:2026-4-php84
ci:package-auth:task/FOUR-28803
ci:package-email-start-event:task/FOUR-28803
ci:package-collections:task/FOUR-28803
ci:package-actions-by-email:task/FOUR-28803
ci:pmql:task/FOUR-28803
ci:package-analytics-reporting:task/FOUR-28803
ci:package-savedsearch:task/FOUR-28803
ci:package-decision-engine:task/FOUR-28803
ci:package-smart-extract:task/FOUR-28803
ci:package-ai:task/FOUR-28803
ci:deploy
....
Note
High Risk
Framework and auth-layer upgrades plus a rewrite of middleware registration can affect request handling, session/auth behavior, and OAuth client/token flows across the app.
Overview
Upgrades the platform baseline to PHP 8.4 and Laravel 12, with broad dependency bumps (notably
laravel/passport13,lcobucci/jwt5,psr/http-message2,l5-swagger9) and adds.envrcto.gitignore.Migrates HTTP middleware configuration out of
ProcessMaker/Http/Kernel.phpinto Laravel 11+/12-stylebootstrap/app.php->withMiddleware()wiring, including global middleware, group composition, aliases, and explicit priority ordering.Updates Passport integrations for the newer APIs: switches OAuth client creation to grant-specific
ClientRepositorymethods (including devlink), changes token repository calls to acceptUserobjects, adds explicit disabling of Passport client UUIDs, and expandsUserTokenResourceto serialize bothPersonalAccessTokenResultandTokenconsistently.Removes the
processmaker:create-test-dbscommand, tightensBuildScriptExecutorretries from 10 to 1, makes PMQL scope parameters nullable, and adjusts schema introspection for data lake views to pass the current database name toSchema::getTables()/getViews().Written by Cursor Bugbot for commit ae36a74. This will update automatically on new commits. Configure here.