From c5262d54f2551ddbc62d83d49bdd63cbbc82d9f4 Mon Sep 17 00:00:00 2001
From: Felipe Zimmerle <felipe@zimmerle.org>
Date: Fri, 17 Jun 2016 15:34:06 -0300
Subject: [PATCH] Fix argument uri decode order

The uri decode happens after the string is splitted, not before.
---
 src/transaction.cc                            | 20 ++------
 .../regression/variable-ARGS_GET.json         | 47 +++++++++++++++++--
 2 files changed, 49 insertions(+), 18 deletions(-)

diff --git a/src/transaction.cc b/src/transaction.cc
index 224c29fd..456dad62 100644
--- a/src/transaction.cc
+++ b/src/transaction.cc
@@ -254,6 +254,8 @@ bool Transaction::extractArguments(const std::string &orig,
             i++;
         }
 
+        key = uri_decode(key);
+        value = uri_decode(value);
         addArgument(orig, key, value);
     }
 }
@@ -391,16 +393,8 @@ int Transaction::processURI(const char *uri, const char *method,
     m_collections.store("REQUEST_URI_RAW", uri);
 
     if (pos != std::string::npos && (m_uri_decoded.length() - pos) > 2) {
-        /**
-         * FIXME:
-         *
-         * This is configurable by secrules, we should respect whatever
-         * the secrules said about it.
-         *
-         */
-        std::string sets(m_uri_decoded, pos + 1, m_uri_decoded.length() -
-            (pos + 1));
-        extractArguments("GET", sets);
+        extractArguments("GET", std::string(uri_s, pos_raw + 1,
+            uri_s.length() - (pos_raw + 1)));
     }
     return true;
 }
@@ -648,11 +642,7 @@ int Transaction::processRequestBody() {
             m_collections.storeOrUpdateFirst("REQBODY_PROCESSOR_ERROR", "0");
         }
     } else if (m_requestBodyType == WWWFormUrlEncoded) {
-        std::string content = uri_decode(m_requestBody.str());
-        if (content.empty() == false) {
-            content.pop_back();
-        }
-        extractArguments("POST", content);
+        extractArguments("POST", m_requestBody.str());
     } else {
         std::string *a = m_collections.resolveFirst(
             "REQUEST_HEADERS:Content-Type");
diff --git a/test/test-cases/regression/variable-ARGS_GET.json b/test/test-cases/regression/variable-ARGS_GET.json
index 1fc6f486..a9590d29 100644
--- a/test/test-cases/regression/variable-ARGS_GET.json
+++ b/test/test-cases/regression/variable-ARGS_GET.json
@@ -2,7 +2,7 @@
   {  
     "enabled":1,
     "version_min":300000,
-    "title":"Testing Variables :: ARGS_GET (1/2)",
+    "title":"Testing Variables :: ARGS_GET (1/3)",
     "client":{  
       "ip":"200.249.12.31",
       "port":123
@@ -40,10 +40,10 @@
       "SecRule ARGS_GET \"@contains test \" \"id:1,pass,t:trim\""
     ]
   },
-  {  
+  {
     "enabled":1,
     "version_min":300000,
-    "title":"Testing Variables :: ARGS_GET (2/2)",
+    "title":"Testing Variables :: ARGS_GET (2/3)",
     "client":{  
       "ip":"200.249.12.31",
       "port":123
@@ -80,6 +80,47 @@
       "SecDebugLogLevel 9",
       "SecRule ARGS_GET \"@contains test \" \"id:1,pass,t:trim\""
     ]
+  },
+  {
+    "enabled":1,
+    "version_min":300000,
+    "title":"Testing Variables :: ARGS_GET (3/3)",
+    "client":{  
+      "ip":"200.249.12.31",
+      "port":123
+    },
+    "server":{  
+      "ip":"200.249.12.31",
+      "port":80
+    },
+    "request":{  
+      "headers":{  
+        "Host":"localhost",
+        "User-Agent":"curl/7.38.0",
+        "Accept":"*/*"
+      },
+      "uri":"/?key=value&key=other_value%26withsomestuff=tootherstuff",
+      "method":"GET"
+    },
+    "response":{  
+      "headers":{  
+        "Date":"Mon, 13 Jul 2015 20:02:41 GMT",
+        "Last-Modified":"Sun, 26 Oct 2014 22:33:37 GMT",
+        "Content-Type":"text/html"
+      },
+      "body":[  
+        "no need."
+      ]
+    },
+    "expected":{  
+      "debug_log":"Target value: \"other_value&withsomestuff=tootherstuff\""
+    },
+    "rules":[  
+      "SecRuleEngine On",
+      "SecDebugLog \/tmp\/modsec_debug.log",
+      "SecDebugLogLevel 9",
+      "SecRule ARGS_GET \"@contains test \" \"id:1,pass,t:trim\""
+    ]
   }
 ]