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

bug: do proper url encoding #12

Closed
cafkafk opened this issue Oct 30, 2023 · 2 comments · Fixed by #36
Closed

bug: do proper url encoding #12

cafkafk opened this issue Oct 30, 2023 · 2 comments · Fixed by #36

Comments

@cafkafk
Copy link
Owner

cafkafk commented Oct 30, 2023

As mentioned in #9, it would probably be a good idea to do "proper" url-encoding.

Specifically because weird characters in :user and :repo might cause issues.

@cafkafk cafkafk changed the title refactor: do proper url encoding bug: do proper url encoding Oct 30, 2023
@algernon
Copy link
Collaborator

algernon commented Nov 3, 2023

I was just about to submit a PR to fix this, but.. then it dawned on me that we don't need to, not really. Other parts of the GitLab API assume that both the user and repo are things that do not need URL encoding. If they are something that would, that'd break everything GitLab related.

As such, the only thing we do need to encode, is the /, which we already do. So I think the proper action here is to update the comment, explaining why %2F is okay: because that's the only part that needs encoding, and encoding the other two might hide other problems later down the line.

To do proper url encoding, we'd need to add form_urlencoded as a direct dependency (it's only a transitive one at the moment), and do something like this:

diff --git a/src/api/v1/gitlab/utils/gitlab_api_get_releases.rs b/src/api/v1/gitlab/utils/gitlab_api_get_releases.rs
index 5410e10..e9423a4 100644
--- a/src/api/v1/gitlab/utils/gitlab_api_get_releases.rs
+++ b/src/api/v1/gitlab/utils/gitlab_api_get_releases.rs
@@ -15,11 +15,10 @@ pub async fn gitlab_api_get_releases(
     user: String,
     repo: String,
 ) -> Result<ForgeReleases, Box<dyn std::error::Error + Send + Sync>> {
-    // TODO: The middle part, `{}%2F{}` is really `user/repo` URL-encoded. We
-    // should do proper URL encoding.
+    let id: String = form_urlencoded::byte_serialize(format!("{user}/{repo}").as_bytes()).collect();
     let releases_uri = Url::parse(&format!(
-        "https://{}/api/v4/projects/{}%2F{}/releases?per_page={}",
-        host, user, repo, page_size
+        "https://{}/api/v4/projects/{}/releases?per_page={}",
+        host, id, page_size
     ))?;
     ForgeReleases::from_url(releases_uri).await
 }

It's not terrible, but it's more effort than updating the comment, and I now think that not urlencoding the user & repo is more correct.

@cafkafk
Copy link
Owner Author

cafkafk commented Nov 3, 2023

Given this, maybe we should only consider URL encoding in the future if something else seems to require it, and I suppose just update the comment for now.

cafkafk pushed a commit that referenced this issue Nov 3, 2023
Other parts of the GitLab API use the user and repo names without URL
encoding, so there really is no need to encode *those*. Thus, the only
remaining part that needs encoding, is the `/` path separator, which we
manually url-encoded to `%2F`.

If we encoded the user and repo too, that might lead to surprising
results later down the line. To not hide problems, leaving them as-is is
the preferable way.

Update the comment on `Gitlab::get_api_releases_url` accordingly.

Fixes #12.

Signed-off-by: Gergely Nagy <me@gergo.csillger.hu>
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 a pull request may close this issue.

2 participants