Skip to content

Add gateway WebView and pending connection admission - #666

Draft
RyanKung wants to merge 23 commits into
masterfrom
codex/webview-gateway-665
Draft

Add gateway WebView and pending connection admission#666
RyanKung wants to merge 23 commits into
masterfrom
codex/webview-gateway-665

Conversation

@RyanKung

Copy link
Copy Markdown
Member

Summary

  • Add a reusable rings-webview crate for controlled gateway URLs, HTML/CSS rewriting, cookie/header policy, rendering, browser bootstrap, and gateway tests.
  • Integrate the WebView entry into the frontend with a dedicated window-style browsing UI, service worker host assets, debug controls, and Rings gateway request flow.
  • Harden core connection admission with a bounded pending pool so pending handshakes never enter the routable/DHT view before data-channel open.
  • Make wasm/web-sys cfg target-aware so native --all-features builds do not select wasm-only transport semantics.

Why

The WebView path needs a controlled origin that can render pages through the local Rings gateway without relying on browser extensions. The DHT transport also needed a real pending-handshake model so speculative connections cannot be treated as logical routing entries.

Closes #665

Validation

  • git diff --check
  • cargo test -p rings-core --all-targets --all-features -q
  • cargo test -p rings-transport --all-targets --all-features -q
  • cargo test -p rings-core --all-targets --features dummy -q
  • cargo test -p rings-core --lib -q
  • cargo clippy -p rings-core --all-targets --all-features -- -D warnings
  • touched-file rustfmt --check

@RyanKung RyanKung left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One blocking CORS-isolation finding.

}

function requestKind(request) {
const taggedKind = request.headers.get("x-rings-webview-kind");

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: X-Rings-Webview-Kind is page-writable, but this value controls whether the host runs the virtual CORS checks. The bootstrap first adds xhr, then page code can call xhr.setRequestHeader("X-Rings-Webview-Kind", "subresource"); browsers combine duplicate request headers, yielding xhr, subresource. That misses the exact matches below and falls back to subresource, so WebviewHostRequest::subresource skips virtual CORS even though the XHR caller can read the response. Please derive the request kind from trusted host state (or reject malformed/multiple runtime tags) rather than treating an unrecognized tag as a subresource.

@RyanKung RyanKung left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rustacean re-review: the pushed head still has required WASM/clippy failures.

.dht_finger_table_size(TEST_DHT_FINGER_TABLE_SIZE)
.build()
.map_err(|error| WebviewError::Transport(format!("build processor: {error:?}")))?;
let provider = Arc::new(Provider::from_processor(Arc::new(processor)));

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: this makes the required WASM/clippy job fail. Provider and Processor are explicitly neither Send nor Sync, so wrapping them in Arc triggers clippy::arc_with_non_send_sync (the CI log points here). This browser-only, single-threaded ownership needs Rc throughout its local test boundary, or a justified Send+Sync boundary. The same job also reports the value.as_ref() call below as useless. Please make this test compile cleanly under the repository's -D warnings policy.

Comment thread frontend/src/webview.rs
})();
"#;

thread_local! {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: the required clippy job also rejects this new thread_local! initializer with clippy::missing_const_for_thread_local under -D warnings. Use a const { ... } initializer (or otherwise make the intentional runtime initialization explicit) so the PR's required lint job is green.

@RyanKung RyanKung left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The lifecycle model still needs a formal witness.


/// Bounded, non-routable handshakes owned by the swarm lifecycle.
///
/// The pool deliberately has no DHT reference: a peer is visible to Chord

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: the pending-admission invariant is documented, but it has no executable state-transition model. Pending -> Active is split across this pool, InnerSwarmCallback, and DHT join/leave effects; the new Stateright-style model covers storage CRDT convergence, not this lifecycle. Add a finite model (or a property/state-machine test) over Absent/Pending(generation)/Active with open, close, failed, timeout, replacement, and late-callback actions. It should witness: a pending peer is never routable; only the matching generation may promote; and every terminal/expiry transition removes the DHT membership and transport slot. The current example tests do not cover those event orderings.

@RyanKung RyanKung left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fresh review of the latest feedback commit: required CI still has reproducible blockers.

addEventListener() {},
},
};
context.globalThis = context;

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: this currently breaks the required WASM job before the frontend checks run. npm run build:frontend-extension-scripts reports TS4111 here because ServiceWorkerTestContext has an index signature: access this as context["globalThis"] = context (or model globalThis as an explicit property). The local Node execution passes, but the generated-script TypeScript check in CI does not.

