fix: normalise trashbin-original-location PROPFIND response (full-ci)

[phil-davis]

In certain situations the trashbin original-location in a PROPFIND response
contained a leading slash. This change ensures that a relative path (without
a leading slash) is always returned.

#39337
#41649

Fixes #39337

The test code changes related to this issue from old PR #39336 have also been reverted in this PR.
The test code checks for an exact match to the original-location specified in the test steps.
For test steps that check that an element does NOT exist in the trashbin, the test steps check the trashbin for both forms of original-location (relative and absolute path). That is a double-check to make sure that no unexpected stuff is left in the trashbin.

[phil-davis]

I have run full-ci because we want to know that all the API acceptance tests still pass.

[DeepDiver1975]

🤖 Automated review by Claude Code review agent.

Thanks for tackling this — it targets the exact inconsistency the triage flagged in #39337 (the oc:trashbin-original-location leading-slash mismatch: the deleter's entry was stored relative/no-slash via move()+pathinfo()['dirname'], while the owner's copy used dirname()/absolute, and getOriginalLocation() reflected that verbatim). Normalising the PROPFIND output to a single, stable leading-slash form is the right direction.

Verdict: changes-requested — the fix is incomplete and CI proves it.

Root-cause coverage

The change in AbstractTrashBinNode::getOriginalLocation():

-    $originalPath = $location . '/' . $originalPath;
+    $originalPath = '/' . ltrim($location, '/') . '/' . $originalPath;

correctly normalises the non-root case: regardless of whether $location arrives with a leading slash (owner copy) or without (deleter copy), the output now gets exactly one leading slash. Good.

But the normalisation only runs inside the if ($location !== '.') branch. For a file deleted from the root ($location === '.'), that branch is skipped and the method returns the bare $originalPath (e.g. textfile0.txt) with no leading slash. So the root-level case — the most common one — is still un-normalised.

CI failure (directly caused by this PR)

Two required API Smoke Test shards are failing, with 8 trashbin scenarios failing:

Failed step: But as "Alice" the file with original path "/textfile0.txt" should exist in the trashbin
File previously located at /textfile0.txt wasn't found in the trashbin of user Alice

This PR also tightens the test helper TrashbinContext::isInTrash() to an exact comparison by removing the ltrim() tolerance on both sides:

