From 8e5c8606682257bfc87e618ed569df8ad37c1575 Mon Sep 17 00:00:00 2001
From: Chris R <Tratcher@outlook.com>
Date: Wed, 8 Nov 2023 11:22:58 -0800
Subject: [PATCH] Conditionally copy Strict-Transport-Security

---
 docs/docfx/articles/header-guidelines.md      |  5 ++-
 src/ReverseProxy/Forwarder/HttpTransformer.cs | 14 +++++++-
 .../Forwarder/RequestUtilities.cs             |  3 +-
 .../Forwarder/HttpTransformerTests.cs         | 33 ++++++++++++++++++-
 4 files changed, 50 insertions(+), 5 deletions(-)

diff --git a/docs/docfx/articles/header-guidelines.md b/docs/docfx/articles/header-guidelines.md
index c47376cdf..c39ec004c 100644
--- a/docs/docfx/articles/header-guidelines.md
+++ b/docs/docfx/articles/header-guidelines.md
@@ -45,6 +45,10 @@ services.AddReverseProxy()
     .ConfigureHttpClient((_, handler) => handler.ActivityHeadersPropagator = null);
 ```
 
+### Strict-Transport-Security
+
+This header instructs clients to always use HTTPS, but there may be a conflict between values provided by the proxy and destination. To avoid confusion, the destination's value is not copied to the response if one was already added to the response by the proxy application.
+
 ## Other Header Guidelines
 
 ### Host
@@ -74,4 +78,3 @@ This response header indicates what server technology was used to generate the r
 ### X-Powered-By
 
 This response header indicates what web framework was used to generate the response (ASP.NET, etc.). ASP.NET Core does not generate this header but IIS can. This header is proxied from the destination by default. Applications that want to remove it can use the [ResponseHeaderRemove](transforms.md#responseheaderremove) transform.
-
diff --git a/src/ReverseProxy/Forwarder/HttpTransformer.cs b/src/ReverseProxy/Forwarder/HttpTransformer.cs
index 6df6237cf..c8d7c36ab 100644
--- a/src/ReverseProxy/Forwarder/HttpTransformer.cs
+++ b/src/ReverseProxy/Forwarder/HttpTransformer.cs
@@ -11,6 +11,7 @@
 using System.Threading.Tasks;
 using Microsoft.AspNetCore.Http;
 using Microsoft.AspNetCore.Http.Features;
+using Microsoft.Extensions.Primitives;
 using Microsoft.Net.Http.Headers;
 using Yarp.ReverseProxy.Transforms.Builder;
 
@@ -258,7 +259,18 @@ private static void CopyResponseHeaders(HttpHeaders source, IHeaderDictionary de
                 continue;
             }
 
-            destination[headerName] = RequestUtilities.Concat(destination[headerName], header.Value);
+            var currentValue = destination[headerName];
+
+            // https://github.com/microsoft/reverse-proxy/issues/2269
+            // The Strict-Transport-Security may be added by the proxy before forwarding. Only copy the header
+            // if it's not already present.
+            if (!StringValues.IsNullOrEmpty(currentValue)
+                && string.Equals(headerName, HeaderNames.StrictTransportSecurity, StringComparison.OrdinalIgnoreCase))
+            {
+                continue;
+            }
+
+            destination[headerName] = RequestUtilities.Concat(currentValue, header.Value);
         }
     }
 }
diff --git a/src/ReverseProxy/Forwarder/RequestUtilities.cs b/src/ReverseProxy/Forwarder/RequestUtilities.cs
index e2914d4de..7b10afa42 100644
--- a/src/ReverseProxy/Forwarder/RequestUtilities.cs
+++ b/src/ReverseProxy/Forwarder/RequestUtilities.cs
@@ -68,7 +68,7 @@ internal static bool ShouldSkipResponseHeader(string headerName)
         return _headersToExclude.Contains(headerName);
     }
 
-    private static readonly FrozenSet<string> _headersToExclude = new HashSet<string>(18, StringComparer.OrdinalIgnoreCase)
+    private static readonly FrozenSet<string> _headersToExclude = new HashSet<string>(17, StringComparer.OrdinalIgnoreCase)
     {
         HeaderNames.Connection,
         HeaderNames.TransferEncoding,
@@ -87,7 +87,6 @@ internal static bool ShouldSkipResponseHeader(string headerName)
         HeaderNames.UpgradeInsecureRequests,
         HeaderNames.TE,
         HeaderNames.AltSvc,
-        HeaderNames.StrictTransportSecurity,
     }.ToFrozenSet(StringComparer.OrdinalIgnoreCase);
 
     // Headers marked as HttpHeaderType.Content in
diff --git a/test/ReverseProxy.Tests/Forwarder/HttpTransformerTests.cs b/test/ReverseProxy.Tests/Forwarder/HttpTransformerTests.cs
index 2c6c038c3..abdb72e0a 100644
--- a/test/ReverseProxy.Tests/Forwarder/HttpTransformerTests.cs
+++ b/test/ReverseProxy.Tests/Forwarder/HttpTransformerTests.cs
@@ -39,7 +39,6 @@ public class HttpTransformerTests
         HeaderNames.UpgradeInsecureRequests,
         HeaderNames.TE,
         HeaderNames.AltSvc,
-        HeaderNames.StrictTransportSecurity,
     };
 
     [Fact]
@@ -163,6 +162,38 @@ public async Task TransformResponseAsync_RemovesRestrictedHeaders()
         }
     }
 
+    [Theory]
+    [InlineData(true)]
+    [InlineData(false)]
+    public async Task TransformResponseAsync_StrictTransportSecurity_CopiedIfNotPresent(bool alreadyPresent)
+    {
+        var transformer = HttpTransformer.Default;
+        var httpContext = new DefaultHttpContext();
+        var proxyResponse = new HttpResponseMessage()
+        {
+            Content = new ByteArrayContent(Array.Empty<byte>())
+        };
+
+        if (alreadyPresent)
+        {
+            httpContext.Response.Headers.StrictTransportSecurity = "max-age=31536000; includeSubDomains";
+        }
+
+        Assert.True(proxyResponse.Headers.TryAddWithoutValidation(HeaderNames.StrictTransportSecurity, "max-age=31000; preload"));
+
+        await transformer.TransformResponseAsync(httpContext, proxyResponse, CancellationToken.None);
+
+        var result = httpContext.Response.Headers.StrictTransportSecurity;
+        if (alreadyPresent)
+        {
+            Assert.Equal("max-age=31536000; includeSubDomains", result);
+        }
+        else
+        {
+            Assert.Equal("max-age=31000; preload", result);
+        }
+    }
+
     [Fact]
     public async Task TransformResponseAsync_ContentLengthAndTransferEncoding_ContentLengthRemoved()
     {