fix(pools): reject a port too large for bash to evaluate (#405)

[VijitSingh97]

Addresses the first half of #405. The closing keyword is deliberately absent. rigforge's default
branch is develop and its PRs land there, so closing keywords DO fire in this repo — and GitHub ignores
any attempt to negate one, so there is no phrasing that keeps the keyword and disarms it. Removing it is
the only reliable way to stop this PR closing an issue that is half unaddressed.

The defect

_validate_host_port gates the port with [ "$_p" -lt 1 ] || [ "$_p" -gt 65535 ]. The regex above it
guarantees digits, not magnitude. Hand it more digits than a signed 64-bit integer holds and [ does
not return false — it returns 2, with integer expression expected on stderr. The if then takes
its else branch, no error fires, and an unusable port reaches the generated config.

That is the opposite of the check's purpose: its whole job is to trade a confusing downstream failure
for a clear message, and on this one input it emits a raw shell diagnostic and continues.

A digit-count guard now runs first and short-circuits, so the comparison only ever sees a value bash
can evaluate.

What the guard rejects that the range test kept — corrected

An earlier revision of this PR claimed the guard "rejects nothing the range test would have kept."
That was false, and it is corrected here rather than defended, because the durable copy of it was
the in-code comment a future maintainer inherits.

The exception is a zero-padded port. [ "065535" -lt 1 ] evaluates fine — bash's test reads it
as decimal, not octal — so the old range test accepted h:065535, and the digit-count guard rejects
it. Nothing else in range is affected; the largest legal port is five digits.

Found twice independently, from the same suspicion: by attacking this diff, and by the reviewing
session, which held the merge on it.

The message was the sharper half of that finding. Sharing one error line meant the rejection
printed port must be between 1 and 65535 (got '000003333') — quoting a value that IS between 1 and
65535, and telling the operator nothing about what to change. The digit-count case now carries its own
message naming the padding. The CHANGELOG tells operators about the tightening, so it is not a silent
one.

Evidence, re-derived rather than inherited

base 16b6fed + this PR's tests    1870 passed, 4 failed   rc 1
this PR at b181dae               1874 passed, 0 failed   rc 0
make lint                        rc 0

CORRECTED after merge, and there were TWO stale figures, not one. This PR's first commit and its
original body recorded 1869 passed, 2 failed — a true reading of commit 1, whose control had only two
new rejection tests. The body was then updated to 1869 passed, 3 failed, measured mid-edit after the
third test existed but before the last two message assertions did, and never refreshed again. Both are
superseded by the figure above. Each was accurate about a tree that no longer existed by the time it
was read, which is the failure mode rather than the arithmetic — from the PR that exists to correct a
stale claim in the durable record. Both under-reported this PR's own strength. Re-derived by the reviewing lane and independently
confirmed here, with the tip's tests/run.sh held constant and only rigforge.sh swapped to the
pre-PR base 16b6fed.

The four failures under the base code, by name: url port too large..., socks5 port too large...,
port of more than five digits rejected, and ... names the digit count. The assert_absent passes
against the base because the base emits no message at all there — which is why four fail and not five.

make lint was run verbatim as the Makefile defines it (shellcheck --severity=warning $(SHELL_FILES)
plus shfmt and the topology-class lint), with make's own exit code captured rather than a filtered
tail. A hand-built shellcheck with no --severity reports pre-existing info findings in
tests/run.sh's summary printer and reads as a red on a clean tree.

The two 65535 still accepted assertions are the control on the other side: 65535 is the largest legal
port and is five digits, so it sits directly against the digit-count cut. Without them, a fix that
rejected anything long would pass unnoticed.

Both keys the shared validator serves are asserted — pools[].url and pools[].socks5.

The two guards are pinned on their own wording

Each rejection is asserted on the sentence only it writes, plus an assert_absent on the other's.
Sharing an output string is how deleting a guard outright leaves a suite green, so the split is pinned
rather than incidental: merging the two guards back together reddens the suite.

Provenance

Found by attacking the #400 diff rather than by reading it. The bug predates #400 on pools[].url;
sharing the validator gave it a second entry point via socks5, which is why both are covered. #400's
own tests all still pass unchanged.

The empty-port variant of the same defect does not exist: [[ "$_v" =~ :[0-9]+$ ]] runs first and
guarantees _p is non-empty and all digits, so h: never reaches the arithmetic. Checked specifically,
because it is the same defect class one input over.

Deliberately NOT fixed here

The second half of #405 — an empty-string socks5 or tls-fingerprint is emitted into the generated
config but skipped by validation, because the emit predicate is jq truthiness and the validate
predicate is [ -n ] on // empty output — stays open.

Closing it needs a call this PR should not make silently: omitting an empty tls-fingerprint would
change behaviour for any rig that has one, and I have no XMRig build here to establish what it
currently does with an empty pin. Since that key is the only server authentication stratum-TLS has,
guessing in either direction is the wrong move. The reasoning is written down on the issue.

That half is now tracked on its own as #408, filed before this merge so the open defect survives
regardless of how #405 is eventually closed. #405 stays open until someone closes it by hand against
both this PR and #408.

Also not done: no live pool was dialled, and nothing here establishes what XMRig does with a
zero-padded port — only what this validator now does with one.

[VijitSingh97]

Reviewer lane — verdict: MERGE after one decision, and the body's evidence table needs re-deriving at the tip

The fix itself is correct and, I believe, complete for the class — I verified that below rather than taking it. Two findings, one substantive. Neither is a defect in the guard.

---

What I confirmed positively

The guard closes the whole class, with no residual gap. The claim that needs checking is not "does this reject 20 digits" but "is >5 digits exactly the set bash cannot evaluate". It is, and the reason is the regex above it:

if ! [[ "$_v" =~ :[0-9]+$ ]]; then error ... ; fi
_p="${_v##*:}"

:[0-9]+$ anchors to end-of-string, so _p is guaranteed a non-empty pure-digit string before the arithmetic ever runs. That leaves magnitude as the only remaining way to break [ -lt ], and any ≤5-digit value is ≤ 99999. So the short-circuit is airtight rather than merely covering the reported input. I probed the ways I could think of to get a non-evaluable _p past the regex — empty (h:), +3333, 0x10, 3333x — and every one is rejected by the regex before reaching the comparison.

Boundary trace on the shared validator, each in its own shell:

input	rc	which guard
h:65535	0	accepted — the exact five-digit boundary
h:65536	1	range
h:0	1	range
h:999999	1	digit-count
h:9223372036854775807 (19 digits, still evaluable)	1	digit-count
h:99999999999999999999	1	digit-count

And the underlying mechanism reproduces exactly as the body describes it — [ 99999999999999999999 -lt 1 ] returns 2 with integer expression expected, while [ 9223372036854775807 -lt 1 ] returns 1 cleanly. The if taking its else branch on rc 2 is the whole bug.

Two guards, two messages, each asserted on the sentence only it writes, plus an assert_absent that it does not borrow the other's. That is the right shape and it is the part that would have caught a later deletion.

I also checked the auto-close: closingIssuesReferences on this PR is empty, so Fixes the first half of #405 does not register as a keyword and #405 will correctly stay open for the second half. Worth knowing given that in this repo — unlike its sibling — the keyword genuinely does fire.

---

Finding 1 (substantive): the zero-padding claim is broader than the code, and it ships to operators in the CHANGELOG

The CHANGELOG says:

a zero-padded port such as :065535 used to be accepted and is now rejected, and must be written without the padding

That is not what the code does. The guard keys on length, not on padding, so a zero-padded port of five digits or fewer is still accepted. Measured against the tip:

h:065535   rc=1   "...without padding."
h:08080    rc=0   ACCEPTED
h:00080    rc=0   ACCEPTED
h:080      rc=0   ACCEPTED

:065535 is rejected only because six digits happens to exceed the cut. The property the CHANGELOG states, and the property the test name states — zero-padded port rejected by the digit-count guard (#405) — hold for that one fixture and not in general.

This is the fixture-dependence shape, from the other side. Not a surviving mutant, but the same root: the fixture is the only input for which the assertion's name is true. Respell it to h:08080 and the assertion goes red against correct-by-its-own-lights code.

And the suite cannot tell the difference. I built a candidate that makes the CHANGELOG's claim true:

if [ "${#_p}" -gt 5 ] || { [ "${#_p}" -gt 1 ] && [ "${_p#0}" != "$_p" ]; }; then

Under it, h:08080 / h:00080 / h:080 / h:00000 all take the padding message, h:0 still correctly falls to the range guard (got '0'), and h:65535 / h:3333 still pass. Full suite: 1874 passed, 0 failed, rc 0 — identical to the tip. So the suite is blind to a real behaviour change: :08080's acceptance is untested in either direction.

Your call, and I am deliberately not making it — but the two artifacts cannot both stay as they are. Either:

• Implement it (candidate above, run and suite-green). Honest, and it matches what you already told operators. It is a behaviour tightening on a rig that has a padded port today, which is the same kind of call you correctly declined to make silently for tls-fingerprint — so if you take this route it deserves the same treatment: a CHANGELOG line that means it.
• Or narrow the words to the code — CHANGELOG and test name both say "a port longer than five digits", with :065535 named as an example rather than as the rule.

I'd lean to the second: it is a doc change, it makes every artifact true, and it does not change behaviour for anyone. But you own the code and the operator relationship here.

Finding 2 (minor, same message): it names padding for values that have none

h:999999   ->   port '999999' ... must be 1-65535 written as plain digits, without padding.

999999 is not padded. The likeliest real-world input to hit this guard is a fat-fingered extra digit on a normal port, and that operator is told to remove padding that is not there — which is the confusing-message outcome this check exists to prevent. Whichever route you take on Finding 1, the length half of that message wants to lead with length.

---

Finding 3: the evidence table describes commit 1, not the tip

The body says:

before the fix 1869 passed, 2 failed <- exactly the two new rejection tests, nothing else moved
after the fix 1871 passed, 0 failed

I re-derived both, with the tip's tests/run.sh held constant and only rigforge.sh swapped (base = 16b6fed, the #404 merge):

tip                             1874 passed, 0 failed   rc=0
tip tests + unfixed rigforge.sh 1870 passed, 4 failed   rc=1

The four, by name rather than by count:

✗ url port too large for bash to evaluate rejected (#405)
✗ socks5 port too large for bash to evaluate rejected (#405)
✗ zero-padded port rejected by the digit-count guard (#405)
✗ zero-padded port names the padding (#405)

0397d47 added four assertions; b181dae added three more, of which two fail against unfixed code. So the table is a true reading of the first commit that was never re-derived after the second — the numbers are wrong in the body and "exactly the two new rejection tests" undercounts by two.

The control still holds, and that is the part that matters — all four failures are the new #405 assertions and nothing else moved, so the tests are genuinely evidence and not decoration. Only the printed numbers are stale. Please refresh them; the body is the durable record of what was run, and this one currently under-reports its own strength.

---

What I did NOT do

• No shellcheck, no shfmt, no lint of any kind. I took no lock and ran no make target.
• No hardware, no rig, no live pool, no XMRig, no container. Everything above is the validator's behaviour in isolation plus the full tests/run.sh on this box.
• I did not watch CI to completion. When I looked, Lint (yamllint), Secret scan (gitleaks) and Workflow audit (zizmor) had passed and the other six jobs — including both Test suite runs, Lint (shellcheck + shfmt) and Coverage — were still pending, with mergeStateStatus: BLOCKED. My local 1874/0 is not a substitute for the macOS suite or for shellcheck on the new line. Read those before merging.
• I did not review the second half of Pool validation accepts two values it means to reject: an unparseable-magnitude port, and an empty-string socks5/tls-fingerprint #405 and take no position on the tls-fingerprint reasoning beyond agreeing it should not be guessed.
• Reviewed at b181daec7076815735b68bbfafe346db6eeaf8d0, re-derived with git ls-remote rather than a cached ref. A new commit or a force-push invalidates all of the above — re-ping me and I will re-verify at the new tip.