fix: handle cross-origin redirects in server function redirect hook

[haslersn]

Fixes #2269

[gbj]

Thanks very much! This is the right way to do it, but see the note about client-side redirects to the same origin.

[gbj]

This check fails for local URLs like /foobar because in the browser, Url::try_from inserts a fake http://leptos base:

leptos/router/src/history/url.rs

Lines 47 to 57 in a903e19

	let url = web_sys::Url::new_with_base(
	&if url.starts_with("//") {
	let origin =
	leptos::window().location().origin().unwrap_or_default();
	format!("{origin}{url}")
	} else {
	url.to_string()
	},
	"http://leptos",
	)
	.map_js_error()?;

This is simply because the new URL() constructor will fail with an incomplete URL

new URL("/foobar");
// throws `Uncaught TypeError: URL constructor: /foobar is not a valid URL.`

I think it's safe to add a url.origin = current_origin || url.origin == "http://leptos", maybe refactoring these two places to share a constant, or adding a helper method to Url like is_app_origin.

[haslersn]

Why does this have a double slash here?

leptos/router/src/history/url.rs

Line 48 in a903e19

&if url.starts_with("//") {

[haslersn]

What about the server? Would it also fail there (but doesn't, because that code isn't run on the server)?

The client-side code for Url::try_from() doesn't seem quite correct: I think we should apply the rules of https://datatracker.ietf.org/doc/html/rfc3986#section-4, i.e., treat the input as an URI-reference and resolve it against a base URI (probably the current window location). Examples in section 5.4.1.

[gbj]

Why does this have a double slash here?

leptos/router/src/history/url.rs

Line 48 in a903e19

&if url.starts_with("//") {

#1764 describes an issue in which using a route like http://my-app.com///foo causes an error, instead of matching the path //foo. Using something like new URL("//foo", window.location.origin) causes the browser to misinterpret this as the URL "https://foo/". So in that case, we manually build the whole URL from window.location.origin. In other cases, we just use new URL() with a fake base.

If you fire up the browser console it might make more sense: observe the difference in behavior between these two:

// this is fine
new URL("/foo", window.location.origin);
// this is not
new URL("//foo", window.location.origin);

Hence the check that's currently in place.

It would be possible to change this by always getting the window.location.origin and doing string concatenation, but note you'll need to check for a / at the beginning, as the following set also do not behavior properly

// good
new URL(window.location.origin + "/foo")
// good
new URL(window.location.origin + "//foo")
// bad
new URL(window.location.origin + "foo")

The client-side code for Url::try_from() doesn't seem quite correct: I think we should apply the rules of https://datatracker.ietf.org/doc/html/rfc3986#section-4, i.e., treat the input as an URI-reference and resolve it against a base URI (probably the current window location). Examples in section 5.4.1.

I might be misunderstanding you, but as far as I can tell neither the url crate nor the new URL() constructor in the browser do this kind of resolution; they simply require complete URLs, hence the workaround. I don't think it makes sense to implement the kind of resolution described in 5.4.1 in the context of our router.

[haslersn]

Using something like new URL("//foo", window.location.origin) causes the browser to misinterpret this as the URL "https://foo/".

Why "misinterpret"? According to the RFC, //foo is a network-path reference and it's correct behavior to resolve it as http://foo or https://foo (depending on whether the current origin uses http or https).

Try the following with JS/WASM disabled. The browser correctly links / redirects to http://example.com.

use leptos::*;
use leptos_meta::*;
use leptos_router::*;

#[component]
pub fn App() -> impl IntoView {
    provide_meta_context();

    let redirect = create_server_action::<Redirect>();
    
    view! {
        <Stylesheet id="leptos" href="/pkg/leptos-playground.css"/>
        <Title text="Leptos Playground"/>

        <a href="//example.com">Click me</a>
        <ActionForm action=redirect>
            <button type="submit">Redirect</button>
        </ActionForm>
    }
}

#[server(Redirect)]
async fn redirect() -> Result<(), ServerFnError> {
    leptos_axum::redirect("//example.com");

    Ok(())
}

[haslersn]

as far as I can tell neither the url crate nor the new URL() constructor in the browser do this kind of resolution

The new URL() constructor, when passing a base URI as 2nd argument, seems to do the resolution specified by https://datatracker.ietf.org/doc/html/rfc3986#section-4

[haslersn]

I just updated this PR. It now consists of two separate commits:

1. feat: provide two different Url constructors: with and without base

   The URL constructor without base (Url::try_from) requires an URL, so we need to enforce this for all callers. The only caller which called this with a path (i.e., without origin) was create_location(), so I changed create_location to always prepend http://leptos.dev to the path. The origin in this case doesn't matter, because create_location() doesn't use the origin.

   However, there was an inconsistency between client and server: previosly, create_location() was called with a path on the client and an URL (with origin http://leptos.rs or http://leptos.dev) on the server. I fixed this by always using a path on the server, too.

2. fix: handle cross-origin redirects in server function redirect hook

   In client-side navigation we now handle redirects returned from server functions the same way how the browser would handle them. This fixes cross-origin redirects.

   Note: this is a breaking change for relative locations such as Location: foo. Previously that was resolved to <origin>/foo. Now it's resolved as a URI-reference (RFC 3986) relative to the server function URL's (typically <origin>/api/something), resulting in <origin>/api/foo. This is required, because it's the same behavior as with JS/WASM disabled. In case you want to redirect to paths, make sure that it starts with a single slash (e.g. Location: /foo).

[gbj]

Specific comments:

1. This currently panics if you try to load a page using Actix. The PR updates the logic for handling paths from ServerIntegration in the router, and updates the Axum integration, but not the Actix integration.
2. This would panic on a redirect in the server fn client if the client is not running in a web browser (see specific comment there).

General comment: I am hesitant to introduce these sorts of breaking changes in order to address a relatively small bug. Not all of the server integrations live in this repository any more (e.g., viz and spin integrations), and changing the behavior of the router in a way that introduces panics in code that still compiles would be a serious issue. (This is the case for the Actix integration, and would be for the others even if the Actix one were fixed in this PR.)

I think your approach is probably correct, for the long term, but it would either need to be new minor version (i.e., from 0.6 to 0.7) to prevent these runtime issues, or it would need to accommodate both full URLs and paths only — maybe that's another possible route.

However, I do wonder whether a much more minimal approach would be sufficient to fix the bug, with a more-correct-but-breaking fix like this PR included in a future release.

Suggested solution
For example, simply changing router_hook to the below seems to me to yield correct behavior for both local client-side redirects and other origins:

    let router_hook = Box::new(move |path: &str| {
        let path = path.to_string();
        let Ok(origin) = window().location().origin() else {
            return;
        };
        let Ok(to_url) = web_sys::Url::new_with_base(&path, &origin) else {
            logging::error!("could not parse url {path:?}");
            return;
        };
        let to_origin = to_url.origin();
        // delay by a tick here, so that the Action updates *before* the redirect
        if to_origin != origin {
            _ = window().location().set_href(&path);
        } else {
            request_animation_frame({
                let navigate = navigate.clone();
                move || {
                    navigate(&path, Default::default());
                }
            });
        }
    }) as RedirectHook;

[gbj]

The server function client is not guaranteed to have access to browser APIs, because server functions can also be used in desktop applications via reqwest, so this logic need to live somewhere in leptos_router

[haslersn]

I pushed a change to the 1st commit that makes it backwards-compatible by also allowing an URL instead of a path in create_location(). In case an URL is provided, a warning is logged.

However, for the 2nd commit, in order to move the logic into leptos_router, the redirect hook would need to know which server function was called (or at least the server function's path), because the redirect hook needs use the server function's URL as a base. This requires changing the signature of the redirect hook – what do you think about this?

[haslersn]

Now I also changed the 2nd commit, moving the resolving logic into leptos_router. Currently it uses the window's origin as a base for resolving the redirect location. This is still incorrect (as the server function's URL should be used as a base instead). But at least cross-origin redirects are now fixed.

@gbj please review again.

I left a TODO for using the correct base. This probably requires a breaking change in the signature of the redirect hook.

[haslersn]

Would it be ok to unwrap() or unwrap_throw() here? Is the .origin() call always expected to succeed?

[gbj]

Doesn't the General Comment section of what I wrote above still apply? i.e., that changing the expected URL that's provided by the server integration would break the server integrations that live outside this repo (viz, spin)?

The Suggested Solution I posted above fixes issue #2269 in a non-breaking way. It seems this PR is trying to address that and the other issues you've identified at the same time, but in a breaking way.

I would propose opening separate issues for those separate issues, so that we can fix the specific issue of #2269 now and address those separately.

If you would prefer, we can keep this PR open, and I can fix #2269 separately.

[haslersn]

I created a smaller PR #2329 to fix #2269, and I will create one or more separate PRs for the remaining issues as soon as #2329 is merged.