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

Merged

trtmn merged 1 commit from fix/code-review-findings into main

2026-07-29 14:06:50 +00:00

trtmn commented

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

Owner

Fixes issues #1-#5 filed from a full code review (safety / functionality / speed pass):

#1 Encode owner/repo/index/username path segments via fillPathParams, closing a path-rewrite risk from unencoded traversal-style input.
#2 (high) call_forgejo_api now coerces a JSON-string body before dispatch, fixing the double-encoding 422 that broke every write through the generic escape hatch. Verified live: a string-body POST now returns HTTP 201.
#3 Added a 30s request timeout (FORGEJO_TIMEOUT_MS) plus structured handling of network errors and malformed JSON responses. Timeout errors never interpolate the token/headers (covered by a dedicated test).
#4 update_issue: milestone "" now detaches the milestone (sends milestone: 0); omitting the field still leaves it unchanged.
#5 fetchAllPages now throws on a non-array response instead of silently returning [], which previously misreported auth/error pages as "label not found".

Adds test/unit.test.js and test/client-unit.test.js (pure unit tests, no network). All 5 new unit tests pass, and all 19 existing live integration tests still pass against the real instance.

Fixes issues #1-#5 filed from a full code review (safety / functionality / speed pass): - #1 Encode owner/repo/index/username path segments via fillPathParams, closing a path-rewrite risk from unencoded traversal-style input. - #2 (high) call_forgejo_api now coerces a JSON-string body before dispatch, fixing the double-encoding 422 that broke every write through the generic escape hatch. Verified live: a string-body POST now returns HTTP 201. - #3 Added a 30s request timeout (FORGEJO_TIMEOUT_MS) plus structured handling of network errors and malformed JSON responses. Timeout errors never interpolate the token/headers (covered by a dedicated test). - #4 update_issue: milestone "" now detaches the milestone (sends milestone: 0); omitting the field still leaves it unchanged. - #5 fetchAllPages now throws on a non-array response instead of silently returning [], which previously misreported auth/error pages as "label not found". Adds test/unit.test.js and test/client-unit.test.js (pure unit tests, no network). All 5 new unit tests pass, and all 19 existing live integration tests still pass against the real instance.

trtmn added 1 commit

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

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

- Route every hand-written path in the issue tools and helpers through
  fillPathParams so owner/repo/index/username are encoded. Unencoded
  segments let a crafted value rewrite the request path via URL
  normalization (closes #1).
- Coerce a JSON-string body in call_forgejo_api before dispatch. Clients
  that serialize the body to a string previously got HTTP 422
  'cannot unmarshal string', breaking every write through the generic
  escape hatch (closes #2).
- Add a 30s request timeout (FORGEJO_TIMEOUT_MS) plus structured handling
  of network errors and malformed JSON bodies; timeout errors never
  interpolate the token or headers (closes #3).
- update_issue: milestone "" now detaches via milestone: 0, while an
  absent milestone still means unchanged (closes #4).
- fetchAllPages: throw on a non-array body instead of silently returning
  [], which misreported auth/error pages as missing labels (closes #5).

Adds test/unit.test.js and test/client-unit.test.js (no network required).
Verified: 5 unit tests pass, all 19 live integration tests pass, and a
string-body POST through the real server now returns HTTP 201.