[Bug] User dashboard doesn't have forward-slashes unlike other pages, resulting in incorrectly generated "pagination" navigation links for subpath-enabled instances #12574

New Issue

@wxiaoguang commented on GitHub (Mar 2, 2024):

Oops .... I found the root problem now. That's my fault (regression of #24107)

-> Fix incorrect cookie path for AppSubURL #29534

@wxiaoguang commented on GitHub (Mar 2, 2024): Oops .... I found the root problem now. That's my fault (regression of #24107) -> Fix incorrect cookie path for AppSubURL #29534

GiteaMirror commented

@OdinVex commented on GitHub (Mar 2, 2024):

To reclarify, this issue is about pagination, in the template for links generated (First, Previous, #, Next, Last...) on the user dashboard.

Edit: Pagination is not using {{.Link}}/, it is using {{$.Link}} without a forward-slash, as I mentioned. I think those need updating.

@OdinVex commented on GitHub (Mar 2, 2024): To reclarify, this issue is about pagination, in the template for links generated (First, Previous, #, Next, Last...) on the user dashboard. Edit: Pagination is not using `{{.Link}}/`, it is using `{{$.Link}}` without a forward-slash, as I mentioned. I think those need updating.

GiteaMirror commented

@wxiaoguang commented on GitHub (Mar 2, 2024):

More details in https://github.com/go-gitea/gitea/pull/29534#issuecomment-1974851486

If you focus on "fixing the trailing slash", I could remove the "fix" operation from that PR, and keep this issue open.

@wxiaoguang commented on GitHub (Mar 2, 2024): More details in https://github.com/go-gitea/gitea/pull/29534#issuecomment-1974851486 If you focus on "fixing the trailing slash", I could remove the "fix" operation from that PR, and keep this issue open.

GiteaMirror commented

@OdinVex commented on GitHub (Mar 2, 2024):

I'm unfamiliar with Go, I'm more of an Assembly/C/C++ kind of guy.

Edit: I took a peek around, I think maybe something in templates/user/dashboard/feeds.tmpl could address it? {{template "base/paginate" .}} That period, does that help to generate a link?

@OdinVex commented on GitHub (Mar 2, 2024): I'm unfamiliar with Go, I'm more of an Assembly/C/C++ kind of guy. Edit: I took a peek around, I think maybe something in `templates/user/dashboard/feeds.tmpl` could address it? `{{template "base/paginate" .}}` That period, does that help to generate a link?

GiteaMirror commented

@wxiaoguang commented on GitHub (Mar 2, 2024):

Edit: I took a peek around, I think maybe something in templates/user/dashboard/feeds.tmpl could address it? {{template "base/paginate" .}} That period, does that help to generate a link?

Nope. The dot syntax is a Golang template syntax, it means "pass all current data into the sub template"

@wxiaoguang commented on GitHub (Mar 2, 2024): > Edit: I took a peek around, I think maybe something in `templates/user/dashboard/feeds.tmpl` could address it? `{{template "base/paginate" .}}` That period, does that help to generate a link? Nope. The `dot syntax` is a Golang template syntax, it means "pass all current data into the sub template"

GiteaMirror commented

@lunny commented on GitHub (Mar 3, 2024):

resolved by #29534

@lunny commented on GitHub (Mar 3, 2024): resolved by #29534

GiteaMirror commented

@OdinVex commented on GitHub (Mar 3, 2024):

resolved by #29534

It is not resolved by #29534, that was a different issue (original fixer misunderstood this bug because another was found at the same time.)

@OdinVex commented on GitHub (Mar 3, 2024): > resolved by #29534 It is not resolved by #29534, that was a different issue (original fixer misunderstood *this* bug because another was found at the same time.)

GiteaMirror commented

@OdinVex commented on GitHub (Mar 3, 2024):

Thank you @lunny. :)

@OdinVex commented on GitHub (Mar 3, 2024): Thank you @lunny. :)

