Skip to content
Closed
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
45 changes: 45 additions & 0 deletions .changeset/nine-cups-approve.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
---
'gatsby-plugin-clerk': minor
'@clerk/clerk-sdk-node': minor
'@clerk/backend': minor
'@clerk/fastify': minor
'@clerk/nextjs': minor
'@clerk/remix': minor
---

Introduce a new `RequestAdapter` interface that may be implemented by any framework that uses the `@clerk/backend` package. A custom `RequestAdapter` instance will be passed to `authenticateRequest` instead of each header/cookie/searchParams as a prop.

Example usage:

```ts
// CustomRequestAdapter.ts
// How we access the headers/cookies/searchParams depends on the specific framework's request representation
export class CustomRequestAdapter implements RequestAdapter {
constructor(readonly req: CustomRequest) {}

headers(key: string) {
return this.req?.headers?.[key];
}
cookies(key: string) {
return this.req?.cookies?.[key];
}
searchParams() {
return new URL(this.req.url).searchParams;
}
}
```

```ts
// FrameworkClerkMiddleware.ts
...
export const withClerkMiddleware = (options: ClerkCustomOptions) => {
return async (req: CustomFrameworkRequest, ...) => {
const requestState = await clerkClient.authenticateRequest({
...options,
secretKey,
publishableKey,
requestAdapter: new CustomRequestAdapter(req),
});
...
};
```
8 changes: 8 additions & 0 deletions .changeset/pink-carpets-matter.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
---
'@clerk/clerk-sdk-node': patch
'@clerk/backend': patch
---

- One pair of legacy or new instance keys are required now and not all 4 of them
- `@clerk/backend` now can handle the `"Bearer "` prefix in Authorization header for better DX
- `host` parameter is now optional in `@clerk/backend`
2 changes: 2 additions & 0 deletions packages/backend/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ import { createBackendApiClient } from './api';
import type { CreateAuthenticateRequestOptions } from './tokens';
import { createAuthenticateRequest } from './tokens';

export type { InstanceKeys, RequestAdapter } from './tokens';

export * from './api/resources';
export * from './tokens';
export * from './tokens/jwt';
Expand Down
1 change: 1 addition & 0 deletions packages/backend/src/tokens/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,3 +8,4 @@ export {
OptionalVerifyTokenOptions,
RequiredVerifyTokenOptions,
} from './request';
export type { InstanceKeys, RequestAdapter } from './request';
116 changes: 106 additions & 10 deletions packages/backend/src/tokens/request.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { API_URL, API_VERSION } from '../constants';
import { API_URL, API_VERSION, constants } from '../constants';
import { assertValidSecretKey } from '../util/assertValidSecretKey';
import { isDevelopmentFromApiKey } from '../util/instance';
import { parsePublishableKey } from '../util/parsePublishableKey';
Expand All @@ -23,6 +23,31 @@ import {
} from './interstitialRule';
import type { VerifyTokenOptions } from './verify';

export interface RequestAdapter {
/**
* The headers method returns the value of the specified header. Keys given to this method are
* normalized to lowercase, as they exist in `packages/backend/src/constants.ts`.
* Implementing this method requires the user to manipulate the header keys accordingly.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Having the caller re-implement the headers spec is risky. Small discrepancies can cause serious issues that are very hard to debug when deployed.

Also, we've been bitten by runtimes that don't respect the spec (Vercel runtime for edge API routes, it was fixed about 8 months ago by Vercel).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@nikosdouvlis We did not change the implementation for our supported frameworks. We just moved the logic inside the RequestAdapter.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sorry, I'm still a bit confused. I'm referring to this comment:

Implementing this method requires the user to manipulate the header keys accordingly

We can use clerk/backend to convert a Record<string,string> to a Headers instance without doing that manually. Is this not the preferred approach? Otherwise the method caller needs to reimplement Headers themselves

*
* @param key The header name to retrieve
*/
headers(key: string): string | undefined;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm somewhat concerned with the proposed API.

