Merge PRs #1, #2, #3, #4, #5, #6, #7, #8, #10, #11, #12, #13, #14 into main#15
Conversation
- Remove hardcoded fallback for JWT_SECRET in production - Restrict CORS origins to trusted domains only - Add security journaling in .jules/sentinel.md Co-authored-by: davidraehles <6085055+davidraehles@users.noreply.github.com>
Implemented `saveNotesToDB` in the offline service to allow saving multiple notes in a single transaction. This significantly improves performance during the initial load and sync processes by reducing transaction overhead from O(N) to O(1). - Added `saveNotesToDB` to `frontend/src/services/offline.ts` - Updated `loadNotesIntent` in `frontend/src/intents/notesIntents.ts` to use bulk save - Added unit tests in `frontend/src/services/__tests__/offline.test.ts` - Documented learning in `.jules/bolt.md` Co-authored-by: davidraehles <6085055+davidraehles@users.noreply.github.com>
- Add keyboard navigation (tabIndex, role="button", onKeyDown) to note list - Add :focus-visible styles for better visual feedback - Add auto-focus to title input on new note creation - Add ARIA labels to improve screen reader support - Fix unused import lint error in NoteEditor.tsx Co-authored-by: davidraehles <6085055+davidraehles@users.noreply.github.com>
…dleware - Installed vitest in the backend - Added "test" script to backend/package.json - Created backend/src/middleware/__tests__/auth.test.ts - Implemented tests for authenticateToken and generateToken - Verified tests pass and catch regressions Co-authored-by: davidraehles <6085055+davidraehles@users.noreply.github.com>
- Added `saveNotesToDB` to `offline.ts` to support bulk writes in a single transaction. - Modified `loadNotesIntent` in `notesIntents.ts` to use `saveNotesToDB` instead of sequential `saveNoteToDB` calls. - Established a benchmark demonstrating ~8x performance improvement for bulk writes. Co-authored-by: davidraehles <6085055+davidraehles@users.noreply.github.com>
- Replaced bcryptjs with bcrypt in backend/package.json - Updated backend/src/routes/auth.ts to use bcrypt - Fixed pre-existing type errors in backend/src/routes/notes.ts for Express 5 compatibility - Documented performance gains in .jules/bolt.md Co-authored-by: davidraehles <6085055+davidraehles@users.noreply.github.com>
- Removed 'http://localhost:3000' fallback for VITE_API_URL in frontend/src/services/api.ts - Added runtime check to throw an error if VITE_API_URL is not defined - Created frontend/.env.example to document required environment variables - Updated frontend/src/test/setup.ts to mock VITE_API_URL for tests Co-authored-by: davidraehles <6085055+davidraehles@users.noreply.github.com>
Introduced a centralized logger utility in `backend/src/utils/logger.ts` and updated all instances of `console.log` and `console.error` in the backend to use this utility. This improves maintainability and allows for easier future upgrades to a full logging library. Changes: - Created `backend/src/utils/logger.ts` - Updated `backend/src/routes/auth.ts` - Updated `backend/src/routes/notes.ts` - Updated `backend/src/server.ts` - Added `backend/pnpm-lock.yaml` (automatically generated during verification) Co-authored-by: davidraehles <6085055+davidraehles@users.noreply.github.com>
- Centralized Prisma Client instantiation in `backend/src/lib/prisma.ts`. - Updated `auth.ts` and `notes.ts` routes to use the shared singleton instance. - Fixed pre-existing TypeScript errors in `notes.ts` caused by Express 5's broad typing of `req.params`. - Documented performance learnings in `.jules/bolt.md`. Co-authored-by: davidraehles <6085055+davidraehles@users.noreply.github.com>
Replaced direct console.log and console.error usage with a centralized logger utility in backend/src/utils/logger.ts. This improves maintainability and provides a single point of control for logging. Co-authored-by: davidraehles <6085055+davidraehles@users.noreply.github.com>
- Restricted CORS origins using ALLOWED_ORIGINS env var - Added robust whitespace handling for comma-separated origins - Defaulted to http://localhost:5173 for local development Co-authored-by: davidraehles <6085055+davidraehles@users.noreply.github.com>
Removed the insecure fallback for JWT_SECRET in the authentication middleware. The application now throws an error at startup if the JWT_SECRET environment variable is not defined. Updated README.md to reflect this requirement. Co-authored-by: davidraehles <6085055+davidraehles@users.noreply.github.com>
This commit moves authentication token storage from `localStorage` to secure, `HttpOnly` cookies to mitigate XSS risks. - Updated backend to set `auth_token` cookie on login/register. - Added `/api/auth/me` for session recovery and `/api/auth/logout` to clear cookies. - Configured CORS and `cookie-parser` for secure cookie handling. - Updated frontend `ApiClient` to use `credentials: 'include'` and removed `localStorage` usage. - Added `checkAuthIntent` to restore session on app load. Co-authored-by: davidraehles <6085055+davidraehles@users.noreply.github.com>
….yaml, add fallback for bulk save
…teractive roles, and focus visibility
…lear auth state on failure
… and CORS origins
- Updated `saveNotesToDB` to use `Promise.all` for `put` operations. - Added try/catch with fallback to sequential save in `loadNotesIntent`. - Removed `pnpm-lock.yaml` from the repository. Co-authored-by: davidraehles <6085055+davidraehles@users.noreply.github.com>
…utdown, remove pnpm-lock.yaml
- Prevent keydown event bubbling from nested buttons in NoteList - Remove invalid nested role="button" from NoteList items - Maintain keyboard accessibility via tabIndex and custom onKeyDown handler Co-authored-by: davidraehles <6085055+davidraehles@users.noreply.github.com>
- Removed pnpm-lock.yaml - Updated package-lock.json using npm install - Verified bcrypt implementation with npm dependencies Co-authored-by: davidraehles <6085055+davidraehles@users.noreply.github.com>
- Require JWT_SECRET and ALLOWED_ORIGINS when NODE_ENV is 'production' or undefined - Improve CORS configuration with origin trimming - Update security journal with latest learnings Co-authored-by: davidraehles <6085055+davidraehles@users.noreply.github.com>
- Removed sequential await in `saveNotesToDB` loop for better performance. - Added explicit assertion for single-transaction usage in `offline.test.ts`. Co-authored-by: davidraehles <6085055+davidraehles@users.noreply.github.com>
- Updated Prisma singleton to use globalThis caching for dev hot-reloads. - Removed backend/pnpm-lock.yaml to keep the PR focused. - Fixed TypeScript errors in notes.ts routes. Co-authored-by: davidraehles <6085055+davidraehles@users.noreply.github.com>
Co-authored-by: davidraehles <6085055+davidraehles@users.noreply.github.com>
- Removed backend/pnpm-lock.yaml - Updated logger rest parameters to use unknown[] instead of any[] Co-authored-by: davidraehles <6085055+davidraehles@users.noreply.github.com>
…re tests - Added test case for malformed authorization header (no Bearer prefix) - Added expiration assertions to generateToken test Co-authored-by: davidraehles <6085055+davidraehles@users.noreply.github.com>
- Added credentials: true to support cookie-based auth - Added filtering to remove empty strings from allowedOrigins list Co-authored-by: davidraehles <6085055+davidraehles@users.noreply.github.com>
…check with credentials and filtering
…487153461403' into combine/merge-prs-1-2-3-4-5-6-7-8-10-11-12-13-14
…pt + Prisma singleton import
…61664679516936' into combine/merge-prs-1-2-3-4-5-6-7-8-10-11-12-13-14
…er signatures with error variable naming
…-fallback-8975384767320290455' into combine/merge-prs-1-2-3-4-5-6-7-8-10-11-12-13-14
…are-7652412391144490455' into combine/merge-prs-1-2-3-4-5-6-7-8-10-11-12-13-14
…ate saveNotesToDB, keep fallback logic
…ts-9932565411486648363' into combine/merge-prs-1-2-3-4-5-6-7-8-10-11-12-13-14
…ve bcrypt + prisma singleton + logger + cookie-parser
…files - Add GitHub Actions CI with backend and frontend jobs - Fix verbatimModuleSyntax type-only import errors - Fix TS 5.9 BufferSource/HeadersInit type compatibility issues - Fix vite.config.ts vitest type reference - Fix ESLint errors (unused vars, no-explicit-any in tests) - Remove stray pnpm-lock.yaml files - Update backend package-lock.json for cookie-parser + bcrypt
There was a problem hiding this comment.
Pull request overview
This pull request consolidates 13 individual PRs into main, implementing critical security fixes, performance optimizations, accessibility improvements, comprehensive testing, and CI/CD infrastructure. The changes address multiple security vulnerabilities (hardcoded JWT secret, permissive CORS, localStorage token storage), significantly improve application performance (native bcrypt, Prisma singleton, IndexedDB bulk operations), and establish a solid foundation for code quality with automated testing and CI workflows.
Changes:
- Security hardening: Removed hardcoded JWT secret fallbacks, implemented environment-based CORS restrictions with origin whitelisting, migrated from localStorage to HttpOnly cookies for authentication tokens
- Performance optimizations: Replaced bcryptjs with native bcrypt (~38% faster hashing), implemented Prisma Client singleton to prevent connection pool exhaustion, optimized IndexedDB writes with bulk transactions (~8x speedup)
- Accessibility & UX: Added keyboard navigation (Enter/Space), ARIA labels, focus management for note list and editor, automatic focus on new note creation
- Testing & quality: Added comprehensive auth middleware tests, IndexedDB offline service tests, TypeScript 5.9 compatibility fixes, centralized backend logging, established CI workflows
- Documentation: Added security learnings (.jules/sentinel.md), performance learnings (.jules/bolt.md), accessibility learnings (.Jules/palette.md), .env.example files, updated README with proper placeholder secrets
Reviewed changes
Copilot reviewed 29 out of 31 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| backend/src/middleware/auth.ts | Removed hardcoded JWT secret, added cookie-based authentication fallback |
| backend/src/routes/auth.ts | Migrated to native bcrypt, added HttpOnly cookie setting, added /me and /logout endpoints |
| backend/src/routes/notes.ts | Imported Prisma singleton, added centralized logging, Express 5 type casting |
| backend/src/server.ts | Implemented CORS origin whitelist with environment validation, added cookie-parser middleware |
| backend/src/lib/prisma.ts | New Prisma Client singleton implementation |
| backend/src/utils/logger.ts | New centralized logging utility |
| backend/src/middleware/tests/auth.test.ts | New comprehensive auth middleware tests |
| backend/package.json | Replaced bcryptjs with bcrypt, added cookie-parser and vitest dependencies |
| frontend/src/services/api.ts | Removed localStorage token storage, added credentials: include, added getMe/logout endpoints |
| frontend/src/services/offline.ts | Added bulk save function for IndexedDB optimization |
| frontend/src/services/encryption.ts | TypeScript 5.9 BufferSource compatibility casts |
| frontend/src/intents/authIntents.ts | Added checkAuthIntent and logoutIntent with proper error handling |
| frontend/src/intents/notesIntents.ts | Integrated bulk IndexedDB save with fallback, removed unused error variables |
| frontend/src/views/NoteList.tsx | Added keyboard navigation, ARIA labels, accessibility improvements |
| frontend/src/views/NoteEditor.tsx | Added auto-focus on new note, ARIA labels for inputs |
| frontend/src/test/setup.ts | Added VITE_API_URL environment variable mocking |
| frontend/src/App.tsx | Added checkAuthIntent on app initialization |
| frontend/src/index.css | Added focus-visible styles for accessibility |
| frontend/.env.example | New environment variable documentation |
| .github/workflows/ci.yml | New CI pipeline for backend and frontend |
| .jules/sentinel.md | Security best practices documentation |
| .jules/bolt.md | Performance optimization documentation |
| .Jules/palette.md | Accessibility learning documentation |
| README.md | Updated JWT_SECRET examples to use placeholders |
Files not reviewed (2)
- backend/package-lock.json: Language not supported
- frontend/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| expect(req.userId).toBe(userId); | ||
| expect(next).toHaveBeenCalled(); | ||
| expect(res.status).not.toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
The authentication middleware now supports cookie-based authentication (line 30-32 in auth.ts), but the test suite doesn't include test cases for this scenario. Consider adding a test that verifies the middleware can successfully authenticate using a token from req.cookies.auth_token when no Authorization header is present.
| }); | |
| }); | |
| it('should authenticate using auth_token cookie when no authorization header is present', () => { | |
| // No Authorization header set | |
| (req as any).cookies = { auth_token: 'valid-cookie-token' }; | |
| const userId = 'user-cookie-123'; | |
| vi.spyOn(jwt, 'verify').mockImplementation((token, secret, callback: any) => { | |
| callback(null, { userId }); | |
| }); | |
| authenticateToken(req as AuthRequest, res as Response, next); | |
| expect(req.userId).toBe(userId); | |
| expect(next).toHaveBeenCalled(); | |
| expect(res.status).not.toHaveBeenCalled(); | |
| }); |
| // Require ALLOWED_ORIGINS in production | ||
| if (!process.env.ALLOWED_ORIGINS && (!process.env.NODE_ENV || process.env.NODE_ENV === 'production')) { | ||
| throw new Error('ALLOWED_ORIGINS environment variable is required in production'); |
There was a problem hiding this comment.
The ALLOWED_ORIGINS check on line 18 uses !process.env.NODE_ENV || process.env.NODE_ENV === 'production' which means it requires ALLOWED_ORIGINS when NODE_ENV is undefined OR when it's 'production'. This is correct for production safety, but the logic might be clearer as process.env.NODE_ENV === 'production' since undefined NODE_ENV typically indicates production. However, the current implementation is actually safer by being more restrictive. Consider documenting this behavior or simplifying to just check for production if you want to allow development flexibility.
| // Require ALLOWED_ORIGINS in production | |
| if (!process.env.ALLOWED_ORIGINS && (!process.env.NODE_ENV || process.env.NODE_ENV === 'production')) { | |
| throw new Error('ALLOWED_ORIGINS environment variable is required in production'); | |
| // Require ALLOWED_ORIGINS in production and when NODE_ENV is not set (treat unset as production for safety) | |
| if (!process.env.ALLOWED_ORIGINS && (!process.env.NODE_ENV || process.env.NODE_ENV === 'production')) { | |
| throw new Error('ALLOWED_ORIGINS environment variable is required in production or when NODE_ENV is not set'); |
| ## 2025-02-18 - [Accessibility for Custom List Items] | ||
| **Learning:** In minimalist designs where list items are used as primary navigation/selection elements, they often lack keyboard accessibility. Using `role="button"` and `tabIndex={0}` is essential, but it must be paired with an `onKeyDown` handler for both 'Enter' and ' ' (Space) keys to meet user expectations. | ||
| **Action:** Always check if clickable `div`s have corresponding keyboard handlers and ARIA roles. | ||
|
|
||
| ## 2025-02-18 - [Visual Focus for Keyboard Users] | ||
| **Learning:** Default browser focus outlines can be inconsistent or clash with minimalist black/white designs. Using `:focus-visible` with a high-contrast `outline` and `outline-offset` provides a clear indicator for keyboard users while remaining invisible for mouse users. | ||
| **Action:** Implement custom `:focus-visible` styles for all interactive elements to ensure a polished keyboard navigation experience. |
There was a problem hiding this comment.
Two directories with the same name but different casing exist: .jules (containing bolt.md and sentinel.md) and .Jules (containing palette.md). This will cause issues on case-insensitive filesystems like macOS and Windows (NTFS default). These should be consolidated into a single directory with consistent casing. Recommend using .jules (lowercase) as the standard and moving palette.md into it.
| ## 2025-02-18 - [Accessibility for Custom List Items] | |
| **Learning:** In minimalist designs where list items are used as primary navigation/selection elements, they often lack keyboard accessibility. Using `role="button"` and `tabIndex={0}` is essential, but it must be paired with an `onKeyDown` handler for both 'Enter' and ' ' (Space) keys to meet user expectations. | |
| **Action:** Always check if clickable `div`s have corresponding keyboard handlers and ARIA roles. | |
| ## 2025-02-18 - [Visual Focus for Keyboard Users] | |
| **Learning:** Default browser focus outlines can be inconsistent or clash with minimalist black/white designs. Using `:focus-visible` with a high-contrast `outline` and `outline-offset` provides a clear indicator for keyboard users while remaining invisible for mouse users. | |
| **Action:** Implement custom `:focus-visible` styles for all interactive elements to ensure a polished keyboard navigation experience. | |
| # File moved | |
| This file has been moved to `.jules/palette.md` to ensure consistent directory casing | |
| across platforms (for example, macOS and Windows, which use case-insensitive | |
| filesystems by default). | |
| Please update any references from `.Jules/palette.md` to `.jules/palette.md`. | |
| Once all references are updated, the `.Jules` directory can be safely removed. |
|
|
||
| - name: Run tests | ||
| working-directory: frontend | ||
| run: npx vitest run |
There was a problem hiding this comment.
The frontend test step (line 79-81) does not set the VITE_API_URL environment variable, but the test setup (frontend/src/test/setup.ts) mocks it using vi.stubEnv. However, for consistency and to ensure tests can run in CI without relying solely on mocks, consider adding the environment variable to this step similar to the build step (line 77).
| run: npx vitest run | |
| run: npx vitest run | |
| env: | |
| VITE_API_URL: http://localhost:3000 |
…dist - Update backend/tsconfig.json to exclude tests from compilation - Add backend/vitest.config.ts to explicitly ignore dist directory - Fixes backend CI failure where Vitest tried to run compiled CommonJS tests
Summary
Combined merge of all 13 open PRs into main, with conflict resolution, CI setup, and code quality fixes.
PRs Included
Additional Changes
.github/workflows/ci.ymlwith backend and frontend jobs (type check, lint, build, tests)verbatimModuleSyntaxtype-only imports, TS 5.9BufferSource/HeadersInitcompatibility, vitest config typesno-explicit-anyin test mocks)pnpm-lock.yamlfiles from both backend and frontendConflict Resolution
JWT_SECRETcredentials: true+ empty origin filteringbcryptfrom PR ⚡ Bolt: Replace bcryptjs with native bcrypt #6 with Prisma singleton import from PR ⚡ Bolt: Implement Prisma Client singleton #10(message: string, ...args: unknown[])signaturessaveNotesToDB, keptPromise.allpattern with per-item fallbackVerification
Closes #1, closes #2, closes #3, closes #4, closes #5, closes #6, closes #7, closes #8, closes #10, closes #11, closes #12, closes #13, closes #14