-        $originalPath = \ltrim($originalPath, '/');
-        if (... \ltrim($entry['original-location'], '/') === $originalPath) {
+        if (... $entry['original-location'] === $originalPath) {

The feature files now assert against /textfile0.txt (leading slash), but for root-deleted files the PROPFIND value is still textfile0.txt (no slash) because of the $location === '.' gap above — so the exact match fails. The test tightening and the production fix are out of sync for the root case.

Suggested fix

Apply the leading slash unconditionally (or handle the . case), e.g.:

$originalPath = '/' . \ltrim($originalPath, '/');
if ($location !== '.') {
    $originalPath = '/' . \ltrim($location, '/') . \rtrim($originalPath, '/') ... ;
}

or simply normalise the final return value once: return '/' . ltrim($originalPath, '/');. Then re-run the trashbin smoke suites until green.

Changelog

changelog/unreleased/41649 is present and well-formed (Bugfix: type, present-tense summary <80 chars, descriptive body, issue + PR URLs including the #39337 link).

Summary

• Resolves #39337's intent, but the fix misses the $location === '.' (root-deleted file) path.
• Required checks are failing (8 trashbin acceptance scenarios) as a direct result.
• Please slash the root case and get CI green; happy to re-review.

[phil-davis]

I have revised the code in this PR to use the solution discussed in #39337 (comment)

[DeepDiver1975]

🤖 Automated review by Claude Code review agent.

Re-review of the updated HEAD (9197b14 → b7558ee). The previous pass flagged that the leading-slash normalisation only ran in the non-root branch, leaving root-deleted files ($location === '.') un-normalised — and the PR's own tightened test helper would have failed for those. This revision fixes exactly that gap.

Root-case gap — fixed ✅

In apps/dav/lib/TrashBin/AbstractTrashBinNode.php, getOriginalLocation() now normalises at the single return point, after the if ($location \!== '.') block:

if ($location \!== '.') {
    $originalPath = $location . '/' . $originalPath;
}
return ltrim($originalPath, '/');

Because ltrim is applied to the final value regardless of which branch ran, the canonical form (relative, no leading slash) is now returned consistently for both root-deleted files and files deleted from a subfolder. This matches the PR description ("a relative path without a leading slash is always returned") and resolves the inconsistency triaged in #39337. The storage side (apps/files_trashbin/lib/Trashbin.php) is intentionally untouched — the fix lives entirely in the DAV presentation layer, which is the right, minimal place.

Test helper / feature files — consistent

TrashbinContext::isInTrash() now does an exact match against the relative form, with an opt-in $checkRelativeAndAbsolutePath flag that additionally accepts the absolute form. That flag is set true only for the negative checks ("should not exist") and the restore check — a sensible belt-and-braces double-check that nothing is left under either path form, while positive-existence assertions pin the exact relative canonical form. The feature files were updated to drop the leading slashes from positive-existence rows accordingly. Coherent with the source change.

Changelog

changelog/unreleased/41649 present and well-formed (Bugfix: prefix, body, cites #39337 and the PR). Changelog-lint check is green.

Checks — currently PENDING

The CI run for this HEAD (run 28015959486) is still in progress. PHP Unit (all DBs), PHP Code Style, Changelog lint, CodeQL, semantic-commit and JS unit are green. However every acceptance shard — including the decisive apiTrashbin,apiTrashbinRestore and cliTrashbin shards that must validate the previously-failing scenarios — is still pending. I can't yet confirm the 8 trashbin scenarios pass; the source+test logic looks correct, but the acceptance result is the real proof. mergeStateStatus is BLOCKED (waiting on required checks/review).

Verdict

The code change is correct and minimal, and it directly closes the root-case gap from the last review. Holding short of approve only because the trashbin acceptance shards have not yet reported — once apiTrashbin,apiTrashbinRestore goes green this is good to go.

[dj4oC]

Normalizes the trashbin original-location PROPFIND response to always be relative (strips leading /), fixing inconsistent absolute/relative values across root vs. subfolder restores.

Findings

• Security: no findings — display/consistency fix only, no access-control or path-traversal implication.
• Stability: the core one-line fix (ltrim($originalPath, '/')) is correct and covers both the root and subfolder cases. All 44 CI checks currently green.
• Defect (blocking) — corrupted test data: tests/acceptance/features/apiTrashbin/trashbinSkip.feature has an Examples table row changed from | new | to | new |lk — a stray lk appended outside the pipe delimiters. This is invalid Gherkin table syntax and looks like an accidental keystroke/paste artifact, independently spotted by two separate review passes.

      | new      |

• Performance: no findings — O(1) string operation.
• Test coverage: no PHPUnit test exercises AbstractTrashBinNode::getOriginalLocation() directly for the $location !== '.' branch where the bug lived (only Behat/acceptance coverage was added). Recommend a unit test in TrashBinFolderTest/TrashBinFileTest asserting no leading slash when trashBinManager->getLocation() returns a non-. value.

Verdict

Changes requested — fix the corrupted feature-file row (likely to break that scenario's table parsing) and add the missing unit-level regression test before merge.

🤖 Automated review by Claude Code (security · stability · performance · coverage)

---

Generated by Claude Code

[phil-davis]

Fixed the stray lk error, rebased and pushed.

[dj4oC]

Normalises the trashbin original-location PROPFIND response to always be relative (no leading slash), fixing the root-vs-subfolder inconsistency behind #39337.

Findings

• Security: no findings — display/consistency fix only, no access-control or path-traversal implication.
• Stability: the single-return-point ltrim($originalPath, '/') normalisation is correct for both root and subfolder cases; no backward-compat break.
• Corrupted Gherkin row (| new |lk) flagged in the prior review — confirmed fixed: re-fetched tests/acceptance/features/apiTrashbin/trashbinSkip.feature at commit 5a932b77 and all Examples rows are clean.
• CI — all 45 checks green, including the apiTrashbin,apiTrashbinRestore and cliTrashbin shards that previously failed on the 8 affected scenarios.
• Test coverage: e2e/acceptance coverage for this exact bug is now confirmed passing (preferred per policy over unit tests where both are feasible). No new PHPUnit unit test was added for AbstractTrashBinNode::getOriginalLocation() directly — non-blocking given the acceptance coverage, but a follow-up unit test would still be good practice.

Verdict

Approved.

🤖 Automated review by Claude Code (security · stability · performance · coverage)

---

Generated by Claude Code

[dj4oC]

• Security: no findings
• Stability: no findings — corrupted Gherkin row from prior review fixed, verified at commit 5a932b7
• Performance: no findings
• Test coverage: e2e (Behat acceptance), threshold met — 8 previously-failing trashbin scenarios now pass; PHPUnit unit test not added, non-blocking
• TODOs found: none
• Dependency touched: no
• CI status: all green
• Stale review: yes — PR fix: normalise trashbin-original-location PROPFIND response (full-ci) #41649 awaiting @jvillafanez >48h (requested 2026-06-23) — escalated

🤖 Automated review by Claude Code (security · stability · performance · coverage)

---

Generated by Claude Code