diff --git a/backend/windmill-api/src/mcp/oauth_server.rs b/backend/windmill-api/src/mcp/oauth_server.rs
index 48c4f73c44..59b99db8ef 100644
--- a/backend/windmill-api/src/mcp/oauth_server.rs
+++ b/backend/windmill-api/src/mcp/oauth_server.rs
@@ -9,6 +9,7 @@ use axum::{
use serde::{Deserialize, Serialize};
use sha2::{Digest, Sha256};
use sqlx::FromRow;
+use url::{Host, Url};
use windmill_common::{
auth::{hash_token, TOKEN_PREFIX_LEN},
error::{Error, Result},
@@ -296,6 +297,10 @@ pub async fn oauth_register(
));
}
+ for redirect_uri in &req.redirect_uris {
+ validate_redirect_uri(redirect_uri)?;
+ }
+
let client_id = format!("mcp-client-{}", rd_string(16));
sqlx::query!(
@@ -376,6 +381,12 @@ async fn handle_authorization_code_grant(
return Err(OAuthTokenError::invalid_grant("redirect_uri mismatch"));
}
+ if validate_redirect_uri(&auth_code.redirect_uri).is_err() {
+ return Err(OAuthTokenError::invalid_grant(
+ "redirect_uri bound to this code is not allowed",
+ ));
+ }
+
let challenge = auth_code.code_challenge.as_ref().ok_or_else(|| {
OAuthTokenError::invalid_grant("Authorization code missing PKCE challenge")
})?;
@@ -646,6 +657,13 @@ async fn oauth_authorize_inner(
.into_response();
}
+ // Re-validate even though it is registered: a client registered before
+ // redirect_uri validation existed may carry an unsafe (poisoned) URI.
+ // Respond with JSON, never a redirect, since the target itself is unsafe.
+ if let Err(e) = validate_redirect_uri(¶ms.redirect_uri) {
+ return OAuthJsonError::new("invalid_request", Some(&e.to_string())).into_response();
+ }
+
if params.response_type != "code" {
return OAuthErrorRedirect::new(
¶ms.redirect_uri,
@@ -762,6 +780,11 @@ async fn oauth_approve_inner(
));
}
+ // Re-validate registered URI in case the client predates redirect_uri
+ // validation (poisoned row) — prevents issuing a code bound to an
+ // unsafe redirect target.
+ validate_redirect_uri(&form.redirect_uri)?;
+
if form.code_challenge.is_empty() {
return Err(Error::BadRequest(
"PKCE required: code_challenge is mandatory".to_string(),
@@ -834,6 +857,60 @@ pub async fn workspaced_oauth_approve(
oauth_approve_inner(&db, &authed, &workspace_id, form).await
}
+/// Validates an OAuth redirect URI to prevent open-redirect and
+/// `javascript:`/`data:` same-origin script execution.
+///
+/// Allowed: absolute `https://` URIs, and `http://` only for loopback hosts
+/// (`localhost`, `127.0.0.0/8`, `[::1]`) per RFC 8252 §7.3 native-app guidance.
+/// Rejected: any other scheme (`javascript:`, `data:`, `file:`, custom schemes),
+/// fragments, and embedded credentials (userinfo) per OAuth 2.0 Security BCP.
+///
+/// Applied at registration *and* re-applied at every authorize/approve/token
+/// boundary so clients registered before this check (poisoned rows) can never
+/// reach a redirect sink.
+fn validate_redirect_uri(redirect_uri: &str) -> Result<()> {
+ let url = Url::parse(redirect_uri)
+ .map_err(|_| Error::BadRequest("redirect_uri is not a valid absolute URI".to_string()))?;
+
+ if url.fragment().is_some() {
+ return Err(Error::BadRequest(
+ "redirect_uri must not contain a fragment".to_string(),
+ ));
+ }
+
+ if !url.username().is_empty() || url.password().is_some() {
+ return Err(Error::BadRequest(
+ "redirect_uri must not contain embedded credentials".to_string(),
+ ));
+ }
+
+ match url.scheme() {
+ "https" => Ok(()),
+ "http" => {
+ let is_loopback = match url.host() {
+ Some(Host::Domain(d)) => d == "localhost",
+ Some(Host::Ipv4(ip)) => ip.is_loopback(),
+ Some(Host::Ipv6(ip)) => ip.is_loopback(),
+ None => false,
+ };
+ if is_loopback {
+ Ok(())
+ } else {
+ Err(Error::BadRequest(
+ "redirect_uri with http scheme is only allowed for loopback hosts \
+ (localhost, 127.0.0.0/8, [::1])"
+ .to_string(),
+ ))
+ }
+ }
+ scheme => Err(Error::BadRequest(format!(
+ "redirect_uri scheme '{}:' is not allowed; must be https \
+ (or http for loopback hosts)",
+ scheme
+ ))),
+ }
+}
+
/// PKCE validation (S256 only)
fn validate_pkce_s256(verifier: &str, challenge: &str) -> bool {
let mut hasher = Sha256::new();
@@ -882,6 +959,12 @@ impl OAuthErrorRedirect {
impl IntoResponse for OAuthErrorRedirect {
fn into_response(self) -> axum::response::Response {
+ // Never emit a Location header to an unsafe target, even on error
+ // paths. Callers validate upstream; this is the last line of defense.
+ if validate_redirect_uri(&self.redirect_uri).is_err() {
+ return OAuthJsonError::new(&self.error, self.error_description.as_deref())
+ .into_response();
+ }
let mut url = format!("{}?error={}", self.redirect_uri, self.error);
if let Some(desc) = &self.error_description {
url.push_str(&format!("&error_description={}", urlencoding::encode(desc)));
@@ -992,3 +1075,58 @@ pub fn gateway_unauthed_service() -> Router {
pub fn gateway_authed_service() -> Router {
Router::new().route("/approve", post(gateway_oauth_approve))
}
+
+#[cfg(test)]
+mod tests {
+ use super::validate_redirect_uri;
+
+ #[test]
+ fn accepts_https() {
+ assert!(validate_redirect_uri("https://example.com/oauth/callback").is_ok());
+ assert!(validate_redirect_uri("https://example.com:8443/cb?x=1").is_ok());
+ }
+
+ #[test]
+ fn accepts_http_loopback() {
+ assert!(validate_redirect_uri("http://localhost/cb").is_ok());
+ assert!(validate_redirect_uri("http://localhost:1455/cb").is_ok());
+ assert!(validate_redirect_uri("http://127.0.0.1:8080/cb").is_ok());
+ assert!(validate_redirect_uri("http://127.0.0.2/cb").is_ok());
+ assert!(validate_redirect_uri("http://[::1]:9000/cb").is_ok());
+ }
+
+ #[test]
+ fn rejects_javascript_scheme() {
+ // The core exploit: javascript: that survives `?code=` concatenation.
+ assert!(validate_redirect_uri("javascript:fetch('/api/users/tokens/create')//").is_err());
+ assert!(validate_redirect_uri("JavaScript:alert(1)").is_err());
+ }
+
+ #[test]
+ fn rejects_other_unsafe_schemes() {
+ assert!(validate_redirect_uri("data:text/html,").is_err());
+ assert!(validate_redirect_uri("file:///etc/passwd").is_err());
+ assert!(validate_redirect_uri("vbscript:msgbox(1)").is_err());
+ assert!(validate_redirect_uri("com.evil.app:/cb").is_err());
+ }
+
+ #[test]
+ fn rejects_non_loopback_http() {
+ assert!(validate_redirect_uri("http://example.com/cb").is_err());
+ assert!(validate_redirect_uri("http://169.254.169.254/cb").is_err());
+ }
+
+ #[test]
+ fn rejects_fragment_and_credentials() {
+ assert!(validate_redirect_uri("https://example.com/cb#frag").is_err());
+ assert!(validate_redirect_uri("https://user:pass@example.com/cb").is_err());
+ assert!(validate_redirect_uri("https://user@example.com/cb").is_err());
+ }
+
+ #[test]
+ fn rejects_relative_and_garbage() {
+ assert!(validate_redirect_uri("/relative/path").is_err());
+ assert!(validate_redirect_uri("not a url").is_err());
+ assert!(validate_redirect_uri("").is_err());
+ }
+}
diff --git a/frontend/src/routes/(root)/(logged)/oauth/mcp_authorize/+page.svelte b/frontend/src/routes/(root)/(logged)/oauth/mcp_authorize/+page.svelte
index 1c9753bc9b..8b4f2b2a7a 100644
--- a/frontend/src/routes/(root)/(logged)/oauth/mcp_authorize/+page.svelte
+++ b/frontend/src/routes/(root)/(logged)/oauth/mcp_authorize/+page.svelte
@@ -19,6 +19,38 @@
let codeChallenge = page.url.searchParams.get('code_challenge') || ''
let codeChallengeMethod = page.url.searchParams.get('code_challenge_method') || ''
+ // Loopback hosts allowed over http per RFC 8252 §7.3 (native apps).
+ const LOOPBACK_HOSTS = new Set(['localhost', '127.0.0.1', '[::1]'])
+
+ // Parse and scheme-check the redirect URI. `new URL()` alone is NOT a
+ // safety check: `new URL('javascript:fetch(1)')` parses successfully with
+ // protocol 'javascript:'. The protocol allowlist is what blocks the
+ // same-origin script-execution vector.
+ function parseSafeRedirectUri(raw: string): URL | null {
+ let url: URL
+ try {
+ url = new URL(raw)
+ } catch {
+ return null
+ }
+ const isHttps = url.protocol === 'https:'
+ const isLoopbackHttp = url.protocol === 'http:' && LOOPBACK_HOSTS.has(url.hostname)
+ return isHttps || isLoopbackHttp ? url : null
+ }
+
+ function buildSafeRedirectUrl(raw: string, params: URLSearchParams): string | null {
+ const url = parseSafeRedirectUri(raw)
+ if (!url) {
+ return null
+ }
+ for (const [key, value] of params) {
+ url.searchParams.set(key, value)
+ }
+ return url.toString()
+ }
+
+ let redirectUriValid = $derived(parseSafeRedirectUri(redirectUri) !== null)
+
let loading = $state(false)
let success = $state(false)
let successRedirectUrl = $state('')
@@ -59,7 +91,15 @@
if (oauthState) {
params.set('state', oauthState)
}
- window.location.href = `${redirectUri}?${params.toString()}`
+ const url = buildSafeRedirectUrl(redirectUri, params)
+ if (!url) {
+ sendUserToast(
+ 'Refusing to redirect: the redirect URI is invalid or uses an unsafe scheme',
+ true
+ )
+ return
+ }
+ window.location.href = url
}
async function onApprove() {
@@ -103,7 +143,15 @@
if (data.state) {
params.set('state', data.state)
}
- const url = `${redirectUri}?${params.toString()}`
+ const url = buildSafeRedirectUrl(redirectUri, params)
+ if (!url) {
+ sendUserToast(
+ 'Authorization granted but the redirect URI is invalid or uses an unsafe scheme; not redirecting',
+ true
+ )
+ loading = false
+ return
+ }
success = true
successRedirectUrl = url
loading = false
@@ -121,7 +169,12 @@
}
-{#if !isGateway && !workspaceId}
+{#if !redirectUriValid}
+
+ Error: the redirect URI is missing, malformed, or uses an unsafe scheme. This authorization
+ request cannot be completed.
+
+{:else if !isGateway && !workspaceId}
Error: missing workspace_id
{:else}