Skip to content

Support RFC 7239: Forwarded HTTP Extension #55

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
yadij wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Support RFC 7239: Forwarded HTTP Extension #55

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
da44a6d
RFC 7239 - Forwarded header initial support
yadij Aug 31, 2017
6938b1e
Use 'Configuration API' required by reviewer
yadij Aug 6, 2025
4c25e09
Update src/http.cc
yadij Aug 6, 2025
4c78e1e
Update src/cf.data.pre
yadij Aug 6, 2025
cd7b5c9
Apply suggestions from code review
yadij Aug 6, 2025
dfcb005
Update release-7.sgml.in
yadij Aug 6, 2025
976da29
Review requested changes
yadij Aug 6, 2025
93ded7c
Preserve true transparency on 'transparent'
yadij Aug 6, 2025
61120fc
maintenance
yadij Aug 6, 2025
afd3e53
Template instantiations cannot be inlined
yadij Aug 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 49 additions & 1 deletion doc/release-notes/release-7.sgml.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ The Squid-@SQUID_RELEASE@ change history can be <url url="https://github.com/squ
<item>Removed purge tool
<item>Remove deprecated languages
<item>Remove Ident protocol support
<item>>Support RFC 7239: Forwarded HTTP Extension
</itemize>

<p>Most user-facing changes are reflected in squid.conf (see further below).
Expand Down Expand Up @@ -127,6 +128,36 @@ in the position of what used to be a %ui record field.
<p>If necessary, an external ACL helper can be written to perform Ident transactions
and deliver the user identity to Squid through the **user=** annotation.

<sect1>>Support RFC 7239: Forwarded HTTP Extension
<p>Details in <url url="https://tools.ietf.org/html/rfc7239" name="RFC 7239: Forwarded HTTP Extension">

<p>The <em>Forwarded</em> extension has been standardized to replace
<em>X-Forwarded-For</em> and similar custom headers. This release contains
initial support for managing this HTTP extension in Squid with the
<em>http_header_forwarded</em> directive.

<p>The default Squid behaviour is now <em>transparent</em> - to relay any <em>Forwarded</em>
or <em>X-Forwarded-For</em> headers from the client unchanged to the server.
This prevents unintentionally exposing client IP in the HTTP headers.

<p>To send client IP and other details in the <em>Forwarded</em> header, the
<em>header_add</em> directive and ACLs can be used to control what gets
sent and on which traffic:
<verb>
acl internal dstdomain .example.com
acl ipv6 src ipv6

request_header_add Forwarded "for=\"%>a\";by=\"%>la:%>lp\"" !ipv6 internal
request_header_add Forwarded "for=\"[%>a]\";by=\"[%>la]:%>lp\"" ipv6 internal

request_header_add Forwarded "for=\"%>a\"" !ipv6 !internal
request_header_add Forwarded "for=\"[%>a]\"" ipv6 !internal
</verb>
Copy link
Contributor

@rousskov rousskov Jan 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Asking admins to use two rules (one for IPv4 and one for IPv6) to add a single XFF/Forwarded header is not acceptable IMO.

Several changes1 can address this concern, but I believe that the overall best way forward here (that addresses this and several other concerns -- I stopped explicitly flagging them at some point) is to go back to the current official design where a dedicated directive (other than generic request_header_add) is responsible for forming sent client IP values (when they are sent). In this context, Squid should treat all sent XFF and Forwarded header fields as a single configurable entity rather than a hodgepodge of items assembled in semi-independent ways, especially since items order matters and correct formatting of individual items is tricky (including being dependent on difficult XFF upgrade decisions).

For example (just to illustrate the concept, I am not insisting on any specific option set or spelling at this time):

forward_client_addresses
    from_received_headers=<on|off>
    from_client_connection=<on|off>
    upgrade_xff=<on|off>
    format=...
    ...

This PR may not have to implement all the possible/useful options, of course, but it has to create the right framework for follow up PRs while implementing the necessary minimum.

Footnotes

  1. For example, a prerequisite PR can introduce new %code formatting flag(s) that will result in %>a (when used with those new flag(s)) expanding to a quoted bracketed string for IPv6 addresses and unquoted string for IPv4 addresses. ↩

Copy link
Contributor Author

@yadij yadij Aug 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not a new requirement, we just have not documented it until now.

Recall that you previously required this PR scope reduction: - to omit the logic for adding any Forwarded header automatically.

Copy link
Contributor

