From: Willy Tarreau <w@1wt.eu>
Date: Wed, 21 Jan 2015 19:39:27 +0000 (+0100)
Subject: BUG/MEDIUM: http: make http-request set-header compute the string before removal
X-Git-Tag: v1.6-dev1~177
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=85603282111d6fecb4a703f4af156c69a983cc5e;p=thirdparty%2Fhaproxy.git

BUG/MEDIUM: http: make http-request set-header compute the string before removal

The way http-request/response set-header works is stupid. For a naive
reuse of the del-header code, it removes all occurrences of the header
to be set before computing the new format string. This makes it almost
unusable because it is not possible to append values to an existing
header without first copying them to a dummy header, performing the
copy back and removing the dummy header.

Instead, let's share the same code as add-header and perform the optional
removal after the string is computed. That way it becomes possible to
write things like :

   http-request set-header X-Forwarded-For %[hdr(X-Forwarded-For)],%[src]

Note that this change is not expected to have any undesirable impact on
existing configs since if they rely on the bogus behaviour, they don't
work as they always retrieve an empty string.

This fix must be backported to 1.5 to stop the spreadth of ugly configs.
---

diff --git a/doc/configuration.txt b/doc/configuration.txt
index cd01724759..e899297cd0 100644
--- a/doc/configuration.txt
+++ b/doc/configuration.txt
@@ -3034,7 +3034,8 @@ http-request { allow | deny | tarpit | auth [realm <realm>] | redirect <rule> |
     - "set-header" does the same as "add-header" except that the header name
       is first removed if it existed. This is useful when passing security
       information to the server, where the header must not be manipulated by
-      external users.
+      external users. Note that the new value is computed before the removal so
+      it is possible to concatenate a value to an existing header.
 
     - "del-header" removes all HTTP header fields whose name is specified in
       <name>.
diff --git a/src/proto_http.c b/src/proto_http.c
index 93259e33eb..4d259df7c8 100644
--- a/src/proto_http.c
+++ b/src/proto_http.c
@@ -3389,17 +3389,15 @@ http_req_get_intercept_rule(struct proxy *px, struct list *rules, struct session
 			break;
 
 		case HTTP_REQ_ACT_DEL_HDR:
-		case HTTP_REQ_ACT_SET_HDR:
 			ctx.idx = 0;
 			/* remove all occurrences of the header */
 			while (http_find_header2(rule->arg.hdr_add.name, rule->arg.hdr_add.name_len,
 						 txn->req.chn->buf->p, &txn->hdr_idx, &ctx)) {
 				http_remove_header2(&txn->req, &txn->hdr_idx, &ctx);
 			}
-			if (rule->action == HTTP_REQ_ACT_DEL_HDR)
-				break;
-			/* now fall through to header addition */
+			break;
 
+		case HTTP_REQ_ACT_SET_HDR:
 		case HTTP_REQ_ACT_ADD_HDR:
 			chunk_printf(&trash, "%s: ", rule->arg.hdr_add.name);
 			memcpy(trash.str, rule->arg.hdr_add.name, rule->arg.hdr_add.name_len);
@@ -3407,6 +3405,16 @@ http_req_get_intercept_rule(struct proxy *px, struct list *rules, struct session
 			trash.str[trash.len++] = ':';
 			trash.str[trash.len++] = ' ';
 			trash.len += build_logline(s, trash.str + trash.len, trash.size - trash.len, &rule->arg.hdr_add.fmt);
+
+			if (rule->action == HTTP_REQ_ACT_SET_HDR) {
+				/* remove all occurrences of the header */
+				ctx.idx = 0;
+				while (http_find_header2(rule->arg.hdr_add.name, rule->arg.hdr_add.name_len,
+							 txn->req.chn->buf->p, &txn->hdr_idx, &ctx)) {
+					http_remove_header2(&txn->req, &txn->hdr_idx, &ctx);
+				}
+			}
+
 			http_header_add_tail2(&txn->req, &txn->hdr_idx, trash.str, trash.len);
 			break;
 
@@ -3611,17 +3619,15 @@ http_res_get_intercept_rule(struct proxy *px, struct list *rules, struct session
 			break;
 
 		case HTTP_RES_ACT_DEL_HDR:
-		case HTTP_RES_ACT_SET_HDR:
 			ctx.idx = 0;
 			/* remove all occurrences of the header */
 			while (http_find_header2(rule->arg.hdr_add.name, rule->arg.hdr_add.name_len,
 						 txn->rsp.chn->buf->p, &txn->hdr_idx, &ctx)) {
 				http_remove_header2(&txn->rsp, &txn->hdr_idx, &ctx);
 			}
-			if (rule->action == HTTP_RES_ACT_DEL_HDR)
-				break;
-			/* now fall through to header addition */
+			break;
 
+		case HTTP_RES_ACT_SET_HDR:
 		case HTTP_RES_ACT_ADD_HDR:
 			chunk_printf(&trash, "%s: ", rule->arg.hdr_add.name);
 			memcpy(trash.str, rule->arg.hdr_add.name, rule->arg.hdr_add.name_len);
@@ -3629,6 +3635,15 @@ http_res_get_intercept_rule(struct proxy *px, struct list *rules, struct session
 			trash.str[trash.len++] = ':';
 			trash.str[trash.len++] = ' ';
 			trash.len += build_logline(s, trash.str + trash.len, trash.size - trash.len, &rule->arg.hdr_add.fmt);
+
+			if (rule->action == HTTP_RES_ACT_SET_HDR) {
+				/* remove all occurrences of the header */
+				ctx.idx = 0;
+				while (http_find_header2(rule->arg.hdr_add.name, rule->arg.hdr_add.name_len,
+							 txn->rsp.chn->buf->p, &txn->hdr_idx, &ctx)) {
+					http_remove_header2(&txn->rsp, &txn->hdr_idx, &ctx);
+				}
+			}
 			http_header_add_tail2(&txn->rsp, &txn->hdr_idx, trash.str, trash.len);
 			break;