Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@missionsquad/mcp-api",
"version": "1.11.1",
"version": "1.11.2",
"description": "MCP Servers exposed via HTTP API",
"main": "dist/index.js",
"repository": "missionsquad/mcp-api",
Expand Down
78 changes: 78 additions & 0 deletions src/services/mcp.ts
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,11 @@ export type McpServerSource = 'platform' | 'external'
export type McpServerAuthMode = 'none' | 'oauth2'
export type McpExternalOAuthDiscoverySource = 'prm' | 'issuer_override'

export interface McpExternalOAuthAuthorizationRequestParam {
name: string
value: string
}

export interface McpExternalSecretField {
name: string
label: string
Expand All @@ -97,6 +102,7 @@ export interface McpExternalOAuthTemplate {
clientIdMetadataDocumentSupported?: boolean
registrationEndpoint?: string
tokenEndpointAuthMethodsSupported?: SupportedTokenEndpointAuthMethod[]
authorizationRequestParams?: McpExternalOAuthAuthorizationRequestParam[]
}

export interface DiscoverExternalAuthorizationInput {
Expand Down Expand Up @@ -422,6 +428,17 @@ const DISCOVERY_BASE_DELAY_MS = 500
const DISCOVERY_TOTAL_BUDGET_MS = 20000
const REQUIRED_PKCE_CHALLENGE_METHOD = 'S256'
const RESOURCE_URI_BACKFILL_FAILURE_COOLDOWN_MS = 15 * 60 * 1000
const OAUTH_AUTHORIZATION_REQUEST_PARAM_NAME_PATTERN = /^[A-Za-z_][A-Za-z0-9_]{0,99}$/
const RESERVED_OAUTH_AUTHORIZATION_REQUEST_PARAM_NAMES = new Set([
'response_type',
'client_id',
'redirect_uri',
'state',
'scope',
'resource',
'code_challenge',
'code_challenge_method'
])

const isRecord = (value: unknown): value is Record<string, unknown> =>
value !== null && typeof value === 'object' && !Array.isArray(value)
Expand All @@ -440,6 +457,59 @@ const toOptionalStringArray = (value: unknown): string[] | undefined => {
const getOptionalString = (value: unknown): string | undefined =>
typeof value === 'string' ? value : undefined

const normalizeAuthorizationRequestParamEntries = (
value: unknown
): McpExternalOAuthAuthorizationRequestParam[] | undefined => {
if (value === undefined) {
return undefined
}
if (!Array.isArray(value)) {
throw new McpValidationError('oauthTemplate.authorizationRequestParams must be an array when provided')
}

const normalized = value.map((item, index) => {
if (!isRecord(item)) {
throw new McpValidationError(
`oauthTemplate.authorizationRequestParams[${index}] must be an object with name and value`
)
}

const name = getOptionalString(item.name)?.trim()
const value = getOptionalString(item.value)?.trim()

Comment on lines +476 to +479
Copy link

Copilot AI Mar 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The local const value = ... inside the map callback shadows the value parameter of normalizeAuthorizationRequestParamEntries, which makes the code harder to read and increases the chance of mistakes during future edits. Renaming the inner variable (e.g., paramValue) would avoid the shadowing.

Copilot uses AI. Check for mistakes.
if (!name) {
throw new McpValidationError(`oauthTemplate.authorizationRequestParams[${index}].name is required`)
}
if (!OAUTH_AUTHORIZATION_REQUEST_PARAM_NAME_PATTERN.test(name)) {
throw new McpValidationError(
`oauthTemplate.authorizationRequestParams[${index}].name is invalid: ${name}`
)
}
if (RESERVED_OAUTH_AUTHORIZATION_REQUEST_PARAM_NAMES.has(name)) {
throw new McpValidationError(
`oauthTemplate.authorizationRequestParams[${index}].name must not override reserved OAuth parameter ${name}`
)
}
if (!value) {
throw new McpValidationError(`oauthTemplate.authorizationRequestParams[${index}].value is required`)
}
Comment on lines +477 to +495
Copy link

Copilot AI Mar 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inside normalizeAuthorizationRequestParamEntries, non-string name/value inputs get reported as "is required" because getOptionalString() returns undefined. This makes the validation error misleading when the field is present but the type is wrong (e.g., number/boolean). Consider explicitly validating the types and throwing a "must be a string" error for incorrect types (and reserving "is required" for missing/empty strings).

Copilot uses AI. Check for mistakes.

return { name, value }
})

const seenNames = new Set<string>()
for (const entry of normalized) {
if (seenNames.has(entry.name)) {
throw new McpValidationError(
`oauthTemplate.authorizationRequestParams contains duplicate parameter name ${entry.name}`
)
}
seenNames.add(entry.name)
}

return normalized.length > 0 ? normalized : undefined
}
Comment on lines +460 to +511
Copy link

Copilot AI Mar 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New behavior was added to normalize/validate oauthTemplate.authorizationRequestParams (name pattern, reserved-name blocking, trimming, empty-array -> undefined, duplicate detection), but there are no corresponding tests. The repo already has coverage for OAuth template validation in test/mcp-external-auth.spec.ts; adding test cases for these new rules would help prevent regressions and clarify the intended constraints.

Copilot uses AI. Check for mistakes.

export const canonicalizeExternalOAuthResourceUri = (input: string): string => {
const parsed = new URL(input)
parsed.search = ''
Expand Down Expand Up @@ -1723,6 +1793,7 @@ export class MCPService implements Resource {
if (!oauthTemplate.resourceUri) {
throw new McpValidationError('oauthTemplate.resourceUri is required for external OAuth servers')
}
normalizeAuthorizationRequestParamEntries(oauthTemplate.authorizationRequestParams)
Copy link

Copilot AI Mar 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

validateExternalOAuthTemplate() calls normalizeAuthorizationRequestParamEntries() purely for validation, but normalizeExternalOAuthTemplateForPersistence() already normalizes (and validates) authorizationRequestParams and then calls validateExternalOAuthTemplate() with the normalized template. This leads to duplicated work and two separate sources of truth for this validation. Consider normalizing/validating once (e.g., have validateExternalOAuthTemplate accept the already-normalized params, or have normalizeExternalOAuthTemplateForPersistence rely on validateExternalOAuthTemplate for this check).

Suggested change
normalizeAuthorizationRequestParamEntries(oauthTemplate.authorizationRequestParams)

Copilot uses AI. Check for mistakes.

if (oauthTemplate.pkceRequired !== true) {
throw new McpValidationError('oauthTemplate.pkceRequired must be true for external OAuth servers')
Expand Down Expand Up @@ -1820,6 +1891,10 @@ export class MCPService implements Resource {
throw new McpValidationError('oauthTemplate is required when authMode is oauth2')
}

const normalizedAuthorizationRequestParams = normalizeAuthorizationRequestParamEntries(
oauthTemplate.authorizationRequestParams
)

if (oauthTemplate.discoveryMode === 'auto') {
const normalizedDiscoverySource = oauthTemplate.discoverySource ?? 'prm'
this.assertAbsoluteUrl(
Expand All @@ -1846,6 +1921,9 @@ export class MCPService implements Resource {

const normalizedTemplate: McpExternalOAuthTemplate = {
...oauthTemplate,
...(normalizedAuthorizationRequestParams
? { authorizationRequestParams: normalizedAuthorizationRequestParams }
: { authorizationRequestParams: undefined }),
...(oauthTemplate.discoveryMode === 'auto' && !oauthTemplate.discoverySource
? { discoverySource: 'prm' as const }
: {}),
Expand Down
Loading