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

feat: add request.routeOptions object #4397

Merged
merged 20 commits into from Nov 5, 2022
Merged

feat: add request.routeOptions object #4397

merged 20 commits into from Nov 5, 2022

Conversation

debadutta98
Copy link
Contributor

@debadutta98 debadutta98 commented Nov 2, 2022

Hi,

This PR Closes #4375

Work

  • Add request.routeOptions.bodyLimit to request (update request.js)
  • Add some test cases of current changes to bodyLimit.test.js
  • Update type declaration request.d.ts

Checklist

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

Copy link
Member

@climba03003 climba03003 left a comment

Choose a reason for hiding this comment

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

I do not block if it landed.
Still, I think mapping the options to property one by one is not a good idea.

Details on #4375 (comment)

docs/Reference/Request.md Outdated Show resolved Hide resolved
lib/request.js Outdated Show resolved Hide resolved
lib/request.js Outdated Show resolved Hide resolved
test/bodyLimit.test.js Outdated Show resolved Hide resolved
test/bodyLimit.test.js Outdated Show resolved Hide resolved
test/bodyLimit.test.js Outdated Show resolved Hide resolved
test/bodyLimit.test.js Outdated Show resolved Hide resolved
test/bodyLimit.test.js Outdated Show resolved Hide resolved
docs/Reference/Request.md Outdated Show resolved Hide resolved
@Eomm Eomm added the semver-minor Issue or PR that should land as semver minor label Nov 3, 2022
@debadutta98
Copy link
Contributor Author

Hi @Eomm @climba03003 ,

I just changed request.routeBodyLimit to request.routeOptions.bodyLimit according to that doc and test cases are also
please confirm if it needs further improvement

docs/Reference/Request.md Show resolved Hide resolved
docs/Reference/Request.md Outdated Show resolved Hide resolved
lib/request.js Outdated Show resolved Hide resolved
Copy link
Member

@jsumners jsumners left a comment

Choose a reason for hiding this comment

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

This does not fully consider, nor implement, the ideas presented in the issue discussion.

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

lib/request.js Outdated Show resolved Hide resolved
lib/request.js Outdated Show resolved Hide resolved
lib/request.js Outdated Show resolved Hide resolved
@debadutta98
Copy link
Contributor Author

Hi guys,

Just validate my solution to match your expectations

  • Add some new property request.routeOptions and they are only readable (if you want to include more property please let me know)
  • As introduced some changes to lib/request.js that way I write some test cases on request-error.test.js and modify the types of routeOptions on types/request.d.ts
  • Update doc on doc/References/Request.md

Thank you!

lib/request.js Outdated Show resolved Hide resolved
lib/request.js Show resolved Hide resolved
lib/request.js Outdated Show resolved Hide resolved
docs/Reference/Request.md Outdated Show resolved Hide resolved
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.

AWESOME!

@Eomm Eomm changed the title [Feature] Add routeBodyLimit to request feat: add request.routeOptions object Nov 5, 2022
@Uzlopak
Copy link
Contributor

Uzlopak commented Nov 5, 2022

Can we modify the test in request-error.js to this please?
Just in case we have in future some routeOption being an Object or Array, we could overlook, that Object.freeze does not cover them properly.

test('request.routeOptions should be immutable', t => {
  t.plan(14)
  const fastify = Fastify()
  const handler = function (req, res) {
    t.equal('POST', req.routeOptions.method)
    t.equal('/', req.routeOptions.url)
    t.throws(() => { req.routeOptions = null }, new TypeError('Cannot set property routeOptions of #<Request> which has only a getter'))
    t.throws(() => { req.routeOptions.method = 'INVALID' }, new TypeError('Cannot assign to read only property \'method\' of object \'#<Object>\''))
    t.throws(() => { req.routeOptions.url = '//' }, new TypeError('Cannot assign to read only property \'url\' of object \'#<Object>\''))
    t.throws(() => { req.routeOptions.bodyLimit = 0xDEADBEEF }, new TypeError('Cannot assign to read only property \'bodyLimit\' of object \'#<Object>\''))
    t.throws(() => { req.routeOptions.attachValidation = true }, new TypeError('Cannot assign to read only property \'attachValidation\' of object \'#<Object>\''))
    t.throws(() => { req.routeOptions.logLevel = 'invalid' }, new TypeError('Cannot assign to read only property \'logLevel\' of object \'#<Object>\''))
    t.throws(() => { req.routeOptions.version = '95.0.1' }, new TypeError('Cannot assign to read only property \'version\' of object \'#<Object>\''))
    t.throws(() => { req.routeOptions.prefixTrailingSlash = true }, new TypeError('Cannot assign to read only property \'prefixTrailingSlash\' of object \'#<Object>\''))
    t.throws(() => { req.routeOptions.newAttribute = {} }, new TypeError('Cannot add property newAttribute, object is not extensible'))

    for (const key of Object.keys(req.routeOptions)) {
      if (typeof req.routeOptions[key] === 'object' && req.routeOptions[key] !== null) {
        t.fail('Object.freeze must run recursively on nested structures to ensure that routeOptions is immutable.')
      }
    }

    res.send({ })
  }
  fastify.post('/', {
    bodyLimit: 1000,
    handler
  })
  fastify.listen({ port: 0 }, function (err) {
    t.error(err)
    t.teardown(() => { fastify.close() })

    sget({
      method: 'POST',
      url: 'http://localhost:' + fastify.server.address().port,
      headers: { 'Content-Type': 'application/json' },
      body: [],
      json: true
    }, (err, response, body) => {
      t.error(err)
      t.equal(response.statusCode, 200)
    })
  })
})

