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

fix: avoid setting body for GET requests #3643

Merged
merged 4 commits into from
Feb 24, 2023

Conversation

melodyVoid
Copy link
Contributor

When making a GET request to certain uplinks, such as https://registry.npmmirror.com, setting the body field can result in a 413 error. Previously, the code was setting the body field for all requests, including GET requests.

This commit fixes the issue by checking the request method and avoiding setting the body field for GET requests. This ensures that GET requests are not affected by the issue and can be made without error.

Fixes #3601

When making a GET request to certain uplinks, such as https://registry.npmmirror.com, setting the body field can result in a 413 error. Previously, the code was setting the body field for all requests, including GET requests.

This commit fixes the issue by checking the request method and avoiding setting the body field for GET requests. This ensures that GET requests are not affected by the issue and can be made without error.

Fixes verdaccio#3601
@@ -228,6 +227,11 @@ class ProxyStorage implements IProxy {
agentOptions: this.agent_options,
};

// GET requests should not have a body, otherwise the request might fail(return 413 status code)
if (method.toUpperCase() !== 'GET') {
Copy link
Member

@juanpicado juanpicado Feb 22, 2023

Choose a reason for hiding this comment

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

great you found a solution, also thanks for the comment, would you mind add some test, here you can star looking at test/unit/modules/uplinks/up-storage.spec.ts if you have questions let me know

You can run the test yarn test test/unit/modules/uplinks/up-storage.spec.ts

note: first get my latest changes from the branch 5.x

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll fix the code format and add some unit tests.

@juanpicado
Copy link
Member

Format is not correct, you can run yarn format to fix it.

juanpicado and others added 2 commits February 22, 2023 19:51
Cause thers is a bug in `isObject` function from `@verdaccio/core`, when `options.json` is `true`
GET request body will be string 'true', some uplinks might return 413 status code such as
https://registry.npmmirror.com

fix verdaccio#3601
src/lib/up-storage.ts Outdated Show resolved Hide resolved
@juanpicado
Copy link
Member

@melodyVoid please feel free to update the dependency in your branch

@melodyVoid
Copy link
Contributor Author

@melodyVoid please feel free to update the dependency in your branch

I updated the dependency, please check it out.

Copy link
Member

@juanpicado juanpicado left a comment

Choose a reason for hiding this comment

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

Great job @melodyVoid ! LGTM

@juanpicado juanpicado added this to the 5.x milestone Feb 24, 2023
@juanpicado juanpicado merged commit e4573c7 into verdaccio:5.x Feb 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants