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

Remove wildcard length lower limit #126

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ggPeti
Copy link

@ggPeti ggPeti commented Feb 13, 2022

This limitation seems arbitrary and I didn't find any discussion preceding it. So, I removed it. Verified with nginx 1.20.2.

@ggPeti
Copy link
Author

ggPeti commented Feb 15, 2022

Hi @agentzh, thanks for this wonderful module.
What do you think about removing this limitation? I am using the patched version without issues, I think it's very useful sometimes to remove all headers from a proxied upstream.
My use case is that I'm using Nextcloud as a file server, and it sends a bunch of session cookies and other unwanted headers. I really only need Content-Type and Content-Length, which need is perfectly covered by two proxy_pass_headers in conjunction with more_clear_headers *

@zhuizhuhaomeng
Copy link
Contributor

Hi @agentzh, thanks for this wonderful module. What do you think about removing this limitation? I am using the patched version without issues, I think it's very useful sometimes to remove all headers from a proxied upstream. My use case is that I'm using Nextcloud as a file server, and it sends a bunch of session cookies and other unwanted headers. I really only need Content-Type and Content-Length, which need is perfectly covered by two proxy_pass_headers in conjunction with more_clear_headers *

I think it is acceptable. And you need to add some test cases for this PR.

@ggPeti
Copy link
Author

ggPeti commented Apr 11, 2022

@zhuizhuhaomeng I am at a loss for ideas how to add relevant tests to a change that removes a gratuitous error case.
@agentzh would you mind merging this?

@zhuizhuhaomeng
Copy link
Contributor

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