Skip to content

Commit ccbf0d2

Browse files
committed
fix(github): reject a backslash in safeEncodedUrlPathSegment
It was the only one of the three guards that encoded a backslash to %5C instead of refusing it. Encoding is safe on the wire — a raw backslash IS a path separator to the WHATWG parser for a special scheme, so https://x/a/b/..\..\etc resolves to /etc, but the encoded form does not move: new URL('https://x/a/b/..%5C..%5Cetc').pathname => /a/b/..%5C..%5Cetc So this is not a live traversal hole. It is refused anyway for the reason the module already gives for safeUrlPath: a value carrying a backslash is a Windows-shaped path the caller did not mean to address literally, and letting one through leaves a segment that reads as traversal to anything downstream that normalizes it. Neither caller — a GitHub label name, a git ref — can legitimately contain one, so the consistency costs nothing. Pinned, including an assertion that all three guards agree.
1 parent 0c5108e commit ccbf0d2

2 files changed

Lines changed: 48 additions & 1 deletion

File tree

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

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
*/
44
import { describe, expect, it } from 'vitest'
55
import {
6+
safeEncodedUrlPathSegment,
67
safeUrlPath,
78
safeUrlPathSegment,
89
strictEncodedUrlPathSegment,
@@ -457,6 +458,31 @@ describe('strict guards refuse padding on state-changing requests', () => {
457458
expect(strictUrlPathSegment(value, 'owner')).toBe(expected)
458459
})
459460

461+
it.each(['a\\b', '..\\..', 'area\\api', '\\'])(
462+
'safeEncodedUrlPathSegment rejects the backslash in %j',
463+
(value) => {
464+
expect(() => safeEncodedUrlPathSegment(value, 'name')).toThrow(
465+
/name cannot contain a backslash/
466+
)
467+
}
468+
)
469+
470+
it('rejects a backslash through the strict wrapper too', () => {
471+
expect(() => strictEncodedUrlPathSegment('a\\b', 'name')).toThrow(
472+
/name cannot contain a backslash/
473+
)
474+
})
475+
476+
it('all three helpers agree that a backslash is refused, not encoded', () => {
477+
for (const [label, fn] of [
478+
['safeUrlPathSegment', safeUrlPathSegment],
479+
['safeUrlPath', safeUrlPath],
480+
['safeEncodedUrlPathSegment', safeEncodedUrlPathSegment],
481+
] as const) {
482+
expect(() => fn('a\\b', label)).toThrow()
483+
}
484+
})
485+
460486
it('keeps a namespaced label encoded as one segment', () => {
461487
expect(strictEncodedUrlPathSegment('area/api', 'name')).toBe('area%2Fapi')
462488
})

apps/sim/tools/url-path.ts

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -347,6 +347,23 @@ export function safeUrlPath(value: string | number | bigint, paramName: string):
347347
* which is why that case is still rejected here rather than encoded, exactly as
348348
* the module note requires.
349349
*
350+
* A backslash is rejected rather than encoded, matching both sibling helpers.
351+
* Encoding it to `%5C` would in fact be safe on the wire — a raw `\` *is* a
352+
* path separator to the WHATWG parser for a special scheme, so
353+
* `https://x/a/b/..\..\etc` resolves to `/etc`, but the encoded form does not
354+
* move at all:
355+
*
356+
* ```
357+
* new URL('https://x/a/b/..%5C..%5Cetc').pathname // => '/a/b/..%5C..%5Cetc'
358+
* ```
359+
*
360+
* It is refused anyway, for the reason the module note gives for `safeUrlPath`:
361+
* a value carrying a backslash is a Windows-shaped path the caller did not mean
362+
* to address literally, and letting one through would leave a segment that
363+
* reads as traversal to any consumer downstream that normalizes it. Neither
364+
* caller — a GitHub label name, a git ref — can legitimately contain one, so
365+
* the consistency is free.
366+
*
350367
* Prefer `safeUrlPathSegment`. Reach for this helper only when the provider
351368
* documents the parameter as a single value that may itself contain `/`.
352369
*
@@ -355,7 +372,7 @@ export function safeUrlPath(value: string | number | bigint, paramName: string):
355372
* @returns The trimmed value percent-encoded as a single segment, separators
356373
* included, safe to interpolate.
357374
* @throws If the value is not a string or a usable number, is empty, is a dot
358-
* segment, or cannot be encoded.
375+
* segment, contains a backslash, or cannot be encoded.
359376
*/
360377
export function safeEncodedUrlPathSegment(
361378
value: string | number | bigint,
@@ -371,6 +388,10 @@ export function safeEncodedUrlPathSegment(
371388
throw new Error(`${paramName} cannot be "${trimmed}" (path traversal is not allowed)`)
372389
}
373390

391+
if (trimmed.includes('\\')) {
392+
throw new Error(`${paramName} cannot contain a backslash`)
393+
}
394+
374395
return encodeSegment(trimmed, paramName)
375396
}
376397

0 commit comments

Comments
 (0)