Skip to content

Commit 515b951

Browse files
committed
fix(github): permit a whitespace-only path component
safeUrlPath rejected a path component made only of spaces. That check had no security value and a real cost: git tracks both a file and a directory whose entire name is spaces, so a valid GitHub file could not be read, updated, or deleted. A whitespace-only segment is not a dot segment, and the parser never removes it: new URL('https://x/a/%20%20%20/b').pathname => /a/%20%20%20/b (kept) new URL('https://x/a/../b').pathname => /b (removed) Only a truly empty component (a `//`, where the caller wrote no name at all) is rejected now. Dot-segment and backslash rejection are unchanged. safeUrlPathSegment still rejects an all-whitespace value. That asymmetry is correct: it trims opaque ids first, so one made only of spaces has named nothing. The TSDoc records why the check is absent, citing the git paths and the parser behaviour, so it is not restored on aesthetic grounds.
1 parent 64273cf commit 515b951

3 files changed

Lines changed: 58 additions & 8 deletions

File tree

apps/sim/tools/github/path_safety.test.ts

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -91,14 +91,19 @@ const MULTI_SEGMENT_PARAMS = new Set(['path', 'branch', 'ref', 'base', 'head'])
9191

9292
/**
9393
* Filenames whose own leading, trailing, or interior spaces are content, not
94-
* padding. Git tracks all three verbatim, so trimming any of them would make
94+
* padding. Git tracks all of these verbatim, so trimming any of them would make
9595
* `update_file` and `delete_file` act on a different file than the caller named
9696
* — a silent wrong-target write, which is why `safeUrlPath` does not trim.
97+
*
98+
* The last entry is a directory whose entire name is spaces. It is a legal git
99+
* path and `%20%20%20` is never normalized away, so rejecting it would only
100+
* make a real file unreachable.
97101
*/
98102
const WHITESPACE_PATHS: ReadonlyArray<readonly [string, string]> = [
99103
['docs/my file .txt', 'docs/my%20file%20.txt'],
100104
['docs/ leading.md', 'docs/%20leading.md'],
101105
['docs/trailing.md ', 'docs/trailing.md%20'],
106+
['docs/ /file.txt', 'docs/%20%20%20/file.txt'],
102107
]
103108

104109
/**

apps/sim/tools/url-path.test.ts

Lines changed: 22 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -377,8 +377,28 @@ describe('whitespace handling differs by purpose', () => {
377377
expect(decodeURIComponent(url.pathname)).toBe('/repos/o/r/contents/docs/my file .txt')
378378
})
379379

380-
it('still rejects a segment that is only whitespace', () => {
381-
expect(() => safeUrlPath('docs/ /file.md', 'path')).toThrow(/whitespace-only path segment/)
380+
it.each([
381+
['docs/ /file.txt', 'docs/%20%20%20/file.txt'],
382+
[' ', '%20%20%20'],
383+
['e/ /f.txt', 'e/%20%20%20/f.txt'],
384+
['d/ ', 'd/%20%20%20'],
385+
])('permits the whitespace-only component in %j', (value, expected) => {
386+
expect(safeUrlPath(value, 'path')).toBe(expected)
387+
})
388+
389+
it('round-trips a whitespace-only component through the URL parser', () => {
390+
const url = new URL(`${ORIGIN}/repos/o/r/contents/${safeUrlPath('docs/ /file.txt', 'path')}`)
391+
392+
expect(url.pathname).toBe('/repos/o/r/contents/docs/%20%20%20/file.txt')
393+
expect(decodeURIComponent(url.pathname)).toBe('/repos/o/r/contents/docs/ /file.txt')
394+
})
395+
396+
it('rejects only a truly empty component', () => {
397+
expect(() => safeUrlPath('docs//file.txt', 'path')).toThrow(/empty path segment/)
398+
})
399+
400+
it('still rejects an opaque id that is only whitespace, since it trims first', () => {
401+
expect(() => safeUrlPathSegment(' ', 'edgeConfigId')).toThrow(/edgeConfigId is required/)
382402
})
383403

384404
it('still rejects a dot segment inside a path', () => {

apps/sim/tools/url-path.ts

Lines changed: 30 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -243,8 +243,33 @@ export function safeUrlPathSegment(value: string | number | bigint, paramName: s
243243
* 404 for a file that does not exist rather than a quiet success against the
244244
* wrong one.
245245
*
246-
* A segment that is *only* whitespace is still rejected: it names nothing the
247-
* caller could have meant, and it is indistinguishable from the `//` case above.
246+
* A segment that is only whitespace is **permitted**, for the same reason the
247+
* rest of the value is not trimmed, and the temptation to reject it on the
248+
* grounds that it "names nothing" should be resisted. It names something: git
249+
* tracks a file and a directory whose entire name is spaces, exactly as typed.
250+
*
251+
* ```
252+
* $ git ls-files | sed -n 'l' # `l` makes the line ends visible
253+
* d/ $
254+
* e/ /f.txt$
255+
* ```
256+
*
257+
* And rejecting it would buy nothing, because a whitespace-only segment is not
258+
* a dot segment and the parser never removes it — the encoded form survives
259+
* intact, where a dot segment does not:
260+
*
261+
* ```
262+
* new URL('https://x/a/%20%20%20/b').pathname // => '/a/%20%20%20/b' (kept)
263+
* new URL('https://x/a/../b').pathname // => '/b' (removed)
264+
* ```
265+
*
266+
* So the check would carry no security value and a real cost: a legitimate file
267+
* that could not be read, updated, or deleted. Only a *truly* empty component
268+
* — the `//` case above, where the caller wrote no name at all — is rejected.
269+
*
270+
* {@link safeUrlPathSegment} does still reject an all-whitespace value, and
271+
* that asymmetry is correct rather than an oversight: it trims first, so an
272+
* opaque id of only spaces really has named nothing.
248273
*
249274
* Not trimming also does not weaken the dot-segment check, which compares the
250275
* raw segment. A space-wrapped dot segment needs no rejection because encoding
@@ -271,7 +296,7 @@ export function safeUrlPathSegment(value: string | number | bigint, paramName: s
271296
* @returns The trimmed path with every segment percent-encoded and the `/`
272297
* separators preserved, safe to interpolate.
273298
* @throws If the value is not a string or a usable number, is empty, contains
274-
* an empty or whitespace-only segment, contains a dot segment, contains a
299+
* a truly empty segment (a `//`), contains a dot segment, contains a
275300
* backslash, or cannot be encoded.
276301
*/
277302
export function safeUrlPath(value: string | number | bigint, paramName: string): string {
@@ -288,8 +313,8 @@ export function safeUrlPath(value: string | number | bigint, paramName: string):
288313
return path
289314
.split('/')
290315
.map((segment) => {
291-
if (!segment.trim()) {
292-
throw new Error(`${paramName} cannot contain an empty or whitespace-only path segment`)
316+
if (!segment) {
317+
throw new Error(`${paramName} cannot contain an empty path segment`)
293318
}
294319

295320
if (segment === '.' || segment === '..') {

0 commit comments

Comments
 (0)