Skip to content

Commit 45a41d4

Browse files
yyodsyyods
committed
[red] Block unsafe GitHub import races
Signed-off-by: Viwat Vchirawongkwin <viwat.v@chula.ac.th>
1 parent bc2c566 commit 45a41d4

4 files changed

Lines changed: 193 additions & 4 deletions

File tree

‎app/test/conformance/file_ops_conformance_test.dart‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -162,6 +162,36 @@ void main() {
162162
);
163163
},
164164
);
165+
166+
test(
167+
'preserves more==1 through the neutral completeness capability',
168+
() async {
169+
final Future<DirectoryListing> pending =
170+
(conn as ConnectionDirectoryListingSource).listDirWithMetadata(
171+
'/big',
172+
);
173+
await pumpEventQueue();
174+
final PbleFrame sent = transport.sentFrames.single;
175+
176+
transport.deliverFrame(
177+
rsp(
178+
PbleOpcode.fileList,
179+
sent.id,
180+
fileListOkPayload(
181+
more: true,
182+
entries: <List<int>>[
183+
fileEntry(isDir: false, size: 1, name: 'a.py'),
184+
],
185+
),
186+
),
187+
);
188+
189+
final DirectoryListing listing = await pending;
190+
expect(listing.truncated, isTrue);
191+
expect(listing.entries.single.name, 'a.py');
192+
expect(sentOf(PbleOpcode.fileList), hasLength(1));
193+
},
194+
);
165195
});
166196

167197
group('A-12 getFile — CRC-verified download streaming (FR-PBLE-9)', () {

‎app/test/unit/github_board_importer_test.dart‎

Lines changed: 44 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -55,17 +55,20 @@ final class _FakeGithubApi extends Fake implements GithubApi {
5555
}
5656

5757
final class _SessionRecordingConnection extends RecordingConnection
58-
implements ConnectionSessionStampSource {
58+
implements ConnectionSessionStampSource, ConnectionDirectoryListingSource {
5959
_SessionRecordingConnection({
6060
List<List<RemoteEntry>>? directoryListings,
61+
List<bool>? listingTruncation,
6162
this.failPutAt,
6263
this.holdPutAt,
6364
}) : directoryListings =
6465
directoryListings ?? <List<RemoteEntry>>[<RemoteEntry>[]],
66+
listingTruncation = listingTruncation ?? <bool>[false],
6567
_sessionStamp = Object(),
6668
super(initial: ConnState.ready);
6769

6870
final List<List<RemoteEntry>> directoryListings;
71+
final List<bool> listingTruncation;
6972
final int? failPutAt;
7073
final int? holdPutAt;
7174

@@ -82,14 +85,28 @@ final class _SessionRecordingConnection extends RecordingConnection
8285
void advanceSession() => _sessionStamp = Object();
8386

8487
@override
85-
Future<List<RemoteEntry>> listDir(String path) async {
88+
Future<DirectoryListing> listDirWithMetadata(String path) async {
8689
operationLog.add('list:$path');
87-
if (directoryListings.isEmpty) return <RemoteEntry>[];
90+
if (directoryListings.isEmpty) {
91+
return const DirectoryListing(entries: <RemoteEntry>[], truncated: false);
92+
}
8893
final int index = _listIndex < directoryListings.length
8994
? _listIndex
9095
: directoryListings.length - 1;
9196
_listIndex += 1;
92-
return List<RemoteEntry>.of(directoryListings[index]);
97+
final int truncationIndex = index < listingTruncation.length
98+
? index
99+
: listingTruncation.length - 1;
100+
return DirectoryListing(
101+
entries: directoryListings[index],
102+
truncated: listingTruncation[truncationIndex],
103+
);
104+
}
105+
106+
@override
107+
Future<List<RemoteEntry>> listDir(String path) async {
108+
final DirectoryListing listing = await listDirWithMetadata(path);
109+
return listing.entries;
93110
}
94111

95112
@override
@@ -133,13 +150,15 @@ final class _Harness {
133150
factory _Harness({
134151
String cwd = '/flash',
135152
List<List<RemoteEntry>>? directoryListings,
153+
List<bool>? listingTruncation,
136154
int? failPutAt,
137155
int? holdPutAt,
138156
}) {
139157
// Share one chronological log so tests can prove that no PUT races ahead
140158
// of a pending or unvalidated Git blob.
141159
final _SessionRecordingConnection connection = _SessionRecordingConnection(
142160
directoryListings: directoryListings,
161+
listingTruncation: listingTruncation,
143162
failPutAt: failPutAt,
144163
holdPutAt: holdPutAt,
145164
);
@@ -217,12 +236,14 @@ void main() {
217236
_Harness harness({
218237
String cwd = '/flash',
219238
List<List<RemoteEntry>>? directoryListings,
239+
List<bool>? listingTruncation,
220240
int? failPutAt,
221241
int? holdPutAt,
222242
}) {
223243
final _Harness value = _Harness(
224244
cwd: cwd,
225245
directoryListings: directoryListings,
246+
listingTruncation: listingTruncation,
226247
failPutAt: failPutAt,
227248
holdPutAt: holdPutAt,
228249
);
@@ -368,6 +389,25 @@ void main() {
368389
expect(h.connection.putFileCalls, isEmpty);
369390
});
370391

392+
test(
393+
'a truncated board listing blocks review before any download',
394+
() async {
395+
final _Harness h = harness(
396+
directoryListings: <List<RemoteEntry>>[
397+
<RemoteEntry>[_remoteFile('visible.py')],
398+
],
399+
listingTruncation: <bool>[true],
400+
);
401+
402+
await expectLater(
403+
h.importer.review(_repository, <GithubEntry>[_file('hidden.py')]),
404+
throwsA(_failure(GithubFailureKind.incompleteBoardListing)),
405+
);
406+
expect(h.api.fetchedRemotePaths, isEmpty);
407+
expect(h.connection.putFileCalls, isEmpty);
408+
},
409+
);
410+
371411
test(
372412
'an existing file requires separate explicit overwrite consent',
373413
() async {

‎app/test/unit/github_repository_client_test.dart‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
// tests describe PyBLE's fresh REST adapter; they do not exercise or copy
66
// another application's implementation.
77

8+
import 'dart:async';
89
import 'dart:convert';
910
import 'dart:io';
1011
import 'dart:typed_data';
@@ -574,5 +575,33 @@ void main() {
574575
);
575576
},
576577
);
578+
579+
test('aborts an in-flight request through its operation token', () async {
580+
final Completer<void> requestStarted = Completer<void>();
581+
final GithubCancellation cancellation = GithubCancellation();
582+
final GithubRepositoryClient api = GithubRepositoryClient(
583+
httpClient: MockClient.streaming((request, bodyStream) async {
584+
expect(request, isA<http.AbortableRequest>());
585+
final http.AbortableRequest abortable =
586+
request as http.AbortableRequest;
587+
requestStarted.complete();
588+
await abortable.abortTrigger;
589+
throw http.RequestAbortedException(request.url);
590+
}),
591+
);
592+
593+
final Future<PinnedRepository> pending = api.resolve(
594+
_locator(),
595+
ref: 'main',
596+
cancellation: cancellation,
597+
);
598+
await requestStarted.future;
599+
cancellation.cancel();
600+
601+
await expectLater(
602+
pending,
603+
throwsA(_githubFailure(GithubFailureKind.cancelled)),
604+
);
605+
});
577606
});
578607
}

‎app/test/widget/github_import_view_test.dart‎

Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -154,6 +154,24 @@ final class _FailingPutConnection extends RecordingConnection {
154154
}
155155
}
156156

157+
final class _HoldingPutConnection extends RecordingConnection {
158+
_HoldingPutConnection() : super(initial: ConnState.ready);
159+
160+
final Completer<void> putSettled = Completer<void>();
161+
final Completer<void> releasePut = Completer<void>();
162+
163+
@override
164+
Future<void> putFile(
165+
String path,
166+
Uint8List bytes, {
167+
ProgressCb? onProgress,
168+
}) async {
169+
await super.putFile(path, bytes, onProgress: onProgress);
170+
putSettled.complete();
171+
await releasePut.future;
172+
}
173+
}
174+
157175
Key _entryKey(String remotePath) => ValueKey<String>('githubEntry_$remotePath');
158176

159177
Key _selectionKey(String remotePath) =>
@@ -476,5 +494,77 @@ void main() {
476494
);
477495
},
478496
);
497+
498+
testWidgets('editing repository or ref invalidates the pinned selection', (
499+
WidgetTester tester,
500+
) async {
501+
final RecordingConnection connection = RecordingConnection(
502+
initial: ConnState.ready,
503+
);
504+
final _FakeGithubApi api = _FakeGithubApi();
505+
addTearDown(connection.dispose);
506+
507+
await _openImport(tester, connection, api);
508+
await _browse(tester);
509+
await tester.tap(find.byKey(_selectionKey('hello.py')));
510+
await tester.pump();
511+
expect(
512+
tester
513+
.widget<CheckboxListTile>(find.byKey(_selectionKey('hello.py')))
514+
.value,
515+
isTrue,
516+
);
517+
518+
await tester.enterText(find.byKey(kGithubRefFieldKey), 'other-branch');
519+
await tester.pump();
520+
521+
expect(find.textContaining(_commitSha), findsNothing);
522+
expect(find.byKey(_selectionKey('hello.py')), findsNothing);
523+
expect(
524+
tester
525+
.widget<FilledButton>(find.byKey(kGithubReviewButtonKey))
526+
.onPressed,
527+
isNull,
528+
);
529+
});
530+
531+
testWidgets('an in-flight PUT cannot dismiss and hide its final result', (
532+
WidgetTester tester,
533+
) async {
534+
final _HoldingPutConnection connection = _HoldingPutConnection();
535+
final _FakeGithubApi api = _FakeGithubApi(
536+
entries: const <GithubEntry>[_helloEntry],
537+
);
538+
addTearDown(() async {
539+
if (!connection.releasePut.isCompleted) {
540+
connection.releasePut.complete();
541+
}
542+
await connection.dispose();
543+
});
544+
545+
await _openImport(tester, connection, api);
546+
await _browse(tester);
547+
await tester.tap(find.byKey(_selectionKey('hello.py')));
548+
await tester.pump();
549+
await tester.tap(find.byKey(kGithubReviewButtonKey));
550+
await tester.pumpAndSettle();
551+
await tester.tap(find.byKey(kGithubCommitButtonKey));
552+
await connection.putSettled.future;
553+
await tester.pump();
554+
555+
final Finder close = find.ancestor(
556+
of: find.byTooltip(l10nOf(tester).githubImportClose),
557+
matching: find.byType(IconButton),
558+
);
559+
expect(tester.widget<IconButton>(close).onPressed, isNull);
560+
expect(find.byType(Dialog), findsOneWidget);
561+
562+
connection.releasePut.complete();
563+
await tester.pumpAndSettle();
564+
expect(
565+
find.byKey(const ValueKey<String>('githubImportResult')),
566+
findsOneWidget,
567+
);
568+
});
479569
});
480570
}

0 commit comments

Comments
 (0)