Encode owner/repo path segments in issue tools (path-rewrite risk) #1

New issue

Closed

opened 2026-07-29 05:57:06 +00:00 by trtmn · 0 comments

trtmn commented

2026-07-29 05:57:06 +00:00

Owner

Problem

Every tool in src/tools/issues.js interpolates owner and repo straight into the request path without encoding, for example:

const res = await forgejoRequest("GET", `/repos/${owner}/${repo}/issues`, { ... });

Because forgejoRequest builds the URL with new URL(), the WHATWG URL parser normalizes .. segments before the request is sent. A crafted value therefore rewrites the target path rather than being rejected. Verified locally:

new URL("https://git.trtmn.io/api/v1" + "/repos/../../../x/issues").toString()
=> "https://git.trtmn.io/x/issues"

So the request escapes the /api/v1/repos prefix entirely and is still sent with the all-access PAT attached. Practical exploitability is limited because the fixed suffix (/issues, /comments, /times, ...) always sticks to the end of the rewritten path, but the same class of input can also cause owner/repo confusion (a value containing / silently targets a different namespace).

This is inconsistent with the generic path: fillPathParams in src/forgejo-client.js already does encodeURIComponent(String(value)) on every path parameter, and subscribe_to_issue / unsubscribe_from_issue already encode username.

Affected

src/tools/issues.js - list_issues, get_issue, create_issue, update_issue, add_issue_comment, subscribe_to_issue, unsubscribe_from_issue, start_stopwatch, stop_stopwatch, add_tracked_time. Also src/issue-helpers.js (resolveLabelIds, resolveMilestoneId build /repos/${owner}/${repo}/labels and /milestones the same way).

Suggested fix

Route every hand-written path through fillPathParams so encoding is centralized and consistent:

forgejoRequest("GET", fillPathParams("/repos/{owner}/{repo}/issues", { owner, repo }), { ... })

Alternatively add a small encodeSegments helper, but reusing fillPathParams avoids a second code path that can drift.

Severity

Medium - defense in depth. The token used here is an all-access PAT, and tool arguments come from model output, so untrusted-ish input reaching the path builder is a realistic scenario.

## Problem Every tool in `src/tools/issues.js` interpolates `owner` and `repo` straight into the request path without encoding, for example: ```js const res = await forgejoRequest("GET", `/repos/${owner}/${repo}/issues`, { ... }); ``` Because `forgejoRequest` builds the URL with `new URL()`, the WHATWG URL parser normalizes `..` segments before the request is sent. A crafted value therefore rewrites the target path rather than being rejected. Verified locally: ``` new URL("https://git.trtmn.io/api/v1" + "/repos/../../../x/issues").toString() => "https://git.trtmn.io/x/issues" ``` So the request escapes the `/api/v1/repos` prefix entirely and is still sent with the all-access PAT attached. Practical exploitability is limited because the fixed suffix (`/issues`, `/comments`, `/times`, ...) always sticks to the end of the rewritten path, but the same class of input can also cause owner/repo confusion (a value containing `/` silently targets a different namespace). This is inconsistent with the generic path: `fillPathParams` in `src/forgejo-client.js` already does `encodeURIComponent(String(value))` on every path parameter, and `subscribe_to_issue` / `unsubscribe_from_issue` already encode `username`. ## Affected `src/tools/issues.js` - list_issues, get_issue, create_issue, update_issue, add_issue_comment, subscribe_to_issue, unsubscribe_from_issue, start_stopwatch, stop_stopwatch, add_tracked_time. Also `src/issue-helpers.js` (`resolveLabelIds`, `resolveMilestoneId` build `/repos/${owner}/${repo}/labels` and `/milestones` the same way). ## Suggested fix Route every hand-written path through `fillPathParams` so encoding is centralized and consistent: ```js forgejoRequest("GET", fillPathParams("/repos/{owner}/{repo}/issues", { owner, repo }), { ... }) ``` Alternatively add a small `encodeSegments` helper, but reusing `fillPathParams` avoids a second code path that can drift. ## Severity Medium - defense in depth. The token used here is an all-access PAT, and tool arguments come from model output, so untrusted-ish input reaching the path builder is a realistic scenario.

trtmn added the

bug

security

labels

2026-07-29 06:02:01 +00:00

trtmn referenced this issue from a commit

2026-07-29 13:55:57 +00:00

Fix code-review findings: path encoding, string bodies, timeouts

trtmn referenced this issue

2026-07-29 13:56:39 +00:00

Fix code-review findings: path encoding, string bodies, timeouts #6

trtmn