Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23466009:
Expand whitelist for media stream APIs. (Closed)
|
Created:
7 years, 3 months ago by bbudge Modified:
7 years, 3 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionExpand whitelist for media stream APIs.
Pre-review heads up.
BUG=none
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=223618
Patch Set 1 #
Total comments: 2
Patch Set 2 : Final URLs. #Patch Set 3 : Fix Android unit test. #Patch Set 4 : Adjust Hangouts URL checking.x #
Total comments: 8
Patch Set 5 : Fix AllowPepperMediaStreamAPI. #Patch Set 6 : Loosen host checking a bit. #
Messages
Total messages: 12 (0 generated)
Justin, this is what the patch would look like. Let me know what the final URLs are. https://codereview.chromium.org/23466009/diff/1/chrome/renderer/chrome_conten... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/23466009/diff/1/chrome/renderer/chrome_conten... chrome/renderer/chrome_content_renderer_client.cc:783: (manifest_url.path().find("hangouts/nacl/") == 1)); This expands the NaCl whitelist to hangouts. https://codereview.chromium.org/23466009/diff/1/chrome/renderer/chrome_conten... chrome/renderer/chrome_content_renderer_client.cc:1292: url.path().find("hangouts/") == 1) { This changes the way we identify the hangouts app. Now we look at the document URL.
For the serving URL, we plan to use https://www.gstatic.com/chat/apps/fx<https://www.gstatic.com/chat/apps/fx/v1.... - we're already hosting a bunch of effects static content there. For the app page URL, we should allow talkgadget.google.com along with *. talkgadget.google.com for testing. Victoria, does that sound right to you? On Fri, Sep 6, 2013 at 11:50 AM, <bbudge@chromium.org> wrote: > Reviewers: darin, juberti2, > > Message: > Justin, this is what the patch would look like. Let me know what the final > URLs > are. > > > https://codereview.chromium.**org/23466009/diff/1/chrome/** > renderer/chrome_content_**renderer_client.cc<https://codereview.chromium.org/23466009/diff/1/chrome/renderer/chrome_content_renderer_client.cc> > File chrome/renderer/chrome_**content_renderer_client.cc (right): > > https://codereview.chromium.**org/23466009/diff/1/chrome/** > renderer/chrome_content_**renderer_client.cc#newcode783<https://codereview.chromium.org/23466009/diff/1/chrome/renderer/chrome_content_renderer_client.cc#newcode783> > chrome/renderer/chrome_**content_renderer_client.cc:**783: > (manifest_url.path().find("**hangouts/nacl/") == 1)); > This expands the NaCl whitelist to hangouts. > > https://codereview.chromium.**org/23466009/diff/1/chrome/** > renderer/chrome_content_**renderer_client.cc#newcode1292<https://codereview.chromium.org/23466009/diff/1/chrome/renderer/chrome_content_renderer_client.cc#newcode1292> > chrome/renderer/chrome_**content_renderer_client.cc:**1292: > url.path().find("hangouts/") == 1) { > This changes the way we identify the hangouts app. Now we look at the > document URL. > > Description: > Expand whitelist for media stream APIs. > > BUG=none > > Please review this at https://codereview.chromium.**org/23466009/<https://codereview.chromium.org/2... > > SVN Base: svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src> > > Affected files (+6, -4 lines): > M chrome/renderer/chrome_**content_renderer_client.cc > > > Index: chrome/renderer/chrome_**content_renderer_client.cc > diff --git a/chrome/renderer/chrome_**content_renderer_client.cc > b/chrome/renderer/chrome_**content_renderer_client.cc > index 1fa7a9cc91667b06716abe8facba1a**0671e46219..** > 8bf2ac3a3a921031fa53fe250b4829**35a5781cd5 100644 > --- a/chrome/renderer/chrome_**content_renderer_client.cc > +++ b/chrome/renderer/chrome_**content_renderer_client.cc > @@ -779,7 +779,8 @@ bool ChromeContentRendererClient::**IsNaClAllowed( > manifest_url.SchemeIs("https") && > manifest_url.host() == "ssl.gstatic.com" && > ((manifest_url.path().find("**s2/oz/nacl/") == 1) || > - (manifest_url.path().find("**photos/nacl/") == 1)); > + (manifest_url.path().find("**photos/nacl/") == 1) || > + (manifest_url.path().find("**hangouts/nacl/") == 1)); > > bool is_extension_from_webstore = > extension && extension->from_webstore(); > @@ -1282,12 +1283,13 @@ bool ChromeContentRendererClient::** > AllowBrowserPlugin( > bool ChromeContentRendererClient::**AllowPepperMediaStreamAPI( > const GURL& url) { > #if !defined(OS_ANDROID) > - std::string host = url.host(); > // Allow only the Hangouts app to use the MediaStream APIs. It's OK to > check > // the whitelist in the renderer, since we're only preventing access > until > // these APIs are public and stable. > - if (url.SchemeIs(extensions::**kExtensionScheme) && > - !host.compare("**hpcogiolnobbkijnnkdahioejpdcdo**ph")) { > + if (url.SchemeIs("https") && > + (url.host() == "plus.google.com" || > + url.host() == "plus.sandbox.google.com") && > + url.path().find("hangouts/") == 1) { > return true; > } > // Allow access for tests. > > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Added the final URLs for Hangouts, and a unit test. Justin, can you verify that your manifest and nexes are served from www.gstatic.com and not ssl.gstatic.com (like photos).
On 2013/09/14 00:40:09, bbudge1 wrote: > Added the final URLs for Hangouts, and a unit test. > > Justin, can you verify that your manifest and nexes are served from > http://www.gstatic.com and not http://ssl.gstatic.com (like photos). ssl.gstatic.com will work fine for us. Note that we need support for not just talkgadget.google.com, but also *.talkgadget.google.com (e.g. today.talkgadget.google.com).
Changed Hangouts manifest/nexe serving domain to ssl.gstatic.com. Fixed domain checking for app url (I misunderstood function of DomainIs method).
LGTM w/ minor name change https://codereview.chromium.org/23466009/diff/25001/chrome/renderer/chrome_co... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/23466009/diff/25001/chrome/renderer/chrome_co... chrome/renderer/chrome_content_renderer_client.cc:789: // Hangouts app. nit: "Hangouts" -> "Chat" since the URLs don't mention Hangouts. https://codereview.chromium.org/23466009/diff/25001/chrome/renderer/chrome_co... File chrome/renderer/chrome_content_renderer_client_unittest.cc (right): https://codereview.chromium.org/23466009/diff/25001/chrome/renderer/chrome_co... chrome/renderer/chrome_content_renderer_client_unittest.cc:45: const char kHangoutsAppURL1[] = "https://talkgadget.google.com"; nit: Hangouts -> Chat
https://codereview.chromium.org/23466009/diff/25001/chrome/renderer/chrome_co... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/23466009/diff/25001/chrome/renderer/chrome_co... chrome/renderer/chrome_content_renderer_client.cc:1299: url.DomainIs("talkgadget.google.com")) { I think this needs the EndsWith treatment too. https://codereview.chromium.org/23466009/diff/25001/chrome/renderer/chrome_co... File chrome/renderer/chrome_content_renderer_client_unittest.cc (right): https://codereview.chromium.org/23466009/diff/25001/chrome/renderer/chrome_co... chrome/renderer/chrome_content_renderer_client_unittest.cc:374: EXPECT_FALSE(test.AllowPepperMediaStreamAPI( add test for foo.talkgadget.google.com
https://codereview.chromium.org/23466009/diff/25001/chrome/renderer/chrome_co... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/23466009/diff/25001/chrome/renderer/chrome_co... chrome/renderer/chrome_content_renderer_client.cc:789: // Hangouts app. On 2013/09/16 19:37:44, darin wrote: > nit: "Hangouts" -> "Chat" since the URLs don't mention Hangouts. Done. https://codereview.chromium.org/23466009/diff/25001/chrome/renderer/chrome_co... chrome/renderer/chrome_content_renderer_client.cc:1299: url.DomainIs("talkgadget.google.com")) { On 2013/09/16 19:41:52, juberti2 wrote: > I think this needs the EndsWith treatment too. Yep, good catch. Done. https://codereview.chromium.org/23466009/diff/25001/chrome/renderer/chrome_co... File chrome/renderer/chrome_content_renderer_client_unittest.cc (right): https://codereview.chromium.org/23466009/diff/25001/chrome/renderer/chrome_co... chrome/renderer/chrome_content_renderer_client_unittest.cc:45: const char kHangoutsAppURL1[] = "https://talkgadget.google.com"; On 2013/09/16 19:37:44, darin wrote: > nit: Hangouts -> Chat Done. https://codereview.chromium.org/23466009/diff/25001/chrome/renderer/chrome_co... chrome/renderer/chrome_content_renderer_client_unittest.cc:374: EXPECT_FALSE(test.AllowPepperMediaStreamAPI( On 2013/09/16 19:41:52, juberti2 wrote: > add test for http://foo.talkgadget.google.com Done.
On 2013/09/16 17:31:41, bbudge1 wrote: > Changed Hangouts manifest/nexe serving domain to http://ssl.gstatic.com. > Fixed domain checking for app url (I misunderstood function of DomainIs method). Sorry for the late reply on this. There are actually a few more to whitelist, just got the definitive list from noahric on Hangouts: "*plus.google.com", // Hangouts "*plus.sandbox.google.com", // Oz page, internal "*talk.google.com", // Gcomm "*talkgadget.google.com", // Supermoles (igoogle, orkut, etc) Bill, could you expand the whitelist to include plus, plus.sandbox, and talk?
On 2013/09/16 20:39:46, Victoria Kirst wrote: > On 2013/09/16 17:31:41, bbudge1 wrote: > > Changed Hangouts manifest/nexe serving domain to http://ssl.gstatic.com. > > Fixed domain checking for app url (I misunderstood function of DomainIs > method). > > Sorry for the late reply on this. There are actually a few more to whitelist, > just got the definitive list from noahric on Hangouts: > > "*plus.google.com", // Hangouts > "*plus.sandbox.google.com", // Oz page, internal > "*talk.google.com", // Gcomm > "*talkgadget.google.com", // Supermoles (igoogle, orkut, etc) > > Bill, could you expand the whitelist to include plus, plus.sandbox, and talk? Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bbudge@chromium.org/23466009/6004
Message was sent while issue was closed.
Change committed as 223618 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
