Refactors Dolphin quick access integration for robustness - Fixes #3883 #114
Conversation
|
|
WalkthroughThis pull request introduces a comprehensive refactor of the Estimated code review effort3 (~45 minutes) Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧠 Learnings (2)📓 Common learningssrc/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java (6)Learnt from: infeo Learnt from: jrauber Learnt from: infeo Learnt from: jrauber Learnt from: infeo Learnt from: infeo 🧰 Additional context used🧠 Learnings (2)📓 Common learningssrc/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java (6)Learnt from: infeo Learnt from: jrauber Learnt from: infeo Learnt from: jrauber Learnt from: infeo Learnt from: infeo 🔇 Additional comments (8)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
src/main/java/org/cryptomator/linux/quickaccess/FileConfiguredQuickAccess.java (1)
34-42: Fix Javadoc typos & complete tags“quickacces” is missing the second “s”, and the
@throwsline lacks a description. Consider:- * Adds the vault path to the quickacces config file + * Adds the vault path to the quick-access config file. ... - * @throws QuickAccessServiceException + * @throws QuickAccessServiceException if the entry cannot be writtensrc/test/java/org/cryptomator/linux/quickaccess/DolphinPlacesTest.java (3)
86-90: Assert specific exception typeCatching generic
Exceptionrisks masking unrelated failures.
Replace with the domain exception:- assertThrows(Exception.class, () -> { + assertThrows(QuickAccessServiceException.class, () -> {…and import the type.
105-109: Same issue as above – narrow the expected exception toQuickAccessServiceException.
162-172: Helper method should beprivate
countOccurrencesis only used inside this test class; mark itprivateto avoid unintended exposure.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/main/java/module-info.java(1 hunks)src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java(4 hunks)src/main/java/org/cryptomator/linux/quickaccess/FileConfiguredQuickAccess.java(1 hunks)src/test/java/org/cryptomator/linux/quickaccess/DolphinPlacesTest.java(1 hunks)src/test/resources/quickaccess/dolphin/user-places-multiple-identical.xbel(1 hunks)src/test/resources/quickaccess/dolphin/user-places-not-valid.xbel(1 hunks)src/test/resources/quickaccess/dolphin/user-places-not-well-formed.xbel(1 hunks)src/test/resources/quickaccess/dolphin/user-places.xbel(1 hunks)
🔇 Additional comments (6)
src/test/resources/quickaccess/dolphin/user-places-not-well-formed.xbel (1)
1-39: Test resource correctly simulates malformed XML
This file intentionally contains XML syntax errors to validate the parser’s handling of non well-formed XBEL documents. It aligns with the test scenario.src/test/resources/quickaccess/dolphin/user-places-multiple-identical.xbel (1)
1-28: Valid test resource for multiple identical bookmarks
The file is well-formed and accurately simulates duplicate<bookmark>entries for replacement logic tests.src/test/resources/quickaccess/dolphin/user-places.xbel (1)
1-39: Valid test resource for baseline bookmarks
This resource is well-formed and provides a solid base case for verifying standard bookmark management.src/test/resources/quickaccess/dolphin/user-places-not-valid.xbel (1)
1-39: Test resource correctly simulates invalid XBEL structure
The use of<brokenbookmark>represents a schema violation and is suitable for testing validation error paths.src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java (2)
158-166: Enable namespace awareness in XML parserThe XBEL schema relies on namespaces (e.g., the
bookmark:prefix).
DocumentBuilderFactoryis created withoutsetNamespaceAware(true), which can break validation and XPath queries involving prefixes.builderFactory = DocumentBuilderFactory.newInstance(); +builderFactory.setNamespaceAware(true);
221-249: Ensurebookmark:prefix is bound
createElement("bookmark:icon")inserts a prefixed element without declaring the namespace.
If the original document lacksxmlns:bookmark, validation will fail.
Either create with namespace:var bookmarkIcon = xmlDocument.createElementNS("http://www.freedesktop.org/standards/desktop-bookmarks", "bookmark:icon");or verify the namespace is already declared before insertion.
src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java (2)
56-60: Trim the trailing slash inCONFIG_PATH_IN_HOMEto avoid double-separator segments
Path.oftreats each argument as an entire path segment; embedding a/inside the segment produces.../.local/share//user-places.xbel(note the empty segment) on Linux.
While the OS will silently normalise this, it is unnecessary and may break path comparisons.- private static final String CONFIG_PATH_IN_HOME = ".local/share/"; + private static final String CONFIG_PATH_IN_HOME = ".local/share";
247-279: Consider creating elements with the XBEL namespace to keep the document validAll newly-created elements are namespace-less, whereas real XBEL files usually declare
xmlns="http://www.freedesktop.org/standards/desktop-bookmarks".
Mixing namespaced and non-namespaced nodes passes validation only because the parser is currently not namespace-aware (see previous comment).
Should namespace awareness be enabled later, validation will start failing.Two minimal options:
- Keep the factory non-namespace-aware (status quo).
- Preferable: enable namespaces and create elements with the correct URI:
private static final String XBEL_NS = "http://www.freedesktop.org/standards/desktop-bookmarks"; var bookmark = xmlDocument.createElementNS(XBEL_NS, "bookmark"); ...Please verify which strategy you want to commit to before this code reaches users.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java(4 hunks)
🔇 Additional comments (1)
src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java (1)
140-156: 👍 XPath variable resolver fixes the earlier quote-escaping bugSwitching from string concatenation to a variable-bound XPath eliminates injection issues and correctly handles special characters in
target.toUri().
src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/test/java/org/cryptomator/linux/quickaccess/DolphinPlacesTest.java (3)
28-30: Use method reference to remove superfluous lambda noise
assertDoesNotThrowaccepts aExecutable. A method reference clearly communicates intent and removes an unnecessary block that returns a value which is subsequently ignored.-assertDoesNotThrow(() -> { new DolphinPlaces(); }); +assertDoesNotThrow(DolphinPlaces::new);
133-145: PotentialFileAlreadyExistsException– copy should replace when rerun
Files.copywithout an explicit option fails if a file with the same name already exists.
If the same temp directory is reused (e.g. when running tests in parallel or after a test abort), the helper will blow up before the actual test executes.-import java.nio.file.Files; +import java.nio.file.Files; +import java.nio.file.StandardCopyOption; @@ - Files.copy(stream, targetDir.resolve("user-places.xbel")); + Files.copy(stream, + targetDir.resolve("user-places.xbel"), + StandardCopyOption.REPLACE_EXISTING);
163-173: Helper leaks unnecessary public API & could be static
countOccurrencesis only consumed inside this test class. Making itprivate staticavoids exporting it as part of the test’s public surface and communicates that it has no instance-level dependency.-public int countOccurrences(String content, String searchString) { +private static int countOccurrences(String content, String searchString) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/org/cryptomator/linux/quickaccess/FileConfiguredQuickAccess.java(1 hunks)src/test/java/org/cryptomator/linux/quickaccess/DolphinPlacesTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/org/cryptomator/linux/quickaccess/FileConfiguredQuickAccess.java
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java (1)
190-199:⚠️ Potential issueHarden XML parser – still vulnerable to XXE via
<!DOCTYPE>
FEATURE_SECURE_PROCESSINGplus theexternal-*switches are a good start, but without explicitly disabling theDOCTYPEdeclaration (disallow-doctype-decl) an attacker can still craft an XXE payload that slips through.builderFactory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true); +builderFactory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true); builderFactory.setFeature("http://xml.org/sax/features/external-general-entities", false); builderFactory.setFeature("http://xml.org/sax/features/external-parameter-entities", false);(Optionally also disable
http://apache.org/xml/features/nonvalidating/load-external-dtd.)
🧹 Nitpick comments (5)
src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java (3)
219-224: Explicit encoding when serialising XML
Transformerwill default to the platform charset ifOutputKeys.ENCODINGis omitted. On non-UTF-8 locales this produces invalid XBEL for KDE which expects UTF-8.xform.setOutputProperty(OutputKeys.INDENT, "yes"); // optional +xform.setOutputProperty(OutputKeys.ENCODING, StandardCharsets.UTF_8.name());(StandardCharsets is already imported.)
124-140: Simplify & avoid extra allocation when removing stale nodesCopying the
NodeListinto anArrayListjust to iterate again is unnecessary.
Iterating while the list is live avoids both allocation and the skipped-index pitfall.-var nodesToRemove = new ArrayList<Node>(); -for (int i = 0; i < nodeList.getLength(); i++) { - nodesToRemove.add(nodeList.item(i)); -} - -for (Node node : nodesToRemove) { - Node parent = node.getParentNode(); - if (parent != null) { - parent.removeChild(node); - } -} +while (nodeList.getLength() > 0) { + Node parent = nodeList.item(0).getParentNode(); + if (parent != null) { + parent.removeChild(nodeList.item(0)); + } +}
331-341: Remove redundant type parametersDiamond operator is sufficient.
- private final Map<QName, Object> vars = new HashMap<QName, Object>(); + private final Map<QName, Object> vars = new HashMap<>();src/test/java/org/cryptomator/linux/quickaccess/DolphinPlacesTest.java (2)
151-160: Read fixtures with an explicit charset
Files.readStringuses the platform default encoding; tests executed on non-UTF-8 systems will fail.-return Files.readString(file); +return Files.readString(file, java.nio.charset.StandardCharsets.UTF_8);(Requires
import java.nio.charset.StandardCharsets;at the top.)
37-50: Assert post-removal state to strengthen the testThe test removes the entry but never checks the file afterwards.
A simple assertion would close the loop and protect against regressions inremove().entry.remove(); assertFalse(loadFile(pathToDoc).contains(CAPTION_FOLDER_1), "Entry was not removed");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java(4 hunks)src/test/java/org/cryptomator/linux/quickaccess/DolphinPlacesTest.java(1 hunks)
🔇 Additional comments (1)
src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java (1)
258-266: Namespace prefix of<icon>element may be lost
createElementNS(XBEL_NAMESPACE, "icon")will serialise as
<icon xmlns="…"> …, i.e. without the expectedbookmark:prefix.
KDE generally tolerates any prefix, but some third-party consumers (and your own XSD validation) might rely on the prefixed form.Consider
createElementNS(XBEL_NAMESPACE, "bookmark:icon")or usesetPrefix("bookmark")afterwards.
This commit refactors the Dolphin quick access integration to use DOM manipulation for adding and removing bookmarks in the XBEL file. This change aims to improve the reliability and correctness of bookmark management by handling stale entries. Changes - Replaces string-based manipulation of the XBEL file with DOM manipulation using org.w3c.dom. - Introduces methods for loading, validating, and writing the XBEL document. - Implements bookmark creation and removal using XPath queries and DOM operations. - Adds more test cases to DolphinPlacesTest to cover adding and removing bookmarks under different scenarios. Impact - Improves the robustness of the Dolphin quick access integration by using standard XML parsing and manipulation techniques. - The tests are modified to verify correct bookmark management. Fixes: cryptomator/cryptomator#3883 Author: jrauber <jrauber@user.noreply.github.com> Date: Fri Jun 6 00:38:43 2025 +0200
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java (3)
182-190: Redundant / vendor-specific SAX features may throw on some JDKs
DocumentBuilderFactory#setFeaturewith
"http://xml.org/sax/features/external-general-entities"and
"http://xml.org/sax/features/external-parameter-entities"is not guaranteed to
be supported by every JAXP implementation. Some (e.g. the default OpenJDK impl)
silently ignore them, others throwParserConfigurationException, which you
convert into aQuickAccessServiceException.You already disable external access via
ACCESS_EXTERNAL_DTD/SCHEMAand provide a dummyEntityResolver, which is
sufficient. Dropping the two SAX features would make the code more portable and
remove an unnecessary failure-path.
136-144: Micro-optimisation: reuse XPath objects
XPathFactory.newInstance()is comparatively expensive and you create a fresh
XPathfor every extraction. Given that the expressions are constant, cache a
singleXPath(and perhaps pre-compile the expression) or at least a factory at
class level. This will reduce object churn inside tight add/remove loops.
248-277: Namespace prefix may go missing
createElementNS(XBEL_NAMESPACE, "bookmark:icon")is correct, but if the root
document does not already declare thebookmarkprefix, the serializer will
addxmlns:bookmark="…"on the<bookmark:icon>element only, leading to
visually noisy output and potential validator complaints in other tools.Consider ensuring the
xmlns:bookmarkdeclaration is present on the root once
(perhaps during initial load) instead of declaring it on every new icon node.src/test/java/org/cryptomator/linux/quickaccess/DolphinPlacesTest.java (4)
30-32:assertDoesNotThrowaround assertions is pointlessWrapping the entire body – which already contains ordinary assertions – in
assertDoesNotThrowmasks the real intent and yields the same failure semantics
as leaving it out. Drop the outer wrapper and keep the direct assertions for
cleaner tests.-assertDoesNotThrow(() -> { new DolphinPlaces(); }); +new DolphinPlaces(); // will raise if the ctor throws
64-75: Same pattern as aboveThe lambda passed to
assertDoesNotThrowalready contains assertions; remove
the wrapper for clarity and to avoid disguising assertion failures as
“unexpected exceptions”.
134-145: Resource helper hard-codes target filename
loadResourceToDiralways copies touser-places.xbelirrespective of the
source resource’s name.
Running tests in parallel on the same@TempDir(or expanding the helper for
other resources later) could lead to unexpected overwrites.Pass the desired filename explicitly or derive it from
source:-Files.copy(stream, targetDir.resolve("user-places.xbel"), …); +Path dest = targetDir.resolve(Path.of(source).getFileName()); +Files.copy(stream, dest, StandardCopyOption.REPLACE_EXISTING);
164-174: Manual substring counting can be simplified
content.split(Pattern.quote(searchString), -1).length - 1is shorter,
doesn’t require a loop, and avoids accidental overlap issues.
Minor, but improves readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.idea/developer-tools.xml(1 hunks)src/main/java/module-info.java(1 hunks)src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java(4 hunks)src/main/java/org/cryptomator/linux/quickaccess/FileConfiguredQuickAccess.java(1 hunks)src/test/java/org/cryptomator/linux/quickaccess/DolphinPlacesTest.java(1 hunks)src/test/resources/quickaccess/dolphin/user-places-multiple-identical.xbel(1 hunks)src/test/resources/quickaccess/dolphin/user-places-not-valid.xbel(1 hunks)src/test/resources/quickaccess/dolphin/user-places-not-well-formed.xbel(1 hunks)src/test/resources/quickaccess/dolphin/user-places.xbel(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- src/main/java/module-info.java
- .idea/developer-tools.xml
- src/main/java/org/cryptomator/linux/quickaccess/FileConfiguredQuickAccess.java
- src/test/resources/quickaccess/dolphin/user-places-multiple-identical.xbel
- src/test/resources/quickaccess/dolphin/user-places.xbel
- src/test/resources/quickaccess/dolphin/user-places-not-valid.xbel
- src/test/resources/quickaccess/dolphin/user-places-not-well-formed.xbel
src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java (1)
209-214: DOCTYPE still dropped when serialising – see earlier comment.
Including the XML declaration and original DOCTYPE is mandatory for valid XBEL.
🧹 Nitpick comments (5)
.idea/developer-tools.xml (1)
1-6: IDE-specific.ideaartefact should not be committed
Project-local IntelliJ settings usually belong in.gitignore. Committing them tends to create merge noise and leaks local preferences.+# .idea/ workspace files +.idea/(or simply delete the file and directory from the repo).
src/test/resources/quickaccess/dolphin/user-places-not-valid.xbel (1)
2-2: Fix typo in commentUse “an invalid structure” instead of “a invalid structure”.
src/main/java/org/cryptomator/linux/quickaccess/FileConfiguredQuickAccess.java (1)
34-42: Trim leading blank line in JavadocStarting the Javadoc block with an empty line is against the project’s style-guide; delete the first blank to stay consistent.
src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java (1)
145-148: Consider normalising file URIs before XPath comparison
Path.toUri()returns a URI without trailing slash, whereas Dolphin can store both
file:///foo/barandfile:///foo/bar/. Normalise (e.g.toUri().normalize())
or compare viastarts-with()to avoid false “duplicate” entries.src/test/resources/quickaccess/dolphin/user-places-not-well-formed.xbel (1)
2-2: Avoid hard-coding line numbers in inline commentsThe explanatory comment mentions “see line 9 to 11”, which will quickly become misleading if the file is edited. A wording that omits specific line numbers is more future-proof.
-<!-- This file simulates a non well-formed document (see line 9 to 11)--> +<!-- This file intentionally contains malformed XML further below to test parser robustness -->
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.idea/developer-tools.xml(1 hunks)src/main/java/module-info.java(1 hunks)src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java(4 hunks)src/main/java/org/cryptomator/linux/quickaccess/FileConfiguredQuickAccess.java(1 hunks)src/test/java/org/cryptomator/linux/quickaccess/DolphinPlacesTest.java(1 hunks)src/test/resources/quickaccess/dolphin/user-places-multiple-identical.xbel(1 hunks)src/test/resources/quickaccess/dolphin/user-places-not-valid.xbel(1 hunks)src/test/resources/quickaccess/dolphin/user-places-not-well-formed.xbel(1 hunks)src/test/resources/quickaccess/dolphin/user-places.xbel(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
src/test/resources/quickaccess/dolphin/user-places-not-well-formed.xbel (1)
Learnt from: jrauber
PR: cryptomator/integrations-linux#114
File: src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java:0-0
Timestamp: 2025-06-15T14:06:09.523Z
Learning: XBEL (XML Bookmark Exchange Language) files used by KDE Dolphin legitimately contain DOCTYPE declarations as part of the format specification. The format requires `<!DOCTYPE xbel>` for formal validation against the official XBEL DTD. Applications reading XBEL files cannot disallow DOCTYPE declarations without breaking compatibility with the standard format.
src/test/resources/quickaccess/dolphin/user-places-multiple-identical.xbel (1)
Learnt from: jrauber
PR: cryptomator/integrations-linux#114
File: src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java:0-0
Timestamp: 2025-06-15T14:06:09.523Z
Learning: XBEL (XML Bookmark Exchange Language) files used by KDE Dolphin legitimately contain DOCTYPE declarations as part of the format specification. The format requires `<!DOCTYPE xbel>` for formal validation against the official XBEL DTD. Applications reading XBEL files cannot disallow DOCTYPE declarations without breaking compatibility with the standard format.
src/test/resources/quickaccess/dolphin/user-places-not-valid.xbel (1)
Learnt from: jrauber
PR: cryptomator/integrations-linux#114
File: src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java:0-0
Timestamp: 2025-06-15T14:06:09.523Z
Learning: XBEL (XML Bookmark Exchange Language) files used by KDE Dolphin legitimately contain DOCTYPE declarations as part of the format specification. The format requires `<!DOCTYPE xbel>` for formal validation against the official XBEL DTD. Applications reading XBEL files cannot disallow DOCTYPE declarations without breaking compatibility with the standard format.
src/test/resources/quickaccess/dolphin/user-places.xbel (1)
Learnt from: jrauber
PR: cryptomator/integrations-linux#114
File: src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java:0-0
Timestamp: 2025-06-15T14:06:09.523Z
Learning: XBEL (XML Bookmark Exchange Language) files used by KDE Dolphin legitimately contain DOCTYPE declarations as part of the format specification. The format requires `<!DOCTYPE xbel>` for formal validation against the official XBEL DTD. Applications reading XBEL files cannot disallow DOCTYPE declarations without breaking compatibility with the standard format.
src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java (1)
Learnt from: jrauber
PR: cryptomator/integrations-linux#114
File: src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java:0-0
Timestamp: 2025-06-15T14:06:09.523Z
Learning: XBEL (XML Bookmark Exchange Language) files used by KDE Dolphin legitimately contain DOCTYPE declarations as part of the format specification. The format requires `<!DOCTYPE xbel>` for formal validation against the official XBEL DTD. Applications reading XBEL files cannot disallow DOCTYPE declarations without breaking compatibility with the standard format.
🔇 Additional comments (5)
src/main/java/module-info.java (1)
21-21: Addition ofjava.xmlcorrectly satisfies DOM/XPath APIs – nice catch
The new dependency addresses the missing module problem noted earlier, andjava.desktopis no longer present. Looks good.If you have CI that runs
jdepsor similar, a quick sweep will confirm no superfluous modules remain.src/test/java/org/cryptomator/linux/quickaccess/DolphinPlacesTest.java (1)
27-33: Nice coverageGood to see constructor sanity checked – keeps CI fast.
src/test/resources/quickaccess/dolphin/user-places-not-well-formed.xbel (1)
9-11: Malformed fragment looks good for the negative-test purposeThe missing “<” before
metadataand the mismatched closing tags create the desired non-well-formed XML. No action needed as long as the associated unit test asserts that parsing fails and no write-back occurs.src/test/resources/quickaccess/dolphin/user-places-multiple-identical.xbel (1)
17-27: Test fixture correctly models duplicate bookmarksHaving two identical
<bookmark>elements with different Cryptomator IDs is exactly what the new duplicate-replacement logic must handle. 👍src/test/resources/quickaccess/dolphin/user-places.xbel (1)
1-39: Fixture appears well-formed and standards-compliantThe XML is valid XBEL with the mandatory
<!DOCTYPE xbel>and correct namespace usage. Nothing to change.
|
Multiple issues and some improvements are needed.
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java (2)
62-75: 🛠️ Refactor suggestionStatic
Validatoris still shared – thread-safety issue persists
Validatorinstances are not thread-safe. Re-using the singleton in parallel quick-access operations can corrupt internal state or throwIllegalStateExceptions.
This has already been pointed out in an earlier review and remains unresolved.- private static final Validator XML_VALIDATOR; +private static final javax.xml.validation.Schema XBEL_SCHEMA; ... - XML_VALIDATOR = factory.newSchema(schemaFile).newValidator(); +XBEL_SCHEMA = factory.newSchema(schemaFile);Then, where validation is required:
try (var validator = XBEL_SCHEMA.newValidator()) { validator.validate(new DOMSource(xmlDocument)); }
205-215: 🛠️ Refactor suggestionSerializer hacks lead to malformed DOCTYPE – emit correct identifiers instead
Setting
DOCTYPE_PUBLIC/SYSTEMto empty strings and later regex-patching the output yields
<!DOCTYPE xbel>– missing the official public & system IDs required for formal XBEL validity.-Transformer transformer = TransformerFactory.newInstance().newTransformer(); -transformer.setOutputProperty(OutputKeys.DOCTYPE_PUBLIC, ""); -transformer.setOutputProperty(OutputKeys.DOCTYPE_SYSTEM, ""); -transformer.setOutputProperty(OutputKeys.OMIT_XML_DECLARATION, "no"); +Transformer transformer = TransformerFactory.newInstance().newTransformer(); +transformer.setOutputProperty(OutputKeys.DOCTYPE_PUBLIC, + "+//IDN python.org//DTD XML Bookmark Exchange Language 1.0//EN//XML"); +transformer.setOutputProperty(OutputKeys.DOCTYPE_SYSTEM, + "http://www.python.org/topics/xml/dtds/xbel-1.0.dtd"); +transformer.setOutputProperty(OutputKeys.OMIT_XML_DECLARATION, "no");With the correct identifiers in place, the subsequent
replaceFirst/replacecalls (lines 218-221) become unnecessary and can be deleted, simplifyingdocumentToString.
🧹 Nitpick comments (2)
src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java (1)
218-221: String post-processing of XML output is brittle and unnecessaryManual regex replacement of
standaloneand DOCTYPE strings couples the serializer to specific JDK formatting and may fail on future versions.
Once the transformer is configured to emit the correct DOCTYPE (see previous comment), these replacements can be dropped.- content = content.replaceFirst("\\s*standalone=\"(yes|no)\"", ""); - content = content.replace("<!DOCTYPE xbel PUBLIC \"\" \"\">","<!DOCTYPE xbel>"); + // No post-processing required when transformer is correctly configuredsrc/test/java/org/cryptomator/linux/quickaccess/DolphinPlacesTest.java (1)
140-148: Test is overly brittle – hard-coded XML prolog comparisonThe root-object assertion hard-codes whitespace-stripped text of the first lines of the file.
Any innocuous formatting change (indentation, attribute order) will break the test while the functional contract remains intact.Consider parsing the file with a DOM builder and asserting on:
Document doc = DocumentBuilderFactory.newInstance().newDocumentBuilder().parse(pathToDoc.toFile()); assertEquals("xbel", doc.getDocumentElement().getNodeName());This keeps the test resilient to non-semantic formatting differences.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java(4 hunks)src/test/java/org/cryptomator/linux/quickaccess/DolphinPlacesTest.java(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java (1)
Learnt from: jrauber
PR: cryptomator/integrations-linux#114
File: src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java:0-0
Timestamp: 2025-06-15T14:06:09.523Z
Learning: XBEL (XML Bookmark Exchange Language) files used by KDE Dolphin legitimately contain DOCTYPE declarations as part of the format specification. The format requires `<!DOCTYPE xbel>` for formal validation against the official XBEL DTD. Applications reading XBEL files cannot disallow DOCTYPE declarations without breaking compatibility with the standard format.
src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java (3)
205-221: 🛠️ Refactor suggestionDOCTYPE preservation is incomplete – produces non-standard
<!DOCTYPE xbel>The transformer is given empty PUBLIC/SYSTEM ids and later the output string is regex-patched.
Result: a bare<!DOCTYPE xbel>– missing the XBEL public & system identifiers required for full validity.Prefer configuring the transformer explicitly:
Transformer tf = TransformerFactory.newInstance().newTransformer(); tf.setOutputProperty(OutputKeys.DOCTYPE_PUBLIC, "+//IDN python.org//DTD XML Bookmark Exchange Language 1.0//EN//XML"); tf.setOutputProperty(OutputKeys.DOCTYPE_SYSTEM, "http://www.python.org/topics/xml/dtds/xbel-1.0.dtd"); tf.setOutputProperty(OutputKeys.OMIT_XML_DECLARATION, "no");and drop the fragile regex replacements.
62-75: 🛠️ Refactor suggestion
⚠️ Potential issueStatic
Validatoris not thread-safe
javax.xml.validation.Validatorinstances are documented as not thread-safe.
With a static singleton the first concurrentadd()/remove()will corrupt internal state.- private static final Validator XML_VALIDATOR; + private static final javax.xml.validation.Schema XBEL_SCHEMA; … - XML_VALIDATOR = factory.newSchema(schemaFile).newValidator(); + XBEL_SCHEMA = factory.newSchema(schemaFile);Then create a fresh
XBEL_SCHEMA.newValidator()in every validation call.
178-191:⚠️ Potential issueSecure-processing flags are commented out – XXE/SSRF re-introduced
All hardening settings (disallow external DTDs, empty
EntityResolver, etc.) are commented.
Parsing an attacker-controlled XBEL can now fetch external resources.builderFactory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true); builderFactory.setAttribute(XMLConstants.ACCESS_EXTERNAL_DTD, ""); builderFactory.setAttribute(XMLConstants.ACCESS_EXTERNAL_SCHEMA, ""); builderFactory.setNamespaceAware(true); builder.setEntityResolver((publicId, systemId) -> new InputSource(new StringReader("")));Re-enable these lines before parsing.
🧹 Nitpick comments (3)
src/test/resources/quickaccess/dolphin/user-places-not-valid.xbel (1)
2-3: Nit: Typo in XML commentThe comment reads “simulates a invalid structure”.
Consider fixing the article for clarity.-<!-- This file simulates a invalid structure ... +<!-- This file simulates an invalid structure ...src/test/java/org/cryptomator/linux/quickaccess/DolphinPlacesTest.java (1)
136-159: Brittle root-element assertion – parse XML instead of substring compareUsing
substringon a serialised file coupled with regex whitespace stripping will break as soon as:
- Transformer pretty-printing changes,
- The encoding declaration differs,
- Attributes reorder.
Parse the file and assert on DOM properties instead:
Document doc = DocumentBuilderFactory.newInstance().newDocumentBuilder().parse(pathToDoc.toFile()); assertEquals("xbel", doc.getDocumentElement().getNodeName());src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java (1)
200-223: Minor: unnecessary double transformation pass
StringWriter+ manual regex manipulation is only needed because of the placeholder DOCTYPE logic.
Fixing DOCTYPE preservation (see above) eliminates both regex operations and the extra string allocation – small but nice win.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.idea/developer-tools.xml(1 hunks)src/main/java/module-info.java(1 hunks)src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java(4 hunks)src/main/java/org/cryptomator/linux/quickaccess/FileConfiguredQuickAccess.java(1 hunks)src/test/java/org/cryptomator/linux/quickaccess/DolphinPlacesTest.java(1 hunks)src/test/resources/quickaccess/dolphin/user-places-multiple-identical.xbel(1 hunks)src/test/resources/quickaccess/dolphin/user-places-not-valid.xbel(1 hunks)src/test/resources/quickaccess/dolphin/user-places-not-well-formed.xbel(1 hunks)src/test/resources/quickaccess/dolphin/user-places.xbel(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
src/test/resources/quickaccess/dolphin/user-places-not-well-formed.xbel (1)
Learnt from: jrauber
PR: cryptomator/integrations-linux#114
File: src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java:0-0
Timestamp: 2025-06-15T14:06:09.523Z
Learning: XBEL (XML Bookmark Exchange Language) files used by KDE Dolphin legitimately contain DOCTYPE declarations as part of the format specification. The format requires `<!DOCTYPE xbel>` for formal validation against the official XBEL DTD. Applications reading XBEL files cannot disallow DOCTYPE declarations without breaking compatibility with the standard format.
src/test/resources/quickaccess/dolphin/user-places-multiple-identical.xbel (1)
Learnt from: jrauber
PR: cryptomator/integrations-linux#114
File: src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java:0-0
Timestamp: 2025-06-15T14:06:09.523Z
Learning: XBEL (XML Bookmark Exchange Language) files used by KDE Dolphin legitimately contain DOCTYPE declarations as part of the format specification. The format requires `<!DOCTYPE xbel>` for formal validation against the official XBEL DTD. Applications reading XBEL files cannot disallow DOCTYPE declarations without breaking compatibility with the standard format.
src/test/resources/quickaccess/dolphin/user-places-not-valid.xbel (1)
Learnt from: jrauber
PR: cryptomator/integrations-linux#114
File: src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java:0-0
Timestamp: 2025-06-15T14:06:09.523Z
Learning: XBEL (XML Bookmark Exchange Language) files used by KDE Dolphin legitimately contain DOCTYPE declarations as part of the format specification. The format requires `<!DOCTYPE xbel>` for formal validation against the official XBEL DTD. Applications reading XBEL files cannot disallow DOCTYPE declarations without breaking compatibility with the standard format.
src/test/resources/quickaccess/dolphin/user-places.xbel (1)
Learnt from: jrauber
PR: cryptomator/integrations-linux#114
File: src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java:0-0
Timestamp: 2025-06-15T14:06:09.523Z
Learning: XBEL (XML Bookmark Exchange Language) files used by KDE Dolphin legitimately contain DOCTYPE declarations as part of the format specification. The format requires `<!DOCTYPE xbel>` for formal validation against the official XBEL DTD. Applications reading XBEL files cannot disallow DOCTYPE declarations without breaking compatibility with the standard format.
src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java (1)
Learnt from: jrauber
PR: cryptomator/integrations-linux#114
File: src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java:0-0
Timestamp: 2025-06-15T14:06:09.523Z
Learning: XBEL (XML Bookmark Exchange Language) files used by KDE Dolphin legitimately contain DOCTYPE declarations as part of the format specification. The format requires `<!DOCTYPE xbel>` for formal validation against the official XBEL DTD. Applications reading XBEL files cannot disallow DOCTYPE declarations without breaking compatibility with the standard format.
🔇 Additional comments (4)
src/main/java/module-info.java (1)
15-22:Details
✅ Verification successful
Dependency fix looks good – confirm no remaining java.desktop usages
java.xmlhas been added andjava.desktopremoved, which aligns with the DOM/XPath refactor.
Please run a quick scan to confirm that no classes fromjava.awt,javax.swing, etc., are still referenced elsewhere in the module.
🏁 Script executed:
#!/bin/bash # Search for any remaining AWT/Swing references rg -n "import java\\.awt" . rg -n "import javax\\.swing" .Length of output: 60
🏁 Script executed:
#!/bin/bash # Deep scan for any AWT/Swing references, including fully-qualified usages rg -n "java\.awt\." . rg -n "javax\.swing\." .Length of output: 50
Dependency fix confirmed – no remaining java.desktop usages
Performed scans for anyjava.awtorjavax.swingimports/references; none were found.src/test/resources/quickaccess/dolphin/user-places-not-well-formed.xbel (1)
1-40: No issues – resource intentionally malformed for negative testsThe file is clearly marked as “non well-formed” and matches the described test scenario.
LGTM.src/test/resources/quickaccess/dolphin/user-places-multiple-identical.xbel (1)
1-28: Test fixture looks correctContains two identical bookmarks to validate duplicate-handling logic. No concerns.
src/test/resources/quickaccess/dolphin/user-places.xbel (1)
1-40: Baseline fixture – looks fineWell-formed, valid XBEL sample for positive tests. Nothing to add.
src/main/java/org/cryptomator/linux/quickaccess/FileConfiguredQuickAccess.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java (1)
170-187: Minor: defensive XML-parser setup is good – add explicit disallow for external DTD loadYou already block external entity resolution via
ACCESS_EXTERNAL_*and a
dummy resolver. For completeness, also disable loading of non-validating
external DTDs:builderFactory.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false);Cheap extra hardening, zero behavioural change.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java(4 hunks)src/main/java/org/cryptomator/linux/quickaccess/FileConfiguredQuickAccess.java(1 hunks)src/test/java/org/cryptomator/linux/quickaccess/DolphinPlacesTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/org/cryptomator/linux/quickaccess/FileConfiguredQuickAccess.java
🧰 Additional context used
🧠 Learnings (1)
src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java (1)
Learnt from: jrauber
PR: cryptomator/integrations-linux#114
File: src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java:0-0
Timestamp: 2025-06-15T14:06:09.538Z
Learning: XBEL (XML Bookmark Exchange Language) files used by KDE Dolphin legitimately contain DOCTYPE declarations as part of the format specification. The format requires `<!DOCTYPE xbel>` for formal validation against the official XBEL DTD. Applications reading XBEL files cannot disallow DOCTYPE declarations without breaking compatibility with the standard format.
🔇 Additional comments (1)
src/test/java/org/cryptomator/linux/quickaccess/DolphinPlacesTest.java (1)
44-50: Nice clean-up after assertion – good practiceCalling
entry.remove()inside the sameassertDoesNotThrowblock keeps the
test self-contained and leaves the temp file in its original state for any
subsequent assertions. 👏
src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java
Outdated
Show resolved
Hide resolved
src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java
Outdated
Show resolved
Hide resolved
src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java (3)
96-108: Updated XML is never validated – still validating the original string
This exact issue was raised previously and remains unfixed.After mutating the DOM you again call
validator.validate(new StreamSource(new StringReader(config)));
so any schema-breaking changes inxmlDocumentslip through.Use the serialised updated XML:
- validator.validate(new StreamSource(new StringReader(config))); + validator.validate(new StreamSource(new StringReader(documentToString(xmlDocument))));
200-215: Fragile DOCTYPE handling & duplicate property assignment persist
The transformer still
- sets
OMIT_XML_DECLARATIONtwice, and- writes an empty public/system id only to patch it back with regex.
The earlier review already provided a diff restoring the canonical XBEL identifiers and removing the regex surgery. Please apply it to avoid broken DOCTYPEs and redundant code.
295-309: removeEntryFromConfig validates the wrong document as wellSame validation lapse as in
addEntryToConfig: the second validation targets the pre-mutationconfigstring instead of the modified DOM. Replace with validation ofdocumentToString(xmlDocument).
🧹 Nitpick comments (1)
src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java (1)
81-83: Constructor treats supplied path as a directory, not a full file path
configFilePath.resolve(CONFIG_FILE_NAME)appendsuser-places.xbelunconditionally.
Callers that already pass the exact file (/tmp/foo.xbel) will silently end up with/tmp/foo.xbel/user-places.xbel, which is almost certainly unintended.Consider accepting either a directory or the final file, e.g.:
-public DolphinPlaces(Path configFilePath) { - super(configFilePath.resolve(CONFIG_FILE_NAME), MAX_FILE_SIZE); -} +public DolphinPlaces(Path configPath) { + var file = Files.isDirectory(configPath) + ? configPath.resolve(CONFIG_FILE_NAME) + : configPath; + super(file, MAX_FILE_SIZE); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java (1)
Learnt from: jrauber
PR: cryptomator/integrations-linux#114
File: src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java:0-0
Timestamp: 2025-06-15T14:06:09.538Z
Learning: XBEL (XML Bookmark Exchange Language) files used by KDE Dolphin legitimately contain DOCTYPE declarations as part of the format specification. The format requires `<!DOCTYPE xbel>` for formal validation against the official XBEL DTD. Applications reading XBEL files cannot disallow DOCTYPE declarations without breaking compatibility with the standard format.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java (1)
202-217: Preserve canonical DOCTYPE – stop writing empty identifiers and post-editing the outputSame issue as highlighted in the earlier review: the transformer is instructed to emit
<!DOCTYPE xbel PUBLIC "" "">and this is patched afterwards with a brittle regex to become
<!DOCTYPE xbel>.
The official XBEL identifiers are still lost and external validators/Dolphin itself may reject the file.
Emit the correct identifiers directly and drop the regex surgery.Transformer transformer = TransformerFactory.newInstance().newTransformer(); -transformer.setOutputProperty(OutputKeys.DOCTYPE_PUBLIC, ""); -transformer.setOutputProperty(OutputKeys.DOCTYPE_SYSTEM, ""); +transformer.setOutputProperty(OutputKeys.DOCTYPE_PUBLIC, + "+//IDN python.org//DTD XML Bookmark Exchange Language 1.0//EN//XML"); +transformer.setOutputProperty(OutputKeys.DOCTYPE_SYSTEM, + "http://www.python.org/topics/xml/dtds/xbel-1.0.dtd"); ... -content = content.replaceFirst("<!DOCTYPE xbel PUBLIC \"\" \"\">","<!DOCTYPE xbel>"); +// no post-processing required – transformer already wrote the canonical declaration
🧹 Nitpick comments (3)
src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java (3)
205-209: DuplicateOMIT_XML_DECLARATIONassignment
OMIT_XML_DECLARATIONis set twice. The first setting at line 205 is immediately overwritten at line 209, adding noise without effect.-transformer.setOutputProperty(OutputKeys.OMIT_XML_DECLARATION, "no"); ... -transformer.setOutputProperty(OutputKeys.OMIT_XML_DECLARATION, "no");Keep a single assignment.
215-216: Avoid regex cleansing – configure the transformer insteadManually deleting the
standaloneattribute and fixing the DOCTYPE with regexes is fragile.
Both can be produced correctly by the transformer:-transformer.setOutputProperty(OutputKeys.OMIT_XML_DECLARATION, "no"); +transformer.setOutputProperty(OutputKeys.STANDALONE, "no"); ... - content = content.replaceFirst("\\s*standalone=\"(yes|no)\"", ""); - content = content.replaceFirst("<!DOCTYPE xbel PUBLIC \"\" \"\">","<!DOCTYPE xbel>"); + // regex post-processing no longer requiredThis removes two regexp passes and guarantees well-formed output.
175-183: Tighten parser – disable loading of external DTDs entirelyWhile
ACCESS_EXTERNAL_DTDis cleared, the feature
http://apache.org/xml/features/nonvalidating/load-external-dtdis still at its default true, meaning a malicious file could trigger a network fetch with a validating parser. Disabling it adds an extra safeguard without affecting normal operation because validation isn’t performed here.builderFactory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true); +builderFactory.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java (1)
Learnt from: jrauber
PR: cryptomator/integrations-linux#114
File: src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java:0-0
Timestamp: 2025-06-15T14:06:09.538Z
Learning: XBEL (XML Bookmark Exchange Language) files used by KDE Dolphin legitimately contain DOCTYPE declarations as part of the format specification. The format requires `<!DOCTYPE xbel>` for formal validation against the official XBEL DTD. Applications reading XBEL files cannot disallow DOCTYPE declarations without breaking compatibility with the standard format.
|
@infeo Not sure if this is good enough, could you please review the PR? |
overheadhunter
left a comment
overheadhunter
left a comment
There was a problem hiding this comment.
Thanks for the PR, I support the idea of using XPath.
Just a formatting issue: can you please get rid of blank lines inside method bodies unless they explicitly group lines to improve readability?
- removed line-breaks - increased usage of 'var'
@overheadhunter Sure, adjusted the code to the existing style. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java (1)
157-170: Fragile DOCTYPE post-processing
- Setting
DOCTYPE_PUBLIC/_SYSTEMto empty strings relies on the
Transformer emittingPUBLIC "" "", which is implementation-specific.- The subsequent
replaceFirst("<!DOCTYPE xbel PUBLIC \"\" \"\">", …)will
fail if the Transformer adds whitespace or a line break.A safer, implementation-agnostic approach:
- transformer.setOutputProperty(OutputKeys.DOCTYPE_PUBLIC, ""); - transformer.setOutputProperty(OutputKeys.DOCTYPE_SYSTEM, ""); + // Let the original DocumentType pass through unchanged.and replace the brittle regex with a tolerant one:
- content = content.replaceFirst("<!DOCTYPE xbel PUBLIC \"\" \"\">","<!DOCTYPE xbel>"); + content = content.replaceFirst("<!DOCTYPE\\s+xbel[^>]*>","<!DOCTYPE xbel>");This keeps the shortened DOCTYPE while avoiding dependency on a specific JDK
serialisation.
🧹 Nitpick comments (2)
src/test/java/org/cryptomator/linux/quickaccess/DolphinPlacesTest.java (1)
134-142: Specify charset inFiles.readStringto avoid platform-dependent behaviour
Files.readString(Path)uses the JVM default charset, which may differ between environments and could break the assertions when the file contains non-ASCII characters. PassStandardCharsets.UTF_8explicitly – your test resources are UTF-8.- return Files.readString(file); + return Files.readString(file, java.nio.charset.StandardCharsets.UTF_8);src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java (1)
111-123: XPath equality on the raw URI is brittleThe expression
[@href=$uri]requires an exact string match. Minor
differences—e.g. presence/absence of a trailing slash, URL-encoded spaces,
case sensitivity on case-insensitive filesystems—will cause duplicates to
slip through.Consider normalising before comparison or switching to a more tolerant XPath,
e.g.starts-with(@href, $uri)or comparing onjava.nio.file.Path#toRealPath
normalised strings prior to insertion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java(4 hunks)src/main/java/org/cryptomator/linux/quickaccess/FileConfiguredQuickAccess.java(1 hunks)src/test/java/org/cryptomator/linux/quickaccess/DolphinPlacesTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/org/cryptomator/linux/quickaccess/FileConfiguredQuickAccess.java
🧰 Additional context used
🧠 Learnings (1)
src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java (2)
Learnt from: jrauber
PR: cryptomator/integrations-linux#114
File: src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java:0-0
Timestamp: 2025-06-16T18:40:46.632Z
Learning: KDE Dolphin generates XBEL files with a shortened DOCTYPE declaration format `<!DOCTYPE xbel>` rather than the canonical XBEL format with public and system identifiers. For compatibility with Dolphin, XBEL files should use this shortened format when integrating with KDE Dolphin's bookmark system.
Learnt from: jrauber
PR: cryptomator/integrations-linux#114
File: src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java:0-0
Timestamp: 2025-06-15T14:06:09.538Z
Learning: XBEL (XML Bookmark Exchange Language) files used by KDE Dolphin legitimately contain DOCTYPE declarations as part of the format specification. The format requires `<!DOCTYPE xbel>` for formal validation against the official XBEL DTD. Applications reading XBEL files cannot disallow DOCTYPE declarations without breaking compatibility with the standard format.
overheadhunter
left a comment
overheadhunter
left a comment
There was a problem hiding this comment.
looks promising, just some minor remarks. sorry that it took me so long to review it again
src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java
Outdated
Show resolved
Hide resolved
src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java
Outdated
Show resolved
Hide resolved
src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java
Outdated
Show resolved
Hide resolved
src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java
Outdated
Show resolved
Hide resolved
src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java
Outdated
Show resolved
Hide resolved
src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java
Outdated
Show resolved
Hide resolved
src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java
Outdated
Show resolved
Hide resolved
src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java (1)
76-78: Constructor parameter name should reflect its purpose.The parameter
configFilePathis misleading because it's used as a directory path in the parent constructor, not a file path. This could lead to confusion and incorrect usage.- public DolphinPlaces(Path configFilePath) { - super(configFilePath, MAX_FILE_SIZE); + public DolphinPlaces(Path configDir) { + super(configDir, MAX_FILE_SIZE);Or alternatively, if you want to accept the full file path:
- public DolphinPlaces(Path configFilePath) { - super(configFilePath, MAX_FILE_SIZE); + public DolphinPlaces(Path configFilePath) { + super(configFilePath.getParent(), MAX_FILE_SIZE);
🧹 Nitpick comments (1)
src/test/java/org/cryptomator/linux/quickaccess/DolphinPlacesTest.java (1)
3-3: Missing import organization.The import
org.cryptomator.integrations.quickaccess.QuickAccessServiceExceptionwas added but the imports aren't properly organized or optimized.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java(4 hunks)src/test/java/org/cryptomator/linux/quickaccess/DolphinPlacesTest.java(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: jrauber
PR: cryptomator/integrations-linux#114
File: src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java:0-0
Timestamp: 2025-06-16T18:40:46.643Z
Learning: KDE Dolphin generates XBEL files with a shortened DOCTYPE declaration format `<!DOCTYPE xbel>` rather than the canonical XBEL format with public and system identifiers. For compatibility with Dolphin, XBEL files should use this shortened format when integrating with KDE Dolphin's bookmark system.
Learnt from: infeo
PR: cryptomator/integrations-linux#102
File: src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java:94-96
Timestamp: 2025-01-14T09:23:48.520Z
Learning: In DolphinPlaces.java, the entry identification uses UUID stored in cryptomator.org metadata, which provides sufficient uniqueness without requiring XML parsing for lookups.
Learnt from: jrauber
PR: cryptomator/integrations-linux#114
File: src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java:0-0
Timestamp: 2025-06-15T14:06:09.538Z
Learning: XBEL (XML Bookmark Exchange Language) files used by KDE Dolphin legitimately contain DOCTYPE declarations as part of the format specification. The format requires `<!DOCTYPE xbel>` for formal validation against the official XBEL DTD. Applications reading XBEL files cannot disallow DOCTYPE declarations without breaking compatibility with the standard format.
src/test/java/org/cryptomator/linux/quickaccess/DolphinPlacesTest.java (3)
Learnt from: infeo
PR: cryptomator/integrations-linux#102
File: src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java:94-96
Timestamp: 2025-01-14T09:23:48.520Z
Learning: In DolphinPlaces.java, the entry identification uses UUID stored in cryptomator.org metadata, which provides sufficient uniqueness without requiring XML parsing for lookups.
Learnt from: jrauber
PR: cryptomator/integrations-linux#114
File: src/test/java/org/cryptomator/linux/quickaccess/DolphinPlacesTest.java:162-177
Timestamp: 2025-06-16T18:20:27.440Z
Learning: In JUnit tests, the @TempDir annotation provides each test method with its own isolated temporary directory, so test helper methods that write to the same filename within these directories don't cause race conditions or overwrites during parallel test execution.
Learnt from: infeo
PR: cryptomator/integrations-linux#77
File: src/main/java/org/cryptomator/linux/quickaccess/NautilusBookmarks.java:31-50
Timestamp: 2024-07-12T20:09:01.834Z
Learning: The `add` method in `NautilusBookmarks` handles the case where the `BOOKMARKS_FILE` does not exist by catching the IOException and wrapping it in a `QuickAccessServiceException`.
src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java (6)
Learnt from: infeo
PR: cryptomator/integrations-linux#102
File: src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java:94-96
Timestamp: 2025-01-14T09:23:48.520Z
Learning: In DolphinPlaces.java, the entry identification uses UUID stored in cryptomator.org metadata, which provides sufficient uniqueness without requiring XML parsing for lookups.
Learnt from: jrauber
PR: cryptomator/integrations-linux#114
File: src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java:0-0
Timestamp: 2025-06-16T18:40:46.643Z
Learning: KDE Dolphin generates XBEL files with a shortened DOCTYPE declaration format `<!DOCTYPE xbel>` rather than the canonical XBEL format with public and system identifiers. For compatibility with Dolphin, XBEL files should use this shortened format when integrating with KDE Dolphin's bookmark system.
Learnt from: infeo
PR: cryptomator/integrations-linux#77
File: src/main/java/org/cryptomator/linux/quickaccess/NautilusBookmarks.java:31-50
Timestamp: 2024-07-12T20:09:01.834Z
Learning: The `add` method in `NautilusBookmarks` handles the case where the `BOOKMARKS_FILE` does not exist by catching the IOException and wrapping it in a `QuickAccessServiceException`.
Learnt from: jrauber
PR: cryptomator/integrations-linux#114
File: src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java:0-0
Timestamp: 2025-06-15T14:06:09.538Z
Learning: XBEL (XML Bookmark Exchange Language) files used by KDE Dolphin legitimately contain DOCTYPE declarations as part of the format specification. The format requires `<!DOCTYPE xbel>` for formal validation against the official XBEL DTD. Applications reading XBEL files cannot disallow DOCTYPE declarations without breaking compatibility with the standard format.
Learnt from: infeo
PR: cryptomator/integrations-linux#77
File: src/main/java/org/cryptomator/linux/quickaccess/NautilusBookmarks.java:31-50
Timestamp: 2024-07-12T20:09:01.834Z
Learning: In `NautilusBookmarks`, file operations are handled by the JDK, which manages the closing of any opened resources, so try-with-resources is not necessary.
Learnt from: infeo
PR: cryptomator/integrations-linux#80
File: src/main/java/org/cryptomator/linux/autostart/FreedesktopAutoStartService.java:56-62
Timestamp: 2024-07-16T22:36:32.769Z
Learning: For the `FreedesktopAutoStartService` in the Cryptomator project, exceptions are preferred to contain all necessary debugging information without additional logging before throwing them.
🔇 Additional comments (22)
src/test/java/org/cryptomator/linux/quickaccess/DolphinPlacesTest.java (11)
17-25: Well-structured test constants for comprehensive coverage.The constant definitions provide good coverage for testing different scenarios including UUIDs, captions, paths, and various XML resource files representing different states (valid, invalid, malformed).
29-31: Improved test structure with lambda expression.The change to use
assertDoesNotThrowwith a lambda expression is a good improvement over the previous implementation, making the test more explicit about what should not throw.
33-46: Comprehensive test for replacement behavior.This test effectively verifies that adding an identical entry replaces the existing one by checking UUID and caption presence before and after the operation. The test properly uses temporary directories and validates the DOM-based replacement logic.
48-63: Thorough test for multiple identical entries replacement.This test covers the important edge case where multiple identical bookmarks exist and verifies that all are properly replaced with a single new entry. The use of
countOccurrencesprovides precise verification of the replacement behavior.
65-76: Proper error handling test for invalid XML.This test correctly verifies that invalid XML files are not modified and that the appropriate exception is thrown, ensuring robustness of the XML validation logic.
78-89: Good test coverage for malformed XML handling.This test ensures that malformed XML files are handled gracefully without modification, which is crucial for maintaining file integrity when XML parsing fails.
91-100: Essential test for XML character escaping.This test verifies that invalid XML characters are properly escaped when using DOM-based serialization, which is important for preventing XML parsing errors in the target application.
102-119: Critical test for XML root preservation.This test addresses the concern mentioned in the PR comments about ensuring the XML root and DOCTYPE declaration are not accidentally modified. The test validates that the DOM manipulation preserves the document structure integrity.
122-132: Helper method implementation is correct.The
loadResourceToDirmethod correctly loads test resources into temporary directories. Based on the retrieved learnings, the use of the same filename across tests is not an issue because@TempDirprovides isolated directories for each test.
134-143: Solid file loading helper method.The
loadFilemethod provides proper error handling and resource management for reading test files, with appropriate exception wrapping.
145-154: Efficient string occurrence counting utility.The
countOccurrencesmethod provides an efficient way to count substring occurrences, which is useful for validating XML content changes in the tests.src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java (11)
9-15: Appropriate imports for DOM-based XML processing.The new imports support the transition from string-based to DOM-based XML manipulation, including proper logging, DOM processing, and XPath support.
53-55: Good logging and namespace constant additions.The logger and XBEL namespace constant improve maintainability and provide proper debugging capabilities for the XML processing logic.
81-99: Well-structured DOM-based bookmark addition.The refactored
addEntryToConfigmethod properly uses DOM parsing, XPath queries, and XML validation. The error handling is comprehensive and the logging provides good traceability.
101-106: Correct node removal implementation.The
removeStaleBookmarksmethod properly handles the live NodeList by iterating backwards, avoiding the skip-every-second-node issue that would occur with forward iteration.
108-120: Secure XPath implementation with variable resolution.The XPath implementation properly uses variable resolution to avoid injection attacks. The exception handling is comprehensive and provides clear error messages.
122-134: Consistent XPath pattern for ID-based extraction.The
extractBookmarksByIdmethod follows the same secure pattern asextractBookmarksByPath, using variable resolution and proper exception handling.
136-152: Proper XML security configuration.The XML document loading includes appropriate security measures to prevent XXE attacks while preserving the ability to parse XBEL files with DOCTYPE declarations, which is essential for compatibility with KDE Dolphin.
154-171: DOCTYPE handling preserves Dolphin compatibility.The
documentToStringmethod correctly handles the DOCTYPE declaration by converting the canonical format to the shortened format that KDE Dolphin expects (<!DOCTYPE xbel>), ensuring compatibility with the target application.
173-222: Comprehensive bookmark creation with proper structure.The
createBookmarkmethod creates the complete XBEL bookmark structure with proper namespaces, metadata, and attributes. The DOM-based approach ensures proper XML escaping and structure validation.
233-246: Consistent removal implementation.The
removeEntryFromConfigmethod in the inner class follows the same pattern as the addition logic, using DOM parsing and proper validation to ensure XML integrity.
248-274: Clean XPath variable resolver implementation.The
SimpleVariableResolverinner class provides a clean, thread-safe implementation for XPath variable resolution, supporting the secure XPath expressions used throughout the class.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java (1)
88-88: Consider using debug level for production logging.The trace-level logging for bookmark operations might be too verbose for production environments. Consider using debug level for this operational information.
- LOG.trace("Adding bookmark for target: '{}', displayName: '{}', id: '{}'", target, displayName, id); + LOG.debug("Adding bookmark for target: '{}', displayName: '{}', id: '{}'", target, displayName, id);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: jrauber
PR: cryptomator/integrations-linux#114
File: src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java:0-0
Timestamp: 2025-06-16T18:40:46.643Z
Learning: KDE Dolphin generates XBEL files with a shortened DOCTYPE declaration format `<!DOCTYPE xbel>` rather than the canonical XBEL format with public and system identifiers. For compatibility with Dolphin, XBEL files should use this shortened format when integrating with KDE Dolphin's bookmark system.
Learnt from: infeo
PR: cryptomator/integrations-linux#102
File: src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java:94-96
Timestamp: 2025-01-14T09:23:48.520Z
Learning: In DolphinPlaces.java, the entry identification uses UUID stored in cryptomator.org metadata, which provides sufficient uniqueness without requiring XML parsing for lookups.
Learnt from: jrauber
PR: cryptomator/integrations-linux#114
File: src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java:0-0
Timestamp: 2025-06-15T14:06:09.538Z
Learning: XBEL (XML Bookmark Exchange Language) files used by KDE Dolphin legitimately contain DOCTYPE declarations as part of the format specification. The format requires `<!DOCTYPE xbel>` for formal validation against the official XBEL DTD. Applications reading XBEL files cannot disallow DOCTYPE declarations without breaking compatibility with the standard format.
src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java (6)
Learnt from: infeo
PR: cryptomator/integrations-linux#102
File: src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java:94-96
Timestamp: 2025-01-14T09:23:48.520Z
Learning: In DolphinPlaces.java, the entry identification uses UUID stored in cryptomator.org metadata, which provides sufficient uniqueness without requiring XML parsing for lookups.
Learnt from: jrauber
PR: cryptomator/integrations-linux#114
File: src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java:0-0
Timestamp: 2025-06-16T18:40:46.643Z
Learning: KDE Dolphin generates XBEL files with a shortened DOCTYPE declaration format `<!DOCTYPE xbel>` rather than the canonical XBEL format with public and system identifiers. For compatibility with Dolphin, XBEL files should use this shortened format when integrating with KDE Dolphin's bookmark system.
Learnt from: infeo
PR: cryptomator/integrations-linux#77
File: src/main/java/org/cryptomator/linux/quickaccess/NautilusBookmarks.java:31-50
Timestamp: 2024-07-12T20:09:01.834Z
Learning: The `add` method in `NautilusBookmarks` handles the case where the `BOOKMARKS_FILE` does not exist by catching the IOException and wrapping it in a `QuickAccessServiceException`.
Learnt from: jrauber
PR: cryptomator/integrations-linux#114
File: src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java:0-0
Timestamp: 2025-06-15T14:06:09.538Z
Learning: XBEL (XML Bookmark Exchange Language) files used by KDE Dolphin legitimately contain DOCTYPE declarations as part of the format specification. The format requires `<!DOCTYPE xbel>` for formal validation against the official XBEL DTD. Applications reading XBEL files cannot disallow DOCTYPE declarations without breaking compatibility with the standard format.
Learnt from: infeo
PR: cryptomator/integrations-linux#77
File: src/main/java/org/cryptomator/linux/quickaccess/NautilusBookmarks.java:31-50
Timestamp: 2024-07-12T20:09:01.834Z
Learning: In `NautilusBookmarks`, file operations are handled by the JDK, which manages the closing of any opened resources, so try-with-resources is not necessary.
Learnt from: infeo
PR: cryptomator/integrations-linux#80
File: src/main/java/org/cryptomator/linux/autostart/FreedesktopAutoStartService.java:56-62
Timestamp: 2024-07-16T22:36:32.769Z
Learning: For the `FreedesktopAutoStartService` in the Cryptomator project, exceptions are preferred to contain all necessary debugging information without additional logging before throwing them.
🔇 Additional comments (6)
src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java (6)
115-117: XPath variable resolution properly prevents injection attacks.The implementation correctly uses XPath variable resolution instead of string concatenation, effectively preventing XPath injection vulnerabilities. This is a significant security improvement over the previous string-based approach.
141-151: XML security configuration is comprehensive and appropriate.The XML parser configuration properly prevents XXE attacks while maintaining compatibility with XBEL format requirements. The security features are well-balanced:
- Prevents external entity resolution
- Blocks external DTD/schema access
- Allows required DOCTYPE declarations for XBEL compatibility
- Custom EntityResolver returns empty content for safety
161-169: DOCTYPE handling maintains Dolphin compatibility.The DOCTYPE post-processing correctly transforms the XML output to match KDE Dolphin's expected format (
<!DOCTYPE xbel>) while preserving the XML structure. This approach maintains compatibility with Dolphin's specific XBEL implementation.
200-224: Comprehensive bookmark creation with proper DOM manipulation.The bookmark creation method correctly constructs the complete XBEL bookmark structure including:
- Proper namespace handling for the icon element
- Correct metadata organization with owner attributes
- Appropriate exception handling for DOM operations
238-244: Entry removal follows the same robust pattern as addition.The removal logic correctly mirrors the addition pattern with validation before and after DOM manipulation, ensuring consistent XML integrity throughout the bookmark management process.
254-276: XPath variable resolver implementation is clean and functional.The
SimpleVariableResolverclass provides a straightforward implementation for XPath parameterization. The HashMap-based storage and simple interface make it easy to use and understand.
src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java
Outdated
Show resolved
Hide resolved
infeo
left a comment
infeo
left a comment
There was a problem hiding this comment.
Looks good, but i have some open questions.
src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java
Outdated
Show resolved
Hide resolved
src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java
Outdated
Show resolved
Hide resolved
…d instead providing a lambda into the xpath - Removed the unnecessary validation before deleting
|
@jrauber Can you merge the current develop branch into yours? i updated the workflows to also execute the maven test phase. |
@infeo Did merge it, the github action awaits confirmation by the maintainer. Checked on my branch, the build is now running: |
4 participants
This commit refactors the Dolphin quick access integration to use DOM manipulation for adding and removing bookmarks in the XBEL file. This change aims to improve the reliability and correctness of bookmark management by handling stale entries.
Changes
Impact
Fixes: cryptomator/cryptomator#3883