test(dns): expand is_idn_hostname_uts46 coverage for the mapping step, Bidi and ContextO - #2726
test(dns): expand is_idn_hostname_uts46 coverage for the mapping step, Bidi and ContextO#2726vtushar06 wants to merge 1 commit into
Conversation
…, Bidi and ContextO Signed-off-by: Tushar Verma <tusharmyself06@gmail.com>
|
|
🤖 Augment PR SummarySummary: Expands UTS #46 IDN-hostname test coverage without changing production code. Changes:
🤖 Was this summary useful? React with 👍 or 👎 |
| static constexpr char value[] = "\xd8\xa8\xdb\xb0\xd9\xa0"; | ||
| static_assert(sizeof(value) - 1 == 6, "literal length"); | ||
| const std::string_view input{value, 6}; | ||
| EXPECT_FALSE(sourcemeta::core::is_idn_hostname_uts46(input)); |
There was a problem hiding this comment.
At test/dns/idn_hostname_uts46_test.cc:373, this label contains U+06F0 (EN) and U+0660 (AN) after an RTL character, so it already fails RFC 5893 condition 4 even if Appendix A.9 is disabled. Consequently, this assertion cannot provide the claimed negative ContextO/A.9 coverage.
Severity: low
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
There was a problem hiding this comment.
1 issue found across 1 file
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="test/dns/idn_hostname_uts46_test.cc">
<violation number="1" location="test/dns/idn_hostname_uts46_test.cc:370">
P3: This test's label mixes U+06F0 (Bidi_Class EN) and U+0660 (Bidi_Class AN) after an RTL character, so it already fails RFC 5893 condition 4 (no mixing of EN and AN in an RTL label) independent of the Appendix A.9 digit-block rule. The comment claims this exercises A.9's prohibition on mixing digit blocks, but the failure here doesn't isolate that rule; use an LTR label or another approach that avoids triggering the bidi rule so the A.9 rule is the actual cause of rejection.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
| // appendix A.9 forbids the two digit blocks in one label | ||
| TEST(invalid_extended_arabic_indic_mixed_with_arabic_indic) { | ||
| static constexpr char value[] = "\xd8\xa8\xdb\xb0\xd9\xa0"; |
There was a problem hiding this comment.
P3: This test's label mixes U+06F0 (Bidi_Class EN) and U+0660 (Bidi_Class AN) after an RTL character, so it already fails RFC 5893 condition 4 (no mixing of EN and AN in an RTL label) independent of the Appendix A.9 digit-block rule. The comment claims this exercises A.9's prohibition on mixing digit blocks, but the failure here doesn't isolate that rule; use an LTR label or another approach that avoids triggering the bidi rule so the A.9 rule is the actual cause of rejection.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At test/dns/idn_hostname_uts46_test.cc, line 370:
<comment>This test's label mixes U+06F0 (Bidi_Class EN) and U+0660 (Bidi_Class AN) after an RTL character, so it already fails RFC 5893 condition 4 (no mixing of EN and AN in an RTL label) independent of the Appendix A.9 digit-block rule. The comment claims this exercises A.9's prohibition on mixing digit blocks, but the failure here doesn't isolate that rule; use an LTR label or another approach that avoids triggering the bidi rule so the A.9 rule is the actual cause of rejection.</comment>
<file context>
@@ -116,3 +118,268 @@ TEST(invalid_trailing_dot) {
+
+// appendix A.9 forbids the two digit blocks in one label
+TEST(invalid_extended_arabic_indic_mixed_with_arabic_indic) {
+ static constexpr char value[] = "\xd8\xa8\xdb\xb0\xd9\xa0";
+ static_assert(sizeof(value) - 1 == 6, "literal length");
+ const std::string_view input{value, 6};
</file context>
jviotti
left a comment
There was a problem hiding this comment.
Looks good! Keep in mind the new CLA CI new requirement, which is now a blocker for merging. Let me know when that's done!
I added 25 tests to
test/dns/idn_hostname_uts46_test.cc. Core already passes all of them, so this is coverage, not a fix.The file had 19 tests, against 98 for the strict function, and the gap was mostly in the parts of UTS #46 that only the lookup profile reaches.
What is new
ais 189 octets before mapping and 63 after, so the length limit has to be applied to the mapped form.xn--nxasmq6bonly becomes an A-label after step 1, so thexn--test has to run after mapping. There are also the cases where the mapping manufactures a violation that the input does not have --abcbecomes a leading hyphen,a..bbecomes an empty label,ab--cdbecomes--in the third and fourth positions.mappedreached through a few different routes (U+1D400, U+24D0, U+FB01),ignoredvia U+FE00,deviationvia U+03C2 kept under nontransitional processing, and the NFC normalisation on its own.IdnaTestV2.txtstates in its own header that "The CONTEXTO tests are optional for client software, and not tested here", so the suite file cannot cover this even though Core implements it. The extended Arabic-Indic digit rule and the per-label scope of A.8/A.9 are both exercised.How I checked the expected values
Every assertion is the live verdict, read off the real functions through a driver linked against
sourcemeta::core::dnsrather than written by hand. Each test asserts bothis_idn_hostname_uts46andis_idn_hostname, so the pair also documents where the two profiles part company - 9 of the 25 differ between them.Any input containing a byte a C string literal would mangle is emitted as an explicit-length
std::string_viewwith astatic_asserton the length, so a shortened literal fails to compile rather than silently testing something else.sourcemeta_core_dns_unitbuilds and runs clean: 335 passed, 0 failed. clang-format applied, and the diff is additions only.One case I deliberately left out: an A-label with an uppercase Punycode body. The strict function rejects it today, and asserting that would pin a verdict that RFC 5891 section 5.3 ("first ensuring that the A-label is entirely in lowercase"), RFC 5891 section 3.1 and RFC 3492 section 5 all point the other way. That seemed like the wrong thing to lock into a coverage PR, so I have raised it on the suite side instead.
RFC references