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.

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

Fix incorrect cookie path for AppSubURL #29534

Merged
merged 2 commits into from
Mar 3, 2024

Conversation

wxiaoguang
Copy link
Contributor

@wxiaoguang wxiaoguang commented Mar 2, 2024

Ref: https://developer.mozilla.org/en-US/docs/Web/HTTP/Cookies

Regression of #24107 : at that time, I was misled by the comment Cookie path to store. Default is "/": because when AppSubURL is empty, the cookie path is not defaulted to "/". So I added a slash there. But when there is a sub path, the extra slash is wrong.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 2, 2024
@pull-request-size pull-request-size bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Mar 2, 2024
@wxiaoguang wxiaoguang added this to the 1.22.0 milestone Mar 2, 2024
@wxiaoguang wxiaoguang added type/bug backport/v1.21 This PR should be backported to Gitea 1.21 labels Mar 2, 2024
@wxiaoguang wxiaoguang enabled auto-merge (squash) March 2, 2024 05:23
@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 Mar 2, 2024
@OdinVex
Copy link

OdinVex commented Mar 2, 2024

How would this fix #29533? That BR mentions all the other links are fine, only pagination is broken because of a missing slash in it alone.

Edit: The cookie thing might be one issue but I'm talking about the pagination being an issue. They need a slash, the link generation sections...

@wxiaoguang
Copy link
Contributor Author

How would this fix #29533? That BR mentions all the other links are fine, only pagination is broken because of a missing slash in it alone.

#29533 (comment)

@OdinVex
Copy link

OdinVex commented Mar 2, 2024

How would this fix #29533? That BR mentions all the other links are fine, only pagination is broken because of a missing slash in it alone.

#29533 (comment)

See updated edit. The cookie thing might be a problem but I'm talking about another issue, pagination.

@wxiaoguang
Copy link
Contributor Author

wxiaoguang commented Mar 2, 2024

See updated edit. The cookie thing might be a problem but I'm talking about another issue, pagination.

I think there are 2 problems:

  1. Unable to click the links in pagination, incorrect cooke path leads to invalid session (logout)
  2. The URL generation in pagination

This PR's purpose is to fix the first one: make end users could use subpath correctly.

For the second one, at the moment I didn't see a easy&clear fix for it. If the generated URLs don't affect daily use, I think we could have more time to do a big refactoring in the future to figure out how to handle the "$.Link" properly. Because the "Link" is just used this way widely, and "paginate.tmpl" is a general template used by many pages, not only for the home page (dashboard).

image

@wxiaoguang wxiaoguang disabled auto-merge March 2, 2024 17:01
@wxiaoguang wxiaoguang enabled auto-merge (squash) March 2, 2024 17:02
@OdinVex
Copy link

OdinVex commented Mar 2, 2024

The URL generation in pagination

Yes, there is an issue in the URL generation, the lack of a forward-slash and it appears to only affect the user dashboard itself at a quick glance. I checked the Instance in repo issues and those generate correct links. This appears isolated to the user dashboard.

@wxiaoguang
Copy link
Contributor Author

wxiaoguang commented Mar 2, 2024

The URL generation in pagination

Yes, there is an issue in the URL generation, the lack of a forward-slash and it appears to only affect the user dashboard itself at a quick glance. I checked the Instance in repo issues and those generate correct links. This appears isolated to the user dashboard.

Gitea itself never adds trailing slash (update: to the full paths). If you see trailing slashes (in the full paths), are they added by your reverse proxy?

@OdinVex
Copy link

OdinVex commented Mar 2, 2024

Gitea itself never adds trailing slash. If you see trailing slashes, are they added by your reverse proxy?

We're not talking about a trailing forward-slash.

Gitea adds the forward-slash after SUBPATH on everything else, just not pagination. They are not added by my proxy, it's Apache with no content-editing at all.

Example:

<a href="/SUBPATH/assets/licenses.txt">Licenses</a>
<a href="/SUBPATH/api/swagger">API</a>

That's in the actual source on every page (browser, view source). Only the dashboard's pagination is broken. Every other page appears fine and with a forward-slash correctly after SUBPATH.

@wxiaoguang
Copy link
Contributor Author

I see your point. The real problem is how Gitea uses the "SUBPATH".

Gitea requires that "SUBPATH" doesn't have a trailing slash, because it is widely used like this: "{SUBPATH}/other-path". They are just hare-coded in many places, and not easy to change.

The definition is:

image

So at the moment, we can't simply change its definition or behavior. That's what I meant in #29534 (comment)

For the second one, at the moment I didn't see a easy&clear fix for it. If the generated URLs don't affect daily use, I think we could have more time to do a big refactoring in the future to figure out how to handle the "$.Link" properly. Because the "Link" is just used this way widely, and "paginate.tmpl" is a general template used by many pages, not only for the home page (dashboard).

@OdinVex
Copy link

OdinVex commented Mar 2, 2024

It does affect daily use but it's somewhat minor. Users simply can't go backward or forward in their dashboard history without editing the URL themselves.

Edit: Perhaps the refactoring should simply have every link not add a forward-slash and have everyone end their SUBPATH in forward-slash (or append if missing). That might be the easiest way to refactor, but it's probably a major revision rather than minor given some parsers out there might need updating and such.

