Edgewall Software
Modify

Opened 12 months ago

Closed 7 months ago

Last modified 6 months ago

#12964 closed enhancement (fixed)

Make HTTP headers configurable

Reported by: Ryan J Ollos Owned by: Ryan J Ollos
Priority: normal Milestone: 1.2.3
Component: web frontend Version:
Severity: normal Keywords:
Cc:
Release Notes:

Request headers are configurable through the [http-headers] section of trac.ini.

API Changes:

Description (last modified by 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.

Attachments (0)

Change History (15)

comment:1 Changed 10 months ago by Ryan J Ollos

Owner: set to Ryan J Ollos
Status: newassigned

comment:2 Changed 8 months ago by Ryan J Ollos

Description: modified (diff)
Release Notes: modified (diff)

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

comment:3 Changed 8 months ago by Ryan J Ollos

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

comment:4 Changed 8 months ago by 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.

comment:5 in reply to:  4 Changed 8 months ago by 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.

comment:6 in reply to:  4 ; Changed 8 months ago by 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).

Last edited 8 months ago by Ryan J Ollos (previous) (diff)

comment:7 Changed 8 months ago by 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.

comment:8 in reply to:  6 ; Changed 8 months ago by 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.

comment:9 in reply to:  8 ; Changed 8 months ago by 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.

Last edited 8 months ago by Ryan J Ollos (previous) (diff)

comment:10 in reply to:  9 ; Changed 8 months ago by 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]

comment:11 in reply to:  10 Changed 8 months ago by 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.

comment:12 Changed 8 months ago by Ryan J Ollos

Description: modified (diff)

comment:14 Changed 7 months ago by Ryan J Ollos

Resolution: fixed
Status: assignedclosed

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

comment:15 Changed 6 months ago by Ryan J Ollos

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

Last edited 6 months ago by Ryan J Ollos (previous) (diff)

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Ryan J Ollos.
The resolution will be deleted.
to The owner will be changed from Ryan J Ollos to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.