internal/relui: fix URL path segment escaping for task names
url.PathEscape can be used to escape a string so it can be safely placed
inside a URL path segment. Do so, which after migrating to http.ServeMux
and its pattern matching makes it possible to successfully restart tasks
with characters like "/" in their name (e.g., such as a task named "Wait
for x/tools stdlib CL submission" where we first ran into this).
Fixes golang/go#76860.
Change-Id: Ie69516e0f3b440baf6cdd60dbc4f5785cd24a822
Reviewed-on: https://go-review.googlesource.com/c/build/+/732262
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Auto-Submit: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
diff --git a/internal/relui/templates/task_list.html b/internal/relui/templates/task_list.html
index a291383..73cb594 100644
--- a/internal/relui/templates/task_list.html
+++ b/internal/relui/templates/task_list.html
@@ -76,7 +76,7 @@
{{if .Error.Valid}}
<div class="TaskList-retryTask">
<form
- action="{{baseLink (printf "/workflows/%s/tasks/%s/retry" $workflow.ID .Name)}}"
+ action="{{baseLink (printf "/workflows/%s/tasks/%s/retry" $workflow.ID (.Name|urlPathEscape))}}"
method="post">
<input type="hidden" id="workflow.id" name="workflow.id" value="{{$workflow.ID}}" />
<input
@@ -90,7 +90,7 @@
{{else if and (not .ApprovedAt.Valid) (.ReadyForApproval)}}
<div class="TaskList-approveTask">
<form
- action="{{baseLink (printf "/workflows/%s/tasks/%s/approve" $workflow.ID .Name)}}"
+ action="{{baseLink (printf "/workflows/%s/tasks/%s/approve" $workflow.ID (.Name|urlPathEscape))}}"
method="post">
<input type="hidden" id="workflow.id" name="workflow.id" value="{{$workflow.ID}}" />
<input
diff --git a/internal/relui/web.go b/internal/relui/web.go
index 5c3264d..71ec952 100644
--- a/internal/relui/web.go
+++ b/internal/relui/web.go
@@ -88,6 +88,7 @@
"prettySize": prettySize,
"sidebarWorkflows": s.sidebarWorkflows,
"unmarshalResultDetail": unmarshalResultDetail,
+ "urlPathEscape": url.PathEscape,
}
s.templates = template.Must(template.New("").Funcs(helpers).ParseFS(templates, "templates/*.html"))
s.homeTmpl = s.mustLookup("home.html")
diff --git a/internal/relui/web_test.go b/internal/relui/web_test.go
index e5bc40d..2828052 100644
--- a/internal/relui/web_test.go
+++ b/internal/relui/web_test.go
@@ -1026,3 +1026,36 @@
}
t.Run("unacld workflow", func(t *testing.T) { testWorkflowACL(t, true, false, false) })
}
+
+// Test that it's possible to retry a task whose name includes characters
+// such as "/" that become different after URL path segment escaping.
+func TestRetryHandlerEscaping(t *testing.T) {
+ s := &Server{m: &metricsRouter{mux: http.NewServeMux()}}
+ var gotTaskName string
+ s.m.HandleFunc("POST /workflows/{id}/tasks/{name}/retry", func(w http.ResponseWriter, req *http.Request) {
+ gotTaskName = req.PathValue("name")
+ })
+ cl := &http.Client{Transport: localRoundTripper{s.m}}
+ resp, err := cl.Post("/workflows/123/tasks/update%20x%2Fbuild/retry", "", nil)
+ if err != nil {
+ t.Fatal(err)
+ }
+ if got, want := resp.StatusCode, http.StatusOK; got != want {
+ t.Fatalf("status code mismatch: got %d, want %d", got, want)
+ }
+ if got, want := gotTaskName, "update x/build"; got != want {
+ t.Errorf("task name mismatch: got %q, want %q", got, want)
+ }
+}
+
+// localRoundTripper is an http.RoundTripper that executes HTTP transactions
+// by using handler directly, instead of going over an HTTP connection.
+type localRoundTripper struct {
+ handler http.Handler
+}
+
+func (l localRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) {
+ w := httptest.NewRecorder()
+ l.handler.ServeHTTP(w, req)
+ return w.Result(), nil
+}
