Opened 7 weeks ago

Closed 5 weeks ago

#27016 closed defect (fixed)

TBA: Audit thirdparty picasso

Reported by: sysrqb Owned by: tbb-team
Priority: Very High Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: tbb-mobile, tbb-proxy-bypass, TorBrowserTeam201808R
Cc: gk, igt0 Actual Points:
Parent ID: #25851 Points:
Reviewer: Sponsor:

Description

Proxy-safe?

$ ls mobile/android/thirdparty/com/squareup/picasso/
Action.java                     ContentStreamBitmapHunter.java  GetAction.java                  PicassoDrawable.java            Stats.java                      Utils.java
AssetBitmapHunter.java          DeferredRequestCreator.java     ImageViewAction.java            PicassoExecutorService.java     StatsSnapshot.java              
BitmapHunter.java               Dispatcher.java                 LruCache.java                   Picasso.java                    TargetAction.java               
Cache.java                      Downloader.java                 MarkableInputStream.java        RequestCreator.java             Target.java                     
Callback.java                   FetchAction.java                MediaStoreBitmapHunter.java     Request.java                    Transformation.java             
ContactsPhotoBitmapHunter.java  FileBitmapHunter.java           NetworkBitmapHunter.java        ResourceBitmapHunter.java       UrlConnectionDownloader.java

Child Tickets

Change History (5)

comment:1 Changed 7 weeks ago by sysrqb

This library using the NetworkInfo via the ConnectivityManager for deciding how many background threads it should create for processing. Higher bandwidth connections result in more threads. The default is 3, so this shouldn't be a problem. The library also registers for network change events. We'll probably need to change it so we don't require these Android permissions.

UrlConnectionDownloader is the only class that bypasses the proxy, so we'll need a patch for that, as well.

comment:2 in reply to:  1 ; Changed 7 weeks ago by sysrqb

Status: newneeds_review

Replying to sysrqb:

This library using the NetworkInfo via the ConnectivityManager for deciding how many background threads it should create for processing. Higher bandwidth connections result in more threads. The default is 3, so this shouldn't be a problem. The library also registers for network change events. We'll probably need to change it so we don't require these Android permissions.

This doesn't seem like it'll be a problem. Picasso catches missing permissions, so that shouldn't be cause a crash.

UrlConnectionDownloader is the only class that bypasses the proxy, so we'll need a patch for that, as well.

I have a patch for this, branch 27016, on my repo.

comment:3 in reply to:  2 Changed 7 weeks ago by sysrqb

Replying to sysrqb:

Replying to sysrqb:

This library using the NetworkInfo via the ConnectivityManager for deciding how many background threads it should create for processing. Higher bandwidth connections result in more threads. The default is 3, so this shouldn't be a problem. The library also registers for network change events. We'll probably need to change it so we don't require these Android permissions.

This doesn't seem like it'll be a problem. Picasso catches missing permissions, so that shouldn't be cause a crash.

I should note here, Picasso watches when "airplane mode" is enabled and disabled so it knows when it should retry downloading a file or failing (fail if airplane mode is enabled). I'm assuming we can safely keep this, for now.

comment:4 Changed 7 weeks ago by sysrqb

Keywords: TorBrowserTeam201808R added

comment:5 Changed 5 weeks ago by gk

Resolution: fixed
Status: needs_reviewclosed

Looks good to me. It's commit dda3c7d44a12cc2d9ff5e071605c8f36670ac956 on tor-browser-60.1.0esr-8.0-1.

Note: See TracTickets for help on using tickets.