mckero/Look4Sat-mckero
1
0 Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 20 Wiki Activity

fix(aprs): the login line was malformed, and the refusal was invisible

Browse Source 
An auditor ran the plan's own release gate against live APRS-IS servers. It failed at
the login step, on every server tried:

    sent: user N0CALL pass -1 vers Look4Sat-4.5.4
    got:  # Invalid login: software name and version are not separated by a space

Reproduced on euro.aprs2.net and noam.aprs2.net, aprsc 2.1.21. `vers` takes TWO tokens,
a software name and a version. An earlier commit read the rule "softwarename must not
contain a space" as "the field must be one token" and hyphenated the space between them
- and the unit test asserted that as correct, so the mistake was frozen in place.

Worse than the malformed line was what happened next. `# Invalid login:` is a comment
but not a logresp, so parse skipped it as keepalive chatter; the login then timed out
into Unknown, which is deliberately treated as "may be working"; so `ok = sent &&
!refused` was true and the operator was shown "APRS: report sent OK" for a login the
server had refused. That is v4.6.0's defining defect - every send reported successful
regardless of outcome - still live on the exact path every operator takes. The rebuild
narrowed it rather than closing it.

Both halves are fixed: the name and version stay separate tokens with whitespace
collapsed within each, and a refusal comment is classified as a refusal before the
logresp test. A socket test now replays the server's actual bytes.

Three smaller things from the same review:

The foreground service type goes back to dataSync. The previous commit chose location
to escape dataSync's six-hour cap, but a location-typed service is refused outright
unless a location runtime permission has already been granted, and the settings card
requests only notifications - so it would have failed silently for anyone who declined
location access. The cap that prompted the switch applies only when targetSdk is 35 or
higher, which this project does not declare. A test now reads the manifest and the
service source and fails if they disagree, which is the only way this class of defect
is visible from a JVM test.

The version string in the login was 4.5.4 while the app was 4.6.0. Now split into name
and version and corrected, though it is still hardcoded - core:data has no BuildConfig,
so passing it in properly is a separate change.

The passcode hint said "empty = auto-computed from callsign" in all five locales. The
app stopped doing that two commits ago; it now connects receive-only, and the hint says
so. It was the first thing an operator read next to the field, promising the behaviour
that was deliberately removed.

Not fixed, and known: the notification body is rebuilt from the previous cycle's state
so it can show a stale verdict, a deliberate receive-only choice is still styled as an
error, and no last-success timestamp exists - so an operator still cannot establish
whether their station has ever reached the network.
This commit is contained in:
mckero committed 2026-08-25 16:04:47 +00:00
1 parent 0208a577c4
commit 321cd8f2fa
13 files changed
+297 -56
Download Patch File Download Diff File Expand all files Collapse all files

No files matched your search

