feat(plugins): bump servo to 0.3.0 and add auth config for private pages#630
feat(plugins): bump servo to 0.3.0 and add auth config for private pages#630staging-devin-ai-integration[bot] wants to merge 5 commits into
Conversation
Upgrade the native Servo web-renderer plugin from servo 0.1 to 0.3.0 and wire a new optional `auth` config through ServoConfig to WebView creation. - Custom request headers / bearer token attached to the initial navigation via the 0.3.0 UrlRequest/load_request API. - HTTP Basic/Digest answered non-interactively via the WebViewDelegate request_authentication hook. - Custom User-Agent applied to the process-global servo Preferences (affects all servo nodes). Auth is init-time only (not hot-swapped on UpdateConfig). Credentials are never logged and userinfo is stripped from logged URLs. Signed-off-by: streamkit-devin <devin@streamkit.dev>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
![staging-devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/870844?s=60&v=4){kind=small}
| // `Preferences` is a process-global singleton, so the User-Agent of | ||
| // the first registered node applies to every servo node thereafter. | ||
| if let Some(user_agent) = config.auth.as_ref().and_then(|a| a.user_agent.as_ref()) { | ||
| prefs.user_agent.clone_from(user_agent); | ||
| } |
There was a problem hiding this comment.
📝 Info: Per-node user_agent silently ignored after the first servo node registers
handle_register applies auth.user_agent to the process-global servo::Preferences only inside get_or_insert_with, which runs exactly once for the first registered node (plugins/native/servo/src/servo_thread.rs:419-430). Any later node's auth.user_agent is parsed and validated but has no effect. This is explicitly documented in the README (plugins/native/servo/README.md:151-155) and the schema description (plugins/native/servo/src/servo_node.rs:130), so it is an intentional, documented limitation rather than a bug — noting here for reviewer awareness since it is a non-obvious global-state coupling.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
| let request_headers = config | ||
| .auth | ||
| .as_ref() | ||
| .map(ServoAuth::build_request_headers) | ||
| .transpose() | ||
| .map_err(|e| format!("invalid auth config: {e}"))?; |
There was a problem hiding this comment.
📝 Info: Auth header validation is performed redundantly
build_request_headers is invoked once during ServoConfig::validate (plugins/native/servo/src/config.rs:288-290) and again in create_webview (plugins/native/servo/src/servo_thread.rs:695-700). The duplication is harmless (cheap, runs only at init) and provides defense-in-depth, but the second call's error mapping (invalid auth config: {e}) is effectively unreachable for configs that passed validate. Not a bug; flagged only so a reviewer knows the second error path is essentially dead for the normal init flow.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
| if header_name == AUTHORIZATION { | ||
| has_authorization = true; | ||
| } | ||
| map.append(header_name, header_value); | ||
| } | ||
| } | ||
|
|
||
| if let Some(ref token) = self.bearer_token { | ||
| if has_authorization { | ||
| return Err("auth.bearer_token conflicts with an explicit 'Authorization' header" | ||
| .to_string()); | ||
| } | ||
| let value = HeaderValue::try_from(format!("Bearer {token}")) | ||
| .map_err(|e| format!("invalid bearer_token: {e}"))?; | ||
| map.insert(AUTHORIZATION, value); | ||
| } |
There was a problem hiding this comment.
📝 Info: Bearer-token / Authorization-header conflict check relies on case-insensitive HeaderName comparison
The conflict detection in build_request_headers compares the parsed HeaderName against AUTHORIZATION (plugins/native/servo/src/config.rs:89). Because HeaderName normalizes to lowercase and compares case-insensitively, a user-supplied authorization, Authorization, or AUTHORIZATION entry will all correctly set has_authorization and trigger the conflict error when bearer_token is also set. Verified this works as intended — no bug.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
Signed-off-by: streamkit-devin <devin@streamkit.dev>
handle_update_config navigated via webview.load(), which dropped the configured custom/bearer request headers when the tunable url parameter changed at runtime, silently breaking header auth on subsequent private pages (basic auth, bound to the delegate, kept working). Route both initial and runtime navigations through a shared navigate_with_auth helper so header/bearer auth stays consistent with basic auth. Signed-off-by: streamkit-devin <devin@streamkit.dev>
| if url_changed { | ||
| if let Ok(parsed) = url::Url::parse(&new_config.url) { | ||
| state.webview.load(parsed); | ||
| navigate_with_auth(&state.webview, state.config.auth.as_ref(), parsed); | ||
| state.delegate.loaded.set(false); | ||
| // Reset the one-shot post-load gate so the new page gets | ||
| // its custom-CSS injection (if any) once it finishes |
There was a problem hiding this comment.
📝 Info: Runtime URL changes correctly re-apply init-time auth headers
I verified the docs claim that custom headers / bearer token are re-applied on runtime url changes. In handle_update_config (servo_thread.rs:594), navigation uses state.config.auth.as_ref(), and since merge_update deliberately never mutates auth, this is always the original init-time auth — consistent on both initial nav (create_webview at servo_thread.rs:706) and runtime nav. Basic-auth challenges are answered by the delegate bound at WebView creation (servo_thread.rs:689), which also persists across runtime URL changes. Behavior matches the README description.
(Refers to lines 592-602)
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
| let rect = DeviceIntRect::new( | ||
| DeviceIntPoint::new(0, 0), | ||
| DeviceIntPoint::new( | ||
| i32::try_from(state.rc_width).unwrap_or(i32::MAX), | ||
| i32::try_from(state.rc_height).unwrap_or(i32::MAX), | ||
| ), |
There was a problem hiding this comment.
📝 Info: DeviceIntRect vs Box2D semantics are equivalent for the origin-anchored read rect
The pixel readback rect changed from Box2D::new(Point2D(0,0), Point2D(w,h)) to DeviceIntRect::new(DeviceIntPoint(0,0), DeviceIntPoint(w,h)) (servo_thread.rs:515-521). DeviceIntRect in webrender is a Box2D (min/max), so this is a min/max construction. Even if a reader assumed origin/size semantics, the region is identical because the origin is (0,0) and the second point equals (w,h). No behavioral change to the read region.
(Refers to lines 515-521)
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
Signed-off-by: streamkit-devin <devin@streamkit.dev>
There was a problem hiding this comment.
📝 Info: update_params does not re-validate auth, but auth is immutable so this is safe
update_params (plugins/native/servo/src/servo_node.rs:240-252) deserializes the update and calls merge_update without calling validate(). Since merge_update never copies auth from the update, no unvalidated auth can reach the WebView at runtime. Malformed runtime URLs are silently dropped by handle_update_config's url::Url::parse guard rather than producing an error to the caller — this is pre-existing behavior unchanged by this PR.
(Refers to lines 240-252)
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
Signed-off-by: streamkit-devin <devin@streamkit.dev>
| pub fn build_request_headers(&self) -> Result<HeaderMap, String> { | ||
| let mut map = HeaderMap::new(); | ||
| let mut has_authorization = false; | ||
|
|
||
| if let Some(ref headers) = self.headers { | ||
| for (name, value) in headers { | ||
| let header_name = HeaderName::try_from(name) | ||
| .map_err(|e| format!("invalid header name '{name}': {e}"))?; | ||
| let header_value = HeaderValue::try_from(value) | ||
| .map_err(|e| format!("invalid header value for '{name}': {e}"))?; | ||
| if header_name == AUTHORIZATION { | ||
| has_authorization = true; | ||
| } | ||
| map.append(header_name, header_value); | ||
| } | ||
| } | ||
|
|
||
| if let Some(ref token) = self.bearer_token { | ||
| if has_authorization { | ||
| return Err("auth.bearer_token conflicts with an explicit 'Authorization' header" | ||
| .to_string()); | ||
| } | ||
| let value = HeaderValue::try_from(format!("Bearer {token}")) | ||
| .map_err(|e| format!("invalid bearer_token: {e}"))?; | ||
| map.insert(AUTHORIZATION, value); | ||
| } | ||
|
|
||
| Ok(map) | ||
| } |
There was a problem hiding this comment.
📝 Info: Credential safety: no Debug-leak of auth and consistent URL redaction
ServoConfig/ServoAuth/ServoBasicAuth all derive Debug and carry credentials, so I checked every logging site. The config is never logged via {:?}; all URL logs route through crate::config::redact_url (e.g. servo_node.rs:189, servo_node.rs:281, servo_thread.rs:439, servo_thread.rs:506, servo_thread.rs:288). Error messages from build_request_headers (config.rs:86-102) deliberately interpolate header names and the http crate's InvalidHeaderValue/InvalidHeaderName errors, which do not echo the offending value/token, so credentials don't leak via validation errors either. No credential-leak issue found.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
2 participants
Summary
servo 0.1toservo 0.3.0. The 0.1→0.3 jump renames parts of the embedding API and, crucially, adds per-navigation custom HTTP request headers (WebView::load_request(UrlRequest::new(url).headers(...))), which this PR builds on. Step 2 (compile-clean, behavior-unchanged) was kept as a separate concern from the new auth feature.authblock toServoConfigso private pages can be loaded non-interactively:headers(arbitrary request headers, e.g.Authorization,Cookie, customX-…) andbearer_token(convenience →Authorization: Bearer …) are attached to the initial navigation via the new 0.3.0load_requestpath.basic(username/password) answers HTTP Basic/Digest challenges via theWebViewDelegate::request_authenticationhook onFrameDelegate.user_agentis set on servo'sPreferences.WebViewat creation and are deliberately not hot-swapped onUpdateConfig(documented inmerge_update); changing auth requires recreating the node.redact_url()stripsuser:password@userinfo from every logged URL. Conflicts (bearer_token+ explicitAuthorizationheader) and malformed header names/values are rejected at config-validation time.Behavior changes / risks
Preferencesare a process-global singleton, soauth.user_agentis taken from the first registered servo node and applies to all servo nodes in the process.headers/bearer_token/basicremain per-node. Documented in the README and param schema; theauth/credential params are markedtunable: false.0.1.0 → 0.2.0(Cargo.toml, plugin.yml) with a matchingdocs/public/registry/index.jsonentry.disable_initial_exec_tlsdlopen-TLS workaround is retained.Review & Validation
cargo build/clippy -p servo-plugin-nativeclean under the crate's strict pedantic/nursery +unwrap_used/expect_usedlints (verified locally; CI re-runs).cargo test -p servo-plugin-native— newconfig.rstests cover auth deserialization, bearer↔Authorization conflict, invalid header name/value rejection,merge_updateleavingauthuntouched, andredact_urluserinfo stripping (45 tests pass).redact_url).Notes
authdeserialization/validation paths are unit-tested.Link to Devin session: https://staging.itsdev.in/sessions/ca387cc24bc3497e8cd0b8e1f2680030
Requested by: @streamer45
Devin Review
b42a05f