Skip to content

Commit

Permalink
chathistory: Fix error handling on timestamp parsing
Browse files Browse the repository at this point in the history 
1. INVALID_PARAMS was missing a 'context' token
2. INVALID_MSGREFTYPE should be returned on msgid= or unknown reference type
  • Loading branch information
@progval @spb
progval authored and spb committed Apr 15, 2024
1 parent 293a871 commit 28e8164
Show file tree
Hide file tree
Showing 4 changed files with 71 additions and 91 deletions.
21 changes: 16 additions & 5 deletions sable_ircd/src/command/client_command.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -179,9 +179,7 @@ impl Command for ClientCommand {
}

fn notify_error(&self, err: CommandError) {
if let Some(n) = self.translate_command_error(err) {
let _ = self.response_sink().numeric(n);
}
self.send_command_error(err)
}

fn response_sink(&self) -> &dyn CommandResponse {
Expand All @@ -206,8 +204,8 @@ impl Command for ClientCommand {
}

impl ClientCommand {
fn translate_command_error(&self, err: CommandError) -> Option<UntargetedNumeric> {
match err {
fn send_command_error(&self, err: CommandError) {
let numeric = match err {
CommandError::UnderlyingError(_) => {
todo!()
}
Expand Down Expand Up @@ -277,6 +275,19 @@ impl ClientCommand {
}
}
CommandError::Numeric(n) => Some(n),
CommandError::Fail {
command,
code,
context,
description,
} => {
self.response_sink
.send(message::Fail::new(command, code, &context, &description));
None
}
};
if let Some(numeric) = numeric {
self.response_sink.numeric(numeric)
}
}
}
11 changes: 11 additions & 0 deletions sable_ircd/src/command/error.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,17 @@ pub enum CommandError {
/// The command couldn't be processed successfully; the provided
/// numeric(messages::UntargetedNumeric) will be sent to the client to notify them
Numeric(messages::UntargetedNumeric),

/// The command couldn't be processed successfully; the provided parameters will
/// be turned into a [`FAIL` standard
/// reply](https://ircv3.net/specs/extensions/standard-replies) and sent to the client
/// to notify them
Fail {
command: &'static str,
code: &'static str,
context: String,
description: String,
},
}

impl From<policy::PermissionError> for CommandError {
Expand Down
126 changes: 40 additions & 86 deletions sable_ircd/src/command/handlers/chathistory.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,35 @@ use sable_network::network::update::HistoricMessageTarget;

use std::cmp::{max, min};

fn parse_msgref(subcommand: &str, target: Option<&str>, msgref: &str) -> Result<i64, CommandError> {
match msgref.split_once('=') {
Some(("timestamp", ts)) => utils::parse_timestamp(ts).ok_or_else(|| CommandError::Fail {
command: "CHATHISTORY",
code: "INVALID_PARAMS",
context: subcommand.to_string(),
description: "Invalid timestamp".to_string(),
}),
Some(("msgid", _)) => Err(CommandError::Fail {
command: "CHATHISTORY",
code: "INVALID_MSGREFTYPE",
context: match target {
Some(target) => format!("{} {}", subcommand, target),
None => subcommand.to_string(),
},
description: "msgid-based history requests are not supported yet".to_string(),
}),
_ => Err(CommandError::Fail {
command: "CHATHISTORY",
code: "INVALID_MSGREFTYPE",
context: match target {
Some(target) => format!("{} {}", subcommand, target),
None => subcommand.to_string(),
},
description: format!("{:?} is not a valid message reference", msgref),
}),
}
}

#[command_handler("CHATHISTORY")]
fn handle_chathistory(
source: UserSource,
Expand All @@ -20,19 +49,10 @@ fn handle_chathistory(

match subcommand.to_ascii_uppercase().as_str() {
"TARGETS" => {
let from_ts = utils::parse_timestamp(arg_1);
let to_ts = utils::parse_timestamp(arg_2);
let from_ts = parse_msgref(subcommand, None, arg_1)?;
let to_ts = parse_msgref(subcommand, None, arg_2)?;
let limit = arg_3.parse().ok();

if from_ts.is_none() || to_ts.is_none() {
response.send(message::Fail::new(
"CHATHISTORY",
"INVALID_PARAMS",
"",
"Invalid timestamp",
));
return Ok(());
}
if limit.is_none() {
response.send(message::Fail::new(
"CHATHISTORY",
Expand All @@ -48,27 +68,16 @@ fn handle_chathistory(
server,
&response,
source,
min(from_ts, to_ts),
max(from_ts, to_ts),
Some(min(from_ts, to_ts)),
Some(max(from_ts, to_ts)),
limit,
);
}
"LATEST" => {
let target = arg_1;
let from_ts = match arg_2 {
"*" => None,
_ => match utils::parse_timestamp(arg_2) {
Some(ts) => Some(ts),
None => {
response.send(message::Fail::new(
"CHATHISTORY",
"INVALID_PARAMS",
"",
"Invalid timestamp",
));
return Ok(());
}
},
_ => Some(parse_msgref(subcommand, Some(target), arg_2)?),
};

let limit = arg_3.parse().ok();
Expand All @@ -87,19 +96,8 @@ fn handle_chathistory(
)?;
}
"BEFORE" => {
let target = arg_1.to_string();
let end_ts = match utils::parse_timestamp(arg_2) {
Some(ts) => ts,
None => {
response.send(message::Fail::new(
"CHATHISTORY",
"INVALID_PARAMS",
"",
"Invalid timestamp",
));
return Ok(());
}
};
let target = arg_1;
let end_ts = parse_msgref(subcommand, Some(target), arg_2)?;

let limit = arg_3.parse().ok();
if limit.is_none() {
Expand All @@ -125,18 +123,7 @@ fn handle_chathistory(
}
"AFTER" => {
let target = arg_1;
let start_ts = match utils::parse_timestamp(arg_2) {
Some(ts) => ts,
None => {
response.send(message::Fail::new(
"CHATHISTORY",
"INVALID_PARAMS",
"",
"Invalid timestamp",
));
return Ok(());
}
};
let start_ts = parse_msgref(subcommand, Some(target), arg_2)?;

let limit = arg_3.parse().ok();
if limit.is_none() {
Expand All @@ -162,18 +149,7 @@ fn handle_chathistory(
}
"AROUND" => {
let target = arg_1;
let around_ts = match utils::parse_timestamp(arg_2) {
Some(ts) => ts,
None => {
response.send(message::Fail::new(
"CHATHISTORY",
"INVALID_PARAMS",
"",
"Invalid timestamp",
));
return Ok(());
}
};
let around_ts = parse_msgref(subcommand, Some(target), arg_2)?;

let limit = match arg_3.parse::<usize>().ok() {
Some(limit) => limit,
Expand Down Expand Up @@ -211,30 +187,8 @@ fn handle_chathistory(
}
"BETWEEN" => {
let target = arg_1;
let start_ts = match utils::parse_timestamp(arg_2) {
Some(ts) => ts,
None => {
response.send(message::Fail::new(
"CHATHISTORY",
"INVALID_PARAMS",
"",
"Invalid timestamp",
));
return Ok(());
}
};
let end_ts = match utils::parse_timestamp(arg_3) {
Some(ts) => ts,
None => {
response.send(message::Fail::new(
"CHATHISTORY",
"INVALID_PARAMS",
"",
"Invalid timestamp",
));
return Ok(());
}
};
let start_ts = parse_msgref(subcommand, Some(target), arg_2)?;
let end_ts = parse_msgref(subcommand, Some(target), arg_3)?;

let limit = arg_4.and_then(|arg| arg.parse().ok());
if limit.is_none() {
Expand Down
4 changes: 4 additions & 0 deletions sable_ircd/src/command/handlers/services/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,10 @@ impl<'a> Command for ServicesCommand<'a> {
tracing::warn!("Translating unknown error numeric from services response: {:?}", n);
self.notice(format_args!("Unknown error: {}", n.debug_format()));
}
CommandError::Fail { command, code, context, description } => {
tracing::warn!("Translating unknown error numeric from services response: {} {} {} :{}", command, code, context, description);
self.notice(format_args!("Unknown error: {} {} {} :{}", command, code, context, description));
}
}
}
}
Expand Down

0 comments on commit 28e8164

Please sign in to comment.