Some IAP fixes (WS reqs, rtc reqs, and recognize gcloud from PATH after init)#12004
Conversation
When WarpDev is launched from the macOS GUI (Finder/Dock/Spotlight), it
inherits launchd's minimal PATH, which omits Homebrew dirs like
/opt/homebrew/bin where `gcloud` usually lives. The IAP token refresh
spawned `gcloud` directly via the process PATH, so the lookup failed with
ENOENT ("Failed to spawn ... No such file or directory") and the Account
settings showed a Staging IAP credentials error.
Resolve `gcloud` against the user's interactive login-shell PATH
(captured and cached by LocalShellState), matching how the `gh` code
path already does this in util/git.rs and code_review/git_status_update.rs.
Also reorder the IapManager singleton registration to after
LocalShellState, since LocalShellState::handle panics if accessed before
registration and the initial refresh runs synchronously in IapManager::new.
Co-Authored-By: Oz <oz-agent@warp.dev>
Extends the staging IAP client support so the Proxy-Authorization token rides on all Warp-server-family traffic, not just the main server pool: - Broaden http_client origin scoping (is_warp_server_origin) to cover the RTC HTTP origin, so the agent-event SSE streams carry the IAP token. - Add WebSocket::connect_with_headers so the token can ride on the websocket upgrade handshake (where IAP validates), plus connect_error_http_response for challenge introspection. - Thread the handshake header through start_graphql_streaming_operation (Warp Drive RTC) and the session-sharing connect/reconnect sites (sharer + viewer). - Detect IAP challenges on websocket handshakes to trigger a reactive refresh; make all SSE entry points consistent; gate get_cached on token expiry. - Resolve gcloud via the interactive shell PATH when Warp is launched from the macOS GUI. - Add unit tests for the IAP helpers (JWT exp parsing, challenge detection, origin scoping) and the IapState transitions. Co-Authored-By: Oz <oz-agent@warp.dev>
IsaiahWitzke
changed the title
|
/oz-review |
|
I'm re-reviewing this pull request in response to a review request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
![oz-for-oss[bot]](https://avatars.githubusercontent.com/in/3450139?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overview
This PR broadens staging IAP coverage to additional Warp-server traffic, websocket handshakes, and gcloud PATH resolution.
Concerns
- Shared-session sharer reconnect attempts attach the IAP header but never report IAP handshake challenges from retry failures, so a rejected cached token is retried unchanged until retry exhaustion or a later proactive refresh.
- Shared-session viewer reconnect has the same gap: retry failures do not notify the IAP manager, preventing the intended refresh-before-next-attempt behavior.
- No approved spec context was available for implementation comparison.
Verdict
Found: 0 critical, 2 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
It was a redundant public accessor with a single internal caller; fold the cached-token lookup directly into iap_handshake_header. Co-Authored-By: Oz <oz-agent@warp.dev>
IsaiahWitzke
changed the title
…nused bool returns - Extract notify_iap_challenge_if_detected(status, headers) as the private shared core for all three challenge-detection paths (HTTP response, SSE, websocket handshake) — no more duplicated log + emit logic. - Rename report_ws_iap_challenge -> check_ws_connect_for_iap_challenge, consistent with check_for_iap_challenge. - Move check_ws_connect_for_iap_challenge next to check_for_iap_challenge so the three detectors are grouped together. - Drop the unused bool return values from check_ws_connect_for_iap_challenge and notify_iap_challenge_if_detected (no caller inspected them). - Update stale doc comments. Co-Authored-By: Oz <oz-agent@warp.dev>
When a websocket reconnect attempt fails with an IAP challenge, the RequestFailedRetryPending arm was silently swallowing the error and retrying with the same stale token, guaranteeing repeated failures until retries exhausted. Now all three reconnect error paths call check_ws_connect_for_iap_challenge: - sharer initial connect failure (already had it, renamed) - sharer RequestFailedRetryPending - viewer RequestFailedRetryPending This fires IapChallengeReceived -> IapManager refresh. Since the retry closure re-reads iap_handshake_header() each attempt, the refreshed token is picked up on the next try. Co-Authored-By: Oz <oz-agent@warp.dev>
Centralize the IAP Proxy-Authorization header format in http_client::iap::proxy_auth_header and build it on IapState so the HTTP client, GraphQL, and websocket-handshake paths share one definition. IapManager now hands out the shared Arc<IapState> (mirroring AuthStateProvider), and the session-sharing websocket sites read the header from it instead of grabbing ServerApi just for the token. Websocket-handshake IAP challenge detection is centralized in the iap module (ws_connect_is_iap_challenge) and reached via IapManager where a ModelContext is available, or a thin ServerApi accessor for the ctx-less Warp Drive subscription in object.rs. Co-Authored-By: Oz <oz-agent@warp.dev>
IsaiahWitzke
force-pushed
the
iw/iap-origin-coverage
branch
from
16855b9 to
bd9e747
Compare
|
I'm starting a first review of this pull request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
IsaiahWitzke
changed the title
PATH after init)
The session-sharing websocket sites now read the IAP header via IapManager, leaving the Warp Drive subscription in object.rs as the only caller. Since object.rs is a child module of server_api, it can read the private iap_state field directly, so drop the one-off ServerApi accessor and inline the IapState::proxy_auth_header lookup at the call site. Co-Authored-By: Oz <oz-agent@warp.dev>
There was a problem hiding this comment.
Overview
This PR broadens IAP coverage for Warp-server-family HTTP/WebSocket traffic and changes IAP token refresh to resolve gcloud through the interactive shell PATH. I found two correctness issues that can either wedge IAP refresh or crash shared-session viewer startup on wasm paths.
Concerns
- The IAP refresh path now awaits interactive PATH discovery before entering the existing
gcloudtimeout, so a hanging shell startup can leave the state permanentlyRefreshingand suppress later challenge-triggered retries. - Shared-session viewer code now unconditionally reads/updates
IapManager, but that singleton is only registered on non-wasm targets while the viewer network has wasm support.
Verdict
Found: 0 critical, 2 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
- Bound the interactive login-shell PATH capture in IapManager::start_refresh with a timeout. A hung shell startup would otherwise leave the state machine stuck in Refreshing forever, disabling all future refreshes and IAP challenges. On timeout, fall back to the ambient PATH so the GCLOUD_TIMEOUT-guarded fetch can still run. - Register IapManager on all targets (including wasm) so the shared-session viewer network can read the singleton without panicking. On wasm iap_state is always None, making it an inert no-op. Co-Authored-By: Oz <oz-agent@warp.dev>
Co-Authored-By: Oz <oz-agent@warp.dev>
| cmd | ||
| // Prevent gcloud from waiting for interactive input (fail fast instead of hanging) |
There was a problem hiding this comment.
This comment feels like its not in the right spot
There was a problem hiding this comment.
i think it is? the line below sets stdin = NULL which should make any processes waiting for CLI user input just see EOF...
are you suggesting i shift it up a line and say "im setting stdin = NULL to prevent gcloud from ..."?
|
/oz-review |
|
I'm re-reviewing this pull request in response to a review request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
Follows the repo convention of placing unit tests in a sibling
${filename}_tests.rs file included via #[path].
Co-Authored-By: Oz <oz-agent@warp.dev>
There was a problem hiding this comment.
Overview
This PR broadens staging IAP coverage across Warp-server-family HTTP and websocket traffic, adds websocket handshake header support for native clients, threads IAP challenge detection through the affected streaming/session-sharing paths, and resolves gcloud via the interactive shell PATH for token refreshes.
Concerns
- No blocking correctness, security, or spec-alignment concerns found in the annotated diff.
Verdict
Found: 0 critical, 0 important, 0 suggestions
Approve
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
IsaiahWitzke
force-pushed
the
iw/iap-origin-coverage
branch
from
c348160 to
6f31958
Compare
The shared-session viewer/sharer Network reads the IapManager singleton, which the test app builders never registered, causing panics. Register a disabled (None-state) IapManager — an inert no-op — in the terminal-view and pane_group test harnesses and the sharer reconnect test. Co-Authored-By: Oz <oz-agent@warp.dev>
2 participants
Description
2 issues are being fixed here:
Proxy-Authorizationtoken rides on all Warp-server-family traffic — not just the mainserver_root_url()pool.get_interactive_path_env_varbefore trying to invoke thegcloudcli to grab iap token so that your gcloud executable doesnt need to be globally finable (i.e. need to wait before zshrc is done). See hereWhat & how:
http_client): broadeniap_token_forvia a newis_warp_server_originhelper to cover bothserver_root_url()andrtc_http_url(), so the agent-event SSE streams (served by warp-server-rtc) carry the IAP token.websocket): add cross-platformWebSocket::connect_with_headersso the token rides on the HTTP upgrade itself — the only place IAP validates (the graphql-wsconnection_initpayload and app-level Initialize messages are sent post-upgrade). Addsconnect_error_http_responsefor challenge introspection. Native-only; wasm ignores handshake headers.graphql+object.rs): thread the handshake header throughstart_graphql_streaming_operation; emitIapChallengeReceivedon a rejected handshake so theIapManagerrefreshes and the listener's retry loop reconnects with a fresh token.sharer+viewernetwork): attach the header at all connect/reconnect sites (re-read per reconnect attempt), with challenge detection on the initial connects._for_ancestor,_for_task) with IAP challenge detection; gateIapState::get_cachedon token expiry so a delayed proactive refresh can't attach an already-expired token.gcloudvia the interactive shell PATH when Warp is launched from the macOS GUI.