Skip to content

Guard the web-logbook upload handler against a missing file1 part - #623

Merged
patrickrb merged 1 commit into
devfrom
optio/task-e975f4c3-ce7e-4eb3-b999-f329af2958f1
Jul 22, 2026
Merged

Guard the web-logbook upload handler against a missing file1 part#623
patrickrb merged 1 commit into
devfrom
optio/task-e975f4c3-ce7e-4eb3-b999-f329af2958f1

Conversation

@patrickrb

Copy link
Copy Markdown
Owner

Root cause

The always-on web logbook server (LogHttpServer, started unconditionally at app launch and reachable on the LAN) handles ADIF uploads at /IMPORTLOGDATA. doImportLogFile read the multipart upload as:

String param = files.get("file1");
ImportTaskList.ImportTask task = importTaskList.addTask(param.hashCode()); // NPE if param == null

IHTTPSession.parseBody(files) only inserts a "file1" entry when the POST/PUT actually carries a file field with that name. A malformed or non-form request — or a bare LAN probe of the logbook port — leaves the map without the key, so param is null and param.hashCode() throws NullPointerException.

That NullPointerException is thrown in the leading dispatch chain of serve() (line ~221), which runs before serve()'s own try/catch (which starts further down and only catches IOException | ResponseException anyway). The throw therefore escapes serve() entirely. NanoHTTPD's worker (ClientHandler.run()) catches it in its generic catch (Exception) and logs "Communication with the client broken", so it is not an app crash — the request is aborted with no useful response.

This is the last unguarded external-input deref in a file whose siblings were already hardened:

Fix

  • Extract a bounds-safe static helper uploadedFilePath(Map<String,String> files) that returns the file1 temp-file path, or null when the part is absent (also null-safe on a null map). This mirrors the existing uriSegment / parseQueryInt static guards and keeps the decision logic unit-testable per the project's Compose/JNI-free testing rule.
  • Reject a null result with the existing R.string.html_illegal_command page — the same fallback a non-POST method already returns — instead of dereferencing it.

Well-formed uploads are byte-for-byte identical: a present file1 flows through unchanged.

Testing performed

  • New LogHttpServerUploadPartTest — pure-JVM (no Robolectric), mirroring LogHttpServerUriSegmentTest / LogHttpServerQueryParamTest. Covers present part, missing part, wrong field name, null map, and empty-string path (which must stay "present", since the downstream LogFileImport opens it and fails with a caught FileNotFoundException).
  • ./gradlew :app:testDebugUnitTest — full suite green.
  • ./gradlew :app:assembleDebug — green (all 4 ABIs, incl. native/hamlib build).

Risk assessment

Very low. Pure Java, one added null-guard on an error path plus a thin extracted helper; no protocol, DSP, threading, or native changes; no behavior change for well-formed requests. LAN-facing reliability (category-2) hardening — completes the LogHttpServer external-input sweep.

Affected platforms

Android (the web-logbook server is Android-only).

The always-on web logbook's /IMPORTLOGDATA handler read the multipart
upload as `files.get("file1").hashCode()`. NanoHTTPD's parseBody() only
adds a "file1" entry when the POST/PUT actually carries that file field,
so a malformed or non-form request — or a bare LAN probe of the logbook
port — left the map without the key and the unconditional deref threw a
NullPointerException.

That throw escaped serve() before its try/catch (the IMPORTLOGDATA branch
runs in the leading dispatch chain, ahead of the try at the bottom of
serve), so NanoHTTPD's worker aborted the request with no useful response.
The sibling handlers in this same file were already hardened against
missing path segments (uriSegment, PR #584) and malformed pagination
params (parseQueryInt/clampPageIndex, PR #585); this closes the matching
gap for the upload part.

Fix: extract a bounds-safe `uploadedFilePath(Map)` helper (null when the
part is absent) mirroring the existing static guards, and reject a null
result with the existing html_illegal_command page — the same fallback a
non-POST method already returns — instead of dereferencing it. Well-formed
uploads are byte-identical.

Adds LogHttpServerUploadPartTest (pure-JVM, no Robolectric, mirroring
LogHttpServerUriSegmentTest / LogHttpServerQueryParamTest).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens the LAN-reachable LogHttpServer /IMPORTLOGDATA upload handler against malformed multipart requests that omit the file1 upload part, preventing an uncaught NullPointerException during request dispatch and returning a consistent “illegal command” response instead.

Changes:

  • Added LogHttpServer.uploadedFilePath(Map<String,String>) to safely fetch the file1 temp-file path (null-safe).
  • Updated doImportLogFile to reject missing file1 uploads with the existing html_illegal_command page instead of dereferencing null.
  • Added a pure-JVM unit test covering present/missing/wrong-name/null-map/empty-path cases for the new helper.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
ft8af/app/src/main/java/com/k1af/ft8af/html/LogHttpServer.java Adds a null-safe upload-part helper and uses it to guard /IMPORTLOGDATA against missing file1 parts.
ft8af/app/src/test/java/com/k1af/ft8af/html/LogHttpServerUploadPartTest.java Adds pure-JVM regression tests pinning the new upload-part guard behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@patrickrb
patrickrb merged commit 09e3908 into dev Jul 22, 2026
16 checks passed
@patrickrb
patrickrb deleted the optio/task-e975f4c3-ce7e-4eb3-b999-f329af2958f1 branch July 22, 2026 22:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants