Skip to content

middleware: fix httpFancyWriter.ReadFrom double-counting bytes with Tee#1085

Merged
VojtechVitek merged 1 commit into
go-chi:masterfrom
alliasgher:fix-httpfancywriter-readfrom-double-count
May 16, 2026
Merged

middleware: fix httpFancyWriter.ReadFrom double-counting bytes with Tee#1085
VojtechVitek merged 1 commit into
go-chi:masterfrom
alliasgher:fix-httpfancywriter-readfrom-double-count

Conversation

@alliasgher

Copy link
Copy Markdown
Contributor

Summary

httpFancyWriter.ReadFrom in middleware/wrap_writer.go double-counted BytesWritten() when a Tee writer was set.

When tee != nil, the code called io.Copy(&f.basicWriter, r), which routes every write through basicWriter.Write. Write already increments basicWriter.bytes. After the copy returned, the old code then did:

f.basicWriter.bytes += int(n)   // BUG: already counted inside Write

…adding n a second time. The result: BytesWritten() returned exactly twice the actual number of bytes transferred.

Fix

Remove the redundant f.basicWriter.bytes += int(n) line in the tee branch. The non-tee path (which delegates to the underlying ReaderFrom, bypassing Write) is not affected and still needs to add n itself.

Test

Added TestHttpFancyWriterReadFromByteCountWithTee that reproduces the original bug (it fails before the fix and passes after). The full middleware test suite still passes.

Fixes #1067

When a Tee writer is set, ReadFrom calls io.Copy(&f.basicWriter, r),
which routes each write through basicWriter.Write. Write already
increments basicWriter.bytes, so the line

    f.basicWriter.bytes += int(n)

that followed the io.Copy double-counted every byte, causing
BytesWritten() to return twice the actual value.

Remove the redundant addition. The non-tee path is unaffected because
it delegates to the underlying ResponseWriter's ReaderFrom (bypassing
Write) and must still add n itself.

Fixes go-chi#1067

Signed-off-by: alliasgher <alliasgher123@gmail.com>

@VojtechVitek VojtechVitek left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM. Thank you for the fix!

