Better authentication handling

[openint-bot]

Important

Improves authentication handling by introducing viewerFromRequest for parsing authorization headers, with comprehensive tests for API key and JWT token scenarios.

• Authentication Handling:
  • Introduces viewerFromRequest in authentication.ts for parsing and verifying authorization headers.
  • Handles API key and JWT token authentication, throwing UNAUTHORIZED errors for invalid cases.
• Testing:
  • Adds authentication.spec.ts to test viewerFromRequest for various scenarios: missing, malformed, valid, and invalid tokens.
  • Tests include API key and JWT token authentication, including expired tokens.
• Context Update:
  • Updates context.ts to use viewerFromRequest for creating router context.
• Dependencies:
  • Adds @openint/loopback-link to devDependencies in package.json.

^{This description was created by for 973384c. It will automatically update as commits are pushed.}

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
openint	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Mar 6, 2025 4:47pm

[recurseml]

The code attempts to verify a token that could be undefined. If no Authorization header is provided, token will be undefined and this will cause a runtime error. Should add a token check before verification: if (!token) throw new TRPCError({code: 'UNAUTHORIZED', message: 'No token provided'})

---

React with 👍 to tell me that this comment was useful, or 👎 if not (and I'll stop posting more comments like this in the future)

[recurseml]

😱 Found 1 issue. Time to roll up your sleeves! 😱

[ellipsis-dev]

❌ Changes requested. Reviewed everything up to 6905bf9 in 2 minutes and 30 seconds

More details

• Looked at 234 lines of code in 3 files
• Skipped 0 files when reviewing.
• Skipped posting 7 drafted comments based on config settings.

1. packages/api-v1/trpc/authentication.spec.ts:45

• Draft comment:
  Test calls viewerFromRequest twice in the same assertion block; consider reusing the promise to avoid any unintended side-effects.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50%
  None

2. packages/api-v1/trpc/authentication.spec.ts:34

• Draft comment:
  The test for missing authorization header expects an anonymous viewer. Confirm that jwt.verifyViewer handles undefined tokens correctly, or consider explicitly checking for missing tokens before verification.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50%
  None

3. packages/api-v1/trpc/authentication.ts:12

• Draft comment:
  Explicitly handle the case where no token is provided instead of passing undefined to jwt.verifyViewer. This clarifies that missing headers yield an anonymous viewer.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
  The comment suggests adding explicit handling for missing tokens, but the code already has error handling in place. The try/catch block will catch any errors from verifyViewer, including undefined token errors. Adding explicit handling would be redundant. The comment also makes an assumption about "anonymous viewer" behavior which isn't evident from the code.
  Maybe there's a performance benefit to checking for undefined tokens early rather than letting it fail in verifyViewer? Maybe the error message could be more specific for missing tokens?
  While those are valid points, they're minor optimizations. The current error handling is sufficient and follows the principle of centralized error handling in the try/catch block.
  Delete the comment. The code already handles missing tokens appropriately through its error handling, and the comment makes assumptions about desired behavior that aren't supported by the code.

4. packages/api-v1/trpc/authentication.ts:21

• Draft comment:
  Relying on token.includes('.') as a heuristic to distinguish JWTs from API keys may be fragile. Ensure that API keys will never include a '.' or document this assumption clearly.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
  The comment raises a speculative concern ("may be fragile") and asks for verification/documentation ("Ensure that..."). It doesn't point out a definite issue. JWTs do always contain dots as they are base64-encoded segments joined by dots - this is part of the JWT standard. The code's comment acknowledges this assumption explicitly.
  The comment might be highlighting a valid security concern about API key format assumptions. Perhaps there should be more robust validation.
  While API key validation could be more robust, this comment is speculative and asks for verification rather than pointing out a concrete issue. The JWT dot requirement is a standard, not just an assumption.
  Delete this comment as it is speculative and asks for verification rather than pointing out a definite issue that needs fixing.

5. packages/api-v1/trpc/authentication.ts:36

• Draft comment:
  Avoid exposing internal error details in the unauthorized error message. Consider logging the error internally and returning a generic message to the client.
• Reason this comment was not posted:
  Marked as duplicate.

6. packages/api-v1/trpc/authentication.spec.ts:45

• Draft comment:
  Avoid calling viewerFromRequest twice for the same input. Instead, store the returned promise and assert both error type and message on it to prevent any unexpected side effects.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
  The suggestion is technically correct - calling an async function twice could potentially have side effects or be less efficient. However, in this case: 1) It's just a test 2) The pattern is used consistently throughout the file 3) The current approach is more readable as it clearly separates the two assertions 4) There's no evidence of actual side effects from viewerFromRequest.
  The comment suggests a valid optimization, and multiple calls could theoretically cause issues if the function is not idempotent.
  While technically correct, this is an over-optimization for test code where clarity is more important than performance, and there's no evidence of actual problems.
  Delete the comment as it suggests an unnecessary optimization that would make the tests less readable without clear benefits.

