From f94969f2534efdd643832b5c50d2d912c9a52c41 Mon Sep 17 00:00:00 2001 From: lagostimgomes Date: Mon, 25 May 2026 17:24:03 -0400 Subject: [PATCH 1/2] Fix 12 issues from code audit: safety, correctness, and clarity - notebook_search_service: add 120s timeout to OpenAI response body read - backup_models: null-safe casts in BackupRecord.fromJson to avoid TypeError - backup_parser: validate table name against allowlist before PRAGMA interpolation - backup_service: sanitize CR/LF in _csv(); emit first-chunk download progress; document why DisabledSecretStore is chosen when a custom root is supplied; log a warning instead of silently swallowing chmod failures - readable_notebook_exporter: return [] (not ['']) for empty _splitText input - archive_extractor: remove dead startsWith('//') branch (covered by '/') - labarchives_auth_flow: replace deprecated hmac.new() with hmac.digest() - secure_secret_store: clarify why MacOSKeychainReadOnlySecretStore has delete() - labarchives_client: document that SHA-1 is mandated by the LabArchives API Co-Authored-By: Claude Sonnet 4.6 --- lib/src/archive_extractor.dart | 1 - lib/src/backup_models.dart | 12 ++++++------ lib/src/backup_parser.dart | 9 +++++++++ lib/src/backup_service.dart | 21 +++++++++++++++++---- lib/src/labarchives_client.dart | 1 + lib/src/notebook_search_service.dart | 6 +++++- lib/src/readable_notebook_exporter.dart | 2 +- lib/src/secure_secret_store.dart | 3 +++ scripts/labarchives_auth_flow.py | 2 +- 9 files changed, 43 insertions(+), 14 deletions(-) diff --git a/lib/src/archive_extractor.dart b/lib/src/archive_extractor.dart index e6621ae..4e85028 100644 --- a/lib/src/archive_extractor.dart +++ b/lib/src/archive_extractor.dart @@ -168,7 +168,6 @@ class ArchiveExtractionGuard { static bool _looksAbsolute(String path) { final normalized = path.replaceAll(r'\', '/'); return normalized.startsWith('/') || - normalized.startsWith('//') || RegExp(r'^[A-Za-z]:/').hasMatch(normalized); } diff --git a/lib/src/backup_models.dart b/lib/src/backup_models.dart index 7f993e0..8991f44 100644 --- a/lib/src/backup_models.dart +++ b/lib/src/backup_models.dart @@ -58,12 +58,12 @@ class BackupRecord { static BackupRecord fromJson(Map json) { return BackupRecord( - id: json['id'] as String, - notebookName: json['notebookName'] as String, - createdAt: DateTime.parse(json['createdAt'] as String), - archivePath: json['archivePath'] as String, - renderPath: json['renderPath'] as String, - pageCount: json['pageCount'] as int, + id: json['id'] as String? ?? '', + notebookName: json['notebookName'] as String? ?? '', + createdAt: DateTime.parse(json['createdAt'] as String? ?? '1970-01-01T00:00:00Z'), + archivePath: json['archivePath'] as String? ?? '', + renderPath: json['renderPath'] as String? ?? '', + pageCount: json['pageCount'] as int? ?? 0, readablePath: json['readablePath'] as String?, searchIndexPath: json['searchIndexPath'] as String?, integrityManifestPath: json['integrityManifestPath'] as String?, diff --git a/lib/src/backup_parser.dart b/lib/src/backup_parser.dart index 17dda56..c2263f4 100644 --- a/lib/src/backup_parser.dart +++ b/lib/src/backup_parser.dart @@ -565,7 +565,16 @@ class BackupParser { ); } + static const _allowedTables = { + 'entry_parts', + 'entry_part_versions', + 'tree_nodes', + }; + Future> _sqliteColumns(File db, String table) async { + if (!_allowedTables.contains(table)) { + throw ArgumentError.value(table, 'table', 'Table name not in allowlist'); + } final rows = await _sqliteRows(db, "PRAGMA table_info('$table')"); return rows .map((row) => _stringValue(row['name'])) diff --git a/lib/src/backup_service.dart b/lib/src/backup_service.dart index f229e3b..7448b15 100644 --- a/lib/src/backup_service.dart +++ b/lib/src/backup_service.dart @@ -50,6 +50,9 @@ class _ZipEntry { class BackupService { BackupService({Directory? root, SecureSecretStore? secretStore}) : root = root ?? _findProjectRoot(), + // Only read from the Keychain when using the default project root. + // A custom `root` means a test/debug environment where the Keychain + // should not be touched; fall back to DisabledSecretStore in that case. _secretStore = secretStore ?? (root == null && MacOSKeychainReadOnlySecretStore.isSupported @@ -2325,6 +2328,7 @@ class BackupService { onProgress: (receivedBytes, totalBytes) { final shouldEmit = receivedBytes == totalBytes || + lastDownloadProgress == 0 || receivedBytes - lastDownloadProgress >= 25 * 1024 * 1024; if (!shouldEmit) { return; @@ -3504,9 +3508,16 @@ class BackupService { return; } try { - await Process.run('chmod', ['600', file.path]); - } catch (_) { - return; + final result = await Process.run('chmod', ['600', file.path]); + if (result.exitCode != 0) { + stderr.writeln( + 'Warning: could not restrict permissions on ${file.path}: ${result.stderr}', + ); + } + } catch (e) { + stderr.writeln( + 'Warning: could not restrict permissions on ${file.path}: $e', + ); } } @@ -3681,7 +3692,9 @@ String _oneLine(String value) { } String _csv(String value) { - final escaped = value.replaceAll('"', '""'); + final escaped = value + .replaceAll(RegExp(r'[\r\n]+'), ' ') + .replaceAll('"', '""'); return '"$escaped"'; } diff --git a/lib/src/labarchives_client.dart b/lib/src/labarchives_client.dart index af19fb6..691f150 100644 --- a/lib/src/labarchives_client.dart +++ b/lib/src/labarchives_client.dart @@ -230,6 +230,7 @@ class LabArchivesClient { String _signatureFor(String method, String expires) { final key = utf8.encode(accessKey); final message = utf8.encode('$accessId$method$expires'); + // SHA-1 is required by the LabArchives API signature scheme; not a free choice. return base64Encode(Hmac(sha1, key).convert(message).bytes); } diff --git a/lib/src/notebook_search_service.dart b/lib/src/notebook_search_service.dart index caa4a9c..114fcc9 100644 --- a/lib/src/notebook_search_service.dart +++ b/lib/src/notebook_search_service.dart @@ -523,7 +523,11 @@ class NotebookSearchService { final response = await request.close().timeout( const Duration(seconds: 90), ); - final responseBody = await utf8.decoder.bind(response).join(); + // Timeout the body read independently; response.close() only covers headers. + final responseBody = await utf8.decoder + .bind(response) + .join() + .timeout(const Duration(seconds: 120)); if (response.statusCode < 200 || response.statusCode >= 300) { throw AiApiHttpException(response.statusCode, responseBody); } diff --git a/lib/src/readable_notebook_exporter.dart b/lib/src/readable_notebook_exporter.dart index 24b4083..58f88d5 100644 --- a/lib/src/readable_notebook_exporter.dart +++ b/lib/src/readable_notebook_exporter.dart @@ -240,7 +240,7 @@ class ReadableNotebookExporter { List _splitText(String text, {required int maxChars}) { final clean = text.trim(); if (clean.isEmpty) { - return const ['']; + return const []; } if (clean.length <= maxChars) { return [clean]; diff --git a/lib/src/secure_secret_store.dart b/lib/src/secure_secret_store.dart index cd9b895..09815b1 100644 --- a/lib/src/secure_secret_store.dart +++ b/lib/src/secure_secret_store.dart @@ -67,6 +67,9 @@ class DisabledSecretStore implements SecureSecretStore { }) async {} } +// "ReadOnly" refers to write operations from the CLI: secrets must be written +// via the BenchVault app. The delete() method is intentionally included because +// removing a stale credential from the Keychain is a safe CLI operation. class MacOSKeychainReadOnlySecretStore implements SecureSecretStore { const MacOSKeychainReadOnlySecretStore(); diff --git a/scripts/labarchives_auth_flow.py b/scripts/labarchives_auth_flow.py index df55c8f..b666db5 100644 --- a/scripts/labarchives_auth_flow.py +++ b/scripts/labarchives_auth_flow.py @@ -44,7 +44,7 @@ def load_env(path: Path) -> Dict[str, str]: def sign(access_id: str, access_key: str, method: str, expires_ms: str) -> str: message = f"{access_id}{method}{expires_ms}".encode("utf-8") - digest = hmac.new(access_key.encode("utf-8"), message, hashlib.sha1).digest() + digest = hmac.digest(access_key.encode("utf-8"), message, hashlib.sha1) return base64.b64encode(digest).decode("ascii") From e3caa0aa2e0608c26cfdc304a0c5f27121708209 Mon Sep 17 00:00:00 2001 From: Xinlian Liu Date: Mon, 25 May 2026 17:36:28 -0400 Subject: [PATCH 2/2] Fix PR backup record validation --- lib/src/backup_models.dart | 36 ++++++++++++++--- test/backup_models_test.dart | 76 ++++++++++++++++++++++++++++++++++++ 2 files changed, 106 insertions(+), 6 deletions(-) create mode 100644 test/backup_models_test.dart diff --git a/lib/src/backup_models.dart b/lib/src/backup_models.dart index 8991f44..a77f3a9 100644 --- a/lib/src/backup_models.dart +++ b/lib/src/backup_models.dart @@ -58,12 +58,12 @@ class BackupRecord { static BackupRecord fromJson(Map json) { return BackupRecord( - id: json['id'] as String? ?? '', - notebookName: json['notebookName'] as String? ?? '', - createdAt: DateTime.parse(json['createdAt'] as String? ?? '1970-01-01T00:00:00Z'), - archivePath: json['archivePath'] as String? ?? '', - renderPath: json['renderPath'] as String? ?? '', - pageCount: json['pageCount'] as int? ?? 0, + id: _requiredString(json, 'id'), + notebookName: _requiredString(json, 'notebookName'), + createdAt: _requiredDateTime(json, 'createdAt'), + archivePath: _requiredString(json, 'archivePath'), + renderPath: _requiredString(json, 'renderPath'), + pageCount: _requiredInt(json, 'pageCount'), readablePath: json['readablePath'] as String?, searchIndexPath: json['searchIndexPath'] as String?, integrityManifestPath: json['integrityManifestPath'] as String?, @@ -75,6 +75,30 @@ class BackupRecord { ); } + static String _requiredString(Map json, String key) { + final value = json[key]; + if (value is String && value.trim().isNotEmpty) { + return value; + } + throw FormatException('Backup record missing required string "$key".'); + } + + static DateTime _requiredDateTime(Map json, String key) { + final value = json[key]; + if (value is String) { + return DateTime.parse(value); + } + throw FormatException('Backup record missing required date "$key".'); + } + + static int _requiredInt(Map json, String key) { + final value = json[key]; + if (value is int) { + return value; + } + throw FormatException('Backup record missing required integer "$key".'); + } + BackupRecord copyWith({ String? id, String? notebookName, diff --git a/test/backup_models_test.dart b/test/backup_models_test.dart new file mode 100644 index 0000000..f5c6edb --- /dev/null +++ b/test/backup_models_test.dart @@ -0,0 +1,76 @@ +import 'dart:convert'; +import 'dart:io'; + +import 'package:benchvault/src/backup_models.dart'; +import 'package:benchvault/src/backup_service.dart'; +import 'package:flutter_test/flutter_test.dart'; + +void main() { + test('backup records reject missing required fields', () { + final valid = { + 'id': 'run_001_demo_lab', + 'notebookName': 'Demo Lab Notebook', + 'createdAt': '2026-05-14T12:00:00.000Z', + 'archivePath': 'notebooks/demo_lab/2026/05/14/run_001/notebook.7z', + 'renderPath': + 'notebooks/demo_lab/2026/05/14/run_001/render_notebook.json', + 'pageCount': 3, + }; + + for (final key in [ + 'id', + 'notebookName', + 'createdAt', + 'archivePath', + 'renderPath', + 'pageCount', + ]) { + final malformed = Map.of(valid)..remove(key); + + expect( + () => BackupRecord.fromJson(malformed), + throwsA(isA()), + reason: 'missing $key should not be accepted as a blank/default value', + ); + } + }); + + test('loadBackups skips malformed backup records', () async { + final root = await Directory.systemTemp.createTemp( + 'benchvault_bad_record_test_', + ); + addTearDown(() => root.delete(recursive: true)); + + final service = BackupService(root: root); + final goodRun = Directory('${root.path}/backups/good'); + final badRun = Directory('${root.path}/backups/bad'); + await goodRun.create(recursive: true); + await badRun.create(recursive: true); + + final record = BackupRecord( + id: 'run_001_demo_lab', + notebookName: 'Demo Lab Notebook', + createdAt: DateTime.utc(2026, 5, 14), + archivePath: 'good/notebook.7z', + renderPath: 'good/render_notebook.json', + pageCount: 1, + ); + await File('${goodRun.path}/backup_record.json').writeAsString( + const JsonEncoder.withIndent(' ').convert(record.toJson()), + ); + await File('${badRun.path}/backup_record.json').writeAsString( + jsonEncode({ + 'id': 'run_002_corrupt', + 'notebookName': 'Corrupt record', + 'createdAt': '2026-05-14T12:00:00.000Z', + 'archivePath': 'bad/notebook.7z', + 'pageCount': 1, + }), + ); + + final records = await service.loadBackups(); + + expect(records, hasLength(1)); + expect(records.single.id, 'run_001_demo_lab'); + }); +}