scan explorer bugs

config.py

@@ -20,6 +20,7 @@
 OPEN_SEARCH_URL = 'http://opensearch-node1:9200'
 OPEN_SEARCH_INDEX = 'scan-explorer'
 OPEN_SEARCH_AGG_BUCKET_LIMIT = 10000
+OPEN_SEARCH_MAX_RESULT_WINDOW = 10000
 
 REDIS_URL = 'redis://redis-backend:6379/4'
 

scan_explorer_service/manifest_factory.py

@@ -1,4 +1,5 @@
 from typing import Dict
+from html import escape
 from iiif_prezi.factory import ManifestFactory, Sequence, Canvas, Image, Annotation, Manifest, Range
 from scan_explorer_service.models import Article, Page, Collection
 from typing import Union
@@ -68,7 +69,9 @@ def get_or_create_canvas(self, page: Page):
 
         if len(page.articles) > 0:
             metadata = {
-                'Abstract': ''.join(f'<a href="https://ui.adsabs.harvard.edu/abs/{str(x.bibcode)}/abstract">{str(x.bibcode)}</a><br/>' for x in page.articles)
+                'Abstract': ''.join(
+                    f'<a href="/abs/{escape(str(article.bibcode))}/abstract">{escape(str(article.bibcode))}</a><br/>'
+                    for article in page.articles)
             }
             canvas.set_metadata(metadata)
 

scan_explorer_service/open_search.py

@@ -138,6 +138,11 @@ def page_os_search(qs: str, page, limit, sort):
     query = create_query_string_query(qs)
     query = set_page_search_fields(query)
     from_number = (page - 1) * limit
+    window = current_app.config.get('OPEN_SEARCH_MAX_RESULT_WINDOW', 10000)
+    if from_number + limit > window:
+        raise ValueError(
+            f'page {page} at limit {limit} reaches result {from_number + limit}, '
+            f'beyond the searchable window of {window}')
     query['size'] = limit
     query['from'] = from_number
     query['track_total_hits'] = True

scan_explorer_service/tests/test_cache.py

@@ -414,5 +414,188 @@ def test_ocr_served_from_cache(self, mock_cache_get):
         self.assertIn('text/plain', r.content_type)
 
 
+
+class TestVariantIsolation(TestCaseDatabase):
+    """Two deployments serving different hostnames must not share cached documents."""
+
+    def create_app(self):
+        from scan_explorer_service.app import create_app
+        return create_app(**{
+            'SQLALCHEMY_DATABASE_URI': self.postgresql_url,
+            'TESTING': True,
+            'PROXY_SERVER': 'https://ui.adsabs.harvard.edu:443',
+            'PROXY_PREFIX': '/v1/scan',
+        })
+
+    def setUp(self):
+        super().setUp()
+        cache_mod._redis_client = None
+
+    def tearDown(self):
+        cache_mod._redis_client = None
+        super().tearDown()
+
+    @patch('scan_explorer_service.utils.cache.redis.from_url')
+    def test_variants_do_not_share_cached_manifests(self, mock_from_url):
+        store = {}
+        client = MagicMock()
+        client.ping.return_value = True
+        client.setex.side_effect = lambda k, ttl, v: store.__setitem__(k, v)
+        client.get.side_effect = store.get
+        mock_from_url.return_value = client
+
+        bibcode = '1993ASPC...52..132K'
+        ads_manifest = '{"@id":"https://ui.adsabs.harvard.edu:443/v1/scan/..."}'
+
+        cache_mod.cache_set_manifest(bibcode, ads_manifest)
+        self.assertEqual(cache_mod.cache_get_manifest(bibcode), ads_manifest)
+
+        self.app.config['PROXY_SERVER'] = 'https://scixplorer.org:443'
+        self.app.config['PROXY_PREFIX'] = '/v1/scix-scan'
+
+        self.assertIsNone(
+            cache_mod.cache_get_manifest(bibcode),
+            'the SciX deployment must not read the manifest cached by the ADS deployment')
+
+        scix_manifest = '{"@id":"https://scixplorer.org:443/v1/scix-scan/..."}'
+        cache_mod.cache_set_manifest(bibcode, scix_manifest)
+        self.assertEqual(cache_mod.cache_get_manifest(bibcode), scix_manifest)
+
+        self.app.config['PROXY_SERVER'] = 'https://ui.adsabs.harvard.edu:443'
+        self.app.config['PROXY_PREFIX'] = '/v1/scan'
+        self.assertEqual(cache_mod.cache_get_manifest(bibcode), ads_manifest)
+
+    @patch('scan_explorer_service.utils.cache.redis.from_url')
+    def test_variants_do_not_share_cached_searches(self, mock_from_url):
+        store = {}
+        client = MagicMock()
+        client.ping.return_value = True
+        client.setex.side_effect = lambda k, ttl, v: store.__setitem__(k, v)
+        client.get.side_effect = store.get
+        mock_from_url.return_value = client
+
+        cache_key = 'ocr:1993ASPC...52..132K:gas'
+        ads_annotations = '{"resources":[{"on":"https://ui.adsabs.harvard.edu:443/v1/scan/canvas/x"}]}'
+
+        cache_mod.cache_set_search(cache_key, ads_annotations)
+        self.assertEqual(cache_mod.cache_get_search(cache_key), ads_annotations)
+
+        self.app.config['PROXY_SERVER'] = 'https://scixplorer.org:443'
+        self.app.config['PROXY_PREFIX'] = '/v1/scix-scan'
+
+        self.assertIsNone(
+            cache_mod.cache_get_search(cache_key),
+            'the SciX deployment must not read content-search annotations cached by ADS')
+
+    @patch('scan_explorer_service.utils.cache.redis.from_url')
+    def test_delete_removes_the_id_from_every_recorded_scope(self, mock_from_url):
+        store = {}
+        scopes = set()
+        client = MagicMock()
+        client.ping.return_value = True
+        client.setex.side_effect = lambda k, ttl, v: store.__setitem__(k, v)
+        client.get.side_effect = store.get
+        client.sadd.side_effect = lambda k, v: scopes.add(v)
+        client.smembers.side_effect = lambda k: set(scopes)
+        client.delete.side_effect = lambda k: store.pop(k, None)
+        mock_from_url.return_value = client
+
+        collection_id = 'ApJ0099'
+        cache_mod.cache_set_manifest(collection_id, '{"ads":1}')
+
+        self.app.config['PROXY_SERVER'] = 'https://scixplorer.org:443'
+        self.app.config['PROXY_PREFIX'] = '/v1/scix-scan'
+        cache_mod.cache_set_manifest(collection_id, '{"scix":1}')
+
+        self.assertEqual(len(store), 2)
+        cache_mod.cache_delete_manifest(collection_id)
+        self.assertEqual(store, {}, 'the collection PUT must clear both deployment scopes')
+
+    @patch('scan_explorer_service.utils.cache.redis.from_url')
+    def test_delete_leaves_other_ids_alone(self, mock_from_url):
+        store = {}
+        scopes = set()
+        client = MagicMock()
+        client.ping.return_value = True
+        client.setex.side_effect = lambda k, ttl, v: store.__setitem__(k, v)
+        client.sadd.side_effect = lambda k, v: scopes.add(v)
+        client.smembers.side_effect = lambda k: set(scopes)
+        client.delete.side_effect = lambda k: store.pop(k, None)
+        mock_from_url.return_value = client
+
+        cache_mod.cache_set_manifest('ApJ0099', '{"a":1}')
+        cache_mod.cache_set_manifest('ApJ00990', '{"b":1}')
+        cache_mod.cache_set_manifest('*', '{"c":1}')
+
+        cache_mod.cache_delete_manifest('ApJ0099')
+
+        remaining = sorted(k.rsplit(':', 1)[-1] for k in store)
+        self.assertEqual(remaining, ['*', 'ApJ00990'])
+    def _fake_redis(self, mock_from_url):
+        store, scopes = {}, set()
+        client = MagicMock()
+        client.ping.return_value = True
+        client.setex.side_effect = lambda k, ttl, v: store.__setitem__(k, v)
+        client.get.side_effect = store.get
+        client.sadd.side_effect = lambda k, v: scopes.add(v)
+        client.smembers.side_effect = lambda k: set(scopes)
+        client.delete.side_effect = lambda *names: [store.pop(n, None) for n in names]
+
+        def _set(name, value, nx=False, ex=None):
+            if ex is None:
+                raise AssertionError('a claim without an expiry would outlive a crashed holder')
+            if nx and name in store:
+                return None
+            store[name] = value
+            return True
+        client.set.side_effect = _set
+        self.redis_client = client
+        mock_from_url.return_value = client
+        return store
+
+    @patch('scan_explorer_service.utils.cache.redis.from_url')
+    def test_bulk_delete_clears_every_id_in_every_scope(self, mock_from_url):
+        store = {}
+        scopes = set()
+        client = MagicMock()
+        client.ping.return_value = True
+        client.setex.side_effect = lambda k, ttl, v: store.__setitem__(k, v)
+        client.sadd.side_effect = lambda k, v: scopes.add(v)
+        client.smembers.side_effect = lambda k: set(scopes)
+        client.delete.side_effect = lambda *names: [store.pop(n, None) for n in names]
+        mock_from_url.return_value = client
+
+        ids = ['ApJ0099', '1988ApJ...333..341R', '1988ApJ...333..352S']
+        for i in ids:
+            cache_mod.cache_set_manifest(i, '{"ads":1}')
+        self.app.config['PROXY_SERVER'] = 'https://scixplorer.org:443'
+        self.app.config['PROXY_PREFIX'] = '/v1/scix-scan'
+        for i in ids:
+            cache_mod.cache_set_manifest(i, '{"scix":1}')
+        cache_mod.cache_set_manifest('untouched', '{"scix":1}')
+
+        self.assertEqual(len(store), 7)
+        cache_mod.cache_delete_manifests(ids)
+
+        remaining = sorted(k.rsplit(':', 1)[-1] for k in store)
+        self.assertEqual(remaining, ['untouched'])
+
+    @patch('scan_explorer_service.utils.cache.redis.from_url')
+    def test_bulk_delete_spans_more_than_one_batch(self, mock_from_url):
+        store = self._fake_redis(mock_from_url)
+        ids = ['id%04d' % i for i in range(cache_mod.DELETE_BATCH_SIZE + 25)]
+        for i in ids:
+            cache_mod.cache_set_manifest(i, '{}')
+        self.assertEqual(len(store), len(ids))
+
+        cache_mod.cache_delete_manifests(ids)
+        leftovers = [k for k in store if k.startswith(cache_mod.MANIFEST_CACHE_PREFIX)]
+        self.assertEqual(leftovers, [], 'every batch must be deleted, not just the first')
+        self.assertGreater(self.redis_client.delete.call_count, 1, 'expected more than one batch')
+
+if __name__ == '__main__':
+    unittest.main()
+
+
 if __name__ == '__main__':
     unittest.main()

