Skip to content

Harden Android app safety defaults#20

Closed
AmirrezaFarnamTaheri wants to merge 6 commits into
WhiteDNS:mainfrom
AmirrezaFarnamTaheri:android-safety-updates
Closed

Harden Android app safety defaults#20
AmirrezaFarnamTaheri wants to merge 6 commits into
WhiteDNS:mainfrom
AmirrezaFarnamTaheri:android-safety-updates

Conversation

@AmirrezaFarnamTaheri

Copy link
Copy Markdown
Contributor
  • Add shared redaction for diagnostics, logs, TOML/profile previews, and default copied export content.
  • Disable Android backup and replace sample backup/data extraction rules with explicit exclusions.
  • Harden clipboard/share flows with sensitive clipboard metadata, confirmed full exports, scoped FileProvider sharing, and stale export cleanup.
  • Add explicit network security config to deny app cleartext traffic by default.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens the Android client’s safety defaults by reducing accidental secret exposure (logs/diagnostics/export previews/clipboard), tightening export sharing behavior, and disabling insecure platform defaults (backup + cleartext networking).

Changes:

  • Introduces SecretRedactor + RedactionSecrets and adds unit tests for redaction behavior.
  • Updates UI/export flows to default to redacted previews/copies, adds “full export” confirmation, and shares exports via a scoped FileProvider cache directory with stale cleanup.
  • Disables Android backup and denies cleartext traffic by default via network security config.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
app/src/test/java/shop/whitedns/client/security/SecretRedactorTest.kt Adds unit tests validating configured and pattern-based secret redaction + idempotency.
app/src/main/java/shop/whitedns/client/security/SecretRedactor.kt Implements centralized redaction for logs/diagnostics/exports, including profile-link and runtime-path stripping.
app/src/main/java/shop/whitedns/client/ui/WhiteDnsViewModel.kt Switches connection log sanitization to use the shared runtime-secret redactor.
app/src/main/java/shop/whitedns/client/ui/WhiteDnsScreen.kt Redacts export previews by default, adds “COPY/SHARE FULL” confirmation, uses sensitive clipboard metadata, and shares exports via FileProvider + scoped cache directory cleanup.
app/src/main/AndroidManifest.xml Disables backup, wires backup/data-extraction rules, and applies network security config.
app/src/main/res/xml/network_security_config.xml Denies cleartext traffic by default.
app/src/main/res/xml/file_paths.xml Restricts FileProvider exposure to cache/exports/ only.
app/src/main/res/xml/data_extraction_rules.xml Replaces sample rules with explicit exclusions for backup/transfer.
app/src/main/res/xml/backup_rules.xml Replaces sample rules with explicit full-backup exclusions.
Comments suppressed due to low confidence (1)

app/src/main/java/shop/whitedns/client/ui/WhiteDnsViewModel.kt:1125

  • SecretRedactor.redact() already strips ANSI escape codes, but this callsite strips them again before calling .redactRuntimeSecrets(). This duplicates work on every log line; consider removing the local ANSI replace(...) here (or moving all ANSI stripping into the redactor consistently) to keep the sanitization pipeline single-sourced.
        val cleanMessage = message
            .replace(Regex("\\u001B\\[[;\\d]*m"), "")
            .trim()
            .redactRuntimeSecrets()

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +2757 to +2758
if (showQr && !redactedLink.contains('\n')) {
ProfileQrPreview(link = redactedLink)
@@ -662,7 +664,7 @@ class WhiteDnsViewModel(
val cleanMessage = message
.replace(Regex("\\u001B\\[[;\\d]*m"), "")
Comment on lines +59 to +67
.filter { it.length >= MinimumDirectSecretLength }
.distinct()
.sortedByDescending(String::length)
.fold(source) { text, value ->
text.replace(value, replacement)
}
}

private const val MinimumDirectSecretLength = 3
@iampedii

Copy link
Copy Markdown
Collaborator

@AmirrezaFarnamTaheri
Thanks for this MR. The safety hardening direction is good overall.

I especially like the Android backup/data extraction changes, the explicit network security config that denies cleartext traffic by default, and the move toward safer export/share handling. Those are useful defaults for an app that handles connection credentials and runtime configuration.

One part I think we should adjust is the redaction of the actual export string and QR code. For profile exports, the exported text and QR need to remain the real stormdns://... value, otherwise copy/share/import and QR-based setup can stop working or become confusing. Redacting the visible export value also makes it hard for users to verify what they are exporting.

Instead of redacting the export string itself, I think we should keep the full export value intact and show a clear warning above the export box, for example:

This export includes connection secrets. Only share it with people or devices you trust.

That keeps the export feature functional while still making the security risk explicit to the user.

So my suggestion is: keep the backup, data extraction, network security, diagnostics/log redaction, and safer sharing changes, but do not redact the profile export text or QR code.

@AmirrezaFarnamTaheri

Copy link
Copy Markdown
Contributor Author

Closing this older branch to keep the active upstream queue limited to PR #35 and PR #36. Useful remaining work is recorded in the local hardening roadmap and should return only as a clean non-overlapping batch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants