diff --git a/android/app/build.gradle.kts b/android/app/build.gradle.kts index cc5e220..f745b89 100644 --- a/android/app/build.gradle.kts +++ b/android/app/build.gradle.kts @@ -15,6 +15,8 @@ val keystoreProperties = Properties() val keystorePropertiesFile = rootProject.file("key.properties") if (keystorePropertiesFile.exists()) { keystoreProperties.load(FileInputStream(keystorePropertiesFile)) +} else { + logger.warn("WARNING: key.properties not found — release builds will use debug signing!") } android { diff --git a/docs/SECURITY_REVIEW.md b/docs/SECURITY_REVIEW.md index 9c21988..699e7a3 100644 --- a/docs/SECURITY_REVIEW.md +++ b/docs/SECURITY_REVIEW.md @@ -408,3 +408,178 @@ Create `android/app/proguard-rules.pro` with rules to keep Flutter engine classe | **LOW** | Add `maxLength` to task name inputs | Trivial | | **LOW** | Add `android:allowBackup="false"` | Trivial | | **LOW** | Update `share_plus` to latest | Medium (breaking changes) | + +--- + +## Round 2 (2026-02-16) + +### Previous Round Verification + +**Fixed (9 of 17):** +- [x] MED-1: URL scheme validation in leaf detail — verified fixed. New `isAllowedUrl()` utility in `display_utils.dart:17-22` with http/https allowlist. Used at `leaf_task_detail.dart:49`. +- [x] MED-2: URL scheme validation in AppBar — verified fixed. Same `isAllowedUrl()` check at `task_list_screen.dart:735`. +- [x] MED-3: Minimal schema validation on DB import — verified fixed. `_validateBackup()` at `database_helper.dart:172-214` now checks: file size, all 3 expected tables, schema version 1-11, no triggers/views. +- [x] MED-4: No file size limit on import — verified fixed. 100 MB limit at `database_helper.dart:166,174-177`. +- [x] LOW-1: No URL validation on save — verified fixed. `showEditUrlDialog` checks `isAllowedUrl()` at `leaf_task_detail.dart:102,130`. Auto-prepends `https://` via `normalizeUrl()`. +- [x] LOW-2: No input length limits (single-task) — verified fixed. `maxLength: 500` at `add_task_dialog.dart:48` and `task_list_screen.dart:207` (rename dialog). +- [x] LOW-3: Malicious triggers/views — verified fixed. Trigger/view check at `database_helper.dart:204-209`. +- [x] LOW-5: `int.parse` without error handling — verified fixed. `int.tryParse` with `.whereType()` at `todays_five_screen.dart:47,49`. +- [x] LOW-8: Missing `android:allowBackup="false"` — verified fixed. Present at `AndroidManifest.xml:6`. + +**Not fixed (5 of 17):** +- [ ] HIGH-1: `file_picker` still at 8.3.7 (pubspec.yaml specifies `^8.1.7`, lock file has `8.3.7`). CVE patches are in 10.3.9+. +- [ ] MED-5: No R8/ProGuard — `build.gradle.kts:51-57` still has no `isMinifyEnabled = true`. +- [ ] LOW-4: No pre-import backup of existing data — `importDatabase` still overwrites directly. +- [ ] LOW-9: Release signing silently falls back to debug key. +- [ ] LOW-10: `firstWhere` without `orElse` — still present at `task_provider.dart:98, 176, 187`. + +**Accepted / Deferred (3 of 17):** +- LOW-6: Unencrypted DB at rest — accepted for threat model. +- LOW-7: Unencrypted backup export — accepted for threat model. +- LOW-11: `share_plus` outdated (10.1.4 vs 12.x) — deferred, no CVEs. + +### Findings + +No new critical or high findings. + +#### MED-6: Brain Dump Dialog Has No Per-Line or Total Length Limits + +- **Severity:** Medium +- **File:** `lib/widgets/brain_dump_dialog.dart:67-77` +- **Code:** + ```dart + TextField( + controller: _controller, + maxLines: 8, + minLines: 4, + autofocus: true, + textInputAction: TextInputAction.newline, + decoration: const InputDecoration( + hintText: 'Buy groceries\nCall dentist\nFinish report\n...', + border: OutlineInputBorder(), + ), + ), + ``` + +**Description:** While the single-task `AddTaskDialog` was fixed with `maxLength: 500` (LOW-2), the brain dump dialog still has no input limits. A user could paste thousands of lines or megabyte-length strings, each of which becomes a separate task and DB INSERT. This creates two risks: +1. **Performance DoS**: Pasting 10,000+ lines triggers 10,000 INSERTs in a single transaction, potentially freezing the UI. +2. **Per-line length**: Individual lines have no 500-char cap, so extremely long task names bypass the limit enforced elsewhere. + +**Recommended Fix:** +```dart +TextField( + controller: _controller, + maxLines: 8, + minLines: 4, + maxLength: 25000, // ~50 lines of 500 chars + decoration: const InputDecoration( + counterText: '', // hide counter + // ... + ), +), +``` +And in `_parseNames()`, truncate each line: +```dart +.map((l) => l.trim().length > 500 ? l.trim().substring(0, 500) : l.trim()) +``` + +--- + +#### LOW-12: URL Text Field Has No `maxLength` + +- **Severity:** Low +- **File:** `lib/widgets/leaf_task_detail.dart:92-99` +- **Code:** + ```dart + TextField( + controller: controller, + decoration: const InputDecoration( + hintText: 'https://...', + border: OutlineInputBorder(), + ), + keyboardType: TextInputType.url, + autofocus: true, + ), + ``` + +**Description:** The URL input dialog has no `maxLength`. A multi-megabyte string pasted into the URL field would be stored in the database and displayed in tooltip text on the task card. While URLs are normalized and scheme-validated, extreme lengths are not checked. + +**Recommended Fix:** Add `maxLength: 2048` (standard URL length limit) to the URL TextField. + +--- + +#### LOW-13: `normalizeUrl` May Save Non-URL Strings as URLs + +- **Severity:** Low +- **File:** `lib/utils/display_utils.dart:9-14` +- **Code:** + ```dart + String? normalizeUrl(String? raw) { + if (raw == null || raw.trim().isEmpty) return null; + final trimmed = raw.trim(); + if (!trimmed.contains('://')) return 'https://$trimmed'; + return trimmed; + } + ``` + +**Description:** `normalizeUrl` auto-prepends `https://` to any string without `://`. Typing random text like `hello world` becomes `https://hello world`, which passes `isAllowedUrl` (scheme is `https`) and gets stored in the database. While this is harmless (it will fail to open in a browser), it means the URL field can contain garbage data that appears as a valid link in the UI. + +**Recommended Fix:** After normalization, validate that the result is a parseable URL with a host: +```dart +final normalized = ...; +final uri = Uri.tryParse(normalized); +if (uri == null || uri.host.isEmpty) return null; +return normalized; +``` + +--- + +#### INFO-7: `SnackBar` Uses Non-Standard `persist` Parameter + +- **Severity:** Informational +- **Files:** Throughout `task_list_screen.dart`, `todays_five_screen.dart`, `backup_service.dart`, `completed_tasks_screen.dart` +- **Description:** All SnackBar constructors pass `persist: false`. Flutter's `SnackBar` does not have a `persist` parameter. This appears to be silently ignored (Dart named parameters on constructors can be ignored if they match an extension or the class accepts them). If this is a custom extension, it should be documented. If not, it's dead code. + +--- + +### Positive Security Findings + +1. **URL scheme validation implemented well.** The `isAllowedUrl()` and `normalizeUrl()` utilities in `display_utils.dart` are clean, centralized, and used consistently across all three code paths (leaf detail launch, AppBar launch, and save dialog). Defense-in-depth: validation at both save time and launch time. + +2. **Backup import validation is thorough.** The `_validateBackup` method now checks 5 properties (file size, SQLite validity, table presence, schema version range, trigger/view absence). Error messages are constructive without leaking internal details. + +3. **SQL injection remains clean.** All new/modified queries continue to use parameterized `?` placeholders. The dynamic `IN (...)` pattern continues to use the safe `taskIds.map((_) => '?').join(',')` idiom throughout. + +4. **No new logging statements.** The codebase remains free of `print()`, `debugPrint()`, or logging calls. + +5. **`allowBackup="false"` properly set.** The Android manifest now explicitly disables ADB backup. + +6. **SharedPreferences parsing is now robust.** `int.tryParse` with `.whereType()` gracefully handles corrupt data. + +### OWASP Mobile Top 10 Assessment (Round 2 Update) + +| Category | Status | Notes | +|----------|--------|-------| +| M1: Improper Credential Usage | N/A | App has no authentication or credentials | +| M2: Inadequate Supply Chain Security | **Still action needed** | `file_picker` 8.3.7 still has known CVEs (HIGH-1 unfixed) | +| M3: Insecure Authentication/Authorization | N/A | App has no auth | +| M4: Insufficient Input/Output Validation | **Improved** | URL scheme validation fixed; brain dump dialog still unbounded (MED-6) | +| M5: Insecure Communication | Pass | No network communication in release | +| M6: Inadequate Privacy Controls | Pass | No PII beyond task names | +| M7: Insufficient Binary Protections | **Still action needed** | No R8/ProGuard (MED-5 unfixed) | +| M8: Security Misconfiguration | **Improved** | `allowBackup="false"` fixed; debug signing fallback remains (LOW-9) | +| M9: Insecure Data Storage | Pass | Data is task names/URLs only; sandboxed on Android | +| M10: Insufficient Cryptography | N/A | App does not use cryptography | + +### Remaining Priority Action Items + +| Priority | Finding | Status | +|----------|---------|--------| +| **HIGH** | HIGH-1: Update `file_picker` to 10.3.10+ | **Still open** | +| **MEDIUM** | MED-5: Enable R8 code shrinking | **Still open** | +| **MEDIUM** | MED-6: Add limits to brain dump dialog | **New** | +| **LOW** | LOW-4: Pre-import backup of existing DB | **Still open** | +| **LOW** | LOW-9: Warn on debug signing fallback | **Still open** | +| **LOW** | LOW-10: `firstWhere` without `orElse` | **Still open** | +| **LOW** | LOW-12: Add `maxLength` to URL field | **New** | +| **LOW** | LOW-13: Validate URL host after normalization | **New** | diff --git a/lib/data/database_helper.dart b/lib/data/database_helper.dart index 55c7ce2..cbe4a98 100644 --- a/lib/data/database_helper.dart +++ b/lib/data/database_helper.dart @@ -220,6 +220,11 @@ class DatabaseHelper { Future importDatabase(String sourcePath) async { await _validateBackup(sourcePath); final dbPath = await getDatabasePath(); + // Create safety backup before overwriting + final existingDb = File(dbPath); + if (await existingDb.exists()) { + await existingDb.copy('$dbPath.bak'); + } if (_dbFuture != null) { final db = await _dbFuture!; await db.close(); diff --git a/lib/providers/task_provider.dart b/lib/providers/task_provider.dart index df9c3a6..5c395dd 100644 --- a/lib/providers/task_provider.dart +++ b/lib/providers/task_provider.dart @@ -95,7 +95,8 @@ class TaskProvider extends ChangeNotifier { Future<({Task task, List parentIds, List childIds, List dependsOnIds, List dependedByIds})> deleteTask(int taskId) async { final task = _currentParent?.id == taskId ? _currentParent! - : _tasks.firstWhere((t) => t.id == taskId); + : _tasks.firstWhere((t) => t.id == taskId, + orElse: () => throw StateError('Task $taskId not found in current list')); final rels = await _db.deleteTaskWithRelationships(taskId); await _refreshCurrentList(); return ( @@ -174,7 +175,8 @@ class TaskProvider extends ChangeNotifier { // Done button) rather than a member of _tasks. final task = _currentParent?.id == taskId ? _currentParent! - : _tasks.firstWhere((t) => t.id == taskId); + : _tasks.firstWhere((t) => t.id == taskId, + orElse: () => throw StateError('Task $taskId not found in current list')); await _db.completeTask(taskId); await navigateBack(); return task; @@ -185,7 +187,8 @@ class TaskProvider extends ChangeNotifier { Future skipTask(int taskId) async { final task = _currentParent?.id == taskId ? _currentParent! - : _tasks.firstWhere((t) => t.id == taskId); + : _tasks.firstWhere((t) => t.id == taskId, + orElse: () => throw StateError('Task $taskId not found in current list')); await _db.skipTask(taskId); await navigateBack(); return task; diff --git a/lib/screens/todays_five_screen.dart b/lib/screens/todays_five_screen.dart index 3ea1d07..85f29fa 100644 --- a/lib/screens/todays_five_screen.dart +++ b/lib/screens/todays_five_screen.dart @@ -311,6 +311,7 @@ class TodaysFiveScreenState extends State { Future _workedOnTask(Task task) async { final provider = context.read(); final wasStarted = task.isStarted; + final previousLastWorkedAt = task.lastWorkedAt; await showCompletionAnimation(context); if (!mounted) return; await provider.markWorkedOn(task.id!); @@ -335,7 +336,7 @@ class TodaysFiveScreenState extends State { action: SnackBarAction( label: 'Undo', onPressed: () async { - await provider.unmarkWorkedOn(task.id!); + await provider.unmarkWorkedOn(task.id!, restoreTo: previousLastWorkedAt); if (!wasStarted) await provider.unstartTask(task.id!); final restored = await DatabaseHelper().getTaskById(task.id!); if (!mounted) return; diff --git a/lib/utils/display_utils.dart b/lib/utils/display_utils.dart index 167d63f..0a67143 100644 --- a/lib/utils/display_utils.dart +++ b/lib/utils/display_utils.dart @@ -5,12 +5,14 @@ String displayUrl(String url, {int maxLength = 40}) { } /// Normalizes a URL: auto-prepends https:// for bare domains. -/// Returns null if the input is empty/null. +/// Returns null if the input is empty/null or not a valid URL with a host. String? normalizeUrl(String? raw) { if (raw == null || raw.trim().isEmpty) return null; final trimmed = raw.trim(); - if (!trimmed.contains('://')) return 'https://$trimmed'; - return trimmed; + final normalized = trimmed.contains('://') ? trimmed : 'https://$trimmed'; + final uri = Uri.tryParse(normalized); + if (uri == null || !uri.host.contains('.')) return null; + return normalized; } /// Returns true if the URL has an allowed scheme (http or https). diff --git a/lib/widgets/brain_dump_dialog.dart b/lib/widgets/brain_dump_dialog.dart index 2d34c99..37634aa 100644 --- a/lib/widgets/brain_dump_dialog.dart +++ b/lib/widgets/brain_dump_dialog.dart @@ -31,6 +31,7 @@ class _BrainDumpDialogState extends State { .split('\n') .map((l) => l.trim()) .where((l) => l.isNotEmpty) + .map((l) => l.length > 500 ? l.substring(0, 500) : l) .toList(); } @@ -68,11 +69,13 @@ class _BrainDumpDialogState extends State { controller: _controller, maxLines: 8, minLines: 4, + maxLength: 25000, autofocus: true, textInputAction: TextInputAction.newline, decoration: const InputDecoration( hintText: 'Buy groceries\nCall dentist\nFinish report\n...', border: OutlineInputBorder(), + counterText: '', ), ), if (_lineCount > 0) ...[ diff --git a/lib/widgets/leaf_task_detail.dart b/lib/widgets/leaf_task_detail.dart index 7f0534b..30be406 100644 --- a/lib/widgets/leaf_task_detail.dart +++ b/lib/widgets/leaf_task_detail.dart @@ -91,15 +91,23 @@ class LeafTaskDetail extends StatelessWidget { }, child: TextField( controller: controller, + maxLength: 2048, decoration: const InputDecoration( hintText: 'https://...', border: OutlineInputBorder(), + counterText: '', ), keyboardType: TextInputType.url, autofocus: true, onSubmitted: (value) { - final url = normalizeUrl(value); - if (url != null && !isAllowedUrl(url)) { + final trimmed = value.trim(); + if (trimmed.isEmpty) { + Navigator.pop(dialogContext); + onUpdateUrl(null); + return; + } + final url = normalizeUrl(trimmed); + if (url == null || !isAllowedUrl(url)) { Navigator.pop(dialogContext); ScaffoldMessenger.of(context).showSnackBar( const SnackBar(content: Text('Only web links (http/https) are supported'), persist: false), @@ -126,8 +134,14 @@ class LeafTaskDetail extends StatelessWidget { ), FilledButton( onPressed: () { - final url = normalizeUrl(controller.text); - if (url != null && !isAllowedUrl(url)) { + final trimmed = controller.text.trim(); + if (trimmed.isEmpty) { + Navigator.pop(dialogContext); + onUpdateUrl(null); + return; + } + final url = normalizeUrl(trimmed); + if (url == null || !isAllowedUrl(url)) { Navigator.pop(dialogContext); ScaffoldMessenger.of(context).showSnackBar( const SnackBar(content: Text('Only web links (http/https) are supported'), persist: false), diff --git a/test/utils/display_utils_test.dart b/test/utils/display_utils_test.dart index a297c46..a60db01 100644 --- a/test/utils/display_utils_test.dart +++ b/test/utils/display_utils_test.dart @@ -38,6 +38,14 @@ void main() { test('preserves other schemes (they get normalized but fail isAllowedUrl)', () { expect(normalizeUrl('ftp://example.com'), 'ftp://example.com'); }); + + test('returns null for random text without valid host', () { + expect(normalizeUrl('hello world'), isNull); + }); + + test('returns null for text with no host after scheme', () { + expect(normalizeUrl('https://'), isNull); + }); }); group('isAllowedUrl', () {