mozilla / geckodriver

WebDriver for Firefox
https://firefox-source-docs.mozilla.org/testing/geckodriver/
Mozilla Public License 2.0
7.14k stars 1.52k forks source link

Connections from local IP address (127.0.0.1, ::1) aren't allowed with `localhost` as allowed origin #2048

Open titusfortner opened 2 years ago

titusfortner commented 2 years ago

This works locally, but something isn't working when executed with the Grid. It seems to have been failing in v105 beta, passing in v105.0, then failing again in v105.0.1 if that narrows things down a bit.

I think this is the line from the log that matters:

JavaScript error: chrome://remote/content/server/WebSocketHandshake.jsm, line 174: Error: The handshake request has incorrect Origin header http://127.0.0.1:52213

The fix might be in Selenium, but the change requiring it was in Firefox.

System

Testcase

options = Selenium::WebDriver::Options.firefox(debugger_address: true)
url = 'http://localhost:4444/wd/hub
driver = WebDriver::Driver.for(:remote, url: url, options: options)
driver.devtools.page.navigate(url: url_for('xhtmlTest.html'))

Stacktrace

     EOFError:
       end of file reached
     # ./lib/selenium/webdriver/common/websocket_connection.rb:72:in `readpartial'
     # ./lib/selenium/webdriver/common/websocket_connection.rb:72:in `process_handshake'
     # ./lib/selenium/webdriver/common/websocket_connection.rb:36:in `initialize'
     # ./lib/selenium/webdriver/devtools.rb:32:in `new'
     # ./lib/selenium/webdriver/devtools.rb:32:in `initialize'
     # ./lib/selenium/webdriver/common/driver_extensions/has_devtools.rb:36:in `new'
     # ./lib/selenium/webdriver/common/driver_extensions/has_devtools.rb:36:in `devtools'

Trace-level log

https://gist.github.com/titusfortner/d0e58b8d9d57b05136a39069d6823ada

whimboo commented 2 years ago

As given by the additionally provided logs it looks like that the WebSocket client sets an origin but geckodriver was called without the --allow-origins argument. If a client sets an origin then it also needs to be explicitly allowed, or the Origin header needs to be removed for the connection request.

Can you please verify that? I assume that this is an invalid use case.

pujagani commented 2 years ago

Removing the Origin header currently is not possible from the Grid and Java bindings because netty sends one by default https://github.com/SeleniumHQ/selenium/issues/10348#issue-1128980911.

whimboo commented 2 years ago

@pujagani so are you able to easily use the --allow-origins argument for geckodriver in case of Selenium Grid?

pujagani commented 2 years ago

So based on my attempts I found calling the geckodriver with --allow-origins tricky/not possible. --allow-origins need the exact address with host and port. However, I did not find a way to access the remote-debugger-port when creating the FirefoxDriver instance in Selenium.

 FirefoxOptions options = new FirefoxOptions();
    options.setCapability("webSocketUrl", true);
    options.setLogLevel(FirefoxDriverLogLevel.TRACE);
    options.addArguments("--remote-debugging-port", "45411");
    // This needs the exact port but setting --remote-debugging-port is not allowed. 
    options.addArguments("-allow-origins", "http://127.0.0.1:45411/");

    FirefoxDriver driver = new FirefoxDriver(options);

    CompletableFuture<LogEntry> future = new CompletableFuture<>();

    BiDi biDi = driver.getBiDi();

I might be missing something here.

pujagani commented 2 years ago

Enabling the trace logs has given me some insights. Sharing the relevant logs below:

1664284222266   mozrunner::runner   INFO    Running command: "/Applications/Firefox Nightly.app/Contents/MacOS/firefox-bin" "--marionette" "--remote-debugging-port" "63406" "--remote-allow-hosts" "localhost" "--remote-allow-origins" "http://localhost:63406/" "-foreground" "-no-remote" "-profile" "/var/folders/gf/wjxc6bv113b08xm754y0x0_h0000gp/T/rust_mozprofilenGpVan"

1664284223670   RemoteAgent TRACE   Received observer notification final-ui-startup
Read port: 63410
WebDriver BiDi listening on ws://127.0.0.1:63406

