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

[breaking] Technical debt #306

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
+133 −169
Draft

[breaking] Technical debt #306

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8a23439
[breaking] Technical debt
githubadministratoruk Nov 26, 2024
e0ec931
[breaking] Technical debt
githubadministratoruk Dec 1, 2024
d62affd
[breaking] Technical debt
githubadministratoruk Dec 1, 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
Next Next commit
Loading
Loading status checks…
[breaking] Technical debt
GitHub committed Nov 26, 2024

Verified

This commit was signed with the committer’s verified signature.
SimonBrandner Šimon Brandner
GPG key ID: D1D45825D60C24D2
Verified
Learn about vigilant mode
commit 8a23439dfb1b5d3eb1893bac12a9b9330cd26bda
2 changes: 1 addition & 1 deletion README.md
View file
Original file line number Diff line number Diff line change
@@ -53,7 +53,7 @@ cd freedit && cargo build -r

* generate local documentation:
```bash
cargo doc --no-deps --open
cargo doc --no-deps --document-private-items --open
```

## Development
15 changes: 1 addition & 14 deletions src/controller/admin.rs
View file
Original file line number Diff line number Diff line change
@@ -200,10 +200,6 @@ pub(crate) async fn admin_view(
let id = u8_slice_to_u32(&v);
ones.push(format!("name: {name}, id: {id}"));
}
"inns_private" => {
let id = u8_slice_to_u32(&k);
ones.push(format!("id: {id}"));
}
"user_solos" => {
let uid = u8_slice_to_u32(&k[0..4]);
let sid = u8_slice_to_u32(&k[4..8]);
@@ -264,7 +260,7 @@ pub(crate) async fn admin_view(
let one_fmt = unescape(&format!("{:?}", one)).unwrap();
ones.push(format!("{key}: {one_fmt}"));
}
"feed_errs" | "pub_keys" => {
"feed_errs" => {
let id = ivec_to_u32(&k);
let msg = String::from_utf8_lossy(&v);
ones.push(format!("{id}: {msg}"));
@@ -280,15 +276,6 @@ pub(crate) async fn admin_view(
let one_fmt = unescape(&format!("{:?}", one)).unwrap();
ones.push(format!("{key}: {one_fmt}"));
}
"home_pages" => {
let uid = u8_slice_to_u32(&k);
ones.push(format!("{uid}: {}", v[0]));
}
"lang" => {
let uid = u8_slice_to_u32(&k);
let lang = String::from_utf8_lossy(&v);
ones.push(format!("{uid}: {lang}"));
}
"tan" => {
let id = String::from_utf8_lossy(&k);
ones.push(format!("{id}: {:?}", v));
10 changes: 2 additions & 8 deletions src/controller/inn.rs
View file
Original file line number Diff line number Diff line change
@@ -305,10 +305,6 @@ pub(crate) async fn mod_inn_post(
limit_edit_seconds: input.limit_edit_seconds,
};

if InnType::from(inn.inn_type) == InnType::Private {
DB.open_tree("inns_private")?.insert(&iid_ivec, &[])?;
}

set_one(&DB, "inns", iid, &inn)?;
inn_names_tree.insert(inn_name_key, iid_ivec)?;

@@ -981,10 +977,8 @@ pub(crate) async fn inn(
if iid == 0 {
index = get_pids_all(&DB, joined_inns, &page_params, is_site_admin)?;
} else {
if DB
.open_tree("inns_private")?
.contains_key(u32_to_ivec(iid))?
{
let inn: Inn = get_one(&DB, "inns", iid)?;
if inn.is_private() {
if joined_inns.contains(&iid) || is_site_admin {
index = get_pids_by_iids(&DB, &[iid], &page_params)?;
}
35 changes: 11 additions & 24 deletions src/controller/message.rs
View file
Original file line number Diff line number Diff line change
@@ -7,7 +7,7 @@ use axum_extra::{headers::Cookie, TypedHeader};
use rinja_axum::{into_response, Template};
use serde::Deserialize;

use crate::controller::filters;
use crate::{controller::filters, set_one};
use crate::{controller::fmt::clean_html, error::AppError, DB};

use super::{
@@ -35,27 +35,18 @@ pub(crate) async fn message(
let cookie = cookie.ok_or(AppError::NonLogin)?;
let site_config = SiteConfig::get(&DB)?;
let claim = Claim::get(&DB, &cookie, &site_config).ok_or(AppError::NonLogin)?;

if DB
.open_tree("pub_keys")?
.get(u32_to_ivec(claim.uid))?
.is_none()
{
let user: User = get_one(&DB, "users", claim.uid)?;
if user.pub_key.is_none() {
return Ok(Redirect::to("/key").into_response());
}

let pub_key = DB
.open_tree("pub_keys")?
.get(u32_to_ivec(uid))?
.map(|s| String::from_utf8_lossy(&s).to_string());

let title = format!("Sending e2ee Message to {}", uid);
let user: User = get_one(&DB, "users", uid)?;
let rcpt: User = get_one(&DB, "users", uid)?;

let title = format!("Sending e2ee Message to {}", rcpt.username);
let page_message = PageMessage {
receiver_id: uid,
page_data: PageData::new(&title, &site_config, Some(claim), false),
pub_key,
pub_key: rcpt.pub_key,
receiver_name: user.username,
};

@@ -112,12 +103,8 @@ pub(crate) async fn key(
let cookie = cookie.ok_or(AppError::NonLogin)?;
let site_config = SiteConfig::get(&DB)?;
let claim = Claim::get(&DB, &cookie, &site_config).ok_or(AppError::NonLogin)?;

let pub_key = DB
.open_tree("pub_keys")?
.get(u32_to_ivec(claim.uid))?
.map(|r| String::from_utf8_lossy(&r).to_string())
.unwrap_or_default();
let user: User = get_one(&DB, "users", claim.uid)?;
let pub_key = user.pub_key.unwrap_or_default();
Comment on lines +109 to +110
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

Public key validation is handled through HTML sanitization only

Based on the code analysis, the public key is only sanitized using clean_html() before storage, without any cryptographic validation. While this prevents XSS attacks, it doesn't ensure the key meets cryptographic standards.

  • The key is stored in User.pub_key as an optional String
  • In key_post(), the key is only processed through clean_html(&input.pub_key)
  • No cryptographic validation is performed despite the codebase having crypto capabilities (as seen in src/lib.rs and src/controller/user.rs)
🔗 Analysis chain

Verify public key format validation

While the code handles the absence of a public key gracefully, there's no validation of the public key format. Consider adding validation to ensure the key meets the required format and security standards.

Let's check if there's any key validation elsewhere in the codebase:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for public key validation patterns
rg -A 5 'pub_key.*valid|validate.*pub_key|check.*pub_key'

# Search for any crypto-related validation functions
ast-grep --pattern 'fn $_(pub_key: $_) {
  $$$
}'

Length of output: 111

Script:

#!/bin/bash
# Let's search for public key related code more broadly
rg -A 5 "pub_key"

# Also search for any key validation or verification functions
rg -A 5 "verify.*key|validate.*key|check.*key"

# Look for crypto-related imports that might handle key validation
rg "use.*crypto"

Length of output: 134574

Script:

#!/bin/bash
# Let's search for key-related functionality and validation
rg -A 5 "pub_key|public.*key" src/

# Look for any crypto-related code or key handling
rg -A 5 "crypto|key.*valid|verify" src/

# Check for any key-related functions in message handling
ast-grep --pattern 'fn $_(pub_key: $_) {
  $$$
}' src/controller/message.rs

Length of output: 3835


let page_key = PageKey {
page_data: PageData::new("Generate Key Pairs", &site_config, Some(claim), false),
@@ -143,9 +130,9 @@ pub(crate) async fn key_post(
let claim = Claim::get(&DB, &cookie, &site_config).ok_or(AppError::NonLogin)?;

let pub_key = clean_html(&input.pub_key);

DB.open_tree("pub_keys")?
.insert(u32_to_ivec(claim.uid), pub_key.as_str())?;
let mut user: User = get_one(&DB, "users", claim.uid)?;
user.pub_key = Some(pub_key);
set_one(&DB, "users", claim.uid, &user)?;
Comment on lines +137 to +139
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add size limit validation for public key

The code should validate the size of the public key to prevent potential DoS attacks through extremely large keys or database storage issues.

Consider adding a size limit check:

    let mut user: User = get_one(&DB, "users", claim.uid)?;
+   if input.pub_key.len() > 2048 {  // Adjust size limit as needed
+       return Err(AppError::InvalidInput("Public key too large".into()));
+   }
    user.pub_key = Some(pub_key);
    set_one(&DB, "users", claim.uid, &user)?;

Committable suggestion skipped: line range outside the PR's diff.


Ok(Redirect::to("/key"))
}
15 changes: 6 additions & 9 deletions src/controller/meta_handler.rs
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use std::sync::LazyLock;

use super::{db_utils::u32_to_ivec, fmt::md2html, Claim, SiteConfig};
use crate::{controller::filters, error::AppError, DB};
use super::{fmt::md2html, Claim, SiteConfig, User};
use crate::{controller::filters, error::AppError, get_one, DB};
use axum::{
http::{HeaderMap, HeaderValue, Uri},
response::{IntoResponse, Redirect, Response},
@@ -71,13 +71,10 @@ pub(crate) async fn home(
let mut home_page_code = site_config.home_page;

if let Some(claim) = claim {
if let Some(home_page) = DB.open_tree("home_pages")?.get(u32_to_ivec(claim.uid))? {
if let Some(code) = home_page.first() {
home_page_code = *code;
if home_page_code == 1 {
return Ok(Redirect::to(&format!("/feed/{}", claim.uid)));
}
};
let user: User = get_one(&DB, "users", claim.uid)?;
home_page_code = user.home_page;
if home_page_code == 1 {
return Ok(Redirect::to(&format!("/feed/{}", claim.uid)));
}
}

68 changes: 34 additions & 34 deletions src/controller/mod.rs
View file
Original file line number Diff line number Diff line change
@@ -9,20 +9,14 @@
//! | default | "users_count" | N |
//! | "users" | `uid` | [`User`] |
//! | "usernames" | `username` | `uid` |
//! | "user_following" | `uid#uid` | `&[]` |
//! | "user_followers" | `uid#uid` | `&[]` |
//! | "user_stats" | `timestamp_uid_type` | N |
//! | "user_following" | `uid#uid` | |
//! | "user_followers" | `uid#uid` | |
//! | "user_stats" | `timestamp#uid#type` | N |
//! | "user_uploads" | `uid#img_id` | `image_hash.ext` |
//! | default | "imgs_count" | N |
//! | "home_pages" | `uid` | `u8` |
//! | "tan" | `ctype#id` | `&[]` |
//! | "lang" | `uid` | `lang` |
//!
//! ### notification
//! | tree | key | value |
//! |-----------------|-----------------------|-------------------|
//! | default | "notifications_count" | N |
//! | "notifications" | `uid#nid#nt_type` | `id1#id2#is_read` |
//! | "tan" | `ctype#id` | |
//! | default | "notifications_count"| N |
//! | "notifications" | `uid#nid#nt_type` | `id1#id2#is_read` |
//!
//! ### captcha
//!
@@ -36,10 +30,10 @@
//! | default | "solos_count" | N |
//! | "solos" | `sid` | [`Solo`] |
//! | "user_solos" | `uid#sid` | `solo_type` |
//! | "user_solos_like" | `uid#sid` | `&[]` |
//! | "solo_users_like" | `sid#uid` | `&[]` |
//! | "user_solos_like" | `uid#sid` | |
//! | "solo_users_like" | `sid#uid` | |
//! | "solo_timeline" | `sid` | `uid#solo_type` |
//! | "hashtags" | `hashtag#sid` | `&[]` |
//! | "hashtags" | `hashtag#sid` | |
//!
//! ### session
//! | tree | key | value |
@@ -57,39 +51,38 @@
//! | default | "inns_count" | N |
//! | "inns" | `iid` | [`Inn`] |
//! | "inn_names" | `inn_name` | `iid` |
//! | "topics" | `topic#iid` | `&[]` |
//! | "mod_inns" | `uid#iid` | `&[]` |
//! | "user_inns" | `uid#iid` | `&[]` |
//! | "inn_apply" | `iid#uid` | `&[]` |
//! | "topics" | `topic#iid` | |
//! | "mod_inns" | `uid#iid` | |
//! | "user_inns" | `uid#iid` | |
//! | "inn_apply" | `iid#uid` | |
//! | "inn_users" | `iid#uid` | `&[1/2/3/4/5/8/10]` |
//! | "inns_private" | `iid` | `&[]` |
//! | "drafts" | `uid` | [`FormPost`] |
//! | "drafts" | `uid#title` | [`FormPost`] |
//! | "inn_feeds" | `iid#feed_id` | `uid` |
//! | "inn_items" | `iid#item_id` | `&[]` |
//! | "inn_items" | `iid#item_id` | |
//!
//! ### post
//! | tree | key | value |
//! |-------------------- |---------------------|----------------------|
//! | default | "posts_count" | N |
//! | "posts" | `pid` | [`Post`] |
//! | "inn_posts" | `iid#pid` | `&[]` |
//! | "inn_posts" | `iid#pid` | |
//! | "user_posts" | `uid#pid` | `iid#inn_type` |
//! | "tags" | `tag#pid` | `&[]` |
//! | "post_upvotes" | `pid#uid` | `&[]` |
//! | "post_downvotes" | `pid#uid` | `&[]` |
//! | "tags" | `tag#pid` | |
//! | "post_upvotes" | `pid#uid` | |
//! | "post_downvotes" | `pid#uid` | |
//! | "post_timeline_idx" | `iid#pid` | `timestamp#inn_type` |
//! | "post_timeline" | `timestamp#iid#pid` | `inn_type` |
//! | "post_pageviews" | `pid` | N |
//! | "post_pins" | `iid#pid` | `&[]` |
//! | "post_pins" | `iid#pid` | |
//!
//! ### comment
//! | tree | key | value |
//! |-----------------------|----------------------|-------------|
//! | "post_comments_count" | `pid` | N |
//! | "post_comments" | `pid#cid` | [`Comment`] |
//! | "user_comments" | `uid#pid#cid` | `&[]` |
//! | "comment_upvotes" | `pid#cid#uid` | `&[]` |
//! | "comment_downvotes" | `pid#cid#uid` | `&[]` |
//! | "user_comments" | `uid#pid#cid` | |
//! | "comment_upvotes" | `pid#cid#uid` | |
//! | "comment_downvotes" | `pid#cid#uid` | |
//!
//! ### rss
//! | tree | key | value |
@@ -103,16 +96,15 @@
//! | "feed_links" | `feed_link` | `feed_id` |
//! | "item_links" | `item_link` | `item_id` |
//! | "items" | `item_id` | [`Item`] |
//! | "read" | `uid#item_id` | `&[]` |
//! | "read" | `uid#item_id` | |
//! | "star" | `uid#item_id` | `timestamp` |
//!
//! ### e2ee message
//! | tree | key | value |
//! |-----------------------|------------------|--------------------|
//! | default | "messages_count" | N |
//! | "messages" | `mid` | `#uid#uid#message` |
//! | "pub_keys" | `uid` | `pub_key` |
//! | "user_messages" | `uid#mid` | `&[]` |
//! | "messages" | `mid` | `message` |
//! | "user_messages" | `uid#uid#mid` | |

pub(super) mod db_utils;
pub(super) mod feed;
@@ -165,6 +157,9 @@ struct User {
role: u8,
url: String,
about: String,
lang: Option<String>,
home_page: u8,
pub_key: Option<String>,
Comment on lines +160 to +162
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Document and validate new User fields.

The new fields need documentation and validation:

  1. lang: Consider validating against supported languages in I18N
  2. home_page: Document valid values and add validation

Add validation and documentation:

 #[derive(Default, Encode, Decode, Serialize, Debug)]
 struct User {
     uid: u32,
     username: String,
     password_hash: String,
     recovery_hash: Option<String>,
     created_at: i64,
     role: u8,
     url: String,
     about: String,
+    /// User's preferred language code (e.g., "en", "zh_cn")
+    #[garde(custom(supported_language))]
     lang: Option<String>,
+    /// User's preferred home page view
+    /// Values:
+    /// - 0: Default view
+    /// - 1: Timeline view
+    /// - 2: Custom view
+    #[garde(range(max = 2))]
     home_page: u8,
     pub_key: Option<String>,
 }

+impl User {
+    fn supported_language(lang: &Option<String>) -> garde::Result {
+        if let Some(lang) = lang {
+            if !["en", "zh_cn", "ja", "fr"].contains(&lang.as_str()) {
+                return Err(garde::Error::new("unsupported language"));
+            }
+        }
+        Ok(())
+    }
+}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
lang: Option<String>,
home_page: u8,
pub_key: Option<String>,
#[derive(Default, Encode, Decode, Serialize, Debug)]
struct User {
uid: u32,
username: String,
password_hash: String,
recovery_hash: Option<String>,
created_at: i64,
role: u8,
url: String,
about: String,
/// User's preferred language code (e.g., "en", "zh_cn")
#[garde(custom(supported_language))]
lang: Option<String>,
/// User's preferred home page view
/// Values:
/// - 0: Default view
/// - 1: Timeline view
/// - 2: Custom view
#[garde(range(max = 2))]
home_page: u8,
pub_key: Option<String>,
}
impl User {
fn supported_language(lang: &Option<String>) -> garde::Result {
if let Some(lang) = lang {
if !["en", "zh_cn", "ja", "fr"].contains(&lang.as_str()) {
return Err(garde::Error::new("unsupported language"));
}
}
Ok(())
}
}

}

impl User {
@@ -301,6 +296,11 @@ impl Inn {
InnType::from(self.inn_type) == InnType::Hidden
|| InnType::from(self.inn_type) == InnType::PrivateHidden
}

fn is_private(&self) -> bool {
InnType::from(self.inn_type) == InnType::Private
|| InnType::from(self.inn_type) == InnType::PrivateHidden
}
}

#[derive(Encode, Decode, Serialize, PartialEq, PartialOrd, Debug, Clone)]
25 changes: 7 additions & 18 deletions src/controller/user.rs
View file
Original file line number Diff line number Diff line change
@@ -633,11 +633,6 @@ pub(crate) async fn user_setting(
}
}

let home_page = DB
.open_tree("home_pages")?
.get(u32_to_ivec(claim.uid))?
.map_or(0, |hp| hp[0]);

let has_unread = User::has_unread(&DB, claim.uid)?;
let page_user_setting = PageUserSetting {
uid: claim.uid,
@@ -646,7 +641,7 @@ pub(crate) async fn user_setting(
about: user.about,
url: user.url,
sessions,
home_page,
home_page: user.home_page,
};

Ok(into_response(&page_user_setting))
@@ -767,18 +762,16 @@ pub(crate) async fn user_setting_post(
user.username = username.to_string();
user.about = clean_html(&input.about);
user.url = clean_html(&input.url);
DB.open_tree("home_pages")?
.insert(u32_to_ivec(user.uid), &[input.home_page])?;
user.home_page = input.home_page;

let lang = match input.lang.as_str() {
match input.lang.as_str() {
"en" | "zh_cn" | "ja" | "fr" => {
claim.update_lang(&DB, &input.lang)?;
&input.lang
user.lang = Some(input.lang.clone());
}
_ => "en",
_ => {}
};

DB.open_tree("lang")?.insert(u32_to_ivec(user.uid), lang)?;
set_one(&DB, "users", claim.uid, &user)?;

let target = format!("/user/{}", claim.uid);
@@ -940,7 +933,7 @@ pub(crate) async fn signup() -> Result<impl IntoResponse, AppError> {

/// Captcha with digits
///
/// From: https://github.com/daniel-e/captcha/blob/master/examples/captcha.rs
/// From: <https://github.com/daniel-e/captcha/blob/master/examples/captcha.rs>
fn captcha_digits() -> Captcha {
let mut c = Captcha::new();
c.set_chars(&['0', '1', '2', '3', '4', '5', '6', '7', '8', '9']);
@@ -1214,18 +1207,14 @@ impl Claim {
let seconds = expire_seconds(expiry);
let now = Timestamp::now().as_second();
let session_id = generate_nanoid_ttl(seconds);
let lang = db
.open_tree("lang")?
.get(u32_to_ivec(user.uid))?
.map(|s| String::from_utf8_lossy(&s).to_string());

let claim = Claim {
uid: user.uid,
username: user.username,
role: user.role,
last_write: now,
session_id: session_id.clone(),
lang,
lang: user.lang,
};

set_one_with_key(db, "sessions", &session_id, &claim)?;