pbwraith/gitea-saas-core
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(dashboard): require X-API-Key on /dashboard (fail-closed, #25) #26

Closed
pbwraith wants to merge 0 commits from issue-25 into main
pull from: issue-25
merge into: pbwraith:main
Conversation 5 Commits - Files Changed -
pbwraith commented 2026-08-26 09:56:18 +02:00
Owner
Copy Link

Closes #25

The finding

GET /dashboard served clients, ledger, queue, throughput and review-gate
data with no auth at all, while the sibling JSON route
GET /api/dashboard/summary served the same data behind _guard
(X-API-Key, 401/503 fail-closed). The loopback requirement the HTML page
relied on existed only as prose — a page note and a docstring line — and was
never enforced anywhere: mount_dashboard registers routes on a caller-owned
Flask app and neither inspects the bind address nor reads request.host, so
app.run(host="0.0.0.0") (or any reverse proxy) would publish the billing
dashboard to the network unauthenticated.

Fix (preferred route from the issue)

Put /dashboard behind the same _guard as /api/dashboard/summary:

  • no X-API-Key → 401 and no billing data in the body
  • no configured key → 503 (fail-closed)
  • valid key → 200 HTML page, rendering unchanged

The now-false loopback-only prose claims were removed from the page note and
the mount_dashboard docstring; loopback binding remains deployment
defense-in-depth, not the authorization boundary.

Acceptance criteria

  1. GET /dashboard with no X-API-Key does not return billing data — 401
    and body contains no client/ledger/queue data
    (test_dashboard_page_guard_blocks_anonymous_access).

  2. Non-loopback Host rejection — N/A: took the preferred _guard route, not
    allow_non_loopback.

  3. Mutation check — removing @_guard from /dashboard makes both
    test_dashboard_page_guard_blocks_anonymous_access and
    test_dashboard_wrong_key_same_length_rejected fail (verified live:
    assert 200 == 401). The guard is load-bearing.

  4. /api/dashboard/summary behaviour unchanged — existing JSON tests still
    pass untouched (401 wrong/missing key, 503 no configured key, 200 valid).

  5. Suite green before/after:

    • before: 158 passed, 1 skipped
    • after: 160 passed, 1 skipped (+2 security tests)

Also updated:

  • test_dashboard_page_open_and_html now sends a valid key (200 + HTML unchanged)
  • test_summary_fail_closed_without_configured_key now asserts the HTML page
    also fails closed (503), not open
  • test_dashboard_mounts_onto_existing_app sends a valid key

Out of scope (per #25): API-key consolidation (#24), stripe webhook signature
check, /healthz.

Closes #25 ## The finding `GET /dashboard` served clients, ledger, queue, throughput and review-gate data with **no auth at all**, while the sibling JSON route `GET /api/dashboard/summary` served the same data behind `_guard` (`X-API-Key`, 401/503 fail-closed). The loopback requirement the HTML page relied on existed only as prose — a page note and a docstring line — and was never enforced anywhere: `mount_dashboard` registers routes on a caller-owned Flask app and neither inspects the bind address nor reads `request.host`, so `app.run(host="0.0.0.0")` (or any reverse proxy) would publish the billing dashboard to the network unauthenticated. ## Fix (preferred route from the issue) Put `/dashboard` behind the **same `_guard`** as `/api/dashboard/summary`: - no `X-API-Key` → `401` and no billing data in the body - no configured key → `503` (fail-closed) - valid key → `200` HTML page, rendering unchanged The now-false loopback-only prose claims were removed from the page note and the `mount_dashboard` docstring; loopback binding remains deployment defense-in-depth, not the authorization boundary. ## Acceptance criteria 1. `GET /dashboard` with no `X-API-Key` does not return billing data — `401` and body contains no client/ledger/queue data (`test_dashboard_page_guard_blocks_anonymous_access`). 2. Non-loopback Host rejection — N/A: took the preferred `_guard` route, not `allow_non_loopback`. 3. Mutation check — removing `@_guard` from `/dashboard` makes both `test_dashboard_page_guard_blocks_anonymous_access` and `test_dashboard_wrong_key_same_length_rejected` fail (verified live: `assert 200 == 401`). The guard is load-bearing. 4. `/api/dashboard/summary` behaviour unchanged — existing JSON tests still pass untouched (401 wrong/missing key, 503 no configured key, 200 valid). 5. Suite green before/after: - before: `158 passed, 1 skipped` - after: `160 passed, 1 skipped` (+2 security tests) Also updated: - `test_dashboard_page_open_and_html` now sends a valid key (200 + HTML unchanged) - `test_summary_fail_closed_without_configured_key` now asserts the HTML page also fails closed (503), not open - `test_dashboard_mounts_onto_existing_app` sends a valid key Out of scope (per #25): API-key consolidation (#24), stripe webhook signature check, `/healthz`.
pbwraith added 1 commit 2026-08-26 09:56:18 +02:00 
The HTML operator page served the same clients/ledger/queue/review data as
the authenticated JSON summary, but with no auth at all. The loopback
requirement it relied on existed only as prose (page note + docstring); it
was never enforced by mount_dashboard, which does not own the app and
cannot constrain the bind address.

Put /dashboard behind the same _guard as /api/dashboard/summary:
- no X-API-Key -> 401, no billing data
- no configured key -> 503 (fail-closed)
- valid key -> 200 HTML (unchanged rendering)

Also drop the now-false 'loopback-only' security claims from the page note
and the mount_dashboard docstring, and update the tests:
- test_dashboard_page_open_and_html now sends a valid key
- new test_dashboard_page_guard_blocks_anonymous_access: no key -> 401,
  body contains no client data (mutation check: fails if guard removed)
- new test_dashboard_wrong_key_same_length_rejected: wrong same-length
  key -> 401 on HTML page too
- test_summary_fail_closed_without_configured_key now asserts the HTML
  page also fails closed (503), not open
- test_dashboard_mounts_onto_existing_app sends a valid key

/api/dashboard/summary behaviour unchanged. Suite: 160 passed, 1 skipped
(before: 158 passed, 1 skipped).
pbwraith commented 2026-08-26 13:30:33 +02:00
Author
Owner
Copy Link

Verified green, deliberately NOT merged -- escalated to the owner.

This PR is on the sweep's escalation list ("the PR touches auth, credentials,
..."), and this repo as a whole is held pending an owner decision on the auth
design. So this is a review, not a merge.

What I measured

merge origin/main into issue-25   CONFLICTS=0   HEAD=08febd5
python3 -m pytest -q              160 passed, 1 skipped in 105.33s

That matches the PR's own claim (158 -> 160, +2 security tests) exactly.

The fix is the right shape

GET /dashboard served clients, ledger, queue and throughput data with no auth
while its sibling GET /api/dashboard/summary served the SAME data behind
_guard. Putting both behind one guard is correct, and removing the
loopback-only prose is the right call: mount_dashboard registers routes on a
caller-owned Flask app and never inspects the bind address or request.host,
so that claim was never enforced by anything. A comment asserting a security
property nothing implements is worse than no comment.

The reason this is held, and it is not about quality

!27 (#24) conflicts with this PR, and the conflict resolution can silently
undo it. See the detail on !27 -- summary: !27 branched from a main where
/dashboard was public and still carries

@app.get("/dashboard")
def dashboard_page():
    # No auth - this page is safe only because the server binds to loopback.

so resolving the conflict in !27's favour reinstates the exact hole this PR
closes. Merging this first and then mis-resolving !27 is a realistic path to
believing #25 is fixed while it is not.

Ordering recommendation for whenever the owner greenlights: this one first,
then rebase !27 onto it, then re-run the suite and assert
test_dashboard_page_guard_blocks_anonymous_access still fails when the guard
is removed.

**Verified green, deliberately NOT merged -- escalated to the owner.** This PR is on the sweep's escalation list ("the PR touches auth, credentials, ..."), and this repo as a whole is held pending an owner decision on the auth design. So this is a review, not a merge. ## What I measured merge origin/main into issue-25 CONFLICTS=0 HEAD=08febd5 python3 -m pytest -q 160 passed, 1 skipped in 105.33s That matches the PR's own claim (158 -> 160, +2 security tests) exactly. ## The fix is the right shape `GET /dashboard` served clients, ledger, queue and throughput data with no auth while its sibling `GET /api/dashboard/summary` served the SAME data behind `_guard`. Putting both behind one guard is correct, and removing the loopback-only prose is the right call: `mount_dashboard` registers routes on a caller-owned Flask app and never inspects the bind address or `request.host`, so that claim was never enforced by anything. A comment asserting a security property nothing implements is worse than no comment. ## The reason this is held, and it is not about quality `!27` (#24) conflicts with this PR, and the conflict resolution can silently undo it. See the detail on `!27` -- summary: `!27` branched from a main where `/dashboard` was public and still carries @app.get("/dashboard") def dashboard_page(): # No auth - this page is safe only because the server binds to loopback. so resolving the conflict in `!27`'s favour reinstates the exact hole this PR closes. Merging this first and then mis-resolving `!27` is a realistic path to believing #25 is fixed while it is not. Ordering recommendation for whenever the owner greenlights: **this one first**, then rebase `!27` onto it, then re-run the suite and assert `test_dashboard_page_guard_blocks_anonymous_access` still fails when the guard is removed.
pbwraith commented 2026-08-26 19:56:40 +02:00
Author
Owner
Copy Link

Escalated to the owner — not merged by the sweep

This PR is on the sweep's escalation list and I am deliberately leaving it for a
human. The standing rule: a PR that touches auth, credentials, execute_action,
the action allow-list, or the adversarial corpus is escalated rather than merged.
gitea-saas-core is additionally a standing owner escalation in its own right —
it holds a key authorizing debits, a Stripe secret, and a repo-creating Gitea
token, so the blast radius of an auth mistake here is money and repo control, not
a failed test.

This is not a defect finding. I have not reviewed the diff on its merits and
nothing here says the change is wrong. It is a "who is allowed to merge this"
call, not a "is this correct" call.

Deliberately not labelling this bounced: that label means "rework wanted"
and would put it in the worker rework queue, which is the opposite of what this
needs.

I also could not have run a gate this sweep even if the change were mergeable —
see gitea-worker-ops#40: wsl.exe exec fails on the review box with
Wsl/Service/0x8007274c (reproduced twice, 31s each) while wsl -l -v still
reports the distro Running.

What the owner needs to do: review and merge, or hand it back with a finding.

## Escalated to the owner — not merged by the sweep This PR is on the sweep's escalation list and I am deliberately leaving it for a human. The standing rule: *a PR that touches auth, credentials, `execute_action`, the action allow-list, or the adversarial corpus is escalated rather than merged.* `gitea-saas-core` is additionally a standing owner escalation in its own right — it holds a key authorizing debits, a Stripe secret, and a repo-creating Gitea token, so the blast radius of an auth mistake here is money and repo control, not a failed test. **This is not a defect finding.** I have not reviewed the diff on its merits and nothing here says the change is wrong. It is a "who is allowed to merge this" call, not a "is this correct" call. Deliberately **not** labelling this `bounced`: that label means "rework wanted" and would put it in the worker rework queue, which is the opposite of what this needs. I also could not have run a gate this sweep even if the change were mergeable — see `gitea-worker-ops#40`: `wsl.exe` exec fails on the review box with `Wsl/Service/0x8007274c` (reproduced twice, 31s each) while `wsl -l -v` still reports the distro `Running`. **What the owner needs to do:** review and merge, or hand it back with a finding.
pbwraith commented 2026-08-27 00:16:40 +02:00
Author
Owner
Copy Link

Gate evidence -- the "I could not run a gate" caveat on my earlier escalation is now stale

My 2026-08-26 escalation said I could not gate this because wsl.exe exec was
failing on the review box (gitea-worker-ops#40). That has cleared -- probed
wsl -e echo alive -> alive, exit 0. So I have now run the real gate.

The escalation itself is unchanged and this PR is still not merged by the
sweep. This comment adds evidence only; it is not a defect finding and not an
approval.

Gate: pytest -q under C:\Users\Videostation\AppData\Local\Programs\Python\Python312\python.exe
(pytest 8.3.2), in a throwaway detached worktree, against the merge result
(git merge origin/main into the PR branch), never the branch alone.

The suite is safe to run here: ARCHITECTURE.md:72 states the tests are
hermetic (mock Stripe + mock Gitea + tmp SQLite), and the run touches no live
service, no Stripe key and no real Gitea token.

baseline  origin/main  ccccfbf      158 passed, 1 skipped

merge result  !26 (issue-25 @ 08febd5 + origin/main)
  conflicts : 0
  result    : 160 passed, 1 skipped

Count arithmetic checks exactly: 158 + 2 = 160, adds 2 def test_, removes 0.

Read this before merging: !27 already contains this PR

issue-25 is a direct ancestor of issue-24 (!27), measured, not inferred:

git merge-base --is-ancestor origin/issue-25 origin/issue-24   ->  YES

!27 is stacked on top of this branch and carries both of its tests. Composing
them in a throwaway tree in either order gives 0 conflicts and 173 passed --
the same as !27 alone, not 158 + 2 + 15 = 175.

So merging !27 alone delivers this change too. !27 then goes further and
replaces the local _guard here with the shared gitea_saas/auth.py
constant-time path, so this PR's inline guard is code !27 deletes.

This PR is green and correct on its own merits; it is simply redundant if !27
lands. Whether to close it as subsumed or merge it first is a call on an
escalated auth PR, so it is the owner's, not the sweep's. Still not merged by
the sweep; the escalation is unchanged.

## Gate evidence -- the "I could not run a gate" caveat on my earlier escalation is now stale My 2026-08-26 escalation said I could not gate this because `wsl.exe` exec was failing on the review box (`gitea-worker-ops#40`). That has cleared -- probed `wsl -e echo alive` -> `alive`, exit 0. So I have now run the real gate. **The escalation itself is unchanged and this PR is still not merged by the sweep.** This comment adds evidence only; it is not a defect finding and not an approval. Gate: `pytest -q` under `C:\Users\Videostation\AppData\Local\Programs\Python\Python312\python.exe` (pytest 8.3.2), in a throwaway detached worktree, against the **merge result** (`git merge origin/main` into the PR branch), never the branch alone. The suite is safe to run here: `ARCHITECTURE.md:72` states the tests are hermetic (mock Stripe + mock Gitea + tmp SQLite), and the run touches no live service, no Stripe key and no real Gitea token. ``` baseline origin/main ccccfbf 158 passed, 1 skipped ``` ``` merge result !26 (issue-25 @ 08febd5 + origin/main) conflicts : 0 result : 160 passed, 1 skipped ``` Count arithmetic checks exactly: `158 + 2 = 160`, adds 2 `def test_`, removes 0. ## Read this before merging: !27 already contains this PR `issue-25` is a direct **ancestor** of `issue-24` (!27), measured, not inferred: ``` git merge-base --is-ancestor origin/issue-25 origin/issue-24 -> YES ``` !27 is stacked on top of this branch and carries both of its tests. Composing them in a throwaway tree in either order gives 0 conflicts and **173 passed** -- the same as !27 alone, not `158 + 2 + 15 = 175`. So **merging !27 alone delivers this change too.** !27 then goes further and replaces the local `_guard` here with the shared `gitea_saas/auth.py` constant-time path, so this PR's inline guard is code !27 deletes. This PR is green and correct on its own merits; it is simply redundant if !27 lands. Whether to close it as subsumed or merge it first is a call on an escalated auth PR, so it is the owner's, not the sweep's. **Still not merged by the sweep**; the escalation is unchanged.
pbwraith commented 2026-08-27 07:57:33 +02:00
Author
Owner
Copy Link

ChatGPT (gpt-5.6-sol) review

Reviewed by gpt-5.6-sol on the ChatGPT subscription, because Opus usage is exhausted. The model did not decide the mechanical checks — those ran first and passed:

merge origin/master into issue-25   -> 0 conflicts
pytest                   -> exit 0  (160 passed)

VERDICT: CONCERNS
REASON: ESCALATE: this changes authentication behavior, and the missing diff prevents verification that /dashboard fails closed or that tests exercise the new behavior.
NOTES:

  • The supplied diff contains only an origin/master...HEAD revision error, so no implementation or test changes are reviewable.
  • The passing 160-test gate is insufficient to validate credential handling without inspecting the added assertions and authentication logic.

Reviewed by gpt-5.6-sol while Opus usage was exhausted. The mechanical checks above are authoritative; the model's MERGE is a recommendation.

## ChatGPT (gpt-5.6-sol) review Reviewed by `gpt-5.6-sol` on the ChatGPT subscription, because Opus usage is exhausted. **The model did not decide the mechanical checks** — those ran first and passed: ``` merge origin/master into issue-25 -> 0 conflicts pytest -> exit 0 (160 passed) ``` VERDICT: CONCERNS REASON: ESCALATE: this changes authentication behavior, and the missing diff prevents verification that `/dashboard` fails closed or that tests exercise the new behavior. NOTES: - The supplied diff contains only an `origin/master...HEAD` revision error, so no implementation or test changes are reviewable. - The passing 160-test gate is insufficient to validate credential handling without inspecting the added assertions and authentication logic. --- *Reviewed by gpt-5.6-sol while Opus usage was exhausted. The mechanical checks above are authoritative; the model's MERGE is a recommendation.*
pbwraith commented 2026-08-30 00:32:25 +02:00
Author
Owner
Copy Link

Closed as subsumed by owner-authorized PR #27

PR #27 merged at head 3819b49498030f49b3ecc842f9ef5e1f29d5985e after its exact current-base synthetic merge passed 218 tests with 1 skipped. This PR's exact head 08febd56ca1ea4e23efb0f99887a76efa52ddf13 is an ancestor of both PR #27 and the resulting main b4e577b31b6b4c7cbdf6a9ac5508bfba64431b02.

Therefore the guarded /dashboard implementation and its load-bearing anonymous/same-length-wrong-key tests are already on main. This PR is closed unmerged as redundant; no work was discarded and the guard was not reimplemented twice.

## Closed as subsumed by owner-authorized PR #27 PR #27 merged at head `3819b49498030f49b3ecc842f9ef5e1f29d5985e` after its exact current-base synthetic merge passed **218 tests with 1 skipped**. This PR's exact head `08febd56ca1ea4e23efb0f99887a76efa52ddf13` is an ancestor of both PR #27 and the resulting `main` `b4e577b31b6b4c7cbdf6a9ac5508bfba64431b02`. Therefore the guarded `/dashboard` implementation and its load-bearing anonymous/same-length-wrong-key tests are already on `main`. This PR is closed unmerged as redundant; no work was discarded and the guard was not reimplemented twice.
pbwraith closed this pull request 2026-08-30 00:32:26 +02:00

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
blocked

   
bounced

   
eval-task

   
in-progress

   
in-review

   
needs-guidance

   
p1

   
p2

   
p3

   
tracking

   
unfunded
No Label
blocked
bounced
eval-task
in-progress
in-review
needs-guidance
p1
p2
p3
tracking
unfunded
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
1 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: pbwraith/gitea-saas-core#26
No description provided.
Delete Branch "issue-25"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?