Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix open redirect check for more cases #25143

Merged
merged 4 commits into from
Jun 8, 2023
Merged

Fix open redirect check for more cases #25143

merged 4 commits into from
Jun 8, 2023

Conversation

lafriks
Copy link
Member

@lafriks lafriks commented Jun 8, 2023

If redirect_to parameter has set value starting with \\example.com redirect will be created with header Location: /\\example.com that will redirect to example.com domain.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jun 8, 2023
@pull-request-size pull-request-size bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jun 8, 2023
@lafriks lafriks added outdated/backport/v1.19 This PR should be backported to Gitea 1.19 backport/v1.20 This PR should be backported to Gitea 1.20 topic/security Something leaks user information or is otherwise vulnerable. Should be fixed! labels Jun 8, 2023
@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 Jun 8, 2023
@silverwind
Copy link
Member

How about a regex like [\\/]{2}?

@silverwind silverwind self-requested a review June 8, 2023 08:40
@GiteaBot GiteaBot added lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jun 8, 2023
@lafriks
Copy link
Member Author

lafriks commented Jun 8, 2023

I would avoid regex for this use case

@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 Jun 8, 2023
@lunny lunny mentioned this pull request Jun 8, 2023
Closed
delvh
delvh approved these changes Jun 8, 2023
View reviewed changes
modules/context/context_response.go Show resolved Hide resolved
@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 Jun 8, 2023
@lafriks lafriks enabled auto-merge (squash) June 8, 2023 13:44
@wxiaoguang
Copy link
Contributor

wxiaoguang commented Jun 8, 2023 •
edited
Loading

Add some tests?

Hmm, not easy.

@lafriks
Copy link
Member Author

lafriks commented Jun 8, 2023 •
edited
Loading

As this part (context package) of code has no unit tests that would allow creating test context etc to test on this is not easy one to add one, especially for such small change

@wxiaoguang
Copy link
Contributor

There could be separate function like isRedirectURLSafe(), then it can be tested separately.

@lafriks
Copy link
Member Author

lafriks commented Jun 8, 2023 •
edited
Loading

This still is not perfect solution as this would need to be actually context aware (like redirecting only to valid Gitea URLs etc) like stated in this FIXME comment:

gitea/routers/web/user/profile.go

Lines 365 to 366 in 263ed09

// FIXME: We should check this URL and make sure that it's a valid Gitea URL
ctx.RedirectToFirst(ctx.FormString("redirect_to"), ctx.ContextUser.HomeLink())

In future this should be fixed so I would avoid making changes just to be able to test small portion of this

@lafriks lafriks merged commit 9aaaf98 into go-gitea:main Jun 8, 2023
@GiteaBot GiteaBot added this to the 1.21.0 milestone Jun 8, 2023
@GiteaBot
Copy link
Contributor

GiteaBot commented Jun 8, 2023

I was unable to create a backport for 1.19. @lafriks, please send one manually. 🍵

go run ./contrib/backport 25143
...  // fix git conflicts if any
go run ./contrib/backport --continue

@GiteaBot GiteaBot added the backport/manual No power to the bots! Create your backport yourself! label Jun 8, 2023
@GiteaBot GiteaBot mentioned this pull request Jun 8, 2023
Merged
GiteaBot pushed a commit to GiteaBot/gitea that referenced this pull request Jun 8, 2023
@lafriks @GiteaBot
Fix open redirect check for more cases (go-gitea#25143)
8d23c1e 
If redirect_to parameter has set value starting with `\\example.com`
redirect will be created with header `Location: /\\example.com` that
will redirect to example.com domain.
@lafriks lafriks deleted the fix/redirect_check branch June 8, 2023 14:22
lafriks added a commit to lafriks-fork/gitea that referenced this pull request Jun 8, 2023
@lafriks
Fix open redirect check for more cases (go-gitea#25143)
303ecbe 
If redirect_to parameter has set value starting with `\\example.com`
redirect will be created with header `Location: /\\example.com` that
will redirect to example.com domain.
@lafriks lafriks mentioned this pull request Jun 8, 2023
Merged
@lafriks lafriks added the backport/done All backports for this PR have been created label Jun 8, 2023
silverwind pushed a commit that referenced this pull request Jun 8, 2023
@GiteaBot @lafriks
Fix open redirect check for more cases (#25143) (#25154)
7679f4d 
Backport #25143 by @lafriks

If redirect_to parameter has set value starting with `\\example.com`
redirect will be created with header `Location: /\\example.com` that
will redirect to example.com domain.

Co-authored-by: Lauris BH <lauris@nix.lv>
lafriks added a commit that referenced this pull request Jun 8, 2023
@lafriks
Fix open redirect check for more cases (#25143) (#25155)
a903005 
Backport #25143

If redirect_to parameter has set value starting with \\example.com
redirect will be created with header Location: /\\example.com that will
redirect to example.com domain.
silverwind added a commit to silverwind/gitea that referenced this pull request Jun 8, 2023
@silverwind
Merge branch 'main' into buttonfix
757ca22 
* main:
  Modify OAuth login ui and fix display name, iconurl related logic (go-gitea#25030)
  Fix open redirect check for more cases (go-gitea#25143)
  Update js dependencies (go-gitea#25137)
  Remove duplicated functions when deleting a branch (go-gitea#25128)
  Add codeowners feature (go-gitea#24910)
  Fix strange UI behavior of cancelling dismiss review modal (go-gitea#25133)
  Fix `MilestoneIDs` when querying issues (go-gitea#25125)
  Fix incorrect git ignore rule and add missing license files (go-gitea#25135)
  Change branch name from master to main in some documents' links (go-gitea#25126)
  Remove incorrect element ID on "post-install" page (go-gitea#25104)
  [skip ci] Updated translations via Crowdin
  Improve notification icon and navbar  (go-gitea#25111)
  fix swagger documentation for multiple files API endpoint (go-gitea#25110)
zjjhot added a commit to zjjhot/gitea that referenced this pull request Jun 9, 2023
@zjjhot
Merge remote-tracking branch 'upstream/main'
482bdb7 
* upstream/main:
  [skip ci] Updated translations via Crowdin
  Modify OAuth login ui and fix display name, iconurl related logic (go-gitea#25030)
  Fix open redirect check for more cases (go-gitea#25143)
  Update js dependencies (go-gitea#25137)
  Remove duplicated functions when deleting a branch (go-gitea#25128)
  Add codeowners feature (go-gitea#24910)
  Fix strange UI behavior of cancelling dismiss review modal (go-gitea#25133)
  Fix `MilestoneIDs` when querying issues (go-gitea#25125)
  Fix incorrect git ignore rule and add missing license files (go-gitea#25135)
  Change branch name from master to main in some documents' links (go-gitea#25126)
  Remove incorrect element ID on "post-install" page (go-gitea#25104)
  [skip ci] Updated translations via Crowdin
  Improve notification icon and navbar  (go-gitea#25111)
  fix swagger documentation for multiple files API endpoint (go-gitea#25110)
  Fix webauthn regression and improve code (go-gitea#25113)
  Add details summary for vertical menus in settings to allow toggling (go-gitea#25098)
  Fix 500 error caused by notifications without an issue such as repo transfers (go-gitea#25101)
@wxiaoguang
Copy link
Contributor

@lafriks I moved the code into IsRiskyRedirectURL in #25258 , and wrote some tests to cover it, please help to review, thank you.

Codeberg-org pushed a commit to Codeberg-org/gitea that referenced this pull request Jun 23, 2023
@lafriks @earl-warren
Fix open redirect check for more cases (go-gitea#25143) (go-gitea#25155)
a259a92 
Backport go-gitea#25143

If redirect_to parameter has set value starting with \\example.com
redirect will be created with header Location: /\\example.com that will
redirect to example.com domain.

(cherry picked from commit a903005)
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Sep 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@delvh delvh delvh approved these changes

@silverwind silverwind silverwind approved these changes

Assignees
No one assigned
Labels
backport/done All backports for this PR have been created backport/manual No power to the bots! Create your backport yourself! backport/v1.20 This PR should be backported to Gitea 1.20 lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. outdated/backport/v1.19 This PR should be backported to Gitea 1.19 size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. topic/security Something leaks user information or is otherwise vulnerable. Should be fixed!
Projects
None yet
Milestone
1.21.0
Development

Successfully merging this pull request may close these issues.
5 participants
@lafriks @silverwind @wxiaoguang @GiteaBot @delvh