From 13c5fc958478b43b0204bc7f12524de57c5134f0 Mon Sep 17 00:00:00 2001 From: QIU Date: Wed, 26 Aug 2026 02:11:55 +0000 Subject: [PATCH] fix(wavelog): the response reader lost contacts three more ways An audit cloned the Wavelog server and read the QSO endpoints rather than the documentation. The previous commit had transcribed the wrong endpoint - `success`, `successful` and `dupe` come from create_station; the QSO path answers `created` on success and `abort` with a 400 when a record in a batch failed. Three defects followed, and the worst reintroduced the very failure the class was written to prevent. Matching the bare word "duplicate" anywhere in the body classified a hard rejection as a duplicate, which maps to success and drops the QSO from the queue. This is not hypothetical: the server's own rejection text is "Duplicate for ", built in Logbook_model::import, and Api_v2 puts strip_tags'd copies of those messages into validation_error bodies. Probed against real response bodies, five of ten lost the contact. Only the status field counts now, or a 409. An HTML body was accepted. A reverse proxy, a maintenance page or a PHP fatal answers 200 with HTML and no status token, so it fell through to Accepted and a misconfigured proxy ate contacts silently. A body starting with `<` is Unreadable, which keeps the QSO queued. A rejection from v2 stopped the upload. v2 refuses a legacy v1 key with 401 invalid_token - the app sends the v1 key as a Bearer token, and Api_v2::authenticate requires a wl2_ prefix - so a v1-only operator could not upload at all. That was a regression against the pre-fix code, which fell through on any non-2xx. Both v2 and the first v1 endpoint now always fall through; only the last one is final, and the failure message carries the most specific reason any endpoint gave plus all three status codes. The third was previously dropped from that message. Also: the v2 error envelope has no status key at all, so `"error":` is now recognised on its own. --- .../core/domain/wavelog/WaveLogApi.kt | 31 ++++--- .../core/domain/wavelog/WavelogResponse.kt | 32 +++++-- .../domain/wavelog/WavelogResponseTest.kt | 86 +++++++++++++++++++ 3 files changed, 132 insertions(+), 17 deletions(-) diff --git a/core/domain/src/main/java/com/rtbishop/look4sat/core/domain/wavelog/WaveLogApi.kt b/core/domain/src/main/java/com/rtbishop/look4sat/core/domain/wavelog/WaveLogApi.kt index cef7bbe3..63e9cfc8 100644 --- a/core/domain/src/main/java/com/rtbishop/look4sat/core/domain/wavelog/WaveLogApi.kt +++ b/core/domain/src/main/java/com/rtbishop/look4sat/core/domain/wavelog/WaveLogApi.kt @@ -242,13 +242,14 @@ object WaveLogApi { // QSO arrives as HTTP 200 with {"status":"failed"}. Trusting the code marked it uploaded // and dropped it from the queue. val (code, resp) = httpRequest("$base/index.php/api/v2/qso", "POST", apiKey, v2Body.toString()) - when (val verdict = WavelogResponse.verdict(code, resp)) { + val v2Verdict = WavelogResponse.verdict(code, resp) + when (v2Verdict) { is WavelogResponse.Verdict.Accepted -> return@withContext WavelogResult.Success("v2") WavelogResponse.Verdict.Duplicate -> return@withContext WavelogResult.Success("duplicate") - is WavelogResponse.Verdict.Rejected -> - return@withContext WavelogResult.Failure(verdict.reason) - // Unreadable falls through to v1: an older server may not have the v2 endpoint at all. - is WavelogResponse.Verdict.Unreadable -> Unit + // Anything else falls through to v1. A rejection here is NOT final: v2 refuses a legacy + // v1 key with 401 invalid_token, and returning at that point stopped a v1-only operator + // from uploading at all. The v1 attempt below is the one that can speak for them. + else -> Unit } // v1: POST /index.php/api/qso (key in body + ADIF) @@ -259,12 +260,13 @@ object WaveLogApi { put("string", toAdif(qso, gridsquare, satName)) } val (code1, resp1) = httpRequest("$base/index.php/api/qso", "POST", apiKey, v1Body.toString()) - when (val verdict = WavelogResponse.verdict(code1, resp1)) { + val v1Verdict = WavelogResponse.verdict(code1, resp1) + when (v1Verdict) { is WavelogResponse.Verdict.Accepted -> return@withContext WavelogResult.Success("v1") WavelogResponse.Verdict.Duplicate -> return@withContext WavelogResult.Success("duplicate") - is WavelogResponse.Verdict.Rejected -> - return@withContext WavelogResult.Failure(verdict.reason) - is WavelogResponse.Verdict.Unreadable -> Unit + // Also falls through: a server with different rewrite rules answers this path with a + // 404 page, which is a rejection but says nothing about whether the QSO can be stored. + else -> Unit } // v1 without index.php, for a server whose rewrite rules differ @@ -279,8 +281,17 @@ object WaveLogApi { // Every endpoint answered something we could not read. Keeping the QSO queued is the only // honest outcome: it may have been stored, and dropping it would lose the contact. + // No endpoint accepted it. The v2 reason is preferred when it explained itself, since a 401 + // invalid_token is the most actionable thing an operator can be told; otherwise all three + // status codes go out, because the third was previously dropped from this message. + val reasons = listOfNotNull( + (v1Verdict as? WavelogResponse.Verdict.Rejected)?.reason, + (v2Verdict as? WavelogResponse.Verdict.Rejected)?.reason + ).filter { it.isNotBlank() } WavelogResult.Failure( - "unreadable response: v2 HTTP $code, v1 HTTP $code1 - ${shortError(resp1.ifBlank { resp1b })}" + reasons.firstOrNull() + ?: ("no endpoint accepted it: v2 HTTP $code, v1 HTTP $code1, v1-alt HTTP $code1b" + + " - " + shortError(resp1.ifBlank { resp1b })) ) } diff --git a/core/domain/src/main/java/com/rtbishop/look4sat/core/domain/wavelog/WavelogResponse.kt b/core/domain/src/main/java/com/rtbishop/look4sat/core/domain/wavelog/WavelogResponse.kt index aa6af9a2..1296f1a8 100644 --- a/core/domain/src/main/java/com/rtbishop/look4sat/core/domain/wavelog/WavelogResponse.kt +++ b/core/domain/src/main/java/com/rtbishop/look4sat/core/domain/wavelog/WavelogResponse.kt @@ -61,17 +61,27 @@ object WavelogResponse { val lower = text.lowercase().replace(AROUND_SEPARATORS, "") // Duplicate reported three ways: a 409, the word in a message, or Wavelog's own // `{"status":"dupe"}` - which arrives with HTTP 200 and is documented, not guessed. - if (statusCode == DUPLICATE_CODE || - DUPLICATE_MARKERS.any { lower.contains(it) } - ) { - return Verdict.Duplicate - } + // An HTML body is a proxy or a PHP fatal, never a verdict. Checked first because such a + // page carries no status token and would otherwise read as an acceptance, so a + // misconfigured reverse proxy answering 200 would eat contacts. + if (lower.startsWith("<")) return Verdict.Unreadable(text.take(MAX_DETAIL)) + + // Duplicate only when the STATUS says so, or on a 409. The word alone is not enough: the + // server's own rejection text is "Duplicate for ", and Api_v2 surfaces that inside + // validation_error bodies - so matching the bare substring classified a hard rejection as + // a duplicate, which maps to success and drops the QSO from the queue. That is the very + // defect this class was written to prevent, reached through a different door. + if (statusCode == DUPLICATE_CODE || lower.contains(DUPE_STATUS)) return Verdict.Duplicate if (statusCode !in SUCCESS_CODES) { return Verdict.Rejected(reasonFrom(text).ifBlank { "HTTP $statusCode" }) } if (FAILURE_MARKERS.any { lower.contains(it) }) { return Verdict.Rejected(reasonFrom(text).ifBlank { "server reported failure" }) } + // The v2 endpoint reports errors in an envelope with no status key at all. + if (lower.contains("\"error\":")) { + return Verdict.Rejected(reasonFrom(text).ifBlank { "server reported an error" }) + } // A status field we cannot read is not a success. Saying so keeps the QSO queued. if (lower.contains("\"status\"") && SUCCESS_MARKERS.none { lower.contains(it) }) { return Verdict.Unreadable(text.take(MAX_DETAIL)) @@ -115,15 +125,23 @@ object WavelogResponse { * `successful` matters as much as `success`: the API reference uses both, and treating one as * unrecognised would leave a stored QSO queued forever. */ + /** + * Success wordings, taken from the Wavelog server source rather than its documentation. + * + * The QSO endpoint answers `created`; `success` and `successful` come from other endpoints and + * are kept because a future version may use them. `abort` is NOT here - v1 uses it when any + * record in a batch failed, and it arrives with a 400. + */ private val SUCCESS_MARKERS = listOf( + "\"status\":\"created\"", "\"status\":\"success\"", "\"status\":\"successful\"", - "\"status\":\"created\"", "\"status\":\"ok\"" ) /** `dupe` is Wavelog's own wording and arrives with a 200. */ - private val DUPLICATE_MARKERS = listOf("\"status\":\"dupe\"", "duplicate") + /** Wavelog's own duplicate status. The bare word "duplicate" is deliberately NOT a marker. */ + private const val DUPE_STATUS = "\"status\":\"dupe\"" /** Whitespace next to a colon or comma, which JSON allows and servers use inconsistently. */ private val AROUND_SEPARATORS = Regex("""\s*(?=[:,])|(?<=[:,])\s*""") diff --git a/core/domain/src/test/java/com/rtbishop/look4sat/core/domain/wavelog/WavelogResponseTest.kt b/core/domain/src/test/java/com/rtbishop/look4sat/core/domain/wavelog/WavelogResponseTest.kt index db11bb8c..b5a9ece7 100644 --- a/core/domain/src/test/java/com/rtbishop/look4sat/core/domain/wavelog/WavelogResponseTest.kt +++ b/core/domain/src/test/java/com/rtbishop/look4sat/core/domain/wavelog/WavelogResponseTest.kt @@ -134,6 +134,92 @@ class WavelogResponseTest { ) } + /** + * The defect that lost contacts. Wavelog's own rejection text is "Duplicate for ", and + * Api_v2 surfaces that inside validation_error bodies - so matching the bare word classified a + * hard rejection as a duplicate, which maps to success and drops the QSO from the queue. + * + * Bodies transcribed from the Wavelog server source, not from its documentation. + */ + @Test + fun `the word duplicate in a rejection does not make it a duplicate`() { + val bodies = listOf( + 400 to """{"error":{"code":"validation_error","message":"duplicate submode is invalid"}}""", + 400 to """{"status":"failed","reason":"duplicate key violation; QSO NOT stored"}""", + 200 to """{"status":"failed","reason":"Duplicate for BG7NTA"}""" + ) + for ((code, body) in bodies) { + val verdict = WavelogResponse.verdict(code, body) + assertTrue( + "must be a rejection, got " + verdict + " for " + body, + verdict is WavelogResponse.Verdict.Rejected + ) + } + } + + /** A success that mentions the word must still be a success. */ + @Test + fun `the word duplicate in a success does not make it a failure`() { + val verdict = WavelogResponse.verdict( + 201, + """{"status":"created","adif_errors":0,"messages":["Removed duplicate mode entry"]}""" + ) + assertTrue("got " + verdict, verdict is WavelogResponse.Verdict.Accepted) + } + + /** + * A reverse proxy or a PHP fatal answers 200 with HTML. There is no status token, so it used to + * read as a stored QSO and a misconfigured proxy would eat contacts silently. + */ + @Test + fun `an html body is never an acceptance`() { + val pages = listOf( + "502 Bad Gateway", + "

Maintenance

", + "
Fatal error: Uncaught Error in /var/www/index.php" + ) + for (page in pages) { + val verdict = WavelogResponse.verdict(200, page) + assertTrue( + "html must not be accepted, got " + verdict, + verdict is WavelogResponse.Verdict.Unreadable + ) + } + } + + /** The v2 endpoint reports errors in an envelope with no status key at all. */ + @Test + fun `a v2 error envelope is a rejection`() { + val verdict = WavelogResponse.verdict( + 401, + """{"error":{"code":"invalid_token","message":"Bearer token must start with wl2_"}}""" + ) + assertEquals( + WavelogResponse.Verdict.Rejected("Bearer token must start with wl2_"), + verdict + ) + } + + /** The QSO endpoint answers `created`, which is what the v1 source actually emits. */ + @Test + fun `the created status the qso endpoint returns is accepted`() { + val verdict = WavelogResponse.verdict( + 201, + """{"status":"created","adif_count":1,"adif_errors":0,"messages":[""]}""" + ) + assertTrue("got " + verdict, verdict is WavelogResponse.Verdict.Accepted) + } + + /** v1 uses `abort` with a 400 when any record in a batch failed. Not a success. */ + @Test + fun `an abort status is a rejection`() { + val verdict = WavelogResponse.verdict( + 400, + """{"status":"abort","messages":["Bad ADIF field CALL"]}""" + ) + assertTrue("got " + verdict, verdict is WavelogResponse.Verdict.Rejected) + } + /** A failure with no explanation still has to say something usable. */ @Test fun `a failure without a reason still reports one`() {