fix: url escape path components by babakks · Pull Request #13261 · cli/cli

babakks · 2026-04-22T15:04:16Z

This PR ensures user-provided values that end up in a URL path are URL escaped.

A few notes for reviewers:

Org and user names cannot contain /.
Environment names can have / in them, and to be able to correctly fetch them the name should be escaped in the URL path (e.g. /repos/owner/repo/environments/foo%2Fbar).
- Note that if you fetch /repos/owner/repo/environments, you'll see the / in the API URL for the example foo/bar env is, incorrectly, not URL-escaped.
Secret and variable names cannot contain /.
Labels can contain / in their names, and the /repos/owner/repo/labels/NAME is smart enough to work with both foo/bar and foo%2Fbar, but we should nevertheless escape it.

Copilot

Pull request overview

This PR updates multiple REST API path builders to URL-escape user-provided values that are interpolated into URL path segments, preventing / and other reserved characters from breaking request routing.

Changes:

Apply url.PathEscape to environment names, variable/secret names, org/user/team identifiers, label names, and codespace names when constructing REST paths/URLs.
Add net/url imports where needed to support path escaping.
Extend escaping to additional endpoints (e.g., codespaces org member routes, repo create owner/team resolution).

Show a summary per file

File	Description
pkg/cmd/variable/list/list.go	Escape env/org names when listing variables.
pkg/cmd/variable/get/get.go	Escape org/env/variable names when fetching a variable.
pkg/cmd/variable/delete/delete.go	Escape org/env/variable names when deleting a variable.
pkg/cmd/secret/set/http.go	Escape org/env/secret names for secret public-key lookups and secret PUT paths.
pkg/cmd/secret/list/list.go	Escape org/env names when listing secrets.
pkg/cmd/secret/delete/delete.go	Escape org/env/secret names (including user secrets) when deleting secrets.
pkg/cmd/ruleset/view/http.go	Escape org login when viewing org rulesets (but ID remains unescaped—see comment).
pkg/cmd/repo/create/http.go	Escape owner login and team slug when resolving owner/team via REST.
pkg/cmd/label/delete.go	Escape label name in delete label endpoint.
pkg/cmd/label/create.go	Escape label name in update label endpoint.
internal/codespaces/api/api.go	Escape org/user/codespace identifiers in codespaces REST URL construction.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Files reviewed: 11/11 changed files
Comments generated: 1

Copilot

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Files reviewed: 11/11 changed files
Comments generated: 6

