Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1050063002:
Web MIDI: allow http://localhost to prompt sysex permission (Closed)
|
Created:
5 years, 8 months ago by Takashi Toyoshima Modified:
5 years, 8 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionWeb MIDI: allow http://localhost to prompt sysex permission
Allow to obtain a sysex permission for localhost even if the scheme is
non-secure. This is inteded for to use for testing.
TEST=manual check with http://localhost:xxx and http://127.0.0.1:xxx
BUG=470170
Committed: https://crrev.com/78fb2d9a429de070b0d7c9c57710cffa52c4b52b
Cr-Commit-Position: refs/heads/master@{#323555}
Patch Set 1 #
Total comments: 3
Patch Set 2 : use net/ #Patch Set 3 : rebase #Messages
Total messages: 18 (4 generated)
toyoshim@chromium.org changed reviewers: + markusheintz@chromium.org, msramek@chromium.org
msramek for security, and markus for owners review. I disabled to edit and add exceptions in content settings UI by a different CL. So, is it ok to allow http://localhost for testing now? Or should we wait something to make it safe enough?
I think the resolution of localhost should be safe (unless "localhost" in /etc/hosts points to something else, but if the attacker has root access, there is nothing we can do anyway). But I'm not an expert in this area and I cannot give you a security approval, please consult with someone from Chrome Security. I also want to note here that there probably isn't a precedent for this. For example, media permissions (https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/med...) also treat HTTP and HTTPS differently, but if I read correctly, there is no exception made for localhost. https://codereview.chromium.org/1050063002/diff/1/chrome/browser/content_sett... File chrome/browser/content_settings/permission_context_base.cc (right): https://codereview.chromium.org/1050063002/diff/1/chrome/browser/content_sett... chrome/browser/content_settings/permission_context_base.cc:24: (!url.host().compare("localhost") || !url.host().compare("127.0.0.1")); How about IPv6 and other options? https://code.google.com/p/chromium/codesearch#chromium/src/net/base/net_util_...
markusheintz@chromium.org changed reviewers: + felt@chromium.org
Adding felt@. MIDI settings are only persisted for secure scheme right? What is the threat you try to protect against? Content settings are read from a text file. So anybody that can write to this file can add content settings. It doesn't matter whether they the UI allows to add these settings or not. In genera I thing making an exception for localhost seems sensible to me. https://codereview.chromium.org/1050063002/diff/1/chrome/browser/content_sett... File chrome/browser/content_settings/permission_context_base.cc (right): https://codereview.chromium.org/1050063002/diff/1/chrome/browser/content_sett... chrome/browser/content_settings/permission_context_base.cc:22: bool IsHTTPLocalhost(const GURL& url) { Have you checked of there is not already a utility method in the code base that does this?
felt@chromium.org changed reviewers: + palmer@chromium.org
It's fine to allow localhost (it is a secure origin by the definition) but I am not sure this is the right way to do it. Adding palmer@ to advise, as he is currently Emperor of Origins, Czar of Boundaries, and King of Host-Scheme-Port.
> It's fine to allow localhost (it is a secure origin by the definition) but I am > not sure this is the right way to do it. > > Adding palmer@ to advise, as he is currently Emperor of Origins, Czar of > Boundaries, and King of Host-Scheme-Port. I am also Early Friend Of Web MIDI. :) Yes, it's totally OK to ask people if they want (*, localhost, *) to have SYSEX permission. But, I am in the process of writing you a nice, standard utility function to do the check: https://codereview.chromium.org/1049533002/ (and https://codereview.chromium.org/1049923002/) (see https://code.google.com/p/chromium/issues/detail?id=362214) You can either wait for my stuff to land and then call it, or you can land this now and add a TODO and file a bug for me or you to replace your check with a call to the standard OriginIsSecure function. Hopefully my stuff will land Real Soon Now and you can just call it. But, my code reviewers are famously busy...
https://codereview.chromium.org/1050063002/diff/1/chrome/browser/content_sett... File chrome/browser/content_settings/permission_context_base.cc (right): https://codereview.chromium.org/1050063002/diff/1/chrome/browser/content_sett... chrome/browser/content_settings/permission_context_base.cc:22: bool IsHTTPLocalhost(const GURL& url) { You should use net::IsLocalhost here
From media/DEPS, > # Do NOT add net/ or ui/base without a great reason, they're huge! So, I'd land this with TODO, or wait for palmer's nice patch.
On 2015/04/02 10:51:38, Takashi Toyoshima (chromium) wrote: > From media/DEPS, > > # Do NOT add net/ or ui/base without a great reason, they're huge! > > So, I'd land this with TODO, or wait for palmer's nice patch. It's up to you. But if you decide to land this CL now you should use net::IsLocalhost as Adrienne suggested.
Ah, sorry. I'm confused because my other changes are inside media/. But here isn't media/. It's fine. I'll update the change to use net.
Updated the CL with TODO + net/ at Patch Set 2, and rebased at Patch Set 3. PTAL.
On 2015/04/02 14:59:03, Takashi Toyoshima (chromium) wrote: > Updated the CL with TODO + net/ at Patch Set 2, and rebased at Patch Set 3. > PTAL. LGTM
The CQ bit was checked by toyoshim@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1050063002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/78fb2d9a429de070b0d7c9c57710cffa52c4b52b Cr-Commit-Position: refs/heads/master@{#323555} |
