Issue 29480569: Issue 5254 - Improve URL verification when whitelisting domains (Closed)
|
Created:
July 5, 2017, 9:05 a.m. by jens Modified:
July 26, 2017, 8:30 a.m. CC:
Felix Dahlke, René Jeschke Visibility:
Public. |
DescriptionIssue 5254 - Improve URL verification when whitelisting domains
Patch Set 1 #
Total comments: 7
Created: July 5, 2017, 9:05 a.m.
(Patch set is too large to download)
MessagesTotal messages: 6
https://codereview.adblockplus.org/29480569/diff/29480570/adblockplussbrowser... File adblockplussbrowser/build.gradle (right): https://codereview.adblockplus.org/29480569/diff/29480570/adblockplussbrowser... adblockplussbrowser/build.gradle:1: this change (new line) is not required https://codereview.adblockplus.org/29480569/diff/29480570/adblockplussbrowser... adblockplussbrowser/build.gradle:42: compile (group: 'commons-validator', name: 'commons-validator', version: '1.6') { should we really use the whole Apache commons module for just URL validation? In libadblockplus-android we originally copied Apache Strings right into app source code and then reimplemented it with our own code. Can't we really implement (or copy the code which is not desired but possible) this on our own? I'd prefer to cover it by tests too. https://codereview.adblockplus.org/29480569/diff/29480570/adblockplussbrowser... File adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/UrlInputOpenerPreference.java (right): https://codereview.adblockplus.org/29480569/diff/29480570/adblockplussbrowser... adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/UrlInputOpenerPreference.java:137: return DomainValidator.getInstance().isValid(getUrl()); it's misleading - we validate URL with Apache DOMAIN validator. Can you please explain it? Also note UrlValidator exists (https://commons.apache.org/proper/commons-validator/apidocs/org/apache/common...) in Apache Commons. Can/shouldn't we rename the method to isValidDomain()?
https://codereview.adblockplus.org/29480569/diff/29480570/adblockplussbrowser... File adblockplussbrowser/build.gradle (right): https://codereview.adblockplus.org/29480569/diff/29480570/adblockplussbrowser... adblockplussbrowser/build.gradle:42: compile (group: 'commons-validator', name: 'commons-validator', version: '1.6') { On 2017/07/06 06:51:10, anton wrote: > should we really use the whole Apache commons module for just URL validation? > In libadblockplus-android we originally copied Apache Strings right into app > source code and then reimplemented it with our own code. > > Can't we really implement (or copy the code which is not desired but possible) > this on our own? > I'd prefer to cover it by tests too. I think it's okay to use commons validator, as the list of valid TLDs is updated regularly. We just need to check if there is a new version available or alternatively use "version: '+'" to always use the lastest release. Even if the latter one is not ideal, it shouldn't be a problem if we cover the validity check by tests. With proguard activated the APK is just slightly bigger than before: 1.666.715 Byte vs. 1.709.685 Byte Copying the code and keeping the TLDs up to date would be a bigger effort than to just use the lib. In view of the fact that the apk is only slightly bigger, this solution would be fine for me. But of course we can talk about this with the others. https://codereview.adblockplus.org/29480569/diff/29480570/adblockplussbrowser... File adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/UrlInputOpenerPreference.java (right): https://codereview.adblockplus.org/29480569/diff/29480570/adblockplussbrowser... adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/UrlInputOpenerPreference.java:137: return DomainValidator.getInstance().isValid(getUrl()); On 2017/07/06 06:51:10, anton wrote: > it's misleading - we validate URL with Apache DOMAIN validator. Can you please > explain it? > Also note UrlValidator exists > (https://commons.apache.org/proper/commons-validator/apidocs/org/apache/common...) > in Apache Commons. > > Can/shouldn't we rename the method to isValidDomain()? The purpose of this change is, to have a reliable check if a TLD is valid. That's why I use DomainValidator. But adjusting the method name would make sense, that's right.
https://codereview.adblockplus.org/29480569/diff/29480570/adblockplussbrowser... File adblockplussbrowser/build.gradle (right): https://codereview.adblockplus.org/29480569/diff/29480570/adblockplussbrowser... adblockplussbrowser/build.gradle:42: compile (group: 'commons-validator', name: 'commons-validator', version: '1.6') { On 2017/07/06 08:49:06, jens wrote: > On 2017/07/06 06:51:10, anton wrote: > > should we really use the whole Apache commons module for just URL validation? > > In libadblockplus-android we originally copied Apache Strings right into app > > source code and then reimplemented it with our own code. > > > > Can't we really implement (or copy the code which is not desired but possible) > > this on our own? > > I'd prefer to cover it by tests too. > > I think it's okay to use commons validator, as the list of valid TLDs is updated > regularly. We just need to check if there is a new version available or > alternatively use "version: '+'" to always use the lastest release. Even if the > latter one is not ideal, it shouldn't be a problem if we cover the validity > check by tests. With proguard activated the APK is just slightly bigger than > before: 1.666.715 Byte vs. 1.709.685 Byte > > Copying the code and keeping the TLDs up to date would be a bigger effort than > to just use the lib. In view of the fact that the apk is only slightly bigger, > this solution would be fine for me. But of course we can talk about this with > the others. > IHMO, for this project, we don't have a high size requirement as we do have in libadblockplus-android, so the size increase in this case sounds fine to me. But also, a extremely high quality TLD validation is currently just a 'nice to have' feature, since this is currently just used when the user whitelist a website. So I'm quite fine with either keeping the dependency or extracting the relevant piece of code to our project.
https://codereview.adblockplus.org/29480569/diff/29480570/adblockplussbrowser... File adblockplussbrowser/build.gradle (right): https://codereview.adblockplus.org/29480569/diff/29480570/adblockplussbrowser... adblockplussbrowser/build.gradle:42: compile (group: 'commons-validator', name: 'commons-validator', version: '1.6') { On 2017/07/06 18:25:16, diegocarloslima wrote: > On 2017/07/06 08:49:06, jens wrote: > > On 2017/07/06 06:51:10, anton wrote: > > > should we really use the whole Apache commons module for just URL > validation? > > > In libadblockplus-android we originally copied Apache Strings right into app > > > source code and then reimplemented it with our own code. > > > > > > Can't we really implement (or copy the code which is not desired but > possible) > > > this on our own? > > > I'd prefer to cover it by tests too. > > > > I think it's okay to use commons validator, as the list of valid TLDs is > updated > > regularly. We just need to check if there is a new version available or > > alternatively use "version: '+'" to always use the lastest release. Even if > the > > latter one is not ideal, it shouldn't be a problem if we cover the validity > > check by tests. With proguard activated the APK is just slightly bigger than > > before: 1.666.715 Byte vs. 1.709.685 Byte > > > > Copying the code and keeping the TLDs up to date would be a bigger effort than > > to just use the lib. In view of the fact that the apk is only slightly bigger, > > this solution would be fine for me. But of course we can talk about this with > > the others. > > > > IHMO, for this project, we don't have a high size requirement as we do have in > libadblockplus-android, so the size increase in this case sounds fine to me. But > also, a extremely high quality TLD validation is currently just a 'nice to have' > feature, since this is currently just used when the user whitelist a website. So > I'm quite fine with either keeping the dependency or extracting the relevant > piece of code to our project. okay, let's just revert adding new empty lines and then it LGTM
On 2017/07/07 13:03:19, anton wrote: > https://codereview.adblockplus.org/29480569/diff/29480570/adblockplussbrowser... > File adblockplussbrowser/build.gradle (right): > > https://codereview.adblockplus.org/29480569/diff/29480570/adblockplussbrowser... > adblockplussbrowser/build.gradle:42: compile (group: 'commons-validator', name: > 'commons-validator', version: '1.6') { > On 2017/07/06 18:25:16, diegocarloslima wrote: > > On 2017/07/06 08:49:06, jens wrote: > > > On 2017/07/06 06:51:10, anton wrote: > > > > should we really use the whole Apache commons module for just URL > > validation? > > > > In libadblockplus-android we originally copied Apache Strings right into > app > > > > source code and then reimplemented it with our own code. > > > > > > > > Can't we really implement (or copy the code which is not desired but > > possible) > > > > this on our own? > > > > I'd prefer to cover it by tests too. > > > > > > I think it's okay to use commons validator, as the list of valid TLDs is > > updated > > > regularly. We just need to check if there is a new version available or > > > alternatively use "version: '+'" to always use the lastest release. Even if > > the > > > latter one is not ideal, it shouldn't be a problem if we cover the validity > > > check by tests. With proguard activated the APK is just slightly bigger than > > > before: 1.666.715 Byte vs. 1.709.685 Byte > > > > > > Copying the code and keeping the TLDs up to date would be a bigger effort > than > > > to just use the lib. In view of the fact that the apk is only slightly > bigger, > > > this solution would be fine for me. But of course we can talk about this > with > > > the others. > > > > > > > IHMO, for this project, we don't have a high size requirement as we do have in > > libadblockplus-android, so the size increase in this case sounds fine to me. > But > > also, a extremely high quality TLD validation is currently just a 'nice to > have' > > feature, since this is currently just used when the user whitelist a website. > So > > I'm quite fine with either keeping the dependency or extracting the relevant > > piece of code to our project. > > okay, let's just revert adding new empty lines and then it LGTM This also LGTM by reverting the unnecessary added line in build.gradle 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
