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

ParametersNameDecodingHandler middleware affects values, adds new keys #179

Closed
ivan-klass opened this issue Aug 17, 2023 · 1 comment · Fixed by #218
Closed

ParametersNameDecodingHandler middleware affects values, adds new keys #179

ivan-klass opened this issue Aug 17, 2023 · 1 comment · Fixed by #218
Assignees
Labels
bug Something isn't working WIP

Comments

@ivan-klass
Copy link

ivan-klass commented Aug 17, 2023

It seems like ParametersNameDecodingHandler middleware is a workaround that should only affect query parameter names, without replacing values and adding new parameters.
However, the code below is corrupting all parameter values that contain escaped meaningful URL symbols like +, =, ?, & - the latter may add extra parameter names into the URL

updated_url: str = str(request.url) #type: ignore
if all(
[
current_options, current_options.enabled, '%' in updated_url,
current_options.characters_to_decode
]
):
updated_url = unquote(updated_url)
request.url = httpx.URL(updated_url)

from urllib.parse import unquote
from httpx import URL

original = URL("https://google.com?q=1%2b2") # search for "1+2"
updated = URL(unquote(str(original)))
assert updated ==  URL('https://google.com?q=1+2')  # corrupted, now searches for "1 2" !

original = URL("https://google.com/?q=M%26A")  # search for "M&A"
updated = URL(unquote(str(original)))  # corrupted, now searches for "M", adds separate A parameter !

This was extremely hard to find when it was resulting in SDK API errors with Invalid continuation token <TOKEN> passed where actually valid TOKEN that had + in the middle and also ends with =.

@baywet
Copy link
Member

baywet commented Aug 18, 2023

Hi @ivan-klass
Thanks for your interest in kiota and for reaching out.

You're right in the sense that line 47 is an overbearing implementation that also decodes the value when it shouldn't. Its implementation also diverges from other languages although the other languages most likely suffer from the same issue.

Is this something you'd be willing to submit a pull request to fix? (align the implementation with other languages and correct the implementation so it only considers parameters names)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working WIP
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants