[geoffg-sentry]

Follow MCP and OAuth specs and perform resource parameter validation during token issuance and token acceptance

[dcramer]

⏺ Based on my review of PR #608, I've identified several issues ranging from RFC violations to security concerns and bugs. Here's my comprehensive analysis:

  🔴 Critical Issues

  1. RFC 8707 Violation: Fragment Component Not Rejected

  Location: packages/mcp-cloudflare/src/server/oauth/helpers.ts - validateResourceParameter()

  RFC 8707 explicitly states: "The URI MUST NOT include a fragment component."

  The current implementation does not check for or reject fragment components:
  // Current code doesn't reject this:
  validateResourceParameter("https://mcp.sentry.dev/mcp#fragment", requestUrl)
  // Returns: true ❌ Should return: false

  Fix: Add validation to reject URIs with fragments:
  if (resourceUrl.hash) {
    return false;
  }

  2. Port Comparison Bug: Default Ports Not Handled

  Location: packages/mcp-cloudflare/src/server/oauth/helpers.ts:328

  The port comparison fails for default ports:
  if (resourceUrl.port !== requestUrlObj.port) {
    return false;
  }

  Issue:
  - https://example.com has port = ""
  - https://example.com:443 has port = "443"
  - These should be equivalent but are rejected as different

  Impact: Valid resource parameters will be incorrectly rejected when explicit default ports are used.

  Fix: Normalize ports before comparison:
  const getEffectivePort = (url: URL) => {
    if (url.port) return url.port;
    return url.protocol === "https:" ? "443" : "80";
  };

  if (getEffectivePort(resourceUrl) !== getEffectivePort(requestUrlObj)) {
    return false;
  }

  3. Protocol Not Validated

  Location: packages/mcp-cloudflare/src/server/oauth/helpers.ts - validateResourceParameter()

  The function doesn't validate that the resource uses the same protocol as the request:
  // Current code allows this:
  validateResourceParameter("http://localhost/mcp", "https://localhost/oauth/authorize")
  // Returns: true ❌ (if ports match)

  Security Impact: Mixed-protocol scenarios could introduce security issues (http resource with https request).

  Fix: Add protocol validation:
  if (resourceUrl.protocol !== requestUrlObj.protocol) {
    return false;
  }

  🟡 Type Safety Issues

  4. Unsafe Type Casting

  Location: Multiple files - authorize.ts:lines 35-36, callback.ts:line 19

  // In authorize.ts POST handler:
  if (!validateResourceParameter(
    oauthReqWithPermissions.resource as string,  // ❌ Unsafe cast
    requestUrl,
  ))

  // In callback.ts:
  if (!validateResourceParameter(oauthReqInfo.resource as string, requestUrl)) // ❌ Unsafe cast

  Issue: The as string cast bypasses TypeScript's type checking. If resource is not a string, it will cause runtime errors in new URL(resource).

  Fix: Use proper type guards:
  if (
    typeof oauthReqWithPermissions.resource === "string" &&
    !validateResourceParameter(oauthReqWithPermissions.resource, requestUrl)
  )

  🟢 Minor Issues

  5. Incomplete Test Coverage for Edge Cases

  Location: packages/mcp-cloudflare/src/server/oauth/helpers.test.ts

  Tests don't cover:
  - Fragment components (which should be rejected per RFC 8707)
  - Query parameters in resource (which are allowed but discouraged)
  - IPv6 addresses
  - Mixed protocols (http vs https)

  6. Error Message Could Be More Helpful

  Location: packages/mcp-cloudflare/src/server/oauth/helpers.ts:356

  error_description: "The resource parameter does not match this authorization server"

  Issue: Doesn't explain what format is expected or why it failed.

  Suggestion: Be more specific:
  error_description: "The resource parameter must match the authorization server's hostname and use /mcp path prefix"

  7. Code Organization: Logic Order

  Location: packages/mcp-cloudflare/src/server/oauth/helpers.ts:313-317

  if (resource === "") {
    return false;
  }

  if (!resource) {
    return true;
  }

  Issue: Checking empty string before undefined is unusual. More conventional to check undefined/null first.

  Suggestion: Reorder for clarity:
  if (!resource) {
    return true;  // Optional parameter
  }

  if (resource === "") {
    return false;  // Explicit empty string is invalid
  }

  ✅ Positive Aspects

  1. Correct error code usage - Uses invalid_target as specified in RFC 8707
  2. Good test coverage structure - Comprehensive test suites for both authorize and callback flows
  3. Proper state preservation - State parameter correctly preserved in error responses
  4. Backward compatibility - Allows undefined resource parameter for existing clients
  5. Security-conscious - Validates hostname and path to prevent SSRF attacks

  Recommendations

  1. Fix the critical RFC violation - Reject fragment components
  2. Fix the port comparison bug - Handle default ports correctly
  3. Add protocol validation - Ensure protocol consistency
  4. Remove unsafe type casts - Use proper type guards
  5. Add missing test cases - Cover edge cases like fragments, default ports, and protocol mismatches
  6. Consider documenting the validation rules - Add JSDoc comments explaining what makes a valid resource parameter

  The PR is implementing an important security feature, but these issues should be addressed before merging to ensure RFC compliance and prevent bugs in production.

[dcramer]