Skip to content
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

Set MIME type for files requested via raw api #9305

Closed

Conversation

danielappelt
Copy link

Closes #8152.

This change sets the media type for text files requested via API /repos/{owner}/{repo}/raw/{filepath} if the type can be deduced from the file extension.

Alternatively, one could set the Content-Type header already in function GetRawFile and streamline the code a little bit more in function ServeData.

@codecov-io
Copy link

codecov-io commented Dec 9, 2019

Codecov Report

Merging #9305 into master will increase coverage by 0.01%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9305      +/-   ##
==========================================
+ Coverage   41.51%   41.52%   +0.01%     
==========================================
  Files         569      569              
  Lines       74294    74294              
==========================================
+ Hits        30844    30853       +9     
+ Misses      39609    39601       -8     
+ Partials     3841     3840       -1
Impacted Files Coverage Δ
modules/migrations/migrate.go 25.78% <50%> (ø) ⬆️
models/repo_list.go 74.07% <0%> (+0.92%) ⬆️
services/pull/check.go 53.52% <0%> (+2.81%) ⬆️
modules/process/manager.go 78.31% <0%> (+3.61%) ⬆️

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 e55d90d...5d5fa63. Read the comment docs.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 9, 2019
@danielappelt danielappelt changed the title Set MIME type for text files requested via raw api Set MIME type for files requested via raw api Dec 10, 2019
@GiteaBot 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 Dec 11, 2019
@lafriks lafriks added the type/enhancement An improvement of existing functionality label Dec 12, 2019
@lafriks lafriks added this to the 1.11.0 milestone Dec 12, 2019
@GiteaBot 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 Dec 18, 2019
@sapk
Copy link
Member

sapk commented Dec 18, 2019

I don't know exactly the details but I remember that it could be a bad idea to do so. For example, someone could store js file that could be interpreted by the browser since they are under the same domain and with good content-type. I would suggest to only allow a restricted list of content-type.

And I think that also the reason why github serve raw file under an other domain. (https://developer.github.com/changes/2014-04-25-user-content-security/)

Copy link
Member

@techknowlogick techknowlogick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewing this PR closer, I'm also share @sapk's concerns regarding security. I know that GitHub uses a different domain for their raw, and I believe they also don't serve the mime type. I am blocking this PR so it doesn't get accidentally merged while this is discussed.

@techknowlogick techknowlogick modified the milestones: 1.11.0, 1.12.0 Dec 18, 2019
@techknowlogick
Copy link
Member

I've also moved this to the 1.12 milestone so that while the discussions happen we don't block the release (we can move it back to 1.11 milestone if discussions finish before an RC is made).

@danielappelt
Copy link
Author

I think there are two mostly orthogonal concerns at play here.

The reason why I created this PR is because I exactly want my gitea instance to serve web resources like JS or CSS files from a git repository in a way that they are usable in a third-party context, that is, via CORS in another web application. In my small work group consisting of people with different skills this would make life a lot easier than, for example, having to host these resources on a separate server (and update with every push to gitea). For the same reason, restricting this PR to a list of content types that are deemed safe is not an option for me as this would surely exclude JS files.

Now, I totally understand the security concerns especially when talking about public gitea instances!

The simplest solution I can think of is adding a configuration option for gitea that would have to be set in order to activate the logic in this PR. In terms of (pseudo) code this could be as easy as changing the added line in routers/api/v1/repo/file.go to:

ctx.Context.Data["IsRawApi"] = <boolean config: deliver MIME types for raw API>

For this change, I would only need to check how configuration options are handled in gitea.

Serving raw API requests from another domain would also be an option for me. At the moment, however, I would not know what changes are needed to achieve this.

@sapk
Copy link
Member

sapk commented Dec 20, 2019

@danielappelt what about setting a proxy (on another domaine) serving the raw file of the api. This seems to satisfy your use case (and others seems to have the same if I look at past issues). If it is ok with you, choose a webserver (nginx, apache, caddy, ...) and I am sure we can find a configuration that could be added in the docs.

@danielappelt
Copy link
Author

@sapk, thank you for the offer. I would then try to use nginx as a proxy server. As far as I can tell, injecting the right MIME type in this scenario is not straight forward though.

@lafriks
Copy link
Member

lafriks commented Jan 2, 2020

maybe we can check here that domain name for requested url here is different from gitea configured full url doamin and then set mime type as proposed here. This way it would be possible to configure additional domain for raw files

@sapk sapk mentioned this pull request Jan 16, 2020
@sapk sapk added the status/blocked This PR cannot be merged yet, i.e. because it depends on another unmerged PR label Feb 3, 2020
@lafriks lafriks modified the milestones: 1.12.0, 1.13.0 May 16, 2020
@codecov-commenter
Copy link

Codecov Report

Merging #9305 into master will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9305      +/-   ##
==========================================
+ Coverage   41.50%   41.52%   +0.02%     
==========================================
  Files         569      569              
  Lines       74289    74294       +5     
==========================================
+ Hits        30835    30853      +18     
+ Misses      39616    39601      -15     
- Partials     3838     3840       +2     
Impacted Files Coverage Δ
routers/api/v1/repo/file.go 71.20% <100.00%> (+0.07%) ⬆️
routers/repo/download.go 42.34% <100.00%> (+2.15%) ⬆️
routers/repo/view.go 39.47% <0.00%> (+0.87%) ⬆️
modules/process/manager.go 78.31% <0.00%> (+3.61%) ⬆️
models/unit.go 67.56% <0.00%> (+5.40%) ⬆️
modules/indexer/code/indexer.go 55.26% <0.00%> (+10.52%) ⬆️

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 d644934...5d5fa63. Read the comment docs.

@lunny lunny modified the milestones: 1.13.0, 1.14.0 Sep 2, 2020
@lunny lunny modified the milestones: 1.14.0, 1.x.x Jan 27, 2021
@lunny
Copy link
Member

lunny commented Jan 27, 2021

Please resolve the conflicts

danielappelt and others added 3 commits January 27, 2021 14:19
This should leave just the intended change as a conflict.
@zeripath
Copy link
Contributor

I think we can close this as there is now a configurable mechanism that users can use at their own risk. Please reopen if I am incorrect about this.

@zeripath zeripath closed this May 14, 2021
@zeripath
Copy link
Contributor

Thanks for your work @danielappelt

@lunny lunny removed this from the 1.x.x milestone Jun 1, 2021
@go-gitea go-gitea locked and limited conversation to collaborators Oct 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. status/blocked This PR cannot be merged yet, i.e. because it depends on another unmerged PR type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[API] Provide correct MIME type when getting a raw text file