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

convert query parameters to lowercase before reading them #661

Closed

Conversation

wunter8
Copy link
Contributor

@wunter8 wunter8 commented Mar 7, 2023

fixes #630

@wunter8
Copy link
Contributor Author

wunter8 commented Mar 7, 2023

I forgot to modify the docs, and since I branched this from main, it doesn't have the doc changes from my other PRs, and I don't want to worry about it trying to auto-merge, haha. If you want to add a line to the docs for this, here it is (but I'm already a little self-conscious about how many times my name is included in this release, so it's fine if you leave it out):

* Query parameters when publishing via a GET request (e.g., `/mytopic/publish?Message=testing`) are now case-insensitive ([#630](https://github.com/binwiederhier/ntfy/issues/630) thanks to [@bbaa-bbaa](https://github.com/bbaa-bbaa) for reporting, and [@wunter8](https://github.com/wunter8) for fixing)

@binwiederhier
Copy link
Owner

binwiederhier commented Mar 7, 2023

I kinda hate this "fix" for two reasons:

  1. It's a pretty unnecessary feature to have. We could have just adjusted to docs to say "look, you gotta have it all lowercase for query params"
  2. It's not efficient, since it loops through every param every time readParam is called. It's probably very much negligible, but I still hate it.

(I intentionally left the conclusion about this open. I gotta think about it.)

@wunter8
Copy link
Contributor Author

wunter8 commented Mar 7, 2023

I was concerned about efficiency and did the lowercase-ing outside of the names loop, but I didn't consider all the different parameters we check for. That's true.

I think just updating the docs could be a valid change. Most people use query parameters for publishing instead of headers or JSON body because they're using a webhook and the only thing they can control is the URL, right? Which means, they have control over the query parameters and can make them lowercase

@binwiederhier
Copy link
Owner

Let's just do a doc update on this. We can always bring the PR back to life when other people ask for it.

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.

Query parameters accept only lowercase parameter names
2 participants