feat(Client): add max response size option

[mateonunez]

This PR implements #1692 by introducing a new client option: maxResponseSize.

The maxResponseSize parameter is set to 0 which means whatever size is allowed, otherwise, the size will be considered in bytes.

There is a new check before the Parser computes the bytesRead. If that computation is greater than the maxResponseSize the socket is destroyed by the corresponding error.

    if (maxResponseSize !== 0 && this.bytesRead + buf.length > maxResponseSize) {
      util.destroy(socket, new ResponseExceededMaxSizeError())
      return -1
    }

This approach enforces the Client to verify large responses and to close the socket before the buffer is completed.

This implementation surf in a low-level way, so tests (such as ResponseContentLengthMismatchError) are covered by scenarios in test/errors.js

[codecov-commenter]

Codecov Report

Base: 89.58% // Head: 89.59% // Increases project coverage by +0.01% 🎉

Coverage data is based on head (ce2df02) compared to base (6a20cb4).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1729      +/-   ##
==========================================
+ Coverage   89.58%   89.59%   +0.01%     
==========================================
  Files          58       58              
  Lines        5262     5278      +16     
==========================================
+ Hits         4714     4729      +15     
- Misses        548      549       +1     

Impacted Files	Coverage Δ
lib/core/symbols.js	100.00% <ø> (ø)
lib/client.js	97.53% <100.00%> (+0.02%)	⬆️
lib/core/errors.js	100.00% <100.00%> (ø)
lib/fetch/index.js	83.64% <0.00%> (-0.07%)	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[mcollina]

Thanks for opening a PR! Can you please add a unit test?

[KhafraDev]

Should 0 be the default - is there ever a possibility that someone would want to ensure a response has an empty body? If so, -1 would be a better default for unlimited body size.

[mateonunez]

Should 0 be the default - is there ever a possibility that someone would want to ensure a response has an empty body? If so, -1 would be a better default for unlimited body size.

I fully agree with you, I changed the default value to -1 and added a test to check even an empty payload.

[mateonunez]

Thanks for the review @ronag and @KhafraDev 🙏. I made the requested changes, disabling the lint rule and fixing the maxResponseSize assignment.

[nosovk]

@mateonunez can you please add maxResponseSize to fetch options also, to close #1692?

[KhafraDev]

This should not be added to fetch options, to keep in line with the fetch spec. You should use it as such instead:

import { Agent, fetch, setGlobalDispatcher } from 'undici'

const agent = new Agent({
  maxResponseSize: 5
})

// either:
setGlobalDispatcher(agent)

await fetch('http://localhost:3000')

// or
await fetch('https://localhost:3000', {
  dispatcher: agent
})