From: Alan T. DeKok <aland@freeradius.org>
Date: Mon, 9 Jun 2025 20:01:19 +0000 (-0400)
Subject: if (failed expansion) --> fail
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=fa763087d964057b491ee52ee3776879f775d2a1;p=thirdparty%2Ffreeradius-server.git

if (failed expansion) --> fail

unless it's followed by an "else" or "elsif".

With documentation updates and fixed tests
---

diff --git a/doc/antora/modules/reference/pages/unlang/if.adoc b/doc/antora/modules/reference/pages/unlang/if.adoc
index add4e13ccce..9c40c826d95 100644
--- a/doc/antora/modules/reference/pages/unlang/if.adoc
+++ b/doc/antora/modules/reference/pages/unlang/if.adoc
@@ -25,6 +25,40 @@ if (User-Name == "bob") {
 }
 ----
 
+== Expansions in an "if" condition
+
+An `if` condition may contain a xref:xlat/index.adoc[dynamic
+expansion], such as a call to the SQL module in the following example:
+
+.Example of Dynamic Expansion in Condition
+[source,unlang]
+----
+if %sql("SELECT reply_message FROM users where username = '%{User-Name}'") {
+    reject
+}
+----
+
+Unlike a simple check as with `if 1==2`, this dynamic expansion may fail.
+This failure can happen when the SQL database is unreachable, for
+example.  Different failures are possible for different functions.
+
+When a xref:xlat/index.adoc[dynamic expansion] fails, and `if`
+statement is followed by a trailing xref:unlang/else.adoc[else] or
+xref:unlang/elsif.adoc[elsif] statement, then `if` condition is treated
+as "false", and the `else` or `elsif statement is processed.
+
+Otherwise, the failed `if` statement returns the `fail`
+xref:unlang/return_codes.adoc[rcode], which is processed according to
+the rules for the current section.
+
+This distinction between "condition which evaluates to `false`" and
+"condition which fails to be processed" allow policies to distinguish
+those two cases.
+
+In previous versions of the server, a failed dynamic expansion was
+treated as a condition which returned `false`, which was not always
+optimal.
+
 == Practical Suggestions
 
 There are a number of practical suggestions which make it easier to work with conditions.
@@ -72,5 +106,5 @@ if (User-Name ==
 
 The last entry in an `if` section can also be an xref:unlang/actions.adoc[actions] subsection.
 
-// Copyright (C) 2021 Network RADIUS SAS.  Licenced under CC-by-NC 4.0.
+// Copyright (C) 2025 Network RADIUS SAS.  Licenced under CC-by-NC 4.0.
 // This documentation was developed by Network RADIUS SAS.
diff --git a/src/lib/unlang/compile.c b/src/lib/unlang/compile.c
index 780edef03df..dad3a4e95f8 100644
--- a/src/lib/unlang/compile.c
+++ b/src/lib/unlang/compile.c
@@ -3615,6 +3615,7 @@ static unlang_t *compile_if_subsection(unlang_t *parent, unlang_compile_t *unlan
 
 	unlang_group_t		*g;
 	unlang_cond_t		*gext;
+	CONF_ITEM		*ci;
 
 	xlat_exp_head_t		*head = NULL;
 	bool			is_truthy = false, value = false;
@@ -3644,7 +3645,7 @@ static unlang_t *compile_if_subsection(unlang_t *parent, unlang_compile_t *unlan
 				.allow_unresolved = false,
 				.allow_unknown = false,
 				.allow_wildcard = true,
-			},
+		},
 			.literals_safe_for = unlang_ctx->rules->literals_safe_for,
 		};
 
@@ -3708,6 +3709,25 @@ static unlang_t *compile_if_subsection(unlang_t *parent, unlang_compile_t *unlan
 	gext->is_truthy = is_truthy;
 	gext->value = value;
 
+	ci = cf_section_to_item(cs);
+	while ((ci = cf_item_next(parent->ci, ci)) != NULL) {
+		if (cf_item_is_data(ci)) continue;
+
+		break;
+	}
+
+	/*
+	 *	If there's an 'if' without an 'else', then remember that.
+	 */
+	if (ci && cf_item_is_section(ci)) {
+		char const *name;
+
+		name = cf_section_name1(cf_item_to_section(ci));
+		fr_assert(name != NULL);
+
+		gext->has_else = (strcmp(name, "else") == 0) || (strcmp(name, "elsif") == 0);
+	}
+
 	return c;
 }
 
diff --git a/src/lib/unlang/condition.c b/src/lib/unlang/condition.c
index f711a30c216..365b10abea4 100644
--- a/src/lib/unlang/condition.c
+++ b/src/lib/unlang/condition.c
@@ -40,6 +40,19 @@ static unlang_action_t unlang_if_resume(rlm_rcode_t *p_result, request_t *reques
 	fr_value_box_t			*box = fr_value_box_list_head(&state->out);
 	bool				value;
 
+	/*
+	 *	Something in the conditional evaluation failed.
+	 */
+	if (!state->success) {
+		unlang_group_t *g = unlang_generic_to_group(frame->instruction);
+		unlang_cond_t  *gext = unlang_group_to_cond(g);
+
+		REDEBUG2("... failed to evaluate condition ...");
+
+		if (!gext->has_else) RETURN_MODULE_FAIL;
+		return UNLANG_ACTION_EXECUTE_NEXT;
+	}
+
 	if (!box) {
 		value = false;
 
diff --git a/src/lib/unlang/condition_priv.h b/src/lib/unlang/condition_priv.h
index a8e8dd23d2c..414062cd736 100644
--- a/src/lib/unlang/condition_priv.h
+++ b/src/lib/unlang/condition_priv.h
@@ -33,6 +33,7 @@ typedef struct {
 	xlat_exp_head_t	*head;
 	bool		is_truthy;
 	bool		value;
+	bool		has_else;
 } unlang_cond_t;
 
 /** Cast a group structure to the cond keyword extension
diff --git a/src/lib/unlang/xlat_expr.c b/src/lib/unlang/xlat_expr.c
index c2da1069ba5..43cae3aea08 100644
--- a/src/lib/unlang/xlat_expr.c
+++ b/src/lib/unlang/xlat_expr.c
@@ -315,12 +315,16 @@ static xlat_action_t xlat_binary_op(TALLOC_CTX *ctx, fr_dcursor_t *out,
 	 *	Each argument is a FR_TYPE_GROUP, with one or more elements in a list.
 	 */
 	a = fr_value_box_list_head(in);
-	b = fr_value_box_list_next(in, a);
-
-	if (!a && !b) return XLAT_ACTION_FAIL;
+	if (!a) {
+		REDEBUG("Left argument to %s is missing", fr_tokens[op]);
+		return XLAT_ACTION_FAIL;
+	}
 
-	fr_assert(!a || (a->type == FR_TYPE_GROUP));
-	fr_assert(!b || (b->type == FR_TYPE_GROUP));
+	b = fr_value_box_list_next(in, a);
+	if (!b) {
+		REDEBUG("Right argument to %s is missing", fr_tokens[op]);
+		return XLAT_ACTION_FAIL;
+	}
 
 	fr_assert(!fr_comparison_op[op]);
 
@@ -452,9 +456,16 @@ static xlat_action_t xlat_cmp_op(TALLOC_CTX *ctx, fr_dcursor_t *out,
 	 *	Each argument is a FR_TYPE_GROUP, with one or more elements in a list.
 	 */
 	a = fr_value_box_list_head(in);
-	b = fr_value_box_list_next(in, a);
+	if (!a) {
+		REDEBUG("Left argument to %s is missing", fr_tokens[op]);
+		return XLAT_ACTION_FAIL;
+	}
 
-	if (!a || !b) return XLAT_ACTION_FAIL;
+	b = fr_value_box_list_next(in, a);
+	if (!b) {
+		REDEBUG("Right argument to %s is missing", fr_tokens[op]);
+		return XLAT_ACTION_FAIL;
+	}
 
 	fr_assert(a->type == FR_TYPE_GROUP);
 	fr_assert(b->type == FR_TYPE_GROUP);
diff --git a/src/tests/keywords/expr b/src/tests/keywords/expr
index b79581f7ad3..8943965b970 100644
--- a/src/tests/keywords/expr
+++ b/src/tests/keywords/expr
@@ -98,7 +98,10 @@ if !(((1 << 2) | 1) == 5) {	# needs extra () to resolve precedence
 
 if ("%{ test_date}" <= 0) {
 	test_fail
+} else {
+  # nothing!  Previous expansion has failed, so it MUST have an "else"
 }
+
 if (test_date <= 0) {
 	test_fail
 }
diff --git a/src/tests/keywords/unpack b/src/tests/keywords/unpack
index 07c23dbf379..49d4a193e96 100644
--- a/src/tests/keywords/unpack
+++ b/src/tests/keywords/unpack
@@ -25,7 +25,7 @@ test_octets := 0x000001020304
 result_integer := %unpack(%{test_octets}, 4, 'uint16')
 
 # Octets 4 and 5 == 0x0304 == 772
-if ~(result_integer == 772) {
+if (result_integer != 772) {
 	test_fail
 }