v1.6.60 - #247
Open
roncodes wants to merge 10 commits into
Open
Conversation
The mysql and sandbox connections were opened with PDO::ATTR_PERSISTENT. PHP then keeps the MySQL session alive in its persistent pool after the PDO object is destroyed, and hands that same session to the next PDO built from the same DSN, username and password - including one built while another handle is still using it. Two handles then share one transaction, and a COMMIT through either ends it for both. The loser's commit() raises "There is no active transaction" for writes that have already been made durable, so the request reports failure for data that landed and anyone who retries applies it twice. Reproduced directly: two live handles reporting the same CONNECTION_ID, one commit, and the other raising the exact error with the row already visible from a third connection. Laravel cannot detect this. Connection::commit() decides whether to issue a COMMIT from its own $transactions counter, while PDO decides whether a COMMIT is legal from the server's SERVER_STATUS_IN_TRANS flag; nothing reconciles the two. Observed in production paths that have nothing to do with each other - onboarding account creation, ledger invoice creation, and inventory stock adjustments - because the fault is in the connection options, not in any caller. Persistent connections also silently defeat Octane's DisconnectFromDatabases listener: disconnect() drops the PHP object and leaves the server-side connection open. Measured on a dev stack, 40 concurrent requests left 17 MySQL connections open and still idle minutes later, with the listener enabled. Defaults to off. DB_PERSISTENT=true restores the previous behaviour for deployments that have measured the reconnect cost and where no request opens a transaction.
…secret SendResourceLifecycleWebhook only populated the session context when a key was absent, then preferred that session value over the event's own. A long running queue worker keeps its session between jobs, so once it had handled an event from one API context every later event was signed and attributed with the first one's credentials, and its company/user context leaked across jobs too. The context serialized on the event is now authoritative: it is resolved once, written to the session unconditionally for downstream code to read, and the previous session state is restored in a finally block so the next job starts clean. Fixes #244
Three defects on the API credential authentication path, all of which left a credential live that an operator had every reason to believe was dead. 1. Soft-deleted credentials still authenticated. AuthenticateOnceWithBasicAuth looks the credential up with withoutGlobalScopes(), which strips SoftDeletingScope along with ExpiryScope. Expiry was re-applied in PHP; soft-deletion never was. A credential the console reports as "Deleted" therefore kept authenticating indefinitely -- and Delete is the only revocation most operators ever perform. Now rejected with the generic "not valid" 401, before the OPTIONS shortcut so a revoked key cannot seed api key session context on a preflight either. 2. Authentication was fail-open when the creating user was gone. A credential carries no identity of its own; it acts as the user that created it. When User::find() no longer resolved that user, the is_admin guard was skipped but setSession() still returned true. Authorization degraded safely -- a null user fails every group and admin check -- but authentication did not, so the key kept working on every read endpoint and every ungated write. Off-boarding a person did not revoke the keys they had created. Auth::setSession() now returns false in that case and the middleware answers 401. Note User is pinned to the mysql connection, so this resolves against the authoritative store for sandbox credentials too. 3. "Expire immediately" did not expire the credential. ApiCredential::setExpiresAtAttribute() maps 'immediately' to Carbon::now(), but Expirable::hasExpired() used a strict `<`, so now() < now() was false. ExpiryScope already treats an exactly-now expiry as expired (it keeps a row only while expires_at > now()), so the two disagreed on that boundary. hasExpired() is now inclusive, which makes the trait and the scope agree. Reported downstream against 1.6.35 and verified against main. Private tracker: FliitAU/fliit-extension#2212
…aned The coverage gate went red on AuthenticateOnceWithBasicAuth (57/58 statements): the early `return;` in bindUserResolver() was previously reached only because the sandbox fixture seeded sandbox-user-1 on the sandbox connection alone, so User::find() came back null on mysql. Correcting that fixture to mirror the real system — User is pinned to mysql and sandbox:sync copies mysql to sandbox — left the guard unexercised. bindUserResolver() is protected static, so a downstream middleware subclass can call it with arguments neither in-tree call site produces. Cover the guard directly through a harness subclass rather than reaching for @codeCoverageIgnore, asserting both halves refuse to bind and that the positive case still does.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #247 +/- ##
===========================================
Coverage 100.00% 100.00%
- Complexity 6730 6734 +4
===========================================
Files 397 397
Lines 22448 22470 +22
===========================================
+ Hits 22448 22470 +22
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…-aliasing fix(database): stop persistent PDO handles sharing one MySQL transaction
…k-session-bleed fix(webhooks): a queue worker signed lifecycle webhooks with a stale secret
…fail-closed fix(auth): api credentials survived revocation and creator removal
Laravel's url() helper renders its second argument as rawurlencoded path
segments with keys discarded, so apiUrl('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/api/user', ['id' => 1]) produced
https://host/api/user/1 instead of the documented ?id=1. Build the query
string with http_build_query after the port insertion instead.
Also correct the test bootstrap's url() shim to mirror the real
UrlGenerator::to() path-segment semantics, which had been masking the
divergence in UtilsTest.
Bugfix: Utils::apiUrl renders query params as path segments
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.
Release branch for v1.6.60. Version bumped and
RELEASE.mdwritten, so merging tagsv1.6.60and publishes.Do not merge until the three PRs below have merged into this branch. They are retargeted here rather than at
main, so this branch accumulates the release andmainonly sees it once.fix(database): stop persistent PDO handles sharing one MySQL transactionfix(webhooks): a queue worker signed lifecycle webhooks with a stale secretfix(auth): api credentials survived revocation and creator removalWhy 1.6.60 specifically
#243 needs to ship as ≥ 1.6.60 to reach consumers pinned at
^1.6.59; that PR deliberately left the bump and the release branch alone rather than triggering the tag flow from a fix branch.What's in it
Three independent defects, each of which let the platform keep doing something an operator had already told it to stop.
422 There is no active transaction, so retries double-applied.Upgrade steps worth reading before merging
RELEASE.mdcalls out three behaviour changes that will be visible to operators on upgrade:PDO::ATTR_PERSISTENTnow defaults to off. Octane connection counts are unchanged; short-lived PHP-FPM workers pay ~1–3 ms per request.DB_PERSISTENT=trueis the escape hatch.Not in this release
Per-key scoping for API credentials. A credential still has no scope of its own —
Auth::setSession()derivesis_adminfrom whoever created the key — so least privilege remains reachable only by scoping the creator into a dedicated service user. That is a feature with a design decision behind it, tracked separately.🤖 Generated with Claude Code