Similar to bug 1732622 which added this feature to geckodriver we need the same opt-in support for the Remote Agent.

With that feature added we might no longer need bug 1745892 and mochitests could just add the appropriate command line args for Firefox.

We should discuss how we want to get this implemented in terms of naming of the command line args. Maybe the following:

--remote-debugging-(allow-)hosts
--remote-debugging-(allow-)origins

We discussed if we should have it as command line arguments or better as preferences. Given that most of the configuration is done via preferences we agreed on to use this method for both allowed hosts and origins. It might be more complicated for clients given that a user.js file needs to be prepared but that is something clients would have to do anyway.

The preferences will then be read probably once when the Remote Agent starts and then checked for each connection attempt.

The preferences will look like:

remote.hosts.allowed
remote.origins.allowed

When using geckodriver the preferences could be automatically set based on its own command line arguments. I'll file a separate bug for this.

Bug 1751712 now handles the geckodriver specific changes.

I want to make sure we agree on the intent for this bug, since the summary is a bit outdated.

My understanding is that this is now about supporting remote.hosts.allowed and remote.origins.allowed in Remote Agent (ie in the host/origin validation logic in remote/server/WebSocketHandshake.jsm)? This bug should not offer any new way of setting those prefs and should only be about consuming the values.

Regarding the expected behavior, can I follow what is described in the summary at https://phabricator.services.mozilla.com/D129012 ?

1. for host validation:
   • remote.hosts.allowed is expected to be a char pref which should contain a comma separated value of hostnames
   • if remote.hosts.allowed is not set, then use the current host validation logic: we accept IP addresses and known loopbacks (localhost etc...)
   • if remote.hosts.allowed is set, then we only accept IP addresses and hostnames matching a value from remote.hosts.allowed (implies: we do NOT accept the usual loopback addresses in that case)
2. for origin validation:
   • remote.origins.allowed is expected to be a char pref which should contain a comma separated value of origins
   • if remote.origins.allowed is not set, then we only accept requests without an origin header
   • if remote.origins.allowed is set, then we accept requests without an origin header or requests with an origin header which has the same scheme, host and port as one of the values in remote.origins.allowed
3. preferences should be read everytime and not be cached

Regarding tests, I am currently expanding my test at testing/web-platform/mozilla/tests/webdriver/bidi/websocket_upgrade.py, and passing the remote.*.allowed preferences via moz:firefoxOptions capabilities. Is this ok, or do you have another suggestion.

Thanks!

