-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
Implement basic file uploads to wiki #6024
Conversation
Closes #574 Signed-off-by: Gabriel Silva Simões <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #6024 +/- ##
==========================================
- Coverage 38.73% 38.66% -0.07%
==========================================
Files 332 332
Lines 48992 49079 +87
==========================================
+ Hits 18976 18977 +1
- Misses 27269 27354 +85
- Partials 2747 2748 +1
Continue to review full report at Codecov.
|
// Copy uploaded files into repository. | ||
for _, upload := range uploads { | ||
tmpPath := upload.LocalPath() | ||
targetPath := path.Join(localPath, upload.Name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will allow you to write to arbitrary places in the filesystem. Admittedly you can't overwrite anything with your current code but you can certainly inject into hooks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps this could be solved by using the hash of the file path instead of the actual path, in a way similar to what is currently done with avatars (although for avatars it's a hash of the file contents, which I don't think is the best way here).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@guillep2k I think if we look for forbidden pathern this shouldnt be a problem. If we hash files on the wiki wont have a human redable name anymore
allow normal chars a-Z numbers and '-' '_' '.' and add a check for /../
patterns
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@6543 but then the users will need to think about how they need to name their files. We should allow them to use any name, as long as it renders a serviceable path from Macaron (i.e. no slashes, no empty string).
} | ||
|
||
if err := ctx.Repo.Repository.UploadWikiFiles(ctx.User, strings.TrimSpace(form.Message), form.Files); err != nil { | ||
ctx.ServerError("DeleteWikiPage", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why DeleteWikiPage
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your current code allows for arbitrary insertion of data, and you can't update or delete uploaded data.
@zeripath updating and deletion is out of scope for this PR |
The thing is just allowing uploads on the wiki is easy - as this PR shows - it's coming up with the rest of the UI that makes this difficult. I don't think we should allow uploads without a way to manage these or at least a proposed UI. Saying you can simply git checkout and push from there there isn't really going to answer the problem - you could do that before. |
After uploaded files, it should be listed some place to easily reference them? |
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs during the next 2 months. Thank you for your contributions. |
@@ -22,7 +22,7 @@ import ( | |||
) | |||
|
|||
var ( | |||
reservedWikiNames = []string{"_pages", "_new", "_edit", "raw"} | |||
reservedWikiNames = []string{"_pages", "_new", "_edit", "_upload", "raw"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There should be a migration check or something if we add a new reserved name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we should reverse all word begin with _
. :)
Please resolve the conflicts. |
@gabrielsimoes can you resolve the confilct? |
It seems that this PR has been stale for long time, so ...... activate it or close it? |
It has been stale for a long time and I can't think of a way to handling it other than closing it. Feel free to reopen if there's any new progress and I could also help. |
Closes #574
Signed-off-by: Gabriel Silva Simões [email protected]
This allows uploading files to the root of the wiki repo. It doesn't add any deleting feature, but I suppose if a user needs that feature, he/she should be cloning the wiki repo and managing the files there (which is the only way to do that on Github),