  1. Why did we use our own interface for headers here? Is there a reason we cannot use Headers?
  2. The naming confuses me. I'd expect headers to be an object/instance, not a function that returns a string. I'd expect something along the lines of headers.get (see point 1)
  3. How can we write a header? This is necessary for the loop detection mechanism

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I thought we'd be doing the following:

  • clerk/backend polyfills fetch and Headers for all runtimes
  • clerk/backend exposes a util to convert an object into a Headers instance
  • clerk/backend exposes an interface requiring at least a headers instance to be present.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@nikosdouvlis

Why did we use our own interface for headers here? Is there a reason we cannot use Headers?

We want to give full control to each framework. @clerk/backend package only passes a header key and the implementation is totally up to each framework.

The naming confuses me. I'd expect headers to be an object/instance, not a function that returns a string. I'd expect something along the lines of headers.get (see point 1)

I agree with the naming, we could rename to something like getHeader.

How can we write a header? This is necessary for the loop detection mechanism

I think it's not. This is an adapter on how the @clerk/backend will have access to headers to call authenticateRequest. Writing on the headers is the framework's job. Am I missing a use case?

@nikosdouvlis nikosdouvlis Jun 15, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

RequestAdapter can expect a Headers instance instead of this proprietary headers method. Each framework will have full control over how to transform data from its request to a Headers instance.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Writing headers is needed in order to get the loop detection mechanism work for every framework without reimplementing the logic.


/**
* The cookies method returns the value of the specified cookie.
* Keys given to this method exist in `packages/backend/src/constants.ts`.
*
* @param key The cookie name to retrieve
*/
cookies(key: string): string | undefined;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why do we need cookies when we already have headers? I thought that we'd be parsing the cookies inside clerk/backend without requiring the user of the function to do it manually. We can parse the cookies as long as we have headers.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@nikosdouvlis Each framework does a separate thing for accessing the cookies, even using a lib for that. So, we kept the functionality for cookies as-is, giving each framework the flexibility to implement a way to access cookies.


/**
* The searchParams method returns a URLSearchParams object that contains the query parameters
* of the request.
*/
searchParams(): URLSearchParams | undefined;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we accept a URL instead?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@nikosdouvlis What do you mean? accepting URL as a param?

}

