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 proxy pass parsing on some keys #393 #457

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

Conversation

cbazureau
Copy link

@cbazureau cbazureau commented Nov 8, 2021

Fix issues due to nginx proxy pass rewriting (https://serverfault.com/questions/459369/disabling-url-decoding-in-nginx-proxy)

For example if your key is a:b:{"url","https,//c.com"}

then redis-commander will call /key/<some-connectionId>/a%3Ab%3A%7B"url"%2C"https%2C%2F%2Fc.com"%7D
that will become /key/<some-connectionId>/a:b:{"url","https,/c.com"} (with one slash) when received by redis-commander server.

See more on #393

@sseide
Copy link
Collaborator

sseide commented Nov 8, 2021

Thanks for the PR - i was thinking about something similar too...

Do you checked https://developer.mozilla.org/de/docs/Web/API/btoa ?
MDN mentions some browsers having problems with Unicode chars in btoa (and linked from this page https://developer.mozilla.org/en-US/docs/Glossary/Base64#solution_.232_.e2.80.93_rewriting_atob()_and_btoa()_using_typedarrays_and_utf-8) therefore the solution shall work too for all types of keys if possible.

It does not mention the browsers itself, tests with the most used ones is needed...
And I did not implement it until know because base64 adds additional length to the strings, maybe there is a solution without this additional costs i did not thought of by now...

@cbazureau
Copy link
Author

Thanks for the feedback, i will look for a better solution (or an adjustment on current one)

@sseide
Copy link
Collaborator

sseide commented Nov 15, 2021

Do you found some time to test different browsers with your patch? Do the code versiones mentioned at MDN work too?
This fix may (potentially) help on other cases too - having redis keys with binary data in it...

@cbazureau
Copy link
Author

cbazureau commented Nov 15, 2021

I've got access to browserstack if needed but for me, as you said before, i think it can create/leave other problems (utf8 char, urls too long). For me the best way to fix it is to give up on the strict REST API and to use a POST /key/:connectionId/manipulate endpoint for get/edit/delete action. key and action will be on the body which eliminate all the problems once for all.

ex :

POST /key/:connectionId/manipulate
{
   "action": "get",
   "key": "a:b:{\"ur\l",\"https,//c.com\"}"
}

Ps: Do you get a purpose in keeping all apiv1 routes (they seem only used twice https://github.com/joeferner/redis-commander/search?q=apiv1) ?

@sseide
Copy link
Collaborator

sseide commented Jan 13, 2022

Regarding the v1 api - not really. Can be removed by now i think...

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