Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cookies are set multiple times when using *server.nest* #396

Open
isgj opened this issue Jan 31, 2020 · 1 comment
Open

Cookies are set multiple times when using *server.nest* #396

isgj opened this issue Jan 31, 2020 · 1 comment

Comments

@isgj
Copy link
Contributor

isgj commented Jan 31, 2020

When setting a cookie in a nested service it is set multiple times depending on the level of the nested service. For example:

use async_std::task;
use cookie::Cookie;
use tide::{self, Response};

fn main() {
    task::block_on(async {
        let mut inner = tide::new();
        inner.at("/3").get(|_| async {
            let mut r = Response::new(200);
            r.set_cookie(Cookie::new("snack", "tuna"));
            r.body_string(String::from("ok"))
        });

        let mut second = tide::new();
        second.at("/2").nest(inner);

        let mut outer = tide::new();
        outer.at("/1").nest(second);

        outer
            .listen("localhost:8080")
            .await
            .expect("Error starting the server");
    })
}

When GET localhost:8080/1/2/3 the response is

HTTP/1.1 200 OK
content-type: text/plain; charset=utf-8
set-cookie: snack=tuna,snack=tuna,snack=tuna
transfer-encoding: chunked
date: Fri, 31 Jan 2020 10:39:54 GMT

ok

The cookie is set 3 times.

@yoshuawuyts
Copy link
Member

@isgj thanks for the bug report! I suspect this is because each instance of Server has its own instance of CookieMiddleware enabled, it looks at the CookieJar struct three times in a row, and sets the headers accordingly.

I see two ways of fixing this:

  1. Make the CookieJar value in Response and Request an Option, so that running it multiple times becomes a no-op.
  2. Make it so nesting applications disables default middleware for the nested applications.

I'm thinking the better solution here would be the second one. Because it would allow us to prevent future issues when we enable other middleware by default (e.g. logging comes to mind).

Implementation

I think the way to fix this is to remove this:

tide/src/server/mod.rs

Lines 206 to 208 in 4e2857a

middleware: vec![Arc::new(
crate::middleware::cookies::CookiesMiddleware::new(),
)],

And instead add it back inside Server::listen, so that the default middleware is only enabled once:

tide/src/server/mod.rs

Lines 308 to 310 in 4e2857a

let http_service = self.into_http_service();
let res = http_service_hyper::Server::builder(listener.incoming())

The default middleware should probably be pushed to the start of the middleware vector though, so that logging, cookies, etc. all run in a predetermined order before any userland middleware.

cc/ @tirr-c

@isgj isgj changed the title Cookies are set multiply times when using *server.nest* Cookies are set multiple times when using *server.nest* Feb 1, 2020
vladan added a commit to vladan/tide that referenced this issue Apr 24, 2020
Fixes http-rs#396 by ensuring the cookies and log middleware is only created
when we actually call .listen and not when nested.

This change adds a public `into_mock` method to Server so that an
instance with the cookie and log middleware can be crated from the
tests.
vladan added a commit to vladan/tide that referenced this issue Apr 24, 2020
Fixes http-rs#396 by ensuring the cookies and log middleware is only created
when we actually call .listen and not when nested.

This change adds a public `into_mock` method to Server so that an
instance with the cookie and log middleware can be crated from the
tests.
vladan added a commit to vladan/tide that referenced this issue Apr 26, 2020
Fixes http-rs#396 by ensuring the cookies and log middleware is only created
when we actually call .listen and not when nested.

This change adds a public `into_mock` method to Server so that an
instance with the cookie and log middleware can be crated from the
tests.
vladan added a commit to vladan/tide that referenced this issue May 31, 2020
Fixes http-rs#396 http-rs#476 http-rs#551 in that it removes the default logging and cookies
middlewares from `Server::with_state()` and allows users to use their
own middlewares, or use the default ones from the framework by calling
`with_cookies()` and/or `with_logging()` on the server instance.
vladan added a commit to vladan/tide that referenced this issue May 31, 2020
Fixes http-rs#396 http-rs#476 http-rs#551 in that it removes the default logging and cookies
middlewares from `Server::with_state()` and allows users to use their
own middlewares, or use the default ones from the framework by calling
`with_cookies()` and/or `with_logging()` on the server instance.
vladan added a commit to vladan/tide that referenced this issue Jun 17, 2020
Fixes http-rs#396 http-rs#476 http-rs#551 in that it removes the default logging and cookies
middlewares from `Server::with_state()` and allows users to use their
own middlewares, or use the default ones from the framework by calling
`with_cookies()` and/or `with_logging()` on the server instance.
vladan added a commit to vladan/tide that referenced this issue Jun 17, 2020
Fixes http-rs#396 http-rs#476 http-rs#551 in that it removes the default logging and cookies
middlewares from `Server::with_state()` and allows users to use their
own middlewares, or use the default ones from the framework by calling
`with_cookies()` and/or `with_logging()` on the server instance.
vladan added a commit to vladan/tide that referenced this issue Jun 17, 2020
Fixes http-rs#396 http-rs#476 http-rs#551 in that it removes the default logging and cookies
middlewares from `Server::with_state()` and allows users to use their
own middlewares, or use the default ones from the framework by calling
`with_cookies()` and/or `with_logging()` on the server instance.
vladan added a commit to vladan/tide that referenced this issue Jun 19, 2020
Fixes http-rs#396 http-rs#476 http-rs#551 in that it removes the default logging and cookies
middlewares from `Server::with_state()` and allows users to use their
own middlewares, or use the default ones from the framework by calling
`with_cookies()` and/or `with_logging()` on the server instance.
vladan added a commit to vladan/tide that referenced this issue Jun 22, 2020
Fixes http-rs#396 http-rs#476 http-rs#551 in that it removes the default logging and cookies
middlewares from `Server::with_state()` and allows users to use their
own middlewares, or use the default ones from the framework by calling
`with_cookies()` and/or `with_logging()` on the server instance.
vladan added a commit to vladan/tide that referenced this issue Jul 11, 2020
Fixes http-rs#396 http-rs#476 http-rs#551 in that it removes the default logging and cookies
middlewares from `Server::with_state()` and allows users to use their
own middlewares, or use the default ones from the framework by calling
`with_cookies()` and/or `with_logging()` on the server instance.
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 a pull request may close this issue.

2 participants