core/domain/src/main/java/com/rtbishop/look4sat/core/domain/aprs/AprsLogin.kt
+23 -5
View File
@@ -51,26 +51,36 @@ object AprsLogin {
data class Unknown(val detail: String) : Outcome
}
/** Any run of whitespace, collapsed to a hyphen inside a single token. */
private val WHITESPACE = Regex("""\s+""")
/** Passcode value that asks for a receive-only connection. */
const val RECEIVE_ONLY_PASSCODE = -1
/**
* Build the login line.
*
* [version] must not contain a space: the server splits `vers` into a software name and a
* version, so an embedded space shifts every field after it. Any space becomes a hyphen
* rather than silently corrupting the line.
* `vers` takes TWO tokens - a software name and a version, separated by a space. An earlier
* version of this replaced that space with a hyphen, reading the rule "softwarename must not
* contain a space" as "the field must be a single token". Live aprsc 2.1.21 rejects the result:
*
* sent: user N0CALL pass -1 vers Look4Sat-4.5.4
* got: # Invalid login: software name and version are not separated by a space
*
* So name and version stay apart, and whitespace is collapsed WITHIN each of them instead.
*/
fun line(
callsign: String,
ssid: String,
passcode: Int,
name: String,
version: String,
filter: String = ""
): String {
val callSsid = AprsPacket.formatCallSsid(callsign, ssid)
val safeVersion = version.trim().replace(' ', '-').ifEmpty { "Look4Sat" }
val base = "user $callSsid pass $passcode vers $safeVersion"
val safeName = name.trim().replace(WHITESPACE, "-").ifEmpty { "Look4Sat" }
val safeVersion = version.trim().replace(WHITESPACE, "-").ifEmpty { "0" }
val base = "user $callSsid pass $passcode vers $safeName $safeVersion"
val trimmedFilter = filter.trim()
return if (trimmedFilter.isEmpty()) base else "$base $trimmedFilter"
}
@@ -93,6 +103,14 @@ object AprsLogin {
return Outcome.Rejected(trimmed)
}
val lower = trimmed.lowercase()
// A refusal arrives as a comment and is NOT a logresp: aprsc answers `# Invalid login: ...`
// and then closes. Treating that as chatter let the login time out into Unknown, which is
// deliberately optimistic - so every send afterwards reported success to an operator the
// server had refused. That was the defect this whole rebuild set out to remove, still live
// on the path every operator takes.
if (lower.contains("invalid login") || lower.contains("login denied")) {
return Outcome.Rejected(trimmed.removePrefix("#").trim())
}
if (!lower.contains("logresp")) {
// Server identification and keepalive comments carry no verdict.
return null
core/domain/src/test/java/com/rtbishop/look4sat/core/domain/aprs/AprsLoginTest.kt
+76 -29
View File
@@ -6,59 +6,103 @@ import org.junit.Assert.assertTrue
import org.junit.Test
/**
* The verified/unverified distinction is the whole point of these tests: an unverified client
* stays connected and its writes keep succeeding while the server discards every packet, so
* getting this wrong means the operator watches reports "succeed" for hours with nothing landing.
* The verified/unverified distinction is the point of these tests: an unverified client stays
* connected and its writes keep succeeding while the server discards every packet, so getting this
* wrong means the operator watches reports "succeed" for hours with nothing landing.
*
* Several are transcriptions of live aprsc 2.1.21 traffic rather than guesses, because an earlier
* version of this file froze a wrong login format as correct and that defect survived nine commits
* and two audits.
*/
class AprsLoginTest {
@Test
fun `builds the line the specification asks for`() {
assertEquals(
"user BG7NTA-5 pass 12345 vers Look4Sat-4.6.0",
AprsLogin.line("BG7NTA", "5", 12345, "Look4Sat-4.6.0")
"user BG7NTA-5 pass 12345 vers Look4Sat 4.6.0",
AprsLogin.line("BG7NTA", "5", 12345, "Look4Sat", "4.6.0")
)
}
/**
* The format a live server rejects, and the reason this file was rewritten.
*
* sent: user N0CALL pass -1 vers Look4Sat-4.5.4
* got: # Invalid login: software name and version are not separated by a space
*
* `vers` is two tokens. The previous code hyphenated the space between them to satisfy the rule
* that the software NAME contains no space, and the test asserted that as correct.
*/
@Test
fun `the software name and version stay separate tokens`() {
val line = AprsLogin.line("N0CALL", "", AprsLogin.RECEIVE_ONLY_PASSCODE, "Look4Sat", "4.5.4")
assertEquals("user N0CALL pass -1 vers Look4Sat 4.5.4", line)
assertEquals(7, line.split(" ").size)
assertTrue("name and version must not be joined", !line.contains("Look4Sat-4.5.4"))
}
/** No SSID means no hyphen; the spec says never to write -0 explicitly. */
@Test
fun `omits the ssid when there is none`() {
assertEquals(
"user BG7NTA pass 12345 vers Look4Sat",
AprsLogin.line("BG7NTA", "", 12345, "Look4Sat")
"user BG7NTA pass 12345 vers Look4Sat 4.6.0",
AprsLogin.line("BG7NTA", "", 12345, "Look4Sat", "4.6.0")
)
}
/**
* The server splits `vers` into name and version, so a space inside it shifts every field
* after that point. The shipped value was "Look4Sat 4.5.4", which parsed correctly only by
* luck - and only because nothing followed it.
*/
/** Whitespace inside either token would shift every field after it, so it is collapsed. */
@Test
fun `a space in the version cannot shift the following fields`() {
val line = AprsLogin.line("BG7NTA", "5", 12345, "Look4Sat 4.6.0")
assertEquals("user BG7NTA-5 pass 12345 vers Look4Sat-4.6.0", line)
// Six tokens: user, callsign, pass, passcode, vers, version. A space inside the version
// would make seven and push the server's field parsing one place along.
assertEquals(6, line.split(" ").size)
fun `whitespace within a token is collapsed rather than passed through`() {
val line = AprsLogin.line("BG7NTA", "5", 12345, "Look 4 Sat", "4.6.0 beta")
assertEquals("user BG7NTA-5 pass 12345 vers Look-4-Sat 4.6.0-beta", line)
assertEquals(7, line.split(" ").size)
}
@Test
fun `appends a filter when one is given`() {
val line = AprsLogin.line("BG7NTA", "5", 12345, "Look4Sat", "r/33.25/-96.5/50")
assertEquals("user BG7NTA-5 pass 12345 vers Look4Sat r/33.25/-96.5/50", line)
val line = AprsLogin.line("BG7NTA", "5", 12345, "Look4Sat", "4.6.0", "r/33.25/-96.5/50")
assertEquals("user BG7NTA-5 pass 12345 vers Look4Sat 4.6.0 r/33.25/-96.5/50", line)
}
@Test
fun `receive-only logins use the documented passcode`() {
val line = AprsLogin.line("BG7NTA", "", AprsLogin.RECEIVE_ONLY_PASSCODE, "Look4Sat")
assertEquals("user BG7NTA pass -1 vers Look4Sat", line)
val line = AprsLogin.line("BG7NTA", "", AprsLogin.RECEIVE_ONLY_PASSCODE, "Look4Sat", "4.6.0")
assertEquals("user BG7NTA pass -1 vers Look4Sat 4.6.0", line)
}
/** Empty tokens would produce a malformed line, so each has a fallback. */
@Test
fun `empty name or version does not produce a malformed line`() {
val line = AprsLogin.line("BG7NTA", "", 12345, "", "")
assertEquals("user BG7NTA pass 12345 vers Look4Sat 0", line)
assertEquals(7, line.split(" ").size)
}
/**
* The refusal that used to be invisible. It is a comment but not a logresp, so it was skipped
* as chatter; the login then timed out into Unknown, which is treated as "may be working", and
* every send afterwards reported success against a server that had refused the login.
*/
@Test
fun `an invalid login comment is a refusal, not chatter`() {
val outcome = AprsLogin.parse(
"# Invalid login: software name and version are not separated by a space"
)
assertTrue("must be a refusal, got $outcome", outcome is AprsLogin.Outcome.Rejected)
assertEquals(
"Invalid login: software name and version are not separated by a space",
(outcome as AprsLogin.Outcome.Rejected).detail
)
}
@Test
fun `a login denied comment is also a refusal`() {
assertTrue(AprsLogin.parse("# Login denied") is AprsLogin.Outcome.Rejected)
}
/**
* "unverified" contains "verified", so the negative has to be tested first. A naive
* contains("verified") reports every rejected login as accepted - which is precisely the
* failure this replaces.
* contains("verified") reports every rejected login as accepted.
*/
@Test
fun `unverified is not mistaken for verified`() {
@@ -73,12 +117,12 @@ class AprsLoginTest {
}
/**
* Server identification and keepalive comments must return null so the caller keeps reading.
* Treating the first comment as an answer would leave every login Unknown.
* Identification and keepalives must return null so the caller keeps reading. The greeting here
* is verbatim from aprsc 2.1.21.
*/
@Test
fun `comments without a verdict are skipped`() {
assertNull(AprsLogin.parse("# aprsc 2.1.19-g730c5c0"))
assertNull(AprsLogin.parse("# aprsc 2.1.21-gbfc2090"))
assertNull(AprsLogin.parse("# Tue Aug 25 08:00:00 UTC 2026"))
assertNull(AprsLogin.parse(""))
assertNull(AprsLogin.parse(" "))
@@ -95,8 +139,10 @@ class AprsLoginTest {
/** A logresp with no recognisable verdict is Unknown, not silently Verified. */
@Test
fun `an unrecognised logresp is not treated as success`() {
val outcome = AprsLogin.parse("# logresp BG7NTA something-new, server X")
assertTrue(outcome is AprsLogin.Outcome.Unknown)
assertTrue(
AprsLogin.parse("# logresp BG7NTA something-new, server X")
is AprsLogin.Outcome.Unknown
)
}
@Test
@@ -109,6 +155,7 @@ class AprsLoginTest {
AprsLogin.Outcome.Verified("BG7NTA"),
AprsLogin.parse("# LogResp BG7NTA Verified, server X")
)
assertTrue(AprsLogin.parse("# INVALID LOGIN: nope") is AprsLogin.Outcome.Rejected)
}
/** The callsign is read back so the UI can show whose login the server acknowledged. */
core/domain/src/test/java/com/rtbishop/look4sat/core/domain/aprs/ForegroundServiceTypeTest.kt
+140
View File
@@ -0,0 +1,140 @@
package com.rtbishop.look4sat.core.domain.aprs
import java.io.File
import org.junit.Assert.assertEquals
import org.junit.Assert.assertTrue
import org.junit.Assume.assumeTrue
import org.junit.Test
/**
* Guards the one defect no other test in this project could catch.
*
* The APRS service passes a foreground service type to startForeground, and AOSP requires it to be
* a subset of the type declared in the manifest, throwing IllegalArgumentException otherwise -
* which the service's own catch turns into a silent stopSelf(). A commit changed the manifest from
* dataSync to location and left the code passing dataSync, so APRS started, died and reported
* nothing on every device running Android 10 or later, while the settings switch stayed on. Two
* auditors found it by reading constants against AOSP; the unit suite could not see it at all,
* because the service is untestable on the JVM and the app module has no test source set.
*
* So this reads both files as text from core:domain, which does have test infrastructure. Crude,
* and it says nothing about whether the service works - but it fails the moment those two files
* disagree, which is exactly the failure that shipped.
*/
class ForegroundServiceTypeTest {
/** Manifest attribute value to the ServiceInfo constant the code must pass. */
private val expectedConstant = mapOf(
"dataSync" to "FOREGROUND_SERVICE_TYPE_DATA_SYNC",
"location" to "FOREGROUND_SERVICE_TYPE_LOCATION",
"connectedDevice" to "FOREGROUND_SERVICE_TYPE_CONNECTED_DEVICE",
"mediaPlayback" to "FOREGROUND_SERVICE_TYPE_MEDIA_PLAYBACK",
"shortService" to "FOREGROUND_SERVICE_TYPE_SHORT_SERVICE"
)
/** Manifest attribute value to the permission that must accompany it. */
private val requiredPermission = mapOf(
"dataSync" to "FOREGROUND_SERVICE_DATA_SYNC",
"location" to "FOREGROUND_SERVICE_LOCATION",
"connectedDevice" to "FOREGROUND_SERVICE_CONNECTED_DEVICE",
"mediaPlayback" to "FOREGROUND_SERVICE_MEDIA_PLAYBACK"
)
/**
* Walk up from the working directory to the repository root.
*
* A unit test's working directory is the module directory, but that is not guaranteed across
* Gradle versions, so the root is found by looking for settings.gradle.kts rather than assumed.
*/
private val repoRoot: File? by lazy {
var dir: File? = File("").absoluteFile
while (dir != null && !File(dir, "settings.gradle.kts").isFile) dir = dir.parentFile
dir
}
private fun read(relative: String): String? =
repoRoot?.let { File(it, relative) }?.takeIf { it.isFile }?.readText()
private val manifest: String? by lazy { read("app/src/main/AndroidManifest.xml") }
private val service: String? by lazy {
read("app/src/main/java/com/rtbishop/look4sat/AprsForegroundService.kt")
}
/** The type declared for the APRS service in the manifest. */
private fun declaredType(text: String): String {
val match = Regex("""android:foregroundServiceType="([^"]+)"""").find(text)
assertTrue("no foregroundServiceType found in the manifest", match != null)
return match!!.groupValues[1]
}
@Test
fun `the service passes the type its manifest declares`() {
val manifestText = manifest
val serviceText = service
// Skipped rather than failed if the layout moves: a broken locator must not read as a
// broken app, and the assertion below is worthless without both files anyway.
assumeTrue("manifest or service source not found", manifestText != null && serviceText != null)
val declared = declaredType(manifestText!!)
val constant = expectedConstant[declared]
assertTrue(
"unrecognised foregroundServiceType '$declared' - add it to this test's map",
constant != null
)
assertTrue(
"manifest declares $declared, so startForeground must pass ServiceInfo.$constant",
serviceText!!.contains("ServiceInfo.$constant")
)
}
/** Passing any type the manifest does not declare is what AOSP rejects. */
@Test
fun `the service passes no other foreground service type`() {
val manifestText = manifest
val serviceText = service
assumeTrue("manifest or service source not found", manifestText != null && serviceText != null)
val passed = Regex("""ServiceInfo\.(FOREGROUND_SERVICE_TYPE_\w+)""")
.findAll(serviceText!!)
.map { it.groupValues[1] }
.toSet()
assertEquals(
"exactly one type may be passed, and it must match the manifest",
setOf(expectedConstant.getValue(declaredType(manifestText!!))),
passed
)
}
/** The matching permission must be declared, or startForeground throws on API 34 and up. */
@Test
fun `the manifest declares the permission its service type requires`() {
val manifestText = manifest
assumeTrue("manifest not found", manifestText != null)
val declared = declaredType(manifestText!!)
val permission = requiredPermission[declared]
assertTrue("no permission mapped for '$declared'", permission != null)
assertTrue(
"foregroundServiceType $declared requires android.permission.$permission",
manifestText.contains("android.permission.$permission")
)
}
/**
* A location-typed service additionally demands an already-granted location permission before
* startForeground. The settings card requests only notifications, so declaring location would
* fail silently for any operator who declined location access - the same class of defect this
* whole test exists to prevent.
*/
@Test
fun `a location typed service would need a runtime permission this app does not request`() {
val manifestText = manifest
assumeTrue("manifest not found", manifestText != null)
assumeTrue("only applies to a location-typed service", declaredType(manifestText!!) == "location")
val card = read(
"feature/settings/src/main/java/com/rtbishop/look4sat/feature/settings/AprsCard.kt"
)
assumeTrue("settings card source not found", card != null)
assertTrue(
"declaring location requires requesting ACCESS_COARSE_LOCATION or ACCESS_FINE_LOCATION",
card!!.contains("ACCESS_COARSE_LOCATION") || card.contains("ACCESS_FINE_LOCATION")
)
}
}