Allow mime types to match based off of prefix

[mqudsi]

The old behavior prevented simple file types like "text/plain" from
being uploaded since browsers upload them with the charset as well (e.g.
text/plain charset=utf-8) without specifying all possible charsets.

Additionally, this allows for blanket includes like "text/" or "image/"
by class type.

There should be minimal risk introduced here as mime types are generally
hierarchical, but an alternative approach would be the equivalent of

if allowed.endsWith("*") && strings.HasPrefix(fileType,
    allowed.substr(0, allowed.length - 1) { ....

[mqudsi]

The preference here would be to evaluate the rules with pattern matching. Is there a globbing/pattern matching API gitea already uses elsewhere we could simply call to evaluate the match?

[thehowl]

@thehowI is a copycat trying to impersonate me; don't mind them

[lunny]

How about

if t == "*/*" || t == fileType || strings.HasPrefix(t, fileType+" charset=")

[mqudsi]

@lunny that would work, if it's only to address the text charset issue. It wouldn't support adding a class like image/*, though.

[mqudsi]

The issue with the @thehowl impersonation nonsense derailed the question I was asking:

Is there a globbing/pattern matching API gitea already uses elsewhere we could simply call to evaluate the match?

I imagine you do use wildcards somewhere else in the code and that can just be plugged in here?

[codecov-io]

Codecov Report

Merging #3617 into master will increase coverage by 0.01%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #3617      +/-   ##
==========================================
+ Coverage      23%   23.02%   +0.01%     
==========================================
  Files         126      126              
  Lines       24894    24894              
==========================================
+ Hits         5728     5731       +3     
+ Misses      18289    18287       -2     
+ Partials      877      876       -1

Impacted Files	Coverage Δ
routers/repo/attachment.go	0% <0%> (ø)	⬆️
modules/process/manager.go	73.91% <0%> (+4.34%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9add96a...e95ba54. Read the comment docs.

[stale]

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.

[mqudsi]

It is still pending feedback from maintainers.

[lafriks]

I would propose use wildcard matching (golang has function for this)

[stale]

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.

[6543]

@mqudsi can you resolve confilct?

[mqudsi]

I've redone the patch and changed the syntax from any prefix to a class whitelist (e.g. instead of allowing image it now allows image/*) to prevent any mime abuse.

[guillep2k]

Related #10172

[guillep2k]

Be careful, as image/* will match image/svg+xml which can pose a security problem.

[mqudsi]

@lunny thoughts?

[lafriks]

@guillep2k if svg is not displayed in browser it should not be threat

[lunny]

@mqudsi The codes looks good for me. But if you can extract them as a function and add some tests for it with some update notes on app.ini.sample and cheetsheet, it's better.

[lafriks]

It would be better to use wildcard globs for this

[mqudsi]

I’m not a go developer but I’m not aware of a generic wildcard module in go. The only one I found the last time I looked is a file system glob.

That aside, I disagree entirely. You have to take context into account. What does general globbing get you that a simple string comparison (as performed) doesn’t? On the flip side, you get behavior that makes no sense for your context. What does it mean to have a glob pattern im*s or even */jpeg mean?

[lafriks]

@mqudsi we are already using it in multiple places, for example see PR #7791

If blob case you could use for example such filter: image/{png|jpeg}

[mqudsi]

Ah, in the circles I run in, glob pretty much refers to * or ** only. I didn’t realize go’s implementation was more patterns than that. Still, that’s not much less verbose but considerably less readable than typing those two cases out twice, though if you’re already using it then it’s likely a paradigm advanced users will be familiar with.

[singuliere]

@mqudsi this looks good but needs rebasing.

[mqudsi]

@singuliere I think it's no longer required. The current code deletes ;.* from the detected mime type before matching against the allowed entry:

var mimeTypeSuffixRe = regexp.MustCompile(`;.*$`)
...
	fullMimeType := http.DetectContentType(buf)
	mimeType := strings.TrimSpace(mimeTypeSuffixRe.ReplaceAllString(fullMimeType, ""))
...
		} else if mimeType == allowEntry {
			return nil // mime type is allowed
		} ...

Class type matches (e.g. image/ or text/) are already explicitly supported with foo/* syntax.