fix(ui-13): origin_matches permissive in dev posture#116
Merged
Conversation
Closes phase-4-dogfood Bug "Could not sign in" (2026-05-09). Root cause: origin_matches() short-circuited on `request_origin in expected_origins` before reaching the dev fallback. With empty expected_origins and a browser-sent Origin header, the check returned False unconditionally and the server replied 403, which the JS form handler surfaced as the generic "could not sign in" chip. Curl worked because curl omits Origin by default; browsers always send it on POST (same-origin too). Fix: short-circuit to True when deployed=False AND expected_origins=(). Strict semantics preserved for deployed posture and for dev posture with explicitly configured origins. Brief §3-F amended to seal the dev-permissive semantics. Dogfood log resolution block under the Bug section + sprint item #2 marked resolved + two hygiene follow-ups added (login chip text per status code; derive_client_ip dev edge that returns the !unknown: sentinel for a direct loopback peer). Tests: 4 new cases in test_auth_session.py covering the bug shape, Referer-only variant, strict-when-configured invariant, and deployed-always-strict invariant. 32/32 in test_auth_session.py + full auth suite green (one pre-existing Windows-only CRLF failure in test_ui_server.py is unrelated and also fails on main). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3e5f9ca0ad
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+643
to
+644
| if not deployed and not expected_origins: | ||
| return True |
There was a problem hiding this comment.
When karasu ui is loopback-bound behind the documented Caddy/nginx deployment but auth.expected_origins is omitted, cmd_ui derives deployed=False from the empty origin list and only refuses non-loopback binds, so this new unconditional True makes public browser requests through the proxy pass the Origin/Referer gate instead of failing closed. That same deployed flag also leaves auth cookies without Secure, while the deploy runbook says the default empty origins must be replaced for deployed posture. Please restrict this bypass to direct local-dev traffic (for example no forwarded/proxy context) or fail startup for the reverse-proxy shape with empty origins.
Useful? React with 👍 / 👎.
This was referenced May 16, 2026
VDP89
added a commit
that referenced
this pull request
May 16, 2026
Closes the phase-4-dogfood Bug "Could not sign in" side observation (logged as hygiene follow-up in PR #116). Background: a dev operator running `karasu ui` on the default 127.0.0.1:8787 with no karasu.yaml hits a direct- loopback browser POST against an auth-enabled listener. The peer is 127.0.0.1, the chain is empty, and the default trusted_proxies=("127.0.0.1", "::1") includes the peer. derive_client_ip then walks the trusted-peer + empty-chain branch and returns None at the primitive level. The wrapper _ip_for_rate_limit synthesises the "!unknown:127.0.0.1" key (designed to prevent the loopback bypass from masking deployed-proxy misconfigs); LoginRateLimit.check then enforces rate-limit against the synthetic key, so a developer iterating against the login form eats 429s after five typos in the password. Fix: posture-aware wrapping. When AUTH_DEPLOYED is False AND the peer is loopback, _ip_for_rate_limit returns the peer verbatim so the loopback bypass at LoginRateLimit.check fires. Deployed posture stays strict — a trusted peer + empty chain there means the proxy is misconfigured (forgot to forward XFF/Forwarded) and the "!unknown:" key surfaces the symptom in logs. The primitive derive_client_ip is unchanged; its contract remains "report what I can verify" and existing test_auth_trusted_ip.py coverage continues to pass. Brief §3-G amended in-place under the "Resolved bypass logic" §4 bullet to seal the new dev-aware semantics. Codex round 1 P0 + round 3 P1 invariants are preserved: deployed posture cannot regress to the loopback bypass, UNTRUSTED_FORWARDED still keys by peer with the "!untrusted:" prefix that can never match is_loopback_ip. Tests: - test_dev_posture_direct_loopback_bypasses_rate_limit — drives 6 wrong-password attempts in dev posture + one right-password; the 7th must succeed (200 with cookies). If the bypass were not firing, the right- password attempt would 429. - test_deployed_posture_trusted_peer_empty_chain_still_rate_limits — drives 5 wrong-password attempts in deployed posture via direct loopback (no XFF, simulating proxy misconfig); the 6th must 429. Contra-positive guard against the fix accidentally masking deployed misconfigs. Suite: 249/249 across auth_credentials + auth_middleware + auth_rate_limit + auth_server + auth_session + auth_trusted_ip + cli_auth (2 pre-existing skips). Co-authored-by: Victor Del Puerto <VDP89@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
VDP89
added a commit
that referenced
this pull request
May 16, 2026
Closes the phase-4-dogfood "Could not sign in" diagnosis hygiene #5(a) (logged in PR #116's resolution block). Background: the login form JS handler in login.html unhid a static "Could not sign in." chip for every 4xx, regardless of cause. The dogfood diagnosis surfaced this as a real bias: a 403 (origin/CSRF mismatch) looked identical to a 401 to the operator, and the hypothesis order skewed toward credentials/rate-limit when the real cause was origin. Same trap for 429 (rate-limit) and 503 (auth-not-configured). Fix: the JS handler now reads the response body via r.text(), parses the canonical {"error":...} JSON, and renders the server's error phrase as the chip text via slot.textContent (capitalised + trailing period added). The JS never invents text; it only RENDERS what the server already declared. Fallback for non-JSON bodies keeps the generic "Could not sign in.". 413 ships plain text ("payload too large") so it gets a dedicated "Request too large." chip. §3-G online-guessing invariant preserved: 401 still shows the generic "Could not sign in." (server JSON: "could not sign in" → chip same after capitalisation), no username-vs-password leak. The chip-text status discrimination scopes to PRE-auth rejection paths (403/429/400/422/503) where there is no credential disclosure risk. Brief §3-E amended in-place to scope the generic-copy invariant to status 401 only and to seal the new discriminating shape for the pre-auth paths. Tests: - tests/test_ui_sw.py::test_login_html_chip_text_reflects_server_error_per_status — parses login.html and asserts: fallback string present, r.text() reads body, slot.textContent is assigned dynamically, 413 fallback present, data.error is the field consulted (not an invented status map). Suite: 18/18 in test_ui_sw.py, 52/52 in test_auth_server.py. Co-authored-by: Victor Del Puerto <VDP89@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 task
VDP89
added a commit
that referenced
this pull request
May 16, 2026
Reconciles the cross-PR references that the individual fix PRs could not touch because they branched off main while the hygiene items only existed in PR #116's resolution block. Updates: - Hygiene #5(a) (login chip text per status) marked resolved by PR #122. - Hygiene #5(b) (`!unknown:` sentinel for direct loopback) marked resolved by PR #121. - Finding #3 sub-friction 3 (modal does not surface CLI command) marked resolved by PR #123. The "Outstanding sprint items" section is rewritten as "Sprint items closed 2026-05-16" — an audit trail of the five items addressed in one session, each linked to its landing PR. Adjacent housekeeping (the Karasu- → Karasu rename + PR #120 reference cleanup) is also captured for the historical record. Final line marks path C VPS deploy as unblocked at the code surface; remaining gate is operational (domain + VPS + caddy/Let's Encrypt per docs/deploy-runbook.md). Pure docs change — no code, no tests, no brief touched. Co-authored-by: Victor Del Puerto <VDP89@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
origin_matches()in_auth.pyshort-circuited onrequest_origin in expected_originsbefore reaching the dev fallback. With emptyexpected_originsand a browser-sentOriginheader (browsers always send Origin on POST, same-origin too), the check returned False and the server replied 403. Curl worked because curl omits Origin by default — that masked the bug from any test that didn't go through a real browser.Truewhendeployed=False AND expected_origins=(). Deployed posture and dev-posture-with-explicit-origins remain strict.Files
src/karasu/ui/_auth.py— the fix + extended docstring tying the change to the brief amendment and the diagnosis.tests/test_auth_session.py— 4 new cases: bug shape, Referer-only variant, strict-when-configured invariant, deployed-always-strict invariant.docs/ui/ui-13-design-brief.md— §3-F amendment 2026-05-16 sealing the dev-permissive semantics.docs/phase-4-dogfood.md— full resolution block under the Bug section, item ci: add GitHub Actions workflow running pytest on push and PRs #2 marked resolved, two hygiene follow-ups added.Diagnosis sequence (for future reference)
verify_passwordagainst the on-disk hash → True. Hypothesis 2 ruled out.karasu uirunning, port 8787 free. Hypothesis 3 ruled out.curl -X POST /auth/login password=wrong→ 401 + log line. Endpoint works; browser is the difference.index:96(form fetch) +sw.js:272(passthrough, not caching).{"error":"forbidden"}localized toorigin_matches. Emptyexpected_origins+ browser-sent Origin = guaranteed false.Hypothesis 1 (rate-limit) was structurally impossible —
LoginRateLimit.checkbypasses loopback IPs before any failure counting.Hygiene follow-ups (out of scope, logged in dogfood §5)
login failed (ip=!unknown:127.0.0.1)—_ip_for_rate_limitreturns the!unknown:sentinel whenderive_client_ipreturnsNone. That branch should not fire for a direct loopback peer with no forwarded chain; suggests a §3-G dev-posture edge worth auditing.Test plan
pytest tests/test_auth_session.py— 32/32 (28 pre-existing + 4 new)test_auth_*.py+test_cli_auth.py+test_ui_server*.py) — 385/386. The single failure istest_valid_asset_under_static_dir_is_servedand it ALSO fails on main (Windows-only CRLF vs LF issue inPath.write_text); unrelated.python -m karasu ui(auth on, default) → login from browser → 200 + redirect + shell renders.🤖 Generated with Claude Code