Disable DevTools remote debugging for default user data dir [chromium/src : main]
Will Harris (Gerrit)
Will Harris added 1 comment![]( "Open in Gerrit")
hi caseq - here is my first stab at the remote debugging port mitigations. it wasn't clear to me where to add it. I did not add it in the same way as the enterprise policy becasue I wanted to emit a helpful error rather than not have the port open at all.
what do you think? Also, I wasn't sure how to test so any help on where to add a fixture would be appreciated.
Related details
- Andrey Kosyakov
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrey Kosyakov (Gerrit)
Andrey Kosyakov added 1 comment![]( "Open in Gerrit")
if (!base::FeatureList::IsEnabled(features::kDevToolsDebuggingRestrictions)) {This is rather unexpected way to implement it ;-) Besides, I don't think it fundamentally limits what can be done over the remote CDP, as it only affects listing of existent targets. Why not just implement it where we [instantiate the HTTP server](https://source.chromium.org/chromium/chromium/src/+/main:content/browser/devtools/devtools_http_handler.h;bpv=1;bpt=1) (and the pipe one nearby)?
Related details
- Will Harris
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Will Harris (Gerrit)
Will Harris added 1 comment![]( "Open in Gerrit")
if (!base::FeatureList::IsEnabled(features::kDevToolsDebuggingRestrictions)) {This is rather unexpected way to implement it ;-) Besides, I don't think it fundamentally limits what can be done over the remote CDP, as it only affects listing of existent targets. Why not just implement it where we [instantiate the HTTP server](https://source.chromium.org/chromium/chromium/src/+/main:content/browser/devtools/devtools_http_handler.h;bpv=1;bpt=1) (and the pipe one nearby)?
Yes I considered doing it that way but I wanted to provide some useful info to folks trying to debug why it wasn't working.
How does this not limit the CDP targets, if you can't enumerate them? Or are the targets predictable in some way?
Happy to do it any way you feel is most appropriate, this was just a first attempt :)
Related details
- Andrey Kosyakov
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrey Kosyakov (Gerrit)
Andrey Kosyakov added 1 comment![]( "Open in Gerrit")
if (!base::FeatureList::IsEnabled(features::kDevToolsDebuggingRestrictions)) {Will HarrisThis is rather unexpected way to implement it ;-) Besides, I don't think it fundamentally limits what can be done over the remote CDP, as it only affects listing of existent targets. Why not just implement it where we [instantiate the HTTP server](https://source.chromium.org/chromium/chromium/src/+/main:content/browser/devtools/devtools_http_handler.h;bpv=1;bpt=1) (and the pipe one nearby)?
Yes I considered doing it that way but I wanted to provide some useful info to folks trying to debug why it wasn't working.
How does this not limit the CDP targets, if you can't enumerate them? Or are the targets predictable in some way?
Happy to do it any way you feel is most appropriate, this was just a first attempt :)
So first of all, I think disabling target list on this level will also interfere with legitimate in-browser debugging use cases -- from chrome://inspect to (potentially, I haven't checked) -- the front-end itself.
As for limiting just getting the list -- a client would still be able to create a new one and navigate wherever they want? Also, there's an auto-attach on the browser level. tl/dr, why not control the actual endpoint -- i.e. the web server and pipe -- rather than some of underlying functionality?
Happy to chat about this if you like!
Related details
- Will Harris
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Will Harris (Gerrit)
Will Harris added 1 comment![]( "Open in Gerrit")
if (!base::FeatureList::IsEnabled(features::kDevToolsDebuggingRestrictions)) {Will HarrisThis is rather unexpected way to implement it ;-) Besides, I don't think it fundamentally limits what can be done over the remote CDP, as it only affects listing of existent targets. Why not just implement it where we [instantiate the HTTP server](https://source.chromium.org/chromium/chromium/src/+/main:content/browser/devtools/devtools_http_handler.h;bpv=1;bpt=1) (and the pipe one nearby)?
Andrey KosyakovYes I considered doing it that way but I wanted to provide some useful info to folks trying to debug why it wasn't working.
How does this not limit the CDP targets, if you can't enumerate them? Or are the targets predictable in some way?
Happy to do it any way you feel is most appropriate, this was just a first attempt :)
So first of all, I think disabling target list on this level will also interfere with legitimate in-browser debugging use cases -- from chrome://inspect to (potentially, I haven't checked) -- the front-end itself.
As for limiting just getting the list -- a client would still be able to create a new one and navigate wherever they want? Also, there's an auto-attach on the browser level. tl/dr, why not control the actual endpoint -- i.e. the web server and pipe -- rather than some of underlying functionality?
Happy to chat about this if you like!
yes happy to chat but I did test in-browser debugging including chrome://inspect and it seemed to work, but maybe I didn't test hard enough.
I'm happy to take an alternative simpler approach but I do want there to be some way for a developer to reason about why the debugging is not working e.g. an error message they can bing for, or something like that.
and yes, let's chat
Related details
- Andrey Kosyakov
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Will Harris (Gerrit)
Will Harris added 2 comments![]( "Open in Gerrit")
hi caseq - here is my first stab at the remote debugging port mitigations. it wasn't clear to me where to add it. I did not add it in the same way as the enterprise policy becasue I wanted to emit a helpful error rather than not have the port open at all.
what do you think? Also, I wasn't sure how to test so any help on where to add a fixture would be appreciated.
Acknowledged
if (!base::FeatureList::IsEnabled(features::kDevToolsDebuggingRestrictions)) {Will HarrisThis is rather unexpected way to implement it ;-) Besides, I don't think it fundamentally limits what can be done over the remote CDP, as it only affects listing of existent targets. Why not just implement it where we [instantiate the HTTP server](https://source.chromium.org/chromium/chromium/src/+/main:content/browser/devtools/devtools_http_handler.h;bpv=1;bpt=1) (and the pipe one nearby)?
Andrey KosyakovYes I considered doing it that way but I wanted to provide some useful info to folks trying to debug why it wasn't working.
How does this not limit the CDP targets, if you can't enumerate them? Or are the targets predictable in some way?
Happy to do it any way you feel is most appropriate, this was just a first attempt :)
Will HarrisSo first of all, I think disabling target list on this level will also interfere with legitimate in-browser debugging use cases -- from chrome://inspect to (potentially, I haven't checked) -- the front-end itself.
As for limiting just getting the list -- a client would still be able to create a new one and navigate wherever they want? Also, there's an auto-attach on the browser level. tl/dr, why not control the actual endpoint -- i.e. the web server and pipe -- rather than some of underlying functionality?
Happy to chat about this if you like!
yes happy to chat but I did test in-browser debugging including chrome://inspect and it seemed to work, but maybe I didn't test hard enough.
I'm happy to take an alternative simpler approach but I do want there to be some way for a developer to reason about why the debugging is not working e.g. an error message they can bing for, or something like that.
and yes, let's chat
simplified this code to not listen on a port at all.
Related details
- Andrey Kosyakov
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrey Kosyakov (Gerrit)
Andrey Kosyakov added 3 comments![]( "Open in Gerrit")
Mostly looks good, but here go a couple of suggestions.
if (base::FeatureList::IsEnabled(features::kDevToolsDebuggingRestrictions) &&just `return !(base::FeatureList::IsEnabled(features::kDevToolsDebuggingRestrictions) && is_default_user_data_dir.value_or(true));`, please.
RemoteDebuggingServer::RemoteDebuggingServer() {Mind doing a bit of refacotring while you're here? It's a bit [unfortunate](https://google.github.io/styleguide/cppguide.html#Doing_Work_in_Constructors) to have a bunch of non-trivial logic in the constructor, especially one that can fail. And then failing without any indication to the caller is also a bit fishy. Can we extract a factory method instead, and return null to indicate we decided not to start the server? This way we would potentially be able to propagate it to higher levels -- and perhaps even bail out from chrome in case this failed, so it's easier for the automation driver to sport the problem? Not really sure about the last part, but at least it's nice to have an option.
Related details
- Will Harris
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Will Harris (Gerrit)
Will Harris added 3 comments![]( "Open in Gerrit")
will set back to WIP until the other dependent CL lands.
if (base::FeatureList::IsEnabled(features::kDevToolsDebuggingRestrictions) &&just `return !(base::FeatureList::IsEnabled(features::kDevToolsDebuggingRestrictions) && is_default_user_data_dir.value_or(true));`, please.
Done. the weird format was because I was previously #if defing on official builds :)
RemoteDebuggingServer::RemoteDebuggingServer() {Mind doing a bit of refacotring while you're here? It's a bit [unfortunate](https://google.github.io/styleguide/cppguide.html#Doing_Work_in_Constructors) to have a bunch of non-trivial logic in the constructor, especially one that can fail. And then failing without any indication to the caller is also a bit fishy. Can we extract a factory method instead, and return null to indicate we decided not to start the server? This way we would potentially be able to propagate it to higher levels -- and perhaps even bail out from chrome in case this failed, so it's easier for the automation driver to sport the problem? Not really sure about the last part, but at least it's nice to have an option.
I'll do this in a separate CL then rebase this one on top of that.
Related details
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrey Kosyakov (Gerrit)
Andrey Kosyakov voted and added 1 comment![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Will Harris (Gerrit)
Will Harris added 2 comments![]( "Open in Gerrit")
ptal second try.
do you know best place to add tests for this? I can enable the feature and then use a non-custom user data dir (somehow) and it should disable debugging.
Will HarrisMind doing a bit of refacotring while you're here? It's a bit [unfortunate](https://google.github.io/styleguide/cppguide.html#Doing_Work_in_Constructors) to have a bunch of non-trivial logic in the constructor, especially one that can fail. And then failing without any indication to the caller is also a bit fishy. Can we extract a factory method instead, and return null to indicate we decided not to start the server? This way we would potentially be able to propagate it to higher levels -- and perhaps even bail out from chrome in case this failed, so it's easier for the automation driver to sport the problem? Not really sure about the last part, but at least it's nice to have an option.
I'll do this in a separate CL then rebase this one on top of that.
- Andrey Kosyakov
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Will Harris (Gerrit)
Will Harris added 1 comment![]( "Open in Gerrit")
hi this is ready for review again. I landed a CL that makes testing of this possible and added a set of tests to verify both the feature and the user data dir behavior. PTAL
Related details
- Andrey Kosyakov
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Will Harris (Gerrit)
Will Harris added 1 comment![]( "Open in Gerrit")
yangguo -> metrics change to keep data consistent for monitoring
avi -> CreateDevToolsProtocolHandler handler in chrome
PTAL :)
Related details
- Andrey Kosyakov
- Avi Drissman
- Yang Guo
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrey Kosyakov (Gerrit)
Andrey Kosyakov voted and added 3 comments![]( "Open in Gerrit")
Votes added by Andrey Kosyakov
| Code-Review | +1 |
3 comments
Thanks, re-stamping with nits
"\nDevTools remote debugging requires a non-standard data directory. "_a non-default user data directory_?
ASSERT_FALSE(RunExtensionTest("discovery_page"));s/ASSERT/EXPECT?
Related details
- Avi Drissman
- Will Harris
- Yang Guo
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Will Harris (Gerrit)
Will Harris voted and added 3 comments![]( "Open in Gerrit")
Votes added by Will Harris
| Commit-Queue | +1 |
3 comments
ty for review.
Danil reviewed this original histograms so has more context so switching reviewers (no slight intended, Yang).
"\nDevTools remote debugging requires a non-standard data directory. "_a non-default user data directory_?
ah yes good spot.
ASSERT_FALSE(RunExtensionTest("discovery_page"));Will Harriss/ASSERT/EXPECT?
Done
Related details
- Avi Drissman
- Danil Somsikov
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Danil Somsikov (Gerrit)
Danil Somsikov voted Code-Review+1![]( "Open in Gerrit")
- Avi Drissman
- Will Harris
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Avi Drissman (Gerrit)
Avi Drissman voted and added 1 comment![]( "Open in Gerrit")
Votes added by Avi Drissman
| Code-Review | +1 |
1 comment
Related details
- Will Harris
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Will Harris (Gerrit)
Will Harris voted and added 2 comments![]( "Open in Gerrit")
Votes added by Will Harris
| Commit-Queue | +2 |
2 comments
ty all for reviews
Good luck!
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Disable DevTools remote debugging for default user data dir
With this CL, on Linux, Windows, macOS if a default user data dir is
being used then the remote debugging server no longer starts but emits
an error message to stderr.
A feature DevToolsDebuggingRestrictions is added to control this
behavior that is enabled by default for Google Chrome builds but could
be used to disable the protection if widespread incompatibilities are
encountered.
As part of this CL, the enterprise policy message to stderr is now only
shown if remote debugging is requested.
BUG=371033189,401281052
- M chrome/browser/browser_process_impl.cc
- M chrome/browser/devtools/devtools_browsertest.cc
- M chrome/browser/devtools/features.cc
- M chrome/browser/devtools/features.h
- M chrome/browser/devtools/remote_debugging_server.cc
- M chrome/browser/devtools/remote_debugging_server.h
- M tools/metrics/histograms/metadata/dev/enums.xml
- M tools/metrics/histograms/metadata/dev/histograms.xml
Code-Review: +1 by Andrey Kosyakov, +1 by Avi Drissman, +1 by Danil Somsikov
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Will Harris (Gerrit)
Will Harris added 1 comment![]( "Open in Gerrit")
See https://developer.chrome.com/blog/remote-debugging-port for more info on this change.
Related details
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |