Conversation
…removal Remediate 95 code review findings across 7 phases: - Add 12 shared server utils and 2 client hooks to eliminate duplication - Harden security: path traversal, FTS injection, CORS, URL validation, body whitelisting - Fix bugs: infinite loops, memory leaks, stale closures, missing cleanup, type errors - Wrap async routes with error handler, wire up request timeout middleware - Remove CI auto-increment of version numbers (now handled in dev commits) - Add PWA support (favicon, manifest, apple-touch-icon)
There was a problem hiding this comment.
Pull request overview
This PR implements comprehensive security hardening, code deduplication, and bug fixes across the SparseTree genealogy application. The changes address 95 findings from a codebase review, reducing code by 227 net lines while improving security posture and maintainability.
Changes:
- Security hardening: path traversal defense, FTS5 injection prevention, CORS restrictions, request body whitelisting, URL validation
- DRY cleanup: 12 shared server utilities and 2 client hooks to eliminate duplicate code patterns
- Bug fixes: database transaction wrapping, infinite loop guards, cache eviction fixes, client-side cleanup improvements
- CI/CD improvements: removed auto-versioning, downgraded permissions
- PWA support: added manifest, icons, and meta tags
Reviewed changes
Copilot reviewed 92 out of 99 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| server/src/utils/validation.ts | New security utilities for ID, URL, and FTS sanitization |
| server/src/utils/paths.ts | Centralized path constants and helpers |
| server/src/routes/augmentation.routes.ts | Path traversal defense with resolve().startsWith() checks |
| server/src/routes/browser.routes.ts | Request sanitization and path validation |
| server/src/db/migrations/002_expanded_facts.ts | Fixed multi-row bug in vital event joins |
| server/src/services/path.service.ts | Added infinite loop guard with MAX_ITERATIONS |
| scripts/migrate-to-sqlite.ts | Wrapped migration in transactions |
| client/src/components/person/PersonDetail.tsx | AbortController cleanup and null parent filtering |
| server/src/index.ts | CORS origin restriction via environment variable |
| shared/src/index.ts | Fixed Person.parents type to allow null values |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 92 out of 99 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…detection, consistent null types
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 92 out of 99 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
Comprehensive code quality remediation addressing 95 findings from a full codebase review across ~54k LOC. Net result: 99 files changed, -227 lines (1691 added, 1918 removed).
Security Hardening
origin: '*'with configurableCORS_ORIGINenv var (defaults tohttp://localhost:6373)pickFields()instead of spreadingreq.bodyurl.includes('domain')with proper URL constructor-based validationDRY Cleanup (12 shared server utils + 2 client hooks)
paths.ts— centralizedDATA_DIR,PHOTOS_DIR,findPhoto(),getPhotoPath()(replaces 20+ local declarations)downloadImage.ts— unified image downloader (replaces 3 implementations)parseYear.ts— year parsing with BC/formal date support (replaces 4 duplications)validation.ts— ID, URL, FTS sanitization utilitiesasyncHandler.ts— Express async route wrappersseHelpers.ts— SSE header setup (replaces 7 duplicate blocks)operationTracker.ts— cancellable operation lifecycle (replaces 3 duplicate patterns)resolveCanonical.ts— ULID validation + ID resolution (replaces 15 blocks in person routes)browserConnect.ts— browser verify/reconnect/launch (replaces 3 blocks)useBrowserConnection+useSSEclient hooksBug Fixes
process.exit(1)with typed error (was crashing the entire server)MAX_ITERATIONSdiscoveryRuns/ResultsMaps with oldest-evictionperson_computedview multi-row bug in vital event joinsuseCallbacktypeof gen !== '?'→gen != null && gen !== '?'--dry-runflagCI/CD
contents: writetocontents: readOther
requestTimeoutmiddlewarePerson.parentstype:string[]→(string | null)[]import.meta.dirnameand shared type importsSelf-Review Checklist
Test Plan
npm run buildpasses across all packagesnpm run test:unitpassesnpm run test:integrationpassescurl http://localhost:6374/api/augment/..%2F..%2Fetc%2Fpasswd/wiki-photoreturns 404/400test OR 1=1returns results without errorsCORS_ORIGINset)