repo: stability improvements for rendering directory listing

[0xless]

Describe the pull request

Gracefully handles failed git log operations due to malicious input in filenames.
Link to the issue: DoS sssue

Checklist

• I agree to follow the Code of Conduct by submitting this pull request.
• I have read and acknowledge the Contributing guide.
• I have added test cases to cover the new code or have provided the test plan.

Test plan

Create a new file in a repository with a filename containing payload "[], do the same for a wiki file.
More details at: DoS issue

Edit: use payload "[] bar for wiki files.

[Copilot]

Pull request overview

This PR addresses a DoS vulnerability by implementing graceful error handling for git log operations that fail due to malicious input in filenames (e.g., filenames containing "[]"). The changes prevent application crashes by using fallback values when git operations fail.

Key changes:

• Modified error handling in wiki operations to use dummy author/timestamp data instead of failing
• Replaced batch commit info fetching with individual file processing that skips problematic files
• Removed unused time package import from view.go

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 12 comments.

File	Description
internal/route/repo/wiki.go	Added fallback handling for failed git log operations in both Wiki() and WikiPages() functions, using dummy author/timestamp values instead of returning errors
internal/route/repo/view.go	Replaced batch CommitsInfo() call with individual CommitByPath() calls that skip files on error, and removed unused time import

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.

[Copilot]

The error handling condition if err != nil || len(commits) == 0 treats both actual errors and empty commit lists the same way. An empty commit list is not necessarily an error condition - it could be a legitimate state for a newly created file. Consider handling these cases separately to avoid masking real errors.

