Support for the X-Frame-Options header?

[etnt]

This is a similar question as the issue I recently just opened. Perhaps the questions should be if there is possible to add arbitrary headers to the content served by Yaws?

For appmods, it can be done by the appmod of course, but for other kind of content...?

[vinoski]

Same question as for #362: is there a reason your code or application can't add this response header itself? Appmods can add headers, .yaws pages can add headers, reverse proxy interceptors can add headers, etc. If you explain what kind of application you have, I can help you figure out how to add these headers.

[etnt]

Well, for static content we don't have any control.

Wouldn't it be nice if Yaws made it possible configure extra HTTP headers to be returned? I just wanted to check if I had missed something and that it already was possible today. Not saying that you should to do this ;-) We could probably do the work and provide a patch for it.

Btw, jfyi: the requirement for these particular headers came up after an external security analysis.

[vinoski]

There's always a way: for example, a dispatchmod could intercept all incoming requests, handle those for static content by returning the content along with any desired response headers, and let Yaws handle all other requests as usual. So it's doable, but I would agree it's not convenient.

I don't mind doing the work to allow extra headers to be returned, as I have some ideas about how best to do it, and since you're interested in the feature I assume you can help test it and provide feedback.

[etnt]

Sounds great! And of course we will test it and give feedback.

[vinoski]

I pushed a new branch named extra-response-headers that provides a simple configuration mechanism to attach extra headers to a response. You can add a block like the following to a server configuration:

<extra_response_headers>
    Header1 = Value1
    Header2 = value for header two
</extra_response_headers>

It's a first cut at this problem that's about as simple as can be. I think it will probably require additions, such as a way to specify headers that are returned only for certain status codes or status code ranges. Please give it a try and let me know your thoughts about it.

[leoliu]

Is it possible for these headers to be added only if not already present?

[vinoski]

It depends what you mean, @leoliu .

• If you mean checking to see if a particular header is already present with a particular value, then that's difficult since different headers can have values composed of multiple pieces, making them difficult to compare for equality.
• But if you mean checking to see if a particular header is already present and not including it as an extra header in that case, then sure, that's relatively easy.

Assuming you meant the second item above, also note that someone else might want the opposite effect, such as overwriting any header that's already present with the extra header, but if the header isn't present, then don't include it as an extra header either. These cases are what I alluded to in my previous comment about this first prototype of this feature being as simple as can be; additional cases like these will have be handled.

[leoliu]

Yes, I mean 2.

Note nginx has something similar to not override by default but provides an option to always override.

[etnt]

Just want to let you know that I haven't forgotten this. I've been busy with other stuff but will soon take a look at this.

[vinoski]

Great, thanks @etnt for letting me know. I'd like to make some changes to add capabilities — shall I go ahead and do that now?

[etnt]

Yes, please go ahead!

[etnt]

Just some quick thoughts: I'm talking with our "WebUI guys" and they wan't to add headers based on "something" (?). Since we runs Yaws embedded and do all our configuration via the gc/sc records, perhaps you should allow for having a callback-function in that extra_headers list. This callback-function could take the #arg{} record as input and produce a list of ExtraHeaders. This way, it would be possible to tailor-make the extra-headers.

[vinoski]

Yes, I was planning to add some sort of callback function like you describe.

[vinoski]

Finally got back to this, sorry for the delay.

I've force-pushed changes to the extra-response-headers branch that adds support for configuring a module supplying an extra_response_headers/1 function for supplying extra headers. The extra_response_headers/1 function is expected to return a list [{Header, Value}] of extra headers, where Header and Value should be either of string type or binary type. The argument to the function is an #outh{} record containing the headers that will be returned if the extra_response_headers/1 function returns an empty list. See the testsuite files extra_resp_hdrs.erl and yaws.conf for examples. I have not yet updated the docs in this branch.

I know you suggested passing #arg{} to the extra response headers function, but unfortunately it's not always available in the context where the output headers are being assembled.

The change also includes support for configuring extra_response_headers in yaws:setup_sconf/2, though it's not yet tested and so may not work, especially if you try to configure an extra response headers module. If you try it, please let me know.

There are more changes I'd like to make, but please give this a try when you get a chance and let me know how it goes. I know I've been slow on developing this branch, but I'd like to speed it up and get it to master soon.

[etnt]

Hm...regarding the #arg{} record: it seems like yaws:outh_serialize/0 is called from yaws_server:deliver_accumulated/4 which has the #arg{} record as its first argument. Do you mean that this argument, not always, will contain an #arg{} record?

Anyway, why not send along the Arg content and let the extra module do its best?

[vinoski]

