Allow passthrough redirects #125
Conversation
| @@ -31,6 +31,10 @@ function waitConnection (socket, write) { | |||
| } | |||
| } | |||
|
|
|||
| function isExternalUrl (url = '') { | |||
| return urlPattern.test(url) | |||
There was a problem hiding this comment.
Would you mind to check if url.startsWith('http://') || url.startsWith('https://') is faster than regex?
|
Awesome, happy new year @mcollina. Waiting for this one getting merged :D |
| @@ -31,6 +31,10 @@ function waitConnection (socket, write) { | |||
| } | |||
| } | |||
|
|
|||
| function isExternalUrl (url = '') { | |||
| return urlPattern.test(url) | |||
There was a problem hiding this comment.
How is this guaranteed to be "an external url"? Shouldn't the headers be inspected to determine if there is a 3xx code that should be honored?
There was a problem hiding this comment.
That was my initial thought but status code is not getting passed through the rewriteHeaders method
There was a problem hiding this comment.
Then I would think the solution starts with resolving that issue. I do not see how this simple test is a guarantee of a redirect.
There was a problem hiding this comment.
If status code is necessary for the checking, I think you can start a PR in fastify-reply-form to achieve it.
|
any updates on this? |
|
I am not sure I can merge this one as there are no action items, I resolved all the issues but merging is blocked and I don't know why. |
Merging is not blocked, per se. I would certainly like my question addressed, but I'm not blocking this. |
|
@vorillaz could you try merging this again if nothing is blocking? |
Description
Allow absolute URL redirects when applicable. Closes #124 .
Checklist
npm run testandnpm run benchmarkand the Code of conduct