diff --git a/CHANGELOG.md b/CHANGELOG.md index 705f67a..7ad29d7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -488,6 +488,7 @@ follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Fixed +- `TMCPIntrospectionAuthorizer` sends the client credentials for the introspection endpoint as a single-line `Authorization: Basic` header. It encoded them with `TNetEncoding.Base64`, which inserts a line break after every 76 characters, so a `client_id:client_secret` of 57 bytes or more broke the request and every token was rejected; it now uses `TNetEncoding.Base64String`. - The `resource_metadata` URL in the `WWW-Authenticate` challenge is derived from `[Auth] ResourceUri` when it is set (RFC 9728: origin of the resource, `/.well-known/oauth-protected-resource`, then the path of the resource). It used to be built from `Protocol`, `Host` and the listening port, so a server behind a reverse proxy sent clients to `http://localhost:8080/...` instead of its public address. `TMCPProtectedResourceMetadata.UrlFor` does the derivation and `PathFor` gives the matching path, which the server now also serves without a token when it differs from `/.well-known/oauth-protected-resource`. `TMCPResourceLocation.TryParse` splits a resource URI into scheme, authority and path. - A record that failed to convert halfway leaked the objects it already held: `TMCPSerializer.JsonToValue` adds them to the caller's list only once the diff --git a/src/Core/MCPServer.Authorization.pas b/src/Core/MCPServer.Authorization.pas index c3abecb..edaa3fc 100644 --- a/src/Core/MCPServer.Authorization.pas +++ b/src/Core/MCPServer.Authorization.pas @@ -546,7 +546,9 @@ function TMCPIntrospectionAuthorizer.TryValidateToken(const Token: string; out C const HasClientId = (FClientId <> ''); if HasClientId then begin - const Credentials = TNetEncoding.Base64.Encode(Format('%s:%s', [FClientId, FClientSecret])); + const ClientCredentials = Format('%s:%s', [FClientId, FClientSecret]); + const Encoding = TNetEncoding.Base64String; + const Credentials = Encoding.Encode(ClientCredentials); Headers := Headers + [TNetHeader.Create('Authorization', Format('Basic %s', [Credentials]))]; end; diff --git a/tests/MCPServer.Tests.Authorization.pas b/tests/MCPServer.Tests.Authorization.pas index ccd8d7d..25afdbe 100644 --- a/tests/MCPServer.Tests.Authorization.pas +++ b/tests/MCPServer.Tests.Authorization.pas @@ -31,6 +31,8 @@ TAuthorizationTests = class FSeenBody: string; procedure HandleIntrospection(Context: TIdContext; Request: TIdHTTPRequestInfo; Response: TIdHTTPResponseInfo); function Decide(const Authorizer: IMCPAuthorizer; const Token: string): TMCPAuthResult; + procedure StartIntrospectionServer; + function IntrospectionUrl: string; public [TearDown] procedure TearDown; @@ -80,6 +82,9 @@ TAuthorizationTests = class [Test] procedure Introspection_PostsTokenWithClientCredentials; + + [Test] + procedure Introspection_LongClientSecret_SendsBasicHeaderOnOneLine; end; implementation @@ -88,7 +93,13 @@ implementation System.DateUtils; const - AUDIENCE = 'https://mcp.example/mcp'; + Audience = 'https://mcp.example/mcp'; + ClaimsWithExpiryFormat = '{"sub":"u","aud":"%s","exp":%d}'; + IntrospectionClientId = 'mcp'; + ActiveToken = 'good'; + LongSecretLength = 64; + LongSecretAuthorization = + 'Basic bWNwOnh4eHh4eHh4eHh4eHh4eHh4eHh4eHh4eHh4eHh4eHh4eHh4eHh4eHh4eHh4eHh4eHh4eHh4eHh4eHh4eHh4eHg='; { TClaimsAuthorizer } @@ -179,26 +190,31 @@ procedure TAuthorizationTests.OAuth_RejectsWrongAudience_Expiry_AndScope; var Future := System.DateUtils.DateTimeToUnix(Now, False) + 600; var Past := System.DateUtils.DateTimeToUnix(Now, False) - 600; - var Invalid: IMCPAuthorizer := TClaimsAuthorizer.Create(AUDIENCE, Format('{"sub":"u","aud":"%s","exp":%d}', [AUDIENCE, Future])); + const InvalidClaims = Format(ClaimsWithExpiryFormat, [Audience, Future]); + const Invalid: IMCPAuthorizer = TClaimsAuthorizer.Create(Audience, InvalidClaims); var Rejected := Decide(Invalid, 'nope'); Assert.AreEqual(TMCPAuthDecision.Unauthorized, Rejected.Decision); Assert.AreEqual('invalid_token', Rejected.Challenge.Error); - var WrongAudience: IMCPAuthorizer := TClaimsAuthorizer.Create(AUDIENCE, Format('{"sub":"u","aud":"https://other","exp":%d}', [Future])); + const OtherAudienceClaims = Format('{"sub":"u","aud":"https://other","exp":%d}', [Future]); + const WrongAudience: IMCPAuthorizer = TClaimsAuthorizer.Create(Audience, OtherAudienceClaims); var ForAnother := Decide(WrongAudience, 'valid'); Assert.AreEqual(TMCPAuthDecision.Unauthorized, ForAnother.Decision); Assert.IsTrue(ForAnother.Challenge.ErrorDescription.Contains('not issued for this server'), ForAnother.Challenge.ErrorDescription); - var Expired: IMCPAuthorizer := TClaimsAuthorizer.Create(AUDIENCE, Format('{"sub":"u","aud":"%s","exp":%d}', [AUDIENCE, Past])); + const ExpiredClaims = Format(ClaimsWithExpiryFormat, [Audience, Past]); + const Expired: IMCPAuthorizer = TClaimsAuthorizer.Create(Audience, ExpiredClaims); var Stale := Decide(Expired, 'valid'); Assert.AreEqual(TMCPAuthDecision.Unauthorized, Stale.Decision); Assert.IsTrue(Stale.Challenge.ErrorDescription.Contains('expired'), Stale.Challenge.ErrorDescription); - var NoExpiry: IMCPAuthorizer := TClaimsAuthorizer.Create(AUDIENCE, Format('{"sub":"u","aud":"%s"}', [AUDIENCE])); + const NoExpiryClaims = Format('{"sub":"u","aud":"%s"}', [Audience]); + const NoExpiry: IMCPAuthorizer = TClaimsAuthorizer.Create(Audience, NoExpiryClaims); Assert.AreEqual(TMCPAuthDecision.Unauthorized, Decide(NoExpiry, 'valid').Decision, 'exp is mandatory'); - var Scoped := TClaimsAuthorizer.Create(AUDIENCE, Format('{"sub":"u","aud":"%s","exp":%d,"scope":"read"}', [AUDIENCE, Future])); + const ScopedClaims = Format('{"sub":"u","aud":"%s","exp":%d,"scope":"read"}', [Audience, Future]); + const Scoped = TClaimsAuthorizer.Create(Audience, ScopedClaims); var ScopedRef: IMCPAuthorizer := Scoped; Scoped.RequiredScopes := ['read', 'write']; var Denied := Decide(ScopedRef, 'valid'); @@ -217,8 +233,9 @@ procedure TAuthorizationTests.OAuth_RejectsWrongAudience_Expiry_AndScope; procedure TAuthorizationTests.OAuth_AcceptsAudienceArray_AndScopeArray; begin var Future := System.DateUtils.DateTimeToUnix(Now, False) + 600; - var Authorizer: IMCPAuthorizer := TClaimsAuthorizer.Create(AUDIENCE, - Format('{"sub":"u","aud":["https://other","%s"],"exp":%d,"scp":["a","b"]}', [AUDIENCE.ToUpper, Future])); + const UpperAudience = Audience.ToUpper; + const ArrayClaims = Format('{"sub":"u","aud":["https://other","%s"],"exp":%d,"scp":["a","b"]}', [UpperAudience, Future]); + const Authorizer: IMCPAuthorizer = TClaimsAuthorizer.Create(Audience, ArrayClaims); var Outcome := Decide(Authorizer, 'valid'); Assert.AreEqual(TMCPAuthDecision.Allow, Outcome.Decision); Assert.IsTrue(Outcome.Principal.HasScope('a')); @@ -293,24 +310,36 @@ procedure TAuthorizationTests.HandleIntrospection(Context: TIdContext; Request: FSeenAuthorization := Request.RawHeaders.Values['Authorization']; FSeenBody := Request.FormParams; Response.ContentType := 'application/json'; + const Expiry = System.DateUtils.DateTimeToUnix(Now, False) + 600; if FSeenBody.Contains('token=good') then - Response.ContentText := Format('{"active":true,"sub":"alice","aud":"%s","exp":%d,"scope":"read"}', - [AUDIENCE, System.DateUtils.DateTimeToUnix(Now, False) + 600]) + Response.ContentText := Format('{"active":true,"sub":"alice","aud":"%s","exp":%d,"scope":"read"}', [Audience, Expiry]) else Response.ContentText := '{"active":false}'; end; -procedure TAuthorizationTests.Introspection_PostsTokenWithClientCredentials; +procedure TAuthorizationTests.StartIntrospectionServer; begin FIntrospection := TIdHTTPServer.Create(nil); - FIntrospection.Bindings.Add.IP := '127.0.0.1'; - FIntrospection.Bindings[0].Port := 0; + const Binding = FIntrospection.Bindings.Add; + Binding.IP := '127.0.0.1'; + Binding.Port := 0; FIntrospection.OnCommandGet := HandleIntrospection; FIntrospection.Active := True; - var Url := Format('http://127.0.0.1:%d/introspect', [FIntrospection.Bindings[0].Port]); +end; + +function TAuthorizationTests.IntrospectionUrl: string; +begin + const Binding = FIntrospection.Bindings[0]; + Result := Format('http://127.0.0.1:%d/introspect', [Binding.Port]); +end; + +procedure TAuthorizationTests.Introspection_PostsTokenWithClientCredentials; +begin + StartIntrospectionServer; + const Url = IntrospectionUrl; - var Authorizer: IMCPAuthorizer := TMCPIntrospectionAuthorizer.Create(AUDIENCE, Url, 'mcp', 's3cret'); - var Active := Decide(Authorizer, 'good'); + const Authorizer: IMCPAuthorizer = TMCPIntrospectionAuthorizer.Create(Audience, Url, IntrospectionClientId, 's3cret'); + const Active = Decide(Authorizer, ActiveToken); Assert.AreEqual(TMCPAuthDecision.Allow, Active.Decision); Assert.AreEqual('alice', Active.Principal.Subject); Assert.IsTrue(Active.Principal.HasScope('read')); @@ -322,4 +351,18 @@ procedure TAuthorizationTests.Introspection_PostsTokenWithClientCredentials; Assert.AreEqual('invalid_token', Stale.Challenge.Error); end; +procedure TAuthorizationTests.Introspection_LongClientSecret_SendsBasicHeaderOnOneLine; +begin + FSeenAuthorization := ''; + StartIntrospectionServer; + const Url = IntrospectionUrl; + const LongSecret = StringOfChar('x', LongSecretLength); + const Authorizer: IMCPAuthorizer = TMCPIntrospectionAuthorizer.Create(Audience, Url, IntrospectionClientId, LongSecret); + + const Active = Decide(Authorizer, ActiveToken); + + Assert.AreEqual(LongSecretAuthorization, FSeenAuthorization, False); + Assert.AreEqual(TMCPAuthDecision.Allow, Active.Decision); +end; + end.