Properly handle the name query param in create issue attachment API #30778
Conversation
GiteaBot
added
the
lgtm/need 2
pull-request-size
bot
added
the
size/S
github-actions
bot
added
modifies/api
services/attachment/attachment.go
Outdated
|
|
||
| // UploadAttachmentWithCustomizableName is like UploadAttachment, but you pass both the original file name and the attachment name | ||
| // (which could be the same). | ||
| func UploadAttachmentWithCustomizableName(ctx context.Context, file io.Reader, allowedTypes string, fileSize int64, originalFileName string, attach *repo_model.Attachment) (*repo_model.Attachment, error) { |
There was a problem hiding this comment.
This function has almost the exact same definition as UploadAttachment(). I would need to tweak a couple areas if I were to change its signature so if that's alright to do let me know!
There was a problem hiding this comment.
I think we should change UploadAttachment and fix all the places but not only this one.
There was a problem hiding this comment.
Changed UploadAttachment() and updated the affected areas 👍
|
An example using the swagger API: # URL
http://localhost:3000/api/v1/repos/korvax/test-repo/issues/1/assets?name=MyNameIsNobody
# Response
{
"id": 5,
"name": "MyNameIsNobody",
"size": 12,
"download_count": 0,
"created_at": "2024-04-30T07:32:31Z",
"uuid": "132694fe-f70c-428e-82d1-66edc88a80b2",
"browser_download_url": "http://localhost:3000/attachments/132694fe-f70c-428e-82d1-66edc88a80b2"
} |
pull-request-size
bot
added
size/M
| ctx.Error(http.StatusBadRequest, "CreateReleaseAttachment", "Could not determine name of attachment.") | ||
| return | ||
| } | ||
|
|
||
| // Create a new attachment and save the file | ||
| attach, err := attachment.UploadAttachment(ctx, content, setting.Repository.Release.AllowedTypes, size, &repo_model.Attachment{ | ||
| Name: filename, | ||
| attach, err := attachment.UploadAttachment(ctx, content, setting.Repository.Release.AllowedTypes, size, attachmentName, &repo_model.Attachment{ |
There was a problem hiding this comment.
The original file name string is hiding within an if block that I can't reach. The original behavior shouldn't have changed, but we may encounter the same problem as the issue we're trying to fix had
There was a problem hiding this comment.
I think we can still also have a variable named filename and assign it like you did for attachmentName
There was a problem hiding this comment.
Done, and I fallback to assigning attachmentName to it in the else block
|
Maybe we should have a filename on the |
GiteaBot
added
lgtm/blocked
Ah, I think I might not have understood well enough/was confused why we looked at file extensions (when we already performed MIME sniffing). Now, it looks to me like we perform these extension checks to reject extensions that may look malicious to users or otherwise undesirable (e.g. file.exe). If this is the case, would it be alright if I close this PR and open a new one that performs a extension check when users edit attachment names (as it appears that we don't do it)? Thank you both for the reviews! |
|
Going to close this as bypassing the file extension check will surprise admins since they expect either the default extensions we provide or their own to be honored. |
Fixes #30766 by passing both the original file name and an attachment name to the lower API calls and performing verification on just the original file name. This behavior is pretty much in-line with how we handle attachment names in
EditIssueAttachment(), where we don't perform any verification against the new name and just go straight to the DB to change it.