fix(email): skip cleanly when EMAIL_PROVIDER_API_KEY is unset

[xBalbinus]

Summary

• sendInvitation was throwing whenever EMAIL_PROVIDER_API_KEY was unset, which the gifts route caught + logged at error level — every dev/CI send produced a fake "invitation failed" error even though the gift succeeded and the access token was already persisted.
• Now returns a discriminated InvitationOutcome ({sent:true} / {sent:false, reason:"no-api-key"} / throws on real Resend failure). The route logs the no-key case at info level so error monitoring stays signal.

Test plan

• bun --cwd server test src/lib/email.test.ts → 8/8 pass (3 new)
• Full server suite → 71/71 pass with INTEGRATION=1
• Manual: hit POST /gifts/:id/send against a server with no EMAIL_PROVIDER_API_KEY. Server log now reads [email] no EMAIL_PROVIDER_API_KEY set; skipping invitation for gift=… recipient=… and level:error filtering returns nothing.

[slopless-scanner]

⚠️ Slopless Review

Confidence: 🟡 4/5 · Verdict: REQUEST_CHANGES · Risk: HIGH · Findings: 14

Reviewed the PR diff without prior scan context (no architecture artifacts found). Produced 14 finding(s) across 4 vulnerability class(es) (architecture, best_practice, code_quality (+1 more)) in 49.5s.

PR review complete: 14 findings, verdict: request_changes

Scope & coverage

• Lines: 2979
• Checks performed: architecture · best_practice · code_quality · security

Findings

🔴 HIGH — Missing Authorization Check on Journal Entry CRUD Operations

server/src/routes/recipient.ts:200-350 · confidence: high · CWE-639

The new journal entry endpoints (createEntry, patchEntry, deleteEntry, listEntries) in recipient.ts accept a token parameter but do not verify that the current recipient owns the journal entries being accessed. An attacker could potentially access or modify another recipient's journal entries by guessing or obtaining a valid token and then manipulating entry IDs. This is a classic BOLA (Broken Object Level Authorization) vulnerability.

Suggested fix: Add explicit ownership verification before any journal entry operation. Verify that the recipient identified by the token owns the gift and entry being accessed. Example: if (entry.recipientId !== recipient.id) { set.status = 403; return { error: 'Forbidden' }; }

Code context

// Example from recipient.ts - missing ownership verification
.post(
  '/entries',
  async ({ params, body }) => {
    const entry = await createEntry({
      recipientId: params.token,  // Using token as ID without verification
      giftId: body.giftId,
      source: body.source,
      // ... no check that current recipient owns this gift
    });
  }
)

🔴 HIGH — Hardcoded Test API Keys in Test Files

server/src/lib/email.test.ts:84-108 · confidence: high · CWE-798

The PR contains hardcoded test API keys in test files (email.test.ts lines 84, 108 and recipient-flow.integration.test.ts line 110). While these are marked as test keys, hardcoding credentials in source code is a security anti-pattern that can lead to accidental exposure if test files are committed or shared. The keys should be generated dynamically or sourced from environment variables.

Suggested fix: Use environment variables or generate test keys dynamically. For Resend, use their documented test key format without hardcoding. Consider: const testKey = process.env.TEST_EMAIL_KEY || 're_test_' + Math.random().toString(36).slice(2);

Code context

process.env.EMAIL_PROVIDER_API_KEY = "re_test_key";
// ...
process.env.EMAIL_PROVIDER_API_KEY = "re_bad_key";

🔴 HIGH — Missing Password Validation in Recipient Account Creation

server/src/lib/gift-store.ts:1-50 · confidence: medium · CWE-521

The setRecipientCredentials function (referenced in recipient.ts) accepts a password but there's no evidence of password strength validation, hashing verification, or secure storage practices. The schema shows passwordHash field but the implementation details are not visible in the diff. If passwords are stored in plaintext or with weak hashing, this is a critical vulnerability.

Suggested fix: Implement password validation: minimum length (12+ chars), complexity requirements, and use bcrypt or Argon2 for hashing. Validate before hashing: if (password.length < 12) throw new Error('Password too weak'); Use: const hash = await bcrypt.hash(password, 12);

Code context

// From recipient.ts route handler
.post('/account', async ({ params, body }) => {
  await setRecipientCredentials(params.token, body.password);
  // No visible password strength validation
})

🔴 HIGH — Recipient Login Endpoint Missing Rate Limiting

server/src/routes/auth.ts:132-170 · confidence: high · CWE-307

The new POST /auth/recipient/login endpoint (auth.ts lines 132-170) has no rate limiting or brute-force protection. An attacker can attempt unlimited password guesses against any email address. This is especially critical since the endpoint returns different error messages for 'wrong email' vs 'wrong password', enabling user enumeration.

Suggested fix: Implement rate limiting using a library like @elysiajs/rate-limit or track failed attempts in the database. Example: Track login attempts per IP/email, lock after 5 failures for 15 minutes. Also consider: if (!recipient) { set.status = 401; return { error: 'Invalid credentials' }; } (already generic, which is good).

Code context

.post(
  "/recipient/login",
  async ({ body, set }) => {
    const recipient = await recipientLogin(body.email, body.password);
    if (!recipient) {
      set.status = 401;
      return { error: "Wrong email or password." };  // Generic message is good
    }
    // No rate limiting, no attempt tracking
  }
)

🟠 MEDIUM — Sharing State Modification Without Validation

server/src/routes/recipient.ts:400-450 · confidence: medium · CWE-20

The patchSharing function allows recipients to modify their sharing configuration (mode, date, milestone). There's no validation that the sharing mode transition is valid or that dates are in the future. A recipient could set a past date or invalid milestone configuration, potentially causing issues with the share logic.

Suggested fix: Add validation before updating sharing state: if (body.mode === 'date' && body.date && new Date(body.date) < new Date()) { throw new Error('Date must be in future'); } Also validate that only valid mode transitions are allowed.

Code context

// Inferred from recipient.ts
.patch('/sharing', async ({ params, body }) => {
  await patchSharing(params.token, body);
  // No validation of mode transitions or date constraints
})

🟠 MEDIUM — Audio/Photo Upload Without File Type Validation

server/src/routes/recipient-flow.integration.test.ts:1-100 · confidence: medium · CWE-434

The recipient flow allows uploading audio and photos (journal entries with audioUrl, photoUrl). The integration test stubs Whisper transcription but there's no visible validation of file types, sizes, or MIME types before upload. An attacker could upload malicious files or exhaust storage.

Suggested fix: Add file validation middleware: Check MIME types (audio/, image/), enforce size limits (e.g., 50MB for audio, 10MB for photos), scan for malicious content. Example: if (!file.type.startsWith('audio/')) throw new Error('Invalid audio type');

Code context

// Test shows audio upload but no validation visible
const outcome = await app.handle(
  new Request(`http://localhost/r/${token}/entries`, {
    method: 'POST',
    body: formData,  // Contains audio file
  })
);

🟠 MEDIUM — Potential SQL Injection in Journal Entry Queries

server/src/db/migrations/004_recipient_flow.sql:1-63 · confidence: low · CWE-89

While the code uses Drizzle ORM (which provides parameterized queries), the gift-store.ts file shows raw SQL in migrations. If any dynamic queries are constructed outside of Drizzle's type-safe API, there could be injection risks. The promptText field is stored as user input without sanitization.

Suggested fix: Ensure all queries use Drizzle's parameterized API. For stored text fields, implement content sanitization if they're rendered in HTML contexts. Use DOMPurify or similar on the frontend when displaying user-generated content.

Code context

CREATE TABLE journal_entries (
  id text PRIMARY KEY,
  recipient_id text NOT NULL,
  prompt_text text,  -- User-controlled, stored as-is
  text text,         -- User-controlled, stored as-is
  // ...
);

🟠 MEDIUM — Missing CSRF Protection on State-Changing Endpoints

server/src/routes/recipient.ts:200-250 · confidence: medium · CWE-352

The recipient flow adds multiple POST/PATCH endpoints that modify state (create entry, patch sharing, set credentials). There's no visible CSRF token validation. If a recipient visits a malicious site while logged in, that site could trigger unwanted state changes.

Suggested fix: Implement CSRF protection: Use SameSite=Strict cookies (already good if configured), add CSRF token validation to POST/PATCH endpoints, or use double-submit cookie pattern. For Elysia: import { csrf } from '@elysiajs/csrf';

Code context

.post('/entries', async ({ params, body }) => {
  // No CSRF token check
  const entry = await createEntry(...);
})

🟠 MEDIUM — Sensitive Data Exposure in Error Messages

server/src/routes/recipient.ts:300-350 · confidence: medium · CWE-209

The recipient login endpoint returns generic error messages (good), but other endpoints may leak information. For example, if a journal entry doesn't exist, the error message might reveal whether the recipient has access to that gift. Error messages should be consistent regardless of the actual reason for failure.

Suggested fix: Return consistent error messages: if (!entry || entry.recipientId !== recipient.id) { set.status = 404; return { error: 'Not found' }; } This prevents attackers from enumerating valid entries.

Code context

// Potential issue - different errors for different failure modes
if (!entry) {
  set.status = 404;
  return { error: 'Entry not found' };  // Reveals entry doesn't exist
}
if (entry.recipientId !== recipient.id) {
  set.status = 403;
  return { error: 'Forbidden' };  // Different error reveals auth failure
}

🟠 MEDIUM — Token-Based Authentication Without Expiration

server/src/db/schema.ts:175-195 · confidence: high · CWE-613

The recipient flow uses access tokens (accessToken field) for authentication, but there's no visible token expiration mechanism. Tokens appear to be long-lived and tied to the gift. If a token is compromised, it provides indefinite access to the recipient's journal.

Suggested fix: Add token expiration: Add tokenExpiresAt field to recipients table, implement token refresh mechanism, or use short-lived JWTs. Example: tokenExpiresAt: timestamp('token_expires_at', { withTimezone: true }).defaultNow().notNull() with logic to refresh on activity.

Code context

export const recipients = pgTable(
  "recipients",
  {
    accessToken: text("access_token").notNull(),
    // No expiresAt or tokenCreatedAt field
    firstSeenAt: timestamp("first_seen_at", { withTimezone: true }),
    lastActiveAt: timestamp("last_active_at", { withTimezone: true }),
  }
);

🟠 MEDIUM — Incomplete Error Handling in Email Sending

server/src/routes/gifts.ts:662-680 · confidence: medium

The email sending logic now returns an InvitationOutcome discriminated union, but the route handler only checks for the 'no-api-key' case. If other error conditions occur (network timeout, invalid email format), they might not be handled consistently across all callers.

Suggested fix: Use exhaustive pattern matching: if (!outcome.sent) { switch(outcome.reason) { case 'no-api-key': ...; default: throw new Error(...); } } Or extend the type to include all possible failure modes and handle each explicitly.

Code context

const outcome = await sendInvitation({...});
if (!outcome.sent && outcome.reason === "no-api-key") {
  console.log(...);
}
// What if outcome.sent is false for other reasons?
// The type system allows for future reasons but they're not handled

🟡 LOW — Test Environment Variable Pollution

server/src/routes/recipient.integration.test.ts:88-100 · confidence: low

The integration tests set process.env.TEST_USER_EMAIL and process.env.OPENAI_API_KEY globally. While the auth-login.integration.test.ts file now properly saves/restores TEST_USER_EMAIL, the recipient.integration.test.ts file sets OPENAI_API_KEY without cleanup. This could affect other tests if they run in the same process.

Suggested fix: Add afterAll cleanup: afterAll(() => { if (!SHOULD_RUN) return; delete process.env.OPENAI_API_KEY; }); Or use a test harness that isolates environment variables per test file.

Code context

beforeAll(() => {
  if (!SHOULD_RUN) return;
  process.env.TEST_USER_EMAIL = TEST_EMAIL;
  process.env.OPENAI_API_KEY = "sk-test-stub";  // No cleanup in afterAll
});

🟡 LOW — Inconsistent Error Response Format

server/src/routes/auth.ts:145-150 · confidence: low

Some endpoints return { error: string } while others might return different error formats. The new recipient login endpoint returns { error: string } but the token envelope returns { error: string }. This inconsistency could confuse API consumers.

Suggested fix: Define a standard error response schema and use it consistently across all endpoints. Example: { status: 'error', code: 'AUTH_FAILED', message: '...' } and apply to all error responses.

Code context

if (!recipient) {
  set.status = 401;
  return { error: "Wrong email or password." };
}

🟡 LOW — Open Redirect Vulnerability Fixed but Incomplete

web/app/oauth/route.ts:47 · confidence: low · CWE-601

The PR fixes an open redirect in web/app/oauth/route.ts by checking if nextHint starts with '/'. However, this check is now optional (nextHint?.startsWith) which could allow null/undefined to bypass the check. The fix is good but could be more explicit.

Suggested fix: The fix is actually correct - if nextHint is null/undefined, the optional chaining returns undefined, which is falsy, so it defaults to '/'. However, for clarity: const baseDest = (nextHint && nextHint.startsWith('/')) ? nextHint : '/' is more explicit.

Code context

const baseDest = nextHint?.startsWith("/") ? nextHint : "/"

---

Reviewed by Slopless · Install on your repo · comment @slopless to re-run