Attachments: Add extension support, allow all types for releases by silverwind · Pull Request #12465 · go-gitea/gitea

silverwind · 2020-08-10T18:15:09Z

Add support for file extensions, matching the accept attribute of <input type="file">
Add support for type wildcard mime types, e.g. image/*
Create repository.release.ALLOWED_TYPES setting (default unrestricted)
Change default for attachment.ALLOWED_TYPES to a list of extensions
Split out POST /attachments into two endpoints for issue/pr and releases to prevent circumvention of allowed types check

Fixes: #10172
Fixes: #7266
Fixes: #12460
Ref: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/file#Unique_file_type_specifiers

silverwind · 2020-08-11T11:17:03Z

Will do a few tweaks soon like using path over filepath because browsers generally only send Unix-style paths. I guess this may not fully resolve #8229 but at least users will not hit those mime-type problems any more in the default configuration.

codecov-commenter · 2020-08-11T16:25:32Z

Codecov Report

Merging #12465 into master will increase coverage by 0.03%.
The diff coverage is 72.97%.

@@            Coverage Diff             @@
##           master   #12465      +/-   ##
==========================================
+ Coverage   43.80%   43.83%   +0.03%     
==========================================
  Files         630      631       +1     
  Lines       69821    69892      +71     
==========================================
+ Hits        30585    30640      +55     
- Misses      34300    34305       +5     
- Partials     4936     4947      +11

Impacted Files	Coverage Δ
modules/setting/repository.go	`57.69% <ø> (ø)`
modules/setting/setting.go	`46.44% <ø> (ø)`
routers/api/v1/repo/release_attachment.go	`0.00% <0.00%> (ø)`
routers/repo/editor.go	`25.04% <0.00%> (+0.04%)`	⬆️
routers/repo/attachment.go	`49.43% <50.00%> (-0.57%)`	⬇️
modules/upload/filetype.go	`82.14% <95.83%> (+17.85%)`	⬆️
routers/repo/release.go	`26.52% <100.00%> (+0.32%)`	⬆️
modules/indexer/stats/queue.go	`52.94% <0.00%> (-11.77%)`	⬇️
modules/util/sanitize.go	`37.50% <0.00%> (-3.68%)`	⬇️
modules/lfs/content_store.go	`48.33% <0.00%> (-1.67%)`	⬇️
... and 51 more

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 faa676c...503bebb. Read the comment docs.

zeripath · 2020-08-11T17:08:57Z

I think we have to mark this as breaking because server managers will have to do something to prevent uploads

zeripath

I think although this is breaking this is probably still the correct thing to do.

silverwind · 2020-08-11T17:40:38Z

The new default can't really "break" any existing uploads but it's certainly worth mentioning it in the release notes that we now allow all files to be uploaded by default.

lunny · 2020-08-12T02:53:47Z

I agree with @zeripath, For a security consideration, it's a break change.

CirnoT · 2020-08-13T12:06:05Z

While this is desired for content uploaded as releases, it is definitely not expected behavior when uploading content embedded in a comment.

We should distinguish them and keep comment one to sane text files + images while changing release one to empty by default. Improvements from this PR should of course be kept for both.

silverwind · 2020-08-13T14:59:03Z

it is definitely not expected behavior when uploading content embedded in a comment

What exactly is the issue with allowing all files? Security concerns? A user can already upload malicious files by just renaming their extension to .png and execution of that could be achieved through other means if such vulnerabilities exist.

These mime/extension filters does not really help much if any to prevent such attacks and golang's magic byte-based (I assume its based on that) detection can be fooled too. A much better alternative would be to mount the directory for uploads using noexec mount flag and restricting the gitea process through capabilities as much as possible.

sane text files + images

How would you match them using the new filter options? text/* may not be supported in all browsers as the spec only specifies audio,video,image wildcard (but we do support them all on the backend).

CirnoT · 2020-08-14T07:28:23Z

It would not be perfect of course, nothing ever will.

How would you match them using the new filter options?

Same as GitHub? .gif,.jpeg,.jpg,.png,.docx,.gz,.log,.pdf,.pptx,.txt,.xlsx,.zip

What exactly is the issue with allowing all files? Security concerns?

I suppose the main point would be that no .exe files and such are directly downloadable when uploaded by any user, thus helping site not being marked as hosting malicious content or using it directly to serve payloads. Of course malicious user can still create repository but such cases are easier to track and remove than random attachment on some comment.

silverwind · 2020-08-14T08:13:00Z

Seems resonable to change the default to the issue attachment filter to the extensions listed here.

silverwind · 2020-08-15T16:32:27Z

Default value .docx,.gif,.gz,.jpeg,.jpg,.log,.pdf,.png,.pptx,.txt,.xlsx,.zip added for issue/pr attachments and server-sent error string added to i18n.

silverwind · 2020-08-29T20:10:28Z

@zeripath: maybe you want to re-review. pr has substantially changed since your review.

CirnoT · 2020-08-30T15:25:33Z

Linter failure

silverwind · 2020-08-30T15:45:50Z

Lint fixed.

silverwind · 2020-08-31T18:23:57Z

Encryption removed, now using separate endpoints for issue and release attachment POSTs.

Known issue: Can not remove files from a release when editing it. Issue seems already present on master.

silverwind · 2020-08-31T18:58:28Z

Rebased, squashed and update OP. I'm not sure yet how hard the fix for the release edits will be, it'll likely requre quite a bit of fiddling with the quirky dropzone code.

lafriks · 2020-09-27T21:11:35Z

@silverwind ping

silverwind · 2020-09-28T16:27:53Z

Will have another look shortly. I opened #12963 for issue with editing so to not distract this PR further.

- Add support for file extensions, matching the `accept` attribute of `<input type="file">` - Add support for type wildcard mime types, e.g. `image/*` - Create repository.release.ALLOWED_TYPES setting (default unrestricted) - Change default for attachment.ALLOWED_TYPES to a list of extensions - Split out POST /attachments into two endpoints for issue/pr and releases to prevent circumvention of allowed types check Fixes: go-gitea#10172 Fixes: go-gitea#7266 Fixes: go-gitea#12460 Ref: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/file#Unique_file_type_specifiers

* Add a storage layer for attachments * Fix some bug * fix test * Fix copyright head and lint * Fix bug * Add setting for minio and flags for migrate-storage * Add documents * fix lint * Add test for minio store type on attachments * fix test * fix test * Apply suggestions from code review Co-authored-by: guillep2k <18600385+guillep2k@users.noreply.github.com> * Add warning when storage migrated successfully * Fix drone * fix test * rebase * Fix test * display the error on console * Move minio test to amd64 since minio docker don't support arm64 * refactor the codes * add trace * Fix test * remove log on xorm * Fi download bug * Add a storage layer for attachments * Add setting for minio and flags for migrate-storage * fix lint * Add test for minio store type on attachments * Apply suggestions from code review Co-authored-by: guillep2k <18600385+guillep2k@users.noreply.github.com> * Fix drone * fix test * Fix test * display the error on console * Move minio test to amd64 since minio docker don't support arm64 * refactor the codes * add trace * Fix test * Add URL function to serve attachments directly from S3/Minio * Add ability to enable/disable redirection in attachment configuration * Fix typo * Add a storage layer for attachments * Add setting for minio and flags for migrate-storage * fix lint * Add test for minio store type on attachments * Apply suggestions from code review Co-authored-by: guillep2k <18600385+guillep2k@users.noreply.github.com> * Fix drone * fix test * Fix test * display the error on console * Move minio test to amd64 since minio docker don't support arm64 * don't change unrelated files * Fix lint * Fix build * update go.mod and go.sum * Use github.com/minio/minio-go/v6 * Remove unused function * Upgrade minio to v7 and some other improvements * fix lint * Fix go mod Co-authored-by: guillep2k <18600385+guillep2k@users.noreply.github.com> Co-authored-by: Tyler <tystuyfzand@gmail.com>

silverwind · 2020-10-01T19:26:56Z

Ready for review again.

lunny · 2020-10-05T04:16:19Z

@lafriks Need your review again.

silverwind force-pushed the attachment-defaults branch 4 times, most recently from cf84720 to d251979 Compare August 10, 2020 18:46

silverwind mentioned this pull request Aug 10, 2020

Fix zip file upload problem #12460

Closed

silverwind force-pushed the attachment-defaults branch 5 times, most recently from 88fd59e to 81803d6 Compare August 10, 2020 19:13

silverwind mentioned this pull request Aug 11, 2020

[WIP] Proposal: mime type shortcuts #10172

Closed

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Aug 11, 2020

silverwind force-pushed the attachment-defaults branch from 739c1c3 to 8c2f481 Compare August 11, 2020 15:49

zeripath added pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! type/feature Completely new functionality. Can only be merged if feature freeze is not active. labels Aug 11, 2020

zeripath approved these changes Aug 11, 2020

View reviewed changes

GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Aug 11, 2020

silverwind commented Aug 15, 2020

View reviewed changes

Comment thread custom/conf/app.example.ini Outdated

silverwind commented Aug 15, 2020

View reviewed changes

Comment thread options/locale/locale_en-US.ini Outdated

silverwind requested a review from zeripath August 29, 2020 20:09

silverwind force-pushed the attachment-defaults branch from 129a38b to 668725e Compare August 30, 2020 17:33

lafriks added this to the 1.13.0 milestone Aug 31, 2020

lafriks reviewed Aug 31, 2020

View reviewed changes

Comment thread modules/upload/upload.go Outdated

silverwind force-pushed the attachment-defaults branch 2 times, most recently from ea2317e to b9f96dc Compare August 31, 2020 18:56

lafriks requested changes Sep 1, 2020

View reviewed changes

Comment thread routers/routes/routes.go Outdated

silverwind force-pushed the attachment-defaults branch from b9f96dc to 74ed89d Compare September 28, 2020 16:51

silverwind added 3 commits October 1, 2020 21:13

rename function

cae5c8d

extract GET routes out of RepoMustNotBeArchived

6ba8131

silverwind force-pushed the attachment-defaults branch from c41a152 to 6ba8131 Compare October 1, 2020 19:25

zeripath approved these changes Oct 1, 2020

View reviewed changes

Merge branch 'master' into attachment-defaults

973421a

lafriks approved these changes Oct 5, 2020

View reviewed changes

GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Oct 5, 2020

techknowlogick merged commit cda4475 into go-gitea:master Oct 5, 2020

silverwind deleted the attachment-defaults branch October 5, 2020 06:37

go-gitea locked and limited conversation to collaborators Nov 24, 2020

Uh oh!

Conversation

silverwind commented Aug 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

silverwind commented Aug 11, 2020

Uh oh!

codecov-commenter commented Aug 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

zeripath commented Aug 11, 2020

Uh oh!

zeripath left a comment

Choose a reason for hiding this comment

Uh oh!

silverwind commented Aug 11, 2020

Uh oh!

lunny commented Aug 12, 2020

Uh oh!

CirnoT commented Aug 13, 2020

Uh oh!

silverwind commented Aug 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CirnoT commented Aug 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

silverwind commented Aug 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

silverwind commented Aug 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

silverwind commented Aug 29, 2020

Uh oh!

CirnoT commented Aug 30, 2020

Uh oh!

silverwind commented Aug 30, 2020

Uh oh!

Uh oh!

silverwind commented Aug 31, 2020

Uh oh!

silverwind commented Aug 31, 2020

Uh oh!

Uh oh!

lafriks commented Sep 27, 2020

Uh oh!

silverwind commented Sep 28, 2020

Uh oh!

silverwind commented Oct 1, 2020

Uh oh!

lunny commented Oct 5, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

silverwind commented Aug 10, 2020 •

edited

Loading

codecov-commenter commented Aug 11, 2020 •

edited

Loading

silverwind commented Aug 13, 2020 •

edited

Loading

CirnoT commented Aug 14, 2020 •

edited

Loading

silverwind commented Aug 14, 2020 •

edited

Loading

silverwind commented Aug 15, 2020 •

edited

Loading