Merge pull request #1733 from DuendeSoftware/beh/update-fragment

fix(error-redirect): use query-safe fragment as recommended in the latest OAuth 2 security best practices
DuendeSoftware · Jan 24, 2025 · 283aeae · 283aeae
2 parents 1c4e8fb + 854a667
commit 283aeae
Show file tree

Hide file tree

Showing 2 changed files with 65 additions and 2 deletions.
diff --git a/identity-server/src/IdentityServer/Endpoints/Results/AuthorizeResult.cs b/identity-server/src/IdentityServer/Endpoints/Results/AuthorizeResult.cs
@@ -177,8 +177,8 @@ private string BuildRedirectUri(AuthorizeResponse response)
 
         if (response.IsError && !uri.Contains('#'))
         {
-            // https://tools.ietf.org/html/draft-bradley-oauth-open-redirector-00
-            uri += "#_=_";
+            // https://datatracker.ietf.org/doc/html/draft-ietf-oauth-security-topics-29#section-4.1.3
+            uri += "#_";
         }
 
         return uri;

diff --git a/identity-server/test/IdentityServer.UnitTests/Endpoints/Results/AuthorizeResultTests.cs b/identity-server/test/IdentityServer.UnitTests/Endpoints/Results/AuthorizeResultTests.cs
@@ -267,4 +267,67 @@ public async Task form_post_mode_should_not_add_deprecated_header_when_it_is_dis
         _context.Response.Headers.ContentSecurityPolicy.First().Should().Contain($"script-src '{IdentityServerConstants.ContentSecurityPolicyHashes.AuthorizeScript}'");
         _context.Response.Headers["X-Content-Security-Policy"].Should().BeEmpty();
     }
+
+    [InlineData(OidcConstants.AuthorizeErrors.AccessDenied)]
+    [InlineData(OidcConstants.AuthorizeErrors.AccountSelectionRequired)]
+    [InlineData(OidcConstants.AuthorizeErrors.LoginRequired)]
+    [InlineData(OidcConstants.AuthorizeErrors.ConsentRequired)]
+    [InlineData(OidcConstants.AuthorizeErrors.InteractionRequired)]
+    [InlineData(OidcConstants.AuthorizeErrors.TemporarilyUnavailable)]
+    [InlineData(OidcConstants.AuthorizeErrors.UnmetAuthenticationRequirements)]
+    [Theory]
+    public async Task error_resulting_in_redirect_should_attach_fragment_to_location_header(string error)
+    {
+        _response.Error = error;
+        _response.Request = new ValidatedAuthorizeRequest
+        {
+            ClientId = "client",
+            GrantType = OidcConstants.GrantTypes.Implicit,
+            ResponseMode = OidcConstants.ResponseModes.Query,
+            RedirectUri = "http://client/callback",
+            ResponseType = OidcConstants.ResponseTypes.Token,
+        };
+
+        await _subject.WriteHttpResponse(new AuthorizeResult(_response), _context);
+
+        _context.Response.StatusCode.Should().Be(302);
+        var location = _context.Response.Headers.Location.First();
+        location.Should().StartWith("http://client/callback");
+        location.Should().Contain("#_");
+    }
+
+    [InlineData(OidcConstants.AuthorizeErrors.InvalidRequest)]
+    [InlineData(OidcConstants.AuthorizeErrors.UnauthorizedClient)]
+    [InlineData(OidcConstants.AuthorizeErrors.UnsupportedResponseType)]
+    [InlineData(OidcConstants.AuthorizeErrors.InvalidScope)]
+    [InlineData(OidcConstants.AuthorizeErrors.ServerError)]
+    [InlineData(OidcConstants.AuthorizeErrors.InvalidRequestUri)]
+    [InlineData(OidcConstants.AuthorizeErrors.InvalidRequestObject)]
+    [InlineData(OidcConstants.AuthorizeErrors.RequestNotSupported)]
+    [InlineData(OidcConstants.AuthorizeErrors.RequestUriNotSupported)]
+    [InlineData(OidcConstants.AuthorizeErrors.RegistrationNotSupported)]
+    [InlineData(OidcConstants.AuthorizeErrors.InvalidTarget)]
+    [Theory]
+    public async Task error_resulting_in_error_page_should_attach_fragment_to_error_model_redirect_uri(string error)
+    {
+        _response.Error = error;
+        _response.Request = new ValidatedAuthorizeRequest
+        {
+            ClientId = "client",
+            GrantType = OidcConstants.GrantTypes.Implicit,
+            ResponseMode = OidcConstants.ResponseModes.Query,
+            RedirectUri = "http://client/callback",
+            ResponseType = OidcConstants.ResponseTypes.Token,
+        };
+
+        await _subject.WriteHttpResponse(new AuthorizeResult(_response), _context);
+
+        _context.Response.StatusCode.Should().Be(302);
+        var location = _context.Response.Headers.Location.First();
+        var queryString = new Uri(location).Query;
+        var queryParams = QueryHelpers.ParseQuery(queryString);
+        var errorId = queryParams.First(kvp => kvp.Key == _options.UserInteraction.ErrorIdParameter).Value;
+        var errorMessage = await _mockErrorMessageStore.ReadAsync(errorId);
+        errorMessage.Data.RedirectUri.Should().Contain("#_");
+    }
 }