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(options): added options for follow redirects with max body length and max redirects #984

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

omarmhamza
Copy link

This PR fixes this issue 983 by adding additional config options (followRedirectsOpts) to configure the maxBodyLength and maxRedirects when followRedirects options is enabled.

Description

Under the hood node-proxy uses follow-redirects module when followRedirects is set to true. It does that by using the http and https agents given by follow-redirects module as shown here

A function is added setFollowRedirectOptions in the constructor to use the additional options followRedirectsOpts:

  /**
   * Configures options for follow-redirects when followRedirects is true.
   * @property {number} [maxRedirects] - The maximum number of redirects to follow.
   * @property {number} [maxBodyLength] - The maximum allowed size of the response body in bytes.
   * @see {@link https://github.com/follow-redirects/follow-redirects?tab=readme-ov-file#global-options}
   */
  followRedirectsOpts?: {
    maxRedirects?: number;
    maxBodyLength?: number;
  };

However, the way it is added is not the cleanest, it is done by setting these properties globally for follow-redirects module. I think this is a limitation since there are no options within http-proxy to set these configurations. Moreover, http-proxy is unmaintained and having the a PR there is useless.

Motivation and Context

  • follow redirects of requests with body forms larger than 10MB do not break.
  • configurable max redirects.

How has this been tested?

  • Creation of a sample express.js server
  • Forked http-proxy-middleware
  • Cloned follow-redirects module
  • Added debug logs in follow-redirects (to make sure the parameters are set correctly)
  • Added this change into http-proxy-middleware
  • Used the middleware in the sample project
  • Followed the reproduction steps mentioned in the issue here
  • Tested by changing the parameters and checking edge cases.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

@omarmhamza
Copy link
Author

I am open for feedback here and any other suggestions on how we can configure these options without this workaround. Thank you.

@chimurai
Copy link
Owner

Did you try to set the follow-redirects global options in your application?

// https://github.com/follow-redirects/follow-redirects?tab=readme-ov-file#global-options
const followRedirects = require('follow-redirects');
followRedirects.maxRedirects = 10;
followRedirects.maxBodyLength = 20 * 1024 * 1024; // 20 MB

If this works, there is no need to modify HPM or http-proxy

@omarmhamza
Copy link
Author

@chimurai yes I have seen these global options in follow-redirects.

But it is difficult to know why there is these limits if we are using HPM solely. As a consumer of the library I expected to have these options available to me to configure. But I had to dig around to then know that different http agents are used if followredirect is set and thus certain limits are then applied which were not applied if the option was set to false.

Looking at this again, maybe it would be a better idea to include these options as globals in HPM or have this documented somewhere giving heads-up to the consumers of this library. What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants