Added ALWAYS_RENDER_RAW_FILES option to the repository section #685

pgaskin · 2017-01-17T00:03:14Z

Used for serving raw files with the right content type, instead of text/plain. Fixes (#683).

This option is disabled by default as it could be a potential security risk because the html in raw mode would render on the same domain as gitea.

appleboy · 2017-01-17T01:09:06Z

modules/setting/setting.go

@@ -772,6 +774,8 @@ please consider changing to GITEA_CUSTOM`)
 	if !filepath.IsAbs(Repository.Upload.TempPath) {
 		Repository.Upload.TempPath = path.Join(workDir, Repository.Upload.TempPath)
 	}
+
+	Repository.AlwaysRenderRawFiles = sec.Key("ALWAYS_RENDER_RAW_FILES").MustBool()


You need also update app.ini configuration.

@appleboy Done.

appleboy · 2017-01-17T01:09:39Z

routers/repo/download.go

@@ -41,6 +44,7 @@ func ServeData(ctx *context.Context, name string, reader io.Reader) error {
 	return err
 }

+


remove empty line.

appleboy · 2017-01-17T01:10:57Z

routers/repo/download.go

-	if base.IsTextFile(buf) || ctx.QueryBool("render") {
+	if setting.Repository.AlwaysRenderRawFiles {
+		ctx.Resp.Header().Set("Content-Disposition", fmt.Sprintf(`inline; filename="%s"`, name))
+	} else if base.IsTextFile(buf) || ctx.QueryBool("render") {
 		ctx.Resp.Header().Set("Content-Type", "text/plain; charset=utf-8")
 	} else if base.IsImageFile(buf) || base.IsPDFFile(buf) {


you should integrate this condition.

@appleboy I think it is more understandable this way.

tboerger · 2017-01-17T09:15:04Z

For reference: #683 (comment)

ghost · 2017-02-01T15:11:52Z

Hi. 👍 for merging this :)

bkcsoft · 2017-02-06T20:55:19Z

I'm with @tboerger on this, this is a really bad idea...

pgaskin · 2017-02-06T21:32:03Z

@bkcsoft Some people like me really need this option. Maybe it could be a build tag, and not offer prebuild builds with this option?

bkcsoft · 2017-02-12T15:11:48Z

@geek1011 well, if one has to compile it themselfs anyway, it could just be a patch-set outside the project, because this is a really bad idea and a huge security risk.

lunny · 2017-02-15T15:40:37Z

So let's move it to 1.x.x

pgaskin · 2017-03-02T12:18:54Z

Doesn't the csrf request token prevent this from being a security risk? If we make it not set the csrf cookie when sending the page, then wouldn't this be perfectly fine?

Used for serving raw files with the right content type. Only enable if you know what you are doing. Signed-off-by: Patrick G <[email protected]>

strk · 2017-03-12T17:20:47Z

Sorry it's not clear to me what the security risk is. I understand is about running arbitrary javascript code embedded in HTML files put in a user repository. Now, cannot this be already done, only not by default ?

pgaskin · 2017-03-12T17:22:20Z

@strk I think it would be submitting a form to delete a user or something like that.

rugk · 2017-03-23T16:01:21Z

I understand is about running arbitrary javascript code embedded in HTML files put in a user repository.

Yes that's the issue…
CSRF tokens won't help since these are (as the name says) to protect from cross-site request forgeries, but here you are on the same site and an attacker can easily request a file with an CSRF token and extract it to use it in a request later.

Basically the attack is an XSS issue AFAIK. By opening a specific URL (with a file saved) untrusted code can be executed in the context of the Gitea installation. This can do anything…

So the fix is to disable it by default and if users need it add an option to enable parsing there. Even in Gogs it has been fixed already, so please don't get even slower than the original version. (Maybe also use the same config option for compatibility…)

Now, cannot this be already done, only not by default ?

Uh, no? I don't understand what you mean…

strk · 2017-03-23T17:20:02Z

On Thu, Mar 23, 2017 at 09:01:22AM -0700, rugk wrote: So the fix is to disable it by default and if users need it add an option to enable parsing there. **Even in Gogs [it has been fixed already](gogs/gogs#3608 (comment), so please don't get even slower than the original version. (Maybe also use the same config option for compatibility…)

Thanks for the pointer, I think it is good to be compatible as much as possible. And yes, I do feel your pain about becoming slower than the original version... (want to help us ?)

> Now, cannot this be already done, only not by default ? Uh, no? I don't understand what you mean…

Passing the `render=1` parameter lets you receive HTML with arbitrary javascript embedded, already.

rugk · 2017-03-23T18:13:07Z

(want to help us ?)

You already have this PR, so what?
Or just use the code/patch from Gogs…

Passing the render=1 parameter lets you receive HTML with arbitrary javascript embedded, already.

Exactly that's the behaviour that needs to be disabled (by default!).

rugk · 2017-03-23T18:17:03Z

Ah I saw that this PR does the opposite of fixing the render issue and should therefore be abolished.
Just as in gogs/gogs@b3c4a39 disable the rendering completely. Raw file means raw file meaning no parsing at all…

Of course you can allow users to use the render=1 parameter if the admin enables a special option. Or just remove it entirely - if admins really want to use it they could configure their reverse-proxy server to add/change the Content-Type header or so.

strk · 2017-03-23T20:05:08Z

On Thu, Mar 23, 2017 at 11:13:07AM -0700, rugk wrote: > (want to help us ?) You already have this PR, so what?

It's not just code that a project needs, but also organizational figures :)

rugk · 2017-03-23T20:25:59Z

I'll keep posting issues and complaining about security-stuff, be sure… 😊

tboerger · 2017-03-23T21:11:47Z

I still don't like this option. This is something that should be handled by a Webserver. If the option is there, we will have users that enable it because they simply don't understand the risk. And than somebody will complain about it.

rugk · 2017-03-23T21:25:24Z

Then remove it entirely…

strk · 2017-03-23T22:42:51Z

I think there are valid use cases where the security implication is not a concern: private hosting with only trusted users. For those cases, it's ok to let the admin allow rendering (even by default) the content. This is @geek1011 usecase, I think.

rugk · 2017-03-24T17:50:16Z

Yes, but as @tboerger says, just use a reverse-proxy and add/modify then headers. There is no need to have this option in Gitea. (Users could not be aware of the risk ad enable it more or less accidentally or for just trying it out etc.)

On the other hand there is of course no need to discuss this. I am fine with an option, but please, please, disable it by default finally.

tboerger · 2017-03-24T18:10:51Z

@lunny? @bkcsoft? I'm against this option.

pgaskin · 2017-03-24T20:29:58Z

Thats a good idea. I might write some caddy middleware to fix the content type.

bkcsoft · 2017-04-18T03:11:36Z

General concensus seems to be that this is a bad idea all together, so I'm closing this 🙂

Feel free to reopen if someone wants to discuss it further

rugk · 2017-04-18T10:08:48Z

So can we now implement/disable the rendering properly?

strk · 2017-04-18T20:00:52Z

To recap: the reason raw rendering is considered unsafe is because an attacker user could publish (via pushes to a repository) malicious javascript code that would steal service credential from other users watching the published code. This means that administrators trusting all users to not be malicious could really as well want to ALWAYS_RENDER_RAW_FILES, while those who cannot trust users should NEVER_RENDER_RAW_FILES instead. Rendering raw files from a _different_ domain would remove the danger altogether. So my proposal for a "proper" way to do things is: 1. Allow setting a ROOT_URL_RAW variable to be used for serving raw files rendering. When set, raw files would be served only from the given domain, when unset, raw files would be disabled. People trusting their users and NOT wanting to register another domain could set ROOT_URL_RAW = %ROOT_URL, the others could leave that unset (the default). What do you think ?

rugk · 2017-04-18T20:18:51Z

Great idea!

bkcsoft · 2017-04-19T07:14:55Z

@strk I'm fine with that proposal 🙂

strk · 2017-04-19T07:19:21Z

So anyone willing to implement ? @geek1011 ?

pgaskin mentioned this pull request Jan 17, 2017

Add config option for HTML render mode gogs/gogs#3608

Closed

appleboy suggested changes Jan 17, 2017

View reviewed changes

lunny added this to the 1.1.0 milestone Jan 17, 2017

lunny added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Jan 17, 2017

tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jan 17, 2017

lunny modified the milestones: 1.x.x, 1.1.0 Feb 15, 2017

lunny mentioned this pull request Feb 15, 2017

Add server config option for always rendering html code #683

Closed

6 tasks

pgaskin and others added 4 commits March 8, 2017 20:37

Added ALWAYS_RENDER_RAW_FILES option to the repository section

2ad4b0f

Used for serving raw files with the right content type. Only enable if you know what you are doing. Signed-off-by: Patrick G <[email protected]>

Add ALWAYS_RENDER_RAW_FILES to app.ini

0adb06c

Removed extra blank lines

0417356

Fixed mimetypes

f2d9e84

bkcsoft closed this Apr 18, 2017

lunny removed this from the 1.x.x milestone Apr 18, 2017

pgaskin mentioned this pull request May 24, 2017

Use caddy as the main server #1721

Closed

davidsvantesson mentioned this pull request Aug 25, 2019

Gitea can't render SVG files. #1095

Closed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added ALWAYS_RENDER_RAW_FILES option to the repository section #685

Added ALWAYS_RENDER_RAW_FILES option to the repository section #685

pgaskin commented Jan 17, 2017

appleboy Jan 17, 2017

pgaskin Jan 17, 2017

appleboy Jan 17, 2017

pgaskin Jan 17, 2017

appleboy Jan 17, 2017

pgaskin Jan 17, 2017

tboerger commented Jan 17, 2017

ghost commented Feb 1, 2017

bkcsoft commented Feb 6, 2017

pgaskin commented Feb 6, 2017

bkcsoft commented Feb 12, 2017

lunny commented Feb 15, 2017

pgaskin commented Mar 2, 2017 •

edited

Loading

strk commented Mar 12, 2017

pgaskin commented Mar 12, 2017

rugk commented Mar 23, 2017

strk commented Mar 23, 2017 via email

rugk commented Mar 23, 2017

rugk commented Mar 23, 2017

strk commented Mar 23, 2017 via email

rugk commented Mar 23, 2017

tboerger commented Mar 23, 2017

rugk commented Mar 23, 2017

strk commented Mar 23, 2017

rugk commented Mar 24, 2017 •

edited

Loading

tboerger commented Mar 24, 2017

pgaskin commented Mar 24, 2017 via email •

edited

Loading

bkcsoft commented Apr 18, 2017

rugk commented Apr 18, 2017

strk commented Apr 18, 2017 via email

rugk commented Apr 18, 2017

bkcsoft commented Apr 19, 2017

strk commented Apr 19, 2017 via email

		@@ -41,6 +44,7 @@ func ServeData(ctx *context.Context, name string, reader io.Reader) error {
		return err
		}

Added ALWAYS_RENDER_RAW_FILES option to the repository section #685

Added ALWAYS_RENDER_RAW_FILES option to the repository section #685

Conversation

pgaskin commented Jan 17, 2017

appleboy Jan 17, 2017

Choose a reason for hiding this comment

pgaskin Jan 17, 2017

Choose a reason for hiding this comment

appleboy Jan 17, 2017

Choose a reason for hiding this comment

pgaskin Jan 17, 2017

Choose a reason for hiding this comment

appleboy Jan 17, 2017

Choose a reason for hiding this comment

pgaskin Jan 17, 2017

Choose a reason for hiding this comment

tboerger commented Jan 17, 2017

ghost commented Feb 1, 2017

bkcsoft commented Feb 6, 2017

pgaskin commented Feb 6, 2017

bkcsoft commented Feb 12, 2017

lunny commented Feb 15, 2017

pgaskin commented Mar 2, 2017 • edited Loading

strk commented Mar 12, 2017

pgaskin commented Mar 12, 2017

rugk commented Mar 23, 2017

strk commented Mar 23, 2017 via email

rugk commented Mar 23, 2017

rugk commented Mar 23, 2017

strk commented Mar 23, 2017 via email

rugk commented Mar 23, 2017

tboerger commented Mar 23, 2017

rugk commented Mar 23, 2017

strk commented Mar 23, 2017

rugk commented Mar 24, 2017 • edited Loading

tboerger commented Mar 24, 2017

pgaskin commented Mar 24, 2017 via email • edited Loading

bkcsoft commented Apr 18, 2017

rugk commented Apr 18, 2017

strk commented Apr 18, 2017 via email

rugk commented Apr 18, 2017

bkcsoft commented Apr 19, 2017

strk commented Apr 19, 2017 via email

pgaskin commented Mar 2, 2017 •

edited

Loading

rugk commented Mar 24, 2017 •

edited

Loading

pgaskin commented Mar 24, 2017 via email •

edited

Loading