opt to validate incoming data before proxy is executed by kovalenp · Pull Request #312 · fastify/fastify-http-proxy

kovalenp · 2023-07-20T18:30:08Z

Provide an option to preValidate incoming data (check out: #311)

Checklist

run npm run test and npm run benchmark
tests and/or benchmarks are included
documentation is changed or added
commit message and code follows the Developer's Certification of Origin
and the Code of conduct

evahteev

LGTM

Uzlopak · 2023-07-20T19:57:18Z

index.js

+  if (opts.preValidation === undefined && opts.proxyPayloads !== false) {
    fastify.addContentTypeParser('application/json', bodyParser)
    fastify.addContentTypeParser('*', bodyParser)
  }

+  if (opts.preValidation) {
+    fastify.addHook('preValidation', opts.preValidation)
+  }
+


Why do we not proxy Payloads if preValidation is set?

req.body is not parsed and comes as aIncomingMessage stream if:

fastify.addContentTypeParser('application/json', bodyParser) fastify.addContentTypeParser('*', bodyParser)

condition is executed, as described here: #86 (comment)

actually, the payload is passed in this case, see:

https://github.com/fastify/fastify-http-proxy/pull/312/files#diff-a561630bb56b82342bc66697aee2ad96efddcbc9d150665abd6fb7ecb7c0ab2fR269

index.js

mcollina

lgtm

mcollina

I'm -1 to add this option.

This can be achieved with the following:

app.register(async function (app) {
  app.addHook('preValidation', ...)
  app.register(httpProxy, {
    proxyPayloads: false
  })
})

Could you add that to the documentation instead?

kovalenp · 2023-07-21T08:31:37Z

thanks @mcollina did you check #311 ? It is on the TODO list.

what if I don't want to run preValidation on every route just on the proxied one?

mcollina · 2023-07-21T08:56:48Z

thanks @mcollina did you check #311 ?

I did now, I still think what you want to achieve can be done with the approach I described in #312 (review). Note that I set proxyPayloads to `false, which does the trick and it enables body validation.

It is on the TODO list.

I assembled that a long time ago, I might have been mistaken in adding this.

what if I don't want to run preValidation on every route just on the proxied one?

The snippet in #312 (review) adds it only to the proxied routes.

kovalenp · 2023-07-21T09:32:45Z

I still find adding preValidator option a more convenient way (and it's consistent to preHandler option), I'd rather remove the proxyPayloads instead.

But it's indeed up to the team. Feel free to close the PR if you decide that it's not the preferable way.

mcollina

lgtm

mcollina · 2023-07-21T09:41:13Z

Can you fix the failing tests?

kovalenp · 2023-07-21T09:48:06Z

Can you fix the failing tests?

I wish!

I can see that tests are failing for node 18 on ubuntu agent and are hardly related to the change:

# Subtest: Proxy websocket with custom upstream url
        1..5
        ok 1 - should be equal
        ok 2 - should be equal
        ok 3 - should be equal
        ok 4 - should be equal
        ok 5 - should be equivalent strictly
    # test count(8) != plan(null)
    # failed 1 of 8 tests
not ok 5 - test/websocket.js # time=30007.488ms
  ---
  env: {}
  file: test/websocket.js
  timeout: 30000
  command: /opt/hostedtoolcache/node/18.16.1/x64/bin/node
  args:
    - test/websocket.js
  stdio:
    - 0
    - pipe
    - 2
  cwd: /home/runner/work/fastify-http-proxy/fastify-http-proxy
  failures:
    - tapError: no plan
  exitCode: null
  signal: SIGTERM
  ...

But the same is happening for me locally on the main branch. 😢 when use 18.17 node

could you please advise how it could be fixed, @mcollina ?

mcollina · 2023-07-28T10:40:11Z

For whatever reason this is actually breaking websocket support as this test PR is passing: #315

kovalenp · 2023-07-29T16:29:47Z

For whatever reason this is actually breaking websocket support as this test PR is passing: #315

This PR didn't trigger unit tests though, there are only two checks. I'm positive main branch have the issue

mcollina · 2023-07-31T08:19:11Z

Verified, it's a problem.

mcollina · 2023-07-31T08:43:14Z

please rebase, I've fixed it

Listen only to IPv4 network for websockets tests (fastify#316)

Co-authored-by: Uzlopak <aras.abbasi@googlemail.com>

kovalenp · 2023-07-31T09:55:52Z

please rebase, I've fixed it

Nice! Respect Sir. Rebase is done

mcollina · 2023-07-31T10:10:14Z

types/index.d.ts

    proxyPayloads?: boolean;
    preHandler?: preHandlerHookHandler;
    beforeHandler?: preHandlerHookHandler;
+    preValidation?: preValidationHookHandler;


Can you add a test for this type in https://github.com/fastify/fastify-http-proxy/blob/master/types/index.test-d.ts?

Addressed here: 0c0e505

mcollina

lgtm

cb1kenobi · 2023-08-01T20:02:36Z

Excellent work! I can't wait to use it. Mind pushing a new release? Thanks!

denchen · 2023-11-01T02:20:54Z

Hi. Can we get a release for this? I saw preValidation in the docs and assumed that I could use it, but the latest release does not have this feature.

Eomm · 2023-11-01T08:17:27Z

Released as 9.3.0 👍🏼

evahteev approved these changes Jul 20, 2023

View reviewed changes

Uzlopak reviewed Jul 20, 2023

View reviewed changes

index.js Outdated Show resolved Hide resolved

mcollina approved these changes Jul 21, 2023

View reviewed changes

mcollina requested changes Jul 21, 2023

View reviewed changes

mcollina approved these changes Jul 21, 2023

View reviewed changes

mcollina mentioned this pull request Jul 28, 2023

test #315

Closed

kovalenp and others added 5 commits July 31, 2023 11:47

Merge pull request #1 from fastify/master

607ae5b

Listen only to IPv4 network for websockets tests (fastify#316)

implements preValidation option

56a4e5c

index.js indends fixed

1d5948d

Applied suggestion to refactor preValidation conditions

4a34ad7

Co-authored-by: Uzlopak <aras.abbasi@googlemail.com>

preValidation docs fix

8ab3717

kovalenp force-pushed the feat/request-payload-validation branch from 4f2e199 to 8ab3717 Compare July 31, 2023 09:54

mcollina reviewed Jul 31, 2023

View reviewed changes

added test for preValidation type

0c0e505

mcollina approved these changes Jul 31, 2023

View reviewed changes

mcollina merged commit 0f5bc42 into fastify:master Jul 31, 2023

Uh oh!

Conversation

kovalenp commented Jul 20, 2023

Checklist

Uh oh!

evahteev left a comment

Choose a reason for hiding this comment

Uh oh!

Uzlopak Jul 20, 2023

Choose a reason for hiding this comment

Uh oh!

kovalenp Jul 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kovalenp commented Jul 21, 2023

Uh oh!

mcollina commented Jul 21, 2023

Uh oh!

kovalenp commented Jul 21, 2023

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina commented Jul 21, 2023

Uh oh!

kovalenp commented Jul 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcollina commented Jul 28, 2023

Uh oh!

kovalenp commented Jul 29, 2023

Uh oh!

mcollina commented Jul 31, 2023

Uh oh!

mcollina commented Jul 31, 2023

Uh oh!

kovalenp commented Jul 31, 2023

Uh oh!

mcollina Jul 31, 2023

Choose a reason for hiding this comment

Uh oh!

kovalenp Jul 31, 2023

Choose a reason for hiding this comment

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

cb1kenobi commented Aug 1, 2023

Uh oh!

denchen commented Nov 1, 2023

Uh oh!

Eomm commented Nov 1, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

kovalenp Jul 20, 2023 •

edited

Loading

mcollina left a comment •

edited

Loading

kovalenp commented Jul 21, 2023 •

edited

Loading