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

Header case sensitivity and status code #88

Merged
merged 2 commits into from
Sep 19, 2019

Conversation

davidcaron
Copy link

  • Use lowercase to compare if header is a hop by hop
  • forward to the client the status code received from the proxied server

Copy link
Contributor

@fmigneault fmigneault left a comment

Choose a reason for hiding this comment

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

Although dict is case insensitive, it doesn't guarantee that the keys are stored as either lowercase or capitalized or anything else for that matter.
We should modify the filtering method to never have to worry about any case.

extra note

Might need to consider backport merge into 0.5.2 if this fix is required for a project.
Current master has a new feature #22 which is not tested with magpie (Ouranosinc/Magpie#190)
@cehbrecht probably we should tag 0.6.x because of oauth feature now in master

@davidcaron
Copy link
Author

We should modify the filtering method to never have to worry about any case.

Not sure what you mean? I'm passing the headers as is, the conversion to lowercase is only to do the filtering, which handles uppercase and lowercase.

@fmigneault
Copy link
Contributor

I didn't notice the .lower() applied to the key also, so it's fine like this.
If was referring to the fact that the headers dict keys are stored as is (they are not automatically converted to lowercase) although they can be retrieved in case-insensitive way.

@cehbrecht cehbrecht merged commit 8fbe0c8 into master Sep 19, 2019
@cehbrecht cehbrecht deleted the connection-header-and-status-code branch September 19, 2019 09:49
@cehbrecht
Copy link
Member

@davidcaron merged. thanks.

@fmigneault I will backport this patch to the 0.5.x branch. I have still some more work to do on the oauth stuff. So, I would prefer to wait before I make a new release.

cehbrecht pushed a commit that referenced this pull request Sep 19, 2019
* headers are case-insensitive (some servers use lowercase only)

* forward to the client the status code received from the proxied server
@cehbrecht
Copy link
Member

@fmigneault ... backported to 0.5.x. Shall we collect more patches before a 0.5.3 release?

@fmigneault
Copy link
Contributor

We can wait for more patches unless this is needed by @davidcaron for a quick fix somewhere?

@davidcaron
Copy link
Author

Yes, I had to confirm, but a 0.5.3 release would fix one bug we have in a deployment (the client expects 201, but gets 200).

@fmigneault
Copy link
Contributor

@davidcaron
Do you refer to fix #84?
Should be already introduced in 0.5.2.
New 0.5.3 would only add #88 if back ported in 511739d to avoid #86 untested with Magpie.

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