Repository navigation
Sync package updates and check QUERY requests for request forgery - #66
Merged
Merged
Conversation
Swoole\Coroutine\Socket::recv() takes the read length first and the timeout second. Ten calls in the HTTP and database test fixtures passed 2 or 3 positionally, intending a timeout, so each read returned at most two or three bytes and waited without a time limit. Pass the timeout by name. The remaining positional recv() calls are on HTTP, HTTP/2 and gRPC clients, whose first argument is the timeout. The affected test files pass.
Laravel master's QUERY route support (laravel/framework#60655, ported earlier) treats QUERY as a reading method, so PreventRequestForgery skipped its origin and token checks. Method override is enabled, so a cross-site form POST with _method=QUERY, in the query string or the body, reached QUERY routes without either check when the browser sent the session cookie. Laravel 13.x verifies QUERY on purpose (laravel/framework#61797 at 3bb5d6084544), so isReading() now covers only HEAD, GET and OPTIONS, with a source comment explaining why. Port #61797's two tests, and cover spoofed POSTs from both request bags, without and with a valid token, and QUERY in origin-only mode. Port the matching csrf.md and routing.md wording from laravel/docs 13.x (226b0649c7, at 12080c72699b), and drop http-tests.md's stale link to the removed Swoole QUERY warning. The rest of #61797 already matched the #60655 port. Formatting, static analysis and the full suite pass.
laravel/ai#1139 (laravel/ai 1.x at d44ae14d17fb) limits fetched remote files in kilobytes and rejects a response whose Content-Length exceeds the limit before reading its body. Hypervel already bounded downloads in bytes. ai.remote_files.max_size is now in kilobytes, shipped as 32 * 1024 and matched by the public MAX_KILOBYTES constant, keeping null to disable the limit, the validation, cancellation precedence and URL-free messages in upstream's wording. Port the on_headers rejection. The sink keeps its tell()-based check, since upstream's getSize() adds clearstatcache() and fstat() for every chunk. The transport drops a compressed response's Content-Length before on_headers, so the sink still bounds decoded bytes. Port upstream's five tests with Hypervel's fakes, replacing a duplicate faked-limit test, move the loopback cases to kilobytes and add a real server case that is rejected before any body is sent. Document the kilobyte unit and the early rejection in ai-sdk.md and the README difference. The same file also takes laravel/docs#11401 (13.x at 12080c72699b): move in the FileStorage tool list and Groq's GPT-OSS notes for web search and code execution. Formatting, static analysis and the AI suite pass.
laravel/ai#1150 (laravel/ai 1.x at d44ae14d17fb): Gemini expects a model turn's thought step before its text, but mapAssistantMessage() rebuilt stored turns with the text first. Emit the thought step first; this is the only place Gemini turns are rebuilt. Port upstream's mapping test and its persistence-test update. Formatting, static analysis and the AI suite pass.
laravel/scout#1015 (laravel/scout 11.x at d6d1774c4fa5) makes queued imports keep their queue and connection. Hypervel's MakeRangeSearchable already prefers its own queue and connection, so only the tests are ported: the queue-import command test, and upstream's two job tests in place of Hypervel's two duplicates, keeping upstream's key and count assertions. The Scout suite passes.
laravel/scout#1013 (laravel/scout 11.x at d6d1774c4fa5) creates missing Typesense collections with the model's indexable name. Hypervel's createCollectionFromModel() already receives that name on update. Give Hypervel's matching unit test upstream's name and port upstream's integration test, where the schema has no name and the read collection is never created. It passes against Typesense 27.1.
laravel/scout#1014 (laravel/scout 11.x at d6d1774c4fa5) escapes values in Meilisearch whereIn and whereNotIn filters. Hypervel's filters() already escapes them, so upstream's unit and integration tests are ported; the integration test filters on title. Both fail without the escaping, and both pass against Meilisearch.
laravel/telescope#1770 (laravel/telescope 5.x at dcc6674c825f) formats query previews by driver. Port formatQuery(), sqlDialect(), the pgsql and sqlsrv aliases, raw output for unknown drivers and the highlightQuery/querycode rename. MongoDB is unsupported, so its alias, the JSON grammar and branch, and queryLanguage() are omitted with REMOVED comments; the preview keeps a static language-sql class. Port upstream's Vitest harness (dev dependencies, config and scripts) and preview.test.js at upstream's location, without the MongoDB-only cases and with SQL clipboard and ready cases. The CI JavaScript job installs npm 11.21.0, which Telescope's devEngines require, and runs npm ci and npm test in src/telescope. The lockfile also takes laravel/telescope#1772, bumping source-map-js to 1.2.2 as upstream does. dist is rebuilt. npm ci and npm test pass.
laravel/wayfinder#324 (laravel/wayfinder main at 81df21e16dc1) adds QUERY route support. Hypervel's Wayfinder already has the query Method type, so port upstream's QueryMethod.test.ts with its SearchController fixture and two QUERY routes. The usage docs follow with the next commit's Wayfinder docs. The Wayfinder JavaScript tests in both modes, the type check and the PHP suite pass.
laravel/wayfinder#331 (laravel/wayfinder main at 81df21e16dc1) adds wayfinder:generate --skip-database, for builds where the database is unreachable. Port the option and BindingResolver::skipDatabase(), which clears cached columns when the setting changes and is reset by flushState(). Hypervel types bindings from casts, then the schema, then docblocks, so skipping the schema falls through to the docblock. Port upstream's SkipDatabaseTest. Its model is non-incrementing, since an incrementing key's int cast correctly outranks the docblock. The test fails without the skip check or the cache reset. Document the option, noting that casts still apply. wayfinder.md also takes #324's QUERY usage and form-spoofing paragraphs, adapted to Route::query(); method override spoofs _method=QUERY over POST. Formatting, static analysis and the Wayfinder suites pass.
Port laravel/docs d9b95fe9c8 (13.x at 12080c72699b): the notes separating server and client authentication, the client example and optional named-client wording, and the Bearer Tokens, Linking User Accounts With OAuth and Client Metadata sections with upstream's projects example, where withOAuth() and withToken() configure separate state. Hypervel's coroutine-owned named clients, TokenSet return types, scope storage (now projects_scope, also in the scopes example) and the custom redirectUri note are kept.
Port laravel/docs#11409 (13.x at 12080c72699b) to blade.md. @stack compiles to yieldPushContent() where it is reached, as in Laravel, so pushes rendered after the stack do not appear in it.
Port laravel/docs#11413 (13.x at 12080c72699b) to the end of urls.md's signature validation section, after Hypervel's ValidateSignature additions. Request::getInputSource() uses the JSON body even for GET requests, and all() gives it precedence over the query string.
spatie/laravel-permission#2978 (main at 264c0dedebca): hasRole() with a Role model ignored the requested guard, so a role from another guard matched. Filter Role models by a non-empty guard, as Hypervel does for role names, and have hasAllRoles() delegate to hasRole(), keeping the partition check. hasPermissionTo() keeps upstream's behavior, where a Permission model's own guard wins. Port upstream's data-provider test; its different-guard case fails before the fix. The Permission suite passes.
spatie/laravel-permission#2969 (main at 264c0dedebca) indexes cached permissions by name and key. Hypervel's catalog already indexed key and name+guard lookups, but built them from attribute reads, so loading ran name and guard accessors, and an index could disagree with filtering when an accessor or cast changed the value. Build the name index from raw attributes, and use an index only when reading its attributes returns the stored value (the key may keep its key-type cast). Non-integer, non-string lookup values use the filter, and filterModels() compares names through enum_value() for enum-returning name accessors. Upstream rescans returned models to catch unsaved changes. Instead, the owner-approved contract is that callers save a changed cached role or permission before looking up roles or permissions again, keeping lookups free of rescans. It is documented in the source, the package README, permission.md and the porting guide. The porting guide also gains the expiration_seconds and same-backend lock requirements, and its AI SDK entry takes the remote file size default from laravel/ai#1139. Port upstream's shared lookup tests, accessor test, custom-collection cases and an exact-matching version of its loose-comparison test, and add a current key-accessor lookup regression. REMOVED comments cover the unsaved-mutation, runtime boolean-cast and excluded-key tests. Port the lookup benchmark as an explicitly run fixture outside normal test discovery. Formatting, static analysis and the Permission suite pass.
saloonphp/rate-limit-plugin#30 (v2 at 180c3f757f55) stops cached responses from counting against rate limits. Hypervel's SaloonManager::send() already reads the cache before rate limiting, so cache hits use no capacity. Port upstream's five CacheHitRateLimitTest cases, sending a cacheable CachedUserRequest in place of upstream's fake-response middleware, and faking real requests with Http::fake(), since Saloon fakes use no capacity. Four cases fail if cache hits go through rate limiting. The Saloon suite passes.
The benchmark harnesses refused full-size runs unless they were given --idle-confirmed or BENCH_IDLE_CONFIRMED=1, and their READMEs asked for an owner-confirmed idle window. That encoded a private review workflow into public developer tools. Remove the flags and their rejection paths, so a run without smoke mode measures at full size, and describe the requirement as ordinary guidance to run on an idle machine. Smoke and probe modes, workload settings, service and port validation, cleanup and correctness assertions are unchanged. Sentry's components.sh keeps an explicit mode, now smoke|measure|inventory, still clears an inherited BENCH_SMOKE, and passes no fetcher flag in measure mode. Reproduction commands in the READMEs and results files drop the flag; recorded results are unchanged. The PHP files pass php -l, components.sh passes bash -n, and each harness's help and argument checks were run without measuring.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Summary by QodoVerify QUERY requests and sync package fixes, tests, and docs
AI Description
Diagram
High-Level Assessment
Files changed (76)
|
Code Review by Qodo
1. Old byte-valued size limits grow 1024-fold
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
 [High impact] The PR reaches shared request, permission, AI, and developer-tool workflows. The PR is not ready to merge because existing published AI settings can raise the remote download limit from 32 MiB to 32 GiB.
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+41
to
+47
| $kilobytes = config()->get('ai.remote_files.max_size', static::MAX_KILOBYTES); | ||
|
|
||
| if ($maxSize !== null && (! is_int($maxSize) || $maxSize <= 0)) { | ||
| if ($kilobytes !== null && (! is_int($kilobytes) || $kilobytes <= 0)) { | ||
| throw new InvalidArgumentException('The remote file maximum size must be a positive integer or null.'); | ||
| } | ||
|
|
||
| $limit = $kilobytes === null ? null : $kilobytes * 1024; |
There was a problem hiding this comment.
Existing published config/ai.php files keep their old byte-based ai.remote_files.max_size value. Multiplying that value by 1024 turns the previous 32 * 1024 * 1024 setting into a 32 GiB limit, not 32 MiB. Remote attachments then read the full body into memory, so an untrusted attachment can exhaust worker memory or temporary storage.
Keep old settings safe through an explicit unit migration or a new setting, and tell existing users how to migrate.
How this was verified: Published application settings override package defaults, and the retained value is multiplied by 1024 before both download checks use it.
Member
Author
There was a problem hiding this comment.
Declining. The byte-valued setting was never released: the AI package isn't in any tagged release, and the setting only existed on the unreleased 0.4 branch, which makes breaking changes without compatibility layers. Upstream reads this key in kilobytes, so there's no published value to migrate. The Laravel porting guide covers the different default.
Comment on lines
+72
to
+76
| public function testSkippingTheDatabaseReadsModelDocblocks(): void | ||
| { | ||
| $this->assertStringContainsString('post: number', $this->generate()); | ||
|
|
||
| $this->assertStringContainsString('post: string', $this->generate(skipDatabase: true)); |
There was a problem hiding this comment.
This test proves that --skip-database chooses docblocks over the schema, but not that it avoids a database call. A change that tries the connection and then discards the result would still pass, yet could delay every database-free build until connection attempts time out.
Add a model connection spy and assert it is never called in skip mode.
Knowledge Base Used: Wayfinder route generation
Member
Author
There was a problem hiding this comment.
Declining. getColumns() returns before calling getConnection() when the option is set, and this test already fails without that check, since the schema's integer key would win over the docblock. A connection spy would only guard against a hypothetical rewrite.
There was a problem hiding this comment.
That’s fair. The observable assertion is sufficient here: without the skipDatabase branch, the schema’s integer key takes precedence and the test fails, while the generated string type proves the database-derived metadata was bypassed. A connection spy would only protect against an implementation rewrite that still made an unnecessary call, so I’m withdrawing this comment.
Since the spatie/laravel-permission#2969 port, getPermissions() and getRoles() send enum lookup values to filterModels(), because the name and key indexes only take strings and integers. filterModels() only normalized the model's name, so an enum value never matched: getPermissions(['name' => Enum::Writer, 'guard_name' => 'web']) and a name-only enum lookup both returned nothing, although the previous name and guard index matched them. attributeMatches() now compares enums by their values on both sides, as Laravel's Collection::where() does, and the name-only special case is removed. Enum-returning name accessors still match enum and string values. findByName() and findById() already normalize their arguments, so the indexed lookups are unchanged. Add a regression test for a plain permission looked up by enum, with and without a guard, and for the enum-accessor fixture role looked up by enum and by string. The Permission suite, formatting and static analysis pass.
The spatie/laravel-permission#2978 port made hasRole() match a Role model by key and the requested guard, and a single model passed to hasAllRoles() delegates to it. A Role model inside an array or collection was still mapped to its name, so a user with the web testRole passed hasAllRoles([$adminTestRole]). hasExactRoles() mapped models to names before calling hasAllRoles(), so it matched the same way, for a single model too. Upstream has the same defect. hasAllRoles() now checks role models through hasRole() and compares only the remaining values by name. hasExactRoles() passes models through unchanged and keeps its partition check before the count comparison, so a role from another partition still throws when the counts differ. Add a regression test with positive controls; removing either fix makes it fail. The Permission suite passes.
The mixed-20 scenario cycles over the last 20 permission names, and the benchmark README documents a minimum BENCH_COUNT of 20. A smaller value failed partway through with a lookup of bench--1. Check the value where it is read, before inserting rows or timing anything.
The laravel/docs#11413 warning says a signature only covers the URL's path and query string. Absolute signatures, the default, also cover the scheme and host, since hasCorrectSignature() signs the full URL; only relative signatures leave them out. Say that a signature covers the URL, including its query string.
The comment said QUERY stays verified because it carries a body, but a GET request can carry a body too. The reason is that method override lets a cross-site form POST arrive as QUERY. Reword the comment to say so.
The laravel/scout#1014 whereNotIn assertion only checks that the injected record is missing, because upstream's suite seeds many records. This test indexes two records, so assert the exact result, which also fails when the filter drops records it should keep. The test passes against Meilisearch.
The laravel/telescope#1770 tests passed with driverAliases emptied, since the alias test called sqlDialect() directly and the PostgreSQL test only checked that @> remained. They also passed when the preview showed the raw query, since the SQLite test only checked for words in it. Assert formatQuery()'s exact output for pgsql and sqlsrv, and the exact formatted SQLite preview, next to upstream's assertions. npm test passes.
laravel/ai#1139 rejects a response whose Content-Length exceeds the limit before reading its body. The transport removes a compressed response's Content-Length before on_headers runs, so only the decoded size counts. Add a download size case whose gzip body is larger than the limit while its decoded body fits, so it must be accepted. Checking the encoded length in on_headers makes it fail.
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.
Upstream Updates
QUERYrequests for request forgery. The middleware treatedQUERYas a reading method and skipped its origin and token checks. Since method override is enabled, a cross-site form could POST with_method=QUERYand reach aQUERYroute with the visitor's session cookie but without either check. Port upstream's tests, and cover spoofed POSTs from the query string and the body, with and without a valid token, andQUERYin origin-only mode. Port the matching CSRF and routing documentation.ai.remote_files.max_size, in kilobytes as upstream does, and reject a response that declares a largerContent-Lengthbefore downloading its body. Hypervel already limited decoded downloads; the default stays 32 MiB andnullstill disables the limit. The streaming check keeps using the body's position instead of upstream's stat call on every chunk. Document the unit and the early rejection.FileStoragetool list, and note that Groq only supports web search and code execution on its GPT-OSS models and ignores the web searchmax,allowandlocationmethods. The rest was already documented.whereInandwhereNotInvalues. Hypervel already had all three fixes. Upstream's job tests replace two Hypervel duplicates.QUERYroutes, which Hypervel's Wayfinder already supported. Document their generatedquerymethod and how forms spoof them.wayfinder:generate --skip-databasefor builds that can't reach the database. Route binding types then come from casts and@propertydocblocks instead of the schema. Document the option.@stackonly renders content pushed before it is reached.query().hasRole()andhasAllRoles()respect the requested guard when given aRolemodel. A role from another guard used to match.Additional Hypervel Fixes
Swoole\Coroutine\Socket::recv()takes the length first, so ten calls meant to wait two or three seconds instead read at most two or three bytes at a time, with no time limit.expiration_secondssetting, the same-backend lock requirement and saving changed models before further lookups.Rolemodels in a list passed tohasAllRoles()orhasExactRoles(), and a single model passed tohasExactRoles(), by key and guard. They were compared by name, so a role with the same name from another guard matched.QUERYroute limitation that no longer exists.The full suite, formatting and static analysis pass, along with the Scout integration tests against Meilisearch and Typesense and the Telescope and Wayfinder JavaScript tests. CI will run the full suite and supported service matrix.
Note
Require forgery verification for QUERY requests and sync package updates
PreventRequestForgery, so cross-site QUERY requests now require an origin or CSRF token, including method-override spoofs; adds tests and CSRF/routing docsPermissionRegistrar: cached catalogs record which indexes are safe, indexed lookups decline unsupported value types, and models with accessors or disallowed casts fall back to filteringHasRolesguard-aware for Role model inputs inhasRole,hasAllRoles, andhasExactRolesmax_sizeto kilobytes (default 32 MiB viaMAX_KILOBYTES), with pre-checks on declaredContent-Lengthand decoded byte counting; adds--skip-databaseoption towayfinder:generateand dialect-aware SQL formatting in the Telescope query previewmax_sizein bytes are now interpreted as kilobytes; indexed permission lookups skip accessor-transformed attributesMacroscope summarized 48c1c8a.