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

fix: bodyLimit must be applied on fully decoded body #4969

Merged
merged 2 commits into from Aug 16, 2023

Conversation

sergburn
Copy link
Contributor

Checklist

This change ensures that bodyLimit is applied to receivedLength, i.e. the actual length of the body, which is going to be parsed. Current code compares receivedEncodedLength instead, if it is available and non-zero. This is correct behaviour for comparing against "content-length" header, but not for maximum allowed body size.

Without this change default Fastify instance with simple decompressing handler is vulnerable to some sort of "zip bomb" - it is possible to POST huge body, which exceeds bodyLimit. Provided new test case shows the problem and fails without changes in contentTypeParser.js.

@sergburn sergburn changed the title bodyLimit must be applied on fully decoded body fix: bodyLimit must be applied on fully decoded body Aug 12, 2023
@sergburn
Copy link
Contributor Author

sergburn commented Aug 12, 2023

Fixes #4970

@Eomm Eomm linked an issue Aug 13, 2023 that may be closed by this pull request
2 tasks
Copy link
Member

@Eomm Eomm left a comment

Choose a reason for hiding this comment

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

Could you update the doc as well?
It must be clear to the user that this option checks the custom preParsing output
https://fastify.dev/docs/latest/Reference/Server/#bodylimit


if ((payload.receivedEncodedLength || receivedLength) > limit) {
const { receivedEncodedLength = 0 } = payload
// first of all - resulting body length must not exceed bodyLimit (see "zip bomb")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// first of all - resulting body length must not exceed bodyLimit (see "zip bomb")
// The resulting body length must not exceed bodyLimit (see "zip bomb")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@Uzlopak
Copy link
Contributor

Uzlopak commented Aug 13, 2023

Is fastify/compress capable of dropping zip bombs?

@metcoder95
Copy link
Member

Unless I'm missing something, not out-of-the-box but might benefit from this PR:

https://github.com/fastify/fastify-compress/blob/18f2bf7fd3425aefb23327aff5dd04a61852ccea/index.js#L333-L342

@Eomm Eomm added the bugfix Issue or PR that should land as semver patch label Aug 14, 2023
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina mcollina merged commit b2f30d8 into fastify:main Aug 16, 2023
34 of 35 checks passed
Uzlopak pushed a commit that referenced this pull request Aug 21, 2023
* bodyLimit must be applied on fully decoded body

* Updated docs and comments
renovate bot added a commit to tomacheese/telcheck that referenced this pull request Aug 27, 2023
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [fastify](https://www.fastify.io/)
([source](https://togithub.com/fastify/fastify)) | [`4.21.0` ->
`4.22.0`](https://renovatebot.com/diffs/npm/fastify/4.21.0/4.22.0) |
[![age](https://developer.mend.io/api/mc/badges/age/npm/fastify/4.22.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/fastify/4.22.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/fastify/4.21.0/4.22.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/fastify/4.21.0/4.22.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>fastify/fastify (fastify)</summary>

###
[`v4.22.0`](https://togithub.com/fastify/fastify/releases/tag/v4.22.0)

[Compare
Source](https://togithub.com/fastify/fastify/compare/v4.21.0...v4.22.0)

#### What's Changed

- make FastifySchemaValidationError.params wider by
[@&#8203;cm-ayf](https://togithub.com/cm-ayf) in
[fastify/fastify#4476
- docs(ecosystem): add fastify-hashids by
[@&#8203;andersonjoseph](https://togithub.com/andersonjoseph) in
[fastify/fastify#4934
- fix: hasPlugin does not track parent plugins by
[@&#8203;Eomm](https://togithub.com/Eomm) in
[fastify/fastify#4929
- docs: early hints plugin is fastify plugin by
[@&#8203;Uzlopak](https://togithub.com/Uzlopak) in
[fastify/fastify#4947
- chore: add pull request title check by
[@&#8203;Uzlopak](https://togithub.com/Uzlopak) in
[fastify/fastify#4951
- chore: Bump
[@&#8203;sinclair/typebox](https://togithub.com/sinclair/typebox) from
0.29.6 to 0.30.2 by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[fastify/fastify#4952
- ci: improve pr title check by
[@&#8203;Uzlopak](https://togithub.com/Uzlopak) in
[fastify/fastify#4953
- ci: fix warnings in benchmark workflows by
[@&#8203;Uzlopak](https://togithub.com/Uzlopak) in
[fastify/fastify#4954
- docs: fix removeAdditional comment by
[@&#8203;G0maa](https://togithub.com/G0maa) in
[fastify/fastify#4948
- fix: Try to fix parser benchmark workflow by
[@&#8203;kibertoad](https://togithub.com/kibertoad) in
[fastify/fastify#4956
- fix: infer correct hook handler by
[@&#8203;Uzlopak](https://togithub.com/Uzlopak) in
[fastify/fastify#4945
- fix: do not double send the response if the request is destroyed but
not aborted by [@&#8203;mcollina](https://togithub.com/mcollina) in
[fastify/fastify#4963
- chore: Bump
[@&#8203;sinclair/typebox](https://togithub.com/sinclair/typebox) from
0.30.4 to 0.31.1 by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[fastify/fastify#4973
- fix: bodyLimit must be applied on fully decoded body by
[@&#8203;sergburn](https://togithub.com/sergburn) in
[fastify/fastify#4969
- chore: updates
[@&#8203;typescript-eslint/eslint-plugin](https://togithub.com/typescript-eslint/eslint-plugin)
and
[@&#8203;typescript-eslint/parser](https://togithub.com/typescript-eslint/parser)
by [@&#8203;climba03003](https://togithub.com/climba03003) in
[fastify/fastify#4977
- chore: use group dependencies in dependabot by
[@&#8203;climba03003](https://togithub.com/climba03003) in
[fastify/fastify#4979
- chore: fix ci bench by [@&#8203;Eomm](https://togithub.com/Eomm) in
[fastify/fastify#4983
- fix: require.cache is undefined breaks SEA by
[@&#8203;climba03003](https://togithub.com/climba03003) in
[fastify/fastify#4982
- ci: remove sync next wf by
[@&#8203;Uzlopak](https://togithub.com/Uzlopak) in
[fastify/fastify#4985
- docs: remove mixing ES6 and commomjs in the example by
[@&#8203;thenicolau](https://togithub.com/thenicolau) in
[fastify/fastify#4990
- fix: errorHandler callback should utilize TypeProvider by
[@&#8203;muan](https://togithub.com/muan) in
[fastify/fastify#4989
- types: add onRoute to ApplicationHookLookup by
[@&#8203;Uzlopak](https://togithub.com/Uzlopak) in
[fastify/fastify#4968
- chore: make tests pass on ipv4 only machine by
[@&#8203;mcollina](https://togithub.com/mcollina) in
[fastify/fastify#4997
- fix: Set `FastifyRequest.id` type as a string by
[@&#8203;samchungy](https://togithub.com/samchungy) in
[fastify/fastify#4992

#### New Contributors

- [@&#8203;G0maa](https://togithub.com/G0maa) made their first
contribution in
[fastify/fastify#4948
- [@&#8203;thenicolau](https://togithub.com/thenicolau) made their first
contribution in
[fastify/fastify#4990
- [@&#8203;muan](https://togithub.com/muan) made their first
contribution in
[fastify/fastify#4989
- [@&#8203;samchungy](https://togithub.com/samchungy) made their first
contribution in
[fastify/fastify#4992

**Full Changelog**:
fastify/fastify@v4.21.0...v4.22.0

</details>

---

### Configuration

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

🚦 **Automerge**: Enabled.

♻ **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 [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://developer.mend.io/github/tomacheese/telcheck).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNi41Ni4wIiwidXBkYXRlZEluVmVyIjoiMzYuNTYuMCIsInRhcmdldEJyYW5jaCI6Im1hc3RlciJ9-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
renovate bot added a commit to redwoodjs/redwood that referenced this pull request Aug 30, 2023
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [fastify](https://www.fastify.io/)
([source](https://togithub.com/fastify/fastify)) | [`4.21.0` ->
`4.22.0`](https://renovatebot.com/diffs/npm/fastify/4.21.0/4.22.0) |
[![age](https://developer.mend.io/api/mc/badges/age/npm/fastify/4.22.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/fastify/4.22.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/fastify/4.21.0/4.22.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/fastify/4.21.0/4.22.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>fastify/fastify (fastify)</summary>

###
[`v4.22.0`](https://togithub.com/fastify/fastify/releases/tag/v4.22.0)

[Compare
Source](https://togithub.com/fastify/fastify/compare/v4.21.0...v4.22.0)

##### What's Changed

- make FastifySchemaValidationError.params wider by
[@&#8203;cm-ayf](https://togithub.com/cm-ayf) in
[fastify/fastify#4476
- docs(ecosystem): add fastify-hashids by
[@&#8203;andersonjoseph](https://togithub.com/andersonjoseph) in
[fastify/fastify#4934
- fix: hasPlugin does not track parent plugins by
[@&#8203;Eomm](https://togithub.com/Eomm) in
[fastify/fastify#4929
- docs: early hints plugin is fastify plugin by
[@&#8203;Uzlopak](https://togithub.com/Uzlopak) in
[fastify/fastify#4947
- chore: add pull request title check by
[@&#8203;Uzlopak](https://togithub.com/Uzlopak) in
[fastify/fastify#4951
- chore: Bump
[@&#8203;sinclair/typebox](https://togithub.com/sinclair/typebox) from
0.29.6 to 0.30.2 by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[fastify/fastify#4952
- ci: improve pr title check by
[@&#8203;Uzlopak](https://togithub.com/Uzlopak) in
[fastify/fastify#4953
- ci: fix warnings in benchmark workflows by
[@&#8203;Uzlopak](https://togithub.com/Uzlopak) in
[fastify/fastify#4954
- docs: fix removeAdditional comment by
[@&#8203;G0maa](https://togithub.com/G0maa) in
[fastify/fastify#4948
- fix: Try to fix parser benchmark workflow by
[@&#8203;kibertoad](https://togithub.com/kibertoad) in
[fastify/fastify#4956
- fix: infer correct hook handler by
[@&#8203;Uzlopak](https://togithub.com/Uzlopak) in
[fastify/fastify#4945
- fix: do not double send the response if the request is destroyed but
not aborted by [@&#8203;mcollina](https://togithub.com/mcollina) in
[fastify/fastify#4963
- chore: Bump
[@&#8203;sinclair/typebox](https://togithub.com/sinclair/typebox) from
0.30.4 to 0.31.1 by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[fastify/fastify#4973
- fix: bodyLimit must be applied on fully decoded body by
[@&#8203;sergburn](https://togithub.com/sergburn) in
[fastify/fastify#4969
- chore: updates
[@&#8203;typescript-eslint/eslint-plugin](https://togithub.com/typescript-eslint/eslint-plugin)
and
[@&#8203;typescript-eslint/parser](https://togithub.com/typescript-eslint/parser)
by [@&#8203;climba03003](https://togithub.com/climba03003) in
[fastify/fastify#4977
- chore: use group dependencies in dependabot by
[@&#8203;climba03003](https://togithub.com/climba03003) in
[fastify/fastify#4979
- chore: fix ci bench by [@&#8203;Eomm](https://togithub.com/Eomm) in
[fastify/fastify#4983
- fix: require.cache is undefined breaks SEA by
[@&#8203;climba03003](https://togithub.com/climba03003) in
[fastify/fastify#4982
- ci: remove sync next wf by
[@&#8203;Uzlopak](https://togithub.com/Uzlopak) in
[fastify/fastify#4985
- docs: remove mixing ES6 and commomjs in the example by
[@&#8203;thenicolau](https://togithub.com/thenicolau) in
[fastify/fastify#4990
- fix: errorHandler callback should utilize TypeProvider by
[@&#8203;muan](https://togithub.com/muan) in
[fastify/fastify#4989
- types: add onRoute to ApplicationHookLookup by
[@&#8203;Uzlopak](https://togithub.com/Uzlopak) in
[fastify/fastify#4968
- chore: make tests pass on ipv4 only machine by
[@&#8203;mcollina](https://togithub.com/mcollina) in
[fastify/fastify#4997
- fix: Set `FastifyRequest.id` type as a string by
[@&#8203;samchungy](https://togithub.com/samchungy) in
[fastify/fastify#4992

##### New Contributors

- [@&#8203;G0maa](https://togithub.com/G0maa) made their first
contribution in
[fastify/fastify#4948
- [@&#8203;thenicolau](https://togithub.com/thenicolau) made their first
contribution in
[fastify/fastify#4990
- [@&#8203;muan](https://togithub.com/muan) made their first
contribution in
[fastify/fastify#4989
- [@&#8203;samchungy](https://togithub.com/samchungy) made their first
contribution in
[fastify/fastify#4992

**Full Changelog**:
fastify/fastify@v4.21.0...v4.22.0

</details>

---

### Configuration

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

🚦 **Automerge**: Enabled.

♻ **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 [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://developer.mend.io/github/redwoodjs/redwood).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNi42OC4xIiwidXBkYXRlZEluVmVyIjoiMzYuNjguMSIsInRhcmdldEJyYW5jaCI6Im1haW4ifQ==-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
jtoar pushed a commit to redwoodjs/redwood that referenced this pull request Sep 2, 2023
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [fastify](https://www.fastify.io/)
([source](https://togithub.com/fastify/fastify)) | [`4.21.0` ->
`4.22.0`](https://renovatebot.com/diffs/npm/fastify/4.21.0/4.22.0) |
[![age](https://developer.mend.io/api/mc/badges/age/npm/fastify/4.22.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/fastify/4.22.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/fastify/4.21.0/4.22.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/fastify/4.21.0/4.22.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>fastify/fastify (fastify)</summary>

###
[`v4.22.0`](https://togithub.com/fastify/fastify/releases/tag/v4.22.0)

[Compare
Source](https://togithub.com/fastify/fastify/compare/v4.21.0...v4.22.0)

##### What's Changed

- make FastifySchemaValidationError.params wider by
[@&#8203;cm-ayf](https://togithub.com/cm-ayf) in
[fastify/fastify#4476
- docs(ecosystem): add fastify-hashids by
[@&#8203;andersonjoseph](https://togithub.com/andersonjoseph) in
[fastify/fastify#4934
- fix: hasPlugin does not track parent plugins by
[@&#8203;Eomm](https://togithub.com/Eomm) in
[fastify/fastify#4929
- docs: early hints plugin is fastify plugin by
[@&#8203;Uzlopak](https://togithub.com/Uzlopak) in
[fastify/fastify#4947
- chore: add pull request title check by
[@&#8203;Uzlopak](https://togithub.com/Uzlopak) in
[fastify/fastify#4951
- chore: Bump
[@&#8203;sinclair/typebox](https://togithub.com/sinclair/typebox) from
0.29.6 to 0.30.2 by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[fastify/fastify#4952
- ci: improve pr title check by
[@&#8203;Uzlopak](https://togithub.com/Uzlopak) in
[fastify/fastify#4953
- ci: fix warnings in benchmark workflows by
[@&#8203;Uzlopak](https://togithub.com/Uzlopak) in
[fastify/fastify#4954
- docs: fix removeAdditional comment by
[@&#8203;G0maa](https://togithub.com/G0maa) in
[fastify/fastify#4948
- fix: Try to fix parser benchmark workflow by
[@&#8203;kibertoad](https://togithub.com/kibertoad) in
[fastify/fastify#4956
- fix: infer correct hook handler by
[@&#8203;Uzlopak](https://togithub.com/Uzlopak) in
[fastify/fastify#4945
- fix: do not double send the response if the request is destroyed but
not aborted by [@&#8203;mcollina](https://togithub.com/mcollina) in
[fastify/fastify#4963
- chore: Bump
[@&#8203;sinclair/typebox](https://togithub.com/sinclair/typebox) from
0.30.4 to 0.31.1 by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[fastify/fastify#4973
- fix: bodyLimit must be applied on fully decoded body by
[@&#8203;sergburn](https://togithub.com/sergburn) in
[fastify/fastify#4969
- chore: updates
[@&#8203;typescript-eslint/eslint-plugin](https://togithub.com/typescript-eslint/eslint-plugin)
and
[@&#8203;typescript-eslint/parser](https://togithub.com/typescript-eslint/parser)
by [@&#8203;climba03003](https://togithub.com/climba03003) in
[fastify/fastify#4977
- chore: use group dependencies in dependabot by
[@&#8203;climba03003](https://togithub.com/climba03003) in
[fastify/fastify#4979
- chore: fix ci bench by [@&#8203;Eomm](https://togithub.com/Eomm) in
[fastify/fastify#4983
- fix: require.cache is undefined breaks SEA by
[@&#8203;climba03003](https://togithub.com/climba03003) in
[fastify/fastify#4982
- ci: remove sync next wf by
[@&#8203;Uzlopak](https://togithub.com/Uzlopak) in
[fastify/fastify#4985
- docs: remove mixing ES6 and commomjs in the example by
[@&#8203;thenicolau](https://togithub.com/thenicolau) in
[fastify/fastify#4990
- fix: errorHandler callback should utilize TypeProvider by
[@&#8203;muan](https://togithub.com/muan) in
[fastify/fastify#4989
- types: add onRoute to ApplicationHookLookup by
[@&#8203;Uzlopak](https://togithub.com/Uzlopak) in
[fastify/fastify#4968
- chore: make tests pass on ipv4 only machine by
[@&#8203;mcollina](https://togithub.com/mcollina) in
[fastify/fastify#4997
- fix: Set `FastifyRequest.id` type as a string by
[@&#8203;samchungy](https://togithub.com/samchungy) in
[fastify/fastify#4992

##### New Contributors

- [@&#8203;G0maa](https://togithub.com/G0maa) made their first
contribution in
[fastify/fastify#4948
- [@&#8203;thenicolau](https://togithub.com/thenicolau) made their first
contribution in
[fastify/fastify#4990
- [@&#8203;muan](https://togithub.com/muan) made their first
contribution in
[fastify/fastify#4989
- [@&#8203;samchungy](https://togithub.com/samchungy) made their first
contribution in
[fastify/fastify#4992

**Full Changelog**:
fastify/fastify@v4.21.0...v4.22.0

</details>

---

### Configuration

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

🚦 **Automerge**: Enabled.

♻ **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 [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://developer.mend.io/github/redwoodjs/redwood).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNi42OC4xIiwidXBkYXRlZEluVmVyIjoiMzYuNjguMSIsInRhcmdldEJyYW5jaCI6Im1haW4ifQ==-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Issue or PR that should land as semver patch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Content reader may accept encoded body larger than bodyLimit
5 participants