Copilot · 2026-04-23T10:45:13Z

 	if orgName != "" {
-		stopURL = fmt.Sprintf("%s/orgs/%s/members/%s/codespaces/%s/stop", a.githubAPI, orgName, userName, codespaceName)
+		stopURL = fmt.Sprintf("%s/orgs/%s/members/%s/codespaces/%s/stop", a.githubAPI, url.PathEscape(orgName), url.PathEscape(userName), url.PathEscape(codespaceName))
 		spanName = "/orgs/*/members/*/codespaces/*/stop"
 	} else {
-		stopURL = fmt.Sprintf("%s/user/codespaces/%s/stop", a.githubAPI, codespaceName)
+		stopURL = fmt.Sprintf("%s/user/codespaces/%s/stop", a.githubAPI, url.PathEscape(codespaceName))
 		spanName = "/user/codespaces/*/stop"


In this file, codespaceName is now URL-escaped for Stop/Delete endpoints, but other methods still build URLs by string concatenation without escaping codespaceName (e.g., GetCodespace/StartCodespace/Patch). This leaves an inconsistent and potentially unsafe behavior if a codespace name contains reserved path characters like / (requests may hit the wrong endpoint or fail). Escape codespaceName consistently everywhere it is interpolated into a path segment (preferably by constructing URLs via url.URL or at least applying url.PathEscape).

Copilot · 2026-04-23T10:45:14Z

 func getEnvVariables(client *http.Client, repo ghrepo.Interface, envName string) ([]shared.Variable, error) {
-	path := fmt.Sprintf("repos/%s/environments/%s/variables", ghrepo.FullName(repo), envName)
+	path := fmt.Sprintf("repos/%s/environments/%s/variables", ghrepo.FullName(repo), url.PathEscape(envName))
 	return getVariables(client, repo.RepoHost(), path)


URL-escaping environment names is the key behavior change here (to support names containing /), but the existing tests for variable listing only cover simple environment names. Add a regression test case where EnvName includes a slash (e.g., foo/bar) and assert that the request path uses %2F in that segment.

Copilot · 2026-04-23T10:45:14Z

 	case shared.Organization:
-		path = fmt.Sprintf("orgs/%s/actions/variables/%s", orgName, opts.VariableName)
+		path = fmt.Sprintf("orgs/%s/actions/variables/%s", url.PathEscape(orgName), url.PathEscape(opts.VariableName))
 		host, _ = cfg.Authentication().DefaultHost()
 	case shared.Environment:
-		path = fmt.Sprintf("repos/%s/environments/%s/variables/%s", ghrepo.FullName(baseRepo), envName, opts.VariableName)
+		path = fmt.Sprintf("repos/%s/environments/%s/variables/%s", ghrepo.FullName(baseRepo), url.PathEscape(envName), url.PathEscape(opts.VariableName))
 		host = baseRepo.RepoHost()
 	case shared.Repository:
-		path = fmt.Sprintf("repos/%s/actions/variables/%s", ghrepo.FullName(baseRepo), opts.VariableName)
+		path = fmt.Sprintf("repos/%s/actions/variables/%s", ghrepo.FullName(baseRepo), url.PathEscape(opts.VariableName))
 		host = baseRepo.RepoHost()


This now URL-escapes envName and VariableName in the request path, but current command tests only cover names without reserved path characters. Add a regression test where the environment name includes / (and optionally a variable name with other escapable characters) and assert the outgoing REST path contains the escaped value (e.g., foo%2Fbar).

Copilot · 2026-04-23T10:45:14Z

 func getEnvSecrets(client *http.Client, repo ghrepo.Interface, envName string) ([]Secret, error) {
-	path := fmt.Sprintf("repos/%s/environments/%s/secrets", ghrepo.FullName(repo), envName)
+	path := fmt.Sprintf("repos/%s/environments/%s/secrets", ghrepo.FullName(repo), url.PathEscape(envName))
 	return getSecrets(client, repo.RepoHost(), path)


As with variables, this change is meant to support environment names containing /, but current secret-listing tests only cover environment names without reserved path characters. Add a regression test case where EnvName contains a slash and assert the request path uses %2F in the environment segment.

Copilot · 2026-04-23T10:45:15Z

 func deleteLabel(client *http.Client, repo ghrepo.Interface, name string) error {
 	apiClient := api.NewClientFromHTTP(client)
-	path := fmt.Sprintf("repos/%s/%s/labels/%s", repo.RepoOwner(), repo.RepoName(), name)
+	path := fmt.Sprintf("repos/%s/%s/labels/%s", repo.RepoOwner(), repo.RepoName(), url.PathEscape(name))

 	return apiClient.REST(repo.RepoHost(), "DELETE", path, nil, nil)


Label names can include /, and the path segment is now escaped accordingly. Add a regression test that deletes/updates a label with a name containing / and asserts that the REST request uses the escaped form (e.g., foo%2Fbar) to prevent regressions.

Copilot · 2026-04-23T10:45:15Z

 func viewRepoRuleset(httpClient *http.Client, repo ghrepo.Interface, databaseId string) (*shared.RulesetREST, error) {
-	path := fmt.Sprintf("repos/%s/%s/rulesets/%s", repo.RepoOwner(), repo.RepoName(), databaseId)
+	path := fmt.Sprintf("repos/%s/%s/rulesets/%s", repo.RepoOwner(), repo.RepoName(), url.PathEscape(databaseId))
 	return viewRuleset(httpClient, repo.RepoHost(), path)
 }

 func viewOrgRuleset(httpClient *http.Client, orgLogin string, databaseId string, host string) (*shared.RulesetREST, error) {
-	path := fmt.Sprintf("orgs/%s/rulesets/%s", orgLogin, databaseId)
+	path := fmt.Sprintf("orgs/%s/rulesets/%s", url.PathEscape(orgLogin), url.PathEscape(databaseId))
 	return viewRuleset(httpClient, host, path)


Now that databaseId (and org login) are URL-escaped in the ruleset view endpoints, add a regression test that uses an ID containing / (or other reserved characters) and asserts that the generated REST path contains the escaped value (e.g., %2F). This helps ensure path injection can’t regress.

williammartin · 2026-04-24T12:15:34Z

 	switch secretEntity {
 	case shared.Organization:
-		path = fmt.Sprintf("orgs/%s/%s/secrets/%s", orgName, secretApp, opts.SecretName)
+		path = fmt.Sprintf("orgs/%s/%s/secrets/%s", url.PathEscape(orgName), secretApp, url.PathEscape(opts.SecretName))


Why escape the orgName here and in other places if as you say, they can't have / in them?

williammartin

Shouldn't these be escaped in variable set?

 // pkg/cmd/variable/set/http.go:110
 path := fmt.Sprintf(`repositories/%d/environments/%s/variables`, repoID, envName)
 
 // pkg/cmd/variable/set/http.go:146 
 path := fmt.Sprintf(`repositories/%d/environments/%s/variables/%s`, repoID, envName, variableName)

Signed-off-by: Babak K. Shandiz <babakks@github.com>

Agent-Logs-Url: https://github.com/cli/cli/sessions/e09c6964-46ba-4baf-a486-f8dce8c7ac48 Co-authored-by: babakks <36728931+babakks@users.noreply.github.com>

Signed-off-by: Babak K. Shandiz <babakks@github.com>

babakks requested review from a team as code owners April 22, 2026 15:04

babakks requested review from Copilot and williammartin April 22, 2026 15:04

Copilot started reviewing on behalf of babakks April 22, 2026 15:04 View session

Copilot AI reviewed Apr 22, 2026

View reviewed changes

Comment thread pkg/cmd/ruleset/view/http.go

Copilot started work on behalf of babakks April 23, 2026 10:34 View session

Copilot finished work on behalf of babakks April 23, 2026 10:39

babakks requested a review from Copilot April 23, 2026 10:40

Copilot started reviewing on behalf of babakks April 23, 2026 10:40 View session

Copilot AI reviewed Apr 23, 2026

View reviewed changes

williammartin reviewed Apr 24, 2026

View reviewed changes

williammartin approved these changes Apr 24, 2026

View reviewed changes

babakks and others added 3 commits May 27, 2026 15:33

fix: url escape path components

bd327c1

Signed-off-by: Babak K. Shandiz <babakks@github.com>

fix: apply url.PathEscape to databaseId in ruleset view HTTP functions

1400e07

Agent-Logs-Url: https://github.com/cli/cli/sessions/e09c6964-46ba-4baf-a486-f8dce8c7ac48 Co-authored-by: babakks <36728931+babakks@users.noreply.github.com>

fix: apply url path escape to variable components

6663170

Signed-off-by: Babak K. Shandiz <babakks@github.com>

babakks force-pushed the babakks/path-escape-urls branch from 0c1c1bd to 6663170 Compare May 27, 2026 14:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: url escape path components#13261

fix: url escape path components#13261
babakks wants to merge 3 commits into
trunkfrom
babakks/path-escape-urls

babakks commented Apr 22, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 23, 2026

Uh oh!

Copilot AI Apr 23, 2026

Uh oh!

Copilot AI Apr 23, 2026

Uh oh!

Copilot AI Apr 23, 2026

Uh oh!

Copilot AI Apr 23, 2026

Uh oh!

Copilot AI Apr 23, 2026

Uh oh!

williammartin Apr 24, 2026

Uh oh!

williammartin left a comment •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

babakks commented Apr 22, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Copilot's findings

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot's findings

Uh oh!

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

williammartin Apr 24, 2026

Choose a reason for hiding this comment

Uh oh!

williammartin left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

williammartin left a comment •

edited

Loading