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: Ensure that all API endpoint URLs are constructed safely. #544

Merged
merged 4 commits into from
Sep 9, 2024

Conversation

kpfleming
Copy link
Contributor

@kpfleming kpfleming commented Sep 6, 2024

#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 through url.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.

fastly/helpers.go Outdated Show resolved Hide resolved
@kpfleming kpfleming force-pushed the safe-url-construction branch 3 times, most recently from 8174ce9 to bd82a2d Compare September 6, 2024 20:37
@kpfleming
Copy link
Contributor Author

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.

@kpfleming kpfleming marked this pull request as ready for review September 6, 2024 20:48
@kpfleming
Copy link
Contributor Author

If we wanted to get extra fancy we could improve ToSafeURL to receive []interface instead of []string, and then it could handle formatting the integers itself. That would eliminate most of the integer-formatting in the PR, except for one case where the resulting string has to be zero-padded.

Copy link
Contributor

@cee-dub cee-dub left a 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.

fastly/helpers.go Outdated Show resolved Hide resolved
fastly/stats_historical.go Show resolved Hide resolved
fastly/stats_realtime.go Outdated Show resolved Hide resolved
@kpfleming
Copy link
Contributor Author

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 sed substitutions :-)

@cee-dub
Copy link
Contributor

cee-dub commented Sep 7, 2024

If we wanted to get extra fancy

I really like that it isn’t fancy.

fastly/helpers.go Outdated Show resolved Hide resolved
Copy link
Contributor

@cee-dub cee-dub left a comment

Choose a reason for hiding this comment

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

Looking good!

fastly/helpers.go Outdated Show resolved Hide resolved
Copy link
Contributor

@cee-dub cee-dub left a comment

Choose a reason for hiding this comment

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

Merge this baby.

Copy link
Collaborator

@Integralist Integralist left a comment

Choose a reason for hiding this comment

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

LGTM. 🚀

@kpfleming kpfleming merged commit 94c0e63 into main Sep 9, 2024
4 checks passed
@kpfleming kpfleming deleted the safe-url-construction branch September 9, 2024 10:35
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