(In reply to Julian Descottes [:jdescottes] from comment #3)

I want to make sure we agree on the intent for this bug, since the summary is a bit outdated.

AFAICS I don't think that the summary is outdated.

My understanding is that this is now about supporting remote.hosts.allowed and remote.origins.allowed in Remote Agent (ie in the host/origin validation logic in remote/server/WebSocketHandshake.jsm)? This bug should not offer any new way of setting those prefs and should only be about consuming the values.

That is not all what we have to take into account here. Note that we have two different methodologies in how a BiDi WebSocket connection could be invoked. The first one is via the opt-in from geckodriver, and here we get support by letting it parse both whitelists and set them as preferences. Means that we indeed could just read out the values and use them. But the second way are direct connections from some WebDriver BiDi clients. Here we cannot trust the already set values for both preferences and would have to do the same parsing as what geckodriver does.

Regarding the expected behavior, can I follow what is described in the summary at https://phabricator.services.mozilla.com/D129012 ?

1. for host validation:
   • remote.hosts.allowed is expected to be a char pref which should contain a comma separated value of hostnames
   • if remote.hosts.allowed is not set, then use the current host validation logic: we accept IP addresses and known loopbacks (localhost etc...)

I wonder if we should define a default value or not. Probably it's fine to not do that and let the client decide. As such we don't have any other host than loopback and no other origin as none to care about.

- if `remote.hosts.allowed` is set, then we only accept IP addresses and hostnames matching a value from `remote.hosts.allowed` (implies: we do NOT accept the usual loopback addresses in that case)

We always have to accept loopback addresses. It should be automatically added to the internal Array/Set of hosts.

1. for origin validation:
   • remote.origins.allowed is expected to be a char pref which should contain a comma separated value of origins
   • if remote.origins.allowed is not set, then we only accept requests without an origin header
   • if remote.origins.allowed is set, then we accept requests without an origin header or requests with an origin header which has the same scheme, host and port as one of the values in remote.origins.allowed

This is correct AFAIC.

1. preferences should be read everytime and not be cached

I would do it once with a lazy getter when the first connection is created. Whenever it turns out that we have to do it each time we could easily remove the lazy getter. But for now lets not do it. Or we allow it via another pref, which might help potential tests to not have to restart Firefox for each setting.

Regarding tests, I am currently expanding my test at testing/web-platform/mozilla/tests/webdriver/bidi/websocket_upgrade.py, and passing the remote.*.allowed preferences via moz:firefoxOptions capabilities. Is this ok, or do you have another suggestion.

I think that sounds fine. I only wonder how to test that geckodriver sets the correct preference values. If we cannot include this in this bug we can file a follow-up.

James, is there anything that I missed?

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #5)

(In reply to Julian Descottes [:jdescottes] from comment #3)

I want to make sure we agree on the intent for this bug, since the summary is a bit outdated.

AFAICS I don't think that the summary is outdated.

My understanding is that this is now about supporting remote.hosts.allowed and remote.origins.allowed in Remote Agent (ie in the host/origin validation logic in remote/server/WebSocketHandshake.jsm)? This bug should not offer any new way of setting those prefs and should only be about consuming the values.

That is not all what we have to take into account here. Note that we have two different methodologies in how a BiDi WebSocket connection could be invoked. The first one is via the opt-in from geckodriver, and here we get support by letting it parse both whitelists and set them as preferences. Means that we indeed could just read out the values and use them. But the second way are direct connections from some WebDriver BiDi clients. Here we cannot trust the already set values for both preferences and would have to do the same parsing as what geckodriver does.

I think the logic should just be:

• If the pref values are set we use the set values
• If the pref values are not set we use default values similar to geckodriver.

In any case I think we should use what's in the profile (nor not in the profile), independent of the entrypoint (i.e. the only difference between BiDi-only and via-geckodriver is that geckodriver will always set pref values in the profile).

Regarding the expected behavior, can I follow what is described in the summary at https://phabricator.services.mozilla.com/D129012 ?

1. for host validation:
   • remote.hosts.allowed is expected to be a char pref which should contain a comma separated value of hostnames
   • if remote.hosts.allowed is not set, then use the current host validation logic: we accept IP addresses and known loopbacks (localhost etc...)

I wonder if we should define a default value or not. Probably it's fine to not do that and let the client decide. As such we don't have any other host than loopback and no other origin as none to care about.

- if `remote.hosts.allowed` is set, then we only accept IP addresses and hostnames matching a value from `remote.hosts.allowed` (implies: we do NOT accept the usual loopback addresses in that case)

We always have to accept loopback addresses. It should be automatically added to the internal Array/Set of hosts.

That's not quite how it works for geckodriver. For geckodriver, if there's an explicitly set list of hosts we set allow_hosts to that value. Otherwise if we bound the server to a hostname (not ip address) we set it to a list containing that hostname (https://searchfox.org/mozilla-central/source/testing/geckodriver/src/main.rs#208-223). Otherwise, if we bound the server to an IP address, and localhost is a loopback address that resolves to that server IP address, we set allow_hosts to a list containing localhost (https://searchfox.org/mozilla-central/source/testing/geckodriver/src/main.rs#190).

In the webdriver crate we only allow hostnames that match the allow_hosts list passed in from geckodriver, but we also allow any ip address in the host field (https://searchfox.org/mozilla-central/source/testing/webdriver/src/server.rs#302).

We also check that the port matches.

1. for origin validation:
   • remote.origins.allowed is expected to be a char pref which should contain a comma separated value of origins
   • if remote.origins.allowed is not set, then we only accept requests without an origin header
   • if remote.origins.allowed is set, then we accept requests without an origin header or requests with an origin header which has the same scheme, host and port as one of the values in remote.origins.allowed

This is correct AFAIC.

I also think this is correct.

1. preferences should be read everytime and not be cached

I would do it once with a lazy getter when the first connection is created. Whenever it turns out that we have to do it each time we could easily remove the lazy getter. But for now lets not do it. Or we allow it via another pref, which might help potential tests to not have to restart Firefox for each setting.

I don't have a usecase in mind for setting this at runtime other than our own tests. Given that I think I'd default to making it read once at server startup, just because that seems like a more conservative option.

(In reply to James Graham [:jgraham] from comment #6)

Thanks for the corrections James. Just one more thing...

In any case I think we should use what's in the profile (nor not in the profile), independent of the entrypoint (i.e. the only difference between BiDi-only and via-geckodriver is that geckodriver will always set pref values in the profile).

Should we really blindly accept any random values as entered for these prefs? What about IP adsresses vs domain names? I don't think that we want to have complex checks each time a client wants to connect.

At a technical level I think we should parse them as URLs (by prefixing an arbitary scheme for the hosts case) and only accept things that parse correctly. Given we will accept any IP address in the host field I don't think that hostname vs ip address in the prefs value is a big concern. But I'm not sure I understand what the bigger issue with trusting the pref value is (assuming a default profile has the prefs unset). What case are you worried about where the pref value might not be what we want to use?

Had a quick discussion with Henrik, because I was not sure if we needed another entry point than preferences for the list of allowed hosts/origins. As far as I understand, we agree that the allowed hosts/origins will be set via the remote.hosts.allowed & remote.origins.allowed preferences. But there is a concern for direct connections, because preferences have not been validated, whereas geckodriver performs some validation. Since we have no way to know if the preferences have been set by geckodriver or by another way, we should try to apply the same kind of validation in WebSocketHandshake.jsm as what is done in geckodriver.

Now based on James' comment #6, it seems that geckodriver does not perform much validation on the custom values provided for hosts and origin:

For geckodriver, if there's an explicitly set list of hosts we set allow_hosts to that value

So to be clear, if someone passes "--allow-hosts somerandomvalue" to geckodriver, geckodriver will set remote.hosts.allowed to somerandomvalue, without performing any check on "somerandomvalue". So I don't think that for custom values it makes a lot of differences whether they come from geckodriver or from something else?

It's rather for the "default" scenario (when no custom values are set) that geckodriver has a lot more logic than our current implementation in WebSocketHandshake.jsm:

if we bound the server to a hostname (not ip address) we set it to a list containing that hostname
if we bound the server to an IP address, and localhost is a loopback address that resolves to that server IP address, we set allow_hosts to a list containing localhost

Those rules could replace the current logic in WebSocketHandshake.jsm (which is to accept IP addresses + loopback).

Which is basically paraphrasing what James said earlier:

I think the logic should just be:

• If the pref values are set we use the set values
• If the pref values are not set we use default values similar to geckodriver.

Hoping that it's an acceptable definition for this bug.

Depends on D137773

https://hg.mozilla.org/mozilla-central/rev/65325e62a37f
https://hg.mozilla.org/mozilla-central/rev/d8110d104677