Passing the yaws_server:deliver_accumulated/4 #arg{} as an argument to yaws:outh_serialize and then tracing it in the extra_response_headers test in the testsuite shows that the Arg is undefined:

(<0.211.0>) call yaws:outh_serialize(undefined)

So it's not at all useful.

[etnt]

I've pushed a commit that provide the ARG record (whenever possible) to that extra-response-header callback. You can find it in the branch: etnt/extra-response-headers which should be your branch + my commit. What do ya think?

[vinoski]

Thanks for pushing your branch up. I looked at it and it's a lot of changes, but they all seem manageable. I'll keep looking at it and incorporate it into new commit on the extra-response-headers branch.

[vinoski]

In my next commit on the extra-response-headers branch, in addition to the #arg handling, I think I'll change how headers are passed to the extramod:out function. Previously I decided to pass them in an #outh record, but there's really no helper functions for managing such a record. Instead, since the minimum Erlang version Yaws supports is 17.0 (and with 22.0 being released yesterday, this will soon be 18.0), I'll use a map instead. Any issues with that plan, @etnt ?

[etnt]

Sounds good to me and thanks for you efforts around this!

[vinoski]

I force-pushed a new version of the extra-response-headers branch that I believe now provides a complete implementation of the extra response headers feature. The branch includes updated documentation, so look there for details, but briefly:

• In a server block you can configure headers to be added or erased.
• You can also configure a module to deal with extra headers. The module's function gets a map of response headers, an Arg, and a tuple indicating the response status code and HTTP version. Returning a modified map with added, changed, or removed headers changes the response headers sent back to the client.
• This can all be programmatically specified in an #sconf{} as well.

One concern I have with the implementation as it currently stands is that the map for the extra response header module contains header names and header values as strings, and the header names are case-sensitive. For example, if an extra response header module adds, say, a "vary" header, but the response headers already contain a "Vary" header, the client will get both. Yaws already provides lots of little yaws_api helpers for handling #headers{} records, the erlang_header_name/1 functions for header name conversions, and functions for other forms of headers here and there, so maybe solving this issue just requires adding map support to various existing helpers.

Feedback welcomed.

[etnt]

There seem to be a bug at the end of one of the clauses in yaws:extra_response_headers/4

... lists:foreach(fun accumulate_header/1, maps:to_list(HdrMap2)), get(outh);

The clause should return the Map containing the headers?

[vinoski]

The yaws:extra_response_headers functions are internal. Overall, it works as follows:

1. The yaws:outh_serialize/1 function calls yaws:extra_response_headers/3
2. The yaws:extra_response_headers/3 creates a map of the current headers, and calls yaws:extra_response_headers/4, which loops over the extra response header directives taken from the server configuration
3. The yaws:extra_response_headers/4 function adds headers or erases headers as the directives instruct. When it hits a directive to call a module's extra_response_headers/3 function, it does so and takes the returned map as its new accumulated map (see https://github.com/klacke/yaws/blob/extra-response-headers/src/yaws.erl#L2055)
4. Once all directives are exhausted, the yaws:extra_response_headers/4 clause at https://github.com/klacke/yaws/blob/extra-response-headers/src/yaws.erl#L2001 then converts the map back into an #outh{} record for final serialization

The return value of yaws:extra_response_headers/4 is the #outh{} in the process dictionary, which yaws:outh_serialize/1 then serializes. The only reason it gets stuffed back into the process dictionary is that all the header accumulation functions work that way, and I didn't feel like writing new variants of them that didn't do that.

[etnt]

