-
-
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
Include file extension checks in attachment API #32151
Conversation
I also written an integration test for the release attachment API but since the default value of |
if isBufEmpty { | ||
continue // skip mime type checks if buffer is empty | ||
} |
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.
One problem with this implementation is that if we have the following conditions:
- The attachment is just an empty file
- We have a file extension that is not allowed
text/plain
is an allowed type
We would return an error
here, even though we may expect it not to (since stdlib treats empty files as text/plain
). Instead of changing Verify()
too much I could just create a separate func called VerifyFileExtensionOnly()
.
Then again, as I look at my example above, the logic in Verify()
is interesting seeing that if you have a potential attachment with the following:
- A filename that uses an extension that is not allowed
- A mime type that is allowed
- (or vice-versa)
... this would pass the verification checks. The current implementation does not follow this behavior and just checks if the filename is allowed.
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.
Currently I see a few approaches to this problem:
- Store the MIME type of the attachment so that we can use it to be consistent with how we currently perform verification checks (we don't seem to have this in the attachment table)
- Keep the logic of
upload.Verify()
unchanged. But for subsequent edits to the attachment name verify that it uses an approved file extension - Modify the logic of
upload.Verify()
so that both the file extension and MIME type must be allowed in order for the attachment to be allowed. Any subsequent edits of the attachment name would have us verify that it uses an approved extension (i.e. it would be inline with the new behavior)
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.
As far I can think of the third approach may suits best in this situation
becuase it will handle both MIME type and file extension .
it can be implemented something like this
func Verify(buf []byte, fileName, allowedTypesStr string) error {
allowedTypesStr = strings.ReplaceAll(allowedTypesStr, "|", ",") // compat for old config format
allowedTypes := []string{}
for _, entry := range strings.Split(allowedTypesStr, ",") {
entry = strings.ToLower(strings.TrimSpace(entry))
if entry != "" {
allowedTypes = append(allowedTypes, entry)
}
}
if len(allowedTypes) == 0 {
return nil // everything is allowed
}
fullMimeType := http.DetectContentType(buf)
mimeType, _, err := mime.ParseMediaType(fullMimeType)
if err != nil {
log.Warn("Detected attachment type could not be parsed %s", fullMimeType)
return ErrFileTypeForbidden{Type: fullMimeType}
}
extension := strings.ToLower(path.Ext(fileName))
isBufEmpty := len(buf) <= 1
// Check both file extension and MIME type
extensionAllowed := false
mimeTypeAllowed := false
for _, allowEntry := range allowedTypes {
if allowEntry == "*/*" {
extensionAllowed = true
mimeTypeAllowed = true
break
}
if strings.HasPrefix(allowEntry, ".") && allowEntry == extension {
extensionAllowed = true
}
if isBufEmpty {
continue // skip mime type checks if buffer is empty
}
if mimeType == allowEntry {
mimeTypeAllowed = true
}
if wildcardTypeRe.MatchString(allowEntry) && strings.HasPrefix(mimeType, allowEntry[:len(allowEntry)-1]) {
mimeTypeAllowed = true
}
}
// Require both extension and MIME type to be allowed
if extensionAllowed && (isBufEmpty || mimeTypeAllowed) {
return nil
}
if !isBufEmpty {
log.Info("Attachment with type %s blocked from upload", fullMimeType)
}
return ErrFileTypeForbidden{Type: fullMimeType}
}
.
.
.
please have a look into this implementation , let me hear your thoughts on this
thanks :)
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.
I think it's acceptable to not allow uploading empty file in reality.
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.
then it above fn will have a simple addon
// Disallow uploading empty files
if len(buf) == 0 {
return ErrFileTypeForbidden{Type: "empty file"}
}
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.
I think it's acceptable to not allow uploading empty file in reality.
Yes, I believe it makes sense to disallow empty attachments.
Then again, as I look at my example above, the logic in Verify() is interesting seeing that if you have a potential attachment with the following:
- A filename that uses an extension that is not allowed
- A mime type that is allowed
- (or vice-versa)
... this would pass the verification checks.
My current problem is I am not sure if we should follow the way Verify()
works when users try to edit attachment names. Since we don't seem to store the MIME types of attachments when they are uploaded, it doesn't seem possible currently. The current snapshot just checks the file extension when users try to do an edit.
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.
should be clear about the file type extension and what all the extension it exactly supports if not all .
tests should be written for all the popular file types
.
.
.
thanks
/reviewed |
Can you add some tests? |
There should be integration tests for editing issue and comment attachments in the API. I'll see if I can add a test for editing release attachment since I don't have one |
91244e0
to
d377e25
Compare
last call @go-gitea/technical-oversight-committee |
* giteaofficial/main: Add new index for action to resolve the performance problem (go-gitea#32333) Include file extension checks in attachment API (go-gitea#32151) Updated tokenizer to better matching when search for code snippets (go-gitea#32261) Correctly query the primary button in a form (go-gitea#32438) # Conflicts: # web_src/js/utils/dom.ts
From testing, I found that issue posters and users with repository write access are able to edit attachment names in a way that circumvents the instance-level file extension restrictions using the edit attachment APIs. This snapshot adds checks for these endpoints.