Fix the issues from the Codex audit - #47
Merged
Merged
Conversation
TunnelProxyHandler had a setPacPolicy that HttpProxy dutifully called and nothing ever read, so its client only knew the static --proxy. CONNECT and WebSocket upgrades did consult the file, which made routing depend on the scheme: the same tunnel sent https:// where the PAC file said and http:// somewhere else entirely. jetty-client picks an upstream by walking ProxyConfiguration for the first Proxy whose matches(Origin) is true -- a fixed address per entry, which a PAC file is not. So each proxy the file names gets one entry that re-asks the PAC and claims only the origins routed to itself; a DIRECT host is claimed by none and dialled directly. Registration happens on the request thread before the exchange, never from inside matches(), because match() is iterating that list at the time. The chosen proxy is also the request tag, so it forms part of the destination's identity -- jetty-client resolves the proxy once per Origin and caches the destination, so otherwise a PAC answer that changed would keep using the previous one. --proxy is no longer registered when a PAC file is loaded; it would match every origin and be picked ahead of the PAC entries, which is the same "file ignored" outcome by another route. That exposed a credential leak. With --proxy-userpwd and --pac-local both set, --proxy is no longer the recipient, but Proxy-Authorization was still stamped on plain-HTTP requests -- so the password would go to a PAC-chosen proxy, or for a DIRECT answer straight to the origin and into its access log. upstreamAuthorization() now withholds it under PAC, which is the rule the CONNECT path already documented. Separately, --connect-to describes where a named destination lives. With an upstream proxy the socket goes to the proxy and the destination travels in the request line, so there is nothing at dial time to remap -- but it was applied to the proxy endpoint anyway, in all three egress paths. A wildcard rule therefore diverted the proxy connection and left the destination alone. In WebsocketHandler the comment above the call already said --connect-to was not applied to the proxy while the next line applied it. The PAC coverage was interpreter unit tests plus an e2e scenario that only ran --pac-test, which evaluates the file without proxying anything, so "what does this file say" was well covered while "where does traffic go" went unasked. ConnectToMapTest covers the mapping and the dial tests covered direct connections only, so neither could see the proxy interaction.
Three paths let a tunnel report itself healthy, or stay alive indefinitely, when it was not working. Readiness. tunnelReady() captured startProxies()'s result and then called setTunnelUp(true) regardless, using the result only to pick a log message; the ready file was written from inside startProxies() on every path. So a tunnel whose Selenium relay could not reach the hub, whose reverse forward could not reach the local proxy, and whose proxy could not reach the internet still answered 200 on /readyz and still touched --readyfile. Both now follow the self-test. This is a behaviour change: a half-working tunnel that used to report ready now reports 503 until a reconnect succeeds, which will surface as newly-failing container health checks where the fault was previously invisible. Polling. TunnelPoller cancelled its scheduler on the first exception, so one dropped connection while the tunnel server was still booting ended the tunnel permanently -- and told nothing, so the process stayed up with a metrics server answering and a tunnel that would never be ready. A failed poll is now retried; only MAX_CONSECUTIVE_ERRORS in a row gives up, and a success resets the count. A TunnelFailedException stays terminal, since the failure is in our own setup rather than in the poll. Terminal state. App.setupFailed is what the poller and tunnelReady's catch now reach. Both run on timer threads with nobody to throw to, so the failure was logged and dropped. It releases everything and exits with a status for the command line client; an embedder's JVM is not ours to exit, so there it leaves a stopped App whose /readyz says what happened. Reconnect. When SSH reconnects but the local proxy cannot rebind, only the proxy is retried. That branch existed but called scheduleRetry(), whose task begins tunnel.stop(); tunnel.connect() -- so the code written to avoid re-dialling a healthy session did exactly that every five seconds, for as long as the port stayed bound, and the comment above it said the opposite. It is also bounded now: returning through onReconnected() skipped attemptReconnect()'s limit check, so a port that never freed up retried forever instead of falling through to the rebuild that would have released it. TunnelPollerTest.aFailureWhilePollingStopsTheSchedule asserted the old behaviour as if it were correct and is replaced. The monitor's existing proxy test fired once and stopped, which is why it never saw the second scheduled attempt re-dial SSH. HealthEndpointsTest sets the gauge by hand, so it could not see the readiness gap; ReadinessGatingTest drives the real startup path.
…tion native and central had no needs: at all and both build with -DskipTests, so a tagged commit with failing tests published the self-contained distributions and a Maven Central artifact. Only docker was gated, and only incidentally, by depending on build. All four now depend on a single test job. The channels stay independent of each other, which is deliberate -- for 4.9 the Central publish queue outlasted the plugin's poll window, the job "failed", and the release and docker steps were skipped while Central published anyway -- but independence from the tests was never intended. docker moves from needs: build to needs: test: the point was not to wait for the release to be created, only not to publish from a failing build, and naming the gate directly lets the image build alongside the release. The gate runs verify rather than test, so the shaded-jar smoke test and JaCoCo run before anything is uploaded. test.yml builds with verify too, so a pull request catches the same failures rather than leaving them for the tag. macos-13 was retired by GitHub on 2025-12-04; macos-15-intel is the Intel runner that replaced it. jlink emits a runtime for the machine it runs on, so that label is what makes the x64 macOS build x64. verify-runtime.sh resolved only bin/testingbot-tunnel and required it to be executable, while build-runtime.sh writes bin/testingbot-tunnel.cmd on Windows. The Windows leg therefore failed at that line, before verifying anything, on every tagged build. It now resolves per platform, and since env -i cannot be used there -- the .cmd needs the command processor, located through the environment being cleared -- that branch strips JAVA_HOME and any JDK or JRE entry from PATH instead, which is what the isolation is actually for. The isolation also dropped TESTINGBOT_KEY and TESTINGBOT_SECRET, so the guard admitted a run because they were set and then launched the tunnel without them: only a ~/.testingbot file ever exercised the live checks. They are carried through now. Kept as an array rather than only a function because the live launch must invoke it directly -- backgrounding a shell function makes $! the wrapping subshell, and killing that orphans the JVM, which then holds a slot against the account's concurrent-tunnel limit.
Api._post parsed the response body without ever looking at the status, so a 500
carrying {"message":"failure"} was handed back as tunnel data. boot() then read
state, id and ip off it, and an API outage surfaced as a missing-field error
somewhere further along -- or as a tunnel that never came up, for no stated
reason. _get has always checked. The body is read on the failure path too and
included in the message: the API says why it refused, and that reason is the
whole value of the message to whoever has to act on it.
ApiTest already had createTunnel_withServerError_shouldThrowException, and it
passed -- because the stubbed body was not JSON, so parsing failed. Its own
comment said so. It looked like coverage of "the API returned 500" and was
really coverage of "the API returned something unparseable", which is why the
case that mattered went unnoticed.
App.VERSION was a Float, which cannot represent this project's version scheme:
5.10 parses to 5.1f and sorts below 5.9, so the upgrade notice would have
stopped appearing at exactly the release it was needed for;
5.0.1 is not a float at all -- Float.parseFloat threw, the exception was
caught, and the version became 0.0, so a patch release made the client
believe it was older than everything and nag on every startup;
and float equality was never something to rely on for two versions that
should match.
Version parses a dotted release and compares component by component, with a
missing component read as zero and a pre-release suffix sorting below the
release it leads to. Something unreadable is null rather than 0.0, which is the
distinction the old code could not make: 0.0 does not mean "unknown", it means
"older than every release". Both sides must parse before anyone is told to
upgrade.
App.VERSION is now the String it was always used as -- every use in this
repository is display -- so this is a source-compatibility break for an
embedder that assigned it to a Float. Keeping a deprecated Float alias was the
alternative and was rejected: it would have gone on returning 5.1 for 5.10,
which is the bug rather than a compatibility shim for it. TunnelMetrics
.setTunnelInfo takes the version as a String for the same reason, so the
tunnel_info label reports the version as shipped.
WebsocketHandler asked whether the target's first response line
contains("101"). That accepts "HTTP/1.1 2101", and it accepts any status at all
whose reason phrase happens to carry those digits -- "HTTP/1.1 500 Internal
Error 101 in upstream handler" is an ordinary way for a server to describe a
failure. Having accepted it, the relay answered the client 101 Switching
Protocols and spliced the two sockets together, so a client held what it
believed was a WebSocket to a server that had refused the upgrade.
The CONNECT path already parsed the line properly, and ConnectFramingTest has
covered exactly this case for it since it was written
(aStatusLineMentioning200InItsReasonIsStillARejection). The two relays are the
only places here that read a response off the wire without Jetty's parser, and
they disagreed about how. HttpStatusLine is now the one parser both use, so
they cannot drift again: it requires the HTTP/ prefix and exactly three digits,
which Integer.parseInt alone would not -- that would take "2101" and "+200".
Tested twice on purpose. HttpStatusLineTest covers the parser, and
WebsocketUpgradeStatusTest drives a real upgrade through the real handler
against a target answering a chosen status line, because the bug was never in a
helper -- it was in what the handler did with the answer. Both refusal cases
fail against the old contains() check; the genuine 101 and the plain 200 pass
either way and are there so a fix that simply stopped relaying upgrades would
not look correct.
Four problems, one of which decides where customer traffic goes. A PAC file that threw at runtime resolved to DIRECT. That is not a neutral default: on a network whose only sanctioned egress is a proxy it takes traffic the operator routed deliberately and sends it straight out -- past a configured --proxy as well, which no reading of "fallback" covers. The component whose entire job is deciding where traffic goes was failing open, and the single word in the log was "direct". PacPolicy.resolveOrNull now distinguishes "could not decide" from "decided: direct", and all three egress paths answer the first with --proxy. Direct remains the answer only when nothing else was configured, because then there is genuinely nowhere else to send it. Failures are not cached: a transient dnsResolve timeout would otherwise hold the fallback route in place for the full cache TTL after the condition had passed. That interacts with the credential rule, so it is now stated properly rather than as "PAC means withhold". --proxy-userpwd is sent when the chosen upstream is the proxy it names -- which covers the file routing there and this fallback -- and withheld otherwise, including for a DIRECT answer, where it would reach the origin. The fetch used a bare HttpURLConnection and honoured neither --proxy nor --cacert-file. On a proxy-only network the PAC URL was unreachable; on a TLS-intercepting network -- the exact case --cacert-file exists for -- an https URL failed the handshake against a CA the JVM has never seen. Both left the tunnel refusing to start over a document it had been told how to reach. The 1 MiB cap applied only to remote documents. readAllBytes() had none, so how much this process would read into memory depended on where the bytes came from. Failover is still not implemented, and this does not pretend otherwise: only the first directive is used, and honouring the list would need a retry loop through three separate transports -- jetty-client's exchange, the hand-rolled CONNECT relay and the WebSocket relay. A file returning more than one directive now logs which is used and that the rest are not, so an operator learns it from a startup line rather than during an outage of the first. Tested at both levels again: PacFailureFallbackTest for what resolution means, PacHttpRoutingTest for what the handler then does with it, and PacFetchRoutingTest for the fetch and for App passing the options through at all. The fallback tests fail against the old fail-open behaviour; the digest in the fetch test is computed independently rather than through PacPolicy's own helper, so the pin checks the fetched bytes instead of agreeing with itself.
…s route Four independent defects, each one where a check reported something other than what it claimed to check. localForwardingActive() matched the port as a substring of the whole entry, so 445 matched "4456:h:80" -- and so did digits in the destination host or the remote port. A previous test documented this as harmless. It is not: this answers "is my forward still there", and the monitor repairs the forward when the answer is no. A false positive means a forward that is gone is reported as present, the repair never runs, and every request through the tunnel keeps failing while the log insists forwarding is fine. Now the local port is parsed and compared, handling the bind-address form JSch also emits. readinessPort() validated the syntax but not the range, so --ready --metrics-port 99999 got through and died in ReadinessProbe with an uncaught IllegalArgumentException and a stack trace -- from the one command whose entire contract is to exit 0 or 1 for a container probe to read. It now uses port(), like every other port option, and ReadinessProbe catches the case anyway since it is public and its job is to return a code rather than throw. The ready file and the pid file were deleted only by their shutdown hooks, and cancel()/stop() removed the hook without deleting the file. On JVM exit that was fine; on an explicit stop it left a ready file claiming a tunnel that no longer exists is forwarding -- for an embedder running a tunnel per job, and across the reconnect monitor's stop()/boot() rebuild, which is exactly the window where nothing is. Doctor built its own HTTP client rather than the one Api uses, and it had drifted: no SOCKS5 support, and no credentials for an authenticated proxy. On those networks --doctor tested a route the tunnel does not take, reporting "can not be reached" for a tunnel that would have started, or reaching the API by a path the tunnel would not have used and calling that a pass -- wrong about exactly the setups it exists to diagnose. There is now one builder. The SOCKS5 test proves the dial rather than asserting the client is non-null: SOCKS points at a dead port while the target is directly reachable, so a client that ignored the proxy would succeed.
The option reformatted java.util.logging and stopped there. This process logs through two stacks -- JUL for its own classes, SLF4J/logback for Jetty, Apache HC and the proxy handlers -- and logback.xml pins its console appender to a text pattern. So the stream was JSON for some records and text for others, which is not a format at all: for the collector this option exists to serve it is worse than plain text, because it parses most of the way and then fails. The file appender had already been dealt with; the console is where almost every record actually goes. The encoder is swapped in place on the configured appender rather than a second appender being added, which would have emitted every record twice -- once as JSON and once as text, the original defect plus duplication. Three things that were not log records were also on the stream. The startup banner is four lines of ASCII art on stdout, and it is the first thing a collector meets; under json it becomes one INFO record instead. "Shutting down your personal Tunnel Server" was System.out.println in two places and is an operational line, so it goes through the logger in both formats. The fatal startup error was System.err.println, and JUL's console handler writes to stderr too -- so a multi-line message landed in the middle of the JSON stream, several parse failures at once; under json it is logged instead. setupFailed printed the reason it had already logged, which duplicated it under text as well. activeLogFormat is static because the fatal path runs from a catch enclosing argument parsing, so the CommandLine may not exist by then -- but the formatter is installed and the stream already has a shape to respect. Verified end to end: every line of `--log-format json` output now parses as JSON, and text mode is byte-for-byte what it was.
DoctorScopeTest made four real internet requests per test. Doctor's constructor runs the checks, so tests asserting something about port selection or Kerberos scope reached testingbot.com and google.com to do it -- slow, and failing in a sandbox or offline for reasons unrelated to what they assert. The endpoints are injectable now and those tests pass none; the connectivity path is covered by DoctorEgressTest against a local server instead. The class went from 1.9s to 0.15s. HealthEndpointsTest constructed an InsightServer per test and dropped it, so each test left a Jetty server and a bound port behind. Surefire forks per class here, which is the only reason it did not accumulate across the suite. LocalWebServer kept its Server in a constructor-local, so nothing could stop it and it had no tests at all -- 0% coverage, and not fixable without this change. It served an operator-chosen directory, with listing enabled, for the life of the JVM: outliving the tunnel it accompanied, leaking a server per App for an embedder, and holding port 8080 so the next run could not bind one. It is now held on the App and stopped in stop(), and the port is injectable so the tests need not assume 8080 is free. Mockito is loaded as an explicit agent. It warned that dynamic self-attachment will stop working in a future JDK, and when that happens every test using the inline mock maker fails at once, on a JDK upgrade, with an error pointing at the JVM rather than at the build. This needs maven-dependency-plugin's properties goal to define the path -- without it the property is not expanded and the JVM fails to start.
…pt drift
The published primary artifact is the thin jar, so a consumer's classpath is
built from this pom's declarations. Six artifacts this code imports directly
were not declared and arrived transitively: httpcore5, jackson-core, and four
Jetty modules. A transitive provider that dropped or renamed one would break
that consumer with a NoClassDefFoundError naming nothing in this pom. Versions
match what already resolved, so the build is unchanged. The remaining warnings
are test-scope aggregators (junit-jupiter, wiremock-jetty12) and do not reach
the artifact.
JaCoCo produced a report that nothing read, so coverage could fall to nothing
without the build noticing. The limits are a floor rather than a target, a
couple of points under what the suite achieves now: ordinary movement passes,
a real drop fails. Verified by raising one and watching it fail.
The two Grafana dashboards are one document in two places, and they had drifted:
the docker-compose copy was five panels behind, missing everything added for
connection, dial and proxy-error observability. Someone running the compose
example to look at those metrics would have found a dashboard that did not show
them, with nothing to say it was stale. Synced, with a test comparing them by
panel title so the failure names what is missing.
The build scripts selected a shaded jar with `ls | head -1`, which takes the
alphabetically first: after a version bump without `mvn clean` that is the
*older* artifact, since 5.10 sorts before 5.9. They now take the newest and say
so when there is more than one.
MalformedQueryStringTest asserted doesNotContain("502") against the whole
response. Jetty's error page echoes the request URI, which carries the proxy's
randomly chosen port, so the test failed whenever that port contained those
digits -- observed here on port 50281. It now reads the status line. This is
the same substring-on-a-variable-body mistake HttpStatusLine was introduced to
remove from the WebSocket relay.
maven-gpg-plugin binds `sign` to the verify phase, for the Maven Central deploy. Moving the CI build and the new release gate from `package` to `verify` therefore asked both to sign artifacts on runners that have no key, so both would have failed on a missing key rather than on anything they exist to check. Signing stays where it belongs: the `central` job runs `deploy` and has the key.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Comment on lines
+72
to
+73
| BufferedReader in = new BufferedReader(new InputStreamReader( | ||
| socket.getInputStream(), StandardCharsets.UTF_8)); |
Comment on lines
+125
to
+126
| BufferedReader in = new BufferedReader(new InputStreamReader( | ||
| client.getInputStream(), StandardCharsets.UTF_8)); |
| return -1; | ||
| } | ||
| // fields[0] is either the local port or a bind address. | ||
| for (int i = 0; i < 2 && i < fields.length; i++) { |
| encoder.setContext(context()); | ||
| encoder.start(); | ||
|
|
||
| ch.qos.logback.classic.Logger logger = context().getLogger("test.logger"); |
| } | ||
| File f = new File(this.readyFile); | ||
| if (f.exists()) { | ||
| f.setLastModified(System.currentTimeMillis()); |
| return INVALID; | ||
| } | ||
| } | ||
| return Integer.parseInt(code); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses the findings from the Codex audit, in dependency order. Every fix was
verified against the unfixed code first: each new test was run with the change
reverted to confirm it fails, so the tests assert behaviour rather than intent.
Routing
--pac-localwas wired intoTunnelProxyHandlerand never read, so plain HTTPignored the PAC file while CONNECT and WebSocket honoured it. Routing depended on
the scheme.
--connect-towas applied to the upstream proxy endpoint instead of thedestination, so a wildcard rule diverted the proxy connection and left the
destination alone.
--proxy-userpwdand--pac-localboth set,
Proxy-Authorizationwas still stamped on requests now going elsewhere,including straight to origins on a DIRECT answer.
Failure semantics
/readyzand--readyfilereported ready regardless of the startup self-test.Behaviour change: a half-working tunnel now reports 503.
alive; setup failures had no terminal state.
session every five seconds, and skipped its own retry limit.
Release integrity
nativeandcentralhad noneeds:and built with-DskipTests, so a taggedcommit with failing tests published artifacts and a Maven Central release.
macos-13was retired by GitHub on 2025-12-04.verify-runtime.shfailed before verifying anything.Correctness
Api._postparsed responses without checking status.App.VERSIONwas aFloat:5.10sorted below5.9, and5.0.1became0.0.Source-compatibility break — it is now a
String.--proxy— therouting component failing open. PAC fetches now honour
--proxyand--cacert-file.Hygiene
present and was never repaired.
--log-format jsonemitted a mixed JSON/text stream.stop();--doctortested a route thetunnel does not take.
matters because the published artifact is thin.
Tests: 883 → 954, all passing. Verified on JDK 17 and 23 locally.