Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(backend): Introduce createIsomorphicRequest #1393

Merged
merged 4 commits into from
Jul 7, 2023

Conversation

anagstef
Copy link
Member

@anagstef anagstef commented Jun 20, 2023

Type of change

  • 🐛 Bug fix
  • 🌟 New feature
  • 🔨 Breaking change
  • 📖 Refactoring / dependency upgrade / documentation
  • other:

Packages affected

  • @clerk/clerk-js
  • @clerk/clerk-react
  • @clerk/nextjs
  • @clerk/remix
  • @clerk/types
  • @clerk/themes
  • @clerk/localizations
  • @clerk/clerk-expo
  • @clerk/backend
  • @clerk/clerk-sdk-node
  • @clerk/shared
  • @clerk/fastify
  • @clerk/chrome-extension
  • gatsby-plugin-clerk
  • build/tooling/chore

Description

  • npm test runs as expected.
  • npm run build runs as expected.

Introducing the new utility createIsomorphicRequest for the @clerk/backend package, so that the authenticateRequest signature will be more simplified, and it will be easier to integrate with more frameworks.

@changeset-bot
Copy link

changeset-bot bot commented Jun 20, 2023

🦋 Changeset detected

Latest commit: ee55518

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 6 packages
Name Type
gatsby-plugin-clerk Minor
@clerk/clerk-sdk-node Minor
@clerk/backend Minor
@clerk/fastify Minor
@clerk/nextjs Minor
@clerk/remix Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link

@jit-ci jit-ci bot left a comment

Choose a reason for hiding this comment

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

✅ Great news! Jit hasn't found any security issues in your PR. Good Job! 🏆

@anagstef anagstef changed the title feat(backend): Introduce createIsomorphicRequest [WIP] feat(backend): Introduce createIsomorphicRequest Jun 20, 2023
@@ -133,6 +134,7 @@ export type AuthenticateRequestOptions = InstanceKeys &
* @experimental
*/
signInUrl?: string;
request?: Request;
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be turned into a new signature - lets take this offline

@anagstef anagstef changed the title [WIP] feat(backend): Introduce createIsomorphicRequest feat(backend): Introduce createIsomorphicRequest Jun 23, 2023
@anagstef anagstef marked this pull request as ready for review June 23, 2023 17:01
@anagstef anagstef force-pushed the stefanos/js-464-introduce-isomorphic-request branch from 048725d to 33ae931 Compare June 26, 2023 11:42
forwardedProto:
options.forwardedProto || isomorphicRequest?.headers?.get(constants.Headers.ForwardedProto) || undefined,
referrer: options.referrer || isomorphicRequest?.headers?.get(constants.Headers.Referrer) || undefined,
userAgent: options.userAgent || isomorphicRequest?.headers?.get(constants.Headers.UserAgent) || undefined,
Copy link
Contributor

Choose a reason for hiding this comment

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

🙃 could we create a helper method for the repetitive parts to reduce the length of each size and make it more readable?
Example:

const getFromReq = (req, key) => req?.headers?.get(key) || undefined ;

// usage
options = {
// ...
forwardedProto: options.forwardedProto  || getFromReq(isomorphicRequest, constants.Headers.ForwardedProto)
//...
}

packages/sdk-node/src/authenticateRequest.ts Outdated Show resolved Hide resolved
packages/sdk-node/src/authenticateRequest.ts Show resolved Hide resolved
return value?.split(',')[0]?.trim() || '';
}

const isRelativeUrl = (url: string) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

🙃 let's add a comment for this one to move it to shared package or somewhere else and re-use it from there. I believe we have that code somewhere too.

packages/fastify/src/withClerkMiddleware.ts Show resolved Hide resolved

type IsomorphicRequestOptions = (Request: Request, Headers: Headers) => Request;

export const createIsomorphicRequest = (cb: IsomorphicRequestOptions): Request => {
Copy link
Contributor

Choose a reason for hiding this comment

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

❓ Why are we calling this an IsomorphicRequest since it's just the global.Request?

Copy link
Member

Choose a reason for hiding this comment

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

I recommended the IsomorphicRequest name because it's a Request that can run on any platform, even platforms that natively do not support global.Request (older node runtimes etc).

Would createRequest make more sense to you? I didn't choose this one because it doesn't make the difference between new Request and createRequest apparent. Happy to change the name though :)

packages/backend/src/tokens/request.ts Outdated Show resolved Hide resolved
packages/backend/src/tokens/request.ts Outdated Show resolved Hide resolved
@@ -1,39 +1,40 @@
import { parse } from 'cookie';
import { createIsomorphicRequest } from '@clerk/backend';
import type { FastifyReply, FastifyRequest } from 'fastify';

import { clerkClient } from './clerkClient';
import * as constants from './constants';
import type { ClerkFastifyOptions } from './types';
import { getSingleValueFromArrayHeader } from './utils';
Copy link
Contributor

