Include file extension checks in attachment API

[kemzeb]

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.

[kemzeb]

I also written an integration test for the release attachment API but since the default value of setting.Repository.Release.AllowedTypes is an empty list, I need a way to temporarily change it in a test-environment-safe way.

[kemzeb]

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.

[kemzeb]

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)

[sundaram2021]

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 :)

[lunny]

I think it's acceptable to not allow uploading empty file in reality.

[sundaram2021]

then it above fn will have a simple addon

// Disallow uploading empty files
	if len(buf) == 0 {
		return ErrFileTypeForbidden{Type: "empty file"}
	}

[sundaram2021]

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

[sundaram2021]

/reviewed