7. packages/api-v1/trpc/authentication.ts:21

• Draft comment:
  Typo found: In the comment 'JWT always include a dot. Without a dot we assume it's an API key', the word 'include' should be 'includes' for proper grammar. This is a trivial issue.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 20% <= threshold 50%
  This comment is pointing out a grammatical error in a comment within the code. While it is not directly related to the functionality or logic of the code, correcting typos can improve code readability and maintainability. However, it does not provide a suggestion for code improvement or testing, nor does it address any potential issues with the code's functionality. According to the rules, purely informative comments should be removed, and this comment seems to fall into that category.

Workflow ID: wflow_HJtyDOUUmGjQmwhl

---

Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

Avoid including the full error object in the TRPCError message (using ${err}) as it may expose internal details. Consider using a generic message.

Suggested change

	throw new TRPCError({code: 'UNAUTHORIZED', message: `${err}`})
	throw new TRPCError({code: 'UNAUTHORIZED', message: 'An error occurred'})

[ellipsis-dev]

👍 Looks good to me! Incremental review on 973384c in 1 minute and 18 seconds

More details

• Looked at 38 lines of code in 2 files
• Skipped 1 files when reviewing.
• Skipped posting 4 drafted comments based on config settings.

1. packages/api-v1/__tests__/app.spec.ts:28

• Draft comment:
  Explicitly assert loopbackLink's expected authentication or routing behavior. If it's meant to bypass standard auth, include assertions that verify this behavior.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
  The comment raises a valid point about testing - the current test doesn't verify what loopbackLink actually does. However, this test file seems focused on basic API endpoint testing, not testing link behavior. Without seeing loopbackLink's implementation or intended behavior, we can't be sure what specific assertions would be appropriate. The comment is somewhat speculative about auth bypassing.
  I might be underestimating the importance of thoroughly testing middleware components. The comment could be highlighting a real security concern.
  While thorough testing is important, making assertions about loopbackLink's behavior requires understanding its intended purpose, which isn't clear from this context. The comment is too speculative.
  Delete the comment as it makes assumptions about loopbackLink's purpose and suggests tests without clear requirements or context.

2. packages/api-v1/package.json:30

• Draft comment:
  Ensure that the added '@openint/loopback-link' dependency is compatible with the intended authentication improvements and its versioning requirement is maintained across workspaces.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50%
  None

3. packages/api-v1/__tests__/app.spec.ts:28

• Draft comment:
  Consider asserting the response status (e.g., res.ok) in the new test for clearer failure info.
• Reason this comment was not posted:
  Confidence changes required: 50% <= threshold 50%
  None

4. packages/api-v1/package.json:30

• Draft comment:
  Verify that '@openint/loopback-link' is intended only for testing. If used in production code, consider moving it from devDependencies.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
  This comment violates several rules. It starts with "Verify that..." which is explicitly discouraged. It's speculative ("If used in production code..."). It asks the author to confirm their intention. There's no strong evidence that this is incorrectly placed - the name suggests it's for testing, and it's grouped with other dev tools.
  The package name alone isn't definitive proof it's for testing. We don't have access to the actual package to confirm its purpose.
  Even with uncertainty about the package's exact purpose, the comment itself violates multiple rules about not asking for verification or making speculative statements.
  Delete this comment as it violates rules against verification requests and speculation, and there's no strong evidence of an actual issue.

Workflow ID: wflow_GwTJJAFw5OlnPoP3

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.