peer,
generation: self.next_generation,
};
self.peers.insert(

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: cargo +nightly fmt --all -- --check still reports this block (and the analogous assertion formatting in transport/tests.rs) as unformatted. QACI runs that exact nightly command, so the required rustfmt job will fail. Please format with the repository's nightly toolchain before pushing.

});
}

self.next_generation = self.next_generation.wrapping_add(1);

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: the new model is not a refinement of the implementation at the generation-exhaustion boundary. The production counter wraps here, whereas LifecycleModel::replace_pending uses saturating_add; consequently the model never represents the ABA case where an old callback generation becomes equal to a new live generation after wraparound. Either make production generation allocation fallible/non-reusing and model that error, or give the model the same wrap semantics and explicitly state the accepted bound. The claimed generation-safety proof is otherwise incomplete.

@RyanKung RyanKung left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Node/wasm follow-up: the wasm CI job is currently blocked by the new registration test.

Comment thread crates/node/src/registration.rs Outdated
["a", "b", "c"]
.into_iter()
.enumerate()
.filter_map(|(bit, value)| (mask & (1 << bit) != 0).then(|| encoded(value)))

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: This new property test makes the wasm CI job fail. The workflow runs clippy with -D warnings, and this filter_map+bool::then is rejected by clippy::filter_map_bool_then, so Build and test for wasm cannot complete. Please rewrite it as filter(...).map(...) (or otherwise avoid the lint) and rerun the wasm job.

@RyanKung RyanKung left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Native follow-up: the native examples/lint workflows are also blocked by the new candidate rewrite.

}

let fields = line.split_whitespace().collect::<Vec<_>>();
if fields.len() < 8 || fields[6] != "typ" || fields[7] != "host" {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: The native node CI cannot pass with the repository's panic-free clippy policy. Even though the preceding len() < 8 guard makes these indices logically safe, cargo clippy -p rings-node --features ffi treats clippy::indexing_slicing as an error here (and at the related accesses below), so both native examples and the lint workflow fail. Destructure the required fields with checked access/an iterator before using them.

@RyanKung RyanKung left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 — The runtime abstraction is not consistent across native and wasm.

The core changes now model browser semantics with all(feature = "wasm", target_family = "wasm"): a native build that enables the wasm feature remains Send-capable and uses native time/storage implementations. rings-node still defines the corresponding runtime distinction with feature = "browser" alone:

  • extension/ext/mod.rs:58-66 makes MaybeSend empty whenever the browser feature is enabled;
  • registration.rs:80-95 and processor/mod.rs:98-111 select window_sleep from that feature alone;
  • RegistrationTask and its async_trait expansion follow the same feature-only predicate.

Thus the same native target has two incompatible models when both capability features are selected: core requires native Send semantics, while node erases them and attempts JS-only adapters. This is not merely an unsupported feature combination: the PR specifically made the core predicate target-aware, but did not refine the node layer to it. cargo check -p rings-node --all-features demonstrates the broken composition (missing js_utils/IndexedDB plus Send and wasm-bindgen errors).

Please define one target-aware runtime predicate (or use cfg(all(feature = "browser", target_family = "wasm")) consistently) for node's MaybeSend, async-trait, timers, browser modules, and exports. Add a native “browser feature enabled” compile check so this refinement cannot drift again.

@RyanKung RyanKung left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 — The lifecycle capability is implemented below the public browser boundary, so native and wasm expose different abstractions.

Native Provider exposes listen_with(StopToken) in crates/node/src/provider/mod.rs:293-299. The wasm-exported Provider::listen in provider/browser/provider.rs:619-625 still always calls Processor::listen(), which constructs StopToken::never(); JavaScript callers therefore cannot request the lifecycle that the new model introduces. The frontend works around this by retaining a separate Processor and spawning Processor::listen_with directly in frontend/src/node.rs:140-156.

The Provider facade is no longer the runtime-neutral boundary: native callers control the loop through Provider, while browser callers must bypass Provider and know its implementation. Expose an equivalent browser lifecycle handle/stop operation at the Provider boundary (or explicitly make the lifecycle owner a separate cross-runtime type) and add a parity test for start → stop → settled listener in both adapters.

@RyanKung RyanKung left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 — The public browser boundary is now present, but the webview frontend still bypasses it.

Provider::listen() now returns ProviderListener, yet frontend/src/node.rs:140-163 retains an Arc<Processor>, creates a separate StopSource, and starts Processor::listen_with directly. Consequently the same browser node has two lifecycle abstractions: external callers use ProviderListener, while the in-repo webview owner uses an unrelated source/token pair below the Provider boundary.

The boundary is not yet the single model. Store ProviderListener in DemoNode and implement DemoNode::stop through it (or make a single runtime-neutral listener type used by both adapters). Add a frontend-level test that proves the owned listener task settles after DemoNode::stop(); the current wasm test only observes StopSource::is_stop_requested, not listener completion.

@rings-auto-reviewer rings-auto-reviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rustacean re-review of e4c48f0: the Provider lifecycle boundary is now used by DemoNode, but its completion contract still has no frontend-level witness.

Comment thread frontend/src/node.rs
listen_abort: AbortHandle,
/// Controlled webview gateway attached to this browser node when its host origin is HTTP(S).
pub webview: Option<WebviewNode>,
listener: ProviderListener,
}

impl DemoNode {
/// Stop the background listen/stabilize loop started for this demo node.
pub fn stop(&self) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 — DemoNode::stop now delegates to ProviderListener, which fixes the duplicated lifecycle owner, but the frontend has no test that observes the listener task settling after this method is called. The listener is private, so the current frontend test surface cannot distinguish a cooperative listener that finishes from a leaked loop that merely records StopSource::is_stop_requested. Add a frontend-level lifecycle test (or an intentionally narrow test accessor) that constructs the demo node, calls stop, awaits listener.task(), and asserts completion under a bounded timeout. The lifecycle proposition is stop(DemoNode) => eventually listener_task_settled; it remains untested.

@rings-auto-reviewer rings-auto-reviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 — Current head fails two required workflows; these are committed-diff failures verified from the GitHub Actions logs.

prepare_repair_node_with_optional_measure(key, None)
}

fn prepare_repair_node_with_measure(key: SecretKey, measure: MeasureImpl) -> Result<Node> {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 — This helper is compiled for the normal core test target but is only called by the dummy-only backpressure test. With the repository -D warnings policy, the required clippy workflow fails with function prepare_repair_node_with_measure is never used at this line. Give this helper the same #[cfg(all(feature = "dummy", not(target_family = "wasm")))] boundary as its only caller (or make its use unconditional) so the test model is valid in every compiled feature domain.

addEventListener() {},
},
};
context.globalThis = context;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 — The required wasm job stops at the extension-script TypeScript check. ServiceWorkerTestContext defines these dynamic properties through an index signature, and TS4111 rejects property access here; use context["globalThis"] = context. The same failure exists for context.loading in the new test-webview-overlay.mts (lines 42, 67, and 76), so fix every indexed fixture property before rerunning the workflow.

@rings-auto-reviewer rings-auto-reviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rustacean re-review of 9caa432: the P1 feature-domain and TypeScript failures are addressed in the committed diff. One lifecycle-test refinement remains.

Comment thread frontend/src/node.rs
.await
.map_err(|error| format!("build provider: {error}"))?,
);
let listener = provider.listen();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 — The new test still does not witness cancellation of a running listener. It creates the promise here and calls node.stop() in the same turn, before yielding to the wasm executor; this is equivalent to the already-covered pre-stopped-token case. It also awaits the promise without a test-owned timeout, so a regression to a non-settling listener can hang the test runner instead of failing the proposition. Yield until the listener has had a scheduling turn (or expose a readiness witness), then stop it and race listener.task() against a short timeout. The required law is: a started listener transitions to settled after DemoNode::stop, within the bounded test interval.

@rings-auto-reviewer rings-auto-reviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rustacean re-review of c5a2517: the stop model now reaches the periodic loops, and the test has a bounded settlement assertion. One test-witness issue remains.

Comment thread frontend/src/node.rs Outdated
let task = node.listener.task();

assert!(!node.listener.is_stopped());
sleep(Duration::from_millis(LISTENER_START_YIELD_MS)).await;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 — This fixed 1 ms delay is not a witness that the listener has started; under wasm scheduling or a busy runner it can be too short, while an implementation that never polls the listener body can still satisfy this test after the delay. The test should synchronize on an observable start boundary (for example a listener readiness signal exposed only under cfg(test)) and then call stop. The property is not “one millisecond elapsed”; it is “a running ProviderListener settles within the bounded timeout after DemoNode::stop.”

@rings-auto-reviewer rings-auto-reviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Node-focused Rustacean re-review: the cooperative stop propagation is structurally sound; one expected-stop outcome is still classified as a failure.

.run_registration_once_with_timeout(task, timeout, stop.clone())
.await
{
tracing::warn!("Failed to run {} registration task: {error:?}", task.name());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 — RegistrationStopped is the expected terminal transition after the shared listener token is requested, but this branch logs it as Failed to run ... registration task. A normal ProviderListener::stop() during a DHT cache poll will therefore emit a warning per registration task, indistinguishable from a real registration fault. Match Err(Error::RegistrationStopped) before this generic error branch and return cleanly (or log it at debug level). The lifecycle model should classify stop as cancellation, not failure.

@rings-auto-reviewer rings-auto-reviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rustacean re-review of 2f42cbd: the registration-stop classification is now correct. The current head is nevertheless blocked by a required formatting check.

Comment thread frontend/Cargo.toml
@@ -13,12 +13,15 @@ publish = false
[dependencies]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 — The required Check rustfmt style && run clippy workflow fails at taplo format --check: frontend/Cargo.toml is not properly formatted. This PR changes this dependency block for the frontend/webview integration, so run the repository Taplo formatter and commit the result; the current head cannot pass required CI.

secure: false,
};

for part in parts {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 — The virtual cookie parser ignores Max-Age and Expires. A target response such as sid=old; Path=/; Max-Age=0 is retained and cookie_header continues sending it indefinitely, breaking logout/session rotation. Model expiry/deletion in StoredCookie and remove the matching tuple for nonpositive Max-Age or past Expires; add expiry and deletion cases.

Comment thread package.json Outdated
@@ -17,7 +17,9 @@
"build:frontend-extension-scripts": "npm run check:frontend-extension-scripts && rm -rf frontend/.generated && tsc -p frontend/tsconfig.extension-assets.json && tsc -p frontend/tsconfig.scripts.json && node frontend/.generated/scripts/check-extension-docs.mjs",
"build:frontend-web": "cd frontend && NO_COLOR=true trunk build --release",
"package:frontend-extension": "npm run build:frontend-extension-scripts && cd frontend && NO_COLOR=true trunk build --release && cd .. && node frontend/.generated/scripts/package-extension.mjs",
"test:frontend-extension-wallet": "npm run package:frontend-extension && node frontend/.generated/scripts/test-extension-wallet-bridge.mjs"
"test:frontend-extension-wallet": "npm run package:frontend-extension && node frontend/.generated/scripts/test-extension-wallet-bridge.mjs",
"test:frontend-webview-service-worker": "node --experimental-strip-types frontend/scripts/test-webview-service-worker.mts",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 — These are the only direct service-worker/overlay behavioral test commands added by the PR, but QACI installs npm dependencies and then runs only extension-script build/package plus Rust checks/tests; it never invokes either command. The gateway trust-boundary behavior therefore has no required-CI witness. Run these tests explicitly after npm ci (or make them part of an existing required test script).

false
}
};
self.webview_ready.set(webview_ready);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 — A disconnect/restart can happen while gateway registration is awaited. It clears the shared gateway/session, but this stale start then resumes, marks webview ready, and continues seed connection; registration can also recreate the listener after clear. Re-check the GenerationToken immediately after the await, and if stale stop only the node owned by this generation and return. Add a delayed-registration plus disconnect/restart regression.

@rings-auto-reviewer

Copy link
Copy Markdown
Contributor

Additional P1 (cross-file): the new pending-admission contract is not enforced before message dispatch. InnerSwarmCallback::on_message verifies and dispatches a signed payload before checking that the data-channel attempt has been promoted to the current active generation. A message arriving before the open/admission callback can therefore mutate DHT, storage, or application state from a pending connection. Please gate dispatch on matching admission/generation and add an open-before-admission no-effect test. The relevant dispatch lines are unchanged context, so GitHub cannot anchor this as an inline diff comment.

