Deprecate Qrequest::post_ok() in favor of valid_post().

valid_post() checks for the POST method as well as the CSRF token. And add a check for the HEAD method to uses of URL capabilities for review accept and decline. It looks like some of the weird behavior I'm seeing is due to MS Azure calling HEAD methods on those URLs. GET methods are "not supposed to change state", but HEAD methods definitely should not.
ubc · Nov 18, 2020 · a0a1271 · a0a1271
1 parent f809c8a
commit a0a1271
Show file tree

Hide file tree

Showing 29 changed files with 182 additions and 137 deletions.
diff --git a/assign.php b/assign.php
@@ -124,16 +124,18 @@ function pcAssignments($qreq) {
     }
 }
 
-if (isset($Qreq->update) && $Me->allow_administer($prow) && $Qreq->post_ok()) {
-    pcAssignments($Qreq);
-} else if (isset($Qreq->update) && $Qreq->ajax) {
-    json_exit(["ok" => false, "error" => "Only administrators can assign papers."]);
+if (isset($Qreq->update) && $Qreq->valid_post()) {
+    if ($Me->allow_administer($prow)) {
+        pcAssignments($Qreq);
+    } else if ($Qreq->ajax) {
+        json_exit(["ok" => false, "error" => "Only administrators can assign papers."]);
+    }
 }
 
 
 // add review requests
 if ((isset($Qreq->requestreview) || isset($Qreq->approvereview))
-    && $Qreq->post_ok()) {
+    && $Qreq->valid_post()) {
     $result = RequestReview_API::requestreview($Me, $Qreq, $prow);
     $result = JsonResult::make($result);
     if ($result->content["ok"]) {
@@ -158,7 +160,7 @@ function pcAssignments($qreq) {
 
 // deny review request
 if ((isset($Qreq->deny) || isset($Qreq->denyreview))
-    && $Qreq->post_ok()) {
+    && $Qreq->valid_post()) {
     $result = RequestReview_API::denyreview($Me, $Qreq, $prow);
     $result = JsonResult::make($result);
     if ($result->content["ok"]) {
@@ -173,7 +175,7 @@ function pcAssignments($qreq) {
 
 // retract review request
 if (isset($Qreq->retractreview)
-    && $Qreq->post_ok()) {
+    && $Qreq->valid_post()) {
     $result = RequestReview_API::retractreview($Me, $Qreq, $prow);
     $result = JsonResult::make($result);
     if ($result->content["ok"]) {
@@ -190,9 +192,9 @@ function pcAssignments($qreq) {
     }
 }
 
-// retract review request
+// remove declined review
 if (isset($Qreq->undeclinereview)
-    && $Qreq->post_ok()) {
+    && $Qreq->valid_post()) {
     $result = RequestReview_API::undeclinereview($Me, $Qreq, $prow);
     $result = JsonResult::make($result);
     if ($result->content["ok"]) {

diff --git a/autoassign.php b/autoassign.php
@@ -13,7 +13,7 @@
 if (!isset($Qreq->q) || trim($Qreq->q) === "(All)") {
     $Qreq->q = "";
 }
-if ($Qreq->post_ok()) {
+if ($Qreq->valid_post()) {
     header("X-Accel-Buffering: no");  // NGINX: do not hold on to file
 }
 
@@ -60,7 +60,7 @@
     if ($Conf->setting("autoassign_badpairs")) {
         $Qreq->badpairs = 1;
     }
-} else if ($Me->privChair && isset($Qreq->assign) && $Qreq->post_ok()) {
+} else if ($Me->privChair && isset($Qreq->assign) && $Qreq->valid_post()) {
     $x = array();
     for ($i = 1; isset($Qreq["bpa$i"]); ++$i) {
         if ($Qreq["bpa$i"]
@@ -150,7 +150,7 @@ function sanitize_qreq_redirect($qreq) {
 if ($Qreq->saveassignment
     && $Qreq->submit
     && isset($Qreq->assignment)
-    && $Qreq->post_ok()) {
+    && $Qreq->valid_post()) {
     $assignset = new AssignmentSet($Me, true);
     $assignset->enable_papers($SSel->selection());
     $assignset->parse($Qreq->assignment);
@@ -546,7 +546,7 @@ function echo_result() {
     '<div class="papmode"><a href="', $Conf->hoturl("bulkassign"), '">Bulk update</a></div>',
     '</div><hr class="c">';
 
-if (isset($Qreq->a) && isset($Qreq->pctyp) && $Qreq->post_ok()) {
+if (isset($Qreq->a) && isset($Qreq->pctyp) && $Qreq->valid_post()) {
     if (isset($Qreq->assignment) && isset($Qreq->showassignment)) {
         $ai = new AutoassignerInterface($Me, $Qreq, $SSel);
         $ai->echo_result();

diff --git a/bulkassign.php b/bulkassign.php
@@ -12,7 +12,7 @@
 ]);
 
 $Qreq->rev_round = (string) $Conf->sanitize_round_name($Qreq->rev_round);
-if ($Qreq->post_ok()) {
+if ($Qreq->valid_post()) {
     header("X-Accel-Buffering: no");  // NGINX: do not hold on to file
 }
 
@@ -90,7 +90,7 @@ function complete_assignment($qreq, $callback) {
 
 // perform quick assignments all at once
 if (isset($Qreq->saveassignment)
-    && $Qreq->post_ok()
+    && $Qreq->valid_post()
     && isset($Qreq->file)
     && $Qreq->assignment_size_estimate < 1000
     && complete_assignment($Qreq, null)) {
@@ -112,7 +112,7 @@ function complete_assignment($qreq, $callback) {
     unset($Qreq->bulkentry);
 }
 if (isset($Qreq->upload)
-    && $Qreq->post_ok()
+    && $Qreq->valid_post()
     && ($Qreq->bulkentry || $Qreq->has_file("bulk"))) {
     flush();
     while (@ob_end_flush()) {
@@ -175,7 +175,7 @@ function complete_assignment($qreq, $callback) {
 }
 
 if (isset($Qreq->saveassignment)
-    && $Qreq->post_ok()
+    && $Qreq->valid_post()
     && isset($Qreq->file)
     && $Qreq->assignment_size_estimate >= 1000) {
     complete_assignment($Qreq, "keep_browser_alive");

diff --git a/buzzer.php b/buzzer.php
@@ -36,7 +36,7 @@ function kiosk_manager(Contact $user, Qrequest $qreq) {
         $user->conf->save_setting("__tracker_kiosk", 1, $kiosks);
     }
     // maybe sign out to kiosk
-    if ($qreq->signout_to_kiosk && $qreq->post_ok()) {
+    if ($qreq->signout_to_kiosk && $qreq->valid_post()) {
         $user = LoginHelper::logout($user, false);
         ensure_session(ENSURE_SESSION_REGENERATE_ID);
         $key = $kiosk_keys[$qreq->buzzer_showpapers ? 1 : 0];

diff --git a/checkupdates.php b/checkupdates.php
@@ -5,7 +5,10 @@
 require_once("src/initweb.php");
 header("Content-Type: " . ($Qreq->text ? "text/plain" : "application/json"));
 
-if ($Me->privChair && $Qreq->post_ok() && isset($Qreq->ignore)) {
+if ($Me->privChair
+    && $Qreq->valid_token()
+    && !$Qreq->is_head()
+    && isset($Qreq->ignore)) {
     $when = time() + 86400 * 2;
     $Conf->qe("insert into Settings (name, value) values (?, ?) on duplicate key update value=?", "ignoreupdate_" . $Qreq->ignore, $when, $when);
 }

diff --git a/index.php b/index.php
@@ -30,7 +30,7 @@ function gx_call_requests(Conf $conf, Contact $user, Qrequest $qreq, $group, Gro
             $not_allowed = true;
         }
     }
-    if ($not_allowed && $qreq->method() === "POST" && !$qreq->post_ok()) {
+    if ($not_allowed && $qreq->is_post() && !$qreq->valid_token()) {
         $conf->msg($conf->_i("badpost"), 2);
     }
     foreach ($reqgj as $gj) {

diff --git a/lib/login.php b/lib/login.php
@@ -72,7 +72,7 @@ static private function user_lookup(Conf $conf, Qrequest $qreq) {
 
     static function login_info(Conf $conf, Qrequest $qreq) {
         assert(!$conf->external_login());
-        assert($qreq->post_ok());
+        assert($qreq->valid_post());
 
         $user = self::user_lookup($conf, $qreq);
         if (is_array($user)) {
@@ -213,7 +213,7 @@ static function check_postlogin(Contact $user, Qrequest $qreq) {
 
     static function new_account_info(Conf $conf, Qrequest $qreq) {
         assert($conf->allow_user_self_register());
-        assert($qreq->post_ok());
+        assert($qreq->valid_post());
 
         $user = self::user_lookup($conf, $qreq);
         if (is_array($user)) {

diff --git a/lib/qrequest.php b/lib/qrequest.php
@@ -43,6 +43,10 @@ function is_get() {
     function is_post() {
         return $this->____method === "POST";
     }
+    /** @return bool */
+    function is_head() {
+        return $this->____method === "HEAD";
+    }
     /** @return ?string */
     function page() {
         return $this->____page;
@@ -287,13 +291,30 @@ function checked_annex($name, $class) {
     function set_annex($name, $x) {
         $this->____x[$name] = $x;
     }
-    function approve_post() {
+    /** @return void */
+    function approve_token() {
         $this->____post_ok = true;
     }
     /** @return bool */
+    function valid_token() {
+        return $this->____post_ok;
+    }
+    /** @deprecated
+     * @return bool */
     function post_ok() {
+        if ($this->____post_ok && $this->____method !== "POST") {
+            error_log("Qrequest::post_ok() on {$this->____method}");
+        }
         return $this->____post_ok;
     }
+    /** @return bool */
+    function valid_post() {
+        if ($this->____post_ok && $this->____method !== "POST") {
+            error_log("Qrequest::valid_post() on {$this->____method}");
+        }
+        return $this->____post_ok && $this->____method === "POST";
+    }
+    /** @return void */
     function set_post_empty() {
         $this->____post_empty = true;
     }
@@ -304,12 +325,11 @@ function post_empty() {
 
     function xt_allow($e) {
         if ($e === "post") {
-            return $this->method() === "POST" && $this->post_ok();
+            return $this->____method === "POST" && $this->____post_ok;
         } else if ($e === "anypost") {
-            return $this->method() === "POST";
+            return $this->____method === "POST";
         } else if ($e === "getpost") {
-            return ($this->method() === "POST" || $this->method() === "GET")
-                && $this->post_ok();
+            return in_array($this->____method, ["POST", "GET", "HEAD"]) && $this->____post_ok;
         } else if (str_starts_with($e, "req.")) {
             foreach (explode(" ", $e) as $w) {
                 if (str_starts_with($w, "req.")

diff --git a/mail.php b/mail.php
@@ -582,13 +582,14 @@ private function run() {
     && !$Qreq->psearch
     && !$Qreq->again
     && !$recip->error
-    && $Qreq->post_ok()) {
-    if ($Qreq->send && $Qreq->mailid)
+    && $Qreq->valid_post()) {
+    if ($Qreq->send && $Qreq->mailid) {
         MailSender::send2($Me, $recip, $Qreq);
-    else if ($Qreq->send)
+    } else if ($Qreq->send) {
         MailSender::send1($Me, $recip, $Qreq);
-    else if ($Qreq->check || $Qreq->group || $Qreq->ungroup)
+    } else if ($Qreq->check || $Qreq->group || $Qreq->ungroup) {
         MailSender::check($Me, $recip, $Qreq);
+    }
 }
 
 

diff --git a/manualassign.php b/manualassign.php
@@ -104,10 +104,12 @@ function saveAssignments($qreq, $reviewer) {
 }
 
 
-if ($Qreq->update && $reviewer && $Qreq->post_ok()) {
-    saveAssignments($Qreq, $reviewer);
-} else if ($Qreq->update) {
-    Conf::msg_error("You need to select a reviewer.");
+if ($Qreq->update && $Qreq->valid_post()) {
+    if ($reviewer) {
+        saveAssignments($Qreq, $reviewer);
+    } else {
+        Conf::msg_error("You need to select a reviewer.");
+    }
 }
 
 

diff --git a/mergeaccounts.php b/mergeaccounts.php
@@ -3,8 +3,9 @@
 // Copyright (c) 2006-2020 Eddie Kohler; see LICENSE.
 
 require_once("src/initweb.php");
-if (!$Me->email)
+if (!$Me->email) {
     $Me->escape();
+}
 $MergeError = "";
 
 function crpmerge($qreq, $MiniMe) {
@@ -45,7 +46,7 @@ function crpmerge($qreq, $MiniMe) {
     }
 }
 
-if (isset($Qreq->merge) && $Qreq->post_ok()) {
+if (isset($Qreq->merge) && $Qreq->valid_post()) {
     if (!$Qreq->email) {
         $MergeError = "Enter an email address to merge.";
         Ht::error_at("email");

diff --git a/offline.php b/offline.php
@@ -3,8 +3,9 @@
 // Copyright (c) 2006-2020 Eddie Kohler; see LICENSE.
 
 require_once("src/initweb.php");
-if (!$Me->email)
+if (!$Me->email) {
     $Me->escape();
+}
 $rf = $Conf->review_form();
 
 
@@ -26,7 +27,7 @@
 // upload review form action
 if (isset($Qreq->uploadForm)
     && $Qreq->has_file("uploadedFile")
-    && $Qreq->post_ok()) {
+    && $Qreq->valid_post()) {
     $tf = ReviewValues::make_text($rf, $Qreq->file_contents("uploadedFile"),
                         $Qreq->file_filename("uploadedFile"));
     while ($tf->parse_text($Qreq->override))
@@ -68,7 +69,7 @@ function setTagIndexes(Contact $user, $qreq) {
 }
 if ((isset($Qreq->setvote) || isset($Qreq->setrank))
     && $Me->is_reviewer()
-    && $Qreq->post_ok()) {
+    && $Qreq->valid_post()) {
     setTagIndexes($Me, $Qreq);
 }