Author: Michael Poole <mdpoole@troilus.org>

Description:

Clean up key cleaning: Do not reject non-ASCII characters, and allow
':' after the first character in the key.  Add a regression test
script to go along with these changes.

git-svn-id: file:///home/klmitch/undernet-ircu/undernet-ircu-svn/ircu2/branches/u2_10_12_branch@1905 c9e4aea6-c8fd-4c43-8297-357d70d61c8c

diff --git a/ChangeLog b/ChangeLog
index ec137672d9f7482555adfc6b8acff8277f2ed2eb..d74896cc54bd8961848c904ca884e8df8621ed76 100644 (file)
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,18 @@
+2009-02-08  Michael Poole <mdpoole@troilus.org>
+
+       * include/numeric.h (ERR_INVALIDKEY): Define new numeric.
+
+       * ircd/s_err.c (ERR_INVALIDKEY): Give it a text string.
+
+       * ircd/channel.c (is_clean_key): Rename from clean_key(), and make
+       this function responsible for sending error messages to the client
+       when necessary.
+       (mode_parse_key): Update to match the new is_clean_key() behavior.
+       (mode_parse_upass): Likewise.
+       (mode_parse_apass): Likewise.
+
+       * tests/channel-keys.cmd: New file for regression testing.
+
 2009-02-08  Michael Poole <mdpoole@troilus.org>
 
        * include/gline.h (gline_forward_deactivation): Undeclare.

diff --git a/include/numeric.h b/include/numeric.h
index 48df5b2205b4bacaca03aaf4df4f79d86a24c957..bb186016d5da08d11439b0c7cd6c18d6ea51a8d7 100644 (file)
--- a/include/numeric.h
+++ b/include/numeric.h
@@ -456,6 +456,7 @@ extern const struct Numeric* get_error_numeric(int err);
        ERR_WHOSYNTAX        522        dalnet
        ERR_WHOLIMEXCEED     523        dalnet */
 #define ERR_QUARANTINED      524       /* Undernet extension -Vampire */
+#define ERR_INVALIDKEY       525        /* Undernet extension */
 
 #define ERR_NOTLOWEROPLEVEL  560       /* Undernet extension */
 #define ERR_NOTMANAGER       561       /* Undernet extension */

diff --git a/ircd/channel.c b/ircd/channel.c
index 8ed3eb9c845011f6d3585f54399e02ae6d3b874b..04d28a2953972db110cf1e24f580ca4546c9e402 100644 (file)
--- a/ircd/channel.c
+++ b/ircd/channel.c
@@ -2330,15 +2330,41 @@ mode_parse_limit(struct ParseState *state, int *flag_p)
   }
 }
 
-/** Helper function to clean key-like parameters. */
-static void
-clean_key(char *s)
+/** Helper function to validate key-like parameters.
+ *
+ * @param[in] state Parse state for feedback to user.
+ * @param[in] s Key to validate.
+ * @param[in] command String to pass for need_more_params() command.
+ * @return Zero on an invalid key, non-zero if the key was okay.
+ */
+static int
+is_clean_key(struct ParseState *state, char *s, char *command)
 {
-  int t_len = KEYLEN;
+  int ii;
 
-  while (*s > ' ' && *s != ':' && *s != ',' && t_len--)
-    s++;
-  *s = '\0';
+  if (s[0] == '\0') {
+    if (MyUser(state->sptr))
+      need_more_params(state->sptr, command);
+    return 0;
+  }
+  else if (s[0] == ':') {
+    if (MyUser(state->sptr))
+      send_reply(state->sptr, ERR_INVALIDKEY, state->chptr->chname);
+    return 0;
+  }
+  for (ii = 0; (ii <= KEYLEN) && (s[ii] != '\0'); ++ii) {
+    if ((unsigned char)s[ii] <= ' ' || s[ii] == ',') {
+      if (MyUser(state->sptr))
+        send_reply(state->sptr, ERR_INVALIDKEY, state->chptr->chname);
+      return 0;
+    }
+  }
+  if (ii > KEYLEN) {
+    if (MyUser(state->sptr))
+      send_reply(state->sptr, ERR_INVALIDKEY, state->chptr->chname);
+    return 0;
+  }
+  return 1;
 }
 
 /*
@@ -2383,14 +2409,10 @@ mode_parse_key(struct ParseState *state, int *flag_p)
     state->done |= DONE_KEY_DEL;
   }
 
-  /* clean up the key string */
-  clean_key(t_str);
-  if (!*t_str || *t_str == ':') { /* warn if empty */
-    if (MyUser(state->sptr))
-      need_more_params(state->sptr, state->dir == MODE_ADD ? "MODE +k" :
-                      "MODE -k");
+  /* If the key is invalid, tell the user and bail. */
+  if (!is_clean_key(state, t_str, state->dir == MODE_ADD ? "MODE +k" :
+                    "MODE -k"))
     return;
-  }
 
   if (!state->mbuf)
     return;
