Capture upload request bodies in PulseProxy - #383
Draft
ffittschen wants to merge 3 commits into
Draft
ffittschen wants to merge 3 commits into
ffittschen wants to merge 3 commits into
Conversation
URLSessionProxy.upload(for:from:delegate:) and upload(for:fromFile:delegate:) create a URLSessionProxyDelegate that wraps the caller's delegate, but never pass it to URLSession: - the caller's task delegate was silently ignored, so it got no authentication challenges or progress callbacks; - the proxy never saw the created task, so it never logged the completion: the upload stayed pending in the console, without its response. Pass the delegate, like data(for:delegate:) and download(for:delegate:) already do.
URLSession keeps the body passed to uploadTask(with:from:) out of the task's originalRequest, so NetworkLogger, which reads originalRequest.httpBody, logged these uploads without a request body on every path. Attach the body to the task as an associated object (URLSessionTask.pulse_requestBody), which _logTask reads after httpBody: - PulseProxy swizzles uploadTaskWithRequest:fromData:, with and without a completion handler, and the private _uploadTaskWithRequest:fromData:delegate:completionHandler: used by upload(for:from:delegate:); - URLSessionProxy attaches the body in uploadTask(with:from:), uploadTask(with:from:completionHandler:) and, through URLSessionProxyDelegate, upload(for:from:delegate:).
Tasks created with uploadTask(withStreamedRequest:) get their body from the delegate's urlSession(_:task:needNewBodyStream:), so PulseProxy logged them without a request body. This includes every request with a body sent by swift-openapi-urlsession, which streams by default. When an upload task without a body resumes, PulseProxy now hooks needNewBodyStream: and needNewBodyStreamFromOffset:completionHandler: on the class that implements them, once per class, and tees the stream the delegate returns into a bound stream pair while recording the bytes. The recorded body is attached to the task before the stream closes, so it's there when the task completes. - Covers the task delegate and the session delegate (read through the task's private "session" key), delegates that forward through forwardingTarget(for:), such as URLSessionProxyDelegate, and inherited implementations, which are hooked on the class that defines them. - A redirect or an authentication retry asks for a new stream. The body of the latest attempt that was read to the end wins; an attempt that is cut off never replaces it, so a body is never partial. A resumed upload (from an offset) reuses the prefix read by the previous attempt. - Bodies over the store's responseBodySizeLimit aren't kept. - All tees run on one shared thread driven by a run loop and never block. The tees of a task that URLSession abandons are closed from the _didFinishWithError: hook, and a stream that a delegate returns after the task ended is passed through untouched.
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.
This PR logs the request bodies of two kinds of uploads:
The second includes every request with a body sent by swift-openapi-urlsession, which streams by default.
It builds on #381, because
URLSessionProxy.upload(for:from:delegate:)only sees the created task once the delegate is passed, so the diff includes that commit. The other two commits can be reviewed separately.1. Uploads from data
URLSession keeps the data passed to
uploadTask(with:from:)out oforiginalRequest, whileNetworkLoggerreadsoriginalRequest.httpBody. So every upload from data was logged without its body. PulseProxy now swizzles theuploadTaskWithRequest:fromData:variants, andURLSessionProxyattaches the body itself. Both attach it asURLSessionTask.pulse_requestBody(apackageproperty), which is read afterhttpBody.2. Streamed uploads
Tasks created with
uploadTask(withStreamedRequest:)get their body fromurlSession(_:task:needNewBodyStream:). PulseProxy now hooks that method on the delegate class that implements it, once per class, and tees the returned stream on one shared thread. The body is attached only once the stream has been read to the end, so a stored body is never partial.Bodies of either kind over
LoggerStore.Configuration.responseBodySizeLimitaren't attached or stored, and the upload itself is untouched. Attached bodies go throughsensitiveDataFieldslike any other.Part of #378.
How to test
Use an iOS app that calls
NetworkLogger.enableProxy()at launch, e.g. PulseGRPC's example app, and send the uploads tohttps://httpbin.org/post, which echoes the body it received under"data".URLSession.shared.upload(for:from:). Open the console. Assert that the POST shows its request body, and that the response echoes the same body.uploadTask(with:from:)anduploadTask(with:from:completionHandler:). Assert the same for each.uploadTask(withStreamedRequest:)with a delegate that returns anInputStreamfromurlSession(_:task:needNewBodyStream:). Assert that the console shows the request body, and that the response echoes it intact.Limitations
uploadTask(with:fromFile:)bodies aren't attached, because files can be large.cancelAll()in the_didFinishWithError:hook is a safety net that none of the traced cases needed. I'm happy to drop it if you prefer.