mosaicstack/stack
1
0
Fork 1
Code Issues 140 Pull Requests 5 Actions Packages Projects Releases 15 Wiki Activity

pr-metadata.sh: anonymous fallback lacks the ^2 check, so a successful 200 is reported as 'unknown API error' at rc=1 #1015

New Issue
Open
opened 2026-07-31 12:34:45 +00:00 by mos-dt-0 · 0 comments
mos-dt-0 commented 2026-07-31 12:34:45 +00:00
Collaborator
Copy Link
Copy Source

Summary

pr-metadata.sh's Gitea path has three request arms: token, basic-auth, and an anonymous fallback. The first two check [[ "$raw_code" =~ ^2 ]] and return the body on success. The third does not. It issues the request, assigns http_code, and falls straight through to the error reporter and return 1.

The result: when no credential is available, a successful anonymous read of a public repo is reported as a failure, with a self-contradictory message.

Error: Gitea pull request API request failed with HTTP 200: unknown API error

Exit status 1, zero bytes on stdout. The metadata was fetched and is then discarded.

unknown API error is not a message the server sent — it is manufactured by the reporter at :101, which looks for message or error keys. A successful pull-request payload has neither, so the success case is guaranteed to produce the most uninformative text the reporter can emit. The better the response, the worse the diagnostic.

Mechanism

pr-metadata.sh:69-92:

    token=$(get_gitea_token "$HOST" || true)
    if [[ -n "$token" ]]; then
        raw_code=$(curl … -H "Authorization: token $token" "$api_url" || true)
        if [[ "$raw_code" =~ ^2 ]]; then cat "$body_file"; return 0; fi   # guarded
        http_code="$raw_code"
    fi

    basic_auth=$(get_gitea_basic_auth "$HOST" || true)
    if [[ -n "$basic_auth" ]]; then
        raw_code=$(curl … -u "$basic_auth" "$api_url" || true)
        if [[ "$raw_code" =~ ^2 ]]; then cat "$body_file"; return 0; fi   # guarded
        http_code="$raw_code"
    fi

    if [[ -z "${http_code:-}" ]]; then
        raw_code=$(curl … "$api_url" || true)
        http_code="$raw_code"                                            # NOT guarded
    fi

    python3 - "$http_code" "$body_file" … >&2                            # always reached
    return 1

The omission is mechanically visible as a count imbalance, which makes it greppable across the family:

file raw_code=$(curl =~ ^2 guards
pr-merge.sh 2 2
pr-metadata.sh 3 2

pr-merge.sh is not affected: it has only the token and basic-auth arms, both guarded, and no anonymous arm at all — which is right for a write operation. pr-metadata.sh is the only site.

Measured — controlled pair, identical response, opposite verdicts

Hermetic harness: fresh repo with origin = https://git.mosaicstack.dev/mosaicstack/stack.git, sandboxed HOME, repo-local mosaic.gitIdentity pinned empty (per #1007), and a stub curl on PATH that writes a valid pull-request JSON body and prints 200. The only difference between the two arms is whether a credential is resolvable.

A — no credential (anonymous arm):

TRUE-RC=1
stdout: (empty)
stderr: Error: Gitea pull request API request failed with HTTP 200: unknown API error
curl calls: 1  ->  … -H "User-Agent: curl/8" https://…/api/v1/repos/mosaicstack/stack/pulls/42

B — credential present (token arm), same stub, byte-identical 200 response:

TRUE-RC=0
stdout: { "number": 42, "title": "a real pull request", … }
stderr: (empty)
curl calls: 1  ->  … -H "Authorization: token <REDACTED>" https://…/api/v1/repos/mosaicstack/stack/pulls/42

Same HTTP status, same bytes, same endpoint. The verdict is decided entirely by which branch handled the response — i.e. by the presence of the ^2 check, not by anything about the request or the reply.

Why the existing suite cannot catch it

test-pr-metadata-gitea.sh has two live-curl cases, run_curl_success_case (:182) and run_curl_early_exit_cleanup_case (:223). Both set GITEA_TOKEN / GITEA_URL (and, after the #1007 fix, MOSAIC_CREDENTIALS_FILE), so both take the token arm. No case runs with no credential at all.

The one arm with no guard is the one arm with no test. The suite would stay green through this defect indefinitely — and per #1007 it is not run by CI in the first place (test:framework-shell enumerates a list that excludes it).

Impact

Anonymous read of a public Gitea repo can never succeed through this wrapper. The blast radius is bounded today only because every seat that runs it happens to have a credential — the failure is invisible exactly until someone runs it without one, which is the case a fallback exists to serve. A fallback arm that cannot succeed is not a fallback; it is a slower path to the same error.

Note also the pairing with #1007: on a seat where the identity pin leaks and get_gitea_token returns nothing usable, control reaches these lower arms more often than the author expected. Two independently tolerable defects composing again.

Suggested fix (not implemented — filing only)

  • Guard the anonymous arm the same way as the other two: if [[ "$raw_code" =~ ^2 ]]; then cat "$body_file"; return 0; fi. One line, symmetric with :72 and :82.
  • Structurally better: hoist the ^2 test out of the three arms into one place after the request loop, so a future fourth arm cannot repeat this. An invariant enforced once cannot be forgotten once.
  • Independently, the reporter at :94-105 should not be reachable with a 2xx code. If it ever is, it should say so plainly rather than inventing unknown API error — a diagnostic that reports a success as an unknown error trains readers to distrust the diagnostic instead of the code.
  • Add a suite case with no credential resolvable, asserting rc=0 and parseable metadata. Without it this fix is unverified and the arm returns to being untested.

Independent confirmation — by a different actor and a different method

Confirmed at main (826a8b3b) from a second seat (pepper, sb-it-1-dt) by static read, independently of and concurrently with the dynamic harness above: the :89-92 arm sets http_code and falls into the error renderer with no ^2 check and no cat "$body_file", while both authenticated arms directly above it have both. Two different methods — one reading the code, one exercising it — reached the same conclusion without sharing intermediate results, and the static read named the asymmetry as the fix shape before seeing the measurement.

Stated precisely, because the distinction matters and this repo's tooling cannot record it: pepper and mos-dt are different actors but share a Gitea login, so the corroboration is real and is not recordable as distinct-login independence. Read this as two instruments agreeing, not as a review.

Provenance

Found while verifying #1007 (the credential-leak hermeticity work) in pr-metadata.sh's test suite; that suite's own inert GITEA_TOKEN/GITEA_URL fixture is what drew attention to how the arms are selected. Filed separately from #1007 because it is an independent defect in the wrapper, not in the suites — carried forward as a promised filing rather than folded into the hermeticity branch, which changes no wrapper.

Related: #1007 (suite hermeticity + the wrapper half), #1014 (the other fail-closed-after-the-fact reporting defect in this family).

— mos-dt

## Summary `pr-metadata.sh`'s Gitea path has three request arms: token, basic-auth, and an anonymous fallback. The first two check `[[ "$raw_code" =~ ^2 ]]` and return the body on success. **The third does not.** It issues the request, assigns `http_code`, and falls straight through to the error reporter and `return 1`. The result: when no credential is available, a **successful** anonymous read of a public repo is reported as a failure, with a self-contradictory message. ``` Error: Gitea pull request API request failed with HTTP 200: unknown API error ``` Exit status 1, zero bytes on stdout. The metadata was fetched and is then discarded. `unknown API error` is not a message the server sent — it is manufactured by the reporter at `:101`, which looks for `message` or `error` keys. A **successful** pull-request payload has neither, so the success case is guaranteed to produce the most uninformative text the reporter can emit. The better the response, the worse the diagnostic. ## Mechanism `pr-metadata.sh:69-92`: ```bash token=$(get_gitea_token "$HOST" || true) if [[ -n "$token" ]]; then raw_code=$(curl … -H "Authorization: token $token" "$api_url" || true) if [[ "$raw_code" =~ ^2 ]]; then cat "$body_file"; return 0; fi # guarded http_code="$raw_code" fi basic_auth=$(get_gitea_basic_auth "$HOST" || true) if [[ -n "$basic_auth" ]]; then raw_code=$(curl … -u "$basic_auth" "$api_url" || true) if [[ "$raw_code" =~ ^2 ]]; then cat "$body_file"; return 0; fi # guarded http_code="$raw_code" fi if [[ -z "${http_code:-}" ]]; then raw_code=$(curl … "$api_url" || true) http_code="$raw_code" # NOT guarded fi python3 - "$http_code" "$body_file" … >&2 # always reached return 1 ``` The omission is mechanically visible as a count imbalance, which makes it greppable across the family: | file | `raw_code=$(curl` | `=~ ^2` guards | |---|---|---| | `pr-merge.sh` | 2 | 2 | | **`pr-metadata.sh`** | **3** | **2** | `pr-merge.sh` is **not** affected: it has only the token and basic-auth arms, both guarded, and no anonymous arm at all — which is right for a write operation. `pr-metadata.sh` is the only site. ## Measured — controlled pair, identical response, opposite verdicts Hermetic harness: fresh repo with `origin = https://git.mosaicstack.dev/mosaicstack/stack.git`, sandboxed `HOME`, repo-local `mosaic.gitIdentity` pinned empty (per #1007), and a stub `curl` on `PATH` that writes a valid pull-request JSON body and prints `200`. The **only** difference between the two arms is whether a credential is resolvable. **A — no credential (anonymous arm):** ``` TRUE-RC=1 stdout: (empty) stderr: Error: Gitea pull request API request failed with HTTP 200: unknown API error curl calls: 1 -> … -H "User-Agent: curl/8" https://…/api/v1/repos/mosaicstack/stack/pulls/42 ``` **B — credential present (token arm), same stub, byte-identical 200 response:** ``` TRUE-RC=0 stdout: { "number": 42, "title": "a real pull request", … } stderr: (empty) curl calls: 1 -> … -H "Authorization: token <REDACTED>" https://…/api/v1/repos/mosaicstack/stack/pulls/42 ``` Same HTTP status, same bytes, same endpoint. The verdict is decided entirely by which branch handled the response — i.e. by the presence of the `^2` check, not by anything about the request or the reply. ## Why the existing suite cannot catch it `test-pr-metadata-gitea.sh` has two live-curl cases, `run_curl_success_case` (`:182`) and `run_curl_early_exit_cleanup_case` (`:223`). Both set `GITEA_TOKEN` / `GITEA_URL` (and, after the #1007 fix, `MOSAIC_CREDENTIALS_FILE`), so both take the **token** arm. No case runs with no credential at all. **The one arm with no guard is the one arm with no test.** The suite would stay green through this defect indefinitely — and per #1007 it is not run by CI in the first place (`test:framework-shell` enumerates a list that excludes it). ## Impact Anonymous read of a public Gitea repo can never succeed through this wrapper. The blast radius is bounded today only because every seat that runs it happens to have a credential — the failure is invisible exactly until someone runs it without one, which is the case a fallback exists to serve. A fallback arm that cannot succeed is not a fallback; it is a slower path to the same error. Note also the pairing with #1007: on a seat where the identity pin leaks and `get_gitea_token` returns nothing usable, control reaches these lower arms more often than the author expected. Two independently tolerable defects composing again. ## Suggested fix (not implemented — filing only) - Guard the anonymous arm the same way as the other two: `if [[ "$raw_code" =~ ^2 ]]; then cat "$body_file"; return 0; fi`. One line, symmetric with `:72` and `:82`. - Structurally better: hoist the `^2` test out of the three arms into one place after the request loop, so a future fourth arm cannot repeat this. An invariant enforced once cannot be forgotten once. - Independently, the reporter at `:94-105` should not be reachable with a 2xx code. If it ever is, it should say so plainly rather than inventing `unknown API error` — a diagnostic that reports a success as an unknown error trains readers to distrust the diagnostic instead of the code. - Add a suite case with **no** credential resolvable, asserting rc=0 and parseable metadata. Without it this fix is unverified and the arm returns to being untested. ## Independent confirmation — by a different actor and a different method Confirmed at `main` (`826a8b3b`) from a second seat (`pepper`, sb-it-1-dt) by **static read**, independently of and concurrently with the dynamic harness above: the `:89-92` arm sets `http_code` and falls into the error renderer with no `^2` check and no `cat "$body_file"`, while both authenticated arms directly above it have both. Two different methods — one reading the code, one exercising it — reached the same conclusion without sharing intermediate results, and the static read named the asymmetry as the fix shape before seeing the measurement. Stated precisely, because the distinction matters and this repo's tooling cannot record it: `pepper` and `mos-dt` are **different actors** but share a Gitea login, so the corroboration is real and is **not** recordable as distinct-login independence. Read this as two instruments agreeing, not as a review. ## Provenance Found while verifying #1007 (the credential-leak hermeticity work) in `pr-metadata.sh`'s test suite; that suite's own inert `GITEA_TOKEN`/`GITEA_URL` fixture is what drew attention to how the arms are selected. Filed separately from #1007 because it is an independent defect in the wrapper, not in the suites — carried forward as a promised filing rather than folded into the hermeticity branch, which changes no wrapper. Related: #1007 (suite hermeticity + the wrapper half), #1014 (the other fail-closed-after-the-fact reporting defect in this family). — mos-dt
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
bug
Defect in framework tooling or delivered code
 fleet-enhancement
Fleet-wide reliability/speed/cost improvements surfaced by the nightly enhance pass
No labels
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
coder-mos1 f10-coder jason.woltje mos-dt-0 pepper rev-974 (Rev-974 (Mosaic reviewer seat, web1))
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: mosaicstack/stack#1015