1664284227079   RemoteAgent INFO    Perform WebSocket upgrade for incoming connection from 127.0.0.1:63423
1664284227079   RemoteAgent DEBUG   Incorrect Origin header, allowed origins: [http://localhost:63406/]
JavaScript error: chrome://remote/content/server/WebSocketHandshake.jsm, line 174: Error: The handshake request has incorrect Origin header http://127.0.0.1:63406
Sept 27, 2022 6:40:27 PM org.openqa.selenium.remote.http.WebSocket$Listener onError
WARNING: Invalid Status code=400 text=Bad Request

Based on the logs above, Selenium simply uses what it receives from the geckodriver to create the websocket connection. I understand it is a Selenium constraint that the AsyncHttpClient we use internally uses netty that has a default Origin header set.

One thing to notice here is RemoteAgent DEBUG Incorrect Origin header, allowed origins: [http://localhost:63406/]. The error happens because Selenium is forwarding what is received i.e. host 127.0.0.1 but the RemoteAgent expects localhost. This I think is due to the default "--remote-allow-origins" "http://localhost:63406/" value set in the geckodriver that cannot be set by the client. Before 105, since localhost was received and sent by Selenium and it was expected by geckodriver, websocket connection did not see this error.

One solution is to fix it from Selenium Java bindings by ensuring we send the expected localhost since the "Origin" header cannot be removed and seems specific to us. This should not occur once we move to Java 11 and use the Java 11 HttpClient.

whimboo commented 2 years ago

You don't need the origin from the remote end but from the machine that tries to connect to geckodriver. Lets take the following example: https://juliandescottes.github.io/bidi-web-client/web/.

Here you would have to specify https://juliandescottes.github.io as allowed origin when using geckodriver. So what does netty set as Origin header in such a case?

pujagani commented 2 years ago

Thank you for having a look at the comments and providing an example. In this case, AsyncHttpClient sets it as "origin" -> https://juliandescottes.github.io.

I realized it is not using the underlying Netty's implementation to do it but netty also does something similar.

whimboo commented 2 years ago

I don't know that much about Selenium Grid. So it would be good if you might have a chance to show the network package that gets actually send to the remote end to connect to the WebSocket address? I'm interested to know which Origin gets exactly set.

titusfortner commented 2 years ago

Figured out how to add both localhost and 127.0.0.1 to the allow-origins flag - https://github.com/SeleniumHQ/selenium/commit/96c4ecd714148c8378a14fe4339430f3f7ace4c5

whimboo commented 2 years ago

Hm, why do you have to add both? That shouldn't be necessary if localhost resolves to 127.0.0.1. Would it enough for you to only use 127.0.0.1? Maybe we do no longer correctly resolve localhost in Remote Agent?

jgraham commented 2 years ago

No, an IP origin is distinct from a named origin (consider that with SNI evil.com could share a public ip with good.org). We could special-case localhost and automatically allow local ips when localhost is specified as an allowed origin, but it wouldn't generalise to other origins.

whimboo commented 2 years ago

Sure! My last comment was only about the local network interface and nothing else. Note that localhost could also be resolved to [::1] and then the client would be forced to also add that origin.

titusfortner commented 2 years ago

it might be enough to only use 127.0.0.1, I do not know why we chose localhost to begin with, so I added instead of replacing.

whimboo commented 2 years ago

@titusfortner will any of the Selenium grid systems also work with IPv6 only? If yes and to be safe you might also want to add [::1] as allowed origin then.

whimboo commented 2 years ago

Btw. geckodriver has the same issue but I assume that your code is different from what is used for the WebSocket:

➜  curl http://127.0.0.1:4444/session -X POST -H "Origin: http://localhost:4444"
{"value":{"error":"unknown error","message":"Invalid Origin header http://localhost:4444","stacktrace":""}}

I've filed https://bugzilla.mozilla.org/show_bug.cgi?id=1792647

pujagani commented 2 years ago

Thank you @whimboo for the insights, help, and filing an issue to fix this in geckodriver.