Skip to content

Fix URLJoin, markup render link reoslving, sign-in/up/linkaccount page common data#36861

Merged
wxiaoguang merged 18 commits intogo-gitea:mainfrom
wxiaoguang:fix-url-join
Mar 8, 2026
Merged

Fix URLJoin, markup render link reoslving, sign-in/up/linkaccount page common data#36861
wxiaoguang merged 18 commits intogo-gitea:mainfrom
wxiaoguang:fix-url-join

Conversation

@wxiaoguang
Copy link
Copy Markdown
Contributor

@wxiaoguang wxiaoguang commented Mar 8, 2026

The logic of "URLJoin" is unclear and it is often abused.

Also:

  • Correct the resolveLinkRelative behavior
  • Fix missing "PathEscape" in ToTag
  • Fix more FIXMEs, and add new FIXMEs for newly found problems
  • Refactor "auth page common template data" because the captcha URL is affected by URLJoin, need to manage the variables together (it is a must in this PR's scope)

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 8, 2026
@github-actions github-actions bot added modifies/go Pull requests that update Go code modifies/templates This PR modifies the template files labels Mar 8, 2026
@wxiaoguang wxiaoguang force-pushed the fix-url-join branch 2 times, most recently from ed90efe to 3b9cd10 Compare March 8, 2026 08:46
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR deprecates/removes the ambiguous URLJoin helper from production code and templates, replacing it with more explicit URL construction, while also correcting a few URL-escaping/link-resolution behaviors.

Changes:

  • Rename util.URLJoin to util.URLJoinDeprecated and migrate remaining usages to either explicit concatenation or the deprecated helper in tests only.
  • Remove URLJoin from the template func map and update templates to construct URLs without it.
  • Adjust link resolution logic (resolveLinkRelative) and improve tag archive URL escaping (ToTag).

Reviewed changes

Copilot reviewed 26 out of 26 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
tests/integration/api_repo_git_tags_test.go Uses URLJoinDeprecated for tag URL expectation in tests.
tests/integration/api_packages_rpm_test.go Uses URLJoinDeprecated for generated RPM repo config test.
templates/user/auth/captcha.tmpl Replaces template URLJoin usage with string concatenation for Recaptcha script URL.
templates/repo/blame.tmpl Replaces template URLJoin usage with print concatenation for ignore-revs link.
templates/package/shared/list.tmpl Replaces template URLJoin usage with print concatenation for packages URL.
services/wiki/wiki_path.go Constructs wiki HTML URLs via explicit concatenation.
services/convert/git_commit_test.go Updates commit meta URL expectation to URLJoinDeprecated in tests.
services/convert/git_commit.go Replaces util.URLJoin usage with explicit concatenation for commit-related URLs.
services/convert/convert.go Escapes tag archive filenames with url.PathEscape; replaces API URL joins with concatenation.
services/context/repo.go Replaces URLJoin use in canonical URL construction with manual trimming/concatenation.
routers/web/repo/wiki.go Replaces redirect URL join with concatenation.
routers/web/repo/editor.go Replaces redirect URL join with concatenation while preserving segment escaping.
modules/util/util_test.go Switches URL join test to URLJoinDeprecated.
modules/util/url.go Renames URLJoinURLJoinDeprecated and documents deprecation intent.
modules/templates/helper.go Removes URLJoin from template function map.
modules/setting/server.go Replaces URLJoin usage in MakeAbsoluteAssetURL with string concatenation.
modules/recaptcha/recaptcha.go Builds Recaptcha verification URL via TrimSuffix + constant path.
modules/markup/render_link_test.go Adds/adjusts tests for relative link resolution.
modules/markup/render_link.go Reimplements relative link resolution using net/url + path.Join.
modules/markup/markdown/markdown_test.go Updates test URL joining to URLJoinDeprecated.
modules/markup/html_test.go Updates multiple test URL joins to URLJoinDeprecated.
modules/markup/html_mention.go Replaces URLJoin usage with fmt.Sprintf for team mention links.
modules/markup/html_issue.go Replaces URLJoin usage with fmt.Sprintf for issue links.
modules/markup/html_internal_test.go Updates test URL joins to URLJoinDeprecated.
modules/markup/html_commit.go Replaces URLJoin usage with fmt.Sprintf for commit links.
modules/markup/camo.go Replaces URLJoin usage with manual concatenation for camo proxy URL.

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

You can also share your feedback on Copilot code review. Take the survey.

@silverwind
Copy link
Copy Markdown
Member

Maybe completely remove it? I could exercise the LLM to remove it from those remaining tests.

@wxiaoguang
Copy link
Copy Markdown
Contributor Author

wxiaoguang commented Mar 8, 2026

Maybe completely remove it? I could exercise the LLM to remove it from those remaining tests.

Next PR.

This PR is already quite complex with many changes.

Copy link
Copy Markdown
Member

@silverwind silverwind left a comment

Choose a reason for hiding this comment

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

Written by Claude on behalf of @silverwind

resolveLinkRelative error path drops link (modules/markup/render_link.go): When url.Parse(link) fails, the fallback ignores link entirely:

if err != nil {
    finalLink = strings.TrimSuffix(base, "/") + path.Join("/"+cur)
}

Should probably still include link, e.g. path.Join("/"+cur, "/"+link).

captcha.tmpl double-slash: The default RECAPTCHA_URL is https://www.google.com/recaptcha/ (trailing slash). The template change {{print .RecaptchaURL "/api.js"}} produces .../recaptcha//api.js. The Go-side change in recaptcha.go does strings.TrimSuffix, but the template doesn't get the same treatment.

Duplicate test assertion in render_link_test.go: Line 314 is identical to line 313 — should be a different case (e.g. testing fragment preservation).

@wxiaoguang
Copy link
Copy Markdown
Contributor Author

wxiaoguang commented Mar 8, 2026

Should probably still include link, e.g. path.Join("/"+cur, "/"+link).

No, it should NOT tolerate invalid link to prevent from security problems.

Update: changed my mind, here is not related to "link security". Updated the tests

This comment was marked as resolved.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 24 out of 24 changed files in this pull request and generated no new comments.


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

You can also share your feedback on Copilot code review. Take the survey.

@wxiaoguang wxiaoguang requested a review from Copilot March 8, 2026 12:53
@wxiaoguang wxiaoguang changed the title Deprecated URLJoin Fix URLJoin, resolve link reoslving for markup render, auth page common data Mar 8, 2026
@wxiaoguang wxiaoguang changed the title Fix URLJoin, resolve link reoslving for markup render, auth page common data Fix URLJoin, relative link reoslving for markup render, sign-in/up/linkaccount page common data Mar 8, 2026
@wxiaoguang wxiaoguang changed the title Fix URLJoin, relative link reoslving for markup render, sign-in/up/linkaccount page common data Fix URLJoin, markup render link reoslving, sign-in/up/linkaccount page common data Mar 8, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 27 out of 27 changed files in this pull request and generated 1 comment.


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

You can also share your feedback on Copilot code review. Take the survey.

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Mar 8, 2026
@wxiaoguang wxiaoguang enabled auto-merge (squash) March 8, 2026 15:55
@wxiaoguang wxiaoguang merged commit 6f8ab6a into go-gitea:main Mar 8, 2026
26 checks passed
@GiteaBot GiteaBot added this to the 1.26.0 milestone Mar 8, 2026
@wxiaoguang wxiaoguang deleted the fix-url-join branch March 8, 2026 15:59
silverwind added a commit to silverwind/gitea that referenced this pull request Mar 8, 2026
* origin/main:
  Optimize Docker build with dependency layer caching (go-gitea#36864)
  Fix URLJoin, markup render link reoslving, sign-in/up/linkaccount page common data (go-gitea#36861)
  Fix CodeQL code scanning alerts (go-gitea#36858)
  Refactor auth middleware (go-gitea#36848)
  Update Nix flake (go-gitea#36857)
  Update JS deps (go-gitea#36850)
  Load `mentionValues` asynchronously (go-gitea#36739)
  [skip ci] Updated translations via Crowdin
silverwind added a commit to silverwind/gitea that referenced this pull request Mar 8, 2026
* main: (26 commits)
  Clean up `refreshViewedFilesSummary` (go-gitea#36868)
  Remove `util.URLJoin` and replace all callers with direct path concatenation (go-gitea#36867)
  Optimize Docker build with dependency layer caching (go-gitea#36864)
  Fix URLJoin, markup render link reoslving, sign-in/up/linkaccount page common data (go-gitea#36861)
  Fix CodeQL code scanning alerts (go-gitea#36858)
  Refactor auth middleware (go-gitea#36848)
  Update Nix flake (go-gitea#36857)
  Update JS deps (go-gitea#36850)
  Load `mentionValues` asynchronously (go-gitea#36739)
  [skip ci] Updated translations via Crowdin
  Fix dbfs error handling (go-gitea#36844)
  Fix OAuth2 authorization code expiry and reuse handling (go-gitea#36797)
  Fix org permission API visibility checks for hidden members and private orgs (go-gitea#36798)
  Fix non-admins unable to automerge PRs from forks (go-gitea#36833)
  upgrade to github.com/cloudflare/circl 1.6.3, svgo 4.0.1, markdownlint-cli 0.48.0 (go-gitea#36837)
  Fix dump release asset bug (go-gitea#36799)
  build(deps): update material-icon-theme v5.32.0 (go-gitea#36832)
  Fix bug to check whether user can update pull request branch or rebase branch (go-gitea#36465)
  Fix forwarded proto handling for public URL detection (go-gitea#36810)
  Fix artifacts v4 backend upload problems (go-gitea#36805)
  ...

# Conflicts:
#	pnpm-lock.yaml
zjjhot added a commit to zjjhot/gitea that referenced this pull request Mar 10, 2026
* giteaofficial/main:
  Update minimum go version to 1.26.1, golangci-lint to 2.11.2, fix test style (go-gitea#36876)
  Add render cache for SVG icons (go-gitea#36863)
  Fix incorrect viewed files counter if reverted change was viewed (go-gitea#36819)
  [skip ci] Updated translations via Crowdin
  Clean up `refreshViewedFilesSummary` (go-gitea#36868)
  Remove `util.URLJoin` and replace all callers with direct path concatenation (go-gitea#36867)
  Optimize Docker build with dependency layer caching (go-gitea#36864)
  Fix URLJoin, markup render link reoslving, sign-in/up/linkaccount page common data (go-gitea#36861)
  Fix CodeQL code scanning alerts (go-gitea#36858)
  Refactor auth middleware (go-gitea#36848)
  Update Nix flake (go-gitea#36857)
  Update JS deps (go-gitea#36850)
  Load `mentionValues` asynchronously (go-gitea#36739)
  [skip ci] Updated translations via Crowdin
  Fix dbfs error handling (go-gitea#36844)
  Fix OAuth2 authorization code expiry and reuse handling (go-gitea#36797)
  Fix org permission API visibility checks for hidden members and private orgs (go-gitea#36798)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/go Pull requests that update Go code modifies/templates This PR modifies the template files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants