From 06f3694c9de2b2fa6c5aedf14b5c87ad32b0e5fa Mon Sep 17 00:00:00 2001 From: Ryan Barrett Date: Wed, 10 Jul 2024 21:04:04 -0700 Subject: [PATCH] Protocol.receive: return HTTP 299 for deterministic errors to prevent retries for https://github.com/snarfed/bridgy-fed/issues/1063 --- protocol.py | 30 +++++++++++++++++++++--------- tests/test_activitypub.py | 4 ++-- tests/test_protocol.py | 26 +++++++++++++------------- 3 files changed, 36 insertions(+), 24 deletions(-) diff --git a/protocol.py b/protocol.py index 97772a2b..9443d6f0 100644 --- a/protocol.py +++ b/protocol.py @@ -19,6 +19,7 @@ from oauth_dropins.webutil import models from oauth_dropins.webutil import util from oauth_dropins.webutil.util import json_dumps, json_loads import werkzeug.exceptions +from werkzeug.exceptions import BadGateway, HTTPException import common from common import ( @@ -26,7 +27,6 @@ from common import ( DOMAIN_BLOCKLIST, DOMAIN_RE, DOMAINS, - error, PRIMARY_DOMAIN, PROTOCOL_DOMAINS, report_error, @@ -62,6 +62,20 @@ seen_ids_lock = Lock() logger = logging.getLogger(__name__) +def error(*args, status=299, **kwargs): + """Default HTTP status code to 299 to prevent retrying task.""" + return common.error(*args, status=status, **kwargs) + + +class ErrorButDoNotRetryTask(HTTPException): + code = 299 + description = 'ErrorButDoNotRetryTask' + +# https://github.com/pallets/flask/issues/1837#issuecomment-304996942 +werkzeug.exceptions.default_exceptions.setdefault(299, ErrorButDoNotRetryTask) +werkzeug.exceptions._aborter.mapping.setdefault(299, ErrorButDoNotRetryTask) + + class Protocol: """Base protocol class. Not to be instantiated; classmethods only. @@ -311,11 +325,11 @@ class Protocol: if protocol.load(id, local=False, remote=True): logger.info(f' {protocol.LABEL} owns id {id}') return protocol - except werkzeug.exceptions.BadGateway: + except BadGateway: # we tried and failed fetching the id over the network. # this depends on ActivityPub.fetch raising this! return None - except werkzeug.exceptions.HTTPException as e: + except HTTPException as e: # internal error we generated ourselves; try next protocol pass except Exception as e: @@ -721,7 +735,7 @@ class Protocol: """Handles an incoming activity. If ``obj``'s key is unset, ``obj.as1``'s id field is used. If both are - unset, raises :class:`werkzeug.exceptions.BadRequest`. + unset, returns HTTP 299. Args: obj (models.Object) @@ -739,8 +753,6 @@ class Protocol: assert isinstance(obj, Object), obj logger.info(f'From {from_cls.LABEL}: {obj.type} {obj.key} AS1: {json_dumps(obj.as1, indent=2)}') - # TODO: return 204 for all of these errors so we don't retry them - # https://cloud.google.com/tasks/docs/creating-appengine-handlers if not obj.as1: error('No object data provided') @@ -789,7 +801,7 @@ class Protocol: # https://www.w3.org/wiki/ActivityPub/Primer/Authentication_Authorization actor = as1.get_owner(obj.as1) if not actor: - error('Activity missing actor or author', status=400) + error('Activity missing actor or author') elif from_cls.owns_id(actor) is False: error(f"{from_cls.LABEL} doesn't own actor {actor}, this is probably a bridged activity. Skipping.", status=204) @@ -800,7 +812,7 @@ class Protocol: if actor != authed_as: report_error("Auth: receive: authed_as doesn't match owner", user=f'{id} authed_as {authed_as} owner {actor}') - error(f"actor {actor} isn't authed user {authed_as}", status=403) + error(f"actor {actor} isn't authed user {authed_as}") # update copy ids to originals obj.normalize_ids() @@ -984,7 +996,7 @@ class Protocol: from_obj = from_cls.load(from_id) if not from_obj: - error(f"Couldn't load {from_id}") + error(f"Couldn't load {from_id}", status=502) if not from_obj.as1: from_obj.our_as1 = from_as1 diff --git a/tests/test_activitypub.py b/tests/test_activitypub.py index 73649ac8..5c5e7df0 100644 --- a/tests/test_activitypub.py +++ b/tests/test_activitypub.py @@ -1065,7 +1065,7 @@ class ActivityPubTest(TestCase): 'actor': ACTOR['id'], 'object': None, }) - self.assertEqual(400, got.status_code) + self.assertEqual(299, got.status_code) def test_inbox_follow_accept_with_id(self, *mocks): self._test_inbox_follow_accept(FOLLOW_WRAPPED, ACCEPT, *mocks) @@ -1548,7 +1548,7 @@ class ActivityPubTest(TestCase): }) headers = sign('/ap/sharedInbox', body, key_id=ACTOR['id']) got = self.client.post('/ap/sharedInbox', data=body, headers=headers) - self.assertEqual(403, got.status_code, got.get_data(as_text=True)) + self.assertEqual(299, got.status_code, got.get_data(as_text=True)) def test_inbox_NO_AUTH_DOMAINS(self, *_): id = 'https://a.gup.pe/a-group' diff --git a/tests/test_protocol.py b/tests/test_protocol.py index 6da4e29c..cce6cc8f 100644 --- a/tests/test_protocol.py +++ b/tests/test_protocol.py @@ -17,7 +17,7 @@ from oauth_dropins.webutil.appengine_config import ndb_client from oauth_dropins.webutil.flask_util import NoContent from oauth_dropins.webutil.testutil import NOW, requests_response import requests -from werkzeug.exceptions import BadRequest, Forbidden +from werkzeug.exceptions import BadRequest # import first so that Fake is defined before URL routes are registered from .testutil import ExplicitEnableFake, Fake, OtherFake, TestCase @@ -28,7 +28,7 @@ from atproto import ATProto import common from models import Follower, Object, PROTOCOLS, Target, User import protocol -from protocol import Protocol +from protocol import ErrorButDoNotRetryTask, Protocol from ui import UIProtocol from web import Web @@ -1188,7 +1188,7 @@ class ProtocolReceiveTest(TestCase): } self.store_object(id='fake:post', our_as1=post_as1, source_protocol='fake') - with self.assertRaises(Forbidden): + with self.assertRaises(ErrorButDoNotRetryTask): Fake.receive_as1({ 'id': 'fake:update', 'objectType': 'activity', @@ -1580,7 +1580,7 @@ class ProtocolReceiveTest(TestCase): self.assertEqual([(like_obj.key.id(), 'fake:post:target')], Fake.sent) def test_like_no_object_error(self): - with self.assertRaises(BadRequest): + with self.assertRaises(ErrorButDoNotRetryTask): Fake.receive_as1({ 'id': 'fake:like', 'objectType': 'activity', @@ -1590,7 +1590,7 @@ class ProtocolReceiveTest(TestCase): }) def test_share_no_object_error(self): - with self.assertRaises(BadRequest): + with self.assertRaises(ErrorButDoNotRetryTask): Fake.receive_as1({ 'id': 'fake:share', 'objectType': 'activity', @@ -1959,7 +1959,7 @@ class ProtocolReceiveTest(TestCase): self.assertEqual([], OtherFake.sent) def test_follow_no_actor(self): - with self.assertRaises(BadRequest): + with self.assertRaises(ErrorButDoNotRetryTask): Fake.receive_as1({ 'id': 'fake:follow', 'objectType': 'activity', @@ -1971,7 +1971,7 @@ class ProtocolReceiveTest(TestCase): self.assertEqual([], Fake.sent) def test_follow_no_object(self): - with self.assertRaises(BadRequest): + with self.assertRaises(ErrorButDoNotRetryTask): Fake.receive_as1({ 'id': 'fake:follow', 'objectType': 'activity', @@ -1983,7 +1983,7 @@ class ProtocolReceiveTest(TestCase): self.assertEqual([], Fake.sent) def test_follow_object_unknown_protocol(self): - with self.assertRaises(BadRequest): + with self.assertRaises(ErrorButDoNotRetryTask): Fake.receive_as1({ 'id': 'fake:follow', 'objectType': 'activity', @@ -2233,7 +2233,7 @@ class ProtocolReceiveTest(TestCase): }) def test_activity_id_blocklisted(self): - with self.assertRaises(BadRequest): + with self.assertRaises(ErrorButDoNotRetryTask): Fake.receive_as1({ 'objectType': 'activity', 'verb': 'delete', @@ -2551,7 +2551,7 @@ class ProtocolReceiveTest(TestCase): self.addCleanup(reset_seen_ids) - # pretend like the two threads below are in different processes + # pretend the two threads below are in different processes protocol.seen_ids = TTLCache(maxsize=10, ttl=0) Follower.get_or_create(to=self.user, from_=self.alice) @@ -2812,7 +2812,7 @@ class ProtocolReceiveTest(TestCase): 'obj': obj.key.urlsafe(), 'authed_as': 'foo.com', }) - self.assertEqual(403, got.status_code) + self.assertEqual(299, got.status_code) self.assertIsNone(Object.get_by_id('http://bar.com/post#bridgy-fed-create')) def test_receive_task_handler_not_authed_as(self): @@ -2826,7 +2826,7 @@ class ProtocolReceiveTest(TestCase): 'obj': obj.key.urlsafe(), 'authed_as': 'fake:eve', }) - self.assertEqual(403, got.status_code) + self.assertEqual(299, got.status_code) self.assertIsNone(Object.get_by_id('fake:post#bridgy-fed-create')) def test_like_not_authed_as_actor(self): @@ -2835,7 +2835,7 @@ class ProtocolReceiveTest(TestCase): 'author': 'fake:bob', } - with self.assertRaises(Forbidden): + with self.assertRaises(ErrorButDoNotRetryTask): Fake.receive_as1({ 'id': 'fake:like', 'objectType': 'activity',