Fail the upload when the converter returns no document - #545
Merged
Conversation
use.opendocument.app answers a converted document with a redirect to it, and doOnlineConvert read the Location header without checking it was there. When the server refuses the file it answers without one, getHeaderField returns null, and the null went straight into the string concatenation - so the loader reported success for the uri "https://use.opendocument.appnull/". The user got a webview DNS error page instead of the "could not open" dialog, and the log a net::ERR_NAME_NOT_RESOLVED that says nothing about what actually went wrong. Treat a missing Location the way doTransferUpload already treats an empty body: throw, with the response code and the error body in the message, so loadSync reports the error and the offer to reopen comes back. Found by opening a truncated .odt whose central directory offset points past the entries: the core rejects it as "not a zip file" and the converter, given the same file, refuses it too. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UgPw1bJBn1sfdw4LuL5dkW
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
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.
What
doOnlineConvertread theLocationheader without checking it was there:use.opendocument.appanswers a converted document with a redirect to it, so when it refuses a file there is noLocation,getHeaderFieldreturnsnull, and the null goes straight into the concatenation. The loader then reported success for the urihttps://use.opendocument.appnull/.How it shows up
Opening a truncated
.odton an emulator:Two things go wrong: the user gets a WebView DNS error page rather than the "could not open" dialog and the offer to reopen, and the log line blames DNS instead of the server refusing the document.
The fix
Treat a missing
Locationthe waydoTransferUploadalready treats an empty body — throw anIOExceptioncarrying the response code and the error body, soloadSyncroutes it throughcallOnError.The guard is on the header rather than on the status code alone, so a
2xx-with-Locationresponse keeps working; the code is included for diagnostics.Notes
The file that surfaced this is genuinely corrupt — it carries two end-of-central-directory records, and the authoritative last one points at offset 24822, which is zeros, while the real central directory sits at 24089.
unziprejects it too, so the core'snot a zip fileis correct and unchanged here. This PR is only about the fallback path lying about having succeeded.The bug predates the Kotlin conversion; the pre-conversion Java had the identical two lines.
Not covered by a unit test:
doOnlineConvertis private, needs a liveHttpURLConnection, andUri.parseneeds a real Android runtime — the existingOnlineLoaderTestonly exercises the pureisSupported/isConvertiblepredicates, and there is no MockWebServer/Robolectric setup to hang a test on.Test plan
./gradlew spotlessCheck testProDebugUnitTest— green