fix: sanitize DNS preference inputs#32
Merged
Merged
Conversation
📝 WalkthroughWalkthroughDNS list inputs for ChangesDNS Input Sanitization
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+50
to
+62
| private fun dnsReloadListener(preference: Preference, newValue: Any?): Boolean { | ||
| val rawValue = newValue as? String ?: return reloadListener.onPreferenceChange(preference, newValue) | ||
| val sanitizedValue = sanitizeDnsPreferenceValue(rawValue) | ||
| if (sanitizedValue != rawValue) { | ||
| if (preference is EditTextPreference) { | ||
| preference.text = sanitizedValue | ||
| } | ||
| needReload() | ||
| return false | ||
| } | ||
| needReload() | ||
| return true | ||
| } |
There was a problem hiding this comment.
needReload() is called unconditionally in both branches of the if/else, so it can be hoisted before the conditional to remove the duplication.
Suggested change
| private fun dnsReloadListener(preference: Preference, newValue: Any?): Boolean { | |
| val rawValue = newValue as? String ?: return reloadListener.onPreferenceChange(preference, newValue) | |
| val sanitizedValue = sanitizeDnsPreferenceValue(rawValue) | |
| if (sanitizedValue != rawValue) { | |
| if (preference is EditTextPreference) { | |
| preference.text = sanitizedValue | |
| } | |
| needReload() | |
| return false | |
| } | |
| needReload() | |
| return true | |
| } | |
| private fun dnsReloadListener(preference: Preference, newValue: Any?): Boolean { | |
| val rawValue = newValue as? String ?: return reloadListener.onPreferenceChange(preference, newValue) | |
| val sanitizedValue = sanitizeDnsPreferenceValue(rawValue) | |
| needReload() | |
| if (sanitizedValue != rawValue) { | |
| if (preference is EditTextPreference) { | |
| preference.text = sanitizedValue | |
| } | |
| return false | |
| } | |
| return true | |
| } |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Validation
Greptile Summary
This PR adds ISO control-character sanitization to the
remoteDnsanddirectDnspreference inputs at two layers: stripping control chars in the UI preference listener before persistence, and re-sanitizing each DNS entry during sing-box config generation as a defense-in-depth measure.SettingsPreferenceFragment.kt):dnsReloadListenersplits the raw input by line, strips ISO control characters per line withfilterNot { it.isISOControl() }, and persists the cleaned value viapreference.text = sanitizedValuebefore returningfalseto discard the raw input from the framework's default persistence path.ConfigBuilder.kt): A file-privatesanitizeDnsEntryfunction replaces the previous plaintrim()call on each split DNS entry; it addsfilterNot { it.isISOControl() }before trimming, ensuring any control characters stored before this fix are neutralized at config build time.Confidence Score: 5/5
Safe to merge; the sanitization logic is applied correctly at both layers and the value-persistence path through EditTextPreference works as intended.
Both sanitization paths are functionally sound. In the UI layer, preference.text = sanitizedValue calls persistString before return false discards the raw value, so the cleaned value reaches the datastore correctly. In ConfigBuilder the change is a straightforward augmentation of the existing trim() with an additional filterNot { it.isISOControl() }. No data-loss or incorrect-value scenarios were found in the changed code paths.
No files require special attention.
Important Files Changed
Sequence Diagram
%%{init: {'theme': 'neutral'}}%% sequenceDiagram participant U as User Input participant SP as SettingsPreferenceFragment participant DS as DataStore participant CB as ConfigBuilder U->>SP: Enter DNS value (may contain control chars) SP->>SP: sanitizeDnsPreferenceValue(rawValue) alt "sanitized != raw" SP->>DS: "preference.text = sanitizedValue (persist)" SP-->>U: return false (discard rawValue) else "sanitized == raw" SP->>DS: return true (framework persists rawValue) end SP->>SP: needReload() Note over CB,DS: Config generation (defense-in-depth) CB->>DS: DataStore.remoteDns / directDns CB->>CB: split(newline) CB->>CB: sanitizeDnsEntry(entry) per line CB->>CB: filterNot isISOControl + trim CB->>CB: takeIf isNotBlank and not comment%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% sequenceDiagram participant U as User Input participant SP as SettingsPreferenceFragment participant DS as DataStore participant CB as ConfigBuilder U->>SP: Enter DNS value (may contain control chars) SP->>SP: sanitizeDnsPreferenceValue(rawValue) alt "sanitized != raw" SP->>DS: "preference.text = sanitizedValue (persist)" SP-->>U: return false (discard rawValue) else "sanitized == raw" SP->>DS: return true (framework persists rawValue) end SP->>SP: needReload() Note over CB,DS: Config generation (defense-in-depth) CB->>DS: DataStore.remoteDns / directDns CB->>CB: split(newline) CB->>CB: sanitizeDnsEntry(entry) per line CB->>CB: filterNot isISOControl + trim CB->>CB: takeIf isNotBlank and not commentReviews (2): Last reviewed commit: "fix: type DNS sanitizing listeners" | Re-trigger Greptile