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.

Sign up for GitHub

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

fix(cookies): add {cookie-name}-parts cookie, style: inline formatting #107

Merged
merged 4 commits into from
Nov 4, 2024
Merged

fix(cookies): add {cookie-name}-parts cookie, style: inline formatting #107

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4e62916
style: use inline variables in html.rs
miwig Oct 18, 2024
ffbcc87
fix: store number of cookie parts in a cookie
miwig Oct 18, 2024
2bbcd4d
style: use inline formatting in errors.rs
miwig Oct 21, 2024
942ca79
chore: remove dead code
miwig Oct 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 15 additions & 44 deletions src/auth.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -570,7 +570,6 @@ impl ConfiguredOidc {
&new_nonce,
self.plugin_config.cookie_name.as_str(),
self.plugin_config.cookie_duration,
self.get_number_of_session_cookies() as u64,
);

// Build cookie headers
Expand All @@ -590,15 +589,7 @@ impl ConfiguredOidc {

/// Clear the cookies and redirect to the base path or `end_session_endpoint`.
fn logout(&self) -> Result<Action, PluginError> {
let cookie_values = Session::make_cookie_values(
// This is a bit hacky, but we need to write something into the cookie to clear all cookies (otherwise the
// `make_cookie_values` function will not overwrite all cookies, as "" is an empty chunk)
"clear",
"",
&self.plugin_config.cookie_name,
0,
self.get_number_of_session_cookies() as u64,
);
let cookie_values = Session::make_cookie_values("", "", &self.plugin_config.cookie_name, 0);

let mut headers = Session::make_set_cookie_headers(&cookie_values);

Expand Down Expand Up @@ -734,7 +725,6 @@ impl ConfiguredOidc {
&nonce,
self.plugin_config.cookie_name.as_str(),
self.plugin_config.cookie_duration,
self.get_number_of_session_cookies() as u64,
);

// Build cookie headers
Expand Down Expand Up @@ -835,30 +825,20 @@ impl ConfiguredOidc {
///
/// The session cookie as a string if found, an error otherwise
pub fn get_session_cookie_as_string(&self) -> Result<String, PluginError> {
// Find all cookies that have the cookie_name, split them by ; and remove the name from the cookie
// as well as the leading =. Then join the cookie values together again.
let cookie = self
.get_http_request_header("cookie")
.ok_or(PluginError::SessionCookieNotFoundError)?;

// Split cookie by ; and filter for the cookie name.
let cookies = cookie
.split(';')
.filter(|x| x.contains(self.plugin_config.cookie_name.as_str()))
.filter(|x| !x.contains(format!("{}-nonce", self.plugin_config.cookie_name).as_str()));

// Check if cookies have values
for cookie in cookies.clone() {
if cookie.split('=').collect::<Vec<&str>>().len() < 2 {
return Err(PluginError::SessionCookieNotFoundError);
}
}

// Then split all cookies by = and get the second element before joining all values together.
let values = cookies
.map(|x| x.split('=').collect::<Vec<&str>>()[1])
.collect::<Vec<&str>>()
// Join the cookie values together again.
let cookie_name = &self.plugin_config.cookie_name;

// Get the number of cookie parts
let num_parts: u8 = self
.get_cookie(&format!("{cookie_name}-parts"))
.unwrap_or_default()
.parse()
.map_err(|_| PluginError::SessionCookieNotFoundError)?;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically its not the Session Cookie right? We should add another Error for this type as Nonce also has its own error

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is worth the effort. I would consider all of the cookies together "the session cookie" because it doesn't really make a difference to the user which is missing, the end result is the same. In fact, I would like to refactor the error handling completely to use anyhow instead of defining a custom error type with thiserror. Initially I thought we might match on the error variant somewhere and do different things depending on the error, but we don't actually do that. Furthermore, PluginError is just a catch-all error type anyway, that doesn't reflect which errors a function can actually throw. fn validate_cookie(&self) -> Result<AuthorizationState, PluginError> will never return Err(PluginError::CodeNotFoundInCallbackError), but the return type makes it look like it could.


// Get the cookie parts and concatenate them into a string
let values = (0..num_parts)
.map(|i| self.get_cookie(&format!("{cookie_name}-{i}")))
.collect::<Option<Vec<String>>>()
.ok_or(PluginError::SessionCookieNotFoundError)?
.join("");

Ok(values)
Expand All @@ -869,13 +849,4 @@ impl ConfiguredOidc {
self.get_cookie(format!("{}-nonce", self.plugin_config.cookie_name).as_str())
.ok_or(PluginError::NonceCookieNotFoundError)
}

/// Helper function to get the number of session cookies from the request headers.
pub fn get_number_of_session_cookies(&self) -> usize {
let cookie = self.get_http_request_header("cookie").unwrap_or_default();
return cookie
.split(';')
.filter(|x| x.contains(&self.plugin_config.cookie_name))
.count();
}
}
12 changes: 6 additions & 6 deletions src/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ pub enum PluginError {
impl ConfiguredOidc {
pub fn show_error_page(&self, status_code: u32, title: &str, message: &str) {
let headers = vec![("cache-control", "no-cache"), ("content-type", "text/html")];
let request_id = self.request_id.clone().unwrap_or_default();

self.send_http_response(
status_code,
Expand All @@ -81,7 +82,7 @@ impl ConfiguredOidc {
<head>
<meta charset="UTF-8">
<meta name="viewport" content="width=device-width, initial-scale=1.0">
<title>Error - {}</title>
<title>Error - {status_code}</title>
<style>
:root {{
--bg-color: #f0f2f5;
Expand Down Expand Up @@ -194,10 +195,10 @@ impl ConfiguredOidc {
</label>
</div>
<div class="error-container">
<h1>Error {}</h1>
<h2>{}</h2>
<p>{}</p>
<p class="request-id">Request-ID: {}</p>
<h1>Error {status_code}</h1>
<h2>{title}</h2>
<p>{message}</p>
<p class="request-id">Request-ID: {request_id}</p>
</div>
<script>
const darkModeToggle = document.getElementById('darkModeToggle');
Expand Down Expand Up @@ -225,7 +226,6 @@ impl ConfiguredOidc {
</body>
</html>
"#,
status_code, status_code, title, message, self.request_id.clone().unwrap()
)
.as_bytes(),
),
Expand Down
7 changes: 3 additions & 4 deletions src/html.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,11 +227,11 @@ pub fn auth_page_html(provider_cards: String) -> String {
<div class="container">
<h1>Select a provider to authenticate with</h1>
<div class="provider-grid">
{}
{provider_cards}
</div>
</div>
<div class="footer">
<a href="https://github.com/antonengelhardt/wasm-oidc-plugin" target="_blank" rel="noopener noreferrer">wasm-oidc-plugin</a> v{}
<a href="https://github.com/antonengelhardt/wasm-oidc-plugin" target="_blank" rel="noopener noreferrer">wasm-oidc-plugin</a> v{version}
</div>
<script>
const darkModeToggle = document.getElementById('darkModeToggle');
Expand Down Expand Up @@ -272,7 +272,6 @@ pub fn auth_page_html(provider_cards: String) -> String {
</script>
</body>
</html>
"#,
provider_cards, version
"#
)
}
14 changes: 4 additions & 10 deletions src/session.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,6 @@ impl Session {
encoded_nonce: &str,
cookie_name: &str,
cookie_duration: u64,
number_current_cookies: u64,
) -> Vec<String> {
// Split every 4000 bytes
let cookie_parts = encoded_cookie
Expand All @@ -109,22 +108,17 @@ impl Session {
cookie_values.push(cookie_value);
}

let num_parts = cookie_values.len();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a comment which explains what you are doing here :)

let num_parts_cookie_value = format!("{cookie_name}-parts={num_parts}; Path=/; HttpOnly; Secure; Max-Age={cookie_duration}; ");
cookie_values.push(num_parts_cookie_value);

// Build nonce cookie value
let nonce_cookie_value = format!(
"{}-nonce={}; Path=/; HttpOnly; Secure; Max-Age={}; ",
cookie_name, &encoded_nonce, cookie_duration
);
cookie_values.push(nonce_cookie_value);

// Overwrite the old cookies because decryption will fail if older and expired cookies are
// still present.
for i in cookie_values.len()..number_current_cookies as usize {
cookie_values.push(format!(
"{}-{}=; Path=/; HttpOnly; Secure; Max-Age=0",
cookie_name, i
));
}

cookie_values
}

Expand Down
Loading