opt to validate incoming data before proxy is executed #312

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 <[email protected]>

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

@@ -31,6 +31,7 @@ declare namespace fastifyHttpProxy {
    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 <[email protected]>

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

opt to validate incoming data before proxy is executed #312

opt to validate incoming data before proxy is executed #312

kovalenp commented Jul 20, 2023

evahteev left a comment

Uzlopak Jul 20, 2023

kovalenp Jul 20, 2023 •

edited

Loading

mcollina left a comment

mcollina left a comment •

edited

Loading

kovalenp commented Jul 21, 2023

mcollina commented Jul 21, 2023

kovalenp commented Jul 21, 2023

mcollina left a comment

mcollina commented Jul 21, 2023

kovalenp commented Jul 21, 2023 •

edited

Loading

mcollina commented Jul 28, 2023

kovalenp commented Jul 29, 2023

mcollina commented Jul 31, 2023

mcollina commented Jul 31, 2023

kovalenp commented Jul 31, 2023

mcollina Jul 31, 2023

kovalenp Jul 31, 2023

mcollina left a comment

cb1kenobi commented Aug 1, 2023

denchen commented Nov 1, 2023

Eomm commented Nov 1, 2023

opt to validate incoming data before proxy is executed #312

opt to validate incoming data before proxy is executed #312

Conversation

kovalenp commented Jul 20, 2023

Checklist

evahteev left a comment

Choose a reason for hiding this comment

Uzlopak Jul 20, 2023

Choose a reason for hiding this comment

kovalenp Jul 20, 2023 • edited Loading

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

mcollina left a comment • edited Loading

Choose a reason for hiding this comment

kovalenp commented Jul 21, 2023

mcollina commented Jul 21, 2023

kovalenp commented Jul 21, 2023

mcollina left a comment

Choose a reason for hiding this comment

mcollina commented Jul 21, 2023

kovalenp commented Jul 21, 2023 • edited Loading

mcollina commented Jul 28, 2023

kovalenp commented Jul 29, 2023

mcollina commented Jul 31, 2023

mcollina commented Jul 31, 2023

kovalenp commented Jul 31, 2023

mcollina Jul 31, 2023

Choose a reason for hiding this comment

kovalenp Jul 31, 2023

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

cb1kenobi commented Aug 1, 2023

denchen commented Nov 1, 2023

Eomm commented Nov 1, 2023

kovalenp Jul 20, 2023 •

edited

Loading

mcollina left a comment •

edited

Loading

kovalenp commented Jul 21, 2023 •

edited

Loading