@VojtechVitek VojtechVitek merged commit 4ef87ea into go-chi:master May 16, 2026
eleboucher pushed a commit to eleboucher/apoci that referenced this pull request May 22, 2026
…(#49)

This PR contains the following updates:

| Package | Change | [Age](https://docs.renovatebot.com/merge-confidence/) | [Confidence](https://docs.renovatebot.com/merge-confidence/) |
|---|---|---|---|
| [github.com/go-chi/chi/v5](https://github.com/go-chi/chi) | `v5.2.5` → `v5.3.0` | ![age](https://developer.mend.io/api/mc/badges/age/go/github.com%2fgo-chi%2fchi%2fv5/v5.3.0?slim=true) | ![confidence](https://developer.mend.io/api/mc/badges/confidence/go/github.com%2fgo-chi%2fchi%2fv5/v5.2.5/v5.3.0?slim=true) |

---

### Release Notes

<details>
<summary>go-chi/chi (github.com/go-chi/chi/v5)</summary>

### [`v5.3.0`](https://github.com/go-chi/chi/releases/tag/v5.3.0)

[Compare Source](go-chi/chi@v5.2.5...v5.3.0)

#### What's Changed

- Use strings.ReplaceAll where applicable by [@&#8203;JRaspass](https://github.com/JRaspass) in [#&#8203;1046](go-chi/chi#1046)
- Propagate inline middlewares across mounted subrouters by [@&#8203;LukasJenicek](https://github.com/LukasJenicek) in [#&#8203;1049](go-chi/chi#1049)
- add go 1.26 to ci by [@&#8203;pkieltyka](https://github.com/pkieltyka) in [#&#8203;1052](go-chi/chi#1052)
- Remove last uses of io/ioutil by [@&#8203;JRaspass](https://github.com/JRaspass) in [#&#8203;1054](go-chi/chi#1054)
- Simplify chi.walk with slices.Concat by [@&#8203;JRaspass](https://github.com/JRaspass) in [#&#8203;1053](go-chi/chi#1053)
- Apply the stringscutprefix modernizer by [@&#8203;JRaspass](https://github.com/JRaspass) in [#&#8203;1051](go-chi/chi#1051)
- Bump minimum Go to 1.23, always use request.Pattern by [@&#8203;JRaspass](https://github.com/JRaspass) in [#&#8203;1048](go-chi/chi#1048)
- middleware: fix httpFancyWriter.ReadFrom double-counting bytes with Tee by [@&#8203;alliasgher](https://github.com/alliasgher) in [#&#8203;1085](go-chi/chi#1085)
- Fix typo in Route doc comment by [@&#8203;gouwazi](https://github.com/gouwazi) in [#&#8203;1073](go-chi/chi#1073)
- fix: set Request.Pattern from RoutePattern() by [@&#8203;leno23](https://github.com/leno23) in [#&#8203;1097](go-chi/chi#1097)
- feat: middleware.ClientIP, a replacement for middleware.RealIP by [@&#8203;VojtechVitek](https://github.com/VojtechVitek) in [#&#8203;967](go-chi/chi#967)

#### New Contributors

- [@&#8203;LukasJenicek](https://github.com/LukasJenicek) made their first contribution in [#&#8203;1049](go-chi/chi#1049)
- [@&#8203;alliasgher](https://github.com/alliasgher) made their first contribution in [#&#8203;1085](go-chi/chi#1085)
- [@&#8203;gouwazi](https://github.com/gouwazi) made their first contribution in [#&#8203;1073](go-chi/chi#1073)
- [@&#8203;leno23](https://github.com/leno23) made their first contribution in [#&#8203;1097](go-chi/chi#1097)

#### SECURITY: middleware.ClientIP, a replacement for middleware.RealIP

PR [#&#8203;967](go-chi/chi#967) introduced `middleware.ClientIP`, a replacement for `middleware.RealIP` that closes the three open spoofing advisories:

- [GHSA-9g5q-2w5x-hmxf](GHSA-9g5q-2w5x-hmxf) — IP spoofing via XFF in `RemoteAddr` resolution (convto)
- [GHSA-rjr7-jggh-pgcp](GHSA-rjr7-jggh-pgcp) — RealIP allows IP spoofing via unvalidated XFF (rezmoss)
- [GHSA-3fxj-6jh8-hvhx](GHSA-3fxj-6jh8-hvhx) — IP spoofing in `middleware.RealIP` (Saku0512, Critical / 9.3)

It also addresses issues outlined at:

- [#&#8203;708](go-chi/chi#708)
- <https://adam-p.ca/blog/2022/03/x-forwarded-for/>
- [#&#8203;711](go-chi/chi#711)
- [#&#8203;453](go-chi/chi#453)
- [#&#8203;908](go-chi/chi#908)

`middleware.RealIP` is deprecated in this PR with pointers to the new API.

The deprecation only adds a `// Deprecated:` doc comment; the function keeps working for backward compatibility.

##### Why a new middleware (not "fix RealIP in place")

`RealIP` has two unfixable design choices: it mutates `r.RemoteAddr`, and it tries to be a one-size-fits-all default by walking a hard-coded list of headers any client can supply. Per [adam-p's "The perils of the 'real' client IP"](https://adam-p.ca/blog/2022/03/x-forwarded-for/) (which calls chi out by name on this), there is no safe default — the user must pick their trust source explicitly.

##### The new API

Four middlewares, two accessors. Pick exactly one middleware based on your
infrastructure, read the result with one of the two accessors:

```go
// One of the four. There is no safe default — pick exactly one.
func ClientIPFromHeader(trustedHeader string) func(http.Handler) http.Handler
func ClientIPFromXFF(trustedIPPrefixes ...string) func(http.Handler) http.Handler
func ClientIPFromXFFTrustedProxies(numTrustedProxies int) func(http.Handler) http.Handler
func ClientIPFromRemoteAddr(h http.Handler) http.Handler

// Read the result.
func GetClientIP(ctx context.Context) string         // for logs, rate-limit keys
func GetClientIPAddr(ctx context.Context) netip.Addr // for typed work
```

#### Example usage:

```go
// Pick a single ClientIP middleware based on your deployment

// Cloudflare.
r.Use(middleware.ClientIPFromHeader("CF-Connecting-IP"))

// Nginx with ngx_http_realip_module.
r.Use(middleware.ClientIPFromHeader("X-Real-IP"))

// Apache with mod_remoteip.
r.Use(middleware.ClientIPFromHeader("X-Client-IP"))

// AWS CloudFront, or any proxy fleet with known CIDRs.
r.Use(middleware.ClientIPFromXFF(
    "13.32.0.0/15",   // CloudFront IPv4
    "52.46.0.0/18",   // CloudFront IPv4
    "2600:9000::/28", // CloudFront IPv6
))

// Behind exactly 2 trusted proxies with dynamic IPs (autoscaling pools,
// ephemeral containers, dynamic CDN edges).
r.Use(middleware.ClientIPFromXFFTrustedProxies(2))

// Server directly on the public internet, no proxy in front.
r.Use(middleware.ClientIPFromRemoteAddr)
```

And in your handler or downstream middleware:

```go
clientIP := middleware.GetClientIP(r.Context())
// log it, use it as a rate-limit key, etc.
```

***

Thanks to [@&#8203;adam-p](https://github.com/adam-p), [@&#8203;c2h5oh](https://github.com/c2h5oh), [@&#8203;rezmoss](https://github.com/rezmoss), [@&#8203;Saku0512](https://github.com/Saku0512), [@&#8203;convto](https://github.com/convto), [@&#8203;Dirbaio](https://github.com/Dirbaio), [@&#8203;jawnsy](https://github.com/jawnsy), [@&#8203;lrstanley](https://github.com/lrstanley), [@&#8203;mfridman](https://github.com/mfridman), [@&#8203;n33pm](https://github.com/n33pm), [@&#8203;pkieltyka](https://github.com/pkieltyka) for the prior discussions, detailed reviews, advisory reports, and test contributions that shaped this PR.

**Full Changelog**: <go-chi/chi@v5.2.5...v5.3.0>

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0My4xMDEuMSIsInVwZGF0ZWRJblZlciI6IjQzLjEwMS4xIiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6WyJ0eXBlL21pbm9yIl19-->

Reviewed-on: https://git.erwanleboucher.dev/eleboucher/apoci/pulls/49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

httpFancyWriter.ReadFrom double-counts BytesWritten when Tee is set

2 participants