[CLEAN] Synthetic Benchmark PR #48512 - feat: support WebSocket authentication handling

[tomerqodo]

User description

Benchmark PR electron#48512

Type: Clean (correct implementation)

Original PR Title: feat: support WebSocket authentication handling
Original PR Description: #### Description of Change

Refs electron#48505.

This PR adds handling for the 'login' event to allow authentication via web sockets.

Checklist

• PR description included and stakeholders cc'd
• npm test passes
• tests are changed or added
• relevant API documentation, tutorials, and examples are updated and follow the documentation style guide
• PR release notes describe the change in a way relevant to app developers, and are capitalized, punctuated, and past tense.

Release Notes

Notes: Added support for WebSocket authentication through the login event on webContents.
Original PR URL: electron#48512

---

PR Type

Enhancement

---

Description

• Add WebSocket authentication handling via login event

• Implement OnAuthRequired method in WebRequest for async auth

• Move AuthRequiredResponse enum from ProxyingWebSocket to WebRequest

• Add LoginHandler constructor overload for WebSocket auth flow

• Add comprehensive test for WebSocket authentication

---

Diagram Walkthrough

flowchart LR
  WS["WebSocket Request"] -->|Auth Challenge| PW["ProxyingWebSocket"]
  PW -->|OnAuthRequired| WR["WebRequest"]
  WR -->|Create LoginHandler| LH["LoginHandler"]
  LH -->|Emit login event| WC["WebContents"]
  WC -->|Credentials via callback| LH
  LH -->|OnLoginAuthResult| WR
  WR -->|Resume auth flow| PW
  PW -->|Send credentials| WS

Loading

File Walkthrough

	Relevant files
Enhancement	electron_api_web_request.h Add auth handling interface and types                                        shell/browser/api/electron_api_web_request.h Define AuthRequiredResponse enum with four states for auth handling Add AuthCallback type alias for async auth completion Add OnAuthRequired method to handle authentication challenges Add OnLoginAuthResult method to bridge LoginHandler callbacks Update MatchesRequest to accept const WebRequestInfo pointer +28/-1    electron_api_web_request.cc Implement WebSocket auth handling logic                                    shell/browser/api/electron_api_web_request.cc Add includes for , utf_string_conversions, and login_handler.h Implement OnAuthRequired method to create LoginHandler and store blocked request Implement OnLoginAuthResult callback to process auth credentials Add auth_callback and login_handler fields to BlockedRequest struct Update method signatures to use const WebRequestInfo pointers +61/-2    login_handler.h Add WebSocket-specific LoginHandler constructor                    shell/browser/login_handler.h Add new constructor overload with simplified parameters for WebSocket auth New constructor delegates to existing constructor with default values Enables LoginHandler reuse for both traditional and WebSocket auth flows +7/-0      login_handler.cc Implement WebSocket LoginHandler constructor                          shell/browser/login_handler.cc Implement new LoginHandler constructor overload Constructor delegates to main constructor with hardcoded defaults Supports WebSocket authentication without navigation/frame context +17/-0    proxying_websocket.h Remove duplicate AuthRequiredResponse enum                              shell/browser/net/proxying_websocket.h Remove AuthRequiredResponse enum definition (moved to WebRequest) Update OnAuthRequiredComplete signature to use api::WebRequest::AuthRequiredResponse Consolidate auth response handling in WebRequest API +1/-16    proxying_websocket.cc Integrate WebRequest auth handling in WebSocket                    shell/browser/net/proxying_websocket.cc Update OnAuthRequiredComplete to use api::WebRequest::AuthRequiredResponse Update enum case references to use new namespaced enum values Implement OnHeadersReceivedCompleteForAuth to call OnAuthRequired Handle AUTH_REQUIRED_RESPONSE_IO_PENDING to pause processing +15/-6
Tests	api-web-request-spec.ts Add WebSocket authentication test case                                      spec/api-web-request-spec.ts Add comprehensive test for WebSocket authentication via login event Test creates auth server requiring Basic authentication Verifies credentials are properly supplied through login event callback Tests retry logic and successful WebSocket connection after auth +75/-0

---

