Repository navigation
Conversation
|
EWS run on previous version of this PR (hash 0977e58) Details |
8628979 to
f61d07f
Compare
|
EWS run on previous version of this PR (hash f61d07f) Details |
| #if ENABLE(WEBDRIVER_BIDI) | ||
| const WebDriverService::BidiCommand WebDriverService::s_bidiCommands[] = { | ||
| const WebDriverService::StaticBidiCommand WebDriverService::s_staticBidiCommands[] = { | ||
| { "session.new"_s, &WebDriverService::bidiSessionNew }, |
There was a problem hiding this comment.
Blocker: with this entry, a bare /session WebSocket can create a session, and nothing checks who is connecting. Browsers don't apply CORS to WebSockets, so any page open on the machine can do new WebSocket("ws://localhost:<port>/session"), send session.new, and then navigate, run script and read cookies in the automated browser. The soup handshake passes a null origin to soup_websocket_server_check_handshake, and acceptHandshake never sees the request headers. Before this stack you had to know the session UUID from the POST /session response.
Please reject a handshake that carries an Origin header we don't allow, and require Host to be a loopback name or IP literal so DNS rebinding doesn't get through. geckodriver and chromedriver both have an allowed-origins option for clients that need one. If you'd rather do this as a separate patch, that's fine as long as it lands before /session starts accepting session.new.
There was a problem hiding this comment.
I created a separate patch to take care of that issue: #77457
| void WebDriverService::bidiSessionStatus(unsigned id, RefPtr<JSON::Object>&&, Function<void(WebSocketMessageHandler::Message&&)>&& completionHandler) | ||
| void WebDriverService::bidiSessionNew(unsigned id, RefPtr<JSON::Object>&& parameters, WebSocketMessageHandler::Connection connection, Function<void(WebSocketMessageHandler::Message&&)>&& replyHandler) | ||
| { | ||
| if (m_session || m_sessionCreationInProgress) { |
There was a problem hiding this comment.
Blocker: once a BiDi-only session exists, nothing ever clears m_session for it, so this check (and POST /session) fails with "Maximum number of active sessions" until WebKitWebDriver restarts.
onBrowserTerminateddisconnects the sockets but leavesm_sessionset.- The HTTP-flag check in
handleRequestruns beforefindSessionOrCompleteWithError, so the!isConnected()cleanup that classic sessions get never runs. - We only call
startListening(id)for HTTP sessions, so a client whose socket drops can't reconnect via/session/<id>, and it can't sendsession.endeither.
The spec says closing a connection doesn't end the session, so I'm not asking you to end it on disconnect. But at minimum onBrowserTerminated should clear a BiDi-only session, and I'd like to know what the plan is for a dropped connection. Is there a way back in?
| return; | ||
|
|
||
| matchedCapabilitiesList.reverse(); | ||
| m_sessionCreationInProgress = true; |
There was a problem hiding this comment.
On GTK/WPE this flag can get stuck. SessionHostGlib keeps the startAutomationSession completion handler in m_startSessionCompletionHandler, and if the browser goes away before the target list arrives, disconnect() and inspectorDisconnected() never call it. The socket version does flush it. Before this patch that left one request hanging; now it leaves the service reporting not ready forever. Could you flush the pending start handler with an error in the GLib disconnect path, the way SessionHostSocket does?
| session->close([this, session, matchedCapabilitiesList, completionHandler = WTF::move(completionHandler)](CommandResult&& result) mutable { | ||
| ASSERT(m_replaceOnNewSession); | ||
| RELEASE_LOG(WebDriverClassic, "WebDriverService::newSession: Replacing existing session."); | ||
| auto replacedSession = std::exchange(m_session, nullptr); |
There was a problem hiding this comment.
With --replace-on-new-session, a classic POST /session will now close a BiDi-only session out from under its BiDi client. Is that intended? Either skip the replace when !m_session->hasHTTPFlag() or call it out in the commit message.
| auto parameters = JSON::Object::create(); | ||
| parameters->setString("browsingContextHandle"_s, uncheckedTopLevelBrowsingContext()); | ||
| parameters->setString("frameHandle"_s, m_currentBrowsingContext.value()); | ||
| parameters->setString("function"_s, "function() { return navigator.userAgent; }"_s); |
There was a problem hiding this comment.
Reading the user agent by evaluating navigator.userAgent in the page works, but the page can override it. Not a blocker since this runs on the fresh top-level context, but please add a FIXME with a bug for getting the UA from the browser side instead.
|
|
||
| private: | ||
| enum class HTTPMethod { Get, Post, Delete }; | ||
| enum class SessionCreationMode : bool { Http, BiDi }; |
There was a problem hiding this comment.
Nit: WebKit style capitalizes acronyms, so Http should be HTTP (it's already HasHTTPFlag in Session).
| void WebDriverService::buildBidiSessionResult(RefPtr<Session>&& session, SessionCreationCompletionHandler&& sessionCreationCompletionHandler) | ||
| { | ||
| ASSERT(session); | ||
| Function<void(CommandResult&&)> userAgentCompletionHandler = [session, finishSessionCreation = WTF::move(sessionCreationCompletionHandler)](CommandResult&& userAgentResult) mutable { |
There was a problem hiding this comment.
I feel like there is a bit of a race here between the page loading and userAgent becoming overridden, e.g., by Emulation domain commands, preload script, etc. It might be a better direction to include the applied userAgent as a used capability in the response to New Session. This represents what the UA was at the time of session minting. This is similar to browserName, browserVersion, which could be overridden at the WebKit API level after a session has been created.
f61d07f to
096c23a
Compare
|
EWS run on previous version of this PR (hash 096c23a) Details |
096c23a to
456c43b
Compare
|
EWS run on previous version of this PR (hash 456c43b) Details |
456c43b to
dd018cd
Compare
|
EWS run on previous version of this PR (hash dd018cd) Details |
dd018cd to
164ec0e
Compare
|
EWS run on previous version of this PR (hash 164ec0e) Details |
https://bugs.webkit.org/show_bug.cgi?id=281721 Reviewed by NOBODY (OOPS!). Classify incoming BiDi commands before requiring the connection to have an associated session. Handle static session.status commands locally, reject known session-bound commands with invalid session id when no session is associated, and reject unknown methods with unknown command. Generate the session-bound command catalog from WebKit's combined BiDi protocol schema instead of maintaining a second protocol-file list. This keeps classification in sync with the domains used by WebKit's BiDi backend and automatically includes newly added domains such as input. Admit connections to the default /session resource, preserve the incoming connection on responses, and make connection cleanup safe when no session exists. WebDriverBidiProcessor remains unchanged and receives only commands that pass the session gate. Regression coverage for static, session-bound, and unknown commands without an active session is submitted upstream in web-platform-tests/wpt#63212 and was exercised locally with WebKitWebDriver. * Source/WebDriver/CMakeLists.txt: Generate the session-bound BiDi command-name header from WebKit's existing combined BiDi protocol schema. * Source/WebDriver/WebDriverService.cpp: (WebDriver::WebDriverService::s_staticBidiCommands): Rename the local table to state that it contains static commands. (WebDriver::WebDriverService::acceptHandshake): Admit connections to /session without requiring an active session and make session-specific validation null-safe. (WebDriver::WebDriverService::handleMessage): Distinguish static, session-bound, and unknown commands before applying the session gate. (WebDriver::WebDriverService::findStaticBidiCommand): Look up a static command descriptor by method name. (WebDriver::WebDriverService::clientDisconnected): Clean up unassociated connections without dereferencing the current session. * Source/WebDriver/WebDriverService.h: Rename the local command descriptor and lookup to identify their static scope. * Source/WebKit/Scripts/generate-webdriver-bidi-command-names.py: Added. (parse_arguments): Parse the output and combined protocol schema paths. (command_names): Collect and validate fully qualified command names from the combined schema's domains. (generate_header): Generate the sorted command-name set. (main): Write the generated header.
164ec0e to
e1652ef
Compare
|
EWS run on previous version of this PR (hash e1652ef) Details |
e1652ef to
9416583
Compare
|
EWS run on previous version of this PR (hash 9416583) Details |
https://bugs.webkit.org/show_bug.cgi?id=281722 Reviewed by NOBODY (OOPS!). Web pages can initiate WebSocket connections without CORS enforcement, so accepting the static /session endpoint without validating its handshake would allow an arbitrary page to reach the local driver. Represent the handshake metadata explicitly and validate it before resource routing. Require Host and libsoup's effective request URI to identify the same normalized host, reject untrusted hosts, and default-deny requests carrying an Origin unless the exact origin was configured with --allowed-origins. Origin-less non-browser clients remain supported. Distinguish malformed handshakes, forbidden handshakes, and invalid resources so libsoup returns 400, 403, and 503 responses respectively. Add GLib integration coverage that launches the real WebDriver process and sends raw WebSocket upgrade requests. Cover allowed and rejected origins, malformed and disallowed hosts, request-target host mismatches, malformed handshakes, and invalid resources. * Source/WebDriver/WebDriverService.cpp: (WebDriver::parseOrigin): Added. (WebDriver::isForbiddenHostHeaderCharacter): Added. (WebDriver::parseHostHeader): Added. (WebDriver::parseEffectiveRequestURIHost): Added. (WebDriver::isIPAddressHost): Added. (WebDriver::WebDriverService::run): Parse and validate --allowed-origins. (WebDriver::WebDriverService::isHostAllowed): Added. (WebDriver::WebDriverService::acceptHandshake): Validate Host, Origin, the effective request URI, and the requested resource. * Source/WebDriver/WebDriverService.h: Declare the handshake policy helpers and configured-origin sets. * Source/WebDriver/WebSocketServer.h: Represent handshake metadata and rejection results explicitly. * Source/WebDriver/soup/WebSocketServerSoup.cpp: (WebDriver::sendHandshakeErrorResponse): Added. (WebDriver::handleIncomingHandshake): Populate handshake metadata and map rejection reasons to HTTP responses. * Tools/TestWebKitAPI/Tests/WebDriver/glib/TestWebSocketHandshake.cpp: Added. * Tools/TestWebKitAPI/glib/CMakeLists.txt: Register the WebSocket handshake integration tests when WebDriver BiDi is enabled.
https://bugs.webkit.org/show_bug.cgi?id=281722 Reviewed by NOBODY (OOPS!). Handle session.new as a static WebDriverService command. Reuse the existing session-creation pipeline in BiDi-only mode, build the required BiDi capabilities result, and associate the requesting WebSocket before replying. Reserve the single session slot across asynchronous HTTP and BiDi creation so overlapping requests cannot create multiple sessions. Validate capabilities before reporting that the slot is unavailable, consistently returning invalid argument for malformed HTTP and BiDi capability requests. Report automation- session startup failures as session not created, close provisional sessions after later creation failures, and keep sessions created through BiDi inaccessible to classic HTTP commands. Keep --replace-on-new-session protocol-independent. When enabled, a classic HTTP new-session request may replace an existing BiDi-only session. Reserve the shared session slot before detaching the old session so another creation cannot claim the temporary gap. Make session startup and the service-level creation finalizer one-shot. Complete a pending startup when the inspector disconnects or a locally launched browser exits before establishing its inspector connection. This ensures every terminal path releases the in-progress session reservation and replies once. Cancel an in-progress BiDi session creation when its initiating WebSocket disconnects, releasing the reservation without attempting to reply on the dead connection. When an active browser terminates, detach only the session whose ID matches the termination notification before cleaning up its WebSocket mapping, so replacement capacity is restored without exposing a newer session to stale cleanup. Register a reconnect resource for a successfully created BiDi-only session. Closing its transport does not end the session, so a client can reconnect to the same session ID. Remove the resource and connection mapping during definitive teardown. Revalidate resource ownership in the delayed post-handshake callback so teardown racing an accepted upgrade cannot attach a new connection to an ended session. Defer browser.close termination to the next main-run-loop turn. This lets the generated BiDi dispatcher send the command's empty success response before automation-session cleanup disconnects the inspector channel. Return the user agent applied by WebPageProxy with the initial browsing-context response. Store that value in the session capabilities and use it directly in the BiDi result, avoiding a second backend command and page-visible navigator.userAgent evaluation. * Source/WebDriver/Session.cpp: (WebDriver::Session::Session): Record whether the session is addressable through classic HTTP. (WebDriver::Session::createTopLevelBrowsingContext): Store the applied user agent returned with the initial context for BiDi session capabilities. * Source/WebDriver/Session.h: Expose the HTTP flag used by BiDi session creation. * Source/WebDriver/Capabilities.h: Store the applied user agent used by a BiDi session result. * Source/WebDriver/SessionHost.cpp: (WebDriver::SessionHost::completeStartAutomationSession): Added. Consume the pending startup completion from one common terminal path. (WebDriver::SessionHost::inspectorDisconnected): Fail a pending startup when the inspector connection closes. * Source/WebDriver/SessionHost.h: Use CompletionHandler for automation-session startup, store it for all platform implementations, expose creation cancellation, and retain the authoritative user agent in the session capabilities. * Source/WebDriver/WebDriverService.cpp: (WebDriver::WebDriverService::s_staticBidiCommands): Register session.new. (WebDriver::WebDriverService::handleRequest): Reject classic commands targeting a BiDi-only session. (WebDriver::WebDriverService::acceptHandshake): Reject a second active connection for the same session. (WebDriver::WebDriverService::handleMessage): Pass the initiating connection to static command handlers. (WebDriver::WebDriverService::newSession): Reserve the shared session slot during asynchronous HTTP creation. (WebDriver::WebDriverService::connectToBrowser): Propagate the creation mode and provisional session result. (WebDriver::WebDriverService::createSession): Share session startup between HTTP and BiDi, report startup failures as session not created, and clean up failed provisional sessions. (WebDriver::WebDriverService::buildBidiSessionResult): Added. (WebDriver::WebDriverService::status): Report an in-progress creation as unavailable. (WebDriver::WebDriverService::bidiSessionNew): Add BiDi-only session creation and validate capabilities before checking session capacity. (WebDriver::WebDriverService::bidiSessionStatus): Account for an in-progress creation. (WebDriver::WebDriverService::clientDisconnected): Cancel session creation owned by the disconnected WebSocket. (WebDriver::WebDriverService::onBrowserTerminated): Release the matching active session before disconnecting its WebSocket mapping. * Source/WebDriver/WebDriverService.h: (WebDriver::WebDriverService::SessionCreationMode): Name the classic WebDriver and BiDi session-creation paths. Declare the shared creation state and handlers. Use CompletionHandler for the terminal session-creation callback. * Source/WebDriver/WebSocketServer.cpp: (WebDriver::WebSocketServer::associateConnectionWithSession): Added. (WebDriver::WebSocketServer::addConnection): Revalidate reconnect-resource ownership and connection uniqueness immediately before inserting a post-handshake connection mapping. (WebDriver::WebSocketServer::startListening): Register session resources idempotently. * Source/WebDriver/WebSocketServer.h: Declare atomic association of an unassociated connection with a session and report whether a post-handshake connection was registered. * Source/WebDriver/glib/SessionHostGlib.cpp: (WebDriver::SessionHost::connectToBrowser): Complete with an error if the launched browser exits before its inspector connection is established, and complete cancelled connection attempts. (WebDriver::SessionHost::cancelSessionCreation): Added. (WebDriver::SessionHost::startAutomationSession): Store the one-shot startup completion. (WebDriver::SessionHost::didStartAutomationSession): Use the common completion path after a capability mismatch. (WebDriver::SessionHost::setTargetList): Use the common completion path after successful startup. * Source/WebDriver/socket/SessionHostSocket.cpp: (WebDriver::SessionHost::cancelSessionCreation): Added. (WebDriver::SessionHost::startAutomationSession): Store the one-shot startup completion. (WebDriver::SessionHost::setTargetList): Route disconnect and success through the common completion paths. * Source/WebDriver/soup/WebSocketServerSoup.cpp: (WebDriver::handleWebSocketConnection): Close connections that lose session ownership during the handshake callback gap. (WebDriver::WebSocketServer::disconnectSession): Remove the reconnect resource and mapping synchronously before closing the socket. * Source/WebKit/UIProcess/Automation/BidiBrowserAgent.cpp: (WebKit::BidiBrowserAgent::close): Schedule session termination after the generated success response has been sent. * Source/WebKit/UIProcess/Automation/BidiBrowsingContextAgent.cpp: (WebKit::BidiBrowsingContextAgent::create): Accept the user agent added to the shared browsing-context response while returning the context identifier. * Source/WebKit/UIProcess/Automation/Automation.json: Add the applied user agent to the createBrowsingContext result. * Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp: (WebKit::WebAutomationSession::createBrowsingContext): Return WebPageProxy::userAgent() with the newly created context. * Source/WebKit/UIProcess/Automation/WebAutomationSession.h: Update the generated createBrowsingContext callback signature. * Tools/TestWebKitAPI/Tests/WebKit/WKWebView/glib/TestAutomationSession.cpp: Verify that createBrowsingContext returns the configured WebKit user agent.
9416583 to
88c1847
Compare
|
EWS run on current version of this PR (hash 88c1847) Details |
🛠 ios
[WebDriver][BiDi] Support session.new to create BiDi-only sessions
https://bugs.webkit.org/show_bug.cgi?id=281722
Reviewed by NOBODY (OOPS!).
Depends on #77457.
Upstream WPT coverage: web-platform-tests/wpt#63552
Handle session.new as a static WebDriverService command. Reuse the existing
session-creation pipeline in BiDi-only mode, build the required BiDi
capabilities result, and associate the requesting WebSocket before replying.
Reserve the single session slot across asynchronous HTTP and BiDi creation so
overlapping requests cannot create multiple sessions. Validate capabilities
before reporting that the slot is unavailable, consistently returning invalid
argument for malformed HTTP and BiDi capability requests. Report automation-
session startup failures as session not created, close provisional sessions
after later creation failures, and keep sessions created through BiDi
inaccessible to classic HTTP commands.
Keep --replace-on-new-session protocol-independent. When enabled, a classic
HTTP new-session request may replace an existing BiDi-only session. Reserve the
shared session slot before detaching the old session so another creation cannot
claim the temporary gap.
Make session startup and the service-level creation finalizer one-shot. Complete
a pending startup when the inspector disconnects or a locally launched browser
exits before establishing its inspector connection. This ensures every
terminal path releases the in-progress session reservation and replies once.
Cancel an in-progress BiDi session creation when its initiating WebSocket
disconnects, releasing the reservation without attempting to reply on the dead
connection. When an active browser terminates, detach only the session whose ID
matches the termination notification before cleaning up its WebSocket mapping,
so replacement capacity is restored without exposing a newer session to stale
cleanup.
Register a reconnect resource for a successfully created BiDi-only session.
Closing its transport does not end the session, so a client can reconnect to
the same session ID. Remove the resource and connection mapping during
definitive teardown. Revalidate resource ownership in the delayed
post-handshake callback so teardown racing an accepted upgrade cannot attach a
new connection to an ended session.
Defer browser.close termination to the next main-run-loop turn. This lets the
generated BiDi dispatcher send the command's empty success response before
automation-session cleanup disconnects the inspector channel.
Return the user agent applied by WebPageProxy with the initial browsing-context
response. Store that value in the session capabilities and use it directly in
the BiDi result, avoiding a second backend command and page-visible
navigator.userAgent evaluation.
(WebDriver::Session::Session): Record whether the session is addressable
through classic HTTP.
(WebDriver::Session::createTopLevelBrowsingContext): Store the applied user
agent returned with the initial context for BiDi session capabilities.
Expose the HTTP flag used by BiDi session creation.
Store the applied user agent used by a BiDi session result.
(WebDriver::SessionHost::completeStartAutomationSession): Added. Consume the
pending startup completion from one common terminal path.
(WebDriver::SessionHost::inspectorDisconnected): Fail a pending startup when
the inspector connection closes.
Use CompletionHandler for automation-session startup, store it for all
platform implementations, expose creation cancellation, and retain the
authoritative user agent in the session capabilities.
(WebDriver::WebDriverService::s_staticBidiCommands): Register session.new.
(WebDriver::WebDriverService::handleRequest): Reject classic commands targeting
a BiDi-only session.
(WebDriver::WebDriverService::acceptHandshake): Reject a second active
connection for the same session.
(WebDriver::WebDriverService::handleMessage): Pass the initiating connection to
static command handlers.
(WebDriver::WebDriverService::newSession): Reserve the shared session slot
during asynchronous HTTP creation.
(WebDriver::WebDriverService::connectToBrowser): Propagate the creation mode and
provisional session result.
(WebDriver::WebDriverService::createSession): Share session startup between HTTP
and BiDi, report startup failures as session not created, and clean up failed
provisional sessions.
(WebDriver::WebDriverService::buildBidiSessionResult): Added.
(WebDriver::WebDriverService::status): Report an in-progress creation as
unavailable.
(WebDriver::WebDriverService::bidiSessionNew): Add BiDi-only session creation
and validate capabilities before checking session capacity.
(WebDriver::WebDriverService::bidiSessionStatus): Account for an in-progress
creation.
(WebDriver::WebDriverService::clientDisconnected): Cancel session creation
owned by the disconnected WebSocket.
(WebDriver::WebDriverService::onBrowserTerminated): Release the matching active
session before disconnecting its WebSocket mapping.
(WebDriver::WebDriverService::SessionCreationMode): Name the classic WebDriver
and BiDi session-creation paths.
Declare the shared creation state and handlers. Use CompletionHandler for the
terminal session-creation callback.
(WebDriver::WebSocketServer::associateConnectionWithSession): Added.
(WebDriver::WebSocketServer::addConnection): Revalidate reconnect-resource
ownership and connection uniqueness immediately before inserting a
post-handshake connection mapping.
(WebDriver::WebSocketServer::startListening): Register session resources
idempotently.
Declare atomic association of an unassociated connection with a session and
report whether a post-handshake connection was registered.
(WebDriver::SessionHost::connectToBrowser): Complete with an error if the
launched browser exits before its inspector connection is established, and
complete cancelled connection attempts.
(WebDriver::SessionHost::cancelSessionCreation): Added.
(WebDriver::SessionHost::startAutomationSession): Store the one-shot startup
completion.
(WebDriver::SessionHost::didStartAutomationSession): Use the common completion
path after a capability mismatch.
(WebDriver::SessionHost::setTargetList): Use the common completion path after
successful startup.
(WebDriver::SessionHost::cancelSessionCreation): Added.
(WebDriver::SessionHost::startAutomationSession): Store the one-shot startup
completion.
(WebDriver::SessionHost::setTargetList): Route disconnect and success through
the common completion paths.
(WebDriver::handleWebSocketConnection): Close connections that lose session
ownership during the handshake callback gap.
(WebDriver::WebSocketServer::disconnectSession): Remove the reconnect resource
and mapping synchronously before closing the socket.
(WebKit::BidiBrowserAgent::close): Schedule session termination after the
generated success response has been sent.
(WebKit::BidiBrowsingContextAgent::create): Accept the user agent added to the
shared browsing-context response while returning the context identifier.
Add the applied user agent to the createBrowsingContext result.
(WebKit::WebAutomationSession::createBrowsingContext): Return
WebPageProxy::userAgent() with the newly created context.
Update the generated createBrowsingContext callback signature.
Verify that createBrowsingContext returns the configured WebKit user agent.
88c1847
🛠 playstation