Fix directory-index links using native path.join instead of path.posix.join - #856
Open
Tech Guy (lukiod) wants to merge 1 commit into
Open
Fix directory-index links using native path.join instead of path.posix.join#856Tech Guy (lukiod) wants to merge 1 commit into
Tech Guy (lukiod) wants to merge 1 commit into
Conversation
createIndexPage() builds each entry's href with path.join(relativePath, childFile), then encodeURI()'s the result. path.join uses the host OS's native separator, so on Windows it returns e.g. "\apps\folder" instead of "/apps/folder" -- encodeURI then turns the backslashes into %5C, producing broken links like %5Capps%5Cfolder (microsoft#855, microsoft#762, microsoft#795). relativePath is always a URL path (it comes from the decoded request URL, not a filesystem path), so it should use posix separators regardless of host OS. Switched that one join to path.posix.join; the sibling absolutePath join on the next line is unchanged since that one is a real filesystem path and needs native separators. Added a regression test following the existing suite's own pattern (ContentLoader + fsReadDir/PathUtil stubbing, as in manager.test.ts).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #855 (and the same root cause as #762, #795).
The bug
createIndexPage()builds each directory-listing link with:path.joinuses the host OS's native path separator.relativePathhere is always a URL path (it comes from the decoded request URL, not a filesystem path), but on Windowspath.joinstill returns\-separated output — e.g.path.join('/apps', 'folder')→\apps\folder. That string is then passed throughencodeURI(), which percent-encodes the backslashes, producing exactly the broken links reported:%5Capps%5Cfolder.The fix
Use
path.posix.joinfor that one join — it always returns/-separated output regardless of host OS, which is what a URL path segment actually needs. The siblingabsolutePathjoin on the next line is untouched, since that one is a real filesystem path and correctly needs native separators.Verification
Exact reproduction of the reported bug, using Node's own platform-specific path modules directly (no Windows machine needed —
path.win32/path.posixare both available cross-platform for exactly this):The first line matches the issue's reported output byte-for-byte.
Added a regression test (
src/test/suite/contentLoader.test.ts) following this repo's own existing pattern exactly (ContentLoaderconstruction +fsReadDir/PathUtil.FileExistsStatstubbing, same asmanager.test.ts/preview.test.ts), asserting the generated index page contains working/apps/folder/-style links and no%5C.What I could and couldn't run in my environment:
npm run compile(tsc -p ./) succeeds clean, andeslinton both changed files is clean. I could not run the full suite (xvfb-run -a npm test, per your CI config) — no display server available in my environment and no way to install one without root. I want to be upfront about that rather than claim a green test run I didn't get, but wanted to flag that the standalone reproduction above uses the exact samepathmodule logic the fix and test exercise, so I'm confident in the fix's correctness even without the full Electron-hosted suite run.