@debadutta98
Copy link
Contributor Author

debadutta98 commented Nov 5, 2022

Can we modify the test in request-error.js to this please? Just in case we have in future some routeOption being an Object or Array, we could overlook, that Object.freeze does not cover them properly.

Thank you!!

Your solution has been implemented

@climba03003
Copy link
Member

Object.freeze does not cover them properly.

Probably the one would be constraints.

@mcollina mcollina merged commit f1bd80e into fastify:main Nov 5, 2022
@metcoder95 metcoder95 mentioned this pull request Dec 7, 2022
2 tasks
Skn0tt pushed a commit to quirrel-dev/quirrel that referenced this pull request Jun 20, 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.9.2` ->
`4.10.2`](https://renovatebot.com/diffs/npm/fastify/4.9.2/4.10.2) |
[![age](https://badges.renovateapi.com/packages/npm/fastify/4.10.2/age-slim)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://badges.renovateapi.com/packages/npm/fastify/4.10.2/adoption-slim)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://badges.renovateapi.com/packages/npm/fastify/4.10.2/compatibility-slim/4.9.2)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://badges.renovateapi.com/packages/npm/fastify/4.10.2/confidence-slim/4.9.2)](https://docs.renovatebot.com/merge-confidence/)
|

### GitHub Vulnerability Alerts

####
[CVE-2022-41919](https://togithub.com/fastify/fastify/security/advisories/GHSA-3fjj-p79j-c9hh)

### Impact

The attacker can use the incorrect `Content-Type` to bypass the
`Pre-Flight` checking of `fetch`. `fetch()` requests with Content-Type’s
[essence](https://mimesniff.spec.whatwg.org/#mime-type-essence) as
"application/x-www-form-urlencoded", "multipart/form-data", or
"text/plain", could potentially be used to invoke routes that only
accepts `application/json` content type, thus bypassing any [CORS
protection](https://fetch.spec.whatwg.org/#simple-header), and therefore
they could lead to a Cross-Site Request Forgery attack.

### Patches
For `4.x` users, please update to at least `4.10.2`
For `3.x` users, please update to at least `3.29.4`

### Workarounds

Implement Cross-Site Request Forgery protection using
[`@fastify/csrf`](https://www.npmjs.com/package/@&#8203;fastify/csrf).

### References

Check out the HackerOne report: https://hackerone.com/reports/1763832.

### For more information

[Fastify security
policy](https://togithub.com/fastify/fastify/security/policy)

---

### Release Notes

<details>
<summary>fastify/fastify</summary>

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

[Compare
Source](https://togithub.com/fastify/fastify/compare/v4.10.1...v4.10.2)

#### ⚠️ Security Release ⚠️

- Fix for ["Incorrect Content-Type parsing can lead to CSRF
attack"](https://togithub.com/fastify/fastify/security/advisories/GHSA-3fjj-p79j-c9hh)
    and CVE-2022-41919

**Full Changelog**:
fastify/fastify@v4.10.1...v4.10.2

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

[Compare
Source](https://togithub.com/fastify/fastify/compare/v4.10.0...v4.10.1)

#### What's Changed

- fix node 19.1.0 port validation test by
[@&#8203;Uzlopak](https://togithub.com/Uzlopak) in
[fastify/fastify#4427
- Add fastify-constraints to community plugins by
[@&#8203;Ceres6](https://togithub.com/Ceres6) in
[fastify/fastify#4428
- build(deps-dev): bump
[@&#8203;sinonjs/fake-timers](https://togithub.com/sinonjs/fake-timers)
from 9.1.2 to 10.0.0 by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[fastify/fastify#4421
- add silent option to LogLevel by
[@&#8203;Uzlopak](https://togithub.com/Uzlopak) in
[fastify/fastify#4432

#### New Contributors

- [@&#8203;Ceres6](https://togithub.com/Ceres6) made their first
contribution in
[fastify/fastify#4428

**Full Changelog**:
fastify/fastify@v4.10.0...v4.10.1

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

[Compare
Source](https://togithub.com/fastify/fastify/compare/v4.9.2...v4.10.0)

#### What's Changed

- docs(reference/reply): spelling fixes by
[@&#8203;Fdawgs](https://togithub.com/Fdawgs) in
[fastify/fastify#4358
- Support different content-type typed reply with TypeProvider by
[@&#8203;rain714](https://togithub.com/rain714) in
[fastify/fastify#4360
- chore: remove leading empty lines by
[@&#8203;LinusU](https://togithub.com/LinusU) in
[fastify/fastify#4364
- fix types after pino 8.7.0 change by
[@&#8203;mcollina](https://togithub.com/mcollina) in
[fastify/fastify#4365
- Node.js V19 support by
[@&#8203;mcollina](https://togithub.com/mcollina) in
[fastify/fastify#4366
- fix: no check on `null` or `undefined` values passed as fn by
[@&#8203;metcoder95](https://togithub.com/metcoder95) in
[fastify/fastify#4367
- docs(server): config is lost when reply.call not found() is called by
[@&#8203;cesarvspr](https://togithub.com/cesarvspr) in
[fastify/fastify#4368
- Fix typo - 'sever' to 'server' by
[@&#8203;utsav91](https://togithub.com/utsav91) in
[fastify/fastify#4372
- Add platformatic to the Acknowledgements by
[@&#8203;mcollina](https://togithub.com/mcollina) in
[fastify/fastify#4378
- docs: add Simone Busoli to plugin maintainers by
[@&#8203;simoneb](https://togithub.com/simoneb) in
[fastify/fastify#4379
- add missing 'validationContext' field to FastifyError type by
[@&#8203;jakubburzynski](https://togithub.com/jakubburzynski) in
[fastify/fastify#4363
- fix(type-providers): assignability of instance with enabled type
provider by [@&#8203;driimus](https://togithub.com/driimus) in
[fastify/fastify#4371
- feat: support async trailer by
[@&#8203;climba03003](https://togithub.com/climba03003) in
[fastify/fastify#4380
- fix: trailers async race condition by
[@&#8203;climba03003](https://togithub.com/climba03003) in
[fastify/fastify#4383
- docs(ecosystem): Add fastify-list-routes by
[@&#8203;chuongtrh](https://togithub.com/chuongtrh) in
[fastify/fastify#4385
- build(deps-dev): bump
[@&#8203;sinclair/typebox](https://togithub.com/sinclair/typebox) from
0.24.51 to 0.25.2 by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[fastify/fastify#4388
- \[ Fix ] Improve error message for hooks check by
[@&#8203;debadutta98](https://togithub.com/debadutta98) in
[fastify/fastify#4387
- fix: tiny-lru usage by
[@&#8203;climba03003](https://togithub.com/climba03003) in
[fastify/fastify#4391
- Removes old note about named imports in ESM by
[@&#8203;fox1t](https://togithub.com/fox1t) in
[fastify/fastify#4392
- docs: Add section about capacity planning by
[@&#8203;kibertoad](https://togithub.com/kibertoad) in
[fastify/fastify#4386
- docs(recommendations): grammar fixes by
[@&#8203;Fdawgs](https://togithub.com/Fdawgs) in
[fastify/fastify#4396
- chore(doc): duplicated menu item by
[@&#8203;Eomm](https://togithub.com/Eomm) in
[fastify/fastify#4398
- feat: add request.routeOptions object by
[@&#8203;debadutta98](https://togithub.com/debadutta98) in
[fastify/fastify#4397
- docs: Document multiple app approach by
[@&#8203;kibertoad](https://togithub.com/kibertoad) in
[fastify/fastify#4393
- fix example using db decorator on fastify instance by
[@&#8203;mmarti](https://togithub.com/mmarti) in
[fastify/fastify#4406
- docs: fix removeAdditional refer by
[@&#8203;shunyue1320](https://togithub.com/shunyue1320) in
[fastify/fastify#4410

#### New Contributors

- [@&#8203;rain714](https://togithub.com/rain714) made their first
contribution in
[fastify/fastify#4360
- [@&#8203;LinusU](https://togithub.com/LinusU) made their first
contribution in
[fastify/fastify#4364
- [@&#8203;cesarvspr](https://togithub.com/cesarvspr) made their first
contribution in
[fastify/fastify#4368
- [@&#8203;utsav91](https://togithub.com/utsav91) made their first
contribution in
[fastify/fastify#4372
- [@&#8203;jakubburzynski](https://togithub.com/jakubburzynski) made
their first contribution in
[fastify/fastify#4363
- [@&#8203;driimus](https://togithub.com/driimus) made their first
contribution in
[fastify/fastify#4371
- [@&#8203;chuongtrh](https://togithub.com/chuongtrh) made their first
contribution in
[fastify/fastify#4385
- [@&#8203;debadutta98](https://togithub.com/debadutta98) made their
first contribution in
[fastify/fastify#4387
- [@&#8203;mmarti](https://togithub.com/mmarti) made their first
contribution in
[fastify/fastify#4406
- [@&#8203;shunyue1320](https://togithub.com/shunyue1320) made their
first contribution in
[fastify/fastify#4410

**Full Changelog**:
fastify/fastify@v4.9.2...v4.10.0

</details>

---

### Configuration

📅 **Schedule**: Branch creation - "" (UTC), 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 [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://developer.mend.io/github/quirrel-dev/quirrel).

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

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Copy link

github-actions bot commented Nov 6, 2023

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
semver-minor Issue or PR that should land as semver minor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Request bodyLimit
6 participants