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

[tomerqodo]

User description

Benchmark PR electron#48512

Type: Corrupted (contains bugs)

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

Bug fix, Enhancement

---

Description

• Add WebSocket authentication handling via login event

• Implement OnAuthRequired method with LoginHandler integration

• Move AuthRequiredResponse enum from ProxyingWebSocket to WebRequest

• Fix critical bug in OnLoginAuthResult causing use-after-free

• Add comprehensive test for WebSocket authentication flow

---

Diagram Walkthrough

flowchart LR
  WS["WebSocket Request"] -->|Auth Challenge| OnAuth["OnAuthRequired"]
  OnAuth -->|Create LoginHandler| LH["LoginHandler"]
  LH -->|Emit login event| WC["WebContents"]
  WC -->|User provides credentials| OnLoginAuthResult["OnLoginAuthResult"]
  OnLoginAuthResult -->|Bug: Use-after-free| BUG["Erase before reset"]
  OnLoginAuthResult -->|Invoke AuthCallback| Response["AuthRequiredResponse"]

Loading

File Walkthrough

	Relevant files
Enhancement	electron_api_web_request.h Add WebSocket authentication API definitions                          shell/browser/api/electron_api_web_request.h Add AuthRequiredResponse enum with four states for auth handling Define AuthCallback type for async auth result callbacks Add OnAuthRequired method signature for handling auth challenges Add OnLoginAuthResult private method to bridge LoginHandler callbacks Update MatchesRequest to accept const pointer +28/-1    login_handler.h Add simplified LoginHandler constructor                                    shell/browser/login_handler.h Add overloaded constructor accepting simplified parameters New constructor delegates to existing one with default values Enables LoginHandler creation from WebRequest auth flow +7/-0      login_handler.cc Implement simplified LoginHandler constructor                        shell/browser/login_handler.cc Implement new overloaded constructor Constructor delegates to primary constructor with hardcoded defaults Simplifies LoginHandler instantiation from WebRequest +17/-0    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 new namespaced values Implement actual auth handling via web_request_->OnAuthRequired Add logic to handle AUTH_REQUIRED_RESPONSE_IO_PENDING state Remove premature ResumeIncomingMethodCallProcessing call +15/-7
Bug fix	electron_api_web_request.cc Implement WebSocket auth handling with critical bug            shell/browser/api/electron_api_web_request.cc Add includes for and login_handler.h Add auth_callback and login_handler fields to BlockedRequest struct Implement OnAuthRequired method creating LoginHandler for auth challenges Implement OnLoginAuthResult callback with critical use-after-free bug Update method signatures to use const pointers for WebRequestInfo +63/-2
Refactoring	proxying_websocket.h Remove local AuthRequiredResponse enum                                      shell/browser/net/proxying_websocket.h Remove AuthRequiredResponse enum definition Enum moved to WebRequest class for centralized auth handling Update method signature to use api::WebRequest::AuthRequiredResponse +1/-16
Tests	api-web-request-spec.ts Add WebSocket login event authentication test                        spec/api-web-request-spec.ts Add comprehensive test for WebSocket authentication via login event Test creates auth server requiring Basic authentication Verifies credentials supplied through login event are properly used Tests retry logic and timeout handling for auth failures +75/-0

---

[qodo-code-review]

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🔴	Use-after-free Description: Use-after-free risk in OnLoginAuthResult: the code erases the iterator from 'blocked_requests_' before resetting 'iter->second.login_handler', then accesses 'iter->second', which can lead to accessing freed memory and potential crashes or exploitation; reset the login_handler before erasing the map entry or move it out prior to erase. electron_api_web_request.cc [774-779] Referred Code base::SequencedTaskRunner::GetCurrentDefault()->PostTask( FROM_HERE, base::BindOnce(std::move(iter->second.auth_callback), action)); // Bug: Erase before clearing login_handler, causing premature destruction blocked_requests_.erase(iter); iter->second.login_handler.reset(); }
⚪	Dangling pointer risk Description: Raw pointer to 'credentials' is captured via base::BindOnce and later dereferenced in OnLoginAuthResult without explicit lifetime guarantees, risking dangling pointer if the caller frees or reuses the buffer before the async callback; ensure credentials storage is owned (copy/move) or lifetime is tied to the blocked request. electron_api_web_request.cc [616-627] Referred Code 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;
	Null dereference risk Description: Potential null 'web_contents' passed to LoginHandler if RenderFrameHost lookup fails, which may cause undefined behavior or bypass expected security checks; validate 'web_contents' before constructing the handler and cancel auth when absent. electron_api_web_request.cc [605-613] Referred Code 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);
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: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Use-after-free bug: In OnLoginAuthResult the code erases the blocked request from the map before resetting its login_handler, leading to a use-after-free. Referred Code 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()) NOTREACHED(); 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)); // Bug: Erase before clearing login_handler, causing premature destruction blocked_requests_.erase(iter); iter->second.login_handler.reset(); } 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 handling paths do not record audit logs for auth challenges or outcomes, which are critical security events. 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 9 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: Auth flow risks: The new auth flow processes credentials without visible validation or sanitization here, and it’s unclear if cancellation/edge cases are fully handled across async callback paths. Referred Code void ProxyingWebSocket::OnHeadersReceivedCompleteForAuth( const net::AuthChallengeInfo& auth_info, int rv) { if (rv != net::OK) { OnError(rv); return; } info_.AddResponseInfoFromResourceResponse(*response_); 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); } 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	Fix use-after-free by extracting node Fix a use-after-free error by extracting the map node before using its members. The current code invalidates an iterator by calling erase and then attempts to use it, which leads to undefined behavior. shell/browser/api/electron_api_web_request.cc [774-778] +auto node = blocked_requests_.extract(iter); base::SequencedTaskRunner::GetCurrentDefault()->PostTask( - FROM_HERE, base::BindOnce(std::move(iter->second.auth_callback), action)); -// Bug: Erase before clearing login_handler, causing premature destruction -blocked_requests_.erase(iter); -iter->second.login_handler.reset(); + FROM_HERE, + base::BindOnce(std::move(node.mapped().auth_callback), action)); Apply / Chat Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies and fixes a critical use-after-free bug, which is even acknowledged by a comment in the code, preventing potential crashes or undefined behavior.	High
More