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: Fix to delete cookie when AppSubURL is non-empty #30375

Merged
merged 20 commits into from
Apr 14, 2024
Merged

fix: Fix to delete cookie when AppSubURL is non-empty #30375

merged 20 commits into from
Apr 14, 2024

Conversation

jtran
Copy link
Contributor

@jtran jtran commented Apr 9, 2024 •
edited
Loading

Problem

We were observing a redirect loop, either when logging out or when trying to view a private repo with an expired session. This only seems to happen when AppSubURL is non-empty.

Using release/v1.21, several commits after the 1.21.10 release, on a server recently upgraded from 1.20.

Steps to reproduce:

  1. Visit https://mydomain.com/sub_path/user/login (where ROOT_URL is configured to https://mydomain.com/sub_path/)
  2. Click Remember This Device
  3. Login
  4. In the browser's dev tools, alter the Path of the gitea cookie from /sub_path to /sub_path/
  5. Attempt to logout
  6. You will now be redirected back to the home page

The logout only clears cookies in the /sub_path, not /sub_path/. Due to PR #29599, the login page now sees the gitea cookie that can't be deleted, auto logs in, and then redirects back to the home page.

Existing code already tries to delete the cookie at /sub_path. https://github.com/go-gitea/gitea/pull/24107/files#diff-7c540b84d46e33f1e7b33c7a4cc4daed15b765c11da81070d2313b42251aa48eR48-R53. But it doesn't delete cookies at /sub_path/. #29552 changed the default value of the path, but it didn't update code that clears it.

This is problematic because the longer path is more specific as far as the browser is concerned, and the browser prefers the old, outdated cookie with the trailing slash over newly set cookies. A similar problem may occur when no path is set on the cookie since browsers use the current page's path to determine the path of the cookie. The new, correct cookie path at the root should be /, but cookies without a path set may override it.

The workaround was to clear browser cookies, which is not desirable.

Solution

This PR changes it so that whenever a cookie is written, we also clear legacy cookies that could override the cookie currently being written. In this way, cookies are lazily upgraded. In the interim, legacy cookies are still sent from the browser to the server and used. This is fine.

Cookies are written in modules/web/middleware/cookie.go, but also deep inside the chi dependency. Rather than modifying the latter, we clear legacy cookies whenever regenerating a session. This is required to handle the case of an expired session that would otherwise cause a redirect loop.

Open to suggestions on a better approach.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 9, 2024
@pull-request-size pull-request-size bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Apr 9, 2024
@github-actions github-actions bot added the modifies/go Pull requests that update Go code label Apr 9, 2024
@jtran
Copy link
Contributor Author

jtran commented Apr 9, 2024

I made a mistake. I will recreate the PR or re-open once fixes have been made.

@jtran jtran closed this Apr 9, 2024
@jtran jtran reopened this Apr 9, 2024
@pull-request-size pull-request-size bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Apr 9, 2024
@jtran jtran changed the title fix: Fix to delete cookie from root path when AppSubURL is non-empty fix: Fix to delete cookie when AppSubURL is non-empty Apr 10, 2024
wxiaoguang
wxiaoguang previously approved these changes Apr 10, 2024
View reviewed changes
@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 10, 2024
@wxiaoguang wxiaoguang added type/bug backport/v1.21 This PR should be backported to Gitea 1.21 backport/v1.22 This PR should be backported to Gitea 1.22 labels Apr 10, 2024
@wxiaoguang wxiaoguang added this to the 1.23.0 milestone Apr 10, 2024
@wxiaoguang wxiaoguang mentioned this pull request Apr 10, 2024
Merged
@jtran
Copy link
Contributor Author

jtran commented Apr 12, 2024 •
edited
Loading

Reverting the change to delete cookies with no path fixed the integration test failures. So maybe we don't want that change. I'm a little unclear about the reasoning behind this.

I think it only matters when AppSubURL is empty, and we never observed a problem with on servers where it's empty. So I think this is fine.

