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: use escaped strings for services' path #755

Merged
merged 1 commit into from
Oct 6, 2022

Conversation

GGabriele
Copy link
Collaborator

@GGabriele GGabriele commented Sep 20, 2022

Right now, when a url containing an encoded whitespace (%20)
is passed to a service, decK unwraps it and replace it with an
actual whitespace. This is because decK uses url.Parse under
the cover, which is the one making this translation.

This commit makes decK URL unwrapping method to encode back
decoded whitespaces.

@GGabriele GGabriele temporarily deployed to Configure ci September 20, 2022 08:10 Inactive
Copy link

@javierguerragiraldez javierguerragiraldez left a comment

Choose a reason for hiding this comment

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

on a quick reading of EscapedPath() source, it doesn't feel like the safest approach, as it checks if the raw url is "okish" to use, and if so, returns it directly.

my first approach would be to first distinguish two very different cases:

  1. "we're having too many issues with "special" characters" -> better to straight up encode.
  2. "everything has been working well with decoded path, except this very specific thing" -> just do path.replace(' ', '%20')

@GGabriele GGabriele temporarily deployed to Configure ci September 20, 2022 15:48 Inactive
@GGabriele GGabriele temporarily deployed to Configure ci September 20, 2022 15:48 Inactive
@GGabriele
Copy link
Collaborator Author

on a quick reading of EscapedPath() source, it doesn't feel like the safest approach, as it checks if the raw url is "okish" to use, and if so, returns it directly.

my first approach would be to first distinguish two very different cases:

  1. "we're having too many issues with "special" characters" -> better to straight up encode.
  2. "everything has been working well with decoded path, except this very specific thing" -> just do path.replace(' ', '%20')

Yeah I think we're falling mostly on the second case, which was also the initial way I was working it out.
Addressing this here: 7df6eae

@codecov-commenter
Copy link

Codecov Report

Merging #755 (7df6eae) into main (3a167e8) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #755      +/-   ##
==========================================
+ Coverage   39.98%   40.00%   +0.01%     
==========================================
  Files          87       87              
  Lines        9715     9717       +2     
==========================================
+ Hits         3885     3887       +2     
  Misses       5459     5459              
  Partials      371      371              
Impacted Files Coverage Δ
file/types.go 62.61% <100.00%> (+0.22%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@GGabriele
Copy link
Collaborator Author

@javierguerragiraldez would you mind taking another look whenever you have time?

Copy link

@javierguerragiraldez javierguerragiraldez left a comment

Choose a reason for hiding this comment

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

LGTM. don't forget to update the commit message, since this no longer uses EscapedPath.

@GGabriele GGabriele force-pushed the fix/encoded-whitespaces-services-url branch from 7df6eae to cfc491e Compare October 6, 2022 06:12
@GGabriele GGabriele temporarily deployed to Configure ci October 6, 2022 06:12 Inactive
@GGabriele GGabriele temporarily deployed to Configure ci October 6, 2022 06:12 Inactive
Right now, when a `url` containing an encoded whitespace (`%20`)
is passed to a service, decK unwraps it and replace it with an
actual whitespace. This is because decK uses `url.Parse` under
the cover, which is the one making this translation.

This commit makes decK URL unwrapping method to encode back
decoded whitespaces.
@GGabriele GGabriele force-pushed the fix/encoded-whitespaces-services-url branch from cfc491e to 7c1ba12 Compare October 6, 2022 06:58
@GGabriele GGabriele temporarily deployed to Configure ci October 6, 2022 06:58 Inactive
@GGabriele GGabriele temporarily deployed to Configure ci October 6, 2022 06:58 Inactive
@GGabriele GGabriele merged commit 9997f56 into main Oct 6, 2022
@GGabriele GGabriele deleted the fix/encoded-whitespaces-services-url branch October 6, 2022 07:02
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