refactor: remove unsafe non-null assertions / double-cast in auth paths
Address the TypeScript-safety review items on the mid-session auth work: - oauthManager: replace the unguarded `this.oauthConfig.storage!` assertions on the EMA re-mint/persist paths with a `requireStorage()` helper that throws a clear error; in `getOAuthState` reuse the already-narrowed local `storage`. - cli: `parseRunnerOAuthCallbackUrl` always returns a config, so make `callMethod`'s `callbackUrlConfig` (and the always-passed `cliAuthOverrides`) required instead of asserting them non-null at three call sites. - test-servers/resolve-config: drop the `as unknown as T` double cast; collect into a base-shaped array and do a single honest downcast at the return. No behavior change; validate + coverage green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DZyeWManYTvcXxjcrhxSQc
This commit is contained in:
parent
74c6491b52
commit
14404bc405
3 changed files with 29 additions and 14 deletions
|
|
@ -71,8 +71,8 @@ async function callMethod(
|
||||||
serverSettings: InspectorServerSettings | undefined,
|
serverSettings: InspectorServerSettings | undefined,
|
||||||
args: MethodArgs & { method: string },
|
args: MethodArgs & { method: string },
|
||||||
clientConfig: ClientConfig,
|
clientConfig: ClientConfig,
|
||||||
cliAuthOverrides?: RunnerClientConfigOverrides,
|
cliAuthOverrides: RunnerClientConfigOverrides,
|
||||||
callbackUrlConfig?: RunnerOAuthCallbackConfig,
|
callbackUrlConfig: RunnerOAuthCallbackConfig,
|
||||||
): Promise<void> {
|
): Promise<void> {
|
||||||
const __dirname = dirname(fileURLToPath(import.meta.url));
|
const __dirname = dirname(fileURLToPath(import.meta.url));
|
||||||
const packageJsonPath = join(__dirname, "../package.json");
|
const packageJsonPath = join(__dirname, "../package.json");
|
||||||
|
|
@ -93,9 +93,8 @@ async function callMethod(
|
||||||
};
|
};
|
||||||
const redirectUrlProvider = new MutableRedirectUrlProvider();
|
const redirectUrlProvider = new MutableRedirectUrlProvider();
|
||||||
if (isOAuthCapableServerConfig(serverConfig)) {
|
if (isOAuthCapableServerConfig(serverConfig)) {
|
||||||
redirectUrlProvider.redirectUrl = formatRunnerOAuthRedirectUrl(
|
redirectUrlProvider.redirectUrl =
|
||||||
callbackUrlConfig!,
|
formatRunnerOAuthRedirectUrl(callbackUrlConfig);
|
||||||
);
|
|
||||||
environment.oauth = {
|
environment.oauth = {
|
||||||
storage: new NodeOAuthStorage(),
|
storage: new NodeOAuthStorage(),
|
||||||
navigation: new ConsoleNavigation(),
|
navigation: new ConsoleNavigation(),
|
||||||
|
|
@ -258,14 +257,14 @@ async function callMethod(
|
||||||
inspectorClient,
|
inspectorClient,
|
||||||
serverConfig,
|
serverConfig,
|
||||||
redirectUrlProvider,
|
redirectUrlProvider,
|
||||||
callbackUrlConfig!,
|
callbackUrlConfig,
|
||||||
serverSettings,
|
serverSettings,
|
||||||
);
|
);
|
||||||
|
|
||||||
const result = await withCliAuthRecoveryRetry(
|
const result = await withCliAuthRecoveryRetry(
|
||||||
inspectorClient,
|
inspectorClient,
|
||||||
redirectUrlProvider,
|
redirectUrlProvider,
|
||||||
callbackUrlConfig!,
|
callbackUrlConfig,
|
||||||
serverSettings,
|
serverSettings,
|
||||||
runMethod,
|
runMethod,
|
||||||
);
|
);
|
||||||
|
|
|
||||||
|
|
@ -10,6 +10,7 @@ import { EMPTY_OAUTH_FLOW_STATE } from "../auth/types.js";
|
||||||
import type { OAuthTokens } from "@modelcontextprotocol/sdk/shared/auth.js";
|
import type { OAuthTokens } from "@modelcontextprotocol/sdk/shared/auth.js";
|
||||||
import type { OAuthClientInformation } from "@modelcontextprotocol/sdk/shared/auth.js";
|
import type { OAuthClientInformation } from "@modelcontextprotocol/sdk/shared/auth.js";
|
||||||
import { mcpAuth } from "../auth/mcpAuth.js";
|
import { mcpAuth } from "../auth/mcpAuth.js";
|
||||||
|
import type { OAuthStorage } from "../auth/storage.js";
|
||||||
import { parseOAuthState } from "../auth/utils.js";
|
import { parseOAuthState } from "../auth/utils.js";
|
||||||
import type { EnterpriseManagedAuthIdpConfig } from "../client/types.js";
|
import type { EnterpriseManagedAuthIdpConfig } from "../client/types.js";
|
||||||
import type { ClientConfig } from "../client/types.js";
|
import type { ClientConfig } from "../client/types.js";
|
||||||
|
|
@ -104,6 +105,19 @@ export class OAuthManager {
|
||||||
return this.params.getServerUrl();
|
return this.params.getServerUrl();
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Return the configured OAuth storage, throwing a clear error if it is
|
||||||
|
* absent. Used on the EMA re-mint/persist paths, which are only reached for
|
||||||
|
* a fully-configured OAuth-capable server (storage present).
|
||||||
|
*/
|
||||||
|
private requireStorage(): OAuthStorage {
|
||||||
|
const storage = this.oauthConfig.storage;
|
||||||
|
if (!storage) {
|
||||||
|
throw new Error("OAuth storage is required for this operation.");
|
||||||
|
}
|
||||||
|
return storage;
|
||||||
|
}
|
||||||
|
|
||||||
private async createOAuthProvider(): Promise<BaseOAuthClientProvider> {
|
private async createOAuthProvider(): Promise<BaseOAuthClientProvider> {
|
||||||
if (
|
if (
|
||||||
!this.oauthConfig.storage ||
|
!this.oauthConfig.storage ||
|
||||||
|
|
@ -284,7 +298,7 @@ export class OAuthManager {
|
||||||
requestedScope,
|
requestedScope,
|
||||||
);
|
);
|
||||||
if (scopeToPersist) {
|
if (scopeToPersist) {
|
||||||
await this.oauthConfig.storage!.saveScope(
|
await this.requireStorage().saveScope(
|
||||||
this.getServerUrl(),
|
this.getServerUrl(),
|
||||||
scopeToPersist,
|
scopeToPersist,
|
||||||
);
|
);
|
||||||
|
|
@ -422,7 +436,7 @@ export class OAuthManager {
|
||||||
protocol: protocolFromOAuthConfig(this.oauthConfig),
|
protocol: protocolFromOAuthConfig(this.oauthConfig),
|
||||||
configuredScope: this.oauthConfig.scope,
|
configuredScope: this.oauthConfig.scope,
|
||||||
enterpriseManagedAuth: this.params.enterpriseManagedAuth,
|
enterpriseManagedAuth: this.params.enterpriseManagedAuth,
|
||||||
storage: this.oauthConfig.storage!,
|
storage,
|
||||||
flowState: this.oauthFlowState ?? undefined,
|
flowState: this.oauthFlowState ?? undefined,
|
||||||
});
|
});
|
||||||
}
|
}
|
||||||
|
|
@ -618,7 +632,7 @@ export class OAuthManager {
|
||||||
const silent = await trySilentEmaAuth(config);
|
const silent = await trySilentEmaAuth(config);
|
||||||
if (silent.status === "success") {
|
if (silent.status === "success") {
|
||||||
if (await this.checkAuthChallengeSatisfied(enriched)) {
|
if (await this.checkAuthChallengeSatisfied(enriched)) {
|
||||||
const minted = await this.oauthConfig.storage!.getTokens(
|
const minted = await this.requireStorage().getTokens(
|
||||||
this.getServerUrl(),
|
this.getServerUrl(),
|
||||||
);
|
);
|
||||||
const scopeToPersist = resolvePersistedScopeAfterGrant(
|
const scopeToPersist = resolvePersistedScopeAfterGrant(
|
||||||
|
|
@ -626,7 +640,7 @@ export class OAuthManager {
|
||||||
enriched.authorizationScopes?.join(" "),
|
enriched.authorizationScopes?.join(" "),
|
||||||
);
|
);
|
||||||
if (scopeToPersist) {
|
if (scopeToPersist) {
|
||||||
await this.oauthConfig.storage!.saveScope(
|
await this.requireStorage().saveScope(
|
||||||
this.getServerUrl(),
|
this.getServerUrl(),
|
||||||
scopeToPersist,
|
scopeToPersist,
|
||||||
);
|
);
|
||||||
|
|
|
||||||
|
|
@ -29,7 +29,7 @@ function resolvePresetRefs<T extends { requiredScopes?: string[] }>(
|
||||||
type: "tool" | "resource" | "resourceTemplate" | "prompt",
|
type: "tool" | "resource" | "resourceTemplate" | "prompt",
|
||||||
): T[] {
|
): T[] {
|
||||||
if (!refs || refs.length === 0) return [];
|
if (!refs || refs.length === 0) return [];
|
||||||
const result: T[] = [];
|
const result: Array<{ requiredScopes?: string[] }> = [];
|
||||||
for (const entry of refs) {
|
for (const entry of refs) {
|
||||||
const items = Array.isArray(entry) ? entry : [entry];
|
const items = Array.isArray(entry) ? entry : [entry];
|
||||||
for (const ref of items) {
|
for (const ref of items) {
|
||||||
|
|
@ -42,11 +42,13 @@ function resolvePresetRefs<T extends { requiredScopes?: string[] }>(
|
||||||
const resolved = resolvePreset(type, presetName, ref.params);
|
const resolved = resolvePreset(type, presetName, ref.params);
|
||||||
const arr = Array.isArray(resolved) ? resolved : [resolved];
|
const arr = Array.isArray(resolved) ? resolved : [resolved];
|
||||||
for (const item of arr) {
|
for (const item of arr) {
|
||||||
result.push(mergeRequiredScopes(item as unknown as T, ref));
|
result.push(mergeRequiredScopes(item, ref));
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
return result;
|
// The caller pairs `type` with the matching `T`, so each resolved preset is
|
||||||
|
// the requested definition kind; TS can't see that correspondence.
|
||||||
|
return result as T[];
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue