fix: resolve translation keys, and fall back to http-post when https-post is rejected

RssCloud/Registry.php

@@ -16,7 +16,7 @@
  * identifies the resource by the `url` parameter of each notification.
  *
  * @phpstan-type RssCloudState array{url:string,kind:string,endpoint:string,registerProcedure:string,
- *   lease_start:int,last_notify:int,error:bool,error_message:string}
+ *   protocol:string,lease_start:int,last_notify:int,error:bool,error_message:string}
  */
 final class RssCloud_Registry {
 
@@ -82,6 +82,13 @@ private static function normalise(array $state): array {
 			'kind' => ($state['kind'] ?? null) === self::KIND_OPML ? self::KIND_OPML : self::KIND_FEED,
 			'endpoint' => is_string($state['endpoint'] ?? null) ? $state['endpoint'] : '',
 			'registerProcedure' => is_string($state['registerProcedure'] ?? null) ? $state['registerProcedure'] : '',
+			// The `protocol` value this server was last known to accept; empty until one has worked.
+			// Constrained to the two this extension can speak, because whatever is stored here is
+			// tried first: an unrecognised one would be advertised to the cloud server ahead of a
+			// value known to work.
+			'protocol' => in_array($state['protocol'] ?? null,
+				[RssCloud_Endpoint::PROTOCOL_HTTP, RssCloud_Endpoint::PROTOCOL_HTTPS], true)
+				? $state['protocol'] : '',
 			'lease_start' => is_numeric($state['lease_start'] ?? null) ? (int)$state['lease_start'] : 0,
 			'last_notify' => is_numeric($state['last_notify'] ?? null) ? (int)$state['last_notify'] : 0,
 			// Assume broken until a notification actually arrives, like the core WebSub code does.
@@ -112,10 +119,12 @@ public function save(string $resourceUrl, array $state): bool {
 	public function remember(string $resourceUrl, RssCloud_Endpoint $endpoint, string $kind): array {
 		$state = $this->load($resourceUrl) ?? self::normalise(['url' => $resourceUrl]);
 		if ($state['endpoint'] !== $endpoint->url) {
-			// The publisher moved to a different cloud server: start over.
+			// The publisher moved to a different cloud server: start over. The new server need not
+			// accept the same `protocol` value as the old one, so that is forgotten too.
 			$state['lease_start'] = 0;
 			$state['error'] = true;
 			$state['error_message'] = '';
+			$state['protocol'] = '';
 		}
 		$state['kind'] = $kind;
 		$state['endpoint'] = $endpoint->url;

RssCloud/Subscriber.php

@@ -72,7 +72,6 @@ public function subscribe(array $state): bool {
 			'domain' => $this->callback->domain,
 			'port' => (string)$this->callback->port,
 			'path' => $this->callback->path,
-			'protocol' => $this->callback->protocol,
 			'registerProcedure' => $state['registerProcedure'],
 			'url1' => $state['url'],
 		];
@@ -81,27 +80,72 @@ public function subscribe(array $state): bool {
 		$state['lease_start'] = time();
 		$this->registry->save($state['url'], $state);
 
-		$response = FreshRSS_http_Util::httpGet($endpoint->url, null, 'xml', [], [
-			CURLOPT_POSTFIELDS => http_build_query($parameters),
-			CURLOPT_MAXREDIRS => 10,
-		]);
+		$candidates = self::protocolCandidates($state['protocol'], $this->callback->protocol);
+		$message = '';
+		$status = 0;
+
+		foreach ($candidates as $i => $protocol) {
+			$response = FreshRSS_http_Util::httpGet($endpoint->url, null, 'xml', [], [
+				CURLOPT_POSTFIELDS => http_build_query($parameters + ['protocol' => $protocol]),
+				CURLOPT_MAXREDIRS => 10,
+			]);
+			$status = (int)$response['status'];
+			[$success, $message] = self::parseNotifyResult((string)$response['body'], $status);
+
+			$log = 'rssCloud pleaseNotify ' . $state['url'] . ' via ' . $endpoint->url
+				. ' with callback ' . $this->callback->url() . ' as ' . $protocol
+				. ': ' . $status . ' ' . $message;
+
+			if ($success) {
+				// Remembered so that later renewals go straight to the value this server accepts,
+				// instead of failing the preferred one every time.
+				$state['protocol'] = $protocol;
+				$state['error'] = false;
+				$state['error_message'] = '';
+				$this->registry->save($state['url'], $state);
+				Minz_Log::notice($log, RSSCLOUD_LOG);
+				return true;
+			}
 
-		[$success, $message] = self::parseNotifyResult((string)$response['body'], (int)$response['status']);
+			// A negative status is FreshRSS-internal: the request never reached the server, so it
+			// cannot be objecting to the protocol and another value would fail identically.
+			$retrying = $status > 0 && isset($candidates[$i + 1]);
+			if ($retrying) {
+				// Not yet a fault: the fallback may still succeed, and warning here every time
+				// would make a working subscription look broken in the log.
+				Minz_Log::debug($log, RSSCLOUD_LOG);
+			} else {
+				Minz_Log::warning($log, RSSCLOUD_LOG);
+				break;
+			}
+		}
 
-		$state['error'] = !$success;
-		$state['error_message'] = $success ? '' : $message;
+		$state['error'] = true;
+		$state['error_message'] = $message === '' ? "HTTP {$status}" : $message;
 		$this->registry->save($state['url'], $state);
 
-		$log = 'rssCloud pleaseNotify ' . $state['url'] . ' via ' . $endpoint->url
-			. ' with callback ' . $this->callback->url()
-			. ': ' . $response['status'] . ' ' . $message;
-		if ($success) {
-			Minz_Log::notice($log, RSSCLOUD_LOG);
-		} else {
-			Minz_Log::warning($log, RSSCLOUD_LOG);
-		}
+		return false;
+	}
 
-		return $success;
+	/**
+	 * The `protocol` parameter of `pleaseNotify` names the notification method — `http-post` for
+	 * REST, as against `xml-rpc` or `soap` — and not the scheme of the callback, which is carried by
+	 * `port`. Servers disagree about this: some accept `https-post` as a TLS-flavoured spelling,
+	 * while others take only the value the specification lists, so an HTTPS callback cannot simply
+	 * assume either one.
+	 *
+	 * The value this server last accepted is therefore tried first, falling back to plain
+	 * `http-post`, which every server understands. Registering over a plain HTTP callback has
+	 * nothing to fall back to, and yields a single candidate.
+	 *
+	 * @return non-empty-list<string>
+	 */
+	public static function protocolCandidates(string $remembered, string $callbackProtocol): array {
+		$preferred = $remembered !== '' ? $remembered : $callbackProtocol;
+		if ($preferred === RssCloud_Endpoint::PROTOCOL_HTTP) {
+			return [$preferred];
+		}
+		return [$preferred, RssCloud_Endpoint::PROTOCOL_HTTP];
 	}
 
 	/**

i18n/en/ext.php

@@ -1,41 +1,68 @@
 <?php
 
+/**
+ * Minz_Translate splits a key on every dot and walks the array one level per segment, so a literal
+ * 'callback.help' key is unreachable: the lookup descends into 'callback' and then looks for a
+ * 'help' child of it. Nested arrays are therefore required, with '_' holding the value of a key
+ * that is itself also a parent. This mirrors core's own i18n files.
+ */
+
 return array(
 	'rsscloud' => array(
-		'base_url' => 'Public base URL override',
-		'base_url.help' => 'Leave empty to use the instance base URL. Set this when a reverse proxy makes FreshRSS reachable under a different scheme, host or port than it sees internally.',
-		'callback' => 'Notification callback',
-		'callback.help' => 'This is the address advertised to rssCloud servers. It must be reachable from the public internet, otherwise the registration handshake fails and subscriptions are cancelled.',
-		'callback.invalid' => 'No usable callback URL could be derived. Check the base URL below and the instance base_url setting.',
-		'callback.private' => 'This address does not look publicly reachable, so rssCloud registration will fail.',
-		'cooldown' => 'Notification cooldown (seconds)',
-		'cooldown.help' => 'Minimum delay between two honoured notifications for the same resource. The callback is unauthenticated, so this bounds how often a stranger can make this server do work.',
-		'feeds_enabled' => 'Subscribe to feeds',
-		'feeds_enabled.help' => 'Use rssCloud for feeds advertising a <cloud> or <source:cloud> element.',
-		'opml_enabled' => 'Subscribe to dynamic OPML',
-		'opml_enabled.help' => 'Use rssCloud for dynamic OPML subscription lists advertising <source:cloud> in their <head>.',
-		'regenerate_token' => 'Regenerate callback token',
-		'regenerate_token.help' => 'Changes the secret path segment of the callback. Existing subscriptions stop being delivered until they are renewed.',
-		'renew_hours' => 'Renew subscriptions after (hours)',
-		'renew_hours.help' => 'rssCloud does not negotiate a lease duration. Subscriptions expire after 25 hours and are meant to be renewed every 24, so the default of 23 leaves a margin. Anything above 24 would simply lapse, and is capped.',
-		'skip_polling' => 'Skip polling covered resources',
-		'skip_polling.help' => 'Stop polling a resource on a timer while its cloud subscription is healthy. It is still polled if it goes stale, or when refreshed individually.',
-		'status' => 'Subscriptions',
-		'status.active' => 'Active',
-		'status.endpoint' => 'Cloud server',
-		'status.error' => 'Failed',
-		'status.help' => 'One row per resource this instance has discovered a cloud server for. "Renewed" is when registration was last attempted, not when it last succeeded; the state column says whether it did.',
-		'status.kind' => 'Type',
-		'status.kind.feed' => 'Feed',
-		'status.kind.opml' => 'Dynamic OPML',
-		'status.never' => 'Never',
-		'status.none' => 'No cloud servers have been discovered yet. Resources are registered as they are refreshed, so this fills in once feeds advertising a cloud have been fetched at least once.',
-		'status.notified' => 'Last notified',
-		'status.pending' => 'Pending',
-		'status.renewed' => 'Renewal attempted',
-		'status.resource' => 'Resource',
-		'status.stale' => 'Stale',
-		'status.state' => 'State',
-		'status.subscribers' => 'Users',
+		'base_url' => array(
+			'_' => 'Public base URL override',
+			'help' => 'Leave empty to use the instance base URL. Set this when a reverse proxy makes FreshRSS reachable under a different scheme, host or port than it sees internally.',
+		),
+		'callback' => array(
+			'_' => 'Notification callback',
+			'help' => 'This is the address advertised to rssCloud servers. It must be reachable from the public internet, otherwise the registration handshake fails and subscriptions are cancelled.',
+			'invalid' => 'No usable callback URL could be derived. Check the base URL below and the instance base_url setting.',
+			'private' => 'This address does not look publicly reachable, so rssCloud registration will fail.',
+		),
+		'cooldown' => array(
+			'_' => 'Notification cooldown (seconds)',
+			'help' => 'Minimum delay between two honoured notifications for the same resource. The callback is unauthenticated, so this bounds how often a stranger can make this server do work.',
+		),
+		'feeds_enabled' => array(
+			'_' => 'Subscribe to feeds',
+			'help' => 'Use rssCloud for feeds advertising a <cloud> or <source:cloud> element.',
+		),
+		'opml_enabled' => array(
+			'_' => 'Subscribe to dynamic OPML',
+			'help' => 'Use rssCloud for dynamic OPML subscription lists advertising <source:cloud> in their <head>.',
+		),
+		'regenerate_token' => array(
+			'_' => 'Regenerate callback token',
+			'help' => 'Changes the secret path segment of the callback. Existing subscriptions stop being delivered until they are renewed.',
+		),
+		'renew_hours' => array(
+			'_' => 'Renew subscriptions after (hours)',
+			'help' => 'rssCloud does not negotiate a lease duration. Subscriptions expire after 25 hours and are meant to be renewed every 24, so the default of 23 leaves a margin. Anything above 24 would simply lapse, and is capped.',
+		),
+		'skip_polling' => array(
+			'_' => 'Skip polling covered resources',
+			'help' => 'Stop polling a resource on a timer while its cloud subscription is healthy. It is still polled if it goes stale, or when refreshed individually.',
+		),
+		'status' => array(
+			'_' => 'Subscriptions',
+			'active' => 'Active',
+			'endpoint' => 'Cloud server',
+			'error' => 'Failed',
+			'help' => 'One row per resource this instance has discovered a cloud server for. "Renewal attempted" is when registration was last tried, not when it last succeeded; the state column says whether it did.',
+			'kind' => array(
+				'_' => 'Type',
+				'feed' => 'Feed',
+				'opml' => 'Dynamic OPML',
+			),
+			'never' => 'Never',
+			'none' => 'No cloud servers have been discovered yet. Resources are registered as they are refreshed, so this fills in once feeds advertising a cloud have been fetched at least once.',
+			'notified' => 'Last notified',
+			'pending' => 'Pending',
+			'renewed' => 'Renewal attempted',
+			'resource' => 'Resource',
+			'stale' => 'Stale',
+			'state' => 'State',
+			'subscribers' => 'Users',
+		),
 	),
 );

tests/RssCloud/RegistryTest.php

@@ -32,7 +32,9 @@ protected function tearDown(): void {
 	/** Write a state file straight to disk, bypassing save(), so malformed content can be staged. */
 	private function writeRaw(string $resourceUrl, string $json): void {
 		$directory = $this->registry->directory($resourceUrl);
-		mkdir($directory, 0770, true);
+		if (!is_dir($directory)) {
+			mkdir($directory, 0770, true);
+		}
 		if ($json !== '') {
 			file_put_contents($directory . '/!cloud.json', $json);
 		}
@@ -114,6 +116,40 @@ public function test_load_treatsMissingErrorFlagAsError(): void {
 		self::assertTrue($state['error']);
 	}
 
+	/** Nothing is known about which protocol a server accepts until one has actually worked. */
+	public function test_load_defaultsProtocolToUnknown(): void {
+		$this->writeState('https://j.example/feed', []);
+
+		$state = $this->registry->load('https://j.example/feed');
+
+		self::assertIsArray($state);
+		self::assertSame('', $state['protocol']);
+	}
+
+	/**
+	 * Whatever is stored here is tried first, so a value this extension cannot speak must not
+	 * survive being read back and be advertised to a cloud server.
+	 */
+	public function test_load_discardsUnrecognisedProtocol(): void {
+		foreach (['xml-rpc', 'soap', 'HTTP-POST', 'nonsense', ''] as $stored) {
+			$this->writeState('https://n.example/feed', ['protocol' => $stored]);
+
+			$state = $this->registry->load('https://n.example/feed');
+
+			self::assertIsArray($state);
+			self::assertSame('', $state['protocol'], "stored protocol: {$stored}");
+		}
+	}
+
+	public function test_load_preservesRememberedProtocol(): void {
+		$this->writeState('https://k.example/feed', ['protocol' => RssCloud_Endpoint::PROTOCOL_HTTP]);
+
+		$state = $this->registry->load('https://k.example/feed');
+
+		self::assertIsArray($state);
+		self::assertSame(RssCloud_Endpoint::PROTOCOL_HTTP, $state['protocol']);
+	}
+
 	public function test_all_yieldsOnlyUsableStateAndReportsTheRest(): void {
 		$this->writeState('https://a.example/feed', ['endpoint' => 'https://rpc.example/x']);
 		$this->writeRaw('https://b.example/feed', '{not json');
@@ -187,6 +223,7 @@ public function test_remember_resetsLeaseWhenEndpointChanges(): void {
 			'endpoint' => 'https://old.example/pleaseNotify',
 			'lease_start' => 999,
 			'error' => false,
+			'protocol' => RssCloud_Endpoint::PROTOCOL_HTTP,
 		]);
 
 		$endpoint = RssCloud_Endpoint::fromUrl('https://new.example/pleaseNotify');
@@ -195,6 +232,8 @@ public function test_remember_resetsLeaseWhenEndpointChanges(): void {
 
 		self::assertSame(0, $moved['lease_start']);
 		self::assertTrue($moved['error']);
+		// A different server need not accept what the old one did.
+		self::assertSame('', $moved['protocol']);
 	}
 
 	public function test_remember_keepsLeaseWhenEndpointIsUnchanged(): void {

tests/RssCloud/SubscriberTest.php

@@ -0,0 +1,65 @@
+<?php
+declare(strict_types=1);
+
+/**
+ * Tests for the parts of RssCloud_Subscriber that do not need a cloud server.
+ *
+ * `protocolCandidates()` decides which `protocol` value a `pleaseNotify` advertises, which is the
+ * one thing servers were found to disagree about: some accept `https-post` for a TLS callback,
+ * others only the `http-post` the specification lists, with the scheme carried by `port`.
+ */
+class SubscriberTest extends \PHPUnit\Framework\TestCase {
+
+	public function test_httpsCallbackFallsBackToHttpPost(): void {
+		self::assertSame(
+			[RssCloud_Endpoint::PROTOCOL_HTTPS, RssCloud_Endpoint::PROTOCOL_HTTP],
+			RssCloud_Subscriber::protocolCandidates('', RssCloud_Endpoint::PROTOCOL_HTTPS),
+		);
+	}
+
+	/** A plain HTTP callback already advertises the universal value, so there is nowhere to fall. */
+	public function test_httpCallbackHasNothingToFallBackTo(): void {
+		self::assertSame(
+			[RssCloud_Endpoint::PROTOCOL_HTTP],
+			RssCloud_Subscriber::protocolCandidates('', RssCloud_Endpoint::PROTOCOL_HTTP),
+		);
+	}
+
+	/** Once a server has accepted http-post, the rejected value is not tried again. */
+	public function test_rememberedHttpPostIsUsedAlone(): void {
+		self::assertSame(
+			[RssCloud_Endpoint::PROTOCOL_HTTP],
+			RssCloud_Subscriber::protocolCandidates(RssCloud_Endpoint::PROTOCOL_HTTP, RssCloud_Endpoint::PROTOCOL_HTTPS),
+		);
+	}
+
+	/** A remembered https-post keeps its fallback, in case the server's behaviour changes back. */
+	public function test_rememberedHttpsPostKeepsTheFallback(): void {
+		self::assertSame(
+			[RssCloud_Endpoint::PROTOCOL_HTTPS, RssCloud_Endpoint::PROTOCOL_HTTP],
+			RssCloud_Subscriber::protocolCandidates(RssCloud_Endpoint::PROTOCOL_HTTPS, RssCloud_Endpoint::PROTOCOL_HTTP),
+		);
+	}
+
+	/** The remembered value wins over the callback's own, since it is evidence rather than a guess. */
+	public function test_rememberedValueTakesPrecedence(): void {
+		$candidates = RssCloud_Subscriber::protocolCandidates(
+			RssCloud_Endpoint::PROTOCOL_HTTP,
+			RssCloud_Endpoint::PROTOCOL_HTTPS,
+		);
+
+		self::assertSame(RssCloud_Endpoint::PROTOCOL_HTTP, $candidates[0]);
+	}
+
+	/** Whatever the inputs, the caller always has at least one value to send. */
+	public function test_alwaysYieldsAtLeastOneCandidate(): void {
+		foreach (['', RssCloud_Endpoint::PROTOCOL_HTTP, RssCloud_Endpoint::PROTOCOL_HTTPS] as $remembered) {
+			foreach ([RssCloud_Endpoint::PROTOCOL_HTTP, RssCloud_Endpoint::PROTOCOL_HTTPS] as $callback) {
+				$candidates = RssCloud_Subscriber::protocolCandidates($remembered, $callback);
+				self::assertNotSame([], $candidates, "remembered=$remembered callback=$callback");
+				self::assertContains(RssCloud_Endpoint::PROTOCOL_HTTP, $candidates,
+					"http-post must always remain reachable: remembered=$remembered callback=$callback");
+			}
+		}
+	}
+}

tests/bootstrap.php

@@ -27,3 +27,4 @@
 // involved here, so the classes under test are required explicitly.
 require dirname(__DIR__) . '/RssCloud/Endpoint.php';
 require dirname(__DIR__) . '/RssCloud/Registry.php';
+require dirname(__DIR__) . '/RssCloud/Subscriber.php';