simplify PR / only keep verification in demo inmemory oauth provider
Co-Authored-By: Claude <noreply@anthropic.com>
This commit is contained in:
Olivier Chafik
parent
cba6a6ea58
commit
ccccb4b7cc
4 changed files with 290 additions and 486 deletions
|
|
@ -1,360 +0,0 @@
|
|||
import express from "express";
|
||||
import request from "supertest";
|
||||
import { authorizationHandler } from "./authorize.js";
|
||||
import { OAuthServerProvider } from "../provider.js";
|
||||
import { OAuthServerConfig } from "../types.js";
|
||||
|
||||
describe("Authorization handler with config", () => {
|
||||
let app: express.Application;
|
||||
let mockProvider: jest.Mocked<OAuthServerProvider>;
|
||||
|
||||
beforeEach(() => {
|
||||
app = express();
|
||||
|
||||
const mockClientsStore = {
|
||||
getClient: jest.fn(),
|
||||
registerClient: jest.fn(),
|
||||
};
|
||||
|
||||
mockProvider = {
|
||||
clientsStore: mockClientsStore,
|
||||
authorize: jest.fn(),
|
||||
exchangeAuthorizationCode: jest.fn(),
|
||||
exchangeRefreshToken: jest.fn(),
|
||||
challengeForAuthorizationCode: jest.fn(),
|
||||
verifyAccessToken: jest.fn(),
|
||||
} as jest.Mocked<OAuthServerProvider>;
|
||||
});
|
||||
|
||||
describe("validateResourceMatchesServer configuration", () => {
|
||||
it("should throw error when validateResourceMatchesServer is true but serverUrl is not set", () => {
|
||||
const invalidConfig: OAuthServerConfig = {
|
||||
validateResourceMatchesServer: true
|
||||
// serverUrl is missing
|
||||
};
|
||||
|
||||
expect(() => {
|
||||
authorizationHandler({
|
||||
provider: mockProvider,
|
||||
config: invalidConfig
|
||||
});
|
||||
}).toThrow("serverUrl must be configured when validateResourceMatchesServer is true");
|
||||
});
|
||||
});
|
||||
|
||||
describe("server URL validation (validateResourceMatchesServer: true)", () => {
|
||||
const serverValidationConfig: OAuthServerConfig = {
|
||||
serverUrl: "https://api.example.com/mcp",
|
||||
validateResourceMatchesServer: true
|
||||
};
|
||||
|
||||
beforeEach(() => {
|
||||
app.use("/oauth/authorize", authorizationHandler({
|
||||
provider: mockProvider,
|
||||
config: serverValidationConfig
|
||||
}));
|
||||
});
|
||||
|
||||
it("should reject requests without resource parameter", async () => {
|
||||
const mockClient = {
|
||||
client_id: "test-client",
|
||||
client_name: "Test Client",
|
||||
redirect_uris: ["https://example.com/callback"],
|
||||
grant_types: ["authorization_code"],
|
||||
response_types: ["code"],
|
||||
scope: "read write",
|
||||
token_endpoint_auth_method: "none",
|
||||
};
|
||||
|
||||
(mockProvider.clientsStore.getClient as jest.Mock).mockResolvedValue(mockClient);
|
||||
|
||||
const response = await request(app)
|
||||
.get("/oauth/authorize")
|
||||
.query({
|
||||
client_id: "test-client",
|
||||
redirect_uri: "https://example.com/callback",
|
||||
response_type: "code",
|
||||
code_challenge: "test-challenge",
|
||||
code_challenge_method: "S256",
|
||||
state: "test-state"
|
||||
});
|
||||
|
||||
expect(response.status).toBe(302);
|
||||
expect(response.headers.location).toContain("error=invalid_request");
|
||||
expect(response.headers.location).toContain("Resource+parameter+is+required+when+server+URL+validation+is+enabled");
|
||||
});
|
||||
|
||||
it("should accept requests with resource parameter", async () => {
|
||||
const mockClient = {
|
||||
client_id: "test-client",
|
||||
client_name: "Test Client",
|
||||
redirect_uris: ["https://example.com/callback"],
|
||||
grant_types: ["authorization_code"],
|
||||
response_types: ["code"],
|
||||
scope: "read write",
|
||||
token_endpoint_auth_method: "none",
|
||||
};
|
||||
|
||||
(mockProvider.clientsStore.getClient as jest.Mock).mockResolvedValue(mockClient);
|
||||
mockProvider.authorize.mockImplementation(async (client, params, res) => {
|
||||
res.redirect(`https://example.com/callback?code=auth-code&state=${params.state}`);
|
||||
});
|
||||
|
||||
const response = await request(app)
|
||||
.get("/oauth/authorize")
|
||||
.query({
|
||||
client_id: "test-client",
|
||||
redirect_uri: "https://example.com/callback",
|
||||
response_type: "code",
|
||||
code_challenge: "test-challenge",
|
||||
code_challenge_method: "S256",
|
||||
state: "test-state",
|
||||
resource: "https://api.example.com/mcp"
|
||||
});
|
||||
|
||||
expect(response.status).toBe(302);
|
||||
expect(response.headers.location).toContain("code=auth-code");
|
||||
expect(mockProvider.authorize).toHaveBeenCalledWith(
|
||||
mockClient,
|
||||
expect.objectContaining({
|
||||
resource: "https://api.example.com/mcp"
|
||||
}),
|
||||
expect.any(Object)
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
describe("warning mode (default behavior)", () => {
|
||||
const warnConfig: OAuthServerConfig = {
|
||||
// No configuration needed - warnings are always enabled by default
|
||||
};
|
||||
|
||||
let consoleWarnSpy: jest.SpyInstance;
|
||||
|
||||
beforeEach(() => {
|
||||
consoleWarnSpy = jest.spyOn(console, "warn").mockImplementation();
|
||||
app.use("/oauth/authorize", authorizationHandler({
|
||||
provider: mockProvider,
|
||||
config: warnConfig
|
||||
}));
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
consoleWarnSpy.mockRestore();
|
||||
});
|
||||
|
||||
it("should log warning when resource is missing", async () => {
|
||||
const mockClient = {
|
||||
client_id: "test-client",
|
||||
client_name: "Test Client",
|
||||
redirect_uris: ["https://example.com/callback"],
|
||||
grant_types: ["authorization_code"],
|
||||
response_types: ["code"],
|
||||
scope: "read write",
|
||||
token_endpoint_auth_method: "none",
|
||||
};
|
||||
|
||||
(mockProvider.clientsStore.getClient as jest.Mock).mockResolvedValue(mockClient);
|
||||
mockProvider.authorize.mockImplementation(async (client, params, res) => {
|
||||
res.redirect(`https://example.com/callback?code=auth-code&state=${params.state}`);
|
||||
});
|
||||
|
||||
await request(app)
|
||||
.get("/oauth/authorize")
|
||||
.query({
|
||||
client_id: "test-client",
|
||||
redirect_uri: "https://example.com/callback",
|
||||
response_type: "code",
|
||||
code_challenge: "test-challenge",
|
||||
code_challenge_method: "S256",
|
||||
state: "test-state"
|
||||
});
|
||||
|
||||
expect(consoleWarnSpy).toHaveBeenCalledWith(
|
||||
expect.stringContaining("test-client is missing the resource parameter")
|
||||
);
|
||||
});
|
||||
|
||||
it("should not log warning when resource is present", async () => {
|
||||
const mockClient = {
|
||||
client_id: "test-client",
|
||||
client_name: "Test Client",
|
||||
redirect_uris: ["https://example.com/callback"],
|
||||
grant_types: ["authorization_code"],
|
||||
response_types: ["code"],
|
||||
scope: "read write",
|
||||
token_endpoint_auth_method: "none",
|
||||
};
|
||||
|
||||
(mockProvider.clientsStore.getClient as jest.Mock).mockResolvedValue(mockClient);
|
||||
mockProvider.authorize.mockImplementation(async (client, params, res) => {
|
||||
res.redirect(`https://example.com/callback?code=auth-code&state=${params.state}`);
|
||||
});
|
||||
|
||||
await request(app)
|
||||
.get("/oauth/authorize")
|
||||
.query({
|
||||
client_id: "test-client",
|
||||
redirect_uri: "https://example.com/callback",
|
||||
response_type: "code",
|
||||
code_challenge: "test-challenge",
|
||||
code_challenge_method: "S256",
|
||||
state: "test-state",
|
||||
resource: "https://api.example.com/mcp"
|
||||
});
|
||||
|
||||
expect(consoleWarnSpy).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
||||
// Note: No silent mode test anymore - warnings are always enabled
|
||||
|
||||
describe("server URL validation (validateResourceMatchesServer: true)", () => {
|
||||
const serverValidationConfig: OAuthServerConfig = {
|
||||
serverUrl: "https://api.example.com/mcp",
|
||||
validateResourceMatchesServer: true
|
||||
};
|
||||
|
||||
beforeEach(() => {
|
||||
app.use("/oauth/authorize", authorizationHandler({
|
||||
provider: mockProvider,
|
||||
config: serverValidationConfig
|
||||
}));
|
||||
});
|
||||
|
||||
it("should accept requests when resource matches server URL", async () => {
|
||||
const mockClient = {
|
||||
client_id: "test-client",
|
||||
client_name: "Test Client",
|
||||
redirect_uris: ["https://example.com/callback"],
|
||||
grant_types: ["authorization_code"],
|
||||
response_types: ["code"],
|
||||
scope: "read write",
|
||||
token_endpoint_auth_method: "none",
|
||||
};
|
||||
|
||||
(mockProvider.clientsStore.getClient as jest.Mock).mockResolvedValue(mockClient);
|
||||
mockProvider.authorize.mockImplementation(async (client, params, res) => {
|
||||
res.redirect(`https://example.com/callback?code=auth-code&state=${params.state}`);
|
||||
});
|
||||
|
||||
const response = await request(app)
|
||||
.get("/oauth/authorize")
|
||||
.query({
|
||||
client_id: "test-client",
|
||||
redirect_uri: "https://example.com/callback",
|
||||
response_type: "code",
|
||||
code_challenge: "test-challenge",
|
||||
code_challenge_method: "S256",
|
||||
state: "test-state",
|
||||
resource: "https://api.example.com/mcp"
|
||||
});
|
||||
|
||||
expect(response.status).toBe(302);
|
||||
expect(response.headers.location).toContain("code=auth-code");
|
||||
});
|
||||
|
||||
it("should reject requests when resource does not match server URL", async () => {
|
||||
const mockClient = {
|
||||
client_id: "test-client",
|
||||
client_name: "Test Client",
|
||||
redirect_uris: ["https://example.com/callback"],
|
||||
grant_types: ["authorization_code"],
|
||||
response_types: ["code"],
|
||||
scope: "read write",
|
||||
token_endpoint_auth_method: "none",
|
||||
};
|
||||
|
||||
(mockProvider.clientsStore.getClient as jest.Mock).mockResolvedValue(mockClient);
|
||||
|
||||
const response = await request(app)
|
||||
.get("/oauth/authorize")
|
||||
.query({
|
||||
client_id: "test-client",
|
||||
redirect_uri: "https://example.com/callback",
|
||||
response_type: "code",
|
||||
code_challenge: "test-challenge",
|
||||
code_challenge_method: "S256",
|
||||
state: "test-state",
|
||||
resource: "https://different.api.com/mcp"
|
||||
});
|
||||
|
||||
expect(response.status).toBe(302);
|
||||
expect(response.headers.location).toContain("error=invalid_target");
|
||||
expect(response.headers.location).toContain("does+not+match+this+server");
|
||||
});
|
||||
|
||||
it("should reject requests without resource parameter when validation is enabled", async () => {
|
||||
const mockClient = {
|
||||
client_id: "test-client",
|
||||
client_name: "Test Client",
|
||||
redirect_uris: ["https://example.com/callback"],
|
||||
grant_types: ["authorization_code"],
|
||||
response_types: ["code"],
|
||||
scope: "read write",
|
||||
token_endpoint_auth_method: "none",
|
||||
};
|
||||
|
||||
(mockProvider.clientsStore.getClient as jest.Mock).mockResolvedValue(mockClient);
|
||||
|
||||
const response = await request(app)
|
||||
.get("/oauth/authorize")
|
||||
.query({
|
||||
client_id: "test-client",
|
||||
redirect_uri: "https://example.com/callback",
|
||||
response_type: "code",
|
||||
code_challenge: "test-challenge",
|
||||
code_challenge_method: "S256",
|
||||
state: "test-state"
|
||||
});
|
||||
|
||||
expect(response.status).toBe(302);
|
||||
expect(response.headers.location).toContain("error=invalid_request");
|
||||
expect(response.headers.location).toContain("Resource+parameter+is+required+when+server+URL+validation+is+enabled");
|
||||
});
|
||||
|
||||
it("should handle server URL with fragment correctly", async () => {
|
||||
// Reconfigure with a server URL that has a fragment (though it shouldn't)
|
||||
const configWithFragment: OAuthServerConfig = {
|
||||
serverUrl: "https://api.example.com/mcp#fragment",
|
||||
validateResourceMatchesServer: true
|
||||
};
|
||||
|
||||
app = express();
|
||||
app.use("/oauth/authorize", authorizationHandler({
|
||||
provider: mockProvider,
|
||||
config: configWithFragment
|
||||
}));
|
||||
|
||||
const mockClient = {
|
||||
client_id: "test-client",
|
||||
client_name: "Test Client",
|
||||
redirect_uris: ["https://example.com/callback"],
|
||||
grant_types: ["authorization_code"],
|
||||
response_types: ["code"],
|
||||
scope: "read write",
|
||||
token_endpoint_auth_method: "none",
|
||||
};
|
||||
|
||||
(mockProvider.clientsStore.getClient as jest.Mock).mockResolvedValue(mockClient);
|
||||
mockProvider.authorize.mockImplementation(async (client, params, res) => {
|
||||
res.redirect(`https://example.com/callback?code=auth-code&state=${params.state}`);
|
||||
});
|
||||
|
||||
const response = await request(app)
|
||||
.get("/oauth/authorize")
|
||||
.query({
|
||||
client_id: "test-client",
|
||||
redirect_uri: "https://example.com/callback",
|
||||
response_type: "code",
|
||||
code_challenge: "test-challenge",
|
||||
code_challenge_method: "S256",
|
||||
state: "test-state",
|
||||
resource: "https://api.example.com/mcp" // No fragment
|
||||
});
|
||||
|
||||
expect(response.status).toBe(302);
|
||||
expect(response.headers.location).toContain("code=auth-code");
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
@ -8,13 +8,10 @@ import {
|
|||
InvalidRequestError,
|
||||
InvalidClientError,
|
||||
InvalidScopeError,
|
||||
InvalidTargetError,
|
||||
ServerError,
|
||||
TooManyRequestsError,
|
||||
OAuthError
|
||||
} from "../errors.js";
|
||||
import { OAuthServerConfig } from "../types.js";
|
||||
import { resourceUrlFromServerUrl } from "../../../shared/auth-utils.js";
|
||||
|
||||
export type AuthorizationHandlerOptions = {
|
||||
provider: OAuthServerProvider;
|
||||
|
|
@ -23,10 +20,6 @@ export type AuthorizationHandlerOptions = {
|
|||
* Set to false to disable rate limiting for this endpoint.
|
||||
*/
|
||||
rateLimit?: Partial<RateLimitOptions> | false;
|
||||
/**
|
||||
* OAuth server configuration options
|
||||
*/
|
||||
config?: OAuthServerConfig;
|
||||
};
|
||||
|
||||
// Parameters that must be validated in order to issue redirects.
|
||||
|
|
@ -45,12 +38,7 @@ const RequestAuthorizationParamsSchema = z.object({
|
|||
resource: z.string().url().optional(),
|
||||
});
|
||||
|
||||
export function authorizationHandler({ provider, rateLimit: rateLimitConfig, config }: AuthorizationHandlerOptions): RequestHandler {
|
||||
// Validate configuration
|
||||
if (config?.validateResourceMatchesServer && !config?.serverUrl) {
|
||||
throw new Error("serverUrl must be configured when validateResourceMatchesServer is true");
|
||||
}
|
||||
|
||||
export function authorizationHandler({ provider, rateLimit: rateLimitConfig }: AuthorizationHandlerOptions): RequestHandler {
|
||||
// Create a router to apply middleware
|
||||
const router = express.Router();
|
||||
router.use(allowedMethods(["GET", "POST"]));
|
||||
|
|
@ -131,26 +119,8 @@ export function authorizationHandler({ provider, rateLimit: rateLimitConfig, con
|
|||
const { scope, code_challenge, resource } = parseResult.data;
|
||||
state = parseResult.data.state;
|
||||
|
||||
// If validateResourceMatchesServer is enabled, resource is required and must match
|
||||
if (config?.validateResourceMatchesServer) {
|
||||
if (!resource) {
|
||||
throw new InvalidRequestError("Resource parameter is required when server URL validation is enabled");
|
||||
}
|
||||
|
||||
// Remove fragment from server URL if present (though it shouldn't have one)
|
||||
const canonicalServerUrl = resourceUrlFromServerUrl(config.serverUrl!);
|
||||
|
||||
if (resource !== canonicalServerUrl) {
|
||||
throw new InvalidTargetError(
|
||||
`Resource parameter '${resource}' does not match this server's URL '${canonicalServerUrl}'`
|
||||
);
|
||||
}
|
||||
} else {
|
||||
// Always log warning if resource is missing (unless validation is enabled)
|
||||
if (!resource) {
|
||||
console.warn(`Authorization request from client ${client_id} is missing the resource parameter. Consider migrating to RFC 8707.`);
|
||||
}
|
||||
}
|
||||
// Pass through the resource parameter to the provider
|
||||
// The provider can decide how to validate it
|
||||
|
||||
// Validate scopes
|
||||
let requestedScopes: string[] = [];
|
||||
|
|
|
|||
|
|
@ -12,11 +12,8 @@ import {
|
|||
UnsupportedGrantTypeError,
|
||||
ServerError,
|
||||
TooManyRequestsError,
|
||||
OAuthError,
|
||||
InvalidTargetError
|
||||
OAuthError
|
||||
} from "../errors.js";
|
||||
import { OAuthServerConfig } from "../types.js";
|
||||
import { resourceUrlFromServerUrl } from "../../../shared/auth-utils.js";
|
||||
|
||||
export type TokenHandlerOptions = {
|
||||
provider: OAuthServerProvider;
|
||||
|
|
@ -25,10 +22,6 @@ export type TokenHandlerOptions = {
|
|||
* Set to false to disable rate limiting for this endpoint.
|
||||
*/
|
||||
rateLimit?: Partial<RateLimitOptions> | false;
|
||||
/**
|
||||
* OAuth server configuration options
|
||||
*/
|
||||
config?: OAuthServerConfig;
|
||||
};
|
||||
|
||||
const TokenRequestSchema = z.object({
|
||||
|
|
@ -48,12 +41,7 @@ const RefreshTokenGrantSchema = z.object({
|
|||
resource: z.string().url().optional(),
|
||||
});
|
||||
|
||||
export function tokenHandler({ provider, rateLimit: rateLimitConfig, config }: TokenHandlerOptions): RequestHandler {
|
||||
// Validate configuration
|
||||
if (config?.validateResourceMatchesServer && !config?.serverUrl) {
|
||||
throw new Error("serverUrl must be configured when validateResourceMatchesServer is true");
|
||||
}
|
||||
|
||||
export function tokenHandler({ provider, rateLimit: rateLimitConfig }: TokenHandlerOptions): RequestHandler {
|
||||
// Nested router so we can configure middleware and restrict HTTP method
|
||||
const router = express.Router();
|
||||
|
||||
|
|
@ -105,25 +93,8 @@ export function tokenHandler({ provider, rateLimit: rateLimitConfig, config }: T
|
|||
|
||||
const { code, code_verifier, redirect_uri, resource } = parseResult.data;
|
||||
|
||||
// If validateResourceMatchesServer is enabled, resource is required and must match
|
||||
if (config?.validateResourceMatchesServer) {
|
||||
if (!resource) {
|
||||
throw new InvalidRequestError("Resource parameter is required when server URL validation is enabled");
|
||||
}
|
||||
|
||||
const canonicalServerUrl = resourceUrlFromServerUrl(config.serverUrl!);
|
||||
|
||||
if (resource !== canonicalServerUrl) {
|
||||
throw new InvalidTargetError(
|
||||
`Resource parameter '${resource}' does not match this server's URL '${canonicalServerUrl}'`
|
||||
);
|
||||
}
|
||||
} else {
|
||||
// Always log warning if resource is missing (unless validation is enabled)
|
||||
if (!resource) {
|
||||
console.warn(`Token exchange request from client ${client.client_id} is missing the resource parameter. Consider migrating to RFC 8707.`);
|
||||
}
|
||||
}
|
||||
// Pass through the resource parameter to the provider
|
||||
// The provider can decide how to validate it
|
||||
|
||||
const skipLocalPkceValidation = provider.skipLocalPkceValidation;
|
||||
|
||||
|
|
@ -156,25 +127,8 @@ export function tokenHandler({ provider, rateLimit: rateLimitConfig, config }: T
|
|||
|
||||
const { refresh_token, scope, resource } = parseResult.data;
|
||||
|
||||
// If validateResourceMatchesServer is enabled, resource is required and must match
|
||||
if (config?.validateResourceMatchesServer) {
|
||||
if (!resource) {
|
||||
throw new InvalidRequestError("Resource parameter is required when server URL validation is enabled");
|
||||
}
|
||||
|
||||
const canonicalServerUrl = resourceUrlFromServerUrl(config.serverUrl!);
|
||||
|
||||
if (resource !== canonicalServerUrl) {
|
||||
throw new InvalidTargetError(
|
||||
`Resource parameter '${resource}' does not match this server's URL '${canonicalServerUrl}'`
|
||||
);
|
||||
}
|
||||
} else {
|
||||
// Always log warning if resource is missing (unless validation is enabled)
|
||||
if (!resource) {
|
||||
console.warn(`Token refresh request from client ${client.client_id} is missing the resource parameter. Consider migrating to RFC 8707.`);
|
||||
}
|
||||
}
|
||||
// Pass through the resource parameter to the provider
|
||||
// The provider can decide how to validate it
|
||||
|
||||
const scopes = scope?.split(" ");
|
||||
const tokens = await provider.exchangeRefreshToken(client, refresh_token, scopes, resource);
|
||||
|
|
|
|||
|
|
@ -27,37 +27,4 @@ export interface AuthInfo {
|
|||
* This field should be used for any additional data that needs to be attached to the auth info.
|
||||
*/
|
||||
extra?: Record<string, unknown>;
|
||||
}
|
||||
|
||||
/**
|
||||
* Configuration options for OAuth server behavior
|
||||
*/
|
||||
export interface OAuthServerConfig {
|
||||
/**
|
||||
* The canonical URL of this MCP server. When provided, the server will validate
|
||||
* that the resource parameter in OAuth requests matches this URL.
|
||||
*
|
||||
* This should be the full URL that clients use to connect to this server,
|
||||
* without any fragment component (e.g., "https://api.example.com/mcp").
|
||||
*
|
||||
* Required when validateResourceMatchesServer is true.
|
||||
*/
|
||||
serverUrl?: string;
|
||||
|
||||
/**
|
||||
* If true, validates that the resource parameter matches the configured serverUrl.
|
||||
*
|
||||
* When enabled:
|
||||
* - serverUrl must be configured (throws error if not)
|
||||
* - resource parameter is required on all requests
|
||||
* - resource must exactly match serverUrl (after fragment removal)
|
||||
* - requests without resource parameter will be rejected with invalid_request error
|
||||
* - requests with non-matching resource will be rejected with invalid_target error
|
||||
*
|
||||
* When disabled:
|
||||
* - warnings are logged when resource parameter is missing (for migration tracking)
|
||||
*
|
||||
* @default false
|
||||
*/
|
||||
validateResourceMatchesServer?: boolean;
|
||||
}
|
||||
Loading…
Reference in a new issue