nandi/glean · b64f9ef

nandi/gleanpublic⑂ Fork 0

⑂ b64f9ef

Fix eight wire-contract bugs in the Flutter client

Reading the handlers rather than trusting the route names turned up eight
fields the client had wrong. None would have raised an error: Go reads a
missing form value as "", so each one silently did the wrong thing.

The worst is mark-all-read, which scopes on "feed" (articles_handler.go:384)
-- posting "feed_url" would have marked every article read instead of one
feed's. The articles list filters on "feed" too, so per-feed views were
showing everything. The settings endpoints are setters, not toggles: they
read expanded_view/digest_enabled from the form and treat anything but "1"
as false, so a body-less POST always set them off. OPML import takes a
multipart file part, not a form field. add/remove/retry feed and the two
dismiss endpoints each wanted a different key than they were sent.

The regression test cites the handler and line behind each expectation,
since these can only be verified by reading the Go side.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

nandithebull committed 2026-09-20T20:27:51-07:00 Browse files

b64f9ef parent: 7d9686b

modified app/lib/src/api/client.dart +27 -17 Viewed

@@ -78,7 +78,7 @@ class GleanClient {
78	78	}) async =>
79	79	ArticlesResponse.fromJson(await _get('/api/articles/', {
80	80	'page': page,
81		- 'feed_url': feedUrl,
	81	+ 'feed': feedUrl,
82	82	'status': status,
83	83	'q': search,
84	84	'category': category,
@@ -105,7 +105,7 @@ class GleanClient {
105	105
106	106	Future<void> markAllRead({String? feedUrl, String? category}) =>
107	107	_post('/api/articles/mark-all-read', form: {
108		- 'feed_url': ?feedUrl,
	108	+ 'feed': ?feedUrl,
109	109	'category': ?category,
110	110	});
111	111
@@ -116,26 +116,32 @@ class GleanClient {
116	116
117	117	Future<Subscription> addFeed(String url, {String? category}) async {
118	118	final j = await _post('/api/feeds/add', form: {
119		- 'url': url,
	119	+ 'feed_url': url,
120	120	'category': ?category,
121	121	});
122	122	return Subscription.fromJson(j['subscription'] as Map<String, dynamic>);
123	123	}
124	124
125		- Future<Subscription> editFeed(String feedUrl, {String? category, String? title}) async {
	125	+ Future<Subscription> editFeed(String feedUrl, {String? category}) async {
126	126	final j = await _post('/api/feeds/edit', form: {
127	127	'feed_url': feedUrl,
128	128	'category': ?category,
129		- 'title': ?title,
130	129	});
131	130	return Subscription.fromJson(j['subscription'] as Map<String, dynamic>);
132	131	}
133	132
134	133	Future<void> removeFeed(String feedUrl) =>
135		- _delete('/api/feeds/remove', form: {'feed_url': feedUrl});
136		-
137		- Future<int> uploadOpml(String opml) async =>
138		- jsonInt((await _post('/api/feeds/opml/upload', form: {'opml': opml}))['added']);
	134	+ _delete('/api/feeds/remove', form: {'url': feedUrl});
	135	+
	136	+ Future<int> uploadOpml(String opml) async {
	137	+ final res = await session.sendFile(
	138	+ '/api/feeds/opml/upload',
	139	+ field: 'opml',
	140	+ filename: 'subscriptions.opml',
	141	+ bytes: utf8.encode(opml),
	142	+ );
	143	+ return jsonInt(_decode(res)['added']);
	144	+ }
139	145
140	146	Future<String> downloadOpml() async {
141	147	final res = await session.get('/api/feeds/opml/download');
@@ -148,7 +154,7 @@ class GleanClient {
148	154	Future<void> refreshFeeds() => _post('/api/feeds/refresh');
149	155
150	156	Future<void> retryFeed(String feedUrl) =>
151		- _post('/api/feeds/retry', form: {'feed_url': feedUrl});
	157	+ _post('/api/feeds/retry', form: {'url': feedUrl});
152	158
153	159	Future<List<Subscription>> feedList() async =>
154	160	jsonList((await _get('/api/feeds/list'))['subscriptions'], Subscription.fromJson);
@@ -206,22 +212,26 @@ class GleanClient {
206	212	Future<void> dismissFeedRec(String feedUrl) =>
207	213	_post('/api/recs/dismiss-feed', form: {'feed_url': feedUrl});
208	214
209		- Future<void> dismissArticleRec(int articleId) =>
210		- _post('/api/recs/dismiss-article', form: {'article_id': '$articleId'});
	215	+ Future<void> dismissArticleRec(String articleUrl) =>
	216	+ _post('/api/recs/dismiss-article', form: {'article_url': articleUrl});
211	217
212	218	Future<void> dismissPersonRec(String did) =>
213		- _post('/api/recs/dismiss-person', form: {'did': did});
	219	+ _post('/api/recs/dismiss-person', form: {'target_did': did});
214	220
215	221	// --- settings ---
216	222
217	223	Future<List<String>> toggleLanguage(String code) async =>
218	224	jsonStrList((await _post('/api/settings/languages/$code'))['languages']);
219	225
220		- Future<bool> toggleExpandedView() async =>
221		- jsonBool((await _post('/api/settings/expanded-view'))['expanded_view']);
	226	+ Future<bool> setExpandedView(bool enabled) async => jsonBool(
	227	+ (await _post('/api/settings/expanded-view',
	228	+ form: {'expanded_view': enabled ? '1' : '0'}))['expanded_view'],
	229	+ );
222	230
223		- Future<bool> toggleDigestEnabled() async =>
224		- jsonBool((await _post('/api/settings/digest-enabled'))['digest_enabled']);
	231	+ Future<bool> setDigestEnabled(bool enabled) async => jsonBool(
	232	+ (await _post('/api/settings/digest-enabled',
	233	+ form: {'digest_enabled': enabled ? '1' : '0'}))['digest_enabled'],
	234	+ );
225	235
226	236	// --- digest ---
227	237

    }) async =>
        ArticlesResponse.fromJson(await _get('/api/articles/', {
          'page': page,
--        'feed_url': feedUrl,
++        'feed': feedUrl,
          'status': status,
          'q': search,
          'category': category,
    Future<void> markAllRead({String? feedUrl, String? category}) =>
        _post('/api/articles/mark-all-read', form: {
--        'feed_url': ?feedUrl,
++        'feed': ?feedUrl,
          'category': ?category,
        });
    Future<Subscription> addFeed(String url, {String? category}) async {
      final j = await _post('/api/feeds/add', form: {
--      'url': url,
++      'feed_url': url,
        'category': ?category,
      });
      return Subscription.fromJson(j['subscription'] as Map<String, dynamic>);
    }
--  Future<Subscription> editFeed(String feedUrl, {String? category, String? title}) async {
++  Future<Subscription> editFeed(String feedUrl, {String? category}) async {
      final j = await _post('/api/feeds/edit', form: {
        'feed_url': feedUrl,
        'category': ?category,
--      'title': ?title,
      });
      return Subscription.fromJson(j['subscription'] as Map<String, dynamic>);
    }
    Future<void> removeFeed(String feedUrl) =>
--      _delete('/api/feeds/remove', form: {'feed_url': feedUrl});
++      _delete('/api/feeds/remove', form: {'url': feedUrl});
--
++
--  Future<int> uploadOpml(String opml) async =>
++  Future<int> uploadOpml(String opml) async {
--      jsonInt((await _post('/api/feeds/opml/upload', form: {'opml': opml}))['added']);
++    final res = await session.sendFile(
++      '/api/feeds/opml/upload',
++      field: 'opml',
++      filename: 'subscriptions.opml',
++      bytes: utf8.encode(opml),
++    );
++    return jsonInt(_decode(res)['added']);
++  }
    Future<String> downloadOpml() async {
      final res = await session.get('/api/feeds/opml/download');
    Future<void> refreshFeeds() => _post('/api/feeds/refresh');
    Future<void> retryFeed(String feedUrl) =>
--      _post('/api/feeds/retry', form: {'feed_url': feedUrl});
++      _post('/api/feeds/retry', form: {'url': feedUrl});
    Future<List<Subscription>> feedList() async =>
        jsonList((await _get('/api/feeds/list'))['subscriptions'], Subscription.fromJson);
    Future<void> dismissFeedRec(String feedUrl) =>
        _post('/api/recs/dismiss-feed', form: {'feed_url': feedUrl});
--  Future<void> dismissArticleRec(int articleId) =>
++  Future<void> dismissArticleRec(String articleUrl) =>
--      _post('/api/recs/dismiss-article', form: {'article_id': '$articleId'});
++      _post('/api/recs/dismiss-article', form: {'article_url': articleUrl});
    Future<void> dismissPersonRec(String did) =>
--      _post('/api/recs/dismiss-person', form: {'did': did});
++      _post('/api/recs/dismiss-person', form: {'target_did': did});
    // --- settings ---
    Future<List<String>> toggleLanguage(String code) async =>
        jsonStrList((await _post('/api/settings/languages/$code'))['languages']);
--  Future<bool> toggleExpandedView() async =>
++  Future<bool> setExpandedView(bool enabled) async => jsonBool(
--      jsonBool((await _post('/api/settings/expanded-view'))['expanded_view']);
++        (await _post('/api/settings/expanded-view',
++            form: {'expanded_view': enabled ? '1' : '0'}))['expanded_view'],
++      );
--  Future<bool> toggleDigestEnabled() async =>
++  Future<bool> setDigestEnabled(bool enabled) async => jsonBool(
--      jsonBool((await _post('/api/settings/digest-enabled'))['digest_enabled']);
++        (await _post('/api/settings/digest-enabled',
++            form: {'digest_enabled': enabled ? '1' : '0'}))['digest_enabled'],
++      );
    // --- digest ---

modified app/lib/src/api/session.dart +16 -0 Viewed

@@ -142,5 +142,21 @@ class GleanSession {
142	142	return res;
143	143	}
144	144
	145	+ /// Multipart POST, for the endpoints that take a file rather than form
	146	+ /// fields (OPML import uses r.FormFile).
	147	+ Future<http.Response> sendFile(
	148	+ String path, {
	149	+ required String field,
	150	+ required String filename,
	151	+ required List<int> bytes,
	152	+ }) async {
	153	+ final req = http.MultipartRequest('POST', _uri(path))
	154	+ ..headers.addAll(_headers(unsafe: true))
	155	+ ..files.add(http.MultipartFile.fromBytes(field, bytes, filename: filename));
	156	+ final res = await http.Response.fromStream(await _client.send(req));
	157	+ _absorb(res);
	158	+ return res;
	159	+ }
	160	+
145	161	void close() => _client.close();
146	162	}

      return res;
    }
++  /// Multipart POST, for the endpoints that take a file rather than form
++  /// fields (OPML import uses r.FormFile).
++  Future<http.Response> sendFile(
++    String path, {
++    required String field,
++    required String filename,
++    required List<int> bytes,
++  }) async {
++    final req = http.MultipartRequest('POST', _uri(path))
++      ..headers.addAll(_headers(unsafe: true))
++      ..files.add(http.MultipartFile.fromBytes(field, bytes, filename: filename));
++    final res = await http.Response.fromStream(await _client.send(req));
++    _absorb(res);
++    return res;
++  }
++
    void close() => _client.close();
  }

added app/test/wire_contract_test.dart +132 -0 Viewed

new file mode 100644
@@ -0,0 +1,132 @@
	1	+import 'dart:convert';
	2	+
	3	+import 'package:flutter_test/flutter_test.dart';
	4	+import 'package:http/http.dart' as http;
	5	+import 'package:http/testing.dart';
	6	+import 'package:shared_preferences/shared_preferences.dart';
	7	+
	8	+import 'package:glean_app/src/api/client.dart';
	9	+import 'package:glean_app/src/api/session.dart';
	10	+
	11	+/// Locks the exact field names the Go handlers read.
	12	+///
	13	+/// These are invisible when wrong: the server happily accepts a request with
	14	+/// an unknown field and acts on the zero value, so `mark-all-read` with the
	15	+/// wrong key silently marks everything read rather than one feed. Each
	16	+/// expectation below cites the handler it mirrors.
	17	+void main() {
	18	+ setUpAll(() => SharedPreferences.setMockInitialValues({}));
	19	+
	20	+ late http.BaseRequest captured;
	21	+ late String capturedBody;
	22	+
	23	+ GleanClient clientCapturing({String responseJson = '{}'}) {
	24	+ final mock = MockClient((req) async {
	25	+ captured = req;
	26	+ capturedBody = req.body;
	27	+ return http.Response(responseJson, 200,
	28	+ headers: {'content-type': 'application/json'});
	29	+ });
	30	+ return GleanClient(GleanSession(baseUrl: 'https://example.test', client: mock));
	31	+ }
	32	+
	33	+ Map<String, String> form() => Uri.splitQueryString(capturedBody);
	34	+ Map<String, String> query() => captured.url.queryParameters;
	35	+
	36	+ test('articles filters on "feed" (articles_handler.go:22)', () async {
	37	+ await clientCapturing(responseJson: '{"articles":[]}')
	38	+ .articles(feedUrl: 'https://ex.test/f', status: 'unread', search: 'go', sortOldest: true);
	39	+ expect(query()['feed'], 'https://ex.test/f');
	40	+ expect(query().containsKey('feed_url'), isFalse);
	41	+ expect(query()['status'], 'unread');
	42	+ expect(query()['q'], 'go');
	43	+ expect(query()['sort'], 'oldest');
	44	+ });
	45	+
	46	+ test('mark-all-read scopes on "feed" (articles_handler.go:384)', () async {
	47	+ await clientCapturing().markAllRead(feedUrl: 'https://ex.test/f');
	48	+ expect(form()['feed'], 'https://ex.test/f');
	49	+ // The dangerous case: an unknown key means an unscoped mark-all-read.
	50	+ expect(form().containsKey('feed_url'), isFalse);
	51	+ });
	52	+
	53	+ test('add feed posts "feed_url" (feeds_handler.go:128)', () async {
	54	+ await clientCapturing(responseJson: '{"subscription":{}}').addFeed('https://ex.test/f');
	55	+ expect(form()['feed_url'], 'https://ex.test/f');
	56	+ });
	57	+
	58	+ test('remove feed sends "url" (feeds_handler.go:287)', () async {
	59	+ await clientCapturing().removeFeed('https://ex.test/f');
	60	+ expect(captured.method, 'DELETE');
	61	+ expect(form()['url'], 'https://ex.test/f');
	62	+ });
	63	+
	64	+ test('retry feed sends "url" (feeds_handler.go:516)', () async {
	65	+ await clientCapturing().retryFeed('https://ex.test/f');
	66	+ expect(form()['url'], 'https://ex.test/f');
	67	+ });
	68	+
	69	+ test('OPML import uploads a file part named "opml" (feeds_handler.go:359)', () async {
	70	+ await clientCapturing(responseJson: '{"added":3}').uploadOpml('<opml/>');
	71	+ expect(captured.headers['content-type'], startsWith('multipart/form-data'));
	72	+ expect(capturedBody, contains('name="opml"'));
	73	+ });
	74	+
	75	+ test('settings endpoints set a value rather than toggling '
	76	+ '(settings_handler.go:66,85)', () async {
	77	+ var c = clientCapturing(responseJson: '{"expanded_view":true}');
	78	+ await c.setExpandedView(true);
	79	+ expect(form()['expanded_view'], '1');
	80	+
	81	+ c = clientCapturing(responseJson: '{"expanded_view":false}');
	82	+ await c.setExpandedView(false);
	83	+ expect(form()['expanded_view'], '0');
	84	+
	85	+ c = clientCapturing(responseJson: '{"digest_enabled":true}');
	86	+ await c.setDigestEnabled(true);
	87	+ expect(form()['digest_enabled'], '1');
	88	+ });
	89	+
	90	+ test('dismiss endpoints key on article_url and target_did '
	91	+ '(recs_handler.go:7-17)', () async {
	92	+ await clientCapturing().dismissArticleRec('https://ex.test/a');
	93	+ expect(form()['article_url'], 'https://ex.test/a');
	94	+
	95	+ await clientCapturing().dismissPersonRec('did:plc:abc');
	96	+ expect(form()['target_did'], 'did:plc:abc');
	97	+
	98	+ await clientCapturing().dismissFeedRec('https://ex.test/f');
	99	+ expect(form()['feed_url'], 'https://ex.test/f');
	100	+ });
	101	+
	102	+ test('unsafe methods carry the CSRF token, safe ones do not '
	103	+ '(middleware.go:90)', () async {
	104	+ final mock = MockClient((req) async {
	105	+ captured = req;
	106	+ return http.Response('{}', 200, headers: {
	107	+ 'content-type': 'application/json',
	108	+ 'set-cookie': 'glean_csrf=tok123; Path=/',
	109	+ });
	110	+ });
	111	+ final session = GleanSession(baseUrl: 'https://example.test', client: mock);
	112	+ final c = GleanClient(session);
	113	+
	114	+ await c.me();
	115	+ expect(captured.headers.containsKey('X-CSRF-Token'), isFalse);
	116	+
	117	+ await c.refreshFeeds();
	118	+ expect(captured.headers['X-CSRF-Token'], 'tok123');
	119	+ expect(captured.headers['Cookie'], contains('glean_csrf=tok123'));
	120	+ });
	121	+
	122	+ test('error responses surface the server message', () async {
	123	+ final mock = MockClient((_) async => http.Response(
	124	+ jsonEncode({'error': 'Please enter your handle.'}), 400,
	125	+ headers: {'content-type': 'application/json'}));
	126	+ final c = GleanClient(GleanSession(baseUrl: 'https://example.test', client: mock));
	127	+ await expectLater(
	128	+ c.startAuth('x'),
	129	+ throwsA(isA<ApiException>().having((e) => e.message, 'message', 'Please enter your handle.')),
	130	+ );
	131	+ });
	132	+}

++import 'dart:convert';
++
++import 'package:flutter_test/flutter_test.dart';
++import 'package:http/http.dart' as http;
++import 'package:http/testing.dart';
++import 'package:shared_preferences/shared_preferences.dart';
++
++import 'package:glean_app/src/api/client.dart';
++import 'package:glean_app/src/api/session.dart';
++
++/// Locks the exact field names the Go handlers read.
++///
++/// These are invisible when wrong: the server happily accepts a request with
++/// an unknown field and acts on the zero value, so `mark-all-read` with the
++/// wrong key silently marks *everything* read rather than one feed. Each
++/// expectation below cites the handler it mirrors.
++void main() {
++  setUpAll(() => SharedPreferences.setMockInitialValues({}));
++
++  late http.BaseRequest captured;
++  late String capturedBody;
++
++  GleanClient clientCapturing({String responseJson = '{}'}) {
++    final mock = MockClient((req) async {
++      captured = req;
++      capturedBody = req.body;
++      return http.Response(responseJson, 200,
++          headers: {'content-type': 'application/json'});
++    });
++    return GleanClient(GleanSession(baseUrl: 'https://example.test', client: mock));
++  }
++
++  Map<String, String> form() => Uri.splitQueryString(capturedBody);
++  Map<String, String> query() => captured.url.queryParameters;
++
++  test('articles filters on "feed" (articles_handler.go:22)', () async {
++    await clientCapturing(responseJson: '{"articles":[]}')
++        .articles(feedUrl: 'https://ex.test/f', status: 'unread', search: 'go', sortOldest: true);
++    expect(query()['feed'], 'https://ex.test/f');
++    expect(query().containsKey('feed_url'), isFalse);
++    expect(query()['status'], 'unread');
++    expect(query()['q'], 'go');
++    expect(query()['sort'], 'oldest');
++  });
++
++  test('mark-all-read scopes on "feed" (articles_handler.go:384)', () async {
++    await clientCapturing().markAllRead(feedUrl: 'https://ex.test/f');
++    expect(form()['feed'], 'https://ex.test/f');
++    // The dangerous case: an unknown key means an unscoped mark-all-read.
++    expect(form().containsKey('feed_url'), isFalse);
++  });
++
++  test('add feed posts "feed_url" (feeds_handler.go:128)', () async {
++    await clientCapturing(responseJson: '{"subscription":{}}').addFeed('https://ex.test/f');
++    expect(form()['feed_url'], 'https://ex.test/f');
++  });
++
++  test('remove feed sends "url" (feeds_handler.go:287)', () async {
++    await clientCapturing().removeFeed('https://ex.test/f');
++    expect(captured.method, 'DELETE');
++    expect(form()['url'], 'https://ex.test/f');
++  });
++
++  test('retry feed sends "url" (feeds_handler.go:516)', () async {
++    await clientCapturing().retryFeed('https://ex.test/f');
++    expect(form()['url'], 'https://ex.test/f');
++  });
++
++  test('OPML import uploads a file part named "opml" (feeds_handler.go:359)', () async {
++    await clientCapturing(responseJson: '{"added":3}').uploadOpml('<opml/>');
++    expect(captured.headers['content-type'], startsWith('multipart/form-data'));
++    expect(capturedBody, contains('name="opml"'));
++  });
++
++  test('settings endpoints set a value rather than toggling '
++      '(settings_handler.go:66,85)', () async {
++    var c = clientCapturing(responseJson: '{"expanded_view":true}');
++    await c.setExpandedView(true);
++    expect(form()['expanded_view'], '1');
++
++    c = clientCapturing(responseJson: '{"expanded_view":false}');
++    await c.setExpandedView(false);
++    expect(form()['expanded_view'], '0');
++
++    c = clientCapturing(responseJson: '{"digest_enabled":true}');
++    await c.setDigestEnabled(true);
++    expect(form()['digest_enabled'], '1');
++  });
++
++  test('dismiss endpoints key on article_url and target_did '
++      '(recs_handler.go:7-17)', () async {
++    await clientCapturing().dismissArticleRec('https://ex.test/a');
++    expect(form()['article_url'], 'https://ex.test/a');
++
++    await clientCapturing().dismissPersonRec('did:plc:abc');
++    expect(form()['target_did'], 'did:plc:abc');
++
++    await clientCapturing().dismissFeedRec('https://ex.test/f');
++    expect(form()['feed_url'], 'https://ex.test/f');
++  });
++
++  test('unsafe methods carry the CSRF token, safe ones do not '
++      '(middleware.go:90)', () async {
++    final mock = MockClient((req) async {
++      captured = req;
++      return http.Response('{}', 200, headers: {
++        'content-type': 'application/json',
++        'set-cookie': 'glean_csrf=tok123; Path=/',
++      });
++    });
++    final session = GleanSession(baseUrl: 'https://example.test', client: mock);
++    final c = GleanClient(session);
++
++    await c.me();
++    expect(captured.headers.containsKey('X-CSRF-Token'), isFalse);
++
++    await c.refreshFeeds();
++    expect(captured.headers['X-CSRF-Token'], 'tok123');
++    expect(captured.headers['Cookie'], contains('glean_csrf=tok123'));
++  });
++
++  test('error responses surface the server message', () async {
++    final mock = MockClient((_) async => http.Response(
++        jsonEncode({'error': 'Please enter your handle.'}), 400,
++        headers: {'content-type': 'application/json'}));
++    final c = GleanClient(GleanSession(baseUrl: 'https://example.test', client: mock));
++    await expectLater(
++      c.startAuth('x'),
++      throwsA(isA<ApiException>().having((e) => e.message, 'message', 'Please enter your handle.')),
++    );
++  });
++}