It's not possible to redirect in a server function if the request comes from a plain HTML form #3298

veigaribo · 2024-11-29T11:11:16Z

I'm trying to be as thorough as possible, so sorry if there's a mistake somewhere.

Also I'm not sure I would consider this a bug, or rather a limitation caused by the server_fn error-handling abstraction that is there by design, but recent discussions in the Discord server have led me to think of it as the former.

Describe the bug

When a server function is invoked through a plain HTML form, presumably because JS and/or WASM is not being used, the following line of code will set the HTTP status code and the Location HTTP header of the response to some values, to allow for server-side rendering of errors (AFAIK):

leptos/server_fn/src/lib.rs

Line 291 in 5947aa2

res.redirect(referer.as_deref().unwrap_or("/"));

But, as a consequence, it is not possible to make the server function redirect anywhere else, since the above will end up overwriting the HTTP response in the end.

To Reproduce
Steps to reproduce the behavior:

Write a server function that tries to redirect somewhere different from the originating URL, for example using leptos_actix::redirect or leptos_axum::redirect;
Write a form that invokes that function, for example using an ActionForm with a ServerAction;
Load and submit that form in a browser with JavaScript disabled;
Note that the value of the Location header of the response is the originating URL, not what you set (presumably your browser will allow you to do that);
Optionally try the above with JavaScript enabled and see that the Location header is correct.

Expected behavior

I think the Location response header, as well as the status code, should end up being set to the values that were set in the server function, but I don't know how that would be the case while preserving the current behavior.

Additional context

I was originally hoping that the form-redirects feature could be turned off from leptos entirely, so that I could sidestep this problem by not enabling the default features; but, if this really is a bug, then there must be a better solution. Allowing disabling of form-redirects would have its problems too.

I am also a proponent of the idea that server functions should be distinct from server requests & responses, i.e. I believe server functions should just do the work and have a function-invocation interface, and there would be another entity that defines how a given function should be interacted with HTTP-wise. In that paradigm, the current form-redirects behavior could just be part of the default HTTP API of a server function, but it would be possible to be overwritten if desired.

To be clear, I'm not asking for that to be implemented at all. If I had the time, I would (try to) do it myself in a different crate. Just mentioning it as food for thought.

The text was updated successfully, but these errors were encountered:

gbj · 2024-11-29T16:38:38Z

Here's my attempt to reproduce the behavior you describe

use leptos::prelude::*;

pub fn shell(options: LeptosOptions) -> impl IntoView {
    view! {
        <!DOCTYPE html>
        <html lang="en">
            <head>
                <meta charset="utf-8"/>
                <meta name="viewport" content="width=device-width, initial-scale=1"/>
            </head>
            <body>
                <App/>
            </body>
        </html>
    }
}

#[server]
pub async fn redirect_me() -> Result<(), ServerFnError> {
    tokio::time::sleep(std::time::Duration::from_secs(1)).await;
    leptos_axum::redirect("/foo");
    Ok(())
}

#[component]
pub fn App() -> impl IntoView {
    let action = ServerAction::<RedirectMe>::new();
    view! {
        <ActionForm action>
            <input type="submit"/>
        </ActionForm>
    }
}

Due to the lack of hydration scripts there, this behaves as a plain old multi-page app and sends a regular form request.

This does, in fact, return a 302 response with Location: /foo, and redirects to /foo.

So it seems that the reproduction requires something more specific than the steps you wrote above.

Could you please provide a small, self-contained example that reproduces the problem?

Thanks.

veigaribo · 2024-11-29T21:05:56Z

So, I've been struggling a lot to produce an example.

First, I tried doing it in a project using 0.6, with the start-actix template, but after much digging I realized that it works fine (apparently) there, because the following seems to handle everything correctly

leptos/integrations/actix/src/lib.rs

Lines 251 to 262 in 1ca279a

    
           // Location is set to redirect to Referer in the server handler handler by default, 
        
           // but it can only have a single value 
        
           // 
        
           // if we have used redirect() we will end up appending this second Location value 
        
           // to the first one, which will cause an invalid response 
        
           // see https://github.com/leptos-rs/leptos/issues/2506 
        
           for location in res_parts.headers.remove(header::LOCATION) { 
        
               headers.insert(header::LOCATION, location); 
        
           } 
        
           for (k, v) in std::mem::take(&mut res_parts.headers) { 
        
               headers.append(k, v); 
        
           }

(I did not know that existed)

Then I moved on to try to reproduce it on 0.7, which is where I discovered the problem in the first place. It seemed promising, given that the new code tries to do the same thing that is already done in server_fn:

leptos/integrations/actix/src/lib.rs

Lines 378 to 391 in 5947aa2

    
           // it it accepts text/html (i.e., is a plain form post) and doesn't already have a 
        
           // Location set, then redirect to to Referer 
        
           if accepts_html { 
        
               if let Some(referrer) = referrer { 
        
                   let has_location = 
        
                       res.0.headers().get(LOCATION).is_some(); 
        
                   if !has_location { 
        
                       *res.0.status_mut() = StatusCode::FOUND; 
        
                       res.0 
        
                           .headers_mut() 
        
                           .insert(LOCATION, referrer); 
        
                   } 
        
               } 
        
           }

