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] parseWildcardRules removes the last non-asterisk character wrongly #57

Closed
wants to merge 1 commit into from

Conversation

maxshine
Copy link

Problem Description

For a pattern with asterisk as suffix, the parseWildcardRules forms the result array wrongly.
i.e. for an allowed origin of http://localhost*, the parseWildcardRules returns the result as
[]string {"http://localhos", "*"} instead of []string {"http://localhost", "*"}

Root Cause

The cause of this error is the code snippet

cors/cors.go

Line 138 in 5f50d4f

wRules = append(wRules, []string{o[:i-1], "*"})

The i is the index where asterisk character found and I think it should take a substring with end of i then the last non-asterisk character is kept.

Solution

The solution is as this PR, using i to take the substring.

Please kindly review the change and accept it if this makes sense.

Thanks

@appleboy
Copy link
Member

appleboy commented Mar 6, 2024

fixed in #106

@maxshine
Copy link
Author

maxshine commented Mar 6, 2024

fixed in #106

I don't understand the rationale of your action to take the same change as my PR from a later PR without endorsing mine.

I provided this fix FIVE years ago but it was just override by another most recent one with exactly the same change.

@appleboy
Copy link
Member

appleboy commented Mar 8, 2024

@maxshine

I would like to extend my sincerest apologies for the oversight regarding the handling of your pull request (PR). I understand your frustration and disappointment in seeing that a more recent PR with the same change has been accepted and merged, while your original contribution, which was submitted five years ago, was not given the due recognition it deserved.

Your early identification of the issue and the fix you provided should have been acknowledged and merged at the time of submission. The fact that it was overlooked is a failure on our part, and I take full responsibility for this mistake. The process of reviewing and merging PRs should be thorough and fair, and in this instance, we did not meet that standard.

Please know that your contributions are valuable to us, and the situation you've experienced is not indicative of the level of respect and consideration we strive to uphold for all contributors. I am personally reviewing our PR management process to ensure that such an error does not happen again. We will also look into ways of rectifying this situation, such as crediting you for the change in our project's documentation or release notes.

Once again, I apologize for any inconvenience or disheartenment this may have caused you. Your work and time are greatly appreciated, and we are committed to making things right.

Please see the v1.6.0 release note.

image

@maxshine
Copy link
Author

maxshine commented Mar 8, 2024

@maxshine

I would like to extend my sincerest apologies for the oversight regarding the handling of your pull request (PR). I understand your frustration and disappointment in seeing that a more recent PR with the same change has been accepted and merged, while your original contribution, which was submitted five years ago, was not given the due recognition it deserved.

Your early identification of the issue and the fix you provided should have been acknowledged and merged at the time of submission. The fact that it was overlooked is a failure on our part, and I take full responsibility for this mistake. The process of reviewing and merging PRs should be thorough and fair, and in this instance, we did not meet that standard.

Please know that your contributions are valuable to us, and the situation you've experienced is not indicative of the level of respect and consideration we strive to uphold for all contributors. I am personally reviewing our PR management process to ensure that such an error does not happen again. We will also look into ways of rectifying this situation, such as crediting you for the change in our project's documentation or release notes.

Once again, I apologize for any inconvenience or disheartenment this may have caused you. Your work and time are greatly appreciated, and we are committed to making things right.

Please see the v1.6.0 release note.

image

Well noted.

The deliberate elaborations enable me to understand the situation thoroughly.

All good and best regards to you @appleboy.

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.

3 participants