self.dht.apply_fixed_finger(*index, msg.did)?;
} else if msg.reports_remote_successor(self.dht.did) {
self.connect_dht_peer(msg.did).await?;
if self.transport.get_connection(msg.did).is_some() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 — This only applies the finger when connect_dht_peer has synchronously produced an admitted transport. Its effect interpreter merely starts the handshake and returns Ok (including AlreadyConnected while still pending); normal WebRTC admission occurs later in the callback. In that normal path the index is still discarded and no later callback applies it, so the original first-connect finger loss remains. Preserve (index, did) until matching admission, or have the admission path complete the pending finger update; add a pending-handshake regression.

.as_ref()
.is_some_and(|node| node.provider.address() == did)
{
*node_ref = None;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 — The token check is now present, but its cleanup identifies ownership only by DID. A rapid restart using the same wallet creates a newer DemoNode with the same provider address; when this stale activation resumes, this predicate matches the newer node, clears node_ref and the newer gateway, and then stops the stale node. The ownership predicate must include generation or node identity, not principal identity. Add delayed-registration -> restart with the same wallet and assert the newer node/gateway survives.

includeUncontrolled: true,
});
for (const client of candidates) {
if (isWebviewPopup(client.url)) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 — Host registration is now capability-bound, but debug delivery is still not. Any remote document under /webview/ is auto-enrolled here, and any same-origin client can also send rings-webview-debug-register. emitResourceDebug includes the decoded upstream target, method, status, and timing, while debug history replays up to 200 prior entries. Thus one remote page can observe other WebView navigations/requests. Restrict debug registration/discovery to the same trusted capability boundary and add a hostile webview-client no-observation test.

Comment thread package.json Outdated
@@ -14,10 +14,12 @@
"wasm_pack": "wasm-pack build crates/node --release --scope ringsnetwork -t web --no-default-features --features browser_default --features console_error_panic_hook",
"prepare": "npm run wasm_pack && rm -rf dist && mkdir -p dist && cp crates/node/pkg/rings_node* ./dist && cp index.ts ./dist && tsc -p tsconfig.json",
"check:frontend-extension-scripts": "biome check frontend/extension-assets/*.ts frontend/scripts/*.mts frontend/tsconfig.base.json frontend/tsconfig.extension-assets.json frontend/tsconfig.scripts.json",
"build:frontend-extension-scripts": "npm run check:frontend-extension-scripts && rm -rf frontend/.generated && tsc -p frontend/tsconfig.extension-assets.json && tsc -p frontend/tsconfig.scripts.json && node frontend/.generated/scripts/check-extension-docs.mjs",
"build:frontend-extension-scripts": "npm run check:frontend-extension-scripts && rm -rf frontend/.generated && tsc -p frontend/tsconfig.extension-assets.json && tsc -p frontend/tsconfig.scripts.json && node frontend/.generated/scripts/check-extension-docs.mjs && npm run test:frontend-webview-service-worker && npm run test:frontend-webview-overlay",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 — This now runs the WebView test scripts in required QACI, but the workflow pins Node 20. The scripts invoke node --experimental-strip-types, which Node 20.20.2 rejects: the current wasm job fails with node: bad option: --experimental-strip-types and exit code 9. Either run these TypeScript tests with a Node version that supports the flag, compile them before execution, or use an existing TypeScript runner compatible with Node 20.

"use strict";

const workerUrl = "/rings-webview-service-worker.js?gateway-host-protocol=4";
const gatewayHostCapability = createGatewayHostCapability();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 — This capability is generated independently in every window. The gateway host owns the capability stored by the worker, but the real WebView popup is a separate /#webview window and therefore generates a different value. Its overlay calls enableDebug(), and registerDebugClient now requires exact equality, so the popup is always rejected and receives no worker diagnostics. The added test only registers the same host client with its own capability; it never models a second popup. Transfer a debug-scoped capability over an authenticated opener/MessageChannel (or route diagnostics through the host), and test a distinct popup client receives entries without allowing a /webview/ document to subscribe.

Comment thread package.json
@@ -14,10 +14,12 @@
"wasm_pack": "wasm-pack build crates/node --release --scope ringsnetwork -t web --no-default-features --features browser_default --features console_error_panic_hook",
"prepare": "npm run wasm_pack && rm -rf dist && mkdir -p dist && cp crates/node/pkg/rings_node* ./dist && cp index.ts ./dist && tsc -p tsconfig.json",
"check:frontend-extension-scripts": "biome check frontend/extension-assets/*.ts frontend/scripts/*.mts frontend/tsconfig.base.json frontend/tsconfig.extension-assets.json frontend/tsconfig.scripts.json",
"build:frontend-extension-scripts": "npm run check:frontend-extension-scripts && rm -rf frontend/.generated && tsc -p frontend/tsconfig.extension-assets.json && tsc -p frontend/tsconfig.scripts.json && node frontend/.generated/scripts/check-extension-docs.mjs",
"build:frontend-extension-scripts": "npm run check:frontend-extension-scripts && rm -rf frontend/.generated && tsc -p frontend/tsconfig.extension-assets.json && tsc -p frontend/tsconfig.scripts.json && node frontend/.generated/scripts/check-extension-docs.mjs && node frontend/.generated/scripts/test-webview-service-worker.mjs && node frontend/.generated/scripts/test-webview-overlay.mjs",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 — The Node 20 flag failure is gone, but required wasm CI still fails here. test-webview-overlay.mjs launches Playwright, while QACI only runs npm ci --ignore-scripts and never downloads a browser; the current run errors that chrome-headless-shell does not exist. Install the required Playwright browser (and its OS dependencies if needed) before this script, or split this browser-dependent test into a job that provisions it.

Comment thread crates/webview/src/cookie.rs Outdated
delete_cookie = true;
cookie.expires_at = None;
} else if let Some(expires_at) =
SystemTime::now().checked_add(Duration::from_secs(seconds as u64))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 — This virtual cookie expiry implementation is not valid on the browser target. wasm32-unknown-unknown has no SystemTime::now clock; the current required wasm job panics in std::sys::time::unsupported here, then reports 28 passed / 4 failed (first failure is webview::tests::host_redirects_then_serves_a_gateway_document_through_its_transport). Inject a browser-safe clock or use an explicit timestamp from the boundary rather than SystemTime in this shared crate, and add a wasm regression for Max-Age/Expires.

const finish = (capability) => {
globalThis.clearTimeout(timeout);
channel.port1.close();
clearPopupOpener();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 — The handoff only authorizes the initial /#webview document, then clears opener. When that shell navigates via location.href to /webview/... the browser creates the remote document client; the injected overlay has neither RingsWebviewHost nor opener, so it cannot obtain the capability or re-register. The old shell client is no longer the displayed document, leaving diagnostics unavailable after the very navigation this popup exists to perform. Preserve authorization across the same browsing-context navigation at the service-worker boundary (without exposing it to the remote document), and add a shell -> gateway navigation test that receives a post-navigation debug entry.

return false;
}
if (typeof clientId !== "string" || !clientId || !debugClientIds.has(clientId)) {
return false;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 — This transfers the authorized popup debug identity to the resulting /webview/ document. debugClients intentionally accepts any same-origin URL, so after the navigation the remote page receives future worker entries containing decoded target URL, method, status, and timing. That reintroduces the exact remote-page disclosure that the capability boundary was meant to prevent; the new test explicitly expects postNavigationMessages to receive the secret entry. Do not propagate a debug-read capability into a remote document. Keep diagnostics in the trusted shell/host and expose only an intentionally sanitized UI channel to the rendered remote page.

.map_err(|error| Error::InvalidMessage(error.to_string()))?;

assert!(transport.is_admitted_connection(peer));
assert!(dht_topology_contains(&transport, peer)?);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 — This test is still named data_channel_open_admits_successor, but the assertion was weakened from successor membership to any predecessor/finger/successor occurrence. A regression that admits the peer into an unrelated topology slot while failing the successor transition now passes. Keep a separate generic topology test if needed, but this proposition must assert the advertised successor/DHT membership transition explicitly.

if (!client) {
return undefined;
}
const url = new URL(client.url);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 — sourceTargetForClient treats the current Client.url as immutable provenance. A remote document served at /webview/<evil> can call history.replaceState to /webview/<encoded https://victim.example/> without a navigation; subsequent fetches are labelled with victim as sourceTarget. The Rust gateway then evaluates virtual CORS and cookie policy under that forged source origin, so the remote document can make what become same-origin credentialed requests and read their responses. Bind the target to state established by a trusted navigation/host, rather than to the mutable client URL, and add a history-mutation regression test.

function isTrustedGatewayHostUrl(url) {
try {
const parsed = new URL(url);
return parsed.origin === self.location.origin && !parsed.pathname.startsWith(gatewayPrefix);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 — This host trust check is also based solely on mutable Client.url. A /webview/ remote document can change history to a non-gateway pathname. After the service worker is restarted and loses its globals, it can register first as the gateway host, choose a capability, and receive every serialized request/response and debug event. Store/verify a host identity that a remote gateway document cannot acquire by rewriting history; add a test that mutates history before a worker restart.

}

self.transport.record_peer_connected(did).await;
self.message_handler.join_dht(did).await?;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 — Promotion and DHT admission are not one lifecycle transition. After promote_pending_connection succeeds, a concurrent Failed/Closed callback can observe the peer admitted, remove it and call leave_dht while this task is awaiting record_peer_connected or join_dht. This task then resumes and re-adds a terminal peer to the DHT and emits Connected. Revalidate a generation/admission token after every await, or make promotion/join/terminal removal serialized, and add an interleaving test.

Comment thread crates/webview/src/cors.rs Outdated
.collect::<BTreeSet<_>>();
let requested_headers = non_safelisted_headers(request);
if !requested_headers.is_empty()
&& !allowed_headers.contains("*")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 — Access-Control-Allow-Headers: * is accepted as a wildcard even when this request uses credentials. Browser CORS treats * as a literal header name for credentialed requests, so the virtual gateway permits credentialed non-safelisted headers that the browser would reject at preflight. Only apply wildcard matching when credentials are not Include, and add the credentialed preflight case.

return Err(WebviewError::Cookie("cookie name is empty".to_string()));
}

let mut cookie = StoredCookie {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 — The virtual cookie jar parses Secure, Path, expiry and domain, but discards SameSite. The later send policy allows navigation/subresource cookies without a source-site decision, so SameSite=Strict or Lax authentication cookies are sent on cross-site iframe/script/image or unsafe navigation requests that browsers suppress. Preserve the attribute and carry enough initiator/top-level-navigation context into cookie selection to enforce it; add strict/lax cross-site regression tests.

// `Failed` and `Closed` are terminal states. Pending handshakes are
// discarded without touching the DHT; active peers leave it.
WebrtcConnectionState::Failed | WebrtcConnectionState::Closed => {
if self

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 — The new pending-handshake special case only handles terminal states. A Disconnected event before data-channel admission falls through, records a peer disconnect, and is forwarded to the application even though the peer was never admitted; a normal recovery then later emits Connected. With retries this lets unauthenticated transient ICE state degrade peer quality and produce a false lifecycle sequence. Handle non-admitted pending Disconnected as a no-op (or explicitly pending) and test Disconnected -> data-channel-open.

}

function injectWebviewHistoryGuard(html) {
if (html.includes(webviewHistoryGuardMarker)) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 — This treats an attacker-controlled substring as proof that the guard was injected. A target response can include data-rings-webview-history-guard anywhere (for example in an HTML comment); this returns the body unchanged, so the remote page receives no history guard and can again call history.replaceState to forge its /webview/<target> URL. After a worker restart, sourceTargetForClient trusts that forged client URL. Detect an actual trusted injected node in a way remote markup cannot pre-satisfy (or inject unconditionally/idempotently), and add a hostile-marker regression test.

Comment thread frontend/src/webview.rs Outdated
}
}

fn emit_onion_debug(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 — Required wasm CI is red: cargo clippy --target=wasm32-unknown-unknown --all-targets -- -D warnings rejects this newly expanded function with clippy::too_many_arguments (8/7). The test stages were skipped after this failure, so the submitted browser path is not validated. Carry these fields in a typed debug-event/context value (which also gives the diagnostic boundary a named model) and rerun the required wasm job.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add webview gateway for onion-backed browsing

1 participant