Skip to content

progress: count amount of data "delivered" to application#20787

Closed
bagder wants to merge 1 commit intomasterfrom
bagder/max-unzip
Closed

progress: count amount of data "delivered" to application#20787
bagder wants to merge 1 commit intomasterfrom
bagder/max-unzip

Conversation

@bagder
Copy link
Copy Markdown
Member

@bagder bagder commented Mar 2, 2026

... and apply the CURLOPT_MAXFILESIZE limit (if set) on that as well. This effectively protects the user against "zip bombs".

Test case 1618 verifies using a 14 byte brotli payload that otherwise explodes to 102400 zero bytes.

  • document
  • add CURLINFO_SIZE_DELIVERED
  • document CURLINFO_SIZE_DELIVERED
  • add size_delivered to -w
  • document size_delivered
  • test size_delivered

@bagder bagder added the feature-window A merge of this requires an open feature window label Mar 2, 2026
@github-actions github-actions bot added the tests label Mar 2, 2026
@bagder
Copy link
Copy Markdown
Member Author

bagder commented Mar 2, 2026

@aisle-analyzer

@bagder bagder marked this pull request as ready for review March 2, 2026 10:21
@aisle-research-bot

This comment was marked as resolved.

@bagder bagder marked this pull request as draft March 2, 2026 10:24
@bagder bagder requested a review from Copilot March 2, 2026 10:34
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR introduces tracking for the amount of response BODY data actually delivered to the application and applies CURLOPT_MAXFILESIZE to that delivered size as well, preventing compressed “bomb” responses from expanding far beyond the configured limit.

Changes:

  • Add a new Progress.deliver counter and a Curl_pgrs_deliver_inc() helper to track delivered-to-application bytes and enforce max_filesize.
  • Enforce the delivered-byte limit in the client writer (cw-out) when writing BODY data to the application callback.
  • Add test case test1618 to validate the behavior using a small brotli payload that expands massively.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
tests/data/test1618 New regression test for brotli expansion exceeding --max-filesize.
tests/data/Makefile.am Registers test1618 in the test data build list.
lib/urldata.h Adds Progress.deliver counter field.
lib/progress.h Declares Curl_pgrs_deliver_inc() API.
lib/progress.c Initializes/reset delivered counter and implements increment+limit check helper.
lib/cw-out.c Hooks delivered-byte accounting/limit enforcement into BODY writes to the application callback.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@bagder
Copy link
Copy Markdown
Member Author

bagder commented Mar 2, 2026

@aisle-analyzer

@aisle-research-bot
Copy link
Copy Markdown

aisle-research-bot bot commented Mar 2, 2026

🔒 Aisle Security Analysis

✅ We scanned this PR and did not find any security vulnerabilities.

Aisle supplements but does not replace security review.


Analyzed PR: #20787 at commit 6b3d36e

Last updated on: 2026-03-02T12:56:37Z

@bagder
Copy link
Copy Markdown
Member Author

bagder commented Mar 2, 2026

augment review

@augmentcode
Copy link
Copy Markdown

augmentcode bot commented Mar 2, 2026

🤖 Augment PR Summary

Summary: This PR tracks how many bytes are actually delivered to the application (after decoding) and enforces CURLOPT_MAXFILESIZE(_LARGE) against that to mitigate decompression “zip bomb” cases.

Changes:

  • Adds a new progress.deliver counter with helper APIs to check/increment delivered bytes
  • Integrates delivered-byte accounting into the cw-out client writer for BODY output
  • Adds regression test 1618 (brotli expands beyond the max-filesize threshold)

Technical Notes: Delivered size can differ from on-the-wire download size for compressed responses; this PR applies the limit on the delivered (expanded) bytes.

🤖 Was this summary useful? React with 👍 or 👎

Copy link
Copy Markdown

@augmentcode augmentcode bot left a comment

Choose a reason for hiding this comment

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

Review completed. 2 suggestions posted.

Fix All in Augment

Comment augment review to trigger a new review at any time.

Copy link
Copy Markdown

@augmentcode augmentcode bot left a comment

Choose a reason for hiding this comment

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

Review completed. 2 suggestions posted.

Fix All in Augment

Comment augment review to trigger a new review at any time.

@bagder bagder marked this pull request as ready for review March 2, 2026 21:34
@testclutch
Copy link
Copy Markdown

Analysis of PR #20787 at 84333580:

Test 220 failed, which has NOT been flaky recently, so there could be a real issue in this PR. Note that this test has failed in 60 different CI jobs (the link just goes to one of them). Note that this CI job has had a number of other flaky tests recently (9, to be exact) so it may be that this failure is rather a systemic issue with this job and not with this specific PR.

Generated by Testclutch

@bagder bagder force-pushed the bagder/max-unzip branch from 8433358 to a0b1c3f Compare March 3, 2026 11:17
@bagder
Copy link
Copy Markdown
Member Author

bagder commented Mar 3, 2026

@codex review

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a0b1c3f35d

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@bagder bagder force-pushed the bagder/max-unzip branch from 4ed858e to d964b0f Compare March 10, 2026 22:38
bagder added a commit that referenced this pull request Mar 21, 2026
... and apply the CURLOPT_MAXFILESIZE limit (if set) on that as well.
This effectively protects the user against "zip bombs".

Test case 1618 verifies using a 14 byte brotli payload that otherwise
explodes to 102400 zero bytes.

Closes #20787
@bagder bagder force-pushed the bagder/max-unzip branch from d964b0f to 7874f42 Compare March 21, 2026 11:34
... and apply the CURLOPT_MAXFILESIZE limit (if set) on that as well.
This effectively protects the user against "zip bombs".

Test case 1618 verifies using a 14 byte brotli payload that otherwise
explodes to 102400 zero bytes.

Closes #20787
@bagder bagder force-pushed the bagder/max-unzip branch from 7874f42 to a417ec8 Compare March 21, 2026 22:04
@bagder bagder closed this in 77ed315 Mar 21, 2026
@bagder bagder deleted the bagder/max-unzip branch March 21, 2026 22:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cmdline tool feature-window A merge of this requires an open feature window libcurl API tests

Development

Successfully merging this pull request may close these issues.

3 participants