@jtran jtran marked this pull request as ready for review April 12, 2024 21:17
@wxiaoguang wxiaoguang merged commit b18c04e into go-gitea:main Apr 14, 2024
26 checks passed
GiteaBot pushed a commit to GiteaBot/gitea that referenced this pull request Apr 14, 2024
@jtran @wxiaoguang
@kdumontnu @GiteaBot
fix: Fix to delete cookie when AppSubURL is non-empty (go-gitea#30375)
02f6c11 
Cookies may exist on "/subpath" and "/subpath/" for some legacy reasons (eg: changed CookiePath behavior in code). The legacy cookie should be removed correctly.

---------

Co-authored-by: wxiaoguang <[email protected]>
Co-authored-by: Kyle D <[email protected]>
@GiteaBot GiteaBot mentioned this pull request Apr 14, 2024
Merged
GiteaBot pushed a commit to GiteaBot/gitea that referenced this pull request Apr 14, 2024
@jtran @wxiaoguang
@kdumontnu @GiteaBot
fix: Fix to delete cookie when AppSubURL is non-empty (go-gitea#30375)
c3902f0 
Cookies may exist on "/subpath" and "/subpath/" for some legacy reasons (eg: changed CookiePath behavior in code). The legacy cookie should be removed correctly.

---------

Co-authored-by: wxiaoguang <[email protected]>
Co-authored-by: Kyle D <[email protected]>
@GiteaBot GiteaBot mentioned this pull request Apr 14, 2024
Merged
zjjhot added a commit to zjjhot/gitea that referenced this pull request Apr 14, 2024
@zjjhot
Merge remote-tracking branch 'giteaofficial/main'
d51ac9b 
* giteaofficial/main:
  Fix JS error when opening to expanded code comment (go-gitea#30463)
  fix: Fix to delete cookie when AppSubURL is non-empty (go-gitea#30375)
  Add `interface{}` to `any` replacement to `make fmt`, exclude `*.pb.go` (go-gitea#30461)
  Fix network error when open/close organization/individual projects and redirect to project page (go-gitea#30387)
  Avoid losing token when updating mirror settings (go-gitea#30429)
  Fix label rendering (go-gitea#30456)
  Add comment for ContainsRedirectURI about the exact match (go-gitea#30457)
  Update JS and PY deps, lock eslint and related plugins (go-gitea#30452)
  Refactor cache and disable go-chi cache (go-gitea#30417)
  Fix admin notice view-detail (go-gitea#30450)
  Fix mirror error when mirror repo is empty (go-gitea#30432)
  Add `/public/assets/img/webpack` to ignore files again (go-gitea#30451)
  Lock a few tool dependencies to major versions (go-gitea#30439)
  Fix commit status cache which missed target_url (go-gitea#30426)
  Remove jQuery from the commit graph (except Fomantic) (go-gitea#30395)
  Fix rename branch 500 when the target branch is deleted but exist in database (go-gitea#30430)
  Limit the max line length when parsing git grep output (go-gitea#30418)
silverwind pushed a commit that referenced this pull request Apr 14, 2024
@GiteaBot @jtran
@wxiaoguang @kdumontnu
fix: Fix to delete cookie when AppSubURL is non-empty (#30375) (#30468)
222d16e 
Backport #30375 by @jtran

Cookies may exist on "/subpath" and "/subpath/" for some legacy reasons
(eg: changed CookiePath behavior in code). The legacy cookie should be
removed correctly.

Co-authored-by: Jonathan Tran <[email protected]>
Co-authored-by: wxiaoguang <[email protected]>
Co-authored-by: Kyle D <[email protected]>
silverwind pushed a commit that referenced this pull request Apr 14, 2024
@GiteaBot @jtran
@wxiaoguang @kdumontnu
fix: Fix to delete cookie when AppSubURL is non-empty (#30375) (#30469)
e64926c 
Backport #30375 by @jtran

Cookies may exist on "/subpath" and "/subpath/" for some legacy reasons
(eg: changed CookiePath behavior in code). The legacy cookie should be
removed correctly.

Co-authored-by: Jonathan Tran <[email protected]>
Co-authored-by: wxiaoguang <[email protected]>
Co-authored-by: Kyle D <[email protected]>
silverwind added a commit to silverwind/gitea that referenced this pull request Apr 14, 2024
@silverwind
Merge remote-tracking branch 'origin/main' into rmapi
db2e80c 
* origin/main: (35 commits)
  Remove fomantic button module (go-gitea#30475)
  Improve "must-change-password" logic and document (go-gitea#30472)
  Fix commitstatus summary (go-gitea#30431)
  Remove fomantic menu module (go-gitea#30325)
  Use `flex-container` for dashboard layout (go-gitea#30214)
  Rewrite and restyle reaction selector and enable no-sizzle eslint rule (go-gitea#30453)
  Pulse page improvements (go-gitea#30149)
  Fix JS error when opening to expanded code comment (go-gitea#30463)
  fix: Fix to delete cookie when AppSubURL is non-empty (go-gitea#30375)
  Add `interface{}` to `any` replacement to `make fmt`, exclude `*.pb.go` (go-gitea#30461)
  Fix network error when open/close organization/individual projects and redirect to project page (go-gitea#30387)
  Avoid losing token when updating mirror settings (go-gitea#30429)
  Fix label rendering (go-gitea#30456)
  Add comment for ContainsRedirectURI about the exact match (go-gitea#30457)
  Update JS and PY deps, lock eslint and related plugins (go-gitea#30452)
  Refactor cache and disable go-chi cache (go-gitea#30417)
  Fix admin notice view-detail (go-gitea#30450)
  Fix mirror error when mirror repo is empty (go-gitea#30432)
  Add `/public/assets/img/webpack` to ignore files again (go-gitea#30451)
  Lock a few tool dependencies to major versions (go-gitea#30439)
  ...
silverwind added a commit to silverwind/gitea that referenced this pull request Apr 14, 2024
@silverwind
Merge remote-tracking branch 'origin/main' into longbranch
efa7943 
* origin/main:
  Improve flex ellipsis (go-gitea#30479)
  Remove fomantic button module (go-gitea#30475)
  Improve "must-change-password" logic and document (go-gitea#30472)
  Fix commitstatus summary (go-gitea#30431)
  Remove fomantic menu module (go-gitea#30325)
  Use `flex-container` for dashboard layout (go-gitea#30214)
  Rewrite and restyle reaction selector and enable no-sizzle eslint rule (go-gitea#30453)
  Pulse page improvements (go-gitea#30149)
  Fix JS error when opening to expanded code comment (go-gitea#30463)
  fix: Fix to delete cookie when AppSubURL is non-empty (go-gitea#30375)
  Add `interface{}` to `any` replacement to `make fmt`, exclude `*.pb.go` (go-gitea#30461)
  Fix network error when open/close organization/individual projects and redirect to project page (go-gitea#30387)
  Avoid losing token when updating mirror settings (go-gitea#30429)
@jtran jtran deleted the jt/fix-cookie branch April 15, 2024 16:42
@lunny lunny added the backport/done All backports for this PR have been created label Apr 16, 2024
@wolfogre wolfogre mentioned this pull request Apr 19, 2024
Merged
wolfogre added a commit that referenced this pull request Apr 19, 2024
@wolfogre
Avoid importing modules/web/middleware in modules/session (#30584)
61457cd 
Related to #30375.

It doesn't make sense to import `modules/web/middleware` and
`modules/setting` in `modules/web/session` since the last one is more
low-level.

And it looks like a workaround to call `DeleteLegacySiteCookie` in
`RegenerateSession`, so maybe we could reverse the importing by
registering hook functions.
GiteaBot pushed a commit to GiteaBot/gitea that referenced this pull request Apr 19, 2024
@wolfogre @GiteaBot
Avoid importing modules/web/middleware in modules/session (go-git…
7b7df64 
…ea#30584)

Related to go-gitea#30375.

It doesn't make sense to import `modules/web/middleware` and
`modules/setting` in `modules/web/session` since the last one is more
low-level.

And it looks like a workaround to call `DeleteLegacySiteCookie` in
`RegenerateSession`, so maybe we could reverse the importing by
registering hook functions.
@GiteaBot GiteaBot mentioned this pull request Apr 19, 2024
Merged
GiteaBot pushed a commit to GiteaBot/gitea that referenced this pull request Apr 19, 2024
@wolfogre @GiteaBot
Avoid importing modules/web/middleware in modules/session (go-git…
ba54b83 
…ea#30584)

Related to go-gitea#30375.

It doesn't make sense to import `modules/web/middleware` and
`modules/setting` in `modules/web/session` since the last one is more
low-level.

And it looks like a workaround to call `DeleteLegacySiteCookie` in
`RegenerateSession`, so maybe we could reverse the importing by
registering hook functions.
@GiteaBot GiteaBot mentioned this pull request Apr 19, 2024
Merged
silverwind pushed a commit that referenced this pull request Apr 19, 2024
@GiteaBot @wolfogre
Avoid importing modules/web/middleware in modules/session (#30584) (
944f1ec 
#30588)

Backport #30584 by @wolfogre

Related to #30375.

It doesn't make sense to import `modules/web/middleware` and
`modules/setting` in `modules/web/session` since the last one is more
low-level.

And it looks like a workaround to call `DeleteLegacySiteCookie` in
`RegenerateSession`, so maybe we could reverse the importing by
registering hook functions.

Co-authored-by: Jason Song <[email protected]>
silverwind pushed a commit that referenced this pull request Apr 19, 2024
@GiteaBot @wolfogre
Avoid importing modules/web/middleware in modules/session (#30584) (
199397a 
#30589)

Backport #30584 by @wolfogre

Related to #30375.

It doesn't make sense to import `modules/web/middleware` and
`modules/setting` in `modules/web/session` since the last one is more
low-level.

And it looks like a workaround to call `DeleteLegacySiteCookie` in
`RegenerateSession`, so maybe we could reverse the importing by
registering hook functions.

Co-authored-by: Jason Song <[email protected]>
@wxiaoguang wxiaoguang added the skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. label Apr 27, 2024
@LauKr LauKr mentioned this pull request May 31, 2024
Closed
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Jul 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@wxiaoguang wxiaoguang wxiaoguang approved these changes

@silverwind silverwind silverwind approved these changes

@lunny lunny lunny approved these changes

Assignees
No one assigned
Labels
backport/done All backports for this PR have been created backport/v1.21 This PR should be backported to Gitea 1.21 backport/v1.22 This PR should be backported to Gitea 1.22 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 size/M Denotes a PR that changes 30-99 lines, ignoring generated files. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. type/bug
Projects
None yet
Milestone
1.23.0
Development

Successfully merging this pull request may close these issues.
6 participants
@jtran @kdumontnu @silverwind @wxiaoguang @lunny @GiteaBot