fix(plugin-cloud-storage): prevent skipCloudStorage leaking onto shared request context - #17558
fix(plugin-cloud-storage): prevent skipCloudStorage leaking onto shared request context#17558Mohith26 wants to merge 2 commits into
Conversation
…ed request context The afterChange hook set skipCloudStorage on req.context, but the nested payload.update re-binds req.context to a shallow copy via createLocalReq, so the finally block deleted the flag from the copy while the caller's original context object kept skipCloudStorage: true. Any later Local API create sharing that context object hit the hook's early return and silently skipped the upload, losing the file binary. Capture the context reference before the nested update and delete the flag from that same reference, plus from the re-bound req.context copy. Fixes payloadcms#17546
edrpls
left a comment
edrpls
left a comment
There was a problem hiding this comment.
Reporter of #17546 here. Thank you for picking this up so quickly, and for following the traced mechanism.
Empirical validation (beyond the unit specs): our end-to-end reproduction (https://github.com/edrpls/payload/tree/repro/cloud-storage-shared-context-upload-loss, test/_community with an in-process S3 sink) fails on unpatched main and passes with this PR applied, control test included. We also verified the PR body's claim independently: 4/4 unit tests pass with the fix, and exactly the two leak tests fail with the implementation reverted.
Two things from our review worth addressing before merge, both left as inline comments:
- Tests 3 and 4 currently pass without the fix too (they are not regression tests yet). Root cause is one mock-ordering detail; a small change makes them both discriminate.
- Non-blocking: this fix makes the adjacent
_payloadCloudStoragestash frompreserveFileDatanewly reachable on shared-context flows. Details inline; fine as a follow-up if maintainers prefer.
If useful, feel free to adapt our test/_community repro into an integration test for this PR (the issue's repro was built for exactly that per the reproduction guide); happy for it to be used without attribution.
| }) | ||
| } finally { | ||
| delete req.context.skipCloudStorage | ||
| delete hookContext.skipCloudStorage |
There was a problem hiding this comment.
Non-blocking, adjacent finding that this fix makes newly relevant: the _payloadCloudStorage stash written by preserveFileData is never cleared anywhere, and before this PR the stuck skipCloudStorage flag made later shared-context calls early-return before ever reading it, so the stale stash was dormant. With the flag now correctly cleared, those calls proceed into getIncomingFiles, and any call without a fresh req.file falls back to the FIRST call's stashed buffer paired with the current document's filename (we reproduced this at unit level against getPreserveFileDataHook + getIncomingFiles). It also pins the first upload's buffers for the life of a shared context object.
Since this finally is the end of upload processing for the operation, clearing the stash here in the same shape (delete hookContext._payloadCloudStorage plus the rebound-copy variant) would close it, but a separate follow-up PR is equally reasonable. Flagging mainly so the reachability change is a conscious decision.
| logger: { error: vi.fn() }, | ||
| update: vi.fn().mockImplementation(async () => { | ||
| await options?.onUpdate?.(req as unknown as PayloadRequest) | ||
| req.context = { ...req.context } |
There was a problem hiding this comment.
The mock re-binds req.context only after onUpdate and only on the resolve path. In the real Local API the re-bind happens at the START of the nested operation (createLocalReq runs at the top of payload.update, before any work that could reject). Because of that ordering difference, tests 3 and 4 currently pass even with the fix reverted; only tests 1 and 2 discriminate (we verified by reverting the implementation and re-running).
Suggested shape that makes the mock faithful and both tests meaningful:
update: vi.fn().mockImplementation(async () => {
// createLocalReq re-binds BEFORE any nested work can run or reject
req.context = { ...req.context }
await options?.onUpdate?.(req as unknown as PayloadRequest)
if (options?.rejectWith) throw options.rejectWith
return doc
}),With rebind-then-reject, the pre-fix code demonstrably leaks the flag onto the shared object and the fixed code does not, so test 4 becomes a true regression test.
| expect(flagDuringUpdate).toBe(true) | ||
| // Cleaned up from the re-bound req.context as well, so later writes | ||
| // reusing this req are not skipped. | ||
| expect(req.context.skipCloudStorage).toBeUndefined() |
There was a problem hiding this comment.
This assertion also holds without the fix: by hook exit, req.context already IS the rebound copy (the mock re-binds inside update()), and the pre-fix delete req.context.skipCloudStorage targeted exactly that copy. Asserting on a captured reference to the ORIGINAL context object (as test 1 does) is what discriminates, once the mock ordering from the comment above is in place.
| it('removes the flag from the shared context even when the nested update rejects', async () => { | ||
| const sharedContext: RequestContext = {} | ||
| const req = createReq(sharedContext) | ||
| ;(req.payload.update as ReturnType<typeof vi.fn>).mockRejectedValue(new Error('update failed')) |
There was a problem hiding this comment.
mockRejectedValue replaces the implementation wholesale, so the re-bind on line 47 never runs here, which diverges from reality: createLocalReq re-binds before anything can reject. Verified: with a faithful rebind-then-reject mock, this test FAILS on the pre-fix code (the leak survives a rejection) and passes with this PR, turning it into a real regression test. Rejecting from inside the implementation after the re-bind (see the suggestion on line 47) gets you there.
2 participants
Fixes #17546, reported by @edrpls, following the reporter's suggested approach.
Verified the exact mechanism traced in the issue: the plugin-cloud-storage
afterChangehook setsskipCloudStorageonreq.context, but the nestedreq.payload.update({..., req})runscreateLocalReq->getRequestContext, which re-bindsreq.contextto a shallow copy. Thefinallycleanup then deletes the flag from that copy while the caller's shared context object keeps it forever, so every laterpayload.createsharing that object hits the early return and silently skips the upload.The fix captures the context reference into a local before the nested update and deletes the flag from that same reference, plus cleans it from the re-bound copy so later writes reusing the same
reqare not skipped either. No new deps, no refactors.Four unit tests cover the leak, the multi-create reuse pattern, recursion prevention during the nested update, and cleanup on rejection; exactly the two leak-regression tests fail on unfixed code. Plugin suite: 23 passed, zero new failures; eslint clean on the impl.