From 17668047bf20d0850fcf82f26abff6d2884a2517 Mon Sep 17 00:00:00 2001 From: Dave Rayment Date: Wed, 17 Dec 2025 08:54:58 +0000 Subject: [PATCH] [PowerRename] Fix date replacement tokens failing if followed by a capital letter (#44267) ## Summary of the Pull Request Fixes date/time-related replacement tokens being rejected if they were followed by capital letters in the user's replacement string. ## PR Checklist - [x] Closes: #44202 - [ ] **Communication:** I've discussed this with core contributors already. If the work hasn't been agreed, this work might be rejected - [x] **Tests:** Added/updated and all pass - [ ] **Localization:** All end-user-facing strings can be localized - [ ] **Dev docs:** Added/updated - [ ] **New binaries:** Added on the required places - [ ] [JSON for signing](https://github.com/microsoft/PowerToys/blob/main/.pipelines/ESRPSigning_core.json) for new binaries - [ ] [WXS for installer](https://github.com/microsoft/PowerToys/blob/main/installer/PowerToysSetup/Product.wxs) for new binaries and localization folder - [ ] [YML for CI pipeline](https://github.com/microsoft/PowerToys/blob/main/.pipelines/ci/templates/build-powertoys-steps.yml) for new test projects - [ ] [YML for signed pipeline](https://github.com/microsoft/PowerToys/blob/main/.pipelines/release.yml) - [ ] **Documentation updated:** If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/windows-uwp/tree/docs/hub/powertoys) and link it here: #xxx ## Detailed Description of the Pull Request / Additional comments With the addition of image metadata replacement options, a strict negative lookahead was added to the date replacement regular expressions to prevent conflicts. This was required because, for example, `$D` would otherwise match before `$DATE_TAKEN_YYYY`. Metadata and date-related replacements are executed separately at the moment, so this awareness of each other is required. However, the negative lookahead was far too aggressive: - It used `(?![A-Z])`, meaning any capital letter after the date token would reject the match entirely. This caused the problem referred to in the linked issue, where `$DDT` was rejected instead of matching to the `$DD` replacement followed by a verbatim `T` character. - It was applied to the majority of fields, whereas it is only actually needed where date tokens are prefixes of metadata tokens. Only `$D` and `$H` are affected. - There was no need to apply negative lookups to catch 'self-matches' like preventing `$D`, `$DD`, and `$DDD` from matching when `$DDDD` was in the replacement string. Instead, the order of processing already matches the longest token first, so this could never happen. To fix these issues, I: - Removed the majority of the negative lookaheads. - Made the remaining negative lookaheads only match actual conflicting suffixes, e.g. `$D(?!(ATE_TAKEN_|ESCRIPTION|OCUMENT_ID))` instead of `$D(?![A-Z])`. This makes mistaken rejections of user-supplied replacement strings much more unlikely. Please note: there remain inherent issues with the current token replacement approach. Tackling these will require a more extensive refactoring PR which separates replacement string tokenization from matching and replacement, and which tackles both image metadata and file date metadata in a unified manner. ## Validation Steps Performed - Corrected unit tests which classified, for example, `$YYY` as invalid, instead of identifying it as a valid `$YY` token + verbatim capital Y replacement. - Wrote new unit tests to exercise the refined negative lookaheads. - Wrote tests to confirm certain negative lookaheads were not required because the order of processing guaranteed the longest match happens before any prefix matches. - Wrote a unit test to exercise the specific issue raised in #44202. - Confirmed that all new and pre-existing PowerRename tests pass. --- src/modules/powerrename/lib/Helpers.cpp | 50 ++++---- .../powerrename/unittests/HelpersTests.cpp | 107 ++++++++++++++---- 2 files changed, 118 insertions(+), 39 deletions(-) diff --git a/src/modules/powerrename/lib/Helpers.cpp b/src/modules/powerrename/lib/Helpers.cpp index c3902c7b93..03977a3732 100644 --- a/src/modules/powerrename/lib/Helpers.cpp +++ b/src/modules/powerrename/lib/Helpers.cpp @@ -407,15 +407,18 @@ HRESULT GetDatedFileName(_Out_ PWSTR result, UINT cchMax, _In_ PCWSTR source, SY hour12 = 12; } + // Order matters. Longer patterns are processed before any prefixes. + // Years. StringCchPrintf(replaceTerm, MAX_PATH, TEXT("%s%04d"), L"$01", fileTime.wYear); res = regex_replace(res, std::wregex(L"(([^\\$]|^)(\\$\\$)*)\\$YYYY"), replaceTerm); StringCchPrintf(replaceTerm, MAX_PATH, TEXT("%s%02d"), L"$01", (fileTime.wYear % 100)); - res = regex_replace(res, std::wregex(L"(([^\\$]|^)(\\$\\$)*)\\$YY(?![A-Z])"), replaceTerm); // Negative lookahead prevents matching $YYY, $YYYY, or metadata patterns + res = regex_replace(res, std::wregex(L"(([^\\$]|^)(\\$\\$)*)\\$YY"), replaceTerm); StringCchPrintf(replaceTerm, MAX_PATH, TEXT("%s%d"), L"$01", (fileTime.wYear % 10)); - res = regex_replace(res, std::wregex(L"(([^\\$]|^)(\\$\\$)*)\\$Y(?![A-Z])"), replaceTerm); // Negative lookahead prevents matching $YY, $YYYY, or metadata patterns + res = regex_replace(res, std::wregex(L"(([^\\$]|^)(\\$\\$)*)\\$Y"), replaceTerm); + // Months. GetDateFormatEx(localeName, NULL, &fileTime, L"MMMM", formattedDate, MAX_PATH, NULL); formattedDate[0] = towupper(formattedDate[0]); StringCchPrintf(replaceTerm, MAX_PATH, TEXT("%s%s"), L"$01", formattedDate); @@ -424,14 +427,15 @@ HRESULT GetDatedFileName(_Out_ PWSTR result, UINT cchMax, _In_ PCWSTR source, SY GetDateFormatEx(localeName, NULL, &fileTime, L"MMM", formattedDate, MAX_PATH, NULL); formattedDate[0] = towupper(formattedDate[0]); StringCchPrintf(replaceTerm, MAX_PATH, TEXT("%s%s"), L"$01", formattedDate); - res = regex_replace(res, std::wregex(L"(([^\\$]|^)(\\$\\$)*)\\$MMM(?!M)"), replaceTerm); // Negative lookahead prevents matching $MMMM + res = regex_replace(res, std::wregex(L"(([^\\$]|^)(\\$\\$)*)\\$MMM"), replaceTerm); StringCchPrintf(replaceTerm, MAX_PATH, TEXT("%s%02d"), L"$01", fileTime.wMonth); - res = regex_replace(res, std::wregex(L"(([^\\$]|^)(\\$\\$)*)\\$MM(?![A-Z])"), replaceTerm); // Negative lookahead prevents matching $MMM, $MMMM, or metadata patterns + res = regex_replace(res, std::wregex(L"(([^\\$]|^)(\\$\\$)*)\\$MM"), replaceTerm); StringCchPrintf(replaceTerm, MAX_PATH, TEXT("%s%d"), L"$01", fileTime.wMonth); - res = regex_replace(res, std::wregex(L"(([^\\$]|^)(\\$\\$)*)\\$M(?![A-Z])"), replaceTerm); // Negative lookahead prevents matching $MM, $MMM, $MMMM, or metadata patterns + res = regex_replace(res, std::wregex(L"(([^\\$]|^)(\\$\\$)*)\\$M"), replaceTerm); + // Days. GetDateFormatEx(localeName, NULL, &fileTime, L"dddd", formattedDate, MAX_PATH, NULL); formattedDate[0] = towupper(formattedDate[0]); StringCchPrintf(replaceTerm, MAX_PATH, TEXT("%s%s"), L"$01", formattedDate); @@ -440,19 +444,27 @@ HRESULT GetDatedFileName(_Out_ PWSTR result, UINT cchMax, _In_ PCWSTR source, SY GetDateFormatEx(localeName, NULL, &fileTime, L"ddd", formattedDate, MAX_PATH, NULL); formattedDate[0] = towupper(formattedDate[0]); StringCchPrintf(replaceTerm, MAX_PATH, TEXT("%s%s"), L"$01", formattedDate); - res = regex_replace(res, std::wregex(L"(([^\\$]|^)(\\$\\$)*)\\$DDD(?![A-Z])"), replaceTerm); // Negative lookahead prevents matching $DDDD or metadata patterns + res = regex_replace(res, std::wregex(L"(([^\\$]|^)(\\$\\$)*)\\$DDD"), replaceTerm); StringCchPrintf(replaceTerm, MAX_PATH, TEXT("%s%02d"), L"$01", fileTime.wDay); - res = regex_replace(res, std::wregex(L"(([^\\$]|^)(\\$\\$)*)\\$DD(?![A-Z])"), replaceTerm); // Negative lookahead prevents matching $DDD, $DDDD, or metadata patterns + res = regex_replace(res, std::wregex(L"(([^\\$]|^)(\\$\\$)*)\\$DD"), replaceTerm); StringCchPrintf(replaceTerm, MAX_PATH, TEXT("%s%d"), L"$01", fileTime.wDay); - res = regex_replace(res, std::wregex(L"(([^\\$]|^)(\\$\\$)*)\\$D(?![A-Z])"), replaceTerm); // Negative lookahead prevents matching $DD, $DDD, $DDDD, or metadata patterns like $DATE_TAKEN_YYYY + // $D overlaps with metadata patterns like $DATE_TAKEN_YYYY, so we use negative + // lookahead to prevent matching those. + res = regex_replace( + res, + std::wregex(L"(([^\\$]|^)(\\$\\$)*)\\$D(?!(ATE_TAKEN_|ESCRIPTION|OCUMENT_ID))"), /* #no-spell-check-line */ + replaceTerm); + // Time. StringCchPrintf(replaceTerm, MAX_PATH, TEXT("%s%02d"), L"$01", hour12); - res = regex_replace(res, std::wregex(L"(([^\\$]|^)(\\$\\$)*)\\$HH(?![A-Z])"), replaceTerm); // Negative lookahead prevents matching $HHH or metadata patterns + res = regex_replace(res, std::wregex(L"(([^\\$]|^)(\\$\\$)*)\\$HH"), replaceTerm); StringCchPrintf(replaceTerm, MAX_PATH, TEXT("%s%d"), L"$01", hour12); - res = regex_replace(res, std::wregex(L"(([^\\$]|^)(\\$\\$)*)\\$H(?![A-Z])"), replaceTerm); // Negative lookahead prevents matching $HH or metadata patterns + // $H overlaps with metadata's $HEIGHT, so we use negative lookahead to prevent + // matching that. + res = regex_replace(res, std::wregex(L"(([^\\$]|^)(\\$\\$)*)\\$H(?!(EIGHT))"), replaceTerm); StringCchPrintf(replaceTerm, MAX_PATH, TEXT("%s%s"), L"$01", (fileTime.wHour < 12) ? L"AM" : L"PM"); res = regex_replace(res, std::wregex(L"(([^\\$]|^)(\\$\\$)*)\\$TT"), replaceTerm); @@ -461,31 +473,31 @@ HRESULT GetDatedFileName(_Out_ PWSTR result, UINT cchMax, _In_ PCWSTR source, SY res = regex_replace(res, std::wregex(L"(([^\\$]|^)(\\$\\$)*)\\$tt"), replaceTerm); StringCchPrintf(replaceTerm, MAX_PATH, TEXT("%s%02d"), L"$01", fileTime.wHour); - res = regex_replace(res, std::wregex(L"(([^\\$]|^)(\\$\\$)*)\\$hh(?!h)"), replaceTerm); // Negative lookahead prevents matching $hhh + res = regex_replace(res, std::wregex(L"(([^\\$]|^)(\\$\\$)*)\\$hh"), replaceTerm); StringCchPrintf(replaceTerm, MAX_PATH, TEXT("%s%d"), L"$01", fileTime.wHour); - res = regex_replace(res, std::wregex(L"(([^\\$]|^)(\\$\\$)*)\\$h(?!h)"), replaceTerm); // Negative lookahead prevents matching $hh + res = regex_replace(res, std::wregex(L"(([^\\$]|^)(\\$\\$)*)\\$h"), replaceTerm); StringCchPrintf(replaceTerm, MAX_PATH, TEXT("%s%02d"), L"$01", fileTime.wMinute); - res = regex_replace(res, std::wregex(L"(([^\\$]|^)(\\$\\$)*)\\$mm(?!m)"), replaceTerm); // Negative lookahead prevents matching $mmm + res = regex_replace(res, std::wregex(L"(([^\\$]|^)(\\$\\$)*)\\$mm"), replaceTerm); StringCchPrintf(replaceTerm, MAX_PATH, TEXT("%s%d"), L"$01", fileTime.wMinute); - res = regex_replace(res, std::wregex(L"(([^\\$]|^)(\\$\\$)*)\\$m(?!m)"), replaceTerm); // Negative lookahead prevents matching $mm + res = regex_replace(res, std::wregex(L"(([^\\$]|^)(\\$\\$)*)\\$m"), replaceTerm); StringCchPrintf(replaceTerm, MAX_PATH, TEXT("%s%02d"), L"$01", fileTime.wSecond); - res = regex_replace(res, std::wregex(L"(([^\\$]|^)(\\$\\$)*)\\$ss(?!s)"), replaceTerm); // Negative lookahead prevents matching $sss + res = regex_replace(res, std::wregex(L"(([^\\$]|^)(\\$\\$)*)\\$ss"), replaceTerm); StringCchPrintf(replaceTerm, MAX_PATH, TEXT("%s%d"), L"$01", fileTime.wSecond); - res = regex_replace(res, std::wregex(L"(([^\\$]|^)(\\$\\$)*)\\$s(?!s)"), replaceTerm); // Negative lookahead prevents matching $ss + res = regex_replace(res, std::wregex(L"(([^\\$]|^)(\\$\\$)*)\\$s"), replaceTerm); StringCchPrintf(replaceTerm, MAX_PATH, TEXT("%s%03d"), L"$01", fileTime.wMilliseconds); - res = regex_replace(res, std::wregex(L"(([^\\$]|^)(\\$\\$)*)\\$fff(?!f)"), replaceTerm); // Negative lookahead prevents matching $ffff + res = regex_replace(res, std::wregex(L"(([^\\$]|^)(\\$\\$)*)\\$fff"), replaceTerm); StringCchPrintf(replaceTerm, MAX_PATH, TEXT("%s%02d"), L"$01", fileTime.wMilliseconds / 10); - res = regex_replace(res, std::wregex(L"(([^\\$]|^)(\\$\\$)*)\\$ff(?!f)"), replaceTerm); // Negative lookahead prevents matching $fff + res = regex_replace(res, std::wregex(L"(([^\\$]|^)(\\$\\$)*)\\$ff"), replaceTerm); StringCchPrintf(replaceTerm, MAX_PATH, TEXT("%s%d"), L"$01", fileTime.wMilliseconds / 100); - res = regex_replace(res, std::wregex(L"(([^\\$]|^)(\\$\\$)*)\\$f(?!f)"), replaceTerm); // Negative lookahead prevents matching $ff or $fff + res = regex_replace(res, std::wregex(L"(([^\\$]|^)(\\$\\$)*)\\$f"), replaceTerm); hr = StringCchCopy(result, cchMax, res.c_str()); } diff --git a/src/modules/powerrename/unittests/HelpersTests.cpp b/src/modules/powerrename/unittests/HelpersTests.cpp index 9a6d1b2028..426bebab02 100644 --- a/src/modules/powerrename/unittests/HelpersTests.cpp +++ b/src/modules/powerrename/unittests/HelpersTests.cpp @@ -507,24 +507,26 @@ namespace HelpersTests return testTime; } - // Category 1: Tests for invalid patterns with extra characters (verify negative lookahead prevents wrong matching) + // Category 1: Tests for patterns with extra characters. Verifies negative + // lookahead doesn't cause issues with partially matched patterns and the + // ordering of pattern matches is correct, i.e. longer patterns are matched + // first. - TEST_METHOD(InvalidPattern_YYY_NotMatched) + TEST_METHOD(ValidPattern_YYY_PartiallyMatched) { - // Test $YYY (3 Y's) is not a valid pattern and should remain unchanged - // Negative lookahead in $YY(?!Y) prevents matching $YYY + // Test $YYY (3 Y's) is recognized as a valid pattern $YY plus a verbatim 'Y' SYSTEMTIME testTime = GetTestTime(); wchar_t result[MAX_PATH] = { 0 }; HRESULT hr = GetDatedFileName(result, MAX_PATH, L"file_$YYY", testTime); Assert::IsTrue(SUCCEEDED(hr)); - Assert::AreEqual(L"file_$YYY", result); // $YYY is invalid, should remain unchanged + Assert::AreEqual(L"file_24Y", result); } - TEST_METHOD(InvalidPattern_DDD_NotPartiallyMatched) + TEST_METHOD(ValidPattern_DDD_Matched) { // Test that $DDD (short weekday) is not confused with $DD (2-digit day) - // This verifies negative lookahead works correctly + // Verifies that the matching of $DDD before $DD works correctly SYSTEMTIME testTime = GetTestTime(); wchar_t result[MAX_PATH] = { 0 }; HRESULT hr = GetDatedFileName(result, MAX_PATH, L"file_$DDD", testTime); @@ -533,9 +535,10 @@ namespace HelpersTests Assert::AreEqual(L"file_Fri", result); // Should be "Fri", not "15D" } - TEST_METHOD(InvalidPattern_MMM_NotPartiallyMatched) + TEST_METHOD(ValidPattern_MMM_Matched) { // Test that $MMM (short month name) is not confused with $MM (2-digit month) + // Verifies that the matching of $MMM before $MM works correctly SYSTEMTIME testTime = GetTestTime(); wchar_t result[MAX_PATH] = { 0 }; HRESULT hr = GetDatedFileName(result, MAX_PATH, L"file_$MMM", testTime); @@ -544,15 +547,16 @@ namespace HelpersTests Assert::AreEqual(L"file_Mar", result); // Should be "Mar", not "03M" } - TEST_METHOD(InvalidPattern_HHH_NotMatched) + TEST_METHOD(ValidPattern_HHH_PartiallyMatched) { - // Test $HHH (3 H's) is not valid and negative lookahead prevents $HH from matching + // Test $HHH (3 H's) should match $HH and leave extra H unchanged + // Also confirms that $HH is matched before $H SYSTEMTIME testTime = GetTestTime(); wchar_t result[MAX_PATH] = { 0 }; HRESULT hr = GetDatedFileName(result, MAX_PATH, L"file_$HHH", testTime); Assert::IsTrue(SUCCEEDED(hr)); - Assert::AreEqual(L"file_$HHH", result); // Should remain unchanged + Assert::AreEqual(L"file_02H", result); } TEST_METHOD(SeparatedPatterns_SingleY) @@ -669,9 +673,9 @@ namespace HelpersTests Assert::AreEqual(E_INVALIDARG, hr); } - // Category 4: Tests to explicitly verify negative lookahead is working + // Category 4: Tests to explicitly verify execution order - TEST_METHOD(NegativeLookahead_YearNotMatchedInYYYY) + TEST_METHOD(ExecutionOrder_YearNotMatchedInYYYY) { // Verify $Y doesn't match when part of $YYYY SYSTEMTIME testTime = GetTestTime(); @@ -682,9 +686,9 @@ namespace HelpersTests Assert::AreEqual(L"file_2024", result); // Should be "2024", not "202Y" } - TEST_METHOD(NegativeLookahead_MonthNotMatchedInMMM) + TEST_METHOD(ExecutionOrder_MonthNotMatchedInMMM) { - // Verify $M doesn't match when part of $MMM + // Verify $M or $MM don't match when $MMM is given SYSTEMTIME testTime = GetTestTime(); wchar_t result[MAX_PATH] = { 0 }; HRESULT hr = GetDatedFileName(result, MAX_PATH, L"file_$MMM", testTime); @@ -693,9 +697,9 @@ namespace HelpersTests Assert::AreEqual(L"file_Mar", result); // Should be "Mar", not "3ar" } - TEST_METHOD(NegativeLookahead_DayNotMatchedInDDDD) + TEST_METHOD(ExecutionOrder_DayNotMatchedInDDDD) { - // Verify $D doesn't match when part of $DDDD + // Verify $D or $DD don't match when $DDDD is given SYSTEMTIME testTime = GetTestTime(); wchar_t result[MAX_PATH] = { 0 }; HRESULT hr = GetDatedFileName(result, MAX_PATH, L"file_$DDDD", testTime); @@ -704,7 +708,7 @@ namespace HelpersTests Assert::AreEqual(L"file_Friday", result); // Should be "Friday", not "15riday" } - TEST_METHOD(NegativeLookahead_HourNotMatchedInHH) + TEST_METHOD(ExecutionOrder_HourNotMatchedInHH) { // Verify $H doesn't match when part of $HH // Note: $HH is 12-hour format, so 14:00 (2 PM) displays as "02" @@ -716,9 +720,9 @@ namespace HelpersTests Assert::AreEqual(L"file_02", result); // 14:00 in 12-hour format is "02 PM" } - TEST_METHOD(NegativeLookahead_MillisecondNotMatchedInFFF) + TEST_METHOD(ExecutionOrder_MillisecondNotMatchedInFFF) { - // Verify $f doesn't match when part of $fff + // Verify $f or $ff don't match when $fff is given SYSTEMTIME testTime = GetTestTime(); wchar_t result[MAX_PATH] = { 0 }; HRESULT hr = GetDatedFileName(result, MAX_PATH, L"file_$fff", testTime); @@ -762,5 +766,68 @@ namespace HelpersTests Assert::IsTrue(SUCCEEDED(hr)); Assert::AreEqual(L"15-15-Fri-Friday", result); } + + // Category 6: Specific bug fixes and collision avoidance + + TEST_METHOD(BugFix_DDT_AllowsSuffixT) + { + // #44202 - $DDT should be allowed and matched as $DD plus verbatim 'T'. It + // was previously blocked due to the negative lookahead for any capital + // letter after $DD. + SYSTEMTIME testTime = GetTestTime(); + wchar_t result[MAX_PATH] = { 0 }; + HRESULT hr = GetDatedFileName(result, MAX_PATH, L"file_$DDT", testTime); + + Assert::IsTrue(SUCCEEDED(hr)); + Assert::AreEqual(L"file_15T", result); + } + + TEST_METHOD(RelaxedConstraint_VerbatimCapitalAfterPatterns) + { + // Verify that patterns can be followed by capital letters that are not part + // of longer patterns, e.g., $DDC should match $DD + 'C'. + SYSTEMTIME testTime = GetTestTime(); + wchar_t result[MAX_PATH] = { 0 }; + HRESULT hr = GetDatedFileName(result, MAX_PATH, L"file_$YYYYA_$MMB_$DDC", testTime); /* #no-spell-check-line */ + + Assert::IsTrue(SUCCEEDED(hr)); + Assert::AreEqual(L"file_2024A_03B_15C", result); + } + + TEST_METHOD(Collision_DateTaken_Protected) + { + // Verify that date patterns do not collide with metadata patterns like + // DATE_TAKEN_YYYY. + SYSTEMTIME testTime = GetTestTime(); + wchar_t result[MAX_PATH] = { 0 }; + HRESULT hr = GetDatedFileName(result, MAX_PATH, L"file_$DATE_TAKEN_YYYY", testTime); + + Assert::IsTrue(SUCCEEDED(hr)); + Assert::AreEqual(L"file_$DATE_TAKEN_YYYY", result); // Not replaced + } + + TEST_METHOD(Collision_Height_Protected) + { + // Verify that HEIGHT metadata pattern does not collide with date pattern $H. + SYSTEMTIME testTime = GetTestTime(); + wchar_t result[MAX_PATH] = { 0 }; + HRESULT hr = GetDatedFileName(result, MAX_PATH, L"file_$HEIGHT", testTime); + + Assert::IsTrue(SUCCEEDED(hr)); + Assert::AreEqual(L"file_$HEIGHT", result); // Not replaced + } + + TEST_METHOD(Collision_SafeSuffix_Deer) + { + // Verifies that patterns can be safely followed by certain suffix letters as + // long as they don't match a longer pattern. $DEER should be matched as + // $D + 'EER' + SYSTEMTIME testTime = GetTestTime(); + wchar_t result[MAX_PATH] = { 0 }; + HRESULT hr = GetDatedFileName(result, MAX_PATH, L"file_$DEER", testTime); + + Assert::IsTrue(SUCCEEDED(hr)); + Assert::AreEqual(L"file_15EER", result); + } }; }