From 2c1db7a726efc295036733f9f5a45d69cc1e35ce Mon Sep 17 00:00:00 2001 From: Ngo Quoc Dat Date: Sat, 10 Oct 2026 17:47:53 +0700 Subject: [PATCH] fix(mcp): stop counting requests without credentials toward the lockout --- CHANGELOG.md | 1 + .../Auth/MCPBearerTokenAuthenticator.swift | 10 +- .../MCPBearerTokenAuthenticatorTests.swift | 101 ++++++++++++++++-- .../Auth/MCPCompositeAuthenticatorTests.swift | 29 +++-- docs/developers/tokens.mdx | 2 +- docs/integrations/mcp-clients.mdx | 2 +- 6 files changed, 120 insertions(+), 25 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c575254ec8..9c962ee3a3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -97,6 +97,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - The toolbar keeping a connection's old name after it was renamed while connected. (#3309) - The iOS widget showing a different icon from the app for Dameng, Snowflake, Beancount, SurrealDB and Kafka connections. (#3309) - No icon for a connection to a database type no installed plugin describes. (#3309) +- MCP clients on the Mac locked out for 5 minutes after five requests sent without a token. ### Security diff --git a/TablePro/Core/MCP/Auth/MCPBearerTokenAuthenticator.swift b/TablePro/Core/MCP/Auth/MCPBearerTokenAuthenticator.swift index 67a09124fc..67534450f2 100644 --- a/TablePro/Core/MCP/Auth/MCPBearerTokenAuthenticator.swift +++ b/TablePro/Core/MCP/Auth/MCPBearerTokenAuthenticator.swift @@ -95,13 +95,11 @@ public actor MCPBearerTokenAuthenticator: MCPAuthenticator { return .deny(.rateLimited(retryAfterSeconds: retry)) } + // Not counted: it carries no guess, and every local client shares the loopback key. guard let header = authorizationHeader, !header.isEmpty else { - return await denyAttempt( - addressKey: addressKey, - ip: ipString, - reason: "missing_authorization_header", - denial: .unauthenticated(reason: "missing_authorization_header") - ) + Self.logger.info("Auth denied: reason=missing_authorization_header") + MCPAuditLogger.logAuthFailure(reason: "missing_authorization_header", ip: ipString) + return .deny(.unauthenticated(reason: "missing_authorization_header")) } guard let token = Self.parseBearerToken(header) else { diff --git a/TableProTests/Core/MCP/Auth/MCPBearerTokenAuthenticatorTests.swift b/TableProTests/Core/MCP/Auth/MCPBearerTokenAuthenticatorTests.swift index 231e67a2bb..42bcd6e91f 100644 --- a/TableProTests/Core/MCP/Auth/MCPBearerTokenAuthenticatorTests.swift +++ b/TableProTests/Core/MCP/Auth/MCPBearerTokenAuthenticatorTests.swift @@ -2,12 +2,6 @@ // MCPBearerTokenAuthenticatorTests.swift // TableProTests // -// The failure counter is keyed on the attacker, not on the credential the attacker presented. -// Keying it on the presented token let one address walk the whole token space without ever -// filling a bucket, and a request with no Authorization header at all was never counted. The -// challenge is a structured value now (RFC 6750 `WWW-Authenticate`), so the realm survives beside -// the error rather than being spliced into one string. -// import Foundation import TableProPluginKit @@ -260,19 +254,104 @@ struct MCPBearerTokenAuthenticatorTests { #expect((reason.retryAfterSeconds ?? 0) > 0) } - @Test("Requests with no Authorization header at all are counted as failures") - func missingHeaderCountsTowardsTheLockout() async throws { + @Test("Requests with no credential get the challenge every time and never lock the address") + func missingCredentialNeverLocksOut() async throws { + let store = FakeMCPTokenStore() + let plaintext = "tp_good" + await store.register(plaintext, validated: makeValidated()) + let (authenticator, limiter) = makeAuthenticator(store: store) + + for _ in 0..<10 { + for header in [nil, ""] as [String?] { + let decision = await authenticator.authenticate(authorizationHeader: header, clientAddress: .loopback) + let reason = try #require(denial(decision)) + #expect(reason.kind == .unauthenticated) + #expect(reason.challenge?.headerValue == "Bearer realm=\"TablePro\"") + } + } + #expect(await limiter.isLocked(key: .authFailure(address: .loopback)) == false) + #expect(await store.presentedTokens().isEmpty) + + let decision = await authenticator.authenticate( + authorizationHeader: "Bearer \(plaintext)", + clientAddress: .loopback + ) + guard case .allow = decision else { + Issue.record("Expected allow after requests with no credential, got \(decision)") + return + } + } + + @Test("Requests with no credential do not add to the failures bad tokens earn") + func missingCredentialDoesNotAddToBadTokenCount() async throws { let (authenticator, _) = makeAuthenticator(store: FakeMCPTokenStore()) - for _ in 0..<4 { - _ = await authenticator.authenticate(authorizationHeader: nil, clientAddress: .loopback) + for index in 0..<4 { + _ = await authenticator.authenticate( + authorizationHeader: "Bearer tp_wrong_\(index)", + clientAddress: .loopback + ) + } + for _ in 0..<10 { + let decision = await authenticator.authenticate(authorizationHeader: nil, clientAddress: .loopback) + #expect(denial(decision)?.kind == .unauthenticated) } - let final = await authenticator.authenticate(authorizationHeader: nil, clientAddress: .loopback) + let final = await authenticator.authenticate( + authorizationHeader: "Bearer tp_wrong_4", + clientAddress: .loopback + ) let reason = try #require(denial(final)) #expect(reason.kind == .rateLimited) } + @Test("Every presented credential that fails counts toward the lockout") + func presentedFailuresStillLockOut() async throws { + let store = FakeMCPTokenStore() + await store.register("tp_expired", validated: makeValidated()) + await store.markExpired("tp_expired") + await store.register("tp_revoked", validated: makeValidated()) + await store.markRevoked("tp_revoked") + let (authenticator, _) = makeAuthenticator(store: store) + + let headers = ["Basic dXNlcjpwYXNz", "Bearer", "Bearer tp_unknown", "Bearer tp_expired"] + for header in headers { + let decision = await authenticator.authenticate(authorizationHeader: header, clientAddress: .loopback) + #expect(denial(decision)?.httpStatus == 401) + } + + let final = await authenticator.authenticate( + authorizationHeader: "Bearer tp_revoked", + clientAddress: .loopback + ) + let reason = try #require(denial(final)) + #expect(reason.kind == .rateLimited) + } + + @Test("A lockout refuses requests with no credential while it lasts") + func lockoutRefusesMissingCredential() async throws { + let clock = MCPTestClock() + let (authenticator, _) = makeAuthenticator(store: FakeMCPTokenStore(), clock: clock) + + for index in 0..<5 { + _ = await authenticator.authenticate( + authorizationHeader: "Bearer tp_wrong_\(index)", + clientAddress: .loopback + ) + } + + for header in [nil, ""] as [String?] { + let decision = await authenticator.authenticate(authorizationHeader: header, clientAddress: .loopback) + let reason = try #require(denial(decision)) + #expect(reason.kind == .rateLimited) + #expect((reason.retryAfterSeconds ?? 0) > 0) + } + + await clock.advance(by: .seconds(301)) + let afterLockout = await authenticator.authenticate(authorizationHeader: nil, clientAddress: .loopback) + #expect(denial(afterLockout)?.kind == .unauthenticated) + } + @Test("A locked address is refused before its token is even looked up") func lockoutPrecedesValidation() async throws { let store = FakeMCPTokenStore() diff --git a/TableProTests/Core/MCP/Auth/MCPCompositeAuthenticatorTests.swift b/TableProTests/Core/MCP/Auth/MCPCompositeAuthenticatorTests.swift index 383e27e08e..2a72a3a3df 100644 --- a/TableProTests/Core/MCP/Auth/MCPCompositeAuthenticatorTests.swift +++ b/TableProTests/Core/MCP/Auth/MCPCompositeAuthenticatorTests.swift @@ -2,12 +2,6 @@ // MCPCompositeAuthenticatorTests.swift // TableProTests // -// Two rules meet here. The anonymous loopback principal is the weakest identity the server can -// hand out, so it carries read scopes only and no issued token: a write or an admin operation is -// refused for it whatever its scope set says. And presenting a credential opts out of anonymity -// entirely, so a revoked or expired token is refused rather than silently downgraded to the -// anonymous principal, which is how a token the user had revoked kept working. -// import Foundation @testable import TablePro @@ -204,6 +198,29 @@ struct MCPCompositeAuthenticatorTests { #expect(reason.httpStatus == 401) } + @Test("Probing without a credential never locks out the client that then sends its token") + func probingWithoutCredentialDoesNotLockOut() async { + let store = FakeMCPTokenStore() + let plaintext = "tp_client" + await store.register(plaintext, validated: makeValidated(label: "Client")) + let composite = makeComposite(store, requireAuthentication: true) + + for _ in 0..<10 { + let probe = await composite.authenticate(authorizationHeader: nil, clientAddress: .loopback) + #expect(denial(probe)?.httpStatus == 401) + } + + let decision = await composite.authenticate( + authorizationHeader: "Bearer \(plaintext)", + clientAddress: .loopback + ) + guard case .allow(let principal) = decision else { + Issue.record("Expected allow, got \(decision)") + return + } + #expect(principal.metadata.label == "Client") + } + @Test("Requiring authentication accepts a valid token from a remote client") func remoteWithValidTokenIsAllowed() async { let store = FakeMCPTokenStore() diff --git a/docs/developers/tokens.mdx b/docs/developers/tokens.mdx index 0a82e8954c..b1ced2628c 100644 --- a/docs/developers/tokens.mdx +++ b/docs/developers/tokens.mdx @@ -117,7 +117,7 @@ Entries are chained: each row's hash covers the previous one, so a row cannot be | Failed pairing exchanges | 5 per 5 minutes | | Pairing lockout | 15 minutes | -Request limits are keyed by token id, or by client address for an anonymous caller, and `subscriptions/listen` is exempt. Auth failures count against both the client address and the token, so one misbehaving client cannot lock out another on the same loopback address, and a successful auth clears the bucket. Anything over a limit returns `-33010` with HTTP `429` and a `Retry-After` header. Transport limits such as the body cap are on [MCP Protocol](/developers/mcp-protocol#transport-limits). +Request limits are keyed by token id, or by client address for an anonymous caller, and `subscriptions/listen` is exempt. Failed auth attempts are counted per client address, and every client on the Mac shares one, so a client that keeps sending a bad token locks the others out too. A malformed `Authorization` header, a scheme other than `Bearer`, and an unknown, expired or revoked token each count. A request with no `Authorization` header gets the `401` challenge and does not count. A successful auth clears the count. Anything over a limit returns `-33010` with HTTP `429` and a `Retry-After` header. Transport limits such as the body cap are on [MCP Protocol](/developers/mcp-protocol#transport-limits). ## What a token cannot do diff --git a/docs/integrations/mcp-clients.mdx b/docs/integrations/mcp-clients.mdx index 750ab2c080..dde4a10d09 100644 --- a/docs/integrations/mcp-clients.mdx +++ b/docs/integrations/mcp-clients.mdx @@ -179,4 +179,4 @@ The bridge token is stale. The bridge re-reads the handshake and retries once on ### Rate limited (`-33010`) -Five failed auth attempts inside 60 seconds triggered a 5-minute lockout. Wait it out, or restart TablePro. The other [rate limits](/developers/tokens#rate-limits) return the same code. +Five failed auth attempts inside 60 seconds triggered a 5-minute lockout for every client on this Mac. A request that sends no token is not a failed attempt. Wait it out, or restart TablePro. The other [rate limits](/developers/tokens#rate-limits) return the same code.