server_fn:

leptos/server_fn/src/lib.rs

Lines 273 to 292 in 5947aa2

    
           // if it accepts HTML, we'll redirect to the Referer 
        
           #[cfg(feature = "form-redirects")] 
        
           if accepts_html { 
        
               // if it had an error, encode that error in the URL 
        
               if let Some(err) = err { 
        
                   if let Ok(url) = ServerFnUrlError::new(Self::PATH, err) 
        
                       .to_url(referer.as_deref().unwrap_or("/")) 
        
                   { 
        
                       referer = Some(url.to_string()); 
        
                   } 
        
               } 
        
               // otherwise, strip error info from referer URL, as that means it's from a previous 
        
               // call 
        
               else if let Some(referer) = referer.as_mut() { 
        
                   ServerFnUrlError::<Self::Error>::strip_error_info(referer) 
        
               } 
        
               // set the status code and Location header 
        
               res.redirect(referer.as_deref().unwrap_or("/")); 
        
           }

Furthermore, the let has_location = res.0.headers().get(LOCATION).is_some(); check seems to be guaranteed to be true, precisely because server_fn should have already set Location.

However, when I ran the application, instead of confirming the bug I have been seeing in my real application all along, I saw my browser displaying an error and refusing to show anything.

Resending the request in cURL, I could see that there were two Location headers set: my own and the one from server_fn.

That makes total sense, since server_fn is setting the Location header in the real response, and I am setting my header in ResponseOptions, and every header in ResponseOptions is later added to the real response:

leptos/integrations/actix/src/lib.rs

Lines 393 to 395 in 5947aa2

    
           // apply status code and headers if used changed them 
        
           res.extend_response(&res_options); 
        
           res.0

leptos/integrations/actix/src/lib.rs

Lines 171 to 183 in 5947aa2

    
           fn extend_response(&mut self, res_options: &Self::ResponseOptions) { 
        
               let mut res_options = res_options.0.write(); 
        
               let headers = self.0.headers_mut(); 
        
               for (key, value) in std::mem::take(&mut res_options.headers) { 
        
                   headers.append(key, value); 
        
               } 
        
               // Set status to what is returned in the function 
        
               if let Some(status) = res_options.status { 
        
                   *self.0.status_mut() = status; 
        
               } 
        
           }

So I came back to my real application to try to understand why it doesn't send two Location headers, but rather just an incorrect one... and the reality is that I don't know. I can print the HttpResponse at the end of the handle_server_fns_with_context handler, and it does contain two Location headers. There must be something that elimi —

Actually I had an epiphany writing this and realized that it was NGINX that was doing it. So the problem really is that both headers are being sent, not one overwriting the other.

Having said that, I made a repo at https://github.com/veigaribo/leptos-server-fn-redirect-bug.

gbj · 2024-11-29T21:26:30Z

Any interest in making a PR, based on what you've discovered, such that it inserts the Location header rather than appending it, when extending the ResponseOptions? It's possible that there are other headers that can only exist once -- the problem is that there are others that do need to support multiple, so not all can simply be inserted and replace the old one.

veigaribo · 2024-11-30T21:00:38Z

I can give it a shot.
Are you thinking of changing only extend_response? (to be like it was in 0.6, I'd assume)

gbj · 2024-12-01T20:47:04Z

Yes specifically this:

     for (key, value) in std::mem::take(&mut res_options.headers) { 
         headers.append(key, value); 
     }

could be special-cased to check whether it's Location, and then "insert" (overwriting the existing value) rather than "appending" (adding it as a second header), if that's the issue.

veigaribo · 2024-12-05T20:35:11Z

Closing since the discussed PR has been merged

gbj added the needs reproduction label Nov 29, 2024

veigaribo added a commit to veigaribo/leptos that referenced this issue Dec 1, 2024

fix: prevent multiple location headers on redirect (leptos-rs#3298)

b1351b8

veigaribo mentioned this issue Dec 2, 2024

fix: prevent multiple location headers on redirect (#3298) #3311

Merged

benwis pushed a commit that referenced this issue Dec 2, 2024

fix: prevent multiple location headers on redirect (#3298) (#3311)

fcba8b3

veigaribo closed this as completed Dec 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

It's not possible to redirect in a server function if the request comes from a plain HTML form #3298

It's not possible to redirect in a server function if the request comes from a plain HTML form #3298

veigaribo commented Nov 29, 2024

gbj commented Nov 29, 2024

veigaribo commented Nov 29, 2024

gbj commented Nov 29, 2024

veigaribo commented Nov 30, 2024

gbj commented Dec 1, 2024

veigaribo commented Dec 5, 2024

It's not possible to redirect in a server function if the request comes from a plain HTML form #3298

It's not possible to redirect in a server function if the request comes from a plain HTML form #3298

Comments

veigaribo commented Nov 29, 2024

gbj commented Nov 29, 2024

veigaribo commented Nov 29, 2024

gbj commented Nov 29, 2024

veigaribo commented Nov 30, 2024

gbj commented Dec 1, 2024

veigaribo commented Dec 5, 2024