Skip to content

feat: Add session support for mcp#17

Open
jawad-khan wants to merge 1 commit into
foss-sandboxfrom
jawad/mcp-auth-fixes
Open

feat: Add session support for mcp#17
jawad-khan wants to merge 1 commit into
foss-sandboxfrom
jawad/mcp-auth-fixes

Conversation

@jawad-khan

Copy link
Copy Markdown

Description

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • Feature (non-breaking change which adds functionality)
  • Improvement (change that would cause existing functionality to not work as expected)
  • Code refactoring
  • Performance improvements
  • Documentation update

Screenshots and Media (if applicable)

Test Scenarios

References

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR adds session-based authentication support to the public v1 API layer by updating the shared base API view classes to accept session cookies in addition to API keys.

Changes:

  • Add BaseSessionAuthentication to BaseAPIView.authentication_classes
  • Add BaseSessionAuthentication to BaseViewSet.authentication_classes

Comment on lines 51 to 53
class BaseAPIView(TimezoneMixin, GenericAPIView, ReadReplicaControlMixin, BasePaginator):
authentication_classes = [APIKeyAuthentication]
authentication_classes = [BaseSessionAuthentication, APIKeyAuthentication]

Comment on lines 168 to 172
class BaseViewSet(TimezoneMixin, ReadReplicaControlMixin, ModelViewSet, BasePaginator):
model = None

authentication_classes = [APIKeyAuthentication]
authentication_classes = [BaseSessionAuthentication, APIKeyAuthentication]
permission_classes = [
@jawad-khan jawad-khan changed the base branch from foss-main to foss-sandbox June 1, 2026 10:54
@jawad-khan jawad-khan force-pushed the jawad/mcp-auth-fixes branch from 387df1d to 09c971f Compare June 1, 2026 10:56

@hunzlahmalik hunzlahmalik left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Review — session auth on the public REST API

This three-line change is the required keystone for the plane-mcp Cognito flow (Pressingly/plane-mcp-server#2 + Pressingly/foss-server-bundle#86): the MCP forwards a Cognito ID token → Traefik → oauth2-proxy sets X-Auth-Request-UserProxyAuthMiddleware authenticates the Django user → DRF needs a session auth class to accept that user on /api/v1/. Without it, every REST tool 401s. Correct and necessary.

🟡 Medium

  • Widens the CSRF-exempt cookie surface to the public REST API (inline). BaseSessionAuthentication.enforce_csrf is a hard no-op, so /api/v1/ now accepts any valid browser session cookie with CSRF disabled. The only barrier to cross-site write forgery is SameSite cookie policy + the mPass edge. This is consistent with the existing fork (plane/app/views/base.py, license/api, space/views already use this class), so you're extending an established posture — but please (a) confirm the Plane session and _oauth2_proxy cookies are SameSite=Lax/Strict in this deployment, and (b) add a one-line note to the PR body acknowledging the change.

🟢 Low

  • Empty PR description. The template is still unfilled for a change touching the auth surface of every REST endpoint. Add what/why + the SameSite confirmation.

Ordering is right: [BaseSessionAuthentication, APIKeyAuthentication] tries session first and falls back to PAT, so existing API-key clients are unaffected.


class BaseAPIView(TimezoneMixin, GenericAPIView, ReadReplicaControlMixin, BasePaginator):
authentication_classes = [APIKeyAuthentication]
authentication_classes = [BaseSessionAuthentication, APIKeyAuthentication]

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Medium (security). This brings the public /api/v1/ REST surface under BaseSessionAuthentication, whose enforce_csrf is a no-op ("Disable csrf for the rest apis" in plane/authentication/session.py). So any valid browser session cookie can now drive state-changing REST calls with no CSRF check — mitigated only by SameSite cookie policy + the mPass ForwardAuth edge.

Required for the plane-mcp forwarded-ID-token → oauth2-proxy → session flow, and consistent with plane/app/views/base.py / license / space, which already use this class. Please confirm the session + _oauth2_proxy cookies are SameSite=Lax/Strict here, and acknowledge the widened surface in the PR description.

model = None

authentication_classes = [APIKeyAuthentication]
authentication_classes = [BaseSessionAuthentication, APIKeyAuthentication]

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Same CSRF-exempt note as BaseAPIView above — BaseViewSet now also accepts session cookies on /api/v1/ writes with CSRF disabled.

@UsamaSadiq

Copy link
Copy Markdown
Collaborator

Review: CSRF Protection Concern

Problem

BaseSessionAuthentication overrides enforce_csrf to a no-op, and this PR adds it to BaseAPIView / BaseViewSet — the base classes for every /api/v1/ endpoint. This globally disables CSRF protection on the entire external API surface.

Any logged-in browser session can now be exploited by a cross-site attacker (POST/PUT/DELETE to any endpoint without a CSRF token).

Why this PR is unnecessary for MCP

The MCP server (plane-mcp-server) authenticates via:

  1. API KeyX-Api-Key header (already handled by APIKeyAuthentication)
  2. OAuth Bearer tokenAuthorization: Bearer <token>

Neither uses Django session cookies. CSRF is irrelevant for token-based auth because a cross-site attacker cannot inject custom headers. The MCP is a server-side process — it doesn't share a browser session.

Root cause

The MCP's OAuth path sends Authorization: Bearer <cognito_id_token> to /api/v1/. The current BaseAPIView only has APIKeyAuthentication (which reads X-Api-Key), so Bearer-authenticated requests get 401'd. Adding session auth was a workaround that happens to "work" if a browser cookie is present, but it doesn't match what the MCP actually sends.

Recommended fix: Add a BearerTokenAuthentication class

# plane/api/middleware/api_authentication.py

class BearerTokenAuthentication(authentication.BaseAuthentication):
    """
    Authenticate MCP/external callers via Authorization: Bearer <token>.
    Validates the token against the OIDC provider (Cognito) or Plane's
    OAuth token store.
    """

    def authenticate(self, request):
        auth_header = request.headers.get("Authorization", "")
        if not auth_header.startswith("Bearer "):
            return None  # Not our auth method — let next authenticator try

        token = auth_header[7:]
        user = self._validate_token(token)
        if user is None:
            raise AuthenticationFailed("Invalid or expired Bearer token")
        return (user, token)

    def _validate_token(self, token):
        # Option A: Validate as Cognito JWT (verify signature against JWKS,
        #           check exp/iss/aud, map sub → Plane user)
        # Option B: Look up token in Plane's OAuthToken store
        #           (if Plane-native OAuth is used)
        ...

Then wire it in:

class BaseAPIView(TimezoneMixin, GenericAPIView, ...):
    authentication_classes = [BearerTokenAuthentication, APIKeyAuthentication]

Comparison

PR #17 (session auth, no CSRF) Bearer token auth
CSRF protection Disabled globally Untouched
Attack surface Every endpoint vulnerable to cross-site session riding No change
Matches MCP's actual auth No — MCP sends Bearer tokens, not cookies Yes
Internal/external API boundary Blurred — external API now accepts browser sessions Clean separation

Minimum viable alternative

If Bearer token validation is too much work right now, scope the CSRF exemption to only the endpoints the MCP actually calls (via @csrf_exempt on individual views or a dedicated URL prefix), rather than disabling it on every API base class.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants