Ben Turner (not reading bugmail, use the needinfo flag!) Reporter
	Description • 12 years ago

Some notes on the discussion I had with dougt:

  device-storage:apps:
    in child process check for webapp-manage, need to check in parent too
    no 'access' checks
    nsIContentPermissionPrompt can be bypassed from child
    maybe assert permission when handing out blobs (maybe /data only)

	Ben Turner (not reading bugmail, use the needinfo flag!) Reporter
	Comment 2 • 12 years ago

Comment on attachment 685300 [details] [diff] [review]
patch v.1

Review of attachment 685300 [details] [diff] [review]:
-----------------------------------------------------------------

This stuff looks fine, but it only addresses a piece of the problem (for "apps"). Are you planning on doing the other type checks in another bug?

	Ben Turner (not reading bugmail, use the needinfo flag!) Reporter
	Comment 3 • 12 years ago

Comment on attachment 685300 [details] [diff] [review]
patch v.1

Actually, hm. It's weird that you're doing this in a constructor since you can't bail out if the app doesn't have permission. The child process will get killed regardless but you keep on going as if nothing happened...

	Ben Turner (not reading bugmail, use the needinfo flag!) Reporter
	Comment 4 • 12 years ago

Comment on attachment 685300 [details] [diff] [review]
patch v.1

Actually, yeah, can we do this in ContentParent::AllocPDeviceStorageRequest instead? Then we can bail out correctly by returning null.

	Ben Turner (not reading bugmail, use the needinfo flag!) Reporter
	Comment 7 • 12 years ago

Comment on attachment 686217 [details] [diff] [review]
patch v.3

Review of attachment 686217 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!

::: dom/devicestorage/DeviceStorageRequestParent.cpp
@@ +182,5 @@
> +      return false;
> +    }
> +  }
> +
> +  // the 'apps' type is special.  We only want this exposed

Nit: Capitalize 'the'

@@ +190,5 @@
> +      return false;
> +    }
> +  }
> +
> +  nsCString permissionName;

Since you're doing some appends below you should use nsAutoCString.

::: dom/devicestorage/DeviceStorageRequestParent.h
@@ +25,5 @@
>  
>    NS_IMETHOD_(nsrefcnt) AddRef();
>    NS_IMETHOD_(nsrefcnt) Release();
> +
> +  bool EnsureRequiredPermissions(mozilla::dom::ContentParent* aParent);  

Nit: trailing whitespace

::: dom/ipc/ContentParent.cpp
@@ +1192,5 @@
>  ContentParent::AllocPDeviceStorageRequest(const DeviceStorageParams& aParams)
>  {
> +  nsRefPtr<DeviceStorageRequestParent> result = new DeviceStorageRequestParent(aParams);
> +  if (!result->EnsureRequiredPermissions(this)) {
> +      return nullptr; 

Nit: trailing whitespace

	Doug Turner (:dougt) Assignee
	Comment 8 • 12 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/0690146190f1

	Doug Turner (:dougt) Assignee
	Comment 9 • 12 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/13fa37380b51
https://hg.mozilla.org/releases/mozilla-beta/rev/fbf8dcadab61

	Andrew Overholt [:overholt]
	Comment 10 • 12 years ago

Setting priority based on triage discussions.  Feel free to decrease priority if you disagree.

	Ryan VanderMeulen [:RyanVM]
	Comment 11 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/0690146190f1