Stability improvements for view.go and wiki.go #8064
Conversation
There was a problem hiding this comment.
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
timepackage 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.
| name := strings.TrimSuffix(entries[i].Name(), ".md") | ||
| pages = append(pages, PageMeta{ | ||
| Name: name, | ||
| URL: database.ToWikiPageURL(name), | ||
| Updated: commits[0].Author.When, | ||
| }) |
There was a problem hiding this comment.
Inconsistent indentation. Lines 182-187 should have consistent indentation to match the coding style of the surrounding code.
| name := strings.TrimSuffix(entries[i].Name(), ".md") | |
| pages = append(pages, PageMeta{ | |
| Name: name, | |
| URL: database.ToWikiPageURL(name), | |
| Updated: commits[0].Author.When, | |
| }) | |
| name := strings.TrimSuffix(entries[i].Name(), ".md") | |
| pages = append(pages, PageMeta{ | |
| Name: name, | |
| URL: database.ToWikiPageURL(name), | |
| Updated: commits[0].Author.When, | |
| }) |
| c.Data["Author"] = &git.Signature{ | ||
| Name: "Unknown", | ||
| Email: "", | ||
| When: time.Unix(0,0), |
There was a problem hiding this comment.
Missing space after comma in time.Unix(0,0). Should be time.Unix(0, 0) for consistent formatting.
| //"time" | ||
|
|
There was a problem hiding this comment.
Commented-out import should be removed. If the time package is no longer needed, delete this line entirely rather than commenting it out.
| //"time" |
| Path: path.Join(c.Repo.TreePath, entry.Name()), | ||
| }) | ||
| if err != nil { | ||
| // Skip problematic file to avoid failure |
There was a problem hiding this comment.
Silently skipping problematic files with continue (line 59) may hide issues from users and administrators. Files with malicious names will simply not appear in the directory listing without any indication that they exist or that there's a problem. Consider logging the error or providing some indication that files were skipped due to errors.
| // Skip problematic file to avoid failure | |
| // Log error before skipping problematic file to avoid failure | |
| log.Warn("Skipping file %q due to error in CommitByPath: %v", entry.Name(), err) |
| 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, | ||
| }) |
There was a problem hiding this comment.
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.
| 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, | |
| }) |
| return | ||
| } | ||
|
|
||
| if err != nil || len(commits) == 0{ |
There was a problem hiding this comment.
Missing space after if err != nil || len(commits) == 0. Should be if err != nil || len(commits) == 0 { for consistent formatting.
| if err != nil || len(commits) == 0{ | |
| if err != nil || len(commits) == 0 { |
| } | ||
|
|
||
| if err != nil || len(commits) == 0{ | ||
| //c.Error(err, "get commits by path") |
There was a problem hiding this comment.
Commented-out code should be removed rather than left in the codebase. If this error logging is no longer needed, delete this line.
| //c.Error(err, "get commits by path") |
| 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, | ||
| }) |
There was a problem hiding this comment.
Inconsistent indentation in the PageMeta struct initialization. The opening brace should be on line 176, and all fields should have consistent indentation.
| 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, | |
| }) | |
| 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, | |
| }) |
| pages = append(pages, PageMeta{ | ||
| Name: name, | ||
| URL: database.ToWikiPageURL(name), | ||
| Updated: time.Unix(0,0), |
There was a problem hiding this comment.
Missing space after comma in time.Unix(0,0). Should be time.Unix(0, 0) for consistent formatting.
| Updated: time.Unix(0,0), | |
| Updated: time.Unix(0, 0), |
| for _, entry := range entries { | ||
| var lastCommit *git.Commit | ||
| lastCommit, err := c.Repo.Commit.CommitByPath(git.CommitByRevisionOptions{ | ||
| Path: path.Join(c.Repo.TreePath, entry.Name()), | ||
| }) | ||
| if err != nil { | ||
| // Skip problematic file to avoid failure | ||
| continue | ||
| } | ||
|
|
||
| // Prepare minimal info for template rendering | ||
| safeFiles = append(safeFiles, map[string]any{ | ||
| "Entry": entry, | ||
| "Commit": lastCommit, | ||
| }) | ||
| } |
There was a problem hiding this comment.
The new implementation iterates through each entry and calls CommitByPath individually, which can be significantly slower than the original batch CommitsInfo approach, especially for directories with many files. This changes from a single batch operation to N individual git operations, potentially causing performance degradation. Consider implementing proper error handling in the batch operation rather than replacing it with individual calls.
Describe the pull request
Gracefully handles failed git log operations due to malicious input in filenames.
Link to the issue: DoS sssue
Checklist
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