[qodo-code-review]

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
⚪	Use-after-free risk Description: The LoginHandler is constructed with a raw WebContents pointer obtained from a potentially null RenderFrameHost, and the object is stored in a BlockedRequest while using base::Unretained(this) in the callback chain, which risks use-after-free if the WebContents or WebRequest outlives the objects or is destroyed before the async callback runs. electron_api_web_request.cc [615-628] Referred Code auto login_callback = base::BindOnce(&WebRequest::OnLoginAuthResult, base::Unretained(this), request_info->id, credentials); scoped_refptr<net::HttpResponseHeaders> response_headers = request_info->response_headers; blocked_requests_[request_info->id].login_handler = std::make_unique<LoginHandler>( auth_info, web_contents, static_cast<base::ProcessId>(request_info->render_process_id), request_info->url, response_headers, std::move(login_callback)); return AuthRequiredResponse::AUTH_REQUIRED_RESPONSE_IO_PENDING; }
	Dangling callback Description: Posting the auth callback using base::BindOnce(std::move(iter->second.auth_callback), action) after erasing the BlockedRequest relies on base::Unretained(this) set earlier and lacks lifetime guarantees or cancellation if WebRequest is destroyed, possibly enabling dereference of a freed 'this'. electron_api_web_request.cc [767-776] Referred Code AuthRequiredResponse action = AuthRequiredResponse::AUTH_REQUIRED_RESPONSE_NO_ACTION; if (maybe_creds.has_value()) { *credentials = maybe_creds.value(); action = AuthRequiredResponse::AUTH_REQUIRED_RESPONSE_SET_AUTH; } base::SequencedTaskRunner::GetCurrentDefault()->PostTask( FROM_HERE, base::BindOnce(std::move(iter->second.auth_callback), action)); blocked_requests_.erase(iter);
	Potential deadlock Description: The authentication flow pauses processing and conditionally early-returns on AUTH_REQUIRED_RESPONSE_IO_PENDING without validating that subsequent completion always resumes processing; if a callback path is missed, the connection could deadlock, enabling a denial-of-service vector within the renderer-initiated WebSocket flow. proxying_websocket.cc [406-416] Referred Code auto continuation = base::BindRepeating( &ProxyingWebSocket::OnAuthRequiredComplete, weak_factory_.GetWeakPtr()); auto auth_rv = web_request_->OnAuthRequired( &info_, auth_info, std::move(continuation), &auth_credentials_); PauseIncomingMethodCallProcessing(); if (auth_rv == api::WebRequest::AuthRequiredResponse:: AUTH_REQUIRED_RESPONSE_IO_PENDING) { return; } OnAuthRequiredComplete(auth_rv);
Ticket Compliance
⚪	🎫 No ticket provided Create ticket/issue
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
⚪	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Missing Audit Logs: The new WebSocket authentication flow handles credentials via login events but does not record any audit log entries for auth challenges or outcomes, making it unclear who authenticated and what action was taken. Referred Code WebRequest::AuthRequiredResponse WebRequest::OnAuthRequired( const extensions::WebRequestInfo* request_info, const net::AuthChallengeInfo& auth_info, WebRequest::AuthCallback callback, net::AuthCredentials* credentials) { content::RenderFrameHost* rfh = content::RenderFrameHost::FromID( request_info->render_process_id, request_info->frame_routing_id); content::WebContents* web_contents = nullptr; if (rfh) web_contents = content::WebContents::FromRenderFrameHost(rfh); BlockedRequest blocked_request; blocked_request.auth_callback = std::move(callback); blocked_requests_[request_info->id] = std::move(blocked_request); auto login_callback = base::BindOnce(&WebRequest::OnLoginAuthResult, base::Unretained(this), request_info->id, credentials); scoped_refptr<net::HttpResponseHeaders> response_headers = request_info->response_headers; ... (clipped 159 lines) Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Limited Edge Cases: OnAuthRequired assumes RenderFrameHost resolution and does not handle cases with null WebContents or missing response headers with explicit fallbacks or error signaling beyond proceeding to IO_PENDING. Referred Code WebRequest::AuthRequiredResponse WebRequest::OnAuthRequired( const extensions::WebRequestInfo* request_info, const net::AuthChallengeInfo& auth_info, WebRequest::AuthCallback callback, net::AuthCredentials* credentials) { content::RenderFrameHost* rfh = content::RenderFrameHost::FromID( request_info->render_process_id, request_info->frame_routing_id); content::WebContents* web_contents = nullptr; if (rfh) web_contents = content::WebContents::FromRenderFrameHost(rfh); BlockedRequest blocked_request; blocked_request.auth_callback = std::move(callback); blocked_requests_[request_info->id] = std::move(blocked_request); auto login_callback = base::BindOnce(&WebRequest::OnLoginAuthResult, base::Unretained(this), request_info->id, credentials); scoped_refptr<net::HttpResponseHeaders> response_headers = request_info->response_headers; ... (clipped 13 lines) Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Credential Handling: The test supplies plaintext credentials via the login callback and the new flow processes them without visible validation or safeguards, requiring verification that sensitive data is neither logged nor exposed elsewhere in the flow. Referred Code contents.on('login', (event, details: any, _: any, callback: (u: string, p: string) => void) => { if (details?.url?.startsWith(`ws://localhost:${port}`)) { event.preventDefault(); callback('user', 'pass'); } }); await contents.loadFile(path.join(fixturesPath, 'blank.html')); const message = await contents.executeJavaScript(`new Promise((resolve, reject) => { let attempts = 0; function connect() { attempts++; const ws = new WebSocket('ws://localhost:${port}'); ws.onmessage = e => resolve(e.data); ws.onerror = () => { if (attempts < 3) { setTimeout(connect, 50); } else { reject(new Error('WebSocket auth failed')); } ... (clipped 8 lines) Learn more about managing compliance generic rules or creating your own custom rules

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

[qodo-code-review]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Prevent crash in release builds Add a return after NOTREACHED() in OnLoginAuthResult to prevent a potential crash in release builds if the request ID is not found. shell/browser/api/electron_api_web_request.cc [759-777] void WebRequest::OnLoginAuthResult( uint64_t id, net::AuthCredentials* credentials, const std::optional<net::AuthCredentials>& maybe_creds) { auto iter = blocked_requests_.find(id); - if (iter == blocked_requests_.end()) + if (iter == blocked_requests_.end()) { NOTREACHED(); + return; + } AuthRequiredResponse action = AuthRequiredResponse::AUTH_REQUIRED_RESPONSE_NO_ACTION; if (maybe_creds.has_value()) { *credentials = maybe_creds.value(); action = AuthRequiredResponse::AUTH_REQUIRED_RESPONSE_SET_AUTH; } base::SequencedTaskRunner::GetCurrentDefault()->PostTask( FROM_HERE, base::BindOnce(std::move(iter->second.auth_callback), action)); blocked_requests_.erase(iter); } Apply / Chat Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies a potential crash in release builds due to dereferencing an end iterator and proposes a simple fix that prevents it.	High
General	Improve map insertion efficiency and clarity Refactor OnAuthRequired to use try_emplace for more efficient and cleaner in-place construction of BlockedRequest in the map. shell/browser/api/electron_api_web_request.cc [600-628] WebRequest::AuthRequiredResponse WebRequest::OnAuthRequired( const extensions::WebRequestInfo* request_info, const net::AuthChallengeInfo& auth_info, WebRequest::AuthCallback callback, net::AuthCredentials* credentials) { content::RenderFrameHost* rfh = content::RenderFrameHost::FromID( request_info->render_process_id, request_info->frame_routing_id); content::WebContents* web_contents = nullptr; if (rfh) web_contents = content::WebContents::FromRenderFrameHost(rfh); - BlockedRequest blocked_request; - blocked_request.auth_callback = std::move(callback); - blocked_requests_[request_info->id] = std::move(blocked_request); + auto [iter, inserted] = blocked_requests_.try_emplace(request_info->id); + iter->second.auth_callback = std::move(callback); auto login_callback = base::BindOnce(&WebRequest::OnLoginAuthResult, base::Unretained(this), request_info->id, credentials); scoped_refptr<net::HttpResponseHeaders> response_headers = request_info->response_headers; - blocked_requests_[request_info->id].login_handler = - std::make_unique<LoginHandler>( - auth_info, web_contents, - static_cast<base::ProcessId>(request_info->render_process_id), - request_info->url, response_headers, std::move(login_callback)); + iter->second.login_handler = std::make_unique<LoginHandler>( + auth_info, web_contents, + static_cast<base::ProcessId>(request_info->render_process_id), + request_info->url, response_headers, std::move(login_callback)); return AuthRequiredResponse::AUTH_REQUIRED_RESPONSE_IO_PENDING; } Apply / Chat Suggestion importance[1-10]: 5 __ Why: The suggestion offers a valid refactoring to use try_emplace for better efficiency and code clarity, but the performance gain is minor and the existing code is not incorrect.	Low
More