Skip to content

Conversation

@erayaydin
Copy link
Member

@erayaydin erayaydin commented Sep 8, 2025

When agent responses containing non-ASCII characters were sent, these characters were getting corrupted on the way through the proxy. We were decoding the body into a JS string and then re-encoding it as binary, which breaks UTF-8.

❓ What Changed?

  • Removed binary/utf-8 toggle because it inferred from content-encoding header.
  • Added received body chunks as-is to the body accumulator.
  • Extra: typed the body accumulator as Buffer[] (from any[]).

⚙️ How This Works?

Keeping everything as raw buffers avoids issues and problems with string encodings. No assumptions about content-encoding. Compressed and uncompressed bodies are preserved byte by byte.

🧑‍🔬 How To Test?

1. Smoke

Build the new CloudFront worker from this branch and deploy it to your AWS environment. Try to use the agent downloading with a basic client. This test will ensure everything works fine (no breaking).

2. Use Mock Server

Use mock server and run agent response body integrity protected with 200 status code test with a CloudFront proxy integration (which points to your mock server). This test will ensure the new changes fix potential Unicode problems.

Shareable mock server form value (only test selected, empty fields):

eyJpbnRlZ3JhdGlvblVybCI6IiIsImluZ3Jlc3NQYXRoIjoiIiwiY2RuUGF0aCI6IiIsInRyYWZmaWNOYW1lIjoiIiwiaW50ZWdyYXRpb25WZXJzaW9uIjoiIiwidGVzdHNGaWx0ZXIiOlsiYWdlbnQgcmVzcG9uc2UgYm9keSBpbnRlZ3JpdHkgcHJvdGVjdGVkIHdpdGggMjAwIHN0YXR1cyBjb2RlIl19

✨ Potential Improvements

  • Don't use Buffer.isBuffer and directly push chunks to the accumulator body. (response.on('data', (chunk) => data.push(chunk)))
  • We can drop content-length from forwarded headers and let CloudFront handle the calculation.
  • We can add a new mock server e2e test cases for gzip/br compression.
  • Compressed/uncompressed test case content-length assertions.

Collect and use raw `Buffer` chunks instead of partial string concat.
Remove `setEncoding` and `binary/utf-8` toggle logic. Push Buffer chunks
as-is. Removed infering `binary` from `content-encoding` because this
header tells about compression, not the medi/content type. New logic
handles both compressed and un-compressed bodies.

Related-Task: INTER-1432
@erayaydin erayaydin self-assigned this Sep 8, 2025
@erayaydin erayaydin added the bug Something isn't working label Sep 8, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Sep 8, 2025

This PR will create a patch release 🚀

2.1.2 (2025-09-08)

Bug Fixes

@github-actions
Copy link
Contributor

github-actions bot commented Sep 8, 2025

Coverage report

St.
Category Percentage Covered / Total
🟢 Statements
92.68% (+0.1% 🔼)
646/697
🟢 Branches
87.28% (+0.5% 🔼)
151/173
🟢 Functions 91.79% 123/134
🟢 Lines
92.87% (+0.11% 🔼)
612/659

Test suite run success

170 tests passing in 18 suites.

Report generated by 🧪jest coverage report action from 29090a4

Show full coverage report
St File % Stmts % Branch % Funcs % Lines Uncovered Line #s
🟢 All files 92.68 87.28 91.79 92.86
🟢  mgmt-lambda 98.93 93.33 100 98.93
🟢   DefaultSettings.ts 100 100 100 100
🟢   app.ts 97.91 95 100 97.91 26
🟢   auth.ts 100 100 100 100
🟢   exceptions.ts 100 66.66 100 100 20
🟢   routing.ts 100 100 100 100
🟢  mgmt-lambda/handlers 87.22 72.5 93.33 87.15
🟢   errorHandlers.ts 100 71.42 100 100 22,41
🟡   statusHandler.ts 80 50 100 80 76-80,84-89
🟢   updateHandler.ts 87.21 75.86 87.5 87.12 50-51,67-68,139-142,214,219-228,290-291,319
🟢  mgmt-lambda/utils 81.25 88.88 66.66 100
🟢   cloudfrontUtils.ts 100 88.88 100 100 6
🔴   delay.ts 40 100 0 100
🟢  proxy/handlers 88.5 90.9 90.32 89.28
🟢   handleAgentDowloading.ts 100 75 100 100 29-34
🟡   handleResult.ts 76.74 100 76.92 78.04 90-105
🟢   handleStatus.ts 100 100 100 100
🟡  proxy/test 77.77 100 50 71.42
🟡   aws.ts 77.77 100 50 71.42 4-5
🟢  proxy/test/utils/customer-variables 100 100 100 100
🟢   in-memory-customer-variables.ts 100 100 100 100
🟢  proxy/utils 94.57 85.71 94.28 94.11
🟢   buffer.ts 100 100 100 100
🟢   cache-control.ts 100 100 100 100
🟢   cookie.ts 100 100 100 100
🟢   headers.ts 100 100 100 100
🔴   is-blob.ts 0 0 0 0 6-7
🟢   log.ts 85.71 33.33 100 83.33 11
🟢   request.ts 100 100 100 100
🟢   routing.ts 100 100 100 100
🔴   string.ts 14.28 100 0 14.28 2-8
🟢   traffic.ts 100 100 100 100
🟢  proxy/utils/customer-variables 100 100 100 100
🟢   customer-variables.ts 100 100 100 100
🟢   defaults.ts 100 100 100 100
🟢   header-customer-variables.ts 100 100 100 100
🟢   maybe-obfuscate-variable.ts 100 100 100 100
🟢   selectors.ts 100 100 100 100
🟢   types.ts 100 100 100 100
🟢  proxy/utils/customer-variables/secrets-manager 95.58 100 100 95.52
🟢   normalize-secret.ts 100 100 100 100
🟢   retrieve-secret.ts 100 100 100 100
🟢   secrets-manager-variables.ts 86.95 100 100 86.95 28,50-55
🟢   validate-secret.ts 100 100 100 100

@erayaydin erayaydin marked this pull request as ready for review September 8, 2025 13:23
Copy link
Contributor

@TheUnderScorer TheUnderScorer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but let's make sure that all E2E tests pass before merging :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants