fix(providers): stop trim_text from corrupting listed names around XML entities (LIVE-1) - #536
Conversation
…ted keys quick-xml emits Text fragments around GeneralRef events; with trim_text(true) each fragment was trimmed, so a key like 'a & b.txt' was listed as 'a&b.txt' (or split into bogus fragments). Upload stores the real name, listing returned the mangled one, GET of the listed name 404'd, and 'sync --direction download --delete' planned delete_local on the user's real file. All eight S3 readers now run trim_text(false): whitespace-only indentation fragments are skipped, Key/Prefix fragments accumulate raw with GeneralRef expansion, and scalar fields (Size, ETag, LastModified, tokens, markers, IsLatest) are trimmed once at consumption. Live-verified failing before this fix on MinIO and Nextcloud WebDAV (axpbuntu lab), FTPS unaffected. Agent: Kimi L2
…ent whitespace Same LIVE-1 class as the s3.rs fix: name-bearing quick-xml readers now run trim_text(false), accumulate Text fragments raw with GeneralRef expansion, skip whitespace-only indentation fragments, and trim scalar fields once at consumption (sizes, dates, etags, markers, quota values, hrefs). jottacloud folder/trash listings keep trim_text(true) with a comment: their names arrive as attributes via xml_text::attr_value and never touch Text events. Regression tests: entity-adjacent whitespace round-trips exactly on WebDAV PROPFIND displaynames and Jottacloud device/mountpoint names (the S3 versions/batch-delete cases ride the previous commit). cargo test --lib providers:: 810/810 green; clippy clean. Agent: Kimi L2
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
|
Warning Review limit reached
Next review available in: 21 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughUpdated Azure, Jottacloud, S3, and WebDAV XML parsers to preserve meaningful whitespace adjacent to XML entities while skipping indentation-only fragments. Final scalar fields and pagination markers are trimmed, with regression tests added for affected name and error parsing. ChangesXML parsing normalization
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src-tauri/src/providers/s3.rs (1)
1174-1189: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winOne root cause: the "skip whitespace-only fragment" filter is unconditional, so it still deletes name payload. A whitespace-only
Textevent is indentation only when no name-bearing element is open; between two entity refs (a& &b) or adjacent to one at an element edge (" &x") it is part of the name.jottacloud.rsgets this right — it accumulates every fragment whilein_nameand trims once at the end tag; the other readers should adopt the same shape (or gate the skip on the current element).
src-tauri/src/providers/s3.rs#L1174-L1189: skip only whencurrent_tag/contextis notContents/KeyorCommonPrefixes/Prefix.src-tauri/src/providers/s3.rs#L2330-L2341: skip only when neitherinside_keynorinside_next_token.src-tauri/src/providers/s3.rs#L3764-L3786: skip only when notin_contents && find_tag == "Key"and notin_next_tok.src-tauri/src/providers/s3.rs#L4218-L4223: skip only when not accumulatingKeyinside<Version>(and not in a marker element).src-tauri/src/providers/s3.rs#L4912-L4928: skip only when neitherin_keynorin_value.src-tauri/src/providers/s3.rs#L5303-L5333: skip only whencurrent_tag != "Key".src-tauri/src/providers/s3.rs#L5475-L5480: skip only when not accumulatingKeyinside<Version>/<DeleteMarker>.src-tauri/src/providers/s3.rs#L5612-L5658: extend the regression tests with a whitespace-only interior fragment (a& &b.txt) and a leading space before an entity, which the current filter still corrupts.src-tauri/src/providers/azure.rs#L519-L524: skip only whenstateis notBlobName/BlobPrefixName.src-tauri/src/providers/azure.rs#L2281-L2286: skip only when notin_blob && tag_name == "Name".src-tauri/src/providers/webdav.rs#L1506-L1519: skip only when the tag is nothref/trashbin-filename/trashbin-original-location.src-tauri/src/providers/webdav.rs#L1899-L1917: skip only when the tag is nothref/displayname.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src-tauri/src/providers/s3.rs` around lines 1174 - 1189, The whitespace-only Text-fragment filter must preserve name payload while still ignoring indentation: in src-tauri/src/providers/s3.rs#L1174-L1189 gate skipping on current_tag/context outside Contents/Key and CommonPrefixes/Prefix; at `#L2330-L2341` gate on neither inside_key nor inside_next_token; at `#L3764-L3786` gate on neither the Key-in-Contents nor next-token accumulation; at `#L4218-L4223` gate on neither Version Key accumulation nor marker elements; at `#L4912-L4928` gate on neither in_key nor in_value; at `#L5303-L5333` gate on current_tag not being Key; and at `#L5475-L5480` gate on neither Version/DeleteMarker Key accumulation. Extend the regression tests at src-tauri/src/providers/s3.rs#L5612-L5658 for whitespace-only interior fragments and leading whitespace before an entity. Apply equivalent state/tag-aware gating in src-tauri/src/providers/azure.rs#L519-L524 (state outside BlobName/BlobPrefixName) and `#L2281-L2286` (not in_blob with tag_name Name), and in src-tauri/src/providers/webdav.rs#L1506-L1519 (tags outside href/trashbin-filename/trashbin-original-location) and `#L1899-L1917` (tags outside href/displayname), preserving all fragments while the relevant name element is open and trimming only at the completed value.
🧹 Nitpick comments (1)
src-tauri/src/providers/s3.rs (1)
1362-1397: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
extract_xml_tagnow slurps nested element text and never resetsaccon a non-matching close.While
inside_targetis set, every descendantTextis appended, soextract_xml_tag(body, "Error")on<Error><Code>X</Code><Message>Y</Message></Error>returnsXYrather than nothing. Current call sites only pass leaf tags (UploadId,ETag,Code,Message), so this is latent rather than live — worth adepthguard or restricting accumulation to the immediate child text if the helper is reused.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src-tauri/src/providers/s3.rs` around lines 1362 - 1397, The extract_xml_tag helper incorrectly accumulates descendant text for matching parent elements and leaves the accumulator active across nested closes. Update extract_xml_tag to track nesting depth or otherwise collect text only from the target element’s immediate content, while preserving entity handling and returning trimmed leaf-tag text for existing callers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src-tauri/src/providers/s3.rs`:
- Around line 1174-1189: The whitespace-only Text-fragment filter must preserve
name payload while still ignoring indentation: in
src-tauri/src/providers/s3.rs#L1174-L1189 gate skipping on current_tag/context
outside Contents/Key and CommonPrefixes/Prefix; at `#L2330-L2341` gate on neither
inside_key nor inside_next_token; at `#L3764-L3786` gate on neither the
Key-in-Contents nor next-token accumulation; at `#L4218-L4223` gate on neither
Version Key accumulation nor marker elements; at `#L4912-L4928` gate on neither
in_key nor in_value; at `#L5303-L5333` gate on current_tag not being Key; and at
`#L5475-L5480` gate on neither Version/DeleteMarker Key accumulation. Extend the
regression tests at src-tauri/src/providers/s3.rs#L5612-L5658 for
whitespace-only interior fragments and leading whitespace before an entity.
Apply equivalent state/tag-aware gating in
src-tauri/src/providers/azure.rs#L519-L524 (state outside
BlobName/BlobPrefixName) and `#L2281-L2286` (not in_blob with tag_name Name), and
in src-tauri/src/providers/webdav.rs#L1506-L1519 (tags outside
href/trashbin-filename/trashbin-original-location) and `#L1899-L1917` (tags
outside href/displayname), preserving all fragments while the relevant name
element is open and trimming only at the completed value.
---
Nitpick comments:
In `@src-tauri/src/providers/s3.rs`:
- Around line 1362-1397: The extract_xml_tag helper incorrectly accumulates
descendant text for matching parent elements and leaves the accumulator active
across nested closes. Update extract_xml_tag to track nesting depth or otherwise
collect text only from the target element’s immediate content, while preserving
entity handling and returning trimmed leaf-tag text for existing callers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 300db377-cdef-4d7d-915a-0bdcb004f467
📒 Files selected for processing (4)
src-tauri/src/providers/azure.rssrc-tauri/src/providers/jottacloud.rssrc-tauri/src/providers/s3.rssrc-tauri/src/providers/webdav.rs
…name elements
CodeRabbit follow-up on the trim_text fix: the skip-whitespace-only-fragment
filter was unconditional, so a Text(" ") fragment between two entity refs
(a& &b.txt) or at an element edge ( &x.txt) was still dropped,
rebuilding the key with a hole. Gate the skip at all 11 listing-reader sites
so the fragment is preserved whenever a payload-bearing element is open and
skipped only when it is pure pretty-print indentation.
Also add a nesting-depth guard to extract_xml_tag so a container tag no
longer concatenates descendant text; leaf-tag callers are unchanged.
Regression tests: a& &b.txt -> a& &b.txt and &x.txt -> &x.txt
across the s3, azure and webdav parsers, with pretty-print indentation
still ignored.
Agent: Kimi L2
1 participant
Closes LIVE-1, the blocker of the independent pre-release audit (PR #535, security-evidence §8.1).
The bug
Every name-bearing quick-xml reader was configured
trim_text(true). For a key likesp ace & ünïcodé.txtthe server sends<Key>sp ace & ünïcodé.txt</Key>; quick-xml emitsText("sp ace ") + GeneralRef("amp") + Text(" ünïcodé.txt")and each fragment was trimmed, so listings showedsp ace&ünïcodé.txt. Upload stores the correct name, so GET of the listed name 404'd, andsync --direction download --deleteplanneddelete_localon the user's real file (data-loss class, live-verified on release code against the axpbuntu lab).The fix
17 readers across 4 providers moved to the same pattern:
trim_text(false), whitespace-only indentation fragments skipped, Key/name fragments accumulate raw withGeneralRefexpansion viaxml_text::xml_entity_to_str, scalar fields (sizes, dates, etags, markers, quota, hrefs) trimmed once at consumption.s3.rs: ListObjectsV2, extract_xml_tag, all-keys pagination, find, ListObjectVersions, tagging, batch-delete errors, trash page (8 readers)webdav.rs: single-file PROPFIND, Nextcloud trashbin, main PROPFIND listing, extract_xml_properties (4)azure.rs: blob list, deleted-blobs list, XML error parse (3)jottacloud.rs: device names, mountpoint names (2); folder/trash listings verified safe (names arrive as attributes viaattr_value) and annotated as suchsts.rsdeliberately untouched (credentials are base64, no entities)Verification
cargo test --lib providers::810/810 green; clippy clean; fmt appliedput/ls/getofraw & name2.txtandsp ace & ünïcodé.txtround-trip with sha256 intact; the listing shows the real namessync --direction download --delete --dry-runon both: plan is downloads-only, zerodelete_local(the cascade is dead)Agent: Kimi L2
Summary by CodeRabbit