scan_explorer_service/tests/test_manifest.py

@@ -5,6 +5,7 @@
 from scan_explorer_service.tests.base import TestCaseDatabase
 from scan_explorer_service.models import Base
 import json
+import opensearchpy
 
 class TestManifest(TestCaseDatabase):
 
@@ -62,6 +63,117 @@ def test_get_canvas(self):
         self.assertStatus(r, 200)
         self.assertEqual(data['@type'], 'sc:Canvas')
 
+    def test_canvas_abstract_link_stays_on_the_readers_host(self):
+        """A hardcoded host would send SciX readers to ADS from the 'About this item' panel."""
+        bibcode = self.article.bibcode
+        url = url_for("manifest.get_manifest", id=self.article.id)
+        r = self.client.get(url)
+        self.assertStatus(r, 200)
+        canvases = json.loads(r.data)['sequences'][0]['canvases']
+        values = [m['value'] for c in canvases for m in c.get('metadata', [])]
+        self.assertTrue(values, 'expected canvas metadata to be present')
+        for value in values:
+            self.assertNotIn('http://', value)
+            self.assertNotIn('https://', value)
+            self.assertIn(f'href="/abs/{bibcode}/abstract"', value)
+
+    def test_a_bibcode_cannot_inject_markup_into_canvas_metadata(self):
+        """The manifest is a public document; a malicious bibcode must not become live markup."""
+        hostile = '"><img src=x onerror=alert(1)>'
+        article = Article(bibcode=hostile, collection_id=self.collection.id)
+        self.app.db.session.add(article)
+        self.app.db.session.commit()
+        self.page.articles.append(article)
+        self.app.db.session.commit()
+
+        r = self.client.get(url_for("manifest.get_manifest", id=self.collection.id))
+        self.assertStatus(r, 200)
+        values = [m['value'] for c in json.loads(r.data)['sequences'][0]['canvases']
+                  for m in c.get('metadata', [])]
+        self.assertTrue(values)
+        for value in values:
+            self.assertNotIn(hostile, value, 'the bibcode must not appear unescaped')
+            self.assertNotIn('<img', value)
+            self.assertNotIn('"><', value)
+        self.assertTrue(any('&lt;img' in v for v in values), 'the markup must survive as escaped text')
+
+    @patch('opensearchpy.OpenSearch')
+    def test_search_skips_hits_that_have_no_highlight(self, OpenSearch):
+        """A stop word is analyzed away, so every page matches with no highlight to show."""
+        hits = [{'_source': {'page_id': self.page.id, 'volume_id': self.page.collection_id,
+                             'page_label': self.page.label,
+                             'page_number': self.page.volume_running_page_num}}]
+        OpenSearch.return_value.search.return_value = {
+            "hits": {"total": {"value": 1, "relation": "eq"}, "max_score": None, "hits": hits}}
+
+        url = url_for("manifest.search", id=self.article.id, q='the')
+        r = self.client.get(url)
+        data = json.loads(r.data)
+        self.assertStatus(r, 200)
+        self.assertEqual(data['@type'], 'sc:AnnotationList')
+        self.assertEqual(data.get('resources', []), [])
+
+    @patch('opensearchpy.OpenSearch')
+    def test_search_reports_an_internal_failure_as_json_500(self, OpenSearch):
+        """Our own failure is a 500, in JSON, and must not leak the exception text."""
+        OpenSearch.return_value.search.side_effect = RuntimeError('could not connect to db.internal')
+
+        url = url_for("manifest.search", id=self.article.id, q='gas')
+        r = self.client.get(url)
+        self.assertStatus(r, 500)
+        self.assertIn('application/json', r.content_type)
+        self.assertNotIn('db.internal', r.data.decode())
+
+    @patch('opensearchpy.OpenSearch')
+    def test_search_reports_a_search_outage_as_503(self, OpenSearch):
+        """A backend outage must be a 5xx so it is visible to alerting."""
+        OpenSearch.return_value.search.side_effect = opensearchpy.exceptions.ConnectionError(
+            'N/A', 'connection refused', Exception('refused'))
+
+        url = url_for("manifest.search", id=self.article.id, q='gas')
+        r = self.client.get(url)
+        self.assertStatus(r, 503)
+        self.assertIn('unavailable', json.loads(r.data)['message'].lower())
+
+    @patch('opensearchpy.OpenSearch')
+    def test_search_reports_a_rejected_query_as_400(self, OpenSearch):
+        """OpenSearch rejecting the query is the caller's problem, not an outage."""
+        OpenSearch.return_value.search.side_effect = opensearchpy.exceptions.RequestError(
+            400, 'search_phase_execution_exception', {'error': 'bad query'})
+
+        url = url_for("manifest.search", id=self.article.id, q='gas')
+        r = self.client.get(url)
+        self.assertStatus(r, 400)
+
+
+class TestCollectionManifest(TestCaseDatabase):
+
+    @patch('opensearchpy.OpenSearch')
+    def test_a_missing_index_is_our_fault_not_an_outage(self, OpenSearch):
+        """A renamed index must not read as 'OpenSearch is down' forever."""
+        OpenSearch.return_value.search.side_effect = opensearchpy.exceptions.NotFoundError(
+            404, 'index_not_found_exception', {'error': 'no such index'})
+
+        r = self.client.get(url_for("manifest.search", id=self.article.id, q='gas'))
+        self.assertStatus(r, 500)
+        self.assertNotIn('no such index', r.data.decode())
+
+    @patch('opensearchpy.OpenSearch')
+    def test_a_bad_credential_is_our_fault_not_an_outage(self, OpenSearch):
+        OpenSearch.return_value.search.side_effect = opensearchpy.exceptions.AuthenticationException(
+            401, 'security_exception', {'error': 'bad credentials'})
+
+        r = self.client.get(url_for("manifest.search", id=self.article.id, q='gas'))
+        self.assertStatus(r, 500)
+
+    @patch('opensearchpy.OpenSearch')
+    def test_an_opensearch_server_error_is_an_outage(self, OpenSearch):
+        OpenSearch.return_value.search.side_effect = opensearchpy.exceptions.TransportError(
+            503, 'search_phase_execution_exception', {'error': 'overloaded'})
+
+        r = self.client.get(url_for("manifest.search", id=self.article.id, q='gas'))
+        self.assertStatus(r, 503)
+
     @patch('opensearchpy.OpenSearch')
     def test_search_article_with_highlight(self, OpenSearch):
         open_search_highlight_response = {"hits":{"total":{"value":1,"relation":"eq"},"max_score":None,"hits":[{'_source':{'page_id':self.page.id, 'volume_id':self.page.collection_id, 'page_label':self.page.label, 'page_number': self.page.volume_running_page_num}, "highlight":{'text':'some <b>highlighted</b> text'}}]}}