kit: harden documentStatus() against unchecked JSON parsing#15738
Closed
SUGE2016 wants to merge 1 commit intoCollaboraOnline:mainfrom
Closed
kit: harden documentStatus() against unchecked JSON parsing#15738SUGE2016 wants to merge 1 commit intoCollaboraOnline:mainfrom
SUGE2016 wants to merge 1 commit intoCollaboraOnline:mainfrom
Conversation
documentStatus() invokes Poco::JSON::Parser::parse() in two places
without any guards:
- getMode(partData) parses the JSON returned by getPartInfo().
- The .uno:AllPageSize branch parses the JSON returned by
getCommandValues().
If the underlying LibreOfficeKit call returns an empty C-string (which
can happen with minimal/edge-case documents), Poco throws "JSON
Exception: unexpected end of text". The exception escapes
documentStatus(), is caught by Session::handleMessage, and the
"status: ..." reply is silently dropped.
Symptom on Android (and presumably any embedder that triggers the
edge case): the document loads in Kit, the view is created, but the
client never receives the status frame, so it never enters edit mode
and shows a blank screen. Once the user backgrounds the app and
returns, the auto-save fires and produces the misleading
"cmd=save kind=nodocloaded" error, because _isDocLoaded was never
set to true.
This change makes both JSON parses defensive:
- getMode("") returns 0 instead of throwing.
- .uno:AllPageSize parsing is wrapped in try/catch and additionally
skipped when the returned string is empty.
No behavior change on the happy path; documentStatus() now degrades
gracefully when individual sub-fields cannot be parsed.
Reproduced and verified on Android (OnePlus 13T, arm64-v8a) where
opening external .doc / .odt documents went from blank screen to
fully working editor after this fix.
Co-authored-by: Cursor <cursoragent@cursor.com>
|
Thanks for opening this pull request! Things that will help get your PR across the finish line:
|
|
Thank you for your contribution! However, the main branch on GitHub is not used for this project. Please submit patches to the main branch of the online repo at https://gerrit.collaboraoffice.com. See CONTRIBUTING.md for details. This PR has been automatically closed. |
2 tasks
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.
Summary
documentStatus()(inkit/KitHelper.hpp) callsPoco::JSON::Parser::parse()in two places without any error handling. When the underlyingLibreOfficeKitcall returns an empty C-string for an edge-case document, Poco throwsJSON Exception: unexpected end of text, the exception escapesdocumentStatus(), is caught bySession::handleMessage, and thestatus: ...reply to the client is silently dropped.This PR adds minimal defensive guards (zero behavior change on the happy path).
How it manifests
Discovered while debugging the Android app. Sequence in logcat:
```
INF Document url [...] for session [001] loaded view [0]. Have 1 view.
INF ToMaster-001: Created new view with viewid: [0]
DBG ToMaster-001: Sending status after loading view 0
ERR Exception while handling [load url=...] in ToMaster-001:
JSON Exception: unexpected end of text
```
The document loads, the view is created, but the client never receives the `status:` frame, so it never enters edit mode → blank screen. Once the user backgrounds the app and returns, the auto-save fires and produces the misleading `cmd=save kind=nodocloaded` error, because `_isDocLoaded` was never set to `true`.
This was likely never reported on the desktop / web Collabora because users there never trigger the affected edge case (they always open existing documents). The Android app produced documents that did, exposing the latent issue.
Changes
Both sites already had fallbacks for when the field is absent — this just extends the same robustness to malformed/empty values.
Test plan
Made with Cursor