Choose a reason for hiding this comment

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

🔧 This also feels it belongs to the request utility

Copy link
Member Author

Choose a reason for hiding this comment

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

What do you mean exactly? This is the fastify package importing the utility from backend as we have designed with @nikosdouvlis

packages/fastify/src/withClerkMiddleware.ts Show resolved Hide resolved
packages/nextjs/src/server/authenticateRequest.ts Outdated Show resolved Hide resolved
signInUrl,
request: createIsomorphicRequest((Request: any, Headers: any) => {
Copy link
Member

Choose a reason for hiding this comment

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

❓ Similar to my other comment, why don't we just pass req directly?

Copy link
Member Author

Choose a reason for hiding this comment

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

We do some sanitization actions first for the Request URL. That's why we need to create it ourselves.

packages/sdk-node/src/authenticateRequest.ts Outdated Show resolved Hide resolved
packages/fastify/src/withClerkMiddleware.ts Outdated Show resolved Hide resolved
@anagstef anagstef force-pushed the stefanos/js-464-introduce-isomorphic-request branch from 0473ca4 to c34134e Compare July 7, 2023 09:45
Copy link

@jit-ci jit-ci bot left a comment

Choose a reason for hiding this comment

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

❌ Jit has detected important findings in this PR that you should review.
Click here to view these findings on Jit.

Copy link

@jit-ci jit-ci bot left a comment

Choose a reason for hiding this comment

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

✅ Great news! All security issues were resolved. Good Job! 🏆

anagstef and others added 4 commits July 7, 2023 13:09
Introducing the new utility `createIsomorphicRequest` for the `@clerk/backend` package, so that the `authenticateRequest` signature will be more simplified, and it will be easier to integrate with more frameworks.
… createIsomorphicRequest

chore(backend): Add changeset for `createIsomorphicRequest`

refactor(clerk-sdk-node,backend): Remove sdk-node cookie dependency

Also refactor authenticateRequest options

fix(backend): Refactor the new IsomorphicRequest utilities

Also, manipulate headers objects to be compatible with Headers constructor

chore(repo): Revert `package-lock.json` changes
@anagstef anagstef force-pushed the stefanos/js-464-introduce-isomorphic-request branch from 04045b1 to ee55518 Compare July 7, 2023 10:09
@nikosdouvlis nikosdouvlis merged commit fd692af into main Jul 7, 2023
9 checks passed
@nikosdouvlis nikosdouvlis deleted the stefanos/js-464-introduce-isomorphic-request branch July 7, 2023 10:33
@clerk-cookie clerk-cookie mentioned this pull request Jul 7, 2023
@RobZuazua
Copy link

RobZuazua commented Jul 22, 2023

@anagstef, this change broke my setup (AWS amplify serverless express lambdas)

I rolled back to previous version (4.10.15) and everything worked again.

{ "errorType": "Runtime.UnhandledPromiseRejection", "errorMessage": "TypeError [ERR_INVALID_URL]: Invalid URL", "reason": { "errorType": "TypeError", "errorMessage": "Invalid URL", "code": "ERR_INVALID_URL", "input": "/api/s3/upload/0718/MP3", "stack": [ "TypeError [ERR_INVALID_URL]: Invalid URL", " at new NodeError (node:internal/errors:387:5)", " at URL.onParseError (node:internal/url:565:9)", " at new URL (node:internal/url:641:5)", " at new Request (/var/task/node_modules/node-fetch-native/dist/shared/node-fetch-native.8afd3fea.cjs:6006:16)", " at /var/task/node_modules/@clerk/clerk-sdk-node/dist/cjs/index.js:143:12", " at createIsomorphicRequest (/var/task/node_modules/@clerk/backend/dist/index.js:1829:10)", " at authenticateRequest (/var/task/node_modules/@clerk/clerk-sdk-node/dist/cjs/index.js:141:72)", " at /var/task/node_modules/@clerk/clerk-sdk-node/dist/cjs/index.js:211:34", " at Layer.handle [as handle_request] (/var/task/node_modules/express/lib/router/layer.js:95:5)", " at next (/var/task/node_modules/express/lib/router/route.js:144:13)" ] }, "promise": {}, "stack": [ "Runtime.UnhandledPromiseRejection: TypeError [ERR_INVALID_URL]: Invalid URL", " at process.<anonymous> (file:///var/runtime/index.mjs:1186:17)", " at process.emit (node:events:513:28)", " at process.emit (node:domain:489:12)", " at emit (node:internal/process/promises:140:20)", " at processPromiseRejections (node:internal/process/promises:274:27)", " at processTicksAndRejections (node:internal/process/task_queues:97:32)" ] }

@clerk-cookie
Copy link
Collaborator

This PR has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@clerk clerk locked as resolved and limited conversation to collaborators Jul 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants