Make HTTP headers configurable

[Ryan J Ollos]

Discused in ​gmessage:trac-dev:gDPzxZEo8v0/VMPI57jNCQAJ.

1. Check whether http header name is valid like [trac] xsendfile_header option.
2. Check whether http header value is valid (the value cannot contain control characters except TAB and SPACE).
3. Ignore some headers, e.g. Content-Type, Content-Length, Location, ETag, Pragma, Cache-Control, Expires.
4. Send configured headers for all send_ methods including send_error.

[Ryan J Ollos]

Do we want to configure any default values for [http-headers]? Is the following ​suggested?:

[http-headers]
Content-Security-Policy = frame-ancestors 'self'; default-src 'none'; img-src 'self'; script-src 'self'; style-src 'self'; base-uri 'self'
Referrer-Policy = same-origin
X-Frame-Options = SAMEORIGIN
X-Content-Type-Options = nosniff
X-XSS-Protection = 1; mode=block

[Ryan J Ollos]

Proposed changes in log:rjollos.git:t12964_http_headers. I can add default values for [http-headers] if that's wanted.

[Jun Omae]

I think the following comment in trac/web/main.py should be removed, or moved to previous line of Request._valid_header_re:

417     def _get_use_xsendfile(self, req):
418         return self.use_xsendfile
419
420     # RFC7230 3.2 Header Fields
421
422     @lazy
423     def _xsendfile_header(self):

I think we should allow to disable configurable headers feature and several default headers.

• Adding [trac] use_configurable_headers option like [trac] use_xsendfile option.
• If the value is empty, the header is not sent.

I'd like to disable the feature by the option because similar headers are added using mod_headers of Apache in production environment.

[Ryan J Ollos]

Replying to Jun Omae:

I think we should allow to disable configurable headers feature and several default headers.

• Adding [trac] use_configurable_headers option like [trac] use_xsendfile option.
• If the value is empty, the header is not sent.

I'm unsure of the exact behavior you are asking for. Could you make a quick patch on top of my branch? I'll take care of adding test coverage.

[Ryan J Ollos]

Replying to Jun Omae:

I think we should allow to disable configurable headers feature and several default headers.

• Adding [trac] use_configurable_headers option like [trac] use_xsendfile option.
• If the value is empty, the header is not sent.

I'd like to disable the feature by the option because similar headers are added using mod_headers of Apache in production environment.

Here are the aspects I'm unsure about:

1. Do you want [http-headers] to have the default values in comment:2?
2. If no configurable headers are wanted, one can either delete the section from trac.ini, or use an empty section [http-headers]. Then, no configurable headers will be sent. Therefore, I'm unsure why [trac] use_configurable_headers is needed.

If we don't add the default values, then no configurable headers will be sent by default. If we do add the default values, then I could see see the utility of use_configurable_headers: it allows us to have a set of default values in trac.ini, but not have those sent until the user explicitly enables configurable headers. However, the same effect could be achieved by having no default values and just documenting the recommended values in TracStandalone (or elsewhere).

[Ryan J Ollos]

I'm fine with any of the proposed solutions (default values and a use_configurable_headers option or no default values and no use_configurable_headers option), but will proceed with committing proposed changes if there aren't any comments on the best solution.

[Jun Omae]

Replying to Ryan J Ollos:

If we don't add the default values, then no configurable headers will be sent by default. If we do add the default values, then I could see see the utility of use_configurable_headers: it allows us to have a set of default values in trac.ini, but not have those sent until the user explicitly enables configurable headers. However, the same effect could be achieved by having no default values and just documenting the recommended values in TracStandalone (or elsewhere).

Okay. I misunderstood we create [http-headers] with secure default values when environment is created and upgraded like [ticket-workflow] section…. If we don't create [http-headers] section when creating and upgrading environment, use_configurable_headers option isn't needed.

BTW, we send X-XSS-Protection: 0 to avoid #12926 but the header is conflicted when [http-headers] has X-XSS-Protection option.

[Ryan J Ollos]

Replying to Jun Omae:

BTW, we send X-XSS-Protection: 0 to avoid #12926 but the header is conflicted when [http-headers] has X-XSS-Protection option.

I had overlooked. I can think of a few ways to handle:

• _outheaders could be an OrderedDict, so that the 'X-XSS-Protection' key is replaced when end_headers is called, rather than appending a conflicting key.
• We could search the tuple and remove conflicting key from configurable headers.
• We could only append 'X-XSS-Protection', 0 only if it's not conflicting. However, that would cause the #12926 issue to be seen for some user-configurable headers.

Do we also have a conflict in case the user specifies X-XSS-Protection through web server, like with mod_headers on Apache? It might be necessary to use the ​setifempty argument.

[Ryan J Ollos]

Replying to Ryan J Ollos:

Replying to Jun Omae:

BTW, we send X-XSS-Protection: 0 to avoid #12926 but the header is conflicted when [http-headers] has X-XSS-Protection option.

I had overlooked. I can think of a few ways to handle:

• _outheaders could be an OrderedDict, so that the 'X-XSS-Protection' key is replaced when end_headers is called, rather than appending a conflicting key.

Changing to a dict won't work because multiple headers might be sent with same name. For example, on login:

Set-Cookie: trac_auth=0339c644721b1307f8cc1de82d6b9382; httponly; Path=/proj-1.2
Set-Cookie: trac_session=b52410efcabf28efdc57bb1f; expires=Fri, 27 Apr 2018 00:37:57 GMT; httponly; Path=/proj-1.2

• We could search the tuple and remove conflicting key from configurable headers.

In Chrome even with duplicate headers the #12926 issue is not seen, and it appears the first header sent takes precedence:

X-XSS-Protection: 1; mode=block
X-XSS-Protection: 0

Would it be sufficient to only add a configurable header if it doesn't collide with an existing header name?: [202c53ad3/rjollos.git]

[Jun Omae]

Replying to Ryan J Ollos:

Would it be sufficient to only add a configurable header if it doesn't collide with an existing header name?: [202c53ad3/rjollos.git]

Yeah. I think that is good enough in this case.

[Ryan J Ollos]

Revised changes:

• rjollos.git:t12964_http_headers.3+1.2-stable
• rjollos.git:t12964_http_headers.3+trunk

[Ryan J Ollos]

Committed to 1.2-stable in r16586, r16587; merged to trunk in r16588, r16589.

[Ryan J Ollos]

Minor addition in r16651, r16654, merged in r16652, r16655 (sorry, forgot to run tests!).