@@ -2495,14 +2517,10 @@ mode_parse_upass(struct ParseState *state, int *flag_p)
     state->done |= DONE_UPASS_DEL;
   }
 
-  /* clean up the upass string */
-  clean_key(t_str);
-  if (!*t_str || *t_str == ':') { /* warn if empty */
-    if (MyUser(state->sptr))
-      need_more_params(state->sptr, state->dir == MODE_ADD ? "MODE +U" :
-                      "MODE -U");
+  /* If the Upass is invalid, tell the user and bail. */
+  if (!is_clean_key(state, t_str, state->dir == MODE_ADD ? "MODE +U" :
+                    "MODE -U"))
     return;
-  }
 
   if (!state->mbuf)
     return;
@@ -2642,14 +2660,10 @@ mode_parse_apass(struct ParseState *state, int *flag_p)
     state->done |= DONE_APASS_DEL;
   }
 
-  /* clean up the apass string */
-  clean_key(t_str);
-  if (!*t_str || *t_str == ':') { /* warn if empty */
-    if (MyUser(state->sptr))
-      need_more_params(state->sptr, state->dir == MODE_ADD ? "MODE +A" :
-                      "MODE -A");
+  /* If the Apass is invalid, tell the user and bail. */
+  if (!is_clean_key(state, t_str, state->dir == MODE_ADD ? "MODE +A" :
+                    "MODE -A"))
     return;
-  }
 
   if (!state->mbuf)
     return;

diff --git a/ircd/s_err.c b/ircd/s_err.c
index b86224f901c28690ca3cd1d6c10434da80ddda8c..b05affba32348760cd8b7b0001e1ee8f43abdac2 100644 (file)
--- a/ircd/s_err.c
+++ b/ircd/s_err.c
@@ -1082,7 +1082,7 @@ static Numeric replyTable[] = {
 /* 524 */
   { ERR_QUARANTINED, "%s :Channel is quarantined : %s", "524" },
 /* 525 */
-  { 0 },
+  { ERR_INVALIDKEY, "%s :Key is not well-formed", "525" },
 /* 526 */
   { 0 },
 /* 527 */

diff --git a/tests/channel-keys.cmd b/tests/channel-keys.cmd
new file mode 100644 (file)
index 0000000..c17bb80
--- /dev/null
+++ b/tests/channel-keys.cmd
@@ -0,0 +1,45 @@
+define srv1 localhost:7601
+define srv1-name irc.example.net
+define srv2 localhost:7611
+define srv2-name irc-2.example.net
+define cl1-nick Op3rm4n
+define cl2-nick Monitor
+define channel #keytest
+
+# Connect a client to each server, and join them to the same channel.
+connect cl1 %cl1-nick% oper %srv1% :Some Channel Operator
+connect cl2 %cl2-nick% oper %srv2% :Snoopy
+:cl1 join %channel%
+sync cl1,cl2
+:cl2 join %channel%
+sync cl1,cl2
+
+# Set a plain and simple key initially.
+:cl1 mode %channel% +k foo
+:cl2 expect %cl1-nick% mode %channel% \\+k foo
+
+# Slighly funny quoting here: one : for test-driver.pl and one for quoting.
+# The final : makes the key invalid.
+:cl1 mode %channel% -k+k foo :::badkey
+:cl1 expect %srv1-name% 525 %channel% :Key is not well-formed
+:cl2 expect %cl1-nick% mode %channel% -k foo
+
+# Non-ASCII characters should be accepted in the key, and colons after the first character.
+:cl1 mode %channel% +k mötör:head
+:cl2 expect %cl1-nick% mode %channel% \\+k mötör:head
+
+# We need to have a key, too.
+:cl1 mode %channel% -k+k mötör:head
+:cl1 expect %srv1-name% 461 MODE \\+k :Not enough parameters
+
+# Are spaces accepted anywhere in the key?
+:cl1 mode %channel% +k :: spaced key
+:cl1 expect %srv1-name% 525 %channel% :Key is not well-formed
+
+# What about commas?
+:cl1 mode %channel% +k foo,bar
+:cl1 expect %srv1-name% 525 %channel% :Key is not well-formed
+
+# Is the key too long?
+:cl1 mode %channel% +k 123456789012345678901234567890
+:cl1 expect %srv1-name% 525 %channel% :Key is not well-formed