Validate hosts file URL #486

gsora · 2017-01-29T16:26:04Z

This patchset adds logic needed to validate hosts file's URL, using both a custom EditTextPreference to just verify if it's a tangible URL (all done using Android parsing and validation facilities), and a simple conditional statement to make sure we never create URL objects without the http/https prefix.

This class uses standard Android facilities to check for the hosts URL validity. Signed-off-by: Gianguido Sora <g.sora4@gmail.com>

HostsTextPreference does not enforce use of the http/https prefix. We add it when creating an URL object instead. Signed-off-by: Gianguido Sora <g.sora4@gmail.com>

Signed-off-by: Gianguido Sora <g.sora4@gmail.com>

M66B · 2017-01-29T17:59:26Z

app/src/main/java/eu/faircode/netguard/ActivitySettings.java

+
+                        }
+                    } catch (MalformedURLException ex) {
+                        Toast.makeText(ActivitySettings.this, ex.toString(), Toast.LENGTH_LONG).show();


Generally the caller should check the validity of its arguments to keep the calling code as simple as possible.

What about moving the entire validation path (RFC 3987 + http{s} prefix presence) to HostsTextPreference? I have a working example ready to be pushed.

M66B · 2017-01-29T17:59:49Z

app/src/main/java/eu/faircode/netguard/ActivitySettings.java

@@ -90,6 +94,8 @@
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;


Unused imports?

M66B · 2017-01-29T18:00:07Z

app/src/main/java/eu/faircode/netguard/ActivitySettings.java

-        EditTextPreference pref_hosts_url = (EditTextPreference) screen.findPreference("hosts_url");
+        final HostsTextPreference pref_hosts_url = (HostsTextPreference) screen.findPreference("hosts_url");
+
+


Extra blank lines?

M66B · 2017-01-29T18:01:05Z

app/src/main/java/eu/faircode/netguard/ActivitySettings.java

+                        } else {
+                            hostsURL = new URL(hostsPresumedURL);
+
+                        }


Unneeded braces.

Reason: to keep as many as possible code lines on screen, the number of opening/closing braces should be kept to a minimum.

M66B · 2017-01-29T18:01:35Z

app/src/main/java/eu/faircode/netguard/HostsTextPreference.java

+        if(dlg instanceof AlertDialog) {
+            AlertDialog alertDlg = (AlertDialog)dlg;
+            Button btn = alertDlg.getButton(AlertDialog.BUTTON_POSITIVE);
+            boolean enable = Patterns.WEB_URL.matcher(getEditText().getText().toString()).matches();


Isn't this going to be slow on low end devices?

M66B · 2017-01-29T18:02:00Z

app/src/main/java/eu/faircode/netguard/HostsTextPreference.java

+    @Override
+    protected void showDialog(Bundle state) {
+        super.showDialog(state);
+        getEditText().removeTextChangedListener(mTextWatcher);


Why removing the watcher?

M66B · 2017-01-29T18:02:40Z

The idea is good, but some details needs to be worked out.

M66B · 2017-01-29T18:03:05Z

Thanks for doing this and creating a pull request.
Contributions are rare.

gsora · 2017-01-29T19:43:09Z

Sadly my Android skills aren't that sharp, but this is a contribution I felt was needed.

Thank you for pointing out all the flaws on my code, I'll try to correct them as soon as possible.

…Type is now set to URI

1) the inner edittext now doesn't show the error message anymore, since the validation is now more prolific 2) the watcher is not removed from the edittext, just added

M66B · 2017-02-02T07:46:19Z

app/src/main/java/eu/faircode/netguard/ActivitySettings.java

+                        Toast.makeText(ActivitySettings.this, ex.toString(), Toast.LENGTH_LONG).show();
+                    }
+
+                    new DownloadTask(ActivitySettings.this, hostsURL, tmp, new DownloadTask.Listener() {


This will crash on a MalformedURLException (hostsURL null), better restore the original.
Basically only EditTextPreference needs to be changed into HostsTextPreference on line 292.

The default behaviour has been restored, please see commit 647e981

The previous behaviour has been restored as suggested in M66B/NetGuard#486 (review)

M66B · 2017-02-02T15:12:27Z

app/src/main/java/eu/faircode/netguard/HostsTextPreference.java

+        switch (which) {
+            case DialogInterface.BUTTON_POSITIVE: {
+                String hostsPresumedURL = getEditText().getText().toString();
+                if (!(hostsPresumedURL.startsWith("http://") || hostsPresumedURL.startsWith("https://"))) {


What if somebody wants to enter a file:// URI type?
Better use getScheme of Uri to check if there is a scheme and build a new URI with the default https (not http) scheme.
https://developer.android.com/reference/android/net/Uri.html#getScheme()

Since Patterns.WEB_URL doesn't match any URI type other than HTTP, HTTPS and RTSP, the "OK" button wouldn't be enabled, thus preventing any exception.

Then this is not an acceptable solution, because there is more in the world than only those protocols.
Google doesn't seem to comply to the standards as well: https://www.ietf.org/rfc/rfc3987.txt

I've just noticed that, they even tell that in the documentation.

I'll try to come up with a better regex in the next few days.

M66B · 2017-03-13T09:26:50Z

I am going to close this pull request, since it seems abandoned and it is being misuse for somebody else's requests.

gsora added 3 commits January 29, 2017 17:19

Added custom EditTextPreference class

8398b9f

This class uses standard Android facilities to check for the hosts URL validity. Signed-off-by: Gianguido Sora <g.sora4@gmail.com>

Use HostsTextPreference and validate http/https presence in URL.

c7f4a61

HostsTextPreference does not enforce use of the http/https prefix. We add it when creating an URL object instead. Signed-off-by: Gianguido Sora <g.sora4@gmail.com>

HostsTextPreference references instead of EditTextPreference.

0ff33ae

Signed-off-by: Gianguido Sora <g.sora4@gmail.com>

M66B reviewed Jan 29, 2017

View reviewed changes

gsora added 5 commits February 2, 2017 00:08

fixed unused imports

b14d0f8

removed extra blank lines

b6b2be3

set maxLines to 1, preventing the input from being lengthy; the input…

08b683b

…Type is now set to URI

move RFC 3987 + http{s} prefix validation inside HostsTextPreference

ad8cd88

complete validation done inside the preference view, more fixes

d395e8d

1) the inner edittext now doesn't show the error message anymore, since the validation is now more prolific 2) the watcher is not removed from the edittext, just added

M66B reviewed Feb 2, 2017

View reviewed changes

reverted to 9db81a3

647e981

The previous behaviour has been restored as suggested in M66B/NetGuard#486 (review)

M66B reviewed Feb 2, 2017

View reviewed changes

M66B force-pushed the master branch from 73ea718 to 00c7f6f Compare February 26, 2017 15:29

M66B force-pushed the master branch 4 times, most recently from 7141894 to 1ffb95a Compare March 12, 2017 17:15

M66B closed this Mar 13, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate hosts file URL #486

Validate hosts file URL #486

gsora commented Jan 29, 2017

M66B Jan 29, 2017

gsora Feb 2, 2017

M66B Jan 29, 2017

M66B Jan 29, 2017

M66B Jan 29, 2017

M66B Jan 29, 2017

M66B Jan 29, 2017

M66B commented Jan 29, 2017

M66B commented Jan 29, 2017

gsora commented Jan 29, 2017

M66B Feb 2, 2017 •

edited

gsora Feb 2, 2017

M66B Feb 2, 2017

gsora Feb 2, 2017

M66B Feb 2, 2017

gsora Feb 2, 2017

M66B commented Mar 13, 2017

		EditTextPreference pref_hosts_url = (EditTextPreference) screen.findPreference("hosts_url");
		final HostsTextPreference pref_hosts_url = (HostsTextPreference) screen.findPreference("hosts_url");

Validate hosts file URL #486

Validate hosts file URL #486

Conversation

gsora commented Jan 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

M66B commented Jan 29, 2017

M66B commented Jan 29, 2017

gsora commented Jan 29, 2017

M66B Feb 2, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

M66B commented Mar 13, 2017

M66B Feb 2, 2017 •

edited