@rousskov rousskov Jan 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alex: Asking admins to use two rules (one for IPv4 and one for IPv6) to add a single XFF/Forwarded header is not acceptable IMO.

Amos: This is not a new requirement, we just have not documented it until now.

I disagree that this requirement is old. To quote the original change request, "... current official design where a dedicated directive (other than generic request_header_add) is responsible for forming sent client IP values (when they are sent)". Official code does not have this requirement. It is new.

Copy link
Contributor

@rousskov rousskov Jan 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it survives other change requests, please move this example from release notes to http_header_forwarded directive documentation in cf.data.pre. Pointing to that example from release notes is fine, of course.


<p>The <em>delete</em> option can be selected to remove all <em>X-Forwarded-For</em>
and <em>Forwarded</em> headers from sent requests.


<sect>Changes to squid.conf since Squid-@SQUID_RELEASE_OLD@
<p>
This section gives an account of those changes in three categories:
Expand All @@ -141,7 +172,21 @@ This section gives an account of those changes in three categories:
<sect1>New directives<label id="newdirectives">
<p>
<descrip>
<p>No new directives in this version.
<tag>http_header_forwarded</tag>
<p>New directive to customize handling of Forwarded headers.
See <url url="https://tools.ietf.org/html/rfc7239" name="RFC 7239">
for details of this HTTP extension.
<p>Option <em>delete</em> removes both Forwarded and
X-Forwarded-For headers from traffic.
<p>Option <em>transparent</em> relays any Forwarded and
X-Forwarded-For headers without changes.
<p>Option <em>truncate</em> removes any existing
Forwarded and X-Forwarded-For headers, then adds
a new X-Forwarded-For header with Client IP.
<p>Deprecated Option <em>off</em> removes Forwarded headers and
appends 'unknown' to X-Forwarded-For.
<p>Deprecated Option <em>on</em> (the default) removes Forwarded
headers and appends Client IP to X-Forwarded-For.
Comment on lines +179 to +189
Copy link
Contributor

@rousskov rousskov Jan 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do not repeat squid.conf.documented documentation here, so that we do not have to keep both snippets in sync. Release notes are meant for summarizing important changes and detailing upgrade caveats, not for documenting new directive details.

Suggested change
<p>Option <em>delete</em> removes both Forwarded and
X-Forwarded-For headers from traffic.
<p>Option <em>transparent</em> relays any Forwarded and
X-Forwarded-For headers without changes.
<p>Option <em>truncate</em> removes any existing
Forwarded and X-Forwarded-For headers, then adds
a new X-Forwarded-For header with Client IP.
<p>Deprecated Option <em>off</em> removes Forwarded headers and
appends 'unknown' to X-Forwarded-For.
<p>Deprecated Option <em>on</em> (the default) removes Forwarded
headers and appends Client IP to X-Forwarded-For.

This change request and the above suggestion do not imply that these release notes should never talk about deprecated "off" and "on" settings. IMHO, the best place to talk about that is in the "Replaced by ..." text further below, where we should discuss how folks are supposed to upgrade their existing configurations.

Copy link
Contributor Author

@yadij yadij Aug 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are the changes to previous Squid UI behaviour that are new in this PR, as required for UI change documentation.

Copy link
Contributor

@rousskov rousskov Jan 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are the changes to previous Squid UI behaviour that are new in this PR, as required for UI change documentation.

These problematic paragraphs are documenting a lot more than changes and making it difficult to figure out what has actually changed. They are focused on documenting the details of how things work and needlessly repeat directive documentation. There is no requirement to repeat squid.conf.documented documentation in release notes. Release notes should flag/summarize changes and, where not obvious, explain their motivation, effects, and/or upgrade steps, all without duplicating directive documentation. Directive documentation lives elsewhere.


</descrip>

Expand Down Expand Up @@ -228,6 +273,9 @@ This section gives an account of those changes in three categories:
<tag>esi_parser</tag>
<p>Edge Side Includes (ESI) protocol is no longer supported natively.

<tag>forwarded_for</tag>
<p>Replaced by <em>http_header_forwarded</em> directive.

<tag>mcast_miss_addr</tag>
<p>The corresponding code has not built for many years, indicating that the
feature is unused.
Expand Down
5 changes: 5 additions & 0 deletions src/SquidConfig.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
#include "DelayConfig.h"
#endif
#include "helper/ChildConfig.h"
#include "http/forward.h"
#include "HttpHeaderTools.h"
#include "ip/Address.h"
#if USE_DELAY_POOLS
Expand Down Expand Up @@ -407,6 +408,10 @@ class SquidConfig
} accessList;
AclDenyInfoList *denyInfoList;

struct {
Http::ExtForwarded *header_forwarded;
} http;

struct {
size_t list_width;
int list_wrap;
Expand Down
1 change: 1 addition & 0 deletions src/cf.data.depend
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ externalAclHelper auth_param
HelperChildConfig
hostdomain cache_peer
hostdomaintype cache_peer
Http::ExtForwarded*
http_header_access acl
http_header_replace
http_upgrade_request_protocols acl
Expand Down
42 changes: 27 additions & 15 deletions src/cf.data.pre
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10294,29 +10294,41 @@ DOC_START
reduced memory thrashing in your malloc library.
DOC_END

NAME: forwarded_for
NAME: http_header_forwarded forwarded_for
Copy link
Contributor

@rousskov rousskov Jan 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not yet convinced that we need to rename this directive. Why is renaming better than keeping the current name? Please share your cost/benefit analysis.

FWIW, if we do end up renaming, I am pretty sure that http_header_forwarded is not a good name. Something like forward_client_addresses would be a lot better IMO.

Copy link
Contributor Author

@yadij yadij Aug 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well this directive is more about the "HTTP header Forwarded:" than about one (of many) possible values that can be sent in that header.

Copy link
Contributor

@rousskov rousskov Jan 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well this directive is more about the "HTTP header Forwarded:" than about one (of many) possible values that can be sent in that header.

In current PR code, this directive is more about the old X-Forwarded-For header than it is about Forwarded header. It controls X-Forwarded-For header generation. It does not control Forwarded header generation. Its only effect on Forwarded header is to prevent its forwarding (in all non-default cases). This PR does not change the scope of this directive; it deprecates old "actions", adds two new "actions", and changes the default, with all of that focusing on that old X-Forwarded-For header. In summary, the offered justification feels invalid, and I am still not convinced we need to rename this directive.

Perhaps renaming becomes a good idea when this directive does more, but then we should agree on that future before renaming, so that we do not have to rename twice.

COMMENT: on|off|transparent|truncate|delete
TYPE: string
DEFAULT: on
LOC: opt_forwarded_for
Copy link
Contributor

@rousskov rousskov Jan 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR adds no new functionality with respect to Forwarded header1, ignores RFC 7239 "Information Leak" section, and yet its title makes a "Support RFC 7239" claim. We should relax or remove that claim. Overall, this PR is not really about that RFC and that header AFAICT.

Footnotes

