cordova-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Ian Clelland" <iclell...@chromium.org>
Subject Re: Review Request 12548: Unify whitelist implementations: Android implementation
Date Wed, 17 Jul 2013 15:18:47 GMT


> On July 17, 2013, 2:54 p.m., Andrew Grieve wrote:
> > framework/src/org/apache/cordova/Whitelist.java, line 65
> > <https://reviews.apache.org/r/12548/diff/1/?file=321768#file321768line65>
> >
> >     Instead of parsing the url yourself, it might be better to use:
> >     
> >     Uri.parse(url)
> >     and use:
> >     getScheme(), getHost(), getPort(), getPath().
> >     
> >     Probably even better would be to take in a Uri as a parameter both here and
in isUrlWhitelisted. That way it's not re-parsed for each pattern.

I'd love to do this, and an earlier iteration of the code did just that. Unfortunately, the
Java URL parser won't recognize arbitrary schemes, unless there is a ProtocolHandler registered
for them.

I could certainly parse it just the once, and pass around a custom URI-like structure, for
efficiency, if you think that's warranted.


> On July 17, 2013, 2:54 p.m., Andrew Grieve wrote:
> > framework/src/org/apache/cordova/Whitelist.java, line 13
> > <https://reviews.apache.org/r/12548/diff/1/?file=321768#file321768line13>
> >
> >     make private & static?

This is for efficiency, and not to change usage, or to support any other use case, right?


> On July 17, 2013, 2:54 p.m., Andrew Grieve wrote:
> > framework/src/org/apache/cordova/Whitelist.java, line 184
> > <https://reviews.apache.org/r/12548/diff/1/?file=321768#file321768line184>
> >
> >     Doesn't look like this cache is very good.
> >     
> >     - It's unbounded
> >     - It's caching URLs without stripping query params
> >     - It's failing to cache negative hits.
> >     
> >     I don't think performance is an issue here, so probably a good idea to just
delete the cache.

Good catch. Deleted. It was there simply because the original implementation was caching results.


- Ian


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12548/#review23273
-----------------------------------------------------------


On July 15, 2013, 2:57 p.m., Ian Clelland wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12548/
> -----------------------------------------------------------
> 
> (Updated July 15, 2013, 2:57 p.m.)
> 
> 
> Review request for cordova.
> 
> 
> Repository: cordova-android-git
> 
> 
> Description
> -------
> 
> Replaced the current android whitelist implementation with a new one which conforms to
CB-4093.
> 
> This is also more robust against bad urls in the config file, and handles several cases
which the previous implementation did not (eg. http://www.apache.org:password@evil.com/ would
be accepted previously)
> 
> 
> Diffs
> -----
> 
>   framework/src/org/apache/cordova/Config.java 6e0c147 
>   framework/src/org/apache/cordova/Whitelist.java 736e5a7 
> 
> Diff: https://reviews.apache.org/r/12548/diff/
> 
> 
> Testing
> -------
> 
> This passes all of the recently-added whitelist tests in cordova-mobile-spec.
> 
> 
> Thanks,
> 
> Ian Clelland
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message