Merged
Conversation
WalkthroughAdds HUB host validation and trust flows to the vault unlock process: new public handlers onHubCheckHostsAllowed and allowedHubHosts in UnlockVaultPresenter, multiple private helpers for config consistency, host/HTTP detection and authority extraction, and logic to record trusted hosts. Introduces HubCheckHostAuthenticityDialog and activity callbacks in UnlockVaultActivity, a settings preference to clear trusted hosts with backing SharedPreferencesHandler methods (add/get/clear), new layout and string resources for the dialog and messages, and a minor formatting tweak. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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.
Actionable comments posted: 2
🧹 Nitpick comments (1)
presentation/src/main/java/org/cryptomator/presentation/ui/dialog/HubCheckHostAuthenticityDialog.kt (1)
48-53: NarrownewInstance()toUnverifiedHubVaultConfig.
setupDialog()immediately casts this argument back toUnverifiedHubVaultConfig, so the wider type just turns misuse into a runtime cast failure.♻️ Suggested change
-import org.cryptomator.domain.UnverifiedVaultConfig @@ - fun newInstance(hostnames: Array<String>, unverifiedVaultConfig: UnverifiedVaultConfig, vault: Vault): HubCheckHostAuthenticityDialog { + fun newInstance(hostnames: Array<String>, unverifiedHubVaultConfig: UnverifiedHubVaultConfig, vault: Vault): HubCheckHostAuthenticityDialog { val dialog = HubCheckHostAuthenticityDialog() val args = Bundle() args.putSerializable(HOSTNAMES_ARG, hostnames) - args.putSerializable(UNVERIFIED_VAULT_CONFIG_ARG, unverifiedVaultConfig) + args.putSerializable(UNVERIFIED_VAULT_CONFIG_ARG, unverifiedHubVaultConfig) args.putSerializable(VAULT_ARG, vault) dialog.arguments = args return dialog }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@presentation/src/main/java/org/cryptomator/presentation/ui/dialog/HubCheckHostAuthenticityDialog.kt` around lines 48 - 53, The factory method newInstance currently accepts a broad UnverifiedVaultConfig but setupDialog always casts it to UnverifiedHubVaultConfig, causing potential runtime cast failures; change the newInstance signature to accept UnverifiedHubVaultConfig (instead of UnverifiedVaultConfig), update the args.putSerializable call to pass that typed value, and update any callers of HubCheckHostAuthenticityDialog.newInstance(...) to supply an UnverifiedHubVaultConfig so the runtime cast in setupDialog no longer fails (refer to the newInstance function and the UNVERIFIED_VAULT_CONFIG_ARG/HubCheckHostAuthenticityDialog class).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@presentation/src/main/java/org/cryptomator/presentation/presenter/UnlockVaultPresenter.kt`:
- Around line 167-177: The branch ordering makes trusted HTTPS hosts hit the
dialog because configContainsAllowedHosts(...) can be true while isHttpHost(...)
is false; update the conditional logic in UnlockVaultPresenter (around the calls
to allowedHubHosts, configContainsAllowedHosts, isHttpHost, and
isCryptomatorCloud) so that if
configContainsAllowedHosts(unverifiedHubVaultConfig) is true you immediately
call allowedHubHosts(unverifiedHubVaultConfig, vault) (i.e., remove the
isHttpHost() requirement), keeping the existing Cryptomator Cloud/http error
branch and the HubCheckHostAuthenticityDialog.newInstance fallback otherwise.
In `@presentation/src/main/res/values/strings.xml`:
- Around line 337-340: Update the user-facing strings so they reflect the actual
flow: change the value of the string resource named
"screen_hub_clear_unknown_hub_hosts_title" from "Clear Unknown Hub Hosts" to
wording that clearly indicates it clears previously approved hosts (e.g., "Clear
Approved Hub Hosts" or "Clear Previously Approved Hosts"), and locate any string
entries whose text is "Trust this hosts?" (and duplicates mentioned elsewhere)
and correct the grammar to "Trust these hosts?". Edit the corresponding string
resource values in strings.xml for those identifiers to ensure consistency
across the file.
---
Nitpick comments:
In
`@presentation/src/main/java/org/cryptomator/presentation/ui/dialog/HubCheckHostAuthenticityDialog.kt`:
- Around line 48-53: The factory method newInstance currently accepts a broad
UnverifiedVaultConfig but setupDialog always casts it to
UnverifiedHubVaultConfig, causing potential runtime cast failures; change the
newInstance signature to accept UnverifiedHubVaultConfig (instead of
UnverifiedVaultConfig), update the args.putSerializable call to pass that typed
value, and update any callers of HubCheckHostAuthenticityDialog.newInstance(...)
to supply an UnverifiedHubVaultConfig so the runtime cast in setupDialog no
longer fails (refer to the newInstance function and the
UNVERIFIED_VAULT_CONFIG_ARG/HubCheckHostAuthenticityDialog class).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ed413e42-ef77-4b38-a98f-7e3121d56df9
📒 Files selected for processing (8)
presentation/src/main/java/org/cryptomator/presentation/presenter/UnlockVaultPresenter.ktpresentation/src/main/java/org/cryptomator/presentation/ui/activity/UnlockVaultActivity.ktpresentation/src/main/java/org/cryptomator/presentation/ui/dialog/HubCheckHostAuthenticityDialog.ktpresentation/src/main/java/org/cryptomator/presentation/ui/fragment/SettingsFragment.ktpresentation/src/main/res/layout/dialog_hub_check_host_authenticity.xmlpresentation/src/main/res/values/strings.xmlpresentation/src/main/res/xml/preferences.xmlutil/src/main/java/org/cryptomator/util/SharedPreferencesHandler.kt
presentation/src/main/java/org/cryptomator/presentation/presenter/UnlockVaultPresenter.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
presentation/src/main/java/org/cryptomator/presentation/presenter/UnlockVaultPresenter.kt (2)
226-233: Consider null safety foruri.schemeanduri.host.Both
URI.schemeandURI.hostcan returnnullfor malformed URIs, which would produce authority strings like"null://null". While such configs should fail consistency checks earlier, adding defensive handling would prevent unexpected behavior.♻️ Suggested defensive implementation
private fun getAuthority(uri: URI): String { + val scheme = uri.scheme ?: return "" + val host = uri.host ?: return "" return when (uri.port) { - -1 -> "%s://%s".format(uri.scheme, uri.host) - 80 -> "http://%s".format(uri.host) - 443 -> "https://%s".format(uri.host) - else -> "%s://%s:%s".format(uri.scheme, uri.host, uri.port) + -1 -> "%s://%s".format(scheme, host) + 80 -> "http://%s".format(host) + 443 -> "https://%s".format(host) + else -> "%s://%s:%s".format(scheme, host, uri.port) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@presentation/src/main/java/org/cryptomator/presentation/presenter/UnlockVaultPresenter.kt` around lines 226 - 233, The getAuthority function can produce "null" parts when URI.scheme or URI.host are null; update getAuthority to defensively handle nulls by using safe defaults or throwing a clear IllegalArgumentException: check uri.scheme and uri.host at the start of getAuthority (or inside each when branch) and either substitute a safe value (e.g., empty string or "unknown") or raise an error with context including the URI; keep decision consistent across branches that reference uri.scheme, uri.host, and uri.port so authority strings are never constructed with literal "null" values.
218-218: Remove trailing semicolon.Semicolons are not idiomatic in Kotlin.
✏️ Fix
- return allowedHubHosts.contains(canonicalHubHost) && allowedHubHosts.contains(canonicalAuthHost); + return allowedHubHosts.contains(canonicalHubHost) && allowedHubHosts.contains(canonicalAuthHost)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@presentation/src/main/java/org/cryptomator/presentation/presenter/UnlockVaultPresenter.kt` at line 218, The return statement in UnlockVaultPresenter (the line returning allowedHubHosts.contains(canonicalHubHost) && allowedHubHosts.contains(canonicalAuthHost)) has an unnecessary trailing semicolon; remove the semicolon so the Kotlin idiomatic statement is just: return allowedHubHosts.contains(canonicalHubHost) && allowedHubHosts.contains(canonicalAuthHost) to fix the style issue.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@presentation/src/main/java/org/cryptomator/presentation/presenter/UnlockVaultPresenter.kt`:
- Around line 205-208: The isCryptomatorCloud check can NPE because URI.host may
be null; update the method (isCryptomatorCloud in UnlockVaultPresenter) to
null-safe-check the hosts from UnverifiedHubVaultConfig.apiBaseUrl.host and
.authEndpoint.host before calling endsWith(trustedCryptomatorCloudDomain) (e.g.,
guard each host with an explicit null check or use a null-safe endsWith check
and only return true if both hosts are non-null and end with
trustedCryptomatorCloudDomain).
- Around line 164-179: The Timber logging tag is inconsistent: replace the
incorrect "VaultListPresenter" tag usages inside UnlockVaultPresenter with
"UnlockVaultPresenter" so all Timber.tag(...) calls in this class (the calls
that currently read Timber.tag("VaultListPresenter").e(...)) consistently use
Timber.tag("UnlockVaultPresenter").e(...), ensuring the log statements in the
hub config handling block and any related error branches match the class name
used elsewhere in UnlockVaultPresenter.
---
Nitpick comments:
In
`@presentation/src/main/java/org/cryptomator/presentation/presenter/UnlockVaultPresenter.kt`:
- Around line 226-233: The getAuthority function can produce "null" parts when
URI.scheme or URI.host are null; update getAuthority to defensively handle nulls
by using safe defaults or throwing a clear IllegalArgumentException: check
uri.scheme and uri.host at the start of getAuthority (or inside each when
branch) and either substitute a safe value (e.g., empty string or "unknown") or
raise an error with context including the URI; keep decision consistent across
branches that reference uri.scheme, uri.host, and uri.port so authority strings
are never constructed with literal "null" values.
- Line 218: The return statement in UnlockVaultPresenter (the line returning
allowedHubHosts.contains(canonicalHubHost) &&
allowedHubHosts.contains(canonicalAuthHost)) has an unnecessary trailing
semicolon; remove the semicolon so the Kotlin idiomatic statement is just:
return allowedHubHosts.contains(canonicalHubHost) &&
allowedHubHosts.contains(canonicalAuthHost) to fix the style issue.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 701843d1-beb4-450a-a9d9-16c252287ba4
📒 Files selected for processing (1)
presentation/src/main/java/org/cryptomator/presentation/presenter/UnlockVaultPresenter.kt
presentation/src/main/java/org/cryptomator/presentation/presenter/UnlockVaultPresenter.kt
Outdated
Show resolved
Hide resolved
presentation/src/main/java/org/cryptomator/presentation/presenter/UnlockVaultPresenter.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
presentation/src/main/res/values/strings.xml (1)
254-254:⚠️ Potential issue | 🟡 MinorCopy still doesn't match the trust flow.
This setting clears previously trusted hosts, so “Reset Unknown Hub Hosts” reads backwards. The dialog title also still has the grammar issue “Trust this hosts?”. Please rename them to reflect trusted hosts / “Trust these hosts?”.
Also applies to: 564-565
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@presentation/src/main/res/values/strings.xml` at line 254, The string "screen_hub_reset_unknown_hub_hosts_title" and the dialog copy that reads "Trust this hosts?" are incorrect for the trust flow; change the title to reflect that this clears previously trusted hosts (e.g., "Reset Trusted Hub Hosts" or "Reset Trusted Hosts") and fix the confirmation grammar by replacing "Trust this hosts?" with "Trust these hosts?" — update the resource value for screen_hub_reset_unknown_hub_hosts_title and the related confirmation string resource (search for the resource containing "Trust this hosts?") accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@presentation/src/main/java/org/cryptomator/presentation/presenter/UnlockVaultPresenter.kt`:
- Around line 215-218: The current containsAllowedHosts() logic trusts hosts
individually which allows mixing approved api/auth pairs (e.g., (apiA,authA) and
(apiB,authB) to validate (apiA,authB)); change the model to validate the exact
approved pair/fingerprint instead: replace the flat host-set check in
containsAllowedHosts(UnverifiedHubVaultConfig) (and the similar check at the
other occurrence) with a lookup against stored approved-pair identifiers (e.g.,
a canonical pair key or fingerprint derived from apiBaseUrl+authEndpoint) and
update the preference helper to store and retrieve that single canonical
pair/fingerprint instead of two independent host entries so only the exact
approved api+auth combination is accepted.
- Around line 161-183: Short-circuit unsupported Hub vault actions by validating
intent.vaultAction() before showing the hub-host authenticity dialog or
persisting allowed hosts: check the action (e.g. CHANGE_PASSWORD,
ENCRYPT_PASSWORD, UNLOCK_FOR_BIOMETRIC_AUTH) at the start of the branch handling
unverifiedVaultConfig (the same place that calls isConsistentHubConfig and
before invoking
view?.showDialog(HubCheckHostAuthenticityDialog.newInstance(...))), and if the
action is not supported for hub operations, call finish() / show the existing
error flow instead of calling allowedHubHosts(...) or letting
onHubCheckHostsAllowed() persist to SharedPreferences; ensure
allowedHubHosts(...) and onHubCheckHostsAllowed() only run for supported
intent.vaultAction() values.
- Around line 201-203: isConsistentHubConfig currently uses getAuthority which
normalizes ports to schemes and can conflate different schemes; change it to
compare scheme, host, and port explicitly (or normalize by removing the port
only when it equals the default for that same scheme) so that https://host:443
and http://host:443 are not treated as equal, and ensure the HTTP-check logic
(isHttpHost or equivalent) inspects tokenEndpoint as well; update the logic in
isConsistentHubConfig and the related hub endpoint canonicalization (the code
around isConsistentHubConfig and the block referenced at 221-232) to perform
scheme+host+port-aware comparison rather than relying on getAuthority.
---
Duplicate comments:
In `@presentation/src/main/res/values/strings.xml`:
- Line 254: The string "screen_hub_reset_unknown_hub_hosts_title" and the dialog
copy that reads "Trust this hosts?" are incorrect for the trust flow; change the
title to reflect that this clears previously trusted hosts (e.g., "Reset Trusted
Hub Hosts" or "Reset Trusted Hosts") and fix the confirmation grammar by
replacing "Trust this hosts?" with "Trust these hosts?" — update the resource
value for screen_hub_reset_unknown_hub_hosts_title and the related confirmation
string resource (search for the resource containing "Trust this hosts?")
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 50d3ae8e-8122-4ebf-b726-ee68175785dd
📒 Files selected for processing (3)
presentation/src/main/java/org/cryptomator/presentation/presenter/UnlockVaultPresenter.ktpresentation/src/main/res/values/strings.xmlpresentation/src/main/res/xml/preferences.xml
presentation/src/main/java/org/cryptomator/presentation/presenter/UnlockVaultPresenter.kt
Show resolved
Hide resolved
presentation/src/main/java/org/cryptomator/presentation/presenter/UnlockVaultPresenter.kt
Show resolved
Hide resolved
presentation/src/main/java/org/cryptomator/presentation/presenter/UnlockVaultPresenter.kt
Show resolved
Hide resolved
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
2 participants
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.
No description provided.