Framework for passkey auth

[ericelliott]

Break into separate issues:

• Server side API
• Client side components

---

• allow users to use either on-device enclaves OR 3rd party passkey hosts (e.g. LastPass, 1Password, etc)
• Don't force web users to scan a QR code if the device supports native enclave

[ericelliott]

@janhesters please address error logging and error handling tests.

"""
Now I'll conduct a comprehensive code review following the OWASP Top 10 and project standards.

🔬 Code Review: Auth Middleware PR

🎯 Summary

This PR adds well-structured authentication middleware wrapping better-auth. The implementation follows existing patterns and includes comprehensive tests. However, there are security concerns around error handling and logging that should be addressed.

---

✅ Strengths

1. Excellent API Design

   • Two factories (createWithAuth / createWithOptionalAuth) avoid boolean trap
   • Follows existing createWith* pattern consistently
   • Clear separation of protected vs. public route concerns

2. Strong Test Coverage

   • 15 tests covering both happy and error paths
   • Proper test isolation with mocked dependencies
   • Tests verify factory independence

3. Good Documentation

   • README includes usage examples
   • Passkey authentication example provided
   • TypeScript definitions complete and accurate

4. Follows Project Standards

   • Adheres to JavaScript style guide (pure functions, SDA, destructuring)
   • Proper docblocks without verbosity
   • Colocated tests

---

🔴 Critical Issues

1. Security Logging: No audit trail for authentication failures (OWASP #9)

Issue: Silent authentication failures make debugging difficult and miss security events.

/**
 * Auth middleware factories for session validation
 * Wraps better-auth to validate sessions and attach user to response.locals
 */

const defaultOnUnauthenticated = ({ response }) => {
  response.status(401).json({ error: "Unauthorized" });
};

const ensureLocals = (response) => {
  if (!response.locals) response.locals = {};
  return response;
};

const getSession = async ({ auth, request }) => {
  const result = await auth.api.getSession({ headers: request.headers });
  return result;
};

/**
 * Creates auth middleware that requires authentication
 * Returns 401 if no valid session
 *
 * @param {Object} options
 * @param {Object} options.auth - better-auth instance
 * @param {Function} [options.onUnauthenticated] - Custom handler for 401 responses
 * @returns {Function} Middleware function
 *
 * @example
 * const withAuth = createWithAuth({ auth });
 *
 * // Protected route - 401 if not logged in
 * export default createRoute(withAuth, async ({ response }) => {
 *   const { user } = response.locals.auth;
 *   response.json({ email: user.email });
 * });
 */
const createWithAuth = ({
  auth,
  onUnauthenticated = defaultOnUnauthenticated,
} = {}) => {
  if (!auth) {
    throw new Error("auth is required. Pass your better-auth instance.");
  }

  return async ({ request, response }) => {
    ensureLocals(response);

    const result = await getSession({ auth, request });

    if (!result) {
      onUnauthenticated({ request, response });
      return { request, response };
    }

    response.locals.auth = {
      user: result.user,
      session: result.session,
    };

    return { request, response };
  };
};

/**
 * Creates auth middleware that allows anonymous requests
 * Attaches user if session exists, otherwise sets auth to null
 *
 * @param {Object} options
 * @param {Object} options.auth - better-auth instance
 * @returns {Function} Middleware function
 *
 * @example
 * const withOptionalAuth = createWithOptionalAuth({ auth });
 *
 * // Public route with optional user context
 * export default createRoute(withOptionalAuth, async ({ response }) => {
 *   const user = response.locals.auth?.user;
 *   response.json({
 *     greeting: user ? `Hello, ${user.name}` : 'Hello, guest'
 *   });
 * });
 */
const createWithOptionalAuth = ({ auth } = {}) => {
  if (!auth) {
    throw new Error("auth is required. Pass your better-auth instance.");
  }

  return async ({ request, response }) => {
    ensureLocals(response);

    const result = await getSession({ auth, request });

    if (!result) {
      response.locals.auth = null;
      return { request, response };
    }

    response.locals.auth = {
      user: result.user,
      session: result.session,
    };

    return { request, response };
  };
};

export { createWithAuth, createWithOptionalAuth };

Problems:

• No logging when authentication fails
• No way to track suspicious authentication attempts
• No context about what went wrong (expired session vs. invalid token vs. missing cookie)
• Inconsistent with security best practices for auth systems

Recommendation: Add optional logging for authentication events, respecting response.locals.requestId if available:

const getSession = async ({ auth, request }) => {
  try {
    const result = await auth.api.getSession({ headers: request.headers });
    return result;
  } catch (error) {
    // Log authentication error for security monitoring
    console.error('Auth session validation error:', {
      error: error.message,
      // Avoid logging sensitive headers
    });
    return null; // Treat errors as unauthenticated
  }
};

const createWithAuth = ({ auth, onUnauthenticated = defaultOnUnauthenticated } = {}) => {
  // ...existing validation...
  
  return async ({ request, response }) => {
    ensureLocals(response);
    const result = await getSession({ auth, request });

    if (!result) {
      // Log failed authentication attempt (consider adding requestId if available)
      const requestId = response.locals.requestId;
      console.warn('Unauthorized access attempt', {
        requestId,
        url: request.url,
        method: request.method,
      });
      
      onUnauthenticated({ request, response });
      return { request, response };
    }
    // ...rest
  };
};

---

2. Error Handling: Unhandled getSession errors (OWASP #7 - Authentication Failures)

Issue: If auth.api.getSession() throws an error, it will propagate up and crash the middleware chain.

const getSession = async ({ auth, request }) => {
  const result = await auth.api.getSession({ headers: request.headers });
  return result;
};

Problem:

• Network errors, database errors, or malformed requests could throw
• No graceful degradation
• User gets 500 instead of 401

Recommendation: Wrap in try-catch and treat errors as unauthenticated:

const getSession = async ({ auth, request }) => {
  try {
    const result = await auth.api.getSession({ headers: request.headers });
    return result;
  } catch (error) {
    // Log error for debugging but don't expose to client
    console.error('Auth session validation error:', {
      error: error.message,
      stack: error.stack,
    });
    return null; // Treat validation errors as unauthenticated
  }
};

---

⚠️ Medium Priority Issues

3. Code Quality: Missing test for error handling

Issue: No tests verify behavior when auth.api.getSession() throws an error.

Current tests (lines 1-19 of test file):

import { describe, test, vi } from "vitest";
import { assert } from "riteway/vitest";
import { createWithAuth, createWithOptionalAuth } from "./with-auth.js";
import { createServer } from "../test-utils.js";

// Mock better-auth instance factory
const createMockAuth = ({
  session = null,
  user = null,
  getSessionError = null,
} = {}) => ({
  api: {
    getSession: vi.fn(async () => {
      if (getSessionError) throw getSessionError;
      if (!session) return null;
      return { session, user };
    }),
  },
});

Recommendation: Add tests for error scenarios:

test("handles getSession errors gracefully", async () => {
  const auth = createMockAuth({ 
    getSessionError: new Error("Database connection failed") 
  });
  const mockResponse = {
    status: vi.fn().mockReturnThis(),
    json: vi.fn(),
  };

  const withAuth = createWithAuth({ auth });
  await withAuth(createServer({ 
    request: { headers: {} }, 
    response: mockResponse 
  }));

  assert({
    given: "getSession throws error",
    should: "return 401 (treat as unauthenticated)",
    actual: mockResponse.status.mock.calls[0]?.[0],
    expected: 401,
  });
});

---

4. Documentation: Missing session lifecycle guidance

Issue: README doesn't mention session refresh, expiration, or lifecycle management.

Current README shows basic usage but doesn't cover:

• How to handle session refresh
• What happens when session expires mid-request
• Whether better-auth auto-refreshes sessions
• Best practices for session duration

Recommendation: Add a "Session Management" section to README:

### Session Management

**Session Lifecycle:**
- Sessions are validated on each request via `auth.api.getSession()`
- better-auth handles session refresh automatically based on your configuration
- Expired sessions return `null`, triggering 401 (or null for optional auth)

**Session Refresh:**
```javascript
// better-auth refreshes sessions automatically
// Configure in your auth setup:
export const auth = betterAuth({
  database: yourDatabaseAdapter,
  session: {
    expiresIn: 60 * 60 * 24 * 7, // 7 days
    updateAge: 60 * 60 * 24, // Refresh daily
  },
});

---

🟡 Minor Issues

5. Code Organization: Redundant ensureLocals pattern

Issue: ensureLocals is called in both middleware but could be extracted to a shared pattern.

Current code:

const ensureLocals = (response) => {
  if (!response.locals) response.locals = {};
  return response;
};

Observation: This pattern exists in other middleware files too. Consider:

• Exporting from a shared utility
• OR documenting why each middleware should handle this independently (my preference for simplicity)

Recommendation: This is actually fine as-is. The duplication is minimal and keeps each middleware self-contained. ✅

---

6. TypeScript: Consider stricter types for auth object

Current types (lines 191-196):

export interface BetterAuthInstance {
  api: {
    getSession(options: { headers: Record<string, any> }): Promise<{ session: Session; user: User } | null>;
  };
  [key: string]: any;
}

Issue: [key: string]: any makes the interface very loose.

Recommendation: Consider removing the index signature if not needed, or make it more specific:

export interface BetterAuthInstance {
  api: {
    getSession(options: { headers: Record<string, any> }): Promise<{ session: Session; user: User } | null>;
    [key: string]: any; // Allow other API methods
  };
  // Remove the root index signature for type safety
}

---

🔍 OWASP Top 10 Security Review

Item	Status	Notes
1. Broken Access Control	✅	Properly delegates to better-auth
2. Cryptographic Failures	✅	N/A (better-auth handles)
3. Injection	✅	No user input directly used
4. Insecure Design	⚠️	Missing error handling (#2 above)
5. Security Misconfiguration	✅	Requires explicit config
6. Vulnerable Components	✅	Peer dependency allows version control
7. Authentication Failures	🔴	Issues #1 and #2 above
8. Data Integrity	✅	Delegates to better-auth
9. Logging/Monitoring	🔴	Issue #1 above
10. SSRF	✅	N/A

---

📋 Recommendations Summary

Must Fix (Security):

1. Add try-catch to getSession to handle errors gracefully
2. Add security logging for failed authentication attempts
3. Add tests for error handling scenarios

Should Fix (Quality):
4. Add session lifecycle documentation to README

Nice to Have:
5. Tighten TypeScript types (remove root index signature)

---

✅ What's Working Well

• Clean, minimal implementation
• Excellent test coverage for happy paths
• Follows project patterns consistently
• Great documentation with practical examples
• Smart API design avoiding boolean trap
• Passkey example is valuable
• All tests pass ✅
• TypeScript checking passes ✅
• Peer dependency approach is correct

---

🎯 Verdict

Approval Status: ⚠️ Approve with changes

The implementation is solid and follows best practices, but security logging and error handling must be addressed before merging to production. The issues are straightforward to fix and don't require architectural changes.

Estimated effort to fix: 1-2 hours (add error handling + 3-4 new tests)

---

 

_{Learn more about Cursor Agents}
"""