Suggested change

	if err != nil || len(commits) == 0{
	//c.Error(err, "get commits by path")
	// sets dummy commit time to prevent failure
	name := strings.TrimSuffix(entries[i].Name(), ".md")
	pages = append(pages, PageMeta{
	Name: name,
	URL: database.ToWikiPageURL(name),
	Updated: time.Unix(0,0),
	})
	} else {
	name := strings.TrimSuffix(entries[i].Name(), ".md")
	pages = append(pages, PageMeta{
	Name: name,
	URL: database.ToWikiPageURL(name),
	Updated: commits[0].Author.When,
	})
	if err != nil {
	// Handle actual error (optionally log or skip this entry)
	//c.Error(err, "get commits by path")
	continue
	}
	if len(commits) == 0 {
	// sets dummy commit time to prevent failure
	name := strings.TrimSuffix(entries[i].Name(), ".md")
	pages = append(pages, PageMeta{
	Name: name,
	URL: database.ToWikiPageURL(name),
	Updated: time.Unix(0,0),
	})
	} else {
	name := strings.TrimSuffix(entries[i].Name(), ".md")
	pages = append(pages, PageMeta{
	Name: name,
	URL: database.ToWikiPageURL(name),
	Updated: commits[0].Author.When,
	})

[unknwon]

Hey @0xless, thanks for the PR! Could you take a look at Copilot's comments: if valid, address the feedback; if not, explain why?

[unknwon]

Link to the issue: DoS sssue

Also the link here seems to be incorrect? i.e., it is currently pointing to a unrelated PR.

[0xless]

Link to the issue: DoS sssue

Also the link here seems to be incorrect? i.e., it is currently pointing to a unrelated PR.

Fixed the link issue.

I'll check the comments as soon as I have some time to dedicate to the project. I honestly missed the comments at the time.

[unknwon]

See previous comment.

[0xless]

I've solved most issues pointed by copilot, function for rendering wiki page listing might be non-optimized, but I was not able to solve the issue in other ways. Hope this works for you.

[unknwon]

Thanks for the followup! Before diving into reviewing the current approach, I want to take a step back and clarify on the original issue.

In the GHSA, Gogs wouldn't be able to get blob info because the path contains special characters, the approach in this PR is to simply ignore all failed reads. Couldn't we escape those special characters before sending to Git so they are not mistaken as pathspec (or whatever)?

[0xless]

Thanks for the followup! Before diving into reviewing the current approach, I want to take a step back and clarify on the original issue.

In the GHSA, Gogs wouldn't be able to get blob info because the path contains special characters, the approach in this PR is to simply ignore all failed reads. Couldn't we escape those special characters before sending to Git so they are not mistaken as pathspec (or whatever)?

@unknwon I sent you an email a while ago detailing what's necessary to do to achieve so, here is an extract:
"For completeness, I think the best fix would be to implement handling of pathspec magic, in particular the :(literal) signature in gogs' git-module (I honestly couldn't find much authoritative material about pathspec magic, but you can find some info here: https://www.kernel.org/pub/software/scm/git/docs/git.html).
This would allow interpreting paths as literal values, safely handling git log operations for filenames containing every character generally supported by git."

It depends on you if you prefer applying the PR I submitted to have the issue temporarily fixed while work on the git-module is done, or if you want to implement the fix directly in git-module. Right now the fix in the PR will use dummy commit data for files in the wiki and ignore "broken" file entries for files in a repository.

Let me know what you think.

[unknwon]

Ah yeah I remember that email.

Skimming through that link, it seems to have the --literal-pathspecs, which says

Treat pathspecs literally (i.e. no globbing, no pathspec magic). This is equivalent to setting the GIT_LITERAL_PATHSPECS environment variable to 1.

May not be what we think but looks like worth a try (and maybe other ones too).

Let's aiming for a "fix" if possible and if we truly can't, we will fall back to "ignore" approach proposed in this PR.

Do you wanna give it a try or else I can take a look (takes about a few days to months 😂 still need to prioritize a patch release).

[unknwon]

I think I got this problem addressed in #8116?

[0xless]

Hi @unknwon , I was able to pull and test the latest version of gogs (git clone --depth 1 https://github.com/gogs/gogs.git gogs) and the fix seems to be partial.

There is an improvement in wiki file listing rendering as the malicious filename "[] won't cause rendering issues anymore. However I was able to discover that any character following "[] would cause the file to be ignored in directory listing unless characters are present before "[] (example: foo "[] bar works, but "[] bar doesn't).

Additionally I couldn't find any differences in behavior in repo file listings as filename "[] would still trigger a 500 error.

Given these issues, I wouldn't consider the fix to be complete. Are you able to confirm these behaviors?

I wasn't considering it at first as it would restrict the allowed charset for filenames, but what do you think about filtering out any non-alphanumeric character from the filenames? (Maybe still allowing chars - and _).
I noticed you sanitize this way against path traversal attacks so this would be aligned with your existing security measures.
It's not a complete fix for the underlying issue, but it would massively improve the security posture of the project.

Let me know what you think.

Best,
Matteo

[unknwon]

There is an improvement in wiki file listing rendering as the malicious filename "[] won't cause rendering issues anymore. However I was able to discover that any character following "[] would cause the file to be ignored in directory listing unless characters are present before "[] (example: foo "[] bar works, but "[] bar doesn't).

Seems working fine for me? Do you wanna create a demo repo on https://try.gogs.io that reproduces it?

[0xless]

@unknwon I was able to reproduce with title "[] a in this repo here: https://try.gogs.io/0xless/test_dos_wiki/wiki (you will need to check the db as the file is simply not shown in file listing).

I confirm the issue with repository files (not wiki files) is still present as well.

On another topic, what do you think of this solution?

I wasn't considering it at first as it would restrict the allowed charset for filenames, but what do you think about filtering out any non-alphanumeric character from the filenames? (Maybe still allowing chars - and _). I noticed you sanitize this way against path traversal attacks so this would be aligned with your existing security measures. It's not a complete fix for the underlying issue, but it would massively improve the security posture of the project.

[unknwon]

Thanks! Will check, meanwhile could you share the exact steps to recreate the wiki you shared?

Re: quote

People can always git push there is no way we can reject creation of them 100%. The rejection of symlink is via web editing only which prevents server-side traversal and that’s the root cause. Git push with symlink is safe.

[0xless]

People can always git push there is no way we can reject creation of them 100%. The rejection of symlink is via web editing only which prevents server-side traversal and that’s the root cause. Git push with symlink is safe.

Yeah fair.

Anyway, to reproduce the issue in wiki (page not shown in listing) you need to:

• create a repository
• create a wiki page with title "[] foo
• the page won't be shown in the listing
• to check that the page exists, try to create a page with the same name and expect an error

Hope this helps.

[unknwon]

Did you create the page via web UI or push from local? I created via web UI seems not hitting the issue. Could you share the exact commands you used?