From: Vsevolod Stakhov <vsevolod@rspamd.com>
Date: Mon, 12 Jan 2026 12:56:57 +0000 (+0000)
Subject: [Fix] Correct CSS duplicate property handling to use last declaration
X-Git-Tag: 4.0.0~203
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=bc2831843db487d38cedf62978f0d0b0f2054d40;p=thirdparty%2Frspamd.git

[Fix] Correct CSS duplicate property handling to use last declaration

Fix two bugs in CSS property handling that caused text to be incorrectly
marked as invisible:

1. Fixed isset() macro misuse in override_values() - was passing a bitmask
   instead of a bit index, causing the override to never find matching values

2. Changed add_rule() to call override_values() instead of merge_values()
   when duplicate properties with normal priority are encountered, ensuring
   later CSS declarations properly override earlier ones per CSS spec

This fixes an issue where HTML emails with duplicate color declarations
(e.g., "color:#FFFFFF;color:#232333") would have text incorrectly filtered
as invisible, since only the first color was being used.

Added test case for duplicate color property handling.
---

diff --git a/src/libserver/css/css_rule.cxx b/src/libserver/css/css_rule.cxx
index 4e33ac7a0a..53001e5774 100644
--- a/src/libserver/css/css_rule.cxx
+++ b/src/libserver/css/css_rule.cxx
@@ -37,7 +37,7 @@ void css_rule::override_values(const css_rule &other)
 	}
 
 	for (const auto &ov: other.values) {
-		if (isset(&bits, static_cast<int>(1 << ov.value.index()))) {
+		if (bits & (1 << ov.value.index())) {
 			/* We need to override the existing value */
 			/*
 			 * The algorithm is not very efficient,
@@ -119,8 +119,8 @@ auto css_declarations_block::add_rule(rule_shared_ptr rule) -> bool
 				ret = false;
 			}
 			else {
-				/* Merge both */
-				local_rule->merge_values(*rule);
+				/* Override with remote - in CSS, later declarations win */
+				local_rule->override_values(*rule);
 			}
 		}
 	}
@@ -526,6 +526,36 @@ TEST_SUITE("css")
 			}
 		}
 	}
+
+	TEST_CASE("duplicate color properties - last wins")
+	{
+		/* Test case: duplicate color declarations should use the last value
+		 * This is a common spam technique to hide text by declaring white color first,
+		 * then overriding with visible color. CSS spec says last declaration wins.
+		 */
+		auto *pool = rspamd_mempool_new(rspamd_mempool_suggest_size(),
+										"css", 0);
+
+		/* First color is white (#FFFFFF), second is dark (#232333) - last should win */
+		const char *css_text = "color:#FFFFFF;font-weight:400;color:#232333";
+		auto res = process_declaration_tokens(pool,
+											  get_rules_parser_functor(pool, css_text));
+
+		CHECK(res.get() != nullptr);
+		CHECK(res->has_property(css_property(css_property_type::PROPERTY_COLOR)));
+
+		auto *block = res->compile_to_block(pool);
+		CHECK(block != nullptr);
+		/* Check that fg_color is set (mask is non-zero) */
+		auto fg_mask = block->fg_color_mask;
+		CHECK(fg_mask != 0);
+		/* The color should be #232333 (dark), not #FFFFFF (white) */
+		CHECK(block->fg_color.r == 0x23);
+		CHECK(block->fg_color.g == 0x23);
+		CHECK(block->fg_color.b == 0x33);
+
+		rspamd_mempool_delete(pool);
+	}
 }
 
 }// namespace rspamd::css
\ No newline at end of file