Skip to content

@cipherstash/nextjs middleware fails the request when CS_WORKSPACE_CRN is missing or invalid — decide whether it should keep throwing #1606

Open

Description

@tobyhede

Background

@cipherstash/nextjs is our helper package for Next.js apps. Its middleware (protectMiddleware and protectClerkMiddleware) keeps a session from CTS (the CipherStash token service) in a cookie. To get a new session, it has to tell CTS which CipherStash workspace the session is for. It reads that from CS_WORKSPACE_CRN, the workspace's CRN (Cloud Resource Name).

Problem

What happens

  1. A signed-in user needs a new session: they have none, theirs expired, or the cookie belongs to someone else.
  2. The middleware calls setCtsToken, which calls fetchCtsToken.
  3. fetchCtsToken calls loadWorkSpaceId() as its first step, before the try that handles network failures.
  4. loadWorkSpaceId() throws when the variable is not set or is not a valid CRN. Nothing catches it, so the request fails.

Who hits it

Only a misconfigured deployment. Users who still hold a valid session keep working, because no new session is needed. So the failures do not all appear at deploy time. They appear one user at a time as sessions expire, on every route the middleware matches, including pages that never touch encrypted data. Code that calls fetchCtsToken or getCtsToken(oidcToken) directly gets the same exception.

Why this is unsettled

The code disagrees with itself about what should happen:

  • fetchCtsToken has a branch written to log an error and return { success: false } when there is no workspace ID. It can never run, because loadWorkSpaceId() either throws or returns a non-empty ID. Its message also names CS_WORKSPACE_ID, a variable this code does not read.
  • loadWorkSpaceId() documents that it throws, and no test covers a missing or invalid CRN.

#1543 stopped a CTS network failure from failing the request. It left this case alone on purpose, because a deployment error may be better reported loudly.

Proposal

  1. Check this against Deprecate CTS-token middleware and document the OIDC identity-claim migration #593 first. That issue tracks retiring the CTS-token middleware. If removal is close, do only step 2 and close this issue.
  2. Delete the unreachable branch in fetchCtsToken. It describes behaviour the code does not have and names the wrong variable. This can ship on its own.
  3. Decide which behaviour we want, and pin it with a test:
    • Keep throwing. A deployment error stays loud. Say so with @throws on fetchCtsToken and getCtsToken, and in the docs for both middlewares. The cost is that pages which never use encryption fail too.
    • Return a failure. Move loadWorkSpaceId() inside the try, log at error level, and return { success: false }, so the middleware passes the request through. The cost is that a misconfigured deployment keeps serving pages with no session, and the problem shows only in logs. fetchCtsToken and getCtsToken(oidcToken) would stop throwing, so this needs a changeset.
  4. Whichever is chosen, the failure must be loud and must name CS_WORKSPACE_CRN, so whoever reads the error knows what to fix.

Evidence

Verified by reading the code at commit 2caabb39, the head of #1543 when this was written:

  • The call order in fetchCtsToken, both throw sites, and the unreachable branch.
  • The package's tests only ever set a valid CS_WORKSPACE_CRN.
  • The same call sits outside any try in the code fix(nextjs): harden the CipherStash session cookie #1543 was branched from, so this predates that PR.

Reproduced with a throwaway test against the package, with the variable unset and set to not-a-crn:

  • The middleware rejected with You have not defined a workspace CRN… and Invalid CRN format.
  • getCtsToken(oidcToken) threw the same errors.
  • A request with a valid, reusable session passed.

Not verified: this was not run on a real Next.js server. That the user sees a 500 is inferred from the middleware rejecting.

Relationship to other work

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    SDKneeds-triageMaintainer needs to evaluate the issue

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions