WIP! A BB-style forum, on the ATmosphere! We're still working... we'll be back soon when we have something to show off!
node typescript hono htmx atproto

docs: CLAUDE.md updates

+300
+300
CLAUDE.md
··· 242 242 - Tests that pass locally but fail in CI 243 243 - Tests that require manual setup or specific data 244 244 - Tests with hardcoded timing (`setTimeout`, `sleep`) - use proper mocks 245 + - Placeholder/stub tests that don't actually test anything 246 + 247 + **Placeholder tests are prohibited:** 248 + ```typescript 249 + // ❌ FORBIDDEN: Stub tests provide false confidence 250 + it("assigns role successfully when admin has authority", async () => { 251 + expect(true).toBe(true); // NOT A REAL TEST 252 + }); 253 + 254 + // ✅ REQUIRED: Real tests with actual assertions 255 + it("assigns role successfully when admin has authority", async () => { 256 + const admin = await createUser(ctx, "Admin"); 257 + const member = await createUser(ctx, "Member"); 258 + const moderatorRole = await createRole(ctx, "Moderator", [], 20); 259 + 260 + const res = await app.request(`/api/admin/members/${member.did}/role`, { 261 + method: "POST", 262 + headers: authHeaders(admin), 263 + body: JSON.stringify({ roleUri: moderatorRole.uri }) 264 + }); 265 + 266 + expect(res.status).toBe(200); 267 + const data = await res.json(); 268 + expect(data.roleAssigned).toBe("Moderator"); 269 + 270 + // Verify database state changed 271 + const updatedMember = await getMembership(ctx, member.did); 272 + expect(updatedMember.roleUri).toBe(moderatorRole.uri); 273 + }); 274 + ``` 275 + 276 + **Why this matters:** Stub tests pass in CI, creating false confidence that code is tested. They hide bugs that would be caught by real tests with actual assertions. 277 + 278 + **If you're unsure how to test something:** Leave a `// TODO: Add test for X` comment and create a Linear issue. Never commit a stub test that pretends to test something. 245 279 246 280 ### Example Test Structure 247 281 ··· 356 390 } 357 391 ``` 358 392 393 + ## Middleware Patterns 394 + 395 + ### Middleware Composition 396 + 397 + **CRITICAL: Authentication must precede authorization checks.** 398 + 399 + When using multiple middleware functions, order matters: 400 + 401 + ```typescript 402 + // ✅ CORRECT: requireAuth runs first, sets c.get("user") 403 + app.post( 404 + "/api/topics", 405 + requireAuth(ctx), // Step 1: Restore session, set user 406 + requirePermission(ctx, "createTopics"), // Step 2: Check permission 407 + async (c) => { 408 + const user = c.get("user")!; // Safe - guaranteed by middleware chain 409 + // ... handler logic 410 + } 411 + ); 412 + 413 + // ❌ WRONG: requirePermission runs first, user is undefined 414 + app.post( 415 + "/api/topics", 416 + requirePermission(ctx, "createTopics"), // user not set yet! 417 + requireAuth(ctx), 418 + async (c) => { /* ... */ } 419 + ); 420 + ``` 421 + 422 + **Why this matters:** `requirePermission` depends on `c.get("user")` being set by `requireAuth`. If authentication middleware doesn't run first, permission checks always fail with 401. 423 + 424 + **Testing middleware composition:** 425 + ```typescript 426 + it("middleware chain executes in correct order", async () => { 427 + // Verify requireAuth sets user before requirePermission checks it 428 + const res = await app.request("/api/topics", { 429 + method: "POST", 430 + headers: { Cookie: "atbb_session=valid_token" } 431 + }); 432 + 433 + // Should succeed if both middlewares run in order 434 + expect(res.status).not.toBe(401); 435 + }); 436 + ``` 437 + 359 438 ## Error Handling Standards 360 439 361 440 Follow these patterns for robust, debuggable production code: ··· 405 484 - Re-throw unexpected errors (don't swallow programming bugs like `TypeError`) 406 485 - Log with structured context: operation name, relevant IDs, error message 407 486 - Return user-friendly messages (no stack traces in production) 487 + - Classify errors by user action (400, 503) vs server bugs (500) 408 488 409 489 **DON'T:** 410 490 - Use bare `catch` blocks that hide all error types ··· 412 492 - Fabricate data in catch blocks (return null or fail explicitly) 413 493 - Use empty catch blocks or catch without logging 414 494 495 + **Programming Error Re-Throwing Pattern:** 496 + 497 + ```typescript 498 + // ✅ CORRECT: Re-throw programming errors, catch runtime errors 499 + try { 500 + const result = await ctx.db.select()...; 501 + return processResult(result); 502 + } catch (error) { 503 + // Re-throw programming errors (code bugs) - don't hide them 504 + if (error instanceof TypeError || 505 + error instanceof ReferenceError || 506 + error instanceof SyntaxError) { 507 + console.error("CRITICAL: Programming error detected", { 508 + error: error.message, 509 + stack: error.stack, 510 + operation: "checkPermission" 511 + }); 512 + throw error; // Let global error handler catch it 513 + } 514 + 515 + // Log and handle expected runtime errors (DB failures, network issues) 516 + console.error("Database query failed", { 517 + operation: "checkPermission", 518 + error: error instanceof Error ? error.message : String(error) 519 + }); 520 + return null; // Fail safely for business logic 521 + } 522 + ``` 523 + 524 + **Why re-throw programming errors:** 525 + - `TypeError` = code bug (e.g., `role.permisions.includes()` typo) 526 + - `ReferenceError` = code bug (e.g., using undefined variable) 527 + - `SyntaxError` = code bug (e.g., malformed JSON.parse in your code) 528 + - These should crash during development, not be silently logged 529 + - Catching them hides bugs and makes debugging impossible 530 + 531 + **Error Classification Helper:** 532 + 533 + Create helper functions to classify errors consistently: 534 + 535 + ```typescript 536 + // File: src/lib/errors.ts 537 + export function isProgrammingError(error: unknown): boolean { 538 + return error instanceof TypeError || 539 + error instanceof ReferenceError || 540 + error instanceof SyntaxError; 541 + } 542 + 543 + export function isNetworkError(error: unknown): boolean { 544 + if (!(error instanceof Error)) return false; 545 + const msg = error.message.toLowerCase(); 546 + return msg.includes("fetch failed") || 547 + msg.includes("econnrefused") || 548 + msg.includes("enotfound") || 549 + msg.includes("timeout"); 550 + } 551 + 552 + // Usage in route handlers: 553 + } catch (error) { 554 + if (isProgrammingError(error)) { 555 + throw error; // Don't catch programming bugs 556 + } 557 + 558 + if (isNetworkError(error)) { 559 + return c.json({ 560 + error: "Unable to reach external service. Please try again later." 561 + }, 503); // User should retry 562 + } 563 + 564 + return c.json({ 565 + error: "An unexpected error occurred. Please contact support." 566 + }, 500); // Server bug, needs investigation 567 + } 568 + ``` 569 + 415 570 ### Helper Functions 416 571 417 572 **Validation helpers should:** ··· 530 685 - **404 (Not Found):** Resource doesn't exist (forum, post, user) 531 686 - **503 (Service Unavailable):** Network errors, PDS connection failures, timeouts — user should retry 532 687 - **500 (Internal Server Error):** Unexpected errors, database errors — needs bug investigation 688 + 689 + ## Security-Critical Code Standards 690 + 691 + When implementing authentication, authorization, or permission systems, follow these additional requirements: 692 + 693 + ### 1. Fail-Closed Security 694 + 695 + **Security checks must deny access by default when encountering errors.** 696 + 697 + ```typescript 698 + // ✅ CORRECT: Fail closed - deny access on any error 699 + export async function checkPermission( 700 + ctx: AppContext, 701 + did: string, 702 + permission: string 703 + ): Promise<boolean> { 704 + try { 705 + const [membership] = await ctx.db.select()...; 706 + if (!membership || !membership.roleUri) { 707 + return false; // No membership = deny access 708 + } 709 + 710 + const [role] = await ctx.db.select()...; 711 + if (!role) { 712 + return false; // Role deleted = deny access 713 + } 714 + 715 + return role.permissions.includes(permission) || 716 + role.permissions.includes("*"); 717 + } catch (error) { 718 + if (isProgrammingError(error)) throw error; 719 + 720 + console.error("Failed to check permissions - denying access", { 721 + did, permission, error: ... 722 + }); 723 + return false; // Error = deny access (fail closed) 724 + } 725 + } 726 + 727 + // ❌ WRONG: Fail open - grants access on error 728 + } catch (error) { 729 + console.error("Permission check failed"); 730 + return true; // SECURITY BUG: grants access on DB error! 731 + } 732 + ``` 733 + 734 + **Test fail-closed behavior:** 735 + ```typescript 736 + it("denies access when database query fails (fail closed)", async () => { 737 + vi.spyOn(ctx.db, "select").mockRejectedValueOnce(new Error("DB connection lost")); 738 + 739 + const result = await checkPermission(ctx, "did:plc:user", "createTopics"); 740 + 741 + expect(result).toBe(false); // Prove fail-closed behavior 742 + expect(console.error).toHaveBeenCalledWith( 743 + expect.stringContaining("denying access"), 744 + expect.any(Object) 745 + ); 746 + }); 747 + ``` 748 + 749 + ### 2. Security Test Coverage Requirements 750 + 751 + **All security features require comprehensive tests covering:** 752 + 753 + - ✅ **Happy path** - Authorized user succeeds 754 + - ✅ **Unauthorized user** - Returns 401 (not authenticated) 755 + - ✅ **Forbidden** - Returns 403 (authenticated but lacks permission) 756 + - ✅ **Privilege escalation prevention** - Cannot grant yourself higher privileges 757 + - ✅ **Peer protection** - Cannot modify users with equal authority 758 + - ✅ **Fail-closed behavior** - Database/network errors deny access 759 + - ✅ **Input validation** - Malformed requests return 400, not 500 760 + - ✅ **Error classification** - Network errors (503) vs server errors (500) 761 + 762 + **Example security test suite structure:** 763 + ```typescript 764 + describe("POST /api/admin/members/:did/role (security-critical)", () => { 765 + describe("Authorization", () => { 766 + it("returns 401 when not authenticated", async () => { /* ... */ }); 767 + it("returns 403 when user lacks manageRoles permission", async () => { /* ... */ }); 768 + }); 769 + 770 + describe("Privilege Escalation Prevention", () => { 771 + it("prevents admin from assigning owner role (higher authority)", async () => { 772 + // Admin (priority 10) tries to assign Owner (priority 0) → 403 773 + }); 774 + 775 + it("prevents admin from assigning admin role (equal authority)", async () => { 776 + // Admin (priority 10) tries to assign Admin (priority 10) → 403 777 + }); 778 + 779 + it("allows admin to assign moderator role (lower authority)", async () => { 780 + // Admin (priority 10) assigns Moderator (priority 20) → 200 781 + }); 782 + }); 783 + 784 + describe("Error Handling", () => { 785 + it("returns 503 when PDS connection fails (network error)", async () => { /* ... */ }); 786 + it("returns 500 when database query fails (server error)", async () => { /* ... */ }); 787 + it("returns 400 for malformed roleUri (input validation)", async () => { /* ... */ }); 788 + }); 789 + }); 790 + ``` 791 + 792 + ### 3. Startup Failures for Missing Security Infrastructure 793 + 794 + **Security-critical infrastructure must fail fast on startup, not at first request.** 795 + 796 + ```typescript 797 + // ✅ CORRECT: Throw error on startup if ForumAgent unavailable 798 + export async function seedDefaultRoles(ctx: AppContext) { 799 + const agent = ctx.forumAgent; 800 + if (!agent) { 801 + console.error("CRITICAL: ForumAgent not available - role system non-functional", { 802 + operation: "seedDefaultRoles", 803 + forumDid: ctx.config.forumDid 804 + }); 805 + throw new Error( 806 + "Cannot seed roles without ForumAgent - permission system would be broken" 807 + ); 808 + } 809 + // ... seeding logic 810 + } 811 + 812 + // ❌ WRONG: Silent failure allows server to start without roles 813 + if (!agent) { 814 + console.warn("ForumAgent not available, skipping role seeding"); 815 + return { created: 0, skipped: 0 }; // Server starts but is broken! 816 + } 817 + ``` 818 + 819 + **Why this matters:** If the permission system is broken, every request will fail authorization. It's better to fail startup loudly than silently deploy a non-functional system. 820 + 821 + ### 4. Security Code Review Checklist 822 + 823 + Before requesting review for authentication/authorization code, verify: 824 + 825 + - [ ] All permission checks fail closed (deny access on error) 826 + - [ ] Database errors in security checks are caught and logged 827 + - [ ] Programming errors (TypeError) are re-thrown, not caught 828 + - [ ] Privilege escalation is prevented (equal/higher authority blocked) 829 + - [ ] Tests cover unauthorized (401), forbidden (403), and error cases 830 + - [ ] Error messages don't leak internal details (priority values, permission names) 831 + - [ ] Middleware composition is correct (auth before permission checks) 832 + - [ ] Startup fails fast if security infrastructure is unavailable 533 833 534 834 ## Documentation & Project Tracking 535 835