Hm...I did get a crash in that code, but perhaps I did something wrong; I need to check at the office tomorrow, anyway here is how the crash looked like (seems like maps:is_key/2 get an #outh{} record as its second argument):

=ERROR REPORT==== 22-May-2019::14:50:50 === Yaws process died: {{badmap,{outh,200,false,false,false,decide,305,0, undefined,undefined,undefined,undefined, undefined, ["Date: ","Wed, 22 May 2019 12:50:50 GMT", "\r\n"], undefined, ["Last-Modified: ", "Wed, 22 May 2019 12:48:25 GMT","\r\n"], ["Etag: ","\"318412GE\"","\r\n"], undefined,undefined, ["Content-Length: ","305","\r\n"], ["Content-Type: ","text/html","\r\n"], undefined,undefined,undefined, ["Vary: ","Accept-Encoding","\r\n"], undefined,undefined}}, [{maps,is_key, ["X-Content-Type-Options", {outh,200,false,false,false,decide,305,0, undefined,undefined,undefined,undefined, undefined, ["Date: ","Wed, 22 May 2019 12:50:50 GMT", "\r\n"], undefined, ["Last-Modified: ", "Wed, 22 May 2019 12:48:25 GMT","\r\n"], ["Etag: ","\"318412GE\"","\r\n"], undefined,undefined, ["Content-Length: ","305","\r\n"], ["Content-Type: ","text/html","\r\n"], undefined,undefined,undefined, ["Vary: ","Accept-Encoding","\r\n"], undefined,undefined}], []}, {yaws,'-extra_response_headers/4-fun-1-',2, [{file,"yaws.erl"},{line,2061}]}, {lists,foldl,3,[{file,"lists.erl"},{line,1263}]}, {yaws,extra_response_headers,4, [{file,"yaws.erl"},{line,2059}]}, {yaws,outh_serialize,1,[{file,"yaws.erl"},{line,1956}]}, {yaws_server,deliver_accumulated,4, [{file,"yaws_server.erl"},{line,3844}]}, {yaws_server,deliver_small_file,5, [{file,"yaws_server.erl"},{line,4069}]}, {yaws_server,handle_method_result,7, [{file,"yaws_server.erl"},{line,1453}]},

[vinoski]

If you configured an extramod module with an extra_response_headers/3 function that returns an #outh{} record instead of a map, it could cause a crash like this if it were last in its config block.

[etnt]

The problem is the foldl loop beginning at 2039, the fun doesn't return a Map so the second iteration will get a outh record in the Map accumulator argument:

                  lists:foldl(fun(HdrVal, Map) ->
                                      {H,_} = split_header(strip_spaces(HdrVal)),
                                      case maps:is_key(H, Map) of
                                          true ->
                                              accumulate_header({H, maps:get(H, Map)});
                                          false ->
                                              ok
                                      end
                              end,
                              HdrMap,
                              string:tokens(lists:flatten(Other), "\r\n"))

[etnt]

Btw: regarding the use of a Map for the returned extra headers, will it be possible to return several headers with the same name (e.g 'Set-Cookie') ?

[vinoski]

OK, I pushed a fix for that, thanks for catching it.

Regarding multi-value headers, currently that's not possible. I thought about it while working on this branch the other day, and came up with a few options for handling that:

• Use a specific atom key for the map, say multi_value_headers, which would have a [{Header,Value}] list as a value, and repeated headers would be allowed in the list.
• Allow a Module:extra_response_headers/3 function to return either a list or a map, and the list could contain multi-value headers.
• Use a special value in the map to indicate multiple values, such as {values, [Value1, Value2|_]}.

I think the first option is kind of clunky, and I like the third option best, but perhaps the second option should also be supported.

[etnt]

Great! I'll look into it right away. Regarding multi-values, I think the third option is good enough.

[vinoski]

I think another bug is currently lurking here, where yaws:erase_header/1 is called. Unfortunately that function is really limited in its input, allowing only particular atoms. I'm working on a fix.

[vinoski]

Commit 262ec97 on the extra-response-headers branch now supports multi-value headers using a map value setting of {multi, [Value1, Value2, ...]}. This same approach is already used for the same purpose with #header{} records in Yaws, so I decided to reuse it here.

[etnt]

Seems like I'm only allowed to set multiple values on a predefined set of headers? Knowing customers...it would be nice if it was possible to set multiple values on any arbitrary header.

[vinoski]

We're restricted by HTTP standards as to what headers can appear multiple times. RFC 7230 states in section 3.2.2. "Field Order":

A sender MUST NOT generate multiple header fields with the same field name in a message unless either the entire field value for that header field is defined as a comma-separated list [i.e., #(values)] or the header field is a well-known exception (as noted below).

and the note referred to there is:

Note: In practice, the "Set-Cookie" header field ([RFC6265]) often appears multiple times in a response message and does not use the list syntax, violating the above requirements on multiple header fields with the same name. Since it cannot be combined into a single field-value, recipients ought to handle "Set-Cookie" as a special case while processing header fields.

When some other header is set with a {multi, ...} value, Yaws should combine the listed values into a single header with a value composed of all the values separated by commas. I don't think the code for output headers does this, so I'll need to add it.

[vinoski]

I've updated the branch with a new commit to handle multi values for all headers. The Set-Cookie header is handled specially as per RFC7230, and for all other headers, the values are combined into a single comma-separated list.

[etnt]

Great! Now everything seem to work nicely. Thanks!

[etnt]

btw: when is the next release due?

[vinoski]

Thanks for all your input and for working with this branch, @etnt . I'll get it to master soon.

As for the next release, I'd like it to be soon. Normally @klacke handles releases, but I haven't heard from him recently.

[vinoski]

I've rebased the extra-response-headers branch to master at c625001, so I'm closing this issue.