[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.

<!-- Please review the items on the PR checklist before submitting-->
## 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

<!-- Provide a more detailed description of the PR, other things fixed,
or any additional comments/features here -->
## 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.
This commit is contained in:
Dave Rayment
2025-12-17 08:54:58 +00:00
committed by GitHub
parent 7b0b284d40
commit 17668047bf
2 changed files with 118 additions and 39 deletions

View File

@@ -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());
}

View File

@@ -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);
}
};
}