go-gitea / gitea

Git with a cup of tea! Painless self-hosted all-in-one software development service, including Git hosting, code review, team collaboration, package registry and CI/CD
https://gitea.com
MIT License
44.1k stars 5.41k forks source link

[Bug Report] Relative links in markdown #18592

Open schorsch13 opened 2 years ago

schorsch13 commented 2 years ago

Introduction

When using relative links in markdown files you have to use either ./file.md or file.md. When trying to use /file.md you will get redirected to the root of the gitea instance.

Example

https://try.gitea.io/schorsch/relative-links-in-markdown

image

Proposal

Change /file.md from directing to the web root to the project root

Credits

The bug was discovered by the codeberg user ivan-paleo. For further information have a look at the issue at codeberg: https://codeberg.org/Codeberg/Community/issues/252

zeripath commented 2 years ago

This was fixed at some point and something has caused a regression!!

zeripath commented 2 years ago

Yes this was fixed in #15088

This is particularly irritating as there was even a test created to prevent this from happening YET AGAIN

zeripath commented 2 years ago

Let's just go through this again.

The base url for an image link rendered in a file GH is:

/owner/repo/blob/branch/path/to/file/

Relative links may traverse out of the repo i.e. if a file that is in / of owner/repo on the default branch master has a link:

../../../../owner2/repo2

then it would like to /owner2/repo2 directly.

Links that start with a / e.g. /../../../owner2/repo2 do not traverse out of the repo. This is not true!

Essentially a leading / is dropped from the links and then the path is made relative to the branch link


I'll do some more test cases in pathological and properly add tests to gitea to prevent this from happening yet again.


Now in issues and presumably elsewhere the opposite happens:

A

/A

../A

../../A

../../../A ../../../../A ../../../../../A ../../../../../../A

/../A

zeripath commented 2 years ago

OK I guess we need the following (this is for a gitea mounted on a suburl /gitea):

name input file fileInDir wiki issue
samedir file /gitea/gogits/gogs/src/branch/master/file /gitea/gogits/gogs/src/branch/master/subdir/file /gitea/gogits/gogs/wiki/file /gitea/gogits/gogs/issues/file
childir subdir/file /gitea/gogits/gogs/src/branch/master/subdir/file /gitea/gogits/gogs/src/branch/master/subdir/subdir/file /gitea/gogits/gogs/wiki/subdir/file /gitea/gogits/gogs/issues/subdir/file
/file /file /gitea/gogits/gogs/src/branch/master/file /gitea/gogits/gogs/src/branch/master/file /gitea/file /gitea/file
../file ../file /gitea/gogits/gogs/src/branch/file /gitea/gogits/gogs/src/branch/master/file /gitea/gogits/gogs/file /gitea/gogits/gogs/file
/../file /../file /gitea/gogits/gogs/src/branch/file /gitea/gogits/gogs/src/branch/file /gitea/file /gitea/file
../../file ../../file /gitea/gogits/gogs/src/file /gitea/gogits/gogs/src/branch/file /gitea/gogits/file /gitea/gogits/file
/../../file /../../file /gitea/gogits/gogs/src/file /gitea/gogits/gogs/src/file /gitea/file /gitea/file
../../../file ../../../file /gitea/gogits/gogs/file /gitea/gogits/gogs/src/file /gitea/file /gitea/file
../../../../file ../../../../file /gitea/gogits/file /gitea/gogits/gogs/file /gitea/file /gitea/file
../../../../../file ../../../../../file /gitea/file /gitea/gogits/file /gitea/file /gitea/file
../../../../../../file ../../../../../../file /gitea/file /gitea/file /gitea/file /gitea/file
../../../../../../../file ../../../../../../../file /gitea/file /gitea/file /gitea/file /gitea/file

One slight problem is that this this is still not github compatible - githubs blob urls are:

https://github.com/zeripath/pathological/blob/master/README.md

Whereas the equivalent in Gitea:

https://try.gitea.io/arandomer/pathological/src/branch/master/README.md

So we're already slightly incompatible.

However, the / behaviour would be the least we should do.

zeripath commented 2 years ago
TestCase to add to modules/markup/markdown/markdown_test.go ```go func TestRender_RelativeLinks(t *testing.T) { setting.AppURL = "https://localhost:3000/gitea/" setting.AppSubURL = "/gitea" testcases := []struct { name string input string expectedFile string expectedFileInDir string expectedWiki string expectedIssue string }{ { name: "samedir", input: "file", expectedFile: "/gitea/gogits/gogs/src/branch/master/file", expectedFileInDir: "/gitea/gogits/gogs/src/branch/master/subdir/file", expectedWiki: "/gitea/gogits/gogs/wiki/file", expectedIssue: "/gitea/gogits/gogs/issues/file", }, { name: "childir", input: "subdir/file", expectedFile: "/gitea/gogits/gogs/src/branch/master/subdir/file", expectedFileInDir: "/gitea/gogits/gogs/src/branch/master/subdir/subdir/file", expectedWiki: "/gitea/gogits/gogs/wiki/subdir/file", expectedIssue: "/gitea/gogits/gogs/issues/subdir/file", }, { name: "/file", input: "/file", expectedFile: "/gitea/gogits/gogs/src/branch/master/file", expectedFileInDir: "/gitea/gogits/gogs/src/branch/master/file", expectedWiki: "/gitea/file", expectedIssue: "/gitea/file", }, { name: "../file", input: "../file", expectedFile: "/gitea/gogits/gogs/src/branch/file", expectedFileInDir: "/gitea/gogits/gogs/src/branch/master/file", expectedWiki: "/gitea/gogits/gogs/file", expectedIssue: "/gitea/gogits/gogs/file", }, { name: "/../file", input: "/../file", expectedFile: "/gitea/gogits/gogs/src/branch/file", expectedFileInDir: "/gitea/gogits/gogs/src/branch/file", expectedWiki: "/gitea/file", expectedIssue: "/gitea/file", }, { name: "../../file", input: "../../file", expectedFile: "/gitea/gogits/gogs/src/file", expectedFileInDir: "/gitea/gogits/gogs/src/branch/file", expectedWiki: "/gitea/gogits/file", expectedIssue: "/gitea/gogits/file", }, { name: "/../../file", input: "/../../file", expectedFile: "/gitea/gogits/gogs/src/file", expectedFileInDir: "/gitea/gogits/gogs/src/file", expectedWiki: "/gitea/file", expectedIssue: "/gitea/file", }, { name: "../../../file", input: "../../../file", expectedFile: "/gitea/gogits/gogs/file", expectedFileInDir: "/gitea/gogits/gogs/src/file", expectedWiki: "/gitea/file", expectedIssue: "/gitea/file", }, { name: "../../../../file", input: "../../../../file", expectedFile: "/gitea/gogits/file", expectedFileInDir: "/gitea/gogits/gogs/file", expectedWiki: "/gitea/file", expectedIssue: "/gitea/file", }, { name: "../../../../../file", input: "../../../../../file", expectedFile: "/gitea/file", expectedFileInDir: "/gitea/gogits/file", expectedWiki: "/gitea/file", expectedIssue: "/gitea/file", }, { name: "../../../../../../file", input: "../../../../../../file", expectedFile: "/gitea/file", expectedFileInDir: "/gitea/file", expectedWiki: "/gitea/file", expectedIssue: "/gitea/file", }, { name: "../../../../../../../file", input: "../../../../../../../file", expectedFile: "/gitea/file", expectedFileInDir: "/gitea/file", expectedWiki: "/gitea/file", expectedIssue: "/gitea/file", }, } for _, testcase := range testcases { t.Run(testcase.name, func(t *testing.T) { t.Run("file", func(t *testing.T) { buffer, err := RenderString(&markup.RenderContext{ URLPrefix: setting.AppSubURL + "/" + Repo + "/src/branch/master/", Filename: "test.md", Metas: map[string]string{ "user": "gogits", "repo": "gogs", "repoPath": "../../../integrations/gitea-repositories-meta/user13/repo11.git/", }, }, "["+testcase.input+"]("+testcase.input+")") assert.NoError(t, err) assert.Equal(t, fmt.Sprintf("

%s

", testcase.expectedFile, testcase.input), strings.TrimSpace(buffer), "Incorrect file path") }) t.Run("fileInDir", func(t *testing.T) { buffer, err := RenderString(&markup.RenderContext{ URLPrefix: setting.AppSubURL + "/" + Repo + "/src/branch/master/subdir/", Filename: "test.md", Metas: map[string]string{ "user": "gogits", "repo": "gogs", "repoPath": "../../../integrations/gitea-repositories-meta/user13/repo11.git/", }, }, "["+testcase.input+"]("+testcase.input+")") assert.NoError(t, err) assert.Equal(t, fmt.Sprintf("

%s

", testcase.expectedFileInDir, testcase.input), strings.TrimSpace(buffer), "Incorrect subdir file path") }) t.Run("wiki", func(t *testing.T) { buffer, err := RenderString(&markup.RenderContext{ URLPrefix: setting.AppSubURL + "/" + Repo, IsWiki: true, Metas: map[string]string{ "user": "gogits", "repo": "gogs", "repoPath": "../../../integrations/gitea-repositories-meta/user13/repo11.git/", }, }, "["+testcase.input+"]("+testcase.input+")") assert.NoError(t, err) assert.Equal(t, fmt.Sprintf("

%s

", testcase.expectedWiki, testcase.input), strings.TrimSpace(buffer), "Incorrect wiki path") }) t.Run("comment", func(t *testing.T) { buffer, err := RenderString(&markup.RenderContext{ URLPrefix: setting.AppSubURL + "/" + Repo + "/issues/", Metas: map[string]string{ "user": "gogits", "repo": "gogs", "repoPath": "../../../integrations/gitea-repositories-meta/user13/repo11.git/", "mode": "comment", }, }, "["+testcase.input+"]("+testcase.input+")") assert.NoError(t, err) assert.Equal(t, fmt.Sprintf("

%s

", testcase.expectedIssue, testcase.input), strings.TrimSpace(buffer), "Incorrect issue path") }) }) } } ```
charles-997 commented 2 years ago

Has there been any progress on this bug? This seems like a reasonably important feature-breaking bug! I was surprised to see that my relative links in existing repositories do not function properly in Gitea.

xbreaker commented 1 year ago

Same problem with using HTML A tag in markdown with relative file name

Steps:

  1. Create markdown file with HTML tag A and link to local file (for example, <a href="readme.md>readme</a>)
  2. Save file, link will lost current path, if you viewing branch link will be /repo/src/branch/file.md instead of /repo/src/branch/main/file.md

Demo: https://try.gitea.io/aybe/markdown-branch/

lunny commented 1 year ago

From https://try.gitea.io/aybe/markdown-branch/ , only relative link from a tag is wrong. Maybe it's accepted?

ell1e commented 11 months ago

I just ran into this, sadly it seems to break almost all links in my documentation so this is a really high impact bug in my opinion. Especially since with the GitHub mirror it works fine.

Here's a demo file with a link that should work but it doesn't:

https://try.gitea.io/blablablablab/BlaTest/src/branch/main/A%20B/outsidelink.md

(The big bold (test link) is what you want to click. Expected behavior is that it gets you to the target file in the repository root, actual behavior is some nonsensical location.)

vilunov commented 7 months ago

In original post it is said that relative links such as [text](file.md) or [text](./file.md) resolve correctly, but absolute ones do not. This is not my experience with Gitea 1.21.4. Example (this was fixed by downgrading to 1.21.3)

I expect that a link such as [Project structure](crates/readme.md) in the project readme should be resolved to example.com/org/repo/src/branch/main/crates/readme.md, instead it is resolved to example.com/org/repo/crates/readme.md, making it pretty much useless, as it is always broken.

vilunov commented 7 months ago

The issue above seems to be a regression between 1.21.3 and 1.21.4, probably even by #28803 Fortunately, there was no migration in 1.21.4, it was easy to downgrade one patch version

LamaBleu commented 7 months ago

Thank you so much @vilunov, you saved my day. Downgrade to 1.21.3 solved this annoying issue.

vilunov commented 7 months ago

@KN4CK3R sorry, pinging for visibility since you made the MR and you seem to be responsible for that area.

KN4CK3R commented 7 months ago

Can't reproduce it on current main. Was it fixed by #28909?

grafik

vilunov commented 7 months ago

Thanks, sorry for panicking. Relative links work again, but the absolute ones are still broken.

A link such as /docs/running.md will be rendered as it is in html and will lead to localhost:3000/docs/running.md

image image

This is on gitea/gitea:1.21-nightly docker image

KN4CK3R commented 7 months ago

Ok, that's not a regression of my PR 🎉 Gitea always rendered them like that. It's debatable what the root is for absolute links. Even Github is inconsistent with the links.

Inside issue: A

/A

../A

../../A

../../../A

../../../../A

../../../../../A

../../../../../../A

/../A

Inside repo: https://github.com/KN4CK3R/Test/blob/4398ccfc630b9b438421ca7e16c60de0538127cd/README.md


A fix would be to strip the / prefix for links too: https://github.com/go-gitea/gitea/blob/3e8414179c3f3e8a12d3a66fdf32c144f941f5c3/modules/markup/markdown/goldmark.go#L148

- link = []byte(giteautil.URLJoin(base, string(link)))
+ link = []byte(giteautil.URLJoin(base, strings.TrimLeft(string(link), "/")))
vilunov commented 7 months ago

GitHub is not very consistent even when it comes to rendering README.md as a repo front page or as an individual file.

Example: link in the first footnote at the end of README.md https://github.com/ratijas/kdesrc-build-sublime -- renders as https://github.com/ratijas/plugins/gen_conf_options.py https://github.com/ratijas/kdesrc-build-sublime/blob/master/README.md -- renders as https://github.com/ratijas/kdesrc-build-sublime/blob/master/plugins/gen_conf_options.py

It really is tricky

ell1e commented 5 months ago

@vilunov the first one seems more like a bug, doesn't it? usually in almost all places on GitHub it behaves like the 2nd example. maybe they just forgot about this corner case of footnotes in the one location, that feels like the most likely reason.

on gitea, sadly it still doesn't seem to work for links starting with / ever, not even in some cases, see "Broken Example 2" here. this makes writing documentation where individual pages can be moved around inside the documentation folder without all their links to other files breaking quite difficult. so i hope this can be fixed eventually.

edit: this also can't be just fixed on the user side by leaving the / away, since when your documentation page is in various sub folders and links to neighboring pages but in other subfolders, you will need to use tons of .. to climb out of where your current doc page is when omitting / (= all outgoing links in the page break when the page is moved around in your repository) while with / it's always relative to the repository base (= a doc page can be moved around without all its outgoing links to others breaking). on github this works perfectly fine, on gitea it doesn't. this use case is also not relevant outside of markdown files inside the repo for documentation, so that e.g. github doesn't also apply this behavior in issue tickets makes sense in my humble opinion.

levicki commented 4 months ago

I am having (a variation of?) this problem on the latest version of Gitea (served at root, not at sub-path) — For example I write:

see [README](README.md)

In the Release description field (which works fine on Github and Gitlab) and the link is replaced with:

https://example.com:3000/my_organization/my_project/README.md

And results in 404 error when clicked. Actual README.md link in the source tree shows as:

https://example.com:3000/my_organization/my_project/src/branch/master/README.md

I don't think either would be correct for Releases though — it should link to specific tagged release version of the file.

The only way I can produce a correct link is if I do it manually like this:

see [README](src/tag/1.0.0/README.md)

Which results in:

https://example.com:3000/my_organization/my_project/src/tag/1.0.0/README.md

It would be nice if Gitea was smart enough to do that sort of expansion for me if I provide a relative path without leading / or ./.

neutronstriker commented 2 months ago

Is this bug resolved now, I am facing the same issue with version 1.21.4. -Thanks.

wxiaoguang commented 2 months ago

It is not really fixed. Because the "absolute path" link (starting with a slash /xxxx) handling is quite tricky and ambiguous.

And, if we change this behavior, it is a "breaking" change and will make all existing "absolute-path" links broken.

neutronstriker commented 2 months ago

It is not really fixed. Because the "absolute path" link (starting with a slash /xxxx) handling is quite tricky and ambiguous.

And, if we change this behavior, it is a "breaking" change and will make all existing "absolute-path" links broken.

Which means we need to use /src/branch/main/ as a prefix right?

wxiaoguang commented 2 months ago

Which means we need to use /src/branch/main/ as a prefix right?

Or use relative path like ../../foo/bar.


Maybe I could take a try in 1.23 to "fix/improve" the path handling, but I can't promise at the moment.

busslina commented 2 months ago

I moved recently to Forgejo, based on Gitea, and works fine. Even MD header URL links works fine, which I think is not the same with Gitea.

neutronstriker commented 2 months ago

I just updated my installation to 1.22.0 and my issue got resolved, my issue was the 1st case referenced in comment https://github.com/go-gitea/gitea/issues/18592#issuecomment-1030591957

wxiaoguang commented 2 months ago

I moved recently to Forgejo, based on Gitea, and works fine. Even MD header URL links works fine, which I think is not the same with Gitea.

There are some different path problems, some have been fixed, while some are still not. Pretty sure they do not have the proper fix either.

Forgejo is still largely based on Gitea's work, but they skipped some Gitea's commits (even some are security related) so I guess it couldn't be more complete than Gitea.

wxiaoguang commented 2 months ago

Made some tests on GitHub, I think we could just copy the behavior (and mark it as breaking)

https://github.com/wxiaoguang/playground/blob/44cc37ef49eb827345dd0d9d510b319f81f63f16/test.md

https://github.com/wxiaoguang/playground/blob/6bd6f1da05afc914abf452628d90c90b71093820/dir/test.md

Actually, the rule is simple:

But not sure whether we should 100% follow it ........

Context Relative Link
In a .md file on the same branch /assets/images/electrocat.png
In a .md file on another branch /../main/assets/images/electrocat.png
In issues, pull requests and comments of the repository ../blob/main/assets/images/electrocat.png?raw=true
In a .md file in another repository /../../../../github/docs/blob/main/assets/images/electrocat.png
In issues, pull requests and comments of another repository ../../../github/docs/blob/main/assets/images/electrocat.png?raw=true
levicki commented 2 months ago

But not sure whether we should 100% follow it ........

I vote for using the same logic as GitHub — that makes it easier to have both private Gitea and public GitHub repo with correct links in both.

Also please make sure that it works not just in issues and comments but in releases as well. Thanks for looking into it and taking time to fix it.

ell1e commented 2 months ago

I moved recently to Forgejo, based on Gitea, and works fine.

In Foregjo 7.0.0 this seems not fixed, and linking e.g. /docs/start.md inside a repository's main folder inside a README.md, won't actually link to https://instance.org/user/repo/src/branch/main/docs/start.md as it probably should. At least as far as I can tell.

wxiaoguang commented 2 months ago

I moved recently to Forgejo, based on Gitea, and works fine.

In Foregjo 7.0.0 this seems not fixed, and linking e.g. /docs/start.md inside a repository's main folder inside a README.md, won't actually link to https://instance.org/user/repo/src/branch/main/docs/start.md as it probably should. At least as far as I can tell.

Although this bug is not fully fixed on either side, actually they missed many bug fixes or improvements, because they hard-forked and skipped many commits.

delvh commented 2 months ago

:face_exhaling: guys, I've marked the comments above as off-topic. They have nothing to do with this issue or Gitea. AFAIK, Forgejo hardforked some months ago and has diverged since. as such, the two projects can no longer be seen as equal. If you encounter issues with Forgejo, please discuss it with them instead of in the Gitea issue tracker. I'll hide my own comment as well to not disturb others.

busslina commented 2 months ago

https://forgejo.org/compare-to-gitea/ https://forgejo.org/compare-to-gitea/#better-security

I can be a bridge between both parts because I have no problem with any of both :) haha

wxiaoguang commented 2 months ago

https://forgejo.org/compare-to-gitea/ https://forgejo.org/compare-to-gitea/#better-security

I can be a bridge between both parts because I have no problem with any of both :) haha

Then you could tell them they have some XSS in some pages, but not in Gitea.