Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Prioritized] Encoded body fragments are decoded appropriately within multipart/mixed payloads #2900

Open
scbedd opened this issue Mar 10, 2022 · 4 comments
Assignees
Labels
Central-EngSys This issue is owned by the Engineering System team. Test-Proxy Anything relating to test-proxy requests or issues.

Comments

@scbedd
Copy link
Member

scbedd commented Mar 10, 2022

Follow-up issue to #2784 and it's PR #2863

multipart/mixed body payloads look like:

--batch_00000000-0000-0000-0000-000000000000
Content-Type: multipart/mixed; boundary=changeset_2deb0284-577b-4bc5-9cd8-594df317b3d7

--changeset_2deb0284-577b-4bc5-9cd8-594df317b3d7
Content-Type: application/http
Content-Transfer-Encoding: binary
Content-ID: 0

POST https://fakeendpoint.table.core.windows.net/uttable913d2aeb HTTP/1.1
x-ms-version: 2019-02-02
DataServiceVersion: 3.0
Prefer: return-no-content
Content-Type: application/json;odata=nometadata
Accept: application/json;odata=minimalmetadata
Content-Length: 317
x-ms-date: Thu, 09 Dec 2021 00:21:32 GMT
Date: Thu, 09 Dec 2021 00:21:32 GMT

{"PartitionKey": "003", "[email protected]": "Edm.String", "RowKey": "batch_all_operations_together", "[email protected]": "Edm.String", "test": true, "test2": "value", "[email protected]": "Edm.String", "test3": 3, "test4": 1234567890, "test5": "2021-12-09T08:21:32.040478Z", "[email protected]": "Edm.DateTime"}
--changeset_2deb0284-577b-4bc5-9cd8-594df317b3d7
Content-Type: application/http
Content-Transfer-Encoding: binary
Content-ID: 1

DELETE https://fakeendpoint.table.core.windows.net/uttable913d2aeb(PartitionKey='003',RowKey='batch_all_operations_together-1') HTTP/1.1
x-ms-version: 2019-02-02
DataServiceVersion: 3.0
If-Match: *
Accept: application/json;odata=minimalmetadata
x-ms-date: Thu, 09 Dec 2021 00:21:32 GMT
Date: Thu, 09 Dec 2021 00:21:32 GMT


--changeset_2deb0284-577b-4bc5-9cd8-594df317b3d7
...

The only gap here is if the content itself is base64encoded (like in the case of binary streams, and the like). We probably need to account for this and do a "second order decode" per body fragment.

This issue will include re-enabling the multipart/mixed sanitization. We have decided to back it out until this issue is resolved.

@ghost ghost added the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label Mar 10, 2022
@JoshLove-msft
Copy link
Member

As part of this, we should apply the default sanitizers to the individual sections of the content, e.g. Authorization headers should be sanitized.

@kurtzeborn kurtzeborn added Central-EngSys This issue is owned by the Engineering System team. and removed needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. labels Mar 14, 2022
@scbedd scbedd added the Test-Proxy Anything relating to test-proxy requests or issues. label May 11, 2022
@timovv
Copy link
Member

timovv commented Jul 19, 2022

Recently I was looking at rerecording some of our tables tests for this PR: Azure/azure-sdk-for-js#22562. They were originally recorded before this issue arose. Many of the recordings in the suite (batch operations) relied on multipart content being sanitized properly. Since this functionality has been disabled for the time being I couldn't get the correct sanitization without going in and 'sanitizing' the recording myself, which is what I ended up doing to unblock the PR.

It looks like we can work around the issue in the medium term by skipping recording the request body, but unfortunately, we don't have support for that yet in JS, although I do have a PR in the works. Longer term, it would be great to take another look at this issue so we can get multipart/mixed sanitization working again.

@mccoyp
Copy link
Member

mccoyp commented Aug 5, 2022

Chiming in on Python's part: batch operations in Tables work in playback today because tests were recorded when multipart/mixed body sanitization was functional. However, now that the Storage team is migrating their tests, sanitizing the random UUID boundaries in Content-Type headers -- like we did for Tables -- produces deserialization errors in playback. The boundaries in the multipart/mixed body aren't sanitized, so the content can't get split based on the sanitized header value.

The solution to this seemed to be: ignore the Content-Type header that contains the random UUID boundary, and turn off body matching. But account keys are included in body content, so even though tests pass with this setup, recordings contain (base64-encoded) secrets. We could try sanitizing bodies out of recordings, but we need the body content in order to run the tests.

All that is to say that for the time being, the Python Storage team will have to either make these tests live-only or manually sanitize recordings in order to use the test proxy for batch operation tests.

@scbedd
Copy link
Member Author

scbedd commented Aug 8, 2022

Ok @mccoyp please have users fall back on live-only for now. (or delay merge of the PR that transitions those tests)

I will have a fix for this out the door soon.

@scbedd scbedd moved this from 🤔Triage to 🐝 Dev in Azure SDK EngSys 🤖🧠 Aug 8, 2022
@scbedd scbedd changed the title [Test-Proxy] Encoded body fragments are decoded appropriately within multipart/mixed payloads [Prioritize] Encoded body fragments are decoded appropriately within multipart/mixed payloads Feb 9, 2023
@scbedd scbedd changed the title [Prioritize] Encoded body fragments are decoded appropriately within multipart/mixed payloads [Prioritized] Encoded body fragments are decoded appropriately within multipart/mixed payloads Feb 9, 2023
@scbedd scbedd moved this from 🐝 Dev to 📋Backlog in Azure SDK EngSys 🤖🧠 Mar 1, 2023
@scbedd scbedd moved this from 📋 Backlog to 🐝 Dev in Azure SDK EngSys 🤖🧠 Feb 6, 2024
@scbedd scbedd moved this from 🐝 Dev to 📋 Backlog in Azure SDK EngSys 🤖🧠 Jan 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Central-EngSys This issue is owned by the Engineering System team. Test-Proxy Anything relating to test-proxy requests or issues.
Projects
Status: 📋 Backlog
Development

No branches or pull requests

5 participants