Skip to content

Commit

Permalink
Fix condition recursion
Browse files Browse the repository at this point in the history
- Show error on home page to admins when recursion is detected
- Clear error when appropriate
  • Loading branch information
kohler committed Sep 28, 2024
1 parent 877101d commit acdd185
Show file tree
Hide file tree
Showing 5 changed files with 57 additions and 24 deletions.
5 changes: 5 additions & 0 deletions src/pages/p_adminhome.php
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,11 @@ static function print(Contact $user) {
}
}
}
// Condition recursion?
if ($conf->setting("__sf_condition_recursion") > 0) {
$ml[] = MessageItem::error("<0>Self-referential search in submission field conditions");
$ml[] = new MessageItem(null, "<5>Some presence conditions in submission fields appear to be circularly defined. The fields involved will never appear. You should <a href=\"" . $conf->hoturl("settings", "group=subform") . "\">update the submission form settings</a> to fix this problem.", MessageSet::INFORM);
}

if (!empty($ml)) {
$ml[] = new MessageItem(null, "", MessageSet::WARNING);
Expand Down
28 changes: 21 additions & 7 deletions src/paperoption.php
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<?php
// paperoption.php -- HotCRP helper class for paper options
// Copyright (c) 2006-2023 Eddie Kohler; see LICENSE.
// Copyright (c) 2006-2024 Eddie Kohler; see LICENSE.

class PaperOption implements JsonSerializable {
const TITLEID = -1000;
Expand Down Expand Up @@ -564,7 +564,9 @@ final function test_exists(PaperInfo $prow, $use_script_expression = false) {
} else if ($this->_exists_state !== 0) {
return $this->_exists_state > 0;
} else if (++$this->_recursion > 5) {
throw new ErrorException("Recursion in {$this->name}::test_exists [{$this->exists_if}]");
$this->mark_condition_recursion("exists_if");
$this->_exists_state = -1;
return false;
} else {
if ($use_script_expression) {
$x = !!($this->exists_script_expression($prow) ?? true);
Expand All @@ -582,6 +584,18 @@ final function exists_script_expression(PaperInfo $prow) {
return $this->exists_term()->script_expression($prow, SearchTerm::ABOUT_PAPER);
}
}
private function mark_condition_recursion($name) {
$scr = $this->conf->setting("__sf_condition_recursion") ?? 0;
if ($scr < 0) {
$this->conf->change_setting("__sf_condition_recursion", $this->id, $name);
} else if ($scr === 0) {
$this->conf->save_setting("__sf_condition_recursion", $this->id, $name);
}
if ($scr >= 0 && !defined("HOTCRP_TESTHARNESS")) {
$prop = property_exists($this, $name) ? " [{$this->$name}]" : "";
error_log("Recursion in {$this->name}::{$name}{$prop}");
}
}

/** @return ?string */
final function editable_condition() {
Expand All @@ -607,12 +621,12 @@ final function test_editable(PaperInfo $prow) {
$this->_editable_term = $s->full_term();
}
if (++$this->_recursion > 5) {
throw new ErrorException("Recursion in {$this->name}::test_editable");
} else {
$x = $this->_editable_term->test($prow, null);
--$this->_recursion;
return $x;
$this->_editable_term = new False_SearchTerm;
$this->mark_condition_recursion("editable_if");
}
$x = $this->_editable_term->test($prow, null);
--$this->_recursion;
return $x;
}

/** @return bool */
Expand Down
1 change: 1 addition & 0 deletions src/settings/s_options.php
Original file line number Diff line number Diff line change
Expand Up @@ -1051,6 +1051,7 @@ function store_value(Si $si, SettingValues $sv) {
$sv->conf->qe("insert into PaperOption (paperId, optionId, value, data, dataOverflow) values ?v", $updates);
}
}
$sv->conf->save_setting("__sf_condition_recursion", null);
$sv->mark_invalidate_caches(["autosearch" => true]);
}

Expand Down
44 changes: 27 additions & 17 deletions src/settings/s_subfieldcondition.php
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<?php
// settings/s_subfieldcondition.php -- HotCRP submission field conditions
// Copyright (c) 2006-2023 Eddie Kohler; see LICENSE.
// Copyright (c) 2006-2024 Eddie Kohler; see LICENSE.

class SubFieldCondition_SettingParser extends SettingParser {
function print(SettingValues $sv) {
Expand Down Expand Up @@ -34,47 +34,57 @@ function print(SettingValues $sv) {
}

/** @param SettingValues $sv
* @param string $siname
* @param int $ctr
* @param 'exists_if'|'editable_if' $type
* @param PaperOption $field
* @param string $q
* @param 1|2 $status */
static function validate1($sv, $siname, $field, $q, $status) {
* @param 1|2 $status
* @param PaperInfo $prow */
static private function validate1($sv, $ctr, $type, $field, $status, $prow) {
$q = $type === "exists_if" ? $field->exists_condition() : $field->editable_condition();
if ($q === null || $q === "NONE" || $q === "phase:final") {
return;
}
$siname = "sf/{$ctr}/" . ($type === "exists_if" ? "condition" : "edit_condition");

$ps = new PaperSearch($sv->conf->root_user(), $q);
foreach ($ps->message_list() as $mi) {
$sv->append_item_at($siname, $mi);
}
try {
$fake_prow = PaperInfo::make_placeholder($sv->conf, -1);
if ($ps->main_term()->script_expression($fake_prow, SearchTerm::ABOUT_PAPER) === null) {
$sv->msg_at($siname, "<0>Invalid search in field condition", $status);
$sv->inform_at($siname, "<0>Field conditions are limited to simple search keywords.");
}
} catch (ErrorException $e) {
$sv->msg_at($siname, "<0>Field condition is defined in terms of itself", 2);

$scr = $sv->conf->setting("__sf_condition_recursion");
$scrd = $sv->conf->setting_data("__sf_condition_recursion");
$sv->conf->change_setting("__sf_condition_recursion", -1);
if ($ps->main_term()->script_expression($prow, SearchTerm::ABOUT_PAPER) === null) {
$sv->msg_at($siname, "<0>Invalid search in field condition", $status);
$sv->inform_at($siname, "<0>Field conditions are limited to simple search keywords.");
}
if ($sv->conf->setting("__sf_condition_recursion") > 0
|| ($status === 1 && $scr === $field->id && $scrd === $type)) {
$sv->msg_at($siname, "<0>Self-referential search in field condition", 2);
}
$sv->conf->change_setting("__sf_condition_recursion", $scr, $scrd);
}

static function crosscheck(SettingValues $sv) {
if ($sv->has_interest("sf")) {
$opts = Options_SettingParser::configurable_options($sv->conf);
$prow = PaperInfo::make_placeholder($sv->conf, -1);
foreach ($opts as $ctrz => $f) {
$ctr = $ctrz + 1;
self::validate1($sv, "sf/{$ctr}/condition", $f, $f->exists_condition(), 1);
self::validate1($sv, "sf/{$ctr}/edit_condition", $f, $f->editable_condition(), 1);
self::validate1($sv, $ctr, "exists_if", $f, 1, $prow);
self::validate1($sv, $ctr, "editable_if", $f, 1, $prow);
}
}
}

static function validate(SettingValues $sv) {
$opts = Options_SettingParser::configurable_options($sv->conf);
$osp = $sv->cs()->callable("Options_SettingParser");
$prow = PaperInfo::make_placeholder($sv->conf, -1);
foreach ($opts as $f) {
if (($ctr = $osp->option_id_to_ctr[$f->id] ?? null) !== null) {
self::validate1($sv, "sf/{$ctr}/condition", $f, $f->exists_condition(), 2);
self::validate1($sv, "sf/{$ctr}/edit_condition", $f, $f->editable_condition(), 2);
self::validate1($sv, $ctr, "exists_if", $f, 2, $prow);
self::validate1($sv, $ctr, "editable_if", $f, 2, $prow);
}
}
}
Expand Down
3 changes: 3 additions & 0 deletions test/t_settings.php
Original file line number Diff line number Diff line change
Expand Up @@ -1868,14 +1868,17 @@ function test_sf_condition_recursion_live() {
xassert_eqq($j[3]->id, 1);
xassert_eqq($j[3]->name, "Brownies");
xassert_eqq($j[3]->exists_if ?? null, null);
xassert_eqq($this->conf->setting("__sf_condition_recursion"), null);

$j[3]->exists_if = "#foobar && !has:brownies";
$this->conf->save_refresh_setting("options", $this->conf->setting("options") + 1, json_encode_db($j));

$pa = PaperInfo::make_new($this->u_chair, null);
$pa->set_prop("paperTags", " foobar#0");
xassert($this->u_chair->can_view_option($pa, $this->conf->option_by_id(1)));
xassert_eqq($this->conf->setting("__sf_condition_recursion"), 1);

$this->conf->save_refresh_setting("options", $this->conf->setting("options") + 1, $old_options);
$this->conf->save_setting("__sf_condition_recursion", null);
}
}

0 comments on commit acdd185

Please sign in to comment.