export type LoadResourcesOptions = {
loadSession?: boolean;
loadUser?: boolean;
Expand All @@ -37,9 +62,59 @@ export type OptionalVerifyTokenOptions = Partial<
Pick<VerifyTokenOptions, 'authorizedParties' | 'clockSkewInSeconds' | 'jwksCacheTtlInMs' | 'skipJwksCache' | 'jwtKey'>
>;

export type AuthenticateRequestOptions = RequiredVerifyTokenOptions &
type PublicKeys =
| {
publishableKey: string;
/**
* @deprecated Use `publishableKey` instead.
*/
frontendApi: never;
}
| {
publishableKey: never;
/**
* @deprecated Use `publishableKey` instead.
*/
frontendApi: string;
}
| {
publishableKey: string;
/**
* @deprecated Use `publishableKey` instead.
*/
frontendApi: string;
};

type SecretKeys =
| {
secretKey: string;
/**
* @deprecated Use `secretKey` instead.
*/
apiKey: never;
}
| {
secretKey: never;
/**
* @deprecated Use `secretKey` instead.
*/
apiKey: string;
}
| {
secretKey: string;
/**
* @deprecated Use `secretKey` instead.
*/
apiKey: string;
};

export type InstanceKeys = PublicKeys & SecretKeys;

export type AuthenticateRequestOptions = InstanceKeys &
OptionalVerifyTokenOptions &
LoadResourcesOptions & {
apiVersion?: string;
apiUrl?: string;
/* Client token cookie value */
cookieToken?: string;
/* Client uat cookie value */
Expand All @@ -48,12 +123,8 @@ export type AuthenticateRequestOptions = RequiredVerifyTokenOptions &
headerToken?: string;
/* Request origin header value */
origin?: string;
/* Clerk frontend Api value */
frontendApi: string;
/* Clerk Publishable Key value */
publishableKey: string;
/* Request host header value */
host: string;
host?: string;
/* Request forwarded host value */
forwardedHost?: string;
/* Request forwarded port value */
Expand Down Expand Up @@ -84,6 +155,7 @@ export type AuthenticateRequestOptions = RequiredVerifyTokenOptions &
* @experimental
*/
signInUrl?: string;
requestAdapter?: RequestAdapter;
};

function assertSignInUrlExists(signInUrl: string | undefined, key: string): asserts signInUrl is string {
Expand All @@ -99,9 +171,29 @@ function assertProxyUrlOrDomain(proxyUrlOrDomain: string | undefined) {
}

export async function authenticateRequest(options: AuthenticateRequestOptions): Promise<RequestState> {
options.frontendApi = parsePublishableKey(options.publishableKey)?.frontendApi || options.frontendApi || '';
options.apiUrl = options.apiUrl || API_URL;
options.apiVersion = options.apiVersion || API_VERSION;
options = {
...options,
frontendApi: parsePublishableKey(options.publishableKey)?.frontendApi || options.frontendApi,
apiUrl: options.apiUrl || API_URL,
apiVersion: options.apiVersion || API_VERSION,
headerToken:
stripAuthorizationHeader(options.headerToken) ||
stripAuthorizationHeader(options.requestAdapter?.headers(constants.Headers.Authorization)) ||
undefined,
cookieToken: options.cookieToken || options.requestAdapter?.cookies(constants.Cookies.Session) || undefined,
clientUat: options.clientUat || options.requestAdapter?.cookies(constants.Cookies.ClientUat) || undefined,
origin: options.origin || options.requestAdapter?.headers(constants.Headers.Origin) || undefined,
host: options.host || options.requestAdapter?.headers(constants.Headers.Host) || undefined,
forwardedHost:
options.forwardedHost || options.requestAdapter?.headers(constants.Headers.ForwardedHost) || undefined,
forwardedPort:
options.forwardedPort || options.requestAdapter?.headers(constants.Headers.ForwardedPort) || undefined,
forwardedProto:
options.forwardedProto || options.requestAdapter?.headers(constants.Headers.ForwardedProto) || undefined,
referrer: options.referrer || options.requestAdapter?.headers(constants.Headers.Referrer) || undefined,
userAgent: options.userAgent || options.requestAdapter?.headers(constants.Headers.UserAgent) || undefined,
searchParams: options.searchParams || options.requestAdapter?.searchParams() || undefined,
};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

☁️ why not something like this:

if (options.headerToken){
  options.headerToken = stripAuthorizationHeader(options.headerToken)
}

options.frontendApi = parsePublishableKey(options.publishableKey)?.frontendApi || options.frontendApi

const requestOptions = {
  headerToken: options.requestAdapter?.headers(constants.Headers.Authorization),
  cookieToken: options.requestAdapter?.cookies(constants.Cookies.Session),
  clientUat: options.requestAdapter?.cookies(constants.Cookies.ClientUat),
  origin: options.requestAdapter?.headers(constants.Headers.Origin),
  host: options.requestAdapter?.headers(constants.Headers.Host),
  forwardedHost: options.requestAdapter?.headers(constants.Headers.ForwardedHost),
  forwardedPort: options.requestAdapter?.headers(constants.Headers.ForwardedPort),
  forwardedProto: options.requestAdapter?.headers(constants.Headers.ForwardedProto),
  referrer: options.requestAdapter?.headers(constants.Headers.Referrer),
  userAgent: options.requestAdapter?.headers(constants.Headers.UserAgent),
  searchParams: options.requestAdapter?.searchParams(),
}

const defaultOptions = {
  apiUrl: API_URL,
  apiVersion: API_VERSION,
}

mergeWithPriority(options, requestOptions, defaultOptions)



const mergeWithPriority = (...records) => {
  const keys = new Set(records.map(r => Object.keys(r)).flat());
  const mergedRecords = {};

  keys.map(key => {
    record = records.find(r => r[key]);
    mergedRecords[key] = record && record[key];
  });

  return mergedRecords;
}


assertValidSecretKey(options.secretKey || options.apiKey);

Expand Down Expand Up @@ -174,3 +266,7 @@ export const debugRequestState = (params: RequestState) => {
};

export type DebugRequestSate = ReturnType<typeof debugRequestState>;

const stripAuthorizationHeader = (authValue: string | undefined): string | undefined => {
return authValue?.replace('Bearer ', '');
};
6 changes: 3 additions & 3 deletions packages/backend/src/util/request.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ export function checkCrossOrigin({
forwardedProto,
}: {
originURL: URL;
host: string;
host?: string | null;
forwardedHost?: string | null;
forwardedPort?: string | null;
forwardedProto?: string | null;
Expand All @@ -37,7 +37,7 @@ export function checkCrossOrigin({

const protocol = fwdProto || originProtocol;
/* The forwarded host prioritised over host to be checked against the referrer. */
const finalURL = convertHostHeaderValueToURL(forwardedHost || host, protocol);
const finalURL = convertHostHeaderValueToURL(forwardedHost || host || undefined, protocol);
finalURL.port = fwdPort || finalURL.port;

if (getPort(finalURL) !== getPort(originURL)) {
Expand All @@ -50,7 +50,7 @@ export function checkCrossOrigin({
return false;
}

export function convertHostHeaderValueToURL(host: string, protocol = 'https'): URL {
export function convertHostHeaderValueToURL(host?: string | undefined, protocol = 'https'): URL {
/**
* The protocol is added for the URL constructor to work properly.
* We do not check for the protocol at any point later on.
Expand Down
48 changes: 47 additions & 1 deletion packages/fastify/src/utils.test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
import { getSingleValueFromArrayHeader } from './utils';
import { constants } from '@clerk/backend';
import type { FastifyReply, FastifyRequest } from 'fastify';
import Fastify from 'fastify';

import { FastifyRequestAdapter, getSingleValueFromArrayHeader } from './utils';

describe('utils', () => {
describe('getSingleValueFromArrayHeader(value)', () => {
Expand All @@ -15,4 +19,46 @@ describe('utils', () => {
expect(getSingleValueFromArrayHeader(['aloha', '2'])).toEqual('aloha');
});
});

test('FastifyRequestAdapter', async () => {
const fastify = Fastify();

fastify.get('/', (request: FastifyRequest, reply: FastifyReply) => {
const requestAdapter = new FastifyRequestAdapter(request);
expect(requestAdapter.headers(constants.Headers.Authorization)).toEqual('Bearer deadbeef');
expect(requestAdapter.headers(constants.Headers.Origin)).toEqual('http://origin.com');
expect(requestAdapter.headers(constants.Headers.ForwardedPort)).toEqual('1234');
expect(requestAdapter.headers(constants.Headers.ForwardedHost)).toEqual('forwarded-host.com');
expect(requestAdapter.headers(constants.Headers.Host)).toEqual('host.com');
expect(requestAdapter.headers(constants.Headers.Referrer)).toEqual('referer.com');
expect(requestAdapter.headers(constants.Headers.UserAgent)).toEqual(
'Mozilla/5.0 AppleWebKit/537.36 (KHTML, like Gecko) Chrome/109.0.0.0 Safari/537.36',
);
expect(requestAdapter.cookies(constants.Cookies.Session)).toEqual('tokenSession');
expect(requestAdapter.cookies(constants.Cookies.ClientUat)).toEqual('tokenClientUat');
expect(requestAdapter.searchParams()).toBeUndefined();
reply.send({});
});

const response = await fastify.inject({
method: 'GET',
headers: {
Authorization: 'Bearer deadbeef',
Origin: 'http://origin.com',
Host: 'host.com',
'X-Forwarded-Port': '1234',
'X-Forwarded-Host': 'forwarded-host.com',
Referer: 'referer.com',
'User-Agent': 'Mozilla/5.0 AppleWebKit/537.36 (KHTML, like Gecko) Chrome/109.0.0.0 Safari/537.36',
},
cookies: {
__session: 'tokenSession',
__client_uat: 'tokenClientUat',
},
query: { __query: 'true' },
url: '/',
});

expect(response.statusCode).toEqual(200);
});
});
25 changes: 25 additions & 0 deletions packages/fastify/src/utils.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,28 @@
import type { RequestAdapter } from '@clerk/backend';
import { constants } from '@clerk/backend';
import { parse } from 'cookie';
import type { FastifyRequest } from 'fastify';

export const getSingleValueFromArrayHeader = (value?: string[] | string): string | undefined => {
return (Array.isArray(value) ? value[0] : value) || undefined;
};

export class FastifyRequestAdapter implements RequestAdapter {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we require using a class here?
If not, let's use a simple function instead. It should return a RequestAdapter eg:

export const fastifyRequestAdapter = (req): RequestAdapter => {
   ...
} 

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@nikosdouvlis No we don't require a class. It could be changed with a simple function.

@nikosdouvlis nikosdouvlis Jun 15, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's use simple functions then!

readonly reqCookies: Record<string, string>;
constructor(readonly req: FastifyRequest) {
this.reqCookies = parse(req?.raw?.headers?.cookie || '');
}

headers(key: string) {
if (key === constants.Headers.ForwardedHost || key === constants.Headers.ForwardedPort) {
return getSingleValueFromArrayHeader(this.req?.headers?.[key]);
}
return (this.req?.headers?.[key] as string) || undefined;
}
cookies(key: string) {
return this.reqCookies?.[key] || undefined;
}
searchParams(): URLSearchParams | undefined {
return undefined;
}
}
25 changes: 4 additions & 21 deletions packages/fastify/src/withClerkMiddleware.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import type { FastifyReply, FastifyRequest } from 'fastify';
import Fastify from 'fastify';

import { clerkPlugin, getAuth } from './index';
import { FastifyRequestAdapter } from './utils';

const authenticateRequestMock = jest.fn();
const localInterstitialMock = jest.fn();
Expand Down Expand Up @@ -59,15 +60,7 @@ describe('withClerkMiddleware(options)', () => {
expect.objectContaining({
secretKey: 'TEST_API_KEY',
apiKey: 'TEST_API_KEY',
headerToken: 'deadbeef',
cookieToken: undefined,
clientUat: undefined,
origin: 'http://origin.com',
host: 'host.com',
forwardedPort: '1234',
forwardedHost: 'forwarded-host.com',
referrer: 'referer.com',
userAgent: 'Mozilla/5.0 AppleWebKit/537.36 (KHTML, like Gecko) Chrome/109.0.0.0 Safari/537.36',
requestAdapter: expect.any(FastifyRequestAdapter),
}),
);
});
Expand Down Expand Up @@ -107,15 +100,7 @@ describe('withClerkMiddleware(options)', () => {
expect.objectContaining({
secretKey: 'TEST_API_KEY',
apiKey: 'TEST_API_KEY',
cookieToken: 'deadbeef',
headerToken: undefined,
clientUat: '1675692233',
origin: 'http://origin.com',
host: 'host.com',
forwardedPort: '1234',
forwardedHost: 'forwarded-host.com',
referrer: 'referer.com',
userAgent: 'Mozilla/5.0 AppleWebKit/537.36 (KHTML, like Gecko) Chrome/109.0.0.0 Safari/537.36',
requestAdapter: expect.any(FastifyRequestAdapter),
}),
);
});
Expand Down Expand Up @@ -211,9 +196,7 @@ describe('withClerkMiddleware(options)', () => {
expect.objectContaining({
secretKey: 'TEST_API_KEY',
apiKey: 'TEST_API_KEY',
headerToken: 'deadbeef',
cookieToken: undefined,
clientUat: undefined,
requestAdapter: expect.any(FastifyRequestAdapter),
}),
);
});
Expand Down
Loading