Skip to content

Fix URL related escaping for oauth2#37334

Merged
wxiaoguang merged 3 commits intogo-gitea:mainfrom
wxiaoguang:fix-url-escape
Apr 21, 2026
Merged

Fix URL related escaping for oauth2#37334
wxiaoguang merged 3 commits intogo-gitea:mainfrom
wxiaoguang:fix-url-escape

Conversation

@wxiaoguang
Copy link
Copy Markdown
Contributor

@wxiaoguang wxiaoguang commented Apr 21, 2026

Follow up #37327. See the comments.

  • Root problem: the design of OAuth2 providers is a mess, the display name is used as provider's name and used in the URL directly
  • The regressions:
  • This fix: always use "path escaping" for the path part, add more tests to cover all escaping cases.

Now, frontend "pathEscape" and "pathEscapeSegments" generate exactly the same result as backend.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 21, 2026
@wxiaoguang wxiaoguang added backport/v1.26 This PR should be backported to Gitea 1.26 type/bug labels Apr 21, 2026
@wxiaoguang wxiaoguang marked this pull request as draft April 21, 2026 08:44
Comment thread modules/templates/helper.go Outdated
@wxiaoguang wxiaoguang force-pushed the fix-url-escape branch 2 times, most recently from 487767d to a53c164 Compare April 21, 2026 10:24
@wxiaoguang wxiaoguang changed the title Fix QueryEscape Fix URL related escaping for oauth2 Apr 21, 2026
@wxiaoguang wxiaoguang requested a review from Copilot April 21, 2026 10:37
@wxiaoguang wxiaoguang marked this pull request as ready for review April 21, 2026 10:40
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 fixes inconsistent URL escaping for OAuth2 auth source names when used as URL path segments, aligning frontend, templates, and backend routing/lookup behavior and adding tests to cover special-character cases.

Changes:

  • Switch OAuth2 provider links/callback URL generation from query-escaping to path-escaping (spaces %20, consistent handling of +, ', etc.).
  • Add/adjust frontend and backend tests to lock in escaping behavior for query vs path components.
  • Treat “OAuth2 source not found” as a not-exist error so it can be handled as a 404 instead of a 500.

Reviewed changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
web_src/js/utils/url.ts Adds urlQueryEscape and pathEscape, and updates pathEscapeSegments to match backend escaping semantics.
web_src/js/utils/url.test.ts Adds unit tests for query/path escaping and segment escaping cases (space/plus).
web_src/js/utils.ts Removes urlQueryEscape from the generic utils module (relocated to utils/url.ts).
web_src/js/utils.test.ts Removes urlQueryEscape test/import now that function moved.
web_src/js/features/admin/common.ts Uses path escaping when showing the OAuth2 callback URL on the admin auth source page.
tests/integration/oauth_test.go Expands integration coverage for OAuth source names with spaces/plus and verifies rendered login links.
templates/user/settings/security/accountlinks.tmpl Path-escapes OAuth2 source names when generating account-link URLs.
templates/user/auth/external_auth_methods.tmpl Path-escapes OAuth2 provider display names in login links.
services/context/context_response.go Adds ErrNotExist→404 handling in ServerError and changes 404 plaintext body behavior.
services/context/base_path.go Adds SetPathParamRaw for pre-escaped route params (primarily useful for tests).
routers/web/auth/oauth.go Decodes provider using PathParam (path-unescape) rather than query-unescape.
routers/web/auth/auth_test.go Updates OAuth2 auth tests for special characters and raw path-param setting.
routers/web/auth/auth.go Uses url.PathEscape for OAuth2 provider path generation during auto-redirect.
modules/util/util_test.go Adds a backend test for PathEscapeSegments behavior (space/plus).
modules/templates/helper_test.go Adds reference tests for Go’s QueryEscape vs PathEscape outputs.
models/auth/oauth2.go Returns util.NewNotExistErrorf when an OAuth2 source name isn’t found.

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

Comment thread services/context/context_response.go Outdated
Comment thread tests/integration/oauth_test.go
Comment thread tests/integration/oauth_test.go Outdated
@wxiaoguang
Copy link
Copy Markdown
Contributor Author

Should be 100% correct now.

Comment thread routers/web/auth/oauth.go
Comment thread tests/integration/oauth_test.go
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Apr 21, 2026
@silverwind
Copy link
Copy Markdown
Member

silverwind commented Apr 21, 2026

And btw sorry for the invalid reviews from GPT-5.4. This was an experiment. It took like 10 times as long as Claude would and produced shitty results, so I will be more careful reviewing with that model in the future.

@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 Apr 21, 2026
@wxiaoguang wxiaoguang merged commit aee6628 into go-gitea:main Apr 21, 2026
26 checks passed
@GiteaBot GiteaBot added this to the 1.27.0 milestone Apr 21, 2026
@wxiaoguang wxiaoguang deleted the fix-url-escape branch April 21, 2026 15:58
@GiteaBot GiteaBot added the backport/done All backports for this PR have been created label Apr 21, 2026
wxiaoguang added a commit that referenced this pull request Apr 21, 2026
Backport #37334 by wxiaoguang

Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
zjjhot added a commit to zjjhot/gitea that referenced this pull request Apr 22, 2026
* main: (25 commits)
  Add URL to `Learn more about blocking a user` (go-gitea#37355)
  fix: use TriggerEvent instead of Event in workflow runs API response for scheduled runs (go-gitea#37288)
  Add event.schedule context for schedule actions task (go-gitea#37320)
  Fix typos (go-gitea#37346)
  Fix an issue where changing an organization’s visibility caused problems when users had forked its repositories. (go-gitea#37324)
  Fail vite build on rolldown warnings via NODE_ENV=test (go-gitea#37270)
  Use modern "git update-index --cacheinfo" syntax to support more file names (go-gitea#37338)
  Fix URL related escaping for oauth2 (go-gitea#37334)
  When the requested arch rpm is missing fall back to noarch (go-gitea#37236)
  Fix `relative-time` error and improve global error handler (go-gitea#37241)
  Enhance styling in actions page (go-gitea#37323)
  fix(oauth): Error on auth sources with spaces (go-gitea#37327)
  Fix actions concurrency groups cross-branch leak (go-gitea#37311)
  Fix bug when accessing user badges (go-gitea#37321)
  Fix AppFullLink (go-gitea#37325)
  Update go js dependencies (go-gitea#37312)
  Update GitHub Actions to latest major versions (go-gitea#37313)
  Revert "Add WebKit to e2e test matrix (go-gitea#37298)" (go-gitea#37315)
  Add `form-fetch-action` to some forms, fix "fetch action" resp bug (go-gitea#37305)
  Move heatmap to first-party code (go-gitea#37262)
  ...
silverwind added a commit to silverwind/gitea that referenced this pull request Apr 23, 2026
* origin/main: (32 commits)
  fix: commit status reporting (go-gitea#37372)
  Support for Custom URI Schemes in OAuth2 Redirect URIs (go-gitea#37356)
  Fix cmd tests by mocking builtin paths (go-gitea#37369)
  chore: upgrade Go version in devcontainer image to 1.26 (go-gitea#37374)
  Fix button layout shift when collapsing file tree in editor (go-gitea#37363)
  Update `Block a user` form (go-gitea#37359)
  Remove IsValidExternalURL/IsAPIURL and use IsValidURL at call sites (go-gitea#37364)
  Add URL to `Learn more about blocking a user` (go-gitea#37355)
  fix: use TriggerEvent instead of Event in workflow runs API response for scheduled runs (go-gitea#37288)
  Add event.schedule context for schedule actions task (go-gitea#37320)
  Fix typos (go-gitea#37346)
  Fix an issue where changing an organization’s visibility caused problems when users had forked its repositories. (go-gitea#37324)
  Fail vite build on rolldown warnings via NODE_ENV=test (go-gitea#37270)
  Use modern "git update-index --cacheinfo" syntax to support more file names (go-gitea#37338)
  Fix URL related escaping for oauth2 (go-gitea#37334)
  When the requested arch rpm is missing fall back to noarch (go-gitea#37236)
  Fix `relative-time` error and improve global error handler (go-gitea#37241)
  Enhance styling in actions page (go-gitea#37323)
  fix(oauth): Error on auth sources with spaces (go-gitea#37327)
  Fix actions concurrency groups cross-branch leak (go-gitea#37311)
  ...

# Conflicts:
#	services/actions/commit_status.go
silverwind added a commit to silverwind/gitea that referenced this pull request Apr 23, 2026
* origin/main: (204 commits)
  fix: commit status reporting (go-gitea#37372)
  Support for Custom URI Schemes in OAuth2 Redirect URIs (go-gitea#37356)
  Fix cmd tests by mocking builtin paths (go-gitea#37369)
  chore: upgrade Go version in devcontainer image to 1.26 (go-gitea#37374)
  Fix button layout shift when collapsing file tree in editor (go-gitea#37363)
  Update `Block a user` form (go-gitea#37359)
  Remove IsValidExternalURL/IsAPIURL and use IsValidURL at call sites (go-gitea#37364)
  Add URL to `Learn more about blocking a user` (go-gitea#37355)
  fix: use TriggerEvent instead of Event in workflow runs API response for scheduled runs (go-gitea#37288)
  Add event.schedule context for schedule actions task (go-gitea#37320)
  Fix typos (go-gitea#37346)
  Fix an issue where changing an organization’s visibility caused problems when users had forked its repositories. (go-gitea#37324)
  Fail vite build on rolldown warnings via NODE_ENV=test (go-gitea#37270)
  Use modern "git update-index --cacheinfo" syntax to support more file names (go-gitea#37338)
  Fix URL related escaping for oauth2 (go-gitea#37334)
  When the requested arch rpm is missing fall back to noarch (go-gitea#37236)
  Fix `relative-time` error and improve global error handler (go-gitea#37241)
  Enhance styling in actions page (go-gitea#37323)
  fix(oauth): Error on auth sources with spaces (go-gitea#37327)
  Fix actions concurrency groups cross-branch leak (go-gitea#37311)
  ...

# Conflicts:
#	web_src/js/index-domready.ts
#	web_src/js/markup/content.ts
#	web_src/js/markup/refissue.ts
silverwind added a commit to silverwind/gitea that referenced this pull request Apr 23, 2026
* origin/main: (204 commits)
  fix: commit status reporting (go-gitea#37372)
  Support for Custom URI Schemes in OAuth2 Redirect URIs (go-gitea#37356)
  Fix cmd tests by mocking builtin paths (go-gitea#37369)
  chore: upgrade Go version in devcontainer image to 1.26 (go-gitea#37374)
  Fix button layout shift when collapsing file tree in editor (go-gitea#37363)
  Update `Block a user` form (go-gitea#37359)
  Remove IsValidExternalURL/IsAPIURL and use IsValidURL at call sites (go-gitea#37364)
  Add URL to `Learn more about blocking a user` (go-gitea#37355)
  fix: use TriggerEvent instead of Event in workflow runs API response for scheduled runs (go-gitea#37288)
  Add event.schedule context for schedule actions task (go-gitea#37320)
  Fix typos (go-gitea#37346)
  Fix an issue where changing an organization’s visibility caused problems when users had forked its repositories. (go-gitea#37324)
  Fail vite build on rolldown warnings via NODE_ENV=test (go-gitea#37270)
  Use modern "git update-index --cacheinfo" syntax to support more file names (go-gitea#37338)
  Fix URL related escaping for oauth2 (go-gitea#37334)
  When the requested arch rpm is missing fall back to noarch (go-gitea#37236)
  Fix `relative-time` error and improve global error handler (go-gitea#37241)
  Enhance styling in actions page (go-gitea#37323)
  fix(oauth): Error on auth sources with spaces (go-gitea#37327)
  Fix actions concurrency groups cross-branch leak (go-gitea#37311)
  ...

Co-Authored-By: Claude (Opus 4.7) <noreply@anthropic.com>

# Conflicts:
#	web_src/js/index-domready.ts
#	web_src/js/markup/content.ts
#	web_src/js/markup/refissue.ts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport/done All backports for this PR have been created backport/v1.26 This PR should be backported to Gitea 1.26 lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

OAuth2 Authentication Service Redirect URL Does Not Encode Apostrophes

5 participants