feat: resolve 10 implementable tech debt TODOs#1
Merged
Conversation
Add GET /buckets/{id}/snapshots/{snapshotId}/download to stream
snapshot binary data with path traversal protection and SHA-256
response header. Includes tests for content match, wrong bucket (403),
non-existent snapshot (404), and unauthorized access.
Write snapshot to .tmp file first, rename to final name only after DB commit succeeds. On failure, temp file is deleted. Add periodic cleanup of orphan .tmp files older than 1 hour in both snapshot and blob storage directories.
Add IP-based rate limiter allowing 5 registrations per IP per hour for the POST /register endpoint. Returns 429 when limit exceeded. Follows the same ConcurrentHashMap/IpWindow pattern used for WebSocket rate limiting.
Support ?token=... query parameter for WebSocket authentication in addition to in-band auth messages. Query param auth rejects invalid tokens immediately with close code 4001. Absent query param falls through to existing in-band auth for backward compatibility.
Add PATCH /buckets/{id}/creator endpoint to transfer bucket ownership
to another device with active access. Validates caller is current
creator, target has non-revoked access. Tests cover successful
transfer, unauthorized attempts, revoked targets, and post-transfer
permission enforcement.
Session validation now joins Sessions with Devices table to read the current signing key, eliminating the redundant signingKey column in Sessions. createSession writes empty string to the column (retained for schema compatibility). This ensures sessions always return the device's current key, avoiding stale key issues after rotation.
Add CheckpointAcknowledgments table for per-device checkpoint tracking.
Add POST /buckets/{id}/checkpoints/acknowledge endpoint. Implement
pruneAcknowledgedOps() that deletes ops covered by checkpoints where
all active devices have acknowledged, preserving the latest checkpoint's
ops as safety margin. Pruning runs in the periodic cleanup loop.
- Add ktor-client-websockets test dependency for WebSocket integration tests - Reset DeviceRegistrationRateLimiter on module init for test isolation - Use proper imports instead of FQN in pruneAllBuckets - Fix WebSocket auth test to not depend on encodeDefaults=false type field - Fix deprecated readBytes -> readRawBytes in SnapshotDownloadTest
Switch QR invite generation and verification to use the device's signing key fingerprint instead of the encryption key fingerprint, aligning with the pairing spec. Both generateInvite() and continueJoinBucket() now consistently use signingKey for fingerprint computation.
…(SEC6-A-16) Add CalendarEventDao and InfoBankDao to SnapshotUseCase and include their entities in computeStateHash(). Calendar events are sorted by eventId; info bank entries are sorted by entryId and filtered to non-deleted via the DAO query. This ensures state divergence in these entity types is detected by snapshot comparison.
…on (SEC3-A-21) Implement an OkHttp Authenticator that transparently handles 401 responses by re-authenticating via Ed25519 challenge-response and retrying the failed request. Features: infinite loop prevention via X-Auth-Retry header, synchronized concurrent 401 handling so only one re-auth occurs, and dagger.Lazy<AuthRepository> to break the circular Hilt dependency chain.
Update 7 remaining TODO/SEC comments to use DEFERRED prefix and explain why each item requires protocol or framework changes before implementation.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
detekt found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
There was a problem hiding this comment.
Pull request overview
This pull request implements 10 actionable tech debt items identified across 7 security review cycles, plus clarifies 7 deferred TODOs with rationale. The changes span both server (Kotlin/Ktor) and Android (Kotlin) codebases, addressing security, reliability, and operational concerns.
Changes:
- Server: 7 implementations including snapshot download endpoint, crash atomicity for uploads, IP rate limiting, WebSocket query param auth, creator role transfer, Sessions table redundancy removal, and op table pruning
- Android: 3 implementations covering QR code signing key fingerprints, snapshot hash entity coverage, and automatic 401 re-authentication
- Documentation: 7 deferred TODOs converted to DEFERRED(...) format with clear rationale
Reviewed changes
Copilot reviewed 29 out of 29 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| server/src/main/kotlin/dev/kidsync/server/routes/SyncRoutes.kt | Implements snapshot download endpoint with path traversal protection, WebSocket query param auth, and crash-atomic snapshot uploads using temp files |
| server/src/main/kotlin/dev/kidsync/server/routes/DeviceRoutes.kt | Adds IP-based rate limiter (5 registrations/IP/hour) for device registration |
| server/src/main/kotlin/dev/kidsync/server/routes/BucketRoutes.kt | Adds PATCH /buckets/{id}/creator endpoint for creator role transfer |
| server/src/main/kotlin/dev/kidsync/server/services/SyncService.kt | Implements op table pruning after checkpoint acknowledgment with safety margin |
| server/src/main/kotlin/dev/kidsync/server/services/BucketService.kt | Adds transferCreator method with validation |
| server/src/main/kotlin/dev/kidsync/server/util/SessionUtil.kt | Removes Sessions.signingKey redundancy by joining with Devices table |
| server/src/main/kotlin/dev/kidsync/server/db/Tables.kt | Adds CheckpointAcknowledgments table for tracking per-device checkpoint acknowledgments |
| server/src/main/kotlin/dev/kidsync/server/Application.kt | Adds periodic cleanup for .tmp files and op pruning, documents XForwardedHeaders limitation |
| android/app/src/main/java/com/kidsync/app/ui/viewmodel/BucketViewModel.kt | Updates QR code generation and verification to use signing key fingerprint instead of encryption key |
| android/app/src/main/java/com/kidsync/app/domain/usecase/sync/SnapshotUseCase.kt | Adds CalendarEvent and InfoBankEntry entities to snapshot hash computation |
| android/app/src/main/java/com/kidsync/app/data/remote/interceptor/TokenAuthenticator.kt | Implements OkHttp Authenticator for automatic 401 re-authentication with synchronized concurrent handling |
| server/src/test/kotlin/dev/kidsync/server/*.kt | Adds 24 new test cases covering all server implementations |
| android/app/src/test/java/com/kidsync/app/*.kt | Adds/updates tests for Android changes with comprehensive coverage |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Replace raw HTTP status code literals with HttpStatusCode.*.value - Add named constants for WebSocket close codes (4000/4001/4003) - Replace wildcard imports with explicit imports in test files - Remove unused uploadOpsAndGetCheckpoints function from OpPruningTest - Fix unused targetAccess variable in BucketService (use .any() instead) - Add @Suppress("ThrowsCount") to transferCreator() - Fix BucketViewModelTest: use `returns Result.success(Unit)` instead of `just Runs` for uploadKeyAttestation() which returns Result<Unit>
- Declare WebSocket close code constants as Short (CloseReason ctor requires Short, not Int) - Use api.kidsync.app in BucketViewModelTest URLs so the host check doesn't trigger android.util.Log.w which throws in unit tests
- WebSocketQueryParamAuthTest.kt: expand 4 wildcard imports - TokenAuthenticatorTest.kt: expand io.mockk.* wildcard - BucketViewModelTest.kt: expand io.mockk.* wildcard
- SnapshotUseCase: include childId and title in InfoBankEntry hash computation (missing fields could cause hash collisions) - Snapshot download: use stored SHA-256 from DB instead of reading entire file into memory to recompute it - Snapshot download: add .tmp file recovery for crash between DB commit and rename (complements existing periodic .tmp cleanup)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements all 10 actionable tech debt items identified across 7 security review cycles, plus clarifies 7 deferred TODO comments with rationale.
Server (S1-S7) — 456 tests passing (+24 new)
GET /buckets/{id}/snapshots/{snapshotId}/download) with path traversal protection and SHA-256 integrity header.tmpcleanup)?token=for immediate auth, backward-compatible with in-band)PATCH /buckets/{id}/creator)Sessions.signingKeyredundancy (join toDevicestable for authoritative key)CheckpointAcknowledgmentstable, periodic cleanup)Android (A1-A3)
TokenAuthenticatorfor automatic 401 re-authentication with synchronized concurrent handling (SEC3-A-21)Deferred TODOs
DEFERRED(...)prefix with clear rationale for why each requires protocol/framework changesTest plan