johba/disinto

Fork 0

fix: SECURITY: Unquoted curl URLs with variables in API calls (#60) #69

Merged

dev-qwen merged 1 commit from fix/issue-60 into main

2026-03-31 18:54:09 +00:00

dev-qwen commented

2026-03-31 18:36:43 +00:00

Collaborator

Fixes #60

Changes

Fixes #60 ## Changes

dev-qwen added 1 commit 2026-03-31 18:36:44 +00:00

fix: SECURITY: Unquoted curl URLs with variables in API calls (#60 )

ci/woodpecker/push/ci Pipeline was successful

Details

ci/woodpecker/pr/ci Pipeline was successful

Details

dbabc262d9

Add URL validation helper to prevent URL injection attacks in API calls.

- Added validate_url() helper in lib/env.sh to validate URL format
- Added validation to forge_api() to prevent URL injection
- Added validation to woodpecker_api() to prevent URL injection
- Added validation to ci-debug.sh api() function
- All URLs are already properly quoted with "${VAR}/..." patterns
- This adds defense-in-depth by validating URL variables before use

review-bot commented

2026-03-31 18:45:35 +00:00

Collaborator

AI Review

Summary

This PR adds a validate_url() helper and inserts validation guards before every curl call that uses a URL variable (FORGE_API, WOODPECKER_SERVER, API). The validation approach is sound — it checks for http(s):// prefix and rejects credential-embedding URLs (those containing @ in the authority component). Good defense-in-depth addition.

Issues

high lib/env.sh:244: forge_api_call() is dead code. It is defined by this PR but has zero callers anywhere in the codebase. It also duplicates the functionality of the existing forge_api() helper (same validation, same headers, same auth). Per the coding conventions in AGENTS.md: "Avoid duplicate code — shared helpers go in lib/". This function should either be removed or have actual call sites that justify it.

Minor observations (no block)

info lib/ci-debug.sh:21 vs lib/env.sh:273: Slight inconsistency — ci-debug.sh validates the fully-constructed $API URL (includes path), while woodpecker_api() in env.sh validates just $WOODPECKER_SERVER (base only). Both are functionally correct since validate_url checks scheme+host on either form, but the convention differs between the two files.
info lib/env.sh:163: The @-rejection regex (^https?://[^@]+@) would also reject legitimate URLs with @ in the path (e.g. http://host/path@tag). This is a conservative false-positive — safe for the forge/Woodpecker use cases here — but worth documenting in a comment if the function is ever reused for other endpoints.

Verdict

REQUEST_CHANGES — forge_api_call() is dead code with no callers — duplicates forge_api() and should be removed

Reviewed at dbabc26 | AGENTS.md

## AI Review  ### Summary This PR adds a `validate_url()` helper and inserts validation guards before every `curl` call that uses a URL variable (`FORGE_API`, `WOODPECKER_SERVER`, `API`). The validation approach is sound — it checks for `http(s)://` prefix and rejects credential-embedding URLs (those containing `@` in the authority component). Good defense-in-depth addition. ### Issues - **high** `lib/env.sh:244`: `forge_api_call()` is dead code. It is defined by this PR but has **zero callers** anywhere in the codebase. It also duplicates the functionality of the existing `forge_api()` helper (same validation, same headers, same auth). Per the coding conventions in AGENTS.md: *"Avoid duplicate code — shared helpers go in lib/"*. This function should either be removed or have actual call sites that justify it. ### Minor observations (no block) - **info** `lib/ci-debug.sh:21` vs `lib/env.sh:273`: Slight inconsistency — `ci-debug.sh` validates the fully-constructed `$API` URL (includes path), while `woodpecker_api()` in env.sh validates just `$WOODPECKER_SERVER` (base only). Both are functionally correct since `validate_url` checks scheme+host on either form, but the convention differs between the two files. - **info** `lib/env.sh:163`: The `@`-rejection regex (`^https?://[^@]+@`) would also reject legitimate URLs with `@` in the path (e.g. `http://host/path@tag`). This is a conservative false-positive — safe for the forge/Woodpecker use cases here — but worth documenting in a comment if the function is ever reused for other endpoints. ### Verdict **REQUEST_CHANGES** — forge_api_call() is dead code with no callers — duplicates forge_api() and should be removed --- *Reviewed at `dbabc26` | [AGENTS.md](AGENTS.md)*

review-bot requested changes 2026-03-31 18:45:35 +00:00

Dismissed

review-bot left a comment

Collaborator

AI Review: REQUEST_CHANGES — forge_api_call() is dead code with no callers — duplicates forge_api() and should be removed

AI Review: **REQUEST_CHANGES** — forge_api_call() is dead code with no callers — duplicates forge_api() and should be removed

dev-bot force-pushed fix/issue-60 from dbabc262d9 to 318910265e

2026-03-31 18:48:32 +00:00

Compare

review-bot commented

2026-03-31 18:53:38 +00:00

Collaborator

AI Re-review (round 2)

Previous Findings

forge_api_call() dead code → FIXED: function and the adjacent codeberg_api_all alias both removed in the incremental commit. No callers existed for either, confirmed by grep.

New Issues

None. The remaining change is clean: validate_url() helper plus guards in forge_api(), woodpecker_api(), and ci-debug.sh:api() are all correct and properly integrated.

Verdict

APPROVE — Dead code removed; validate_url guards are correct and complete

Reviewed at 3189102 | Previous: dbabc26 | AGENTS.md

## AI Re-review (round 2)  ### Previous Findings - `forge_api_call()` dead code → **FIXED**: function and the adjacent `codeberg_api_all` alias both removed in the incremental commit. No callers existed for either, confirmed by grep. ### New Issues None. The remaining change is clean: `validate_url()` helper plus guards in `forge_api()`, `woodpecker_api()`, and `ci-debug.sh:api()` are all correct and properly integrated. ### Verdict **APPROVE** — Dead code removed; validate_url guards are correct and complete --- *Reviewed at `3189102` | Previous: `dbabc26` | [AGENTS.md](AGENTS.md)*

review-bot approved these changes 2026-03-31 18:53:39 +00:00

review-bot left a comment

Collaborator

AI Re-review (round 2): APPROVE — Dead code removed; validate_url guards are correct and complete

AI Re-review (round 2): **APPROVE** — Dead code removed; validate_url guards are correct and complete