Bug - Fix CLI login error - #231
tonygermano merged 6 commits into
Conversation
20f5769 to
599ec0c
Compare
mgaffigan
left a comment
mgaffigan
left a comment
There was a problem hiding this comment.
Omit the null check or handle at :192. Other comment is just a suggestion.
71cc19c to
8e7a76a
Compare
NicoPiel
left a comment
NicoPiel
left a comment
There was a problem hiding this comment.
Looks good. Mitch already suggested the only change I would have made.
tonygermano
left a comment
tonygermano
left a comment
There was a problem hiding this comment.
I was just testing, and this does resolve the uncaught exception, but the program still hangs after a failed login. We're going to need to dig a little more to see why the program is still not ending.
Looking at the error messages, I think the first one on line 191 is fine, because I can't think of a reason that we'd ever get a UnauthorizedException where the response wasn't a LoginStatus, so printing the stack trace would be useful there.
The second one on line 197 we might want to make a little more user-friendly, since that is the one that shows up for a bad password. I don't know that it's necessary to print the loginStatus, since we already know we have one, and it's not successful at this point. Perhaps a message like, Could not login to server. Please check your username and password and try again. would be more appropriate. This is similar to what was requested in the linked issue.
tonygermano
left a comment
tonygermano
left a comment
There was a problem hiding this comment.
It was hanging because the client wasn't getting closed after the function returned, but I think it's better to return an appropriate exit code than to let it return 0 like it worked successfully since this can be used to run scripts as well as interactively.
There was a problem hiding this comment.
Pull request overview
This PR fixes a CLI login error handling issue by catching UnauthorizedException and extracting the LoginStatus from the exception's response. The fix addresses issue #224 where login failures were not properly handled.
Key Changes:
- Added exception handling around
client.login()to catchUnauthorizedException - Extracted
LoginStatusfrom the exception response using pattern matching - Improved error messaging to include the login status details
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
db45250 to
10e6da1
Compare
|
I'm curious about this patch (I haven't tried it). The root problem is that the What is required in order to shut-down this thread? Do you just need to fetch the status from the error, or is the "Real work" being done by the If |
@ChristopherSchultz See my previous comment #231 (review) |
0cb7d2c to
fc7b90b
Compare
|
I pushed an update that builds on what @miguel-figueiredoPT had already started and squashed it all into a single commit fc7b90b. |
processMessage returns a single message id, taken from the response
handler's selected result. For a batch that is only the first or last
message, leaving a caller no way to learn the ids of the others. The
smoke test harness needs them to assert per-message content.
POST /channels/{channelId}/batchMessagesWithObj returns every id. The
response handler is already threaded through dispatchBatchMessage, so
CollectingResponseHandler only has to record each id as it is set. It
keeps the ids and not the DispatchResults, so a long batch does not
retain the processed messages, maps and content of the messages that
have already finished.
Processing is unchanged. Whether a batch is split is still governed by
the channel's Process Batch setting; on a channel with batch processing
disabled the dispatch takes the single-message path and the endpoint
returns a one-element list.
EngineController.dispatchRawMessage gains a five-argument overload. The
existing four-argument form delegates to it with a SimpleResponseHandler,
so its callers are unaffected, but the interface gains an abstract method
and out-of-tree implementations will need updating.
Signed-off-by: Mitch Gaffigan <mitch.gaffigan@comcast.net>
A batch fixture produces several messages, and the harness could only submit a payload and assert against one of them. Assertion files for a fixture that expects N messages now live in numbered subdirectories, 01 through NN; a fixture that expects one message keeps its flat layout and reads exactly as before. The generator rejects numbering that does not start at 01 or that leaves gaps, and rejects a fixture that mixes loose assertion files with numbered ones. An empty source_rejected file declares that the server must refuse the submission. It cannot be combined with assertion files, since a refused submission produces no message to assert against. source_raw, destNN_raw and destNN_encoded are now assertable. OieServer.submitMessage calls the new batchMessagesWithObj endpoint so the harness learns every id the payload produced, and runMessage fails if the count does not match what the fixture expects. Failure output renders every message rather than one, and now includes the source map. ci/tests/120-delimited-batch covers the new layout with a CSV batch channel, independent of any XML change. Note that routing every submission through batchMessagesWithObj leaves processMessage without end-to-end coverage. Signed-off-by: Mitch Gaffigan <mitch.gaffigan@comcast.net>
Signed-off-by: Mitch Gaffigan <mitch.gaffigan@comcast.net>
CVE-2026-82578 found an XXE vulnerability in XML batch parsing. This closes that vulnerability and adds regression tests. setNamespaceAware(true) is not a behavior change. XPath.evaluate(String, InputSource, QName) built its DOM with a namespace aware DocumentBuilder, so the batch splitter has always been namespace aware. Without it, split messages lose the xmlns declarations they inherit from the batch element. Signed-off-by: Mitch Gaffigan <mitch.gaffigan@comcast.net>
The startup scripts read .vmoptions files with a shell `while read` loop, which skips a last line that has no trailing newline. An option typed as the last line of conf/custom.vmoptions in an editor that does not add a final newline never reached the JVM, so the engine kept the -Xmx256m from base_includes.vmoptions. The install4j-generated scripts (oieservice, oieserver, oiecommand) use that loop and are not ours to change, so the template now ends with a comment line. Options added above it always end in a newline. If an editor leaves the comment without a newline, the scripts drop the comment, which is harmless. Closes OpenIntegrationEngine#464 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Finnegan's Owner <44065187+pacmano1@users.noreply.github.com>
pacmano1
left a comment
pacmano1
left a comment
There was a problem hiding this comment.
Tested the CLI from fc7b90bcf against an engine built from the same commit (Temurin 17, Linux).
| Case | Result |
|---|---|
| 4.6.0 release CLI, wrong password (control) | Stack trace, then hangs until killed |
| Wrong password | One-line error, exit 77, no hang |
Wrong password, -d |
Same, plus the UnauthorizedException stack trace |
| Nothing listening on the port | "Could not communicate with server", exit 69 |
| Missing script file | Connects, "Could not load script file", logs out, exit 74 |
Good login, script running status |
Runs, disconnects, exit 0 |
Good login, status piped into the console |
Runs, disconnects, exit 0 |
Each case was java -jar mirth-cli-launcher.jar -a https://localhost:8443 -u <user> -p <password> -v 0.0.0 -s status.txt under a 30-second timeout, run from that build's own install directory. The launcher loads cli-lib from the current directory, so running one build's launcher from another build's directory tests the wrong CLI.
Related: #439 stops the server from telling a locked account apart from a wrong password. The fixed message here already matches that, and any later change to print the server's reason in the CLI should build on #439.
The CLI only closed its Client on the success path. Client starts non-daemon threads, so a failed login, a connection error or a missing script file left the process hanging instead of exiting. Manage the Client with try-with-resources so it is always closed, and log out in a finally block so a failed script still ends the session. runShell now returns an exit code and run() exits once, after the client has been closed: - 0 (EX_OK) on success - 77 (EX_NOPERM) when the server rejects the login - 69 (EX_UNAVAILABLE) when the server cannot be reached - 70 (EX_SOFTWARE) for other server errors - 74 (EX_IOERR) when the script file cannot be read Invalid arguments and configuration errors still exit with 2, now through a named constant. Signed-off-by: miguelfigueiredo <miguelsfigueiredo90@gmail.com> Co-authored-by: Tony Germano <tony@germano.name> Signed-off-by: Tony Germano <tony@germano.name>
This PR implements a change on the CLI - When logging in, catch UnauthorizedException and fetch the login status from it.
Resolution for issue: #224