-
Notifications
You must be signed in to change notification settings - Fork 126
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: Ensure that all API endpoint URLs are constructed safely. #544
Conversation
8174ce9
to
bd82a2d
Compare
Also, I am fairly certain that all of these changes are correct, as during development when I made mistakes (more often than I wished) the existing tests caught those mistakes. Assuming we have 100% test coverage for the modified files, these changes should effectively be a no-op when proper data is provided to the API operations. |
If we wanted to get extra fancy we could improve |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some stylistic comments. Thanks for doing this! Lots of manual conversion.
I've addressed the feedback and also added unit tests for the new helper function. Also, most of the conversion wasn't manual, it was done using some complicated |
05175e0
to
7b4446f
Compare
I really like that it isn’t fancy. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Merge this baby.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. 🚀
#543 will be reverted before this PR is merged, as this achieves the same result (disallowing injection of path elements in serviceID fields).
As discussed in our chat today, this PR adds a
ToSafeURL
function which accepts a slice of strings that are used to construct the API endpoint URL. Each string is checked for unacceptable values (currently just..
) and passed throughurl.PathEscape
. The result is that if any components contains a 'special character' which could affect parsing or routing of the resulting URL, it will be URL-encoded for safety.I attempted to find all places where API endpoint URLs were being constructed from one or more components dynamically, but it is certainly possible that I missed some!
While this PR touches a lot of files, I find that the code is more readable now, as the URL-construction process isn't polluted by
Sprintf
format strings.