  1. The "delete" action is already available via request_header_access. The "transparent" action is the old default Squid behavior for that Forwarded header. ↩

TYPE: Http::ExtForwarded*
DEFAULT: transparent
LOC: Config.http.header_forwarded
DOC_START
If set to "on", Squid will append your client's IP address
in the HTTP requests it forwards. By default it looks like:
Controls how Squid handles the X-Forwarded-For and Forwarded
HTTP headers received from clients.

Copy link
Contributor

@rousskov rousskov Jan 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Controls how Squid handles received headers" is not precise enough for this context because the same can be said about follow_x_forwarded_for. Please rephrase to clarify the difference with follow_x_forwarded_for and to explicitly document that this directive does not affect how Squid uses received XFF and Forwarded headers for client IP address calculations. This directive is dedicated to forming request headers sent by Squid (using, in some cases, XFF and Forwarded headers received by Squid). In that updated documentation snippet, please refer to follow_x_forwarded_for, so that a reader has a better chance of understanding how the two directives control the two distinct aspects of Squid behavior tied to these headers.

Copy link
Contributor Author

@yadij yadij Aug 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed. Now consider this directive with a "follow [acl ...]" parameter - given that most follow_x_forwarded_for are just an allow and one ACL checking src-IPs of some trusted upstream proxies.

The purpose of this directive is (as documented), slightly wider in scope than what this restricted PR is able to implement right now.

Copy link
Contributor

@rousskov rousskov Jan 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The purpose of this directive is (as documented), slightly wider in scope than what this restricted PR is able to implement right now.

Be as it may, documentation problems mentioned in this change request still need to be fixed (as was detailed in the original change request).

The future of this directive is a separate matter. There is currently no consensus that it is a good idea to combine XFF forwarding instructions (i.e. this directive) and XFF using instructions (i.e. follow_x_forwarded_for). There is also a related unaddressed change request. If you think it is necessary, we can try to establish that consensus first, before advancing this old PR, but that may require serious new work, which will take even more time because there are more important problems (unrelated to this PR) that should be solved first.

Actions:

transparent
Squid will send all received Forwarded and X-Forwarded-For
headers without changes.

X-Forwarded-For: 192.1.2.3
delete Do not send any Forwarded or X-Forwarded-For headers.

If set to "off", it will appear as

X-Forwarded-For: unknown
Deprecated Actions:

If set to "transparent", Squid will not alter the
X-Forwarded-For header in any way.
on Sends received X-Forwarded-For with client IP address
or 'unknown' appended. Or, if no such header was
received, sends a new one containing the client IP or
'unknown' word.
Any received Forwarded header is not sent.

If set to "delete", Squid will delete the entire
X-Forwarded-For header.
off Sends received X-Forwarded-For with 'unknown' appended.
Or, if no such header was received, sends a new one
containing the 'unknown' word.
Any received Forwarded header is not sent.

If set to "truncate", Squid will remove all existing
X-Forwarded-For entries, and place the client IP as the sole entry.
truncate
Any received X-Forwarded-For or Forwarded header is removed.
Sends X-Forwarded-For containing only the client IP or
'unknown' word.
DOC_END

NAME: cachemgr_passwd
Expand Down
1 change: 0 additions & 1 deletion src/globals.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@ extern int neighbors_do_private_keys; /* 1 */
extern int opt_catch_signals; /* 1 */
extern int opt_foreground; /* 0 */
extern int opt_foreground_rebuild; /* 0 */
extern char *opt_forwarded_for; /* NULL */
extern int opt_reload_hit_only; /* 0 */

extern int opt_udp_hit_obj; /* 0 */
Expand Down
137 changes: 91 additions & 46 deletions src/http.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,12 @@
#include "comm/Write.h"
#include "error/Detail.h"
#include "errorpage.h"
#include "FadingCounter.h"
#include "fd.h"
#include "fde.h"
#include "globals.h"
#include "http.h"
#include "http/ExtForwarded.h"
#include "http/one/ResponseParser.h"
#include "http/one/TeChunkedParser.h"
#include "http/StatusCode.h"
Expand Down Expand Up @@ -1899,6 +1901,84 @@ httpFixupAuthentication(HttpRequest * request, const HttpHeader * hdr_in, HttpHe
return;
}

/// handle Forwarded and X-Forwarded-For headers
static void
fixupForwardedHeader(const HttpRequest &request, HttpHeader &hdr_out)
{
if (!Config.http.header_forwarded)
return;

switch (Config.http.header_forwarded->mode)
{
case Http::ExtForwarded::Mode::fwdDelete:
// do not send
break;

case Http::ExtForwarded::Mode::fwdTransparent:
// send what was received
// handled by copyOneHeaderFromClientsideRequestToUpstreamRequest()
break;

case Http::ExtForwarded::Mode::xffOn: {
// send what was received, with client-IP or 'unknown' appended
auto strFwd = request.header.getList(Http::HdrType::X_FORWARDED_FOR);

// if we cannot double strFwd size, then it grew past 50% of the limit
if (!strFwd.canGrowBy(strFwd.size())) {
Copy link
Contributor

@rousskov rousskov Jan 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please preserve official loop detection logic when moving that official code here. Changing that tricky logic is outside this PR scope AFAICT.

Copy link
Contributor Author

@yadij yadij Aug 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

None of the touched code is/was actually doing loop detection - that is done elsewhere. It was about preventing String crashes.

Copy link
Contributor

@rousskov rousskov Jan 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please address the essence of this change request rather than ignoring it because you disagree with informal code name/label used by the change request! I will rephrase using your name/label:

Please preserve official "String crashes" prevention logic when moving that official code here. Changing that tricky logic is outside this PR scope AFAICT.

// There is probably a forwarding loop with Via detection disabled.
// If we do nothing, String will assert on overflow soon.
// TODO: Terminate all transactions with huge XFF?
strFwd = "error";

static FadingCounter warnedCount;
if (warnedCount.count(1) < 100)
debugs(11, DBG_IMPORTANT, "WARNING: likely forwarding loop with " << request.effectiveRequestUri());
}

if (request.client_addr.isNoAddr()) {
strListAdd(&strFwd, "unknown", ',');
} else {
static char ntoabuf[MAX_IPSTRLEN];
strListAdd(&strFwd, request.client_addr.toStr(ntoabuf, MAX_IPSTRLEN), ',');
Copy link
Contributor

@rousskov rousskov Jan 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please avoid duplicating code that converts an IP address to a string. It is used here and in the xffOff case below.

Copy link
Contributor Author

@yadij yadij Aug 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the behaviour changes in Squid this code now falls into the rarely-used category. It is best to leave these one-liner for the compiler to optimize (or not) instead of adding wrappers for every function call.

Copy link
Contributor

@rousskov rousskov Jan 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change request applies to eight (poorly) duplicated address conversion lines rather than one:

        // send only client-IP or 'unknown'
        if (request.client_addr.isNoAddr()) {
            hdr_out.putStr(Http::HdrType::X_FORWARDED_FOR, "unknown");
        } else {
            static char ntoabuf[MAX_IPSTRLEN];
            request.client_addr.toStr(ntoabuf, MAX_IPSTRLEN);
            hdr_out.putStr(Http::HdrType::X_FORWARDED_FOR, ntoabuf);
        }

Look for isNoAddr(). One use case uses a strFwd variable. The other immediately calls hdr_out. I am pretty sure that difference (that does not exist in official code) can be removed while avoiding duplication.

P.S. The original change request text said "xffOff", but the second copy of this address conversion code is further down below in "xffTruncate" case. Sorry.

}

hdr_out.putStr(Http::HdrType::X_FORWARDED_FOR, strFwd.termedBuf());
}
break;

case Http::ExtForwarded::Mode::xffOff: {
// send what was received, with 'unknown' appended
auto strFwd = request.header.getList(Http::HdrType::X_FORWARDED_FOR);

// if we cannot double strFwd size, then it grew past 50% of the limit
Copy link
Contributor

@rousskov rousskov Jan 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do not duplicate this tricky code.

Copy link
Contributor Author

@yadij yadij Aug 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, tricky indeed - its not duplicated even if the one line and comments are.

Copy link
Contributor

@rousskov rousskov Jan 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, tricky indeed - its not duplicated even if the one line and comments are.

This change request applies to all duplicated code in this area, including that "one line and comment" code. In current PR code, the following code appears at least twice (i.e. it is duplicated):

        // if we cannot double strFwd size, then it grew past 50% of the limit
        if (!strFwd.canGrowBy(strFwd.size())) {
            // There is probably a forwarding loop with Via detection disabled.
            // If we do nothing, String will assert on overflow soon.
            // TODO: Terminate all transactions with huge XFF?
            strFwd = "error";

            static FadingCounter warnedCount;
            if (warnedCount.count(1) < 100)
                debugs(11, DBG_IMPORTANT, "WARNING: likely forwarding loop with " << request.effectiveRequestUri());
        }

if (!strFwd.canGrowBy(strFwd.size())) {
// There is probably a forwarding loop with Via detection disabled.
// If we do nothing, String will assert on overflow soon.
// TODO: Terminate all transactions with huge XFF?
strFwd = "error";

static FadingCounter warnedCount;
if (warnedCount.count(1) < 100)
debugs(11, DBG_IMPORTANT, "WARNING: likely forwarding loop with " << request.effectiveRequestUri());
}
strListAdd(&strFwd, "unknown", ',');
hdr_out.putStr(Http::HdrType::X_FORWARDED_FOR, strFwd.termedBuf());
}
break;

case Http::ExtForwarded::Mode::xffTruncate:
// send only client-IP or 'unknown'
if (request.client_addr.isNoAddr()) {
hdr_out.putStr(Http::HdrType::X_FORWARDED_FOR, "unknown");
} else {
static char ntoabuf[MAX_IPSTRLEN];
request.client_addr.toStr(ntoabuf, MAX_IPSTRLEN);
hdr_out.putStr(Http::HdrType::X_FORWARDED_FOR, ntoabuf);
}
break;
}
}

/*
* build request headers and append them to a given MemBuf
* used by buildRequestPrefix()
Expand All @@ -1915,7 +1995,6 @@ HttpStateData::httpBuildRequestHeader(HttpRequest * request,
/* building buffer for complex strings */
#define BBUF_SZ (MAX_URL+32)
LOCAL_ARRAY(char, bbuf, BBUF_SZ);
LOCAL_ARRAY(char, ntoabuf, MAX_IPSTRLEN);
const HttpHeader *hdr_in = &request->header;
const HttpHeaderEntry *e = nullptr;
HttpHeaderPos pos = HttpHeaderInitPos;
Expand Down Expand Up @@ -1961,48 +2040,7 @@ HttpStateData::httpBuildRequestHeader(HttpRequest * request,
hdr_out->putStr(Http::HdrType::SURROGATE_CAPABILITY, strSurrogate.termedBuf());
}

/** \pre Handle X-Forwarded-For */
if (strcmp(opt_forwarded_for, "delete") != 0) {

String strFwd = hdr_in->getList(Http::HdrType::X_FORWARDED_FOR);

// Detect unreasonably long header values. And paranoidly check String
// limits: a String ought to accommodate two reasonable-length values.
if (strFwd.size() > 32*1024 || !strFwd.canGrowBy(strFwd.size())) {
// There is probably a forwarding loop with Via detection disabled.
// If we do nothing, String will assert on overflow soon.
// TODO: Terminate all transactions with huge XFF?
strFwd = "error";

static int warnedCount = 0;
if (warnedCount++ < 100) {
const SBuf url(entry ? SBuf(entry->url()) : request->effectiveRequestUri());
debugs(11, DBG_IMPORTANT, "WARNING: likely forwarding loop with " << url);
}
}

if (strcmp(opt_forwarded_for, "on") == 0) {
/** If set to ON - append client IP or 'unknown'. */
if ( request->client_addr.isNoAddr() )
strListAdd(&strFwd, "unknown", ',');
else
strListAdd(&strFwd, request->client_addr.toStr(ntoabuf, MAX_IPSTRLEN), ',');
} else if (strcmp(opt_forwarded_for, "off") == 0) {
/** If set to OFF - append 'unknown'. */
strListAdd(&strFwd, "unknown", ',');
} else if (strcmp(opt_forwarded_for, "transparent") == 0) {
/** If set to TRANSPARENT - pass through unchanged. */
} else if (strcmp(opt_forwarded_for, "truncate") == 0) {
/** If set to TRUNCATE - drop existing list and replace with client IP or 'unknown'. */
if ( request->client_addr.isNoAddr() )
strFwd = "unknown";
else
strFwd = request->client_addr.toStr(ntoabuf, MAX_IPSTRLEN);
}
if (strFwd.size() > 0)
hdr_out->putStr(Http::HdrType::X_FORWARDED_FOR, strFwd.termedBuf());
}
/** If set to DELETE - do not copy through. */
fixupForwardedHeader(*request, *hdr_out);

/* append Host if not there already */
if (!hdr_out->has(Http::HdrType::HOST)) {
Expand Down Expand Up @@ -2303,10 +2341,8 @@ copyOneHeaderFromClientsideRequestToUpstreamRequest(const HttpHeaderEntry *e, co
hdr_out->addEntry(e->clone());
break;

case Http::HdrType::X_FORWARDED_FOR:

case Http::HdrType::CACHE_CONTROL:
/** \par X-Forwarded-For:, Cache-Control:
/** \par Cache-Control:
* handled specially by Squid, so leave off for now.
* append these after the loop if needed */
break;
Expand All @@ -2319,6 +2355,15 @@ copyOneHeaderFromClientsideRequestToUpstreamRequest(const HttpHeaderEntry *e, co

break;

case Http::HdrType::X_FORWARDED_FOR:
case Http::HdrType::FORWARDED:
/** \par X-Forwarded-For:, Forwarded:
* Pass thru only for 'transparent' action configured or default'ed.
Copy link
Contributor

@rousskov rousskov Jan 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR description: Unlike XFF these operations are enabled in all builds and transparent operation is default.

The above text says "Unlike XFF", but the corresponding code appears to apply to both XFF and Forwarded header. Should the above description be replaced with something like "these options are now enabled in all builds and transparent option is now the default"?

*/
if (!Config.http.header_forwarded || Config.http.header_forwarded->mode == Http::ExtForwarded::Mode::fwdTransparent)
hdr_out->addEntry(e->clone());
Comment on lines +2358 to +2364
Copy link
Contributor

@rousskov rousskov Jan 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR description: This adds the delete and transparent options as configurable for
this extension header in accordance with the RFC. Unlike XFF these operations are enabled in all builds and transparent operation is default.

"adds options as configurable for this header" is difficult to interpret correctly when this PR actually does not add those two options.

Why "Unlike XFF"? This code applies to XFF (in all builds) just like it applies to XFF AFAICT.

I suggest saying something like this instead (using a bulleted list format with proper wrapping):

  • Change forwarded_for default from on to transparent because ...

  • Apply forwarded_for delete and transparent options to Forwarded header handling. The other three options (i.e. on, off, and truncate) are not applied but are now deprecated.

  • Rename forwarded_for to ...

break;

default:
/** \par default.
* pass on all other header fields
Expand Down
Loading
Loading