@wxiaoguang
Copy link
Contributor Author

wxiaoguang commented Mar 2, 2024

It does affect daily use but it's somewhat minor. Users simply can't go backward or forward in their dashboard history without editing the URL themselves.

Could you elaborate? What's wrong happens in this case if a user go backward or forward?

By design: https://host/gitea should work the same as https://host/gitea/, no matter whether there is a trailing slash.

@OdinVex
Copy link

OdinVex commented Mar 2, 2024

It does affect daily use but it's somewhat minor. Users simply can't go backward or forward in their dashboard history without editing the URL themselves.

Could you elaborate? What's wrong happens in this case if a user go backward or forward?

By design: https://host/gitea should work the same as https://host/gitea/, no matter whether there is a trailing slash.

/gitea would be a file, /gitea/ would be a subpath. Servers should treat those correctly as such, that's standard. If there is no file by the same name then it could default to a directory, but the cookie issue just redirects them to the login page (no way around it unless a user adjusts the URL).

@wxiaoguang
Copy link
Contributor Author

wxiaoguang commented Mar 2, 2024

/gitea would be a file, /gitea/ would be a subpath. Servers should treat those correctly as such, that's standard. If there is no file by the same name then it could default to a directory, but the cookie issue just redirects them to the login page (no way around it unless a user adjusts the URL).

But for modern web servers, they never serve /gitea and /gitea/ for different purposes. It is just a "proxy path" to the backend.

For the "cookie issue", that's what we are fixing now. According to MDN: it is right to use "/subpath" as cookie path, and that's what Gitea's old code did. After the fix, everything just works fine.

@OdinVex
Copy link

OdinVex commented Mar 2, 2024

But for modern web servers, they never serve /gitea and /gitea/ for different purposes. It is just a "proxy path" to the backend.

They are different, one is a file and the other is a directory. Modern web servers respect that. I've double-checked it with an Ubuntu server running Apache. They've respected the differentiation since at least 2004. Edit: If there is no file called 'gitea' it will assume you meant to use the directory but the backend must be able to understand that (Gitea does, but not the cookie logic, that is what you found). Regardless, the issue of pagination is the only one I'm concerned with.

@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 2, 2024
@wxiaoguang
Copy link
Contributor Author

But for modern web servers, they never serve /gitea and /gitea/ for different purposes. It is just a "proxy path" to the backend.

They are different, one is a file and the other is a directory. Modern web servers respect that. I've double-checked it with an Ubuntu server running Apache. They've respected the differentiation since at least 2004. Edit: If there is no file called 'gitea' it will assume you meant to use the directory but the backend must be able to understand that (Gitea does, but not the cookie logic, that is what you found). Regardless, the issue of pagination is the only one I'm concerned with.

I mean when you configure the "proxy pass" for Gitea, they never serve /gitea and /gitea/ for different purposes. It is just a "proxy path". I think it is off-topic in this PR to discuss how to configure subpath for Gitea, but indeed it is well-documented. It must configure "/gitea" to work with "/gitea/". The whole system was designed to work this way long time ago. And TBH, I don't see a real requirement for end users to make "/gitea" and "/gitea/" work differently.

https://docs.gitea.com/administration/reverse-proxies#apache-httpd-with-a-sub-path

image

@wxiaoguang wxiaoguang merged commit 44398e4 into go-gitea:main Mar 3, 2024
26 checks passed
GiteaBot pushed a commit to GiteaBot/gitea that referenced this pull request Mar 3, 2024
@GiteaBot GiteaBot added the backport/done All backports for this PR have been created label Mar 3, 2024
@wxiaoguang wxiaoguang deleted the fix-cookie-path branch March 3, 2024 00:26
wxiaoguang added a commit that referenced this pull request Mar 3, 2024
@OdinVex
Copy link

OdinVex commented Mar 3, 2024

Respectfully, I don't care what Gitea practices regarding files versus directories, that's a matter for RFC specs and I'm sticking to them for standards. Agreed it's off-topic, so I won't bother replying about it.

zjjhot added a commit to zjjhot/gitea that referenced this pull request Mar 3, 2024
* upstream/main:
  Breaking summary for template refactoring (go-gitea#29395)
  [skip ci] Updated translations via Crowdin
  Fix incorrect cookie path for AppSubURL (go-gitea#29534)
  gitea.service: Remove syslog.target (go-gitea#29550)
  Add option to set language in admin user view (go-gitea#28449)
  Fix elipsis button not working if the last commit loading is deferred (go-gitea#29544)
  Fix incorrect relative/absolute URL usages (go-gitea#29531)
  Add support for API blob upload of release attachments (go-gitea#29507)
  Fix queue worker incorrectly stopped when there are still more items in the queue (go-gitea#29532)
  remove util.OptionalBool and related functions (go-gitea#29513)
  Rename Action.GetDisplayName to GetActDisplayName (go-gitea#29540)
  Make PR form use toast to show error message (go-gitea#29545)
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport/done All backports for this PR have been created backport/v1.21 This PR should be backported to Gitea 1.21 lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants