-
-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
Attachments: Add extension support, allow all types for releases #12465
Attachments: Add extension support, allow all types for releases #12465
Conversation
cf84720
to
d251979
Compare
88fd59e
to
81803d6
Compare
Will do a few tweaks soon like using |
739c1c3
to
8c2f481
Compare
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
I think we have to mark this as breaking because server managers will have to do something to prevent uploads |
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 although this is breaking this is probably still the correct thing to do.
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. |
I agree with @zeripath, For a security consideration, it's a break change. |
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. |
What exactly is the issue with allowing all files? Security concerns? A user can already upload malicious files by just renaming their extension to 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
How would you match them using the new filter options? |
It would not be perfect of course, nothing ever will.
Same as GitHub?
I suppose the main point would be that no |
Seems resonable to change the default to the issue attachment filter to the extensions listed here. |
Default value |
@zeripath: maybe you want to re-review. pr has substantially changed since your review. |
Linter failure |
Lint fixed. |
129a38b
to
668725e
Compare
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. |
ea2317e
to
b9f96dc
Compare
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. |
@silverwind ping |
Will have another look shortly. I opened #12963 for issue with editing so to not distract this PR further. |
b9f96dc
to
74ed89d
Compare
- 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>
c41a152
to
6ba8131
Compare
Ready for review again. |
@lafriks Need your review again. |
accept
attribute of<input type="file">
image/*
Fixes: #10172
Fixes: #7266
Fixes: #12460
Ref: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/file#Unique_file_type_specifiers