GiteaMirror commented

@wxiaoguang commented on GitHub (Mar 3, 2024):

original fixer misunderstood this bug because another was found at the same time.

Well, TBH I do not think I misunderstand this issue (actually I won't call the "slash" problem is a "bug").

Maybe you misunderstand the problem. I will quote my explanations and summarize them again:

The fact is: it is a requirement that making Gitea work with both https://host/SUBPATH and https://host/SUBPATH/, the document has said so:
- https://docs.gitea.com/administration/reverse-proxies#apache-httpd-with-a-sub-path
- # Note: no trailing slash after either /git or port
Based on the fact, a URL href="/SUBPATH?page=2... should work, it doesn't need an extra slash.
Gitea never adds extra slash to a full path
- so you only see: https://host/SUBPATH , https://host/SUBPATH/user, https://host/SUBPATH/user/repo
- you won't see: https://host/SUBPATH/ , https://host/SUBPATH/user/, https://host/SUBPATH/user/repo/
The root problem of your bug report is: "Without the forward-slash users are told to login again (Gitea destroys the session maybe?)."
- I think it is caused by incorrect cookie path
- After the fix (29534), I think it should have been resolved. Users won't get logout, nor they need to "manually edit the URL"

I don't see why you wouldn't follow the document and why you insist to add an extra trailing slash for some links, I don't see such a real use-case for end users either.

If you think the trailing slash is a must, please show some real bad cases for no-trailing-slash. Without a real bad case, I couldn't figure out what you mean.

@wxiaoguang commented on GitHub (Mar 3, 2024): > original fixer misunderstood this bug because another was found at the same time. Well, TBH I do not think I misunderstand this issue (actually I won't call the "slash" problem is a "bug"). Maybe you misunderstand the problem. I will quote my explanations and summarize them again: 1. The fact is: it is a requirement that making Gitea work with both `https://host/SUBPATH` and `https://host/SUBPATH/`, the document has said so: * https://docs.gitea.com/administration/reverse-proxies#apache-httpd-with-a-sub-path * `# Note: no trailing slash after either /git or port` 2. Based on the fact, a URL `href="/SUBPATH?page=2...` should work, it doesn't need an extra slash. 3. Gitea never adds extra slash to a full path * so you only see: `https://host/SUBPATH` , `https://host/SUBPATH/user`, `https://host/SUBPATH/user/repo` * you won't see: `https://host/SUBPATH/` , `https://host/SUBPATH/user/`, `https://host/SUBPATH/user/repo/` 4. The root problem of your bug report is: "Without the forward-slash users are told to login again (Gitea destroys the session maybe?)." * I think it is caused by incorrect cookie path * After the fix (29534), I think it should have been resolved. Users won't get logout, nor they need to "manually edit the URL" I don't see why you wouldn't follow the document and why you insist to add an extra trailing slash for some links, I don't see such a real use-case for end users either. If you think the trailing slash is a must, please show some real bad cases for no-trailing-slash. Without a real bad case, I couldn't figure out what you mean.

GiteaMirror commented

@OdinVex commented on GitHub (Mar 3, 2024):

The fact Gitea uses a forward-slash to begin with is enough to follow convention. Calling #29534 a fix is a hackjob following non-standard practice to assume a file is a directory to circumvent fixing the issue.

Edit: The root of the problem has nothing to do with the Cookie issue. That is a side-effect. The issue of parsing comes into play, mind. Some people build parsers and to have to create an exception for mis-generated links in pagination on one specific page just lends to the idea it should be polished and fixed, not to mention web server parsing specs.

@OdinVex commented on GitHub (Mar 3, 2024): The fact Gitea uses a forward-slash to begin with is enough to follow convention. Calling #29534 a fix is a hackjob following non-standard practice to *assume* a file is a directory to circumvent fixing the issue. Edit: The root of the problem has nothing to do with the Cookie issue. That is a side-effect. The issue of parsing comes into play, mind. Some people build parsers and to have to create an exception for mis-generated links in pagination on one specific page just lends to the idea it should be polished and fixed, not to mention web server parsing specs.

GiteaMirror commented

@wxiaoguang commented on GitHub (Mar 3, 2024):

Calling #29534 a fix is a hackjob following non-standard practice to assume a file is a directory to circumvent fixing the issue.

As I explained before, Gitea requires to make "/SUBPATH" the same as "/SUBPATH/". Since it is a "proxy path", it is not related to file or directory. There is no file nor directory in this case.

@wxiaoguang commented on GitHub (Mar 3, 2024): > Calling #29534 a fix is a hackjob following non-standard practice to _assume_ a file is a directory to circumvent fixing the issue. As I explained before, Gitea requires to make "/SUBPATH" the same as "/SUBPATH/". Since it is a "proxy path", it is not related to file or directory. There is no file nor directory in this case.

GiteaMirror commented

@OdinVex commented on GitHub (Mar 3, 2024):

Calling #29534 a fix is a hackjob following non-standard practice to assume a file is a directory to circumvent fixing the issue.

As I explained before, Gitea requires to make "/SUBPATH" the same as "/SUBPATH/". Since it is a "proxy path", it is not related to file or directory. There is no file nor directory in this case.

Maybe not on your instance or Gitea's official instances. I happen to have a binary file named the same. Access to the instance can only be achieved with that slash, as intended by specs, not just my own. Edit: And yes, /SUBPATH/ is a directory, even if virtual. You might be thinking a piece of path rather than directory as in "folder", the term is interchangeable, but it is indeed intended for the server to provide a default ("app") in this case or conventionally index/default.html or return an error, etc.

Edit: To summarize, the issue is a lack of slashes. Your PR does not add slashes, it fixes a different issue (awesome). So when the slashes are added to the user dashboard (the only place this problem occurs) then we'll call it fixed, cause that's this Issue.

@OdinVex commented on GitHub (Mar 3, 2024): > > Calling #29534 a fix is a hackjob following non-standard practice to _assume_ a file is a directory to circumvent fixing the issue. > > As I explained before, Gitea requires to make "/SUBPATH" the same as "/SUBPATH/". Since it is a "proxy path", it is not related to file or directory. There is no file nor directory in this case. Maybe not on your instance or Gitea's official instances. I happen to have a binary file named the same. Access to the instance can only be achieved with that slash, as intended by specs, not just my own. Edit: And yes, `/SUBPATH/` is a directory, even if virtual. You might be thinking a piece of path rather than directory as in "folder", the term is interchangeable, but it is indeed intended for the server to provide a default ("app") in this case or conventionally index/default.html or return an error, etc. Edit: To summarize, the issue is a lack of slashes. Your PR does not add slashes, it fixes a different issue (awesome). So when the slashes are added to the user dashboard (the only place this problem occurs) then we'll call it fixed, cause that's this Issue.

GiteaMirror commented

@wxiaoguang commented on GitHub (Mar 3, 2024):

https://docs.gitea.com/administration/reverse-proxies#apache-httpd-with-a-sub-path
# Note: no trailing slash after either /git or port

The document was written there to help users configure their instances correctly. If some users don't follow, then that's another problem.

As I explained above, Gitea never adds extra slash to the full path. So if you think it is a problem, then not only "pagination" is affected, a lot of hard-coded links are also affected, then it is not a simple problem, it is a new proposal to totally refactor the Gitea's link system.

Would you like to edit the title to clarify what you mean? For example: Always add an extra slash when the AppSubURL is used without other sub paths ("pagination" is just a tip of the iceberg)

@wxiaoguang commented on GitHub (Mar 3, 2024): > https://docs.gitea.com/administration/reverse-proxies#apache-httpd-with-a-sub-path > `# Note: no trailing slash after either /git or port` The document was written there to help users configure their instances correctly. If some users don't follow, then that's another problem. As I explained above, Gitea never adds extra slash to the full path. So if you think it is a problem, then not only "pagination" is affected, a lot of hard-coded links are also affected, then it is not a simple problem, it is a new proposal to totally refactor the Gitea's link system. Would you like to edit the title to clarify what you mean? For example: `Always add an extra slash when the AppSubURL is used without other sub paths` ("pagination" is just a tip of the iceberg)

GiteaMirror commented

@OdinVex commented on GitHub (Mar 3, 2024):

This has nothing to do with how users configure their servers. That is not the issue. The issue is not the variable, it's the links generated specifically ONLY on the user dashboard. Yes, Gitea DOES add forward-slashes, you can grep the code for it all over. Pagination ONLY on the USER DASHBOARD is affected. ONLY ON THE USER DASHBOARD. On other pages pagination works correctly (AND HAS FORWARD-SLASHES) such as the repo page.

Repo Page:

<a class="item" href="/SUBPATH/issues">Issues</a>
<a class="item" href="/SUBPATH/pulls">Pull Requests</a>
<a class="item" href="/SUBPATH/milestones">Milestones</a>
<a class="item" href="/SUBPATH/explore/repos">Explore</a>

USER BLEEPING DASHBOARD:

<div class="center page buttons">
  <div class="ui borderless pagination menu">
    <a class="disabled item navigation">
      <span class="navigation_label">First</span>
    </a>
    <a class="disabled item navigation">
      <span class="navigation_label">Previous</span>
    </a>
    <a class="active item gt-content-center">1</a>
    <a class="item gt-content-center" href="/SUBPATH?page=2&amp;date=">2</a>
    <a class=" item navigation" href="/SUBPATH?page=2&amp;date=">
      <span class="navigation_label">Next</span>
    </a>
    <a class=" item navigation" href="/SUBPATH?page=2&amp;date=">
      <span class="navigation_label">Last</span>
    </a>
  </div>
</div>

@OdinVex commented on GitHub (Mar 3, 2024): This has nothing to do with how users configure their servers. That is not the issue. The issue is not the variable, it's the links generated specifically ONLY on the user dashboard. Yes, Gitea DOES add forward-slashes, you can grep the code for it all over. Pagination ONLY on the USER DASHBOARD is affected. **ONLY ON THE USER DASHBOARD.** On other pages pagination works correctly (AND HAS FORWARD-SLASHES) such as the repo page. Repo Page: ``` <a class="item" href="/SUBPATH/issues">Issues</a> <a class="item" href="/SUBPATH/pulls">Pull Requests</a> <a class="item" href="/SUBPATH/milestones">Milestones</a> <a class="item" href="/SUBPATH/explore/repos">Explore</a> ``` USER BLEEPING DASHBOARD: ``` <div class="center page buttons"> <div class="ui borderless pagination menu"> <a class="disabled item navigation"> <span class="navigation_label">First</span> </a> <a class="disabled item navigation"> <span class="navigation_label">Previous</span> </a> <a class="active item gt-content-center">1</a> <a class="item gt-content-center" href="/SUBPATH?page=2&date=">2</a> <a class=" item navigation" href="/SUBPATH?page=2&date="> <span class="navigation_label">Next</span> </a> <a class=" item navigation" href="/SUBPATH?page=2&date="> <span class="navigation_label">Last</span> </a> </div> </div> ```

GiteaMirror commented

@OdinVex commented on GitHub (Mar 3, 2024):

First and Previous will also be affected, they using {{.Link}} instead of {{.Link}}/ as seen on other pages such as ./user/settings/security/webauthn.tmpl (data-url="{{$.Link}}/webauthn/delete" data-id="{{.ID}}">). :O A FORWARD-SLASH. If you're re-using the pagination template then it's getting incorrect link information (variable assignment/determination/whatever) from the user dashboard I'd guess.

@OdinVex commented on GitHub (Mar 3, 2024): First and Previous will also be affected, they using {{$.Link}} instead of {{$.Link}}/ as seen on other pages such as `./user/settings/security/webauthn.tmpl` (`data-url="{{$.Link}}/webauthn/delete" data-id="{{.ID}}">`). :O A FORWARD-SLASH. If you're re-using the pagination template then it's getting incorrect link information (variable assignment/determination/whatever) from the user dashboard I'd guess.

GiteaMirror commented

@wxiaoguang commented on GitHub (Mar 3, 2024):

Yes, Gitea DOES add forward-slashes, you can grep the code for it all over.

I guess I have explained many times ... Gitea never adds extra slash to a full path:

* so you only see: `https://host/SUBPATH` , `https://host/SUBPATH/user`, `https://host/SUBPATH/user/repo`
* you won't see: `https://host/SUBPATH/` , `https://host/SUBPATH/user/`, `https://host/SUBPATH/user/repo/`

(Update: OK, https://host/SUBPATH/ is quite special in many cases, sometime it has the trailing slash, sometimes it doesn't)

There could be more cases that the SUBPATH misses the correct trailing slash .... for example, I just fixed some: #29535

Anyway, if you believe only the "dashboard" page needs to be fixed, it's fine to me. It could be fixed by this:

-> Add an trailing slash to dashboard links #29555

You could apply this patch to your instance to see whether it satisfy your requirement, I will add some tests later.

@wxiaoguang commented on GitHub (Mar 3, 2024): > Yes, Gitea DOES add forward-slashes, you can grep the code for it all over. I guess I have explained many times ... `Gitea never adds extra slash to a full path`: * so you only see: `https://host/SUBPATH` , `https://host/SUBPATH/user`, `https://host/SUBPATH/user/repo` * you won't see: `https://host/SUBPATH/` , `https://host/SUBPATH/user/`, `https://host/SUBPATH/user/repo/` (Update: OK, `https://host/SUBPATH/` is quite special in many cases, sometime it has the trailing slash, sometimes it doesn't) There could be more cases that the SUBPATH misses the correct trailing slash .... for example, I just fixed some: #29535 Anyway, if you believe only the "dashboard" page needs to be fixed, it's fine to me. It could be fixed by this: -> Add an trailing slash to dashboard links #29555 You could apply this patch to your instance to see whether it satisfy your requirement, I will add some tests later.

GiteaMirror commented

@OdinVex commented on GitHub (Mar 3, 2024):

I guess I have explained many times ... Gitea never adds extra slash to a full path:

Ah, so the issue of understanding was a language one. It does add forward-slashes for any subdirectory but not for root directory (aka subpath), is what you meant. It should, otherwise clients are sending a parameter request to a file (or a cgi script even such as PHP). Separation of routing by paths (not virtual file pathing) is not only best-practices it can be a requirement in some environments due to software collisions. There is no reason to not use a forward-slash except cosmetic when accounting for all that.

There could be more cases that the SUBPATH misses the correct trailing slash .... for example, I just fixed some: #29535

I took a quick look in most places, only the user dashboard had the issue without going too deep into things, I also had to disable Notifications because of an Apache bug with reverse-proxies that prevents multiple threads (ends up blocking new threads because of concurrently held threads) so we wouldn't have noticed the Notifications. We also don't use comments on the instance but rather in Matrix, so we wouldn't have noticed it there either, good finding.

You could apply this patch to your instance to see whether it satisfy your requirement, I will add some tests later.

I will find the time to test it, thank you.

(Update: OK, https://host/SUBPATH/ is quite special in many cases, sometime it has the trailing slash, sometimes it doesn't)

Yes, the inconsistency could accidentally be parsed and routed as a file on some servers (if they are not canonizing /SUBPATH as /SUBPATH/). That was why I was trying to point out the whole "it could be a file" thing, because it genuinely could. If it always has forward-slashes then we know for certain we want the instance, instead of any file or accidental misrouting because the forward-slash would be.

@OdinVex commented on GitHub (Mar 3, 2024): > I guess I have explained many times ... `Gitea never adds extra slash to a full path`: Ah, so the issue of understanding was a language one. It does add forward-slashes for any subdirectory but not for root directory (aka subpath), is what you meant. It should, otherwise clients are sending a parameter request to a file (or a cgi script even such as PHP). Separation of routing by paths (not virtual file pathing) is not only best-practices it can be a requirement in some environments due to software collisions. There is no reason to not use a forward-slash except cosmetic when accounting for all that. > There could be more cases that the SUBPATH misses the correct trailing slash .... for example, I just fixed some: #29535 I took a quick look in most places, only the user dashboard had the issue without going too deep into things, I also had to disable Notifications because of an Apache bug with reverse-proxies that prevents multiple threads (ends up blocking new threads because of concurrently held threads) so we wouldn't have noticed the Notifications. We also don't use comments on the instance but rather in Matrix, so we wouldn't have noticed it there either, good finding. > You could apply this patch to your instance to see whether it satisfy your requirement, I will add some tests later. I will find the time to test it, thank you. > (Update: OK, https://host/SUBPATH/ is quite special in many cases, sometime it has the trailing slash, sometimes it doesn't) Yes, the inconsistency could accidentally be parsed and routed as a file on some servers (if they are not canonizing /SUBPATH as /SUBPATH/). That was why I was trying to point out the whole "it could be a file" thing, because it genuinely could. If it always has forward-slashes then we know for certain we want the instance, instead of any file or accidental misrouting because the forward-slash would be.

GiteaMirror commented

@wxiaoguang commented on GitHub (Mar 3, 2024):

Oh one more thing, since we have a long discussion for this simple problem, I think maybe we could try to improve the communication in the future.

What I understand:

Gitea requires to make "/SUBPATH" and "/SUBPATH/" work equivalently.
"SUBPATH" is only a proxy path.
So the trailing slash shouldn't be a hard requirement, so using "/SUBPATH" should be right in Gitea's deployment.

What you proposed if I understand correctly now:

Some users need to deploy Gitea in a subpath where the subpath could point to an existing file, they don't follow the document.
There are some mentioned RFC standards (could you share the link? I'd like to take a deep look).

Fortunately, eventually, we could understand each other better. 🙏 In the future if there would be some real cases for a proposal/bug report, I think I could catch you up more quickly. Thank you again for the constructive discussion.

@wxiaoguang commented on GitHub (Mar 3, 2024): Oh one more thing, since we have a long discussion for this simple problem, I think maybe we could try to improve the communication in the future. What I understand: * Gitea requires to make "/SUBPATH" and "/SUBPATH/" work equivalently. * "SUBPATH" is only a proxy path. * So the trailing slash shouldn't be a hard requirement, so using "/SUBPATH" should be right in Gitea's deployment. What you proposed if I understand correctly now: * Some users need to deploy Gitea in a subpath where the subpath could point to an existing file, they don't follow the document. * There are some mentioned RFC standards (could you share the link? I'd like to take a deep look). Fortunately, eventually, we could understand each other better. 🙏 In the future if there would be some real cases for a proposal/bug report, I think I could catch you up more quickly. Thank you again for the constructive discussion.

GiteaMirror commented

@wxiaoguang commented on GitHub (Mar 3, 2024):

I also had to disable Notifications because of an Apache bug with reverse-proxies that prevents multiple threads (ends up blocking new threads because of concurrently held threads)

Do you mean this? #19265 I also participated in it .... 🤣

@wxiaoguang commented on GitHub (Mar 3, 2024): > I also had to disable Notifications because of an Apache bug with reverse-proxies that prevents multiple threads (ends up blocking new threads because of concurrently held threads) Do you mean this? #19265 I also participated in it .... 🤣

GiteaMirror commented