Skip to content

Commit

Permalink
Set a Path on the CSRF cookie (#944)
Browse files Browse the repository at this point in the history
Without setting a Path, the UA will infer the Path from the URL path,
meaning that it will take the "directory name" of a URL path (so for a
response for a resource at `/index`, the path would be inferred as `/`,
and for a response for `/foo/bar` it would be inferred to be `/foo/`
etc.).

This is problematic with CSRF for multiple reasons:

- It causes a proliferation of CSRF cookies which pollute client-side
  storage.
- It breaks CSRF when requests are sent across paths. For example, if a
  resource at `/foo/bar` contains a form which submits to `/index`, they
  would be theoretically using different CSRF states.
- Poem only processes a single cookie, which means that we have to rely
  on the ordering specified in RFC 6265 Section 5.4. This is bad for two
  reasons:

  1. The ordering is only a SHOULD, not a MUST.
  2. Poem does, in fact, not process cookies in the correct ordering
     (this is subject of another commit to fix).

Note that this commit may break applications if they share a CSRF cookie
name with other applications hosted at different paths on the same
domain.
  • Loading branch information
horazont authored Jan 4, 2025
1 parent e5b6317 commit 61d625a
Showing 1 changed file with 20 additions and 1 deletion.
21 changes: 20 additions & 1 deletion poem/src/middleware/csrf.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use std::{sync::Arc, time::Duration};
use std::{borrow::Cow, sync::Arc, time::Duration};

use base64::{engine::general_purpose::STANDARD, Engine};
use libcsrf::{
Expand Down Expand Up @@ -83,6 +83,10 @@ pub struct Csrf {
http_only: bool,
same_site: Option<SameSite>,
ttl: Duration,
// Using Arc<Cow<'static, _>> here because the path is most likely going
// to be a static str, and if it's not, we don't want to have to copy it
// into every endpoint.
path: Arc<Cow<'static, str>>,
}

impl Default for Csrf {
Expand All @@ -94,6 +98,7 @@ impl Default for Csrf {
http_only: true,
same_site: Some(SameSite::Strict),
ttl: Duration::from_secs(24 * 60 * 60),
path: Arc::new(Cow::Borrowed("/")),
}
}
}
Expand Down Expand Up @@ -147,6 +152,17 @@ impl Csrf {
}
}

/// Set the path for which the CSRF cookie should be set.
///
/// By default, this is `"/"`.
#[must_use]
pub fn path(self, value: impl Into<Cow<'static, str>>) -> Self {
Self {
path: Arc::new(value.into()),
..self
}
}

/// Sets the protection ttl. This will be used for both the cookie
/// expiry and the time window over which CSRF tokens are considered
/// valid.
Expand All @@ -170,6 +186,7 @@ impl<E: Endpoint> Middleware<E> for Csrf {
http_only: self.http_only,
same_site: self.same_site,
ttl: self.ttl,
path: Arc::clone(&self.path),
})
}
}
Expand All @@ -184,6 +201,7 @@ pub struct CsrfEndpoint<E> {
http_only: bool,
same_site: Option<SameSite>,
ttl: Duration,
path: Arc<Cow<'static, str>>,
}

impl<E> CsrfEndpoint<E> {
Expand Down Expand Up @@ -226,6 +244,7 @@ impl<E: Endpoint> Endpoint for CsrfEndpoint<E> {
cookie.set_http_only(self.http_only);
cookie.set_same_site(self.same_site);
cookie.set_max_age(self.ttl);
cookie.set_path(&**self.path);
cookie
};

Expand Down

0 comments on commit 61d625a

Please sign in to comment.