fix: golang html template url escaping (#38363) (#38369)

Backport #38363

fix #38362

Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>

modules/templates/helper_test.go

@@ -92,9 +92,12 @@ func TestTemplateEscape(t *testing.T) {
 	}
 
 	t.Run("Golang URL Escape", func(t *testing.T) {
-		// Golang template considers "href", "*src*", "*uri*", "*url*" (and more) ... attributes as contentTypeURL and does auto-escaping
+		// HINT: GOLANG-HTML-TEMPLATE-URL-ESCAPING: demo cases (html/template/attr.go):
+		// Golang template considers "href", "data-href", "*src*", "*uri*", "*url*" (and more) ... attributes as contentTypeURL and does auto-escaping
 		actual := execTmpl(`<a href="?a={{"%"}}"></a>`)
 		assert.Equal(t, `<a href="?a=%25"></a>`, actual)
+		actual = execTmpl(`<a data-href="?a={{"%"}}"></a>`)
+		assert.Equal(t, `<a data-href="?a=%25"></a>`, actual)
 		actual = execTmpl(`<a data-xxx-url="?a={{"%"}}"></a>`)
 		assert.Equal(t, `<a data-xxx-url="?a=%25"></a>`, actual)
 	})
@@ -102,6 +105,10 @@ func TestTemplateEscape(t *testing.T) {
 		// non-URL content isn't auto-escaped
 		actual := execTmpl(`<a data-link="?a={{"%"}}"></a>`)
 		assert.Equal(t, `<a data-link="?a=%"></a>`, actual)
+		// the attr names like "data-href" and "data-action" are treated as URL (as the "data-" prefix is stripped)
+		// but "data-xxx-href" and "data-xxx-action" are not, so no escaping.
+		actual = execTmpl(`<a data-xxx-href="?a={{"%"}}"></a>`)
+		assert.Equal(t, `<a data-xxx-href="?a=%"></a>`, actual)
 	})
 	t.Run("QueryBuild", func(t *testing.T) {
 		actual := execTmpl(`<a href="{{QueryBuild "?" "a" "%"}}"></a>`)

templates/admin/packages/list.tmpl

@@ -73,7 +73,7 @@
 							<td>{{DateUtils.AbsoluteShort .Version.CreatedUnix}}</td>
 							<td>
 								<a class="tw-text-red show-modal" href data-modal="#admin-package-delete-modal"
-									data-modal-form.action="{{$.Link}}/delete?page={{$.Page.Paginater.Current}}&sort={{$.SortType}}&id={{.Version.ID}}"
+									data-modal-form.url="{{$.Link}}/delete?page={{$.Page.Paginater.Current}}&sort={{$.SortType}}&id={{.Version.ID}}"
 									data-modal-package-name="{{.Package.Name}}" data-modal-package-version="{{.Version.Version}}"
 								>{{svg "octicon-trash"}}</a>
 							</td>

templates/admin/repo/list.tmpl

@@ -86,7 +86,7 @@
 							<td>{{DateUtils.AbsoluteShort .CreatedUnix}}</td>
 							<td>
 								<a class="tw-text-red show-modal" href data-modal="#admin-repo-delete-modal"
-									data-modal-form.action="{{$.Link}}/delete?page={{$.Page.Paginater.Current}}&sort={{$.SortType}}&id={{.ID}}"
+									data-modal-form.url="{{$.Link}}/delete?page={{$.Page.Paginater.Current}}&sort={{$.SortType}}&id={{.ID}}"
 									data-modal-repo-name="{{.Name}}"
 								>{{svg "octicon-trash"}}</a>
 							</td>

templates/org/team/members.tmpl

@@ -35,7 +35,7 @@
 								<div class="item-trailing">
 									{{if and $.IsOrganizationOwner (not (and ($.Team.IsOwnerTeam) (eq (len $.Team.Members) 1)))}}
 										<button class="ui red button show-modal" data-modal="#remove-team-member"
-											data-modal-form.action="{{$.OrgLink}}/teams/{{$.Team.LowerName | PathEscape}}/action/remove?uid={{.ID}}"
+											data-modal-form.url="{{$.OrgLink}}/teams/{{$.Team.LowerName | PathEscape}}/action/remove?uid={{.ID}}"
 											data-modal-name="{{.DisplayName}}"
 											data-modal-team-name="{{$.Team.Name}}">{{ctx.Locale.Tr "org.members.remove"}}</button>
 									{{end}}

templates/org/team/sidebar.tmpl

@@ -13,7 +13,7 @@
 		<div class="flex-text-block">
 			{{if .Team.IsMember ctx $.SignedUser.ID}}
 				<button class="ui red mini compact button show-modal" data-modal="#org-member-leave-team"
-					data-modal-form.action="{{$.OrgLink}}/teams/{{$.Team.LowerName | PathEscape}}/action/leave?uid={{$.SignedUser.ID}}"
+					data-modal-form.url="{{$.OrgLink}}/teams/{{$.Team.LowerName | PathEscape}}/action/leave?uid={{$.SignedUser.ID}}"
 					data-modal-to-leave-team-name="{{$.Team.Name}}"
 				>{{ctx.Locale.Tr "org.teams.leave"}}</button>
 			{{else if .IsOrganizationOwner}}

templates/org/team/teams.tmpl

@@ -37,7 +37,7 @@
 							<a href="{{$.OrgLink}}/teams/{{.LowerName | PathEscape}}/repositories">{{.NumRepos}} {{ctx.Locale.Tr "org.lower_repositories"}}</a>
 							{{if .IsMember ctx $.SignedUser.ID}}
 								<button class="ui red mini compact button show-modal" data-modal="#org-member-leave-team"
-									data-modal-form.action="{{$.OrgLink}}/teams/{{.LowerName | PathEscape}}/action/leave?uid={{$.SignedUser.ID}}"
+									data-modal-form.url="{{$.OrgLink}}/teams/{{.LowerName | PathEscape}}/action/leave?uid={{$.SignedUser.ID}}"
 									data-modal-to-leave-team-name="{{.Name}}"
 								>{{ctx.Locale.Tr "org.teams.leave"}}</button>
 							{{end}}

templates/repo/branch/list.tmpl

@@ -200,7 +200,7 @@
 											</span>
 										</button>
 									{{else}}
-										<button class="btn interact-bg tw-p-2 show-modal delete-branch-button tw-text-red" data-modal="#delete-branch-modal" data-modal-form.action="{{$.Link}}/delete?name={{.DBBranch.Name}}&page={{$.Page.Paginater.Current}}" data-tooltip-content="{{ctx.Locale.Tr "repo.branch.delete" (.DBBranch.Name)}}" data-modal-name="{{.DBBranch.Name}}">
+										<button class="btn interact-bg tw-p-2 show-modal delete-branch-button tw-text-red" data-modal="#delete-branch-modal" data-modal-form.url="{{$.Link}}/delete?name={{.DBBranch.Name}}&page={{$.Page.Paginater.Current}}" data-tooltip-content="{{ctx.Locale.Tr "repo.branch.delete" (.DBBranch.Name)}}" data-modal-name="{{.DBBranch.Name}}">
 											{{svg "octicon-trash"}}
 										</button>
 									{{end}}

templates/repo/issue/view_content/context_menu.tmpl

@@ -34,10 +34,10 @@
 					<div class="divider"></div>
 				{{end}}
 				{{if $canUserBlock}}
-				<div class="item context js-aria-clickable show-modal" data-modal="#block-user-modal" data-modal-modal-blockee="{{.item.Poster.Name}}" data-modal-modal-blockee-name="{{.item.Poster.GetDisplayName}}" data-modal-modal-form.action="{{AppSubUrl}}/user/settings/blocked_users">{{ctx.Locale.Tr "user.block.block.user"}}</div>
+				<div class="item context js-aria-clickable show-modal" data-modal="#block-user-modal" data-modal-modal-blockee="{{.item.Poster.Name}}" data-modal-modal-blockee-name="{{.item.Poster.GetDisplayName}}" data-modal-modal-form.url="{{AppSubUrl}}/user/settings/blocked_users">{{ctx.Locale.Tr "user.block.block.user"}}</div>
 				{{end}}
 				{{if $canOrgBlock}}
-				<div class="item context js-aria-clickable show-modal" data-modal="#block-user-modal" data-modal-modal-blockee="{{.item.Poster.Name}}" data-modal-modal-blockee-name="{{.item.Poster.GetDisplayName}}" data-modal-modal-form.action="{{ctx.RootData.Repository.Owner.OrganisationLink}}/settings/blocked_users">{{ctx.Locale.Tr "user.block.block.org"}}</div>
+				<div class="item context js-aria-clickable show-modal" data-modal="#block-user-modal" data-modal-modal-blockee="{{.item.Poster.Name}}" data-modal-modal-blockee-name="{{.item.Poster.GetDisplayName}}" data-modal-modal-form.url="{{ctx.RootData.Repository.Owner.OrganisationLink}}/settings/blocked_users">{{ctx.Locale.Tr "user.block.block.org"}}</div>
 				{{end}}
 			{{end}}
 		{{end}}

templates/shared/secrets/add_list.tmpl

@@ -3,7 +3,7 @@
 	<div class="ui right">
 		<button class="ui primary tiny button show-modal"
 			data-modal="#add-secret-modal"
-			data-modal-form.action="{{.Link}}"
+			data-modal-form.url="{{.Link}}"
 			data-modal-header="{{ctx.Locale.Tr "secrets.add_secret"}}"
 			data-modal-secret-name.value=""
 			data-modal-secret-name.read-only="false"
@@ -39,7 +39,7 @@
 				</span>
 				<button class="btn interact-bg show-modal tw-p-2"
 					data-modal="#add-secret-modal"
-					data-modal-form.action="{{$.Link}}"
+					data-modal-form.url="{{$.Link}}"
 					data-modal-header="{{ctx.Locale.Tr "secrets.edit_secret"}}"
 					data-tooltip-content="{{ctx.Locale.Tr "secrets.edit_secret"}}"
 					data-modal-secret-name.value="{{.Name}}"

templates/shared/user/profile_big_avatar.tmpl

@@ -132,7 +132,7 @@
 				{{end}}
 				<li>
 					{{if not .UserBlocking}}
-						<a class="muted show-modal" href="#" data-modal="#block-user-modal" data-modal-modal-blockee="{{.ContextUser.Name}}" data-modal-modal-blockee-name="{{.ContextUser.GetDisplayName}}" data-modal-modal-form.action="{{AppSubUrl}}/user/settings/blocked_users">{{ctx.Locale.Tr "user.block.block.user"}}</a>
+						<a class="muted show-modal" href="#" data-modal="#block-user-modal" data-modal-modal-blockee="{{.ContextUser.Name}}" data-modal-modal-blockee-name="{{.ContextUser.GetDisplayName}}" data-modal-modal-form.url="{{AppSubUrl}}/user/settings/blocked_users">{{ctx.Locale.Tr "user.block.block.user"}}</a>
 					{{else}}
 						<a class="muted" href="{{AppSubUrl}}/user/settings/blocked_users">{{ctx.Locale.Tr "user.block.unblock"}}</a>
 					{{end}}

templates/shared/variables/variable_list.tmpl

@@ -3,7 +3,7 @@
 	<div class="ui right">
 		<button class="ui primary tiny button show-modal"
 			data-modal="#edit-variable-modal"
-			data-modal-form.action="{{.Link}}/new"
+			data-modal-form.url="{{.Link}}/new"
 			data-modal-header="{{ctx.Locale.Tr "actions.variables.creation"}}"
 			data-modal-dialog-variable-name=""
 			data-modal-dialog-variable-data=""
@@ -39,7 +39,7 @@
 				<button class="btn interact-bg tw-p-2 show-modal"
 					data-tooltip-content="{{ctx.Locale.Tr "actions.variables.edit"}}"
 					data-modal="#edit-variable-modal"
-					data-modal-form.action="{{$.Link}}/{{.ID}}/edit"
+					data-modal-form.url="{{$.Link}}/{{.ID}}/edit"
 					data-modal-header="{{ctx.Locale.Tr "actions.variables.edit"}}"
 					data-modal-dialog-variable-name="{{.Name}}"
 					data-modal-dialog-variable-data="{{.Data}}"

templates/user/settings/organization.tmpl

@@ -24,7 +24,7 @@
 						</div>
 						<div class="item-trailing">
 							<button class="ui red button show-modal" data-modal="#leave-organization"
-											data-modal-form.action="{{.OrganisationLink}}/members/action/leave?uid={{$.SignedUser.ID}}"
+											data-modal-form.url="{{.OrganisationLink}}/members/action/leave?uid={{$.SignedUser.ID}}"
 											data-modal-organization-name="{{.DisplayName}}">{{ctx.Locale.Tr "org.members.leave"}}
 							</button>
 						</div>

tests/integration/branches_test.go

@@ -58,7 +58,7 @@ func TestUndoDeleteBranch(t *testing.T) {
 	}
 
 	onGiteaRun(t, func(t *testing.T, u *url.URL) {
-		htmlDoc, name := branchAction(t, ".delete-branch-button", "data-modal-form.action")
+		htmlDoc, name := branchAction(t, ".delete-branch-button", "data-modal-form.url")
 		assert.Contains(t,
 			htmlDoc.doc.Find(".ui.positive.message").Text(),
 			translation.NewLocale("en-US").TrString("repo.branch.deletion_success", name),

web_src/js/features/common-button.test.ts

@@ -2,9 +2,10 @@ import {assignElementProperty, type ElementWithAssignableProperties} from './com
 
 test('assignElementProperty', () => {
   const elForm = document.createElement('form');
-  assignElementProperty(elForm, 'action', '/test-link');
-  expect(elForm.action).contains('/test-link'); // the DOM always returns absolute URL
-  expect(elForm.getAttribute('action')).eq('/test-link');
+  expect(() => assignElementProperty(elForm, 'action', '/test-link')).toThrow();
+  assignElementProperty(elForm, 'url', '/test-url');
+  expect(elForm.action).contains('/test-url'); // the DOM always returns absolute URL
+  expect(elForm.getAttribute('action')).eq('/test-url');
   assignElementProperty(elForm, 'text-content', 'dummy');
   expect(elForm.textContent).toBe('dummy');
 
@@ -14,8 +15,9 @@ test('assignElementProperty', () => {
     _attrs: Record<string, string> = {};
     setAttribute(name: string, value: string) { this._attrs[name] = value }
     getAttribute(name: string): string | null { return this._attrs[name] }
+    nodeName = 'FORM';
   }();
-  assignElementProperty(elFormWithAction, 'action', '/bar');
+  assignElementProperty(elFormWithAction, 'url', '/bar');
   expect(elFormWithAction.getAttribute('action')).eq('/bar');
 
   const elInput = document.createElement('input');

web_src/js/features/common-button.ts

@@ -42,11 +42,20 @@ function onHidePanelClick(el: HTMLElement, e: MouseEvent) {
 }
 
 export type ElementWithAssignableProperties = {
+  nodeName: string;
   getAttribute: (name: string) => string | null;
   setAttribute: (name: string, value: string) => void;
 } & Record<string, any>;
 
 export function assignElementProperty(el: ElementWithAssignableProperties, kebabName: string, val: string) {
+  if (el.nodeName === 'FORM') {
+    // HINT: GOLANG-HTML-TEMPLATE-URL-ESCAPING: a special case for Golang HTML template escaping.
+    // Golang HTML template only handles some "known" attribute names as URL (e.g.: when the name is "action" or contains "url")
+    // To prevent template developers from making mistakes like `data-modal-form.action="?k={{ValueWithSpecialChars}}" (no escaping),
+    // here we use `data-modal-form.url="?k={{ValueWithSpecialChars}}", then the value gets correctly escaped by Golang HTML template.
+    if (kebabName === 'action') throw new Error(`don't assign element property "action" by value, use "data-modal-form.url" instead`);
+    if (kebabName === 'url') kebabName = 'action';
+  }
   const camelizedName = camelize(kebabName);
   const old = el[camelizedName];
   if (typeof old === 'boolean') {
@@ -59,7 +68,7 @@ export function assignElementProperty(el: ElementWithAssignableProperties, kebab
     // "form" has an edge case: its "<input name=action>" element overwrites the "action" property, we can only set attribute
     el.setAttribute(kebabName, val);
   } else {
-    // in the future, we could introduce a better typing system like `data-modal-form.action:string="..."`
+    // in the future, maybe we could introduce a better typing system if it is really needed
     throw new Error(`cannot assign element property "${camelizedName}" by value "${val}"`);
   }
 }
@@ -71,7 +80,11 @@ function onShowModalClick(el: HTMLElement, e: MouseEvent) {
   // * Then, try to query '[name=target]'
   // * Then, try to query '.target'
   // * Then, try to query 'target' as HTML tag
-  // If there is a ".{prop-name}" part like "data-modal-form.action", the "form" element's "action" property will be set, the "prop-name" will be camel-cased to "propName".
+  // If there's a ".{prop-name}" part like "data-modal-input.value", the "input" element's "value" property will be set,
+  // the "prop-name" will be camel-cased to "propName" (e.g.: "data-modal-input.read-only" for "readOnly" property).
+  //
+  // HINT: GOLANG-HTML-TEMPLATE-URL-ESCAPING: Form element's "action" property must be set by "data-modal-form.url"
+  // to make the template variables get correctly escaped in the URL.
   e.preventDefault();
   const modalSelector = el.getAttribute('data-modal')!;
   const elModal = document.querySelector(modalSelector);