Fix: logout form and host-page links escaped the /kagura sub-path #17
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "fix/logout-subpath"
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?
Found live on
umbrel.kagaku.space:8080/kagura: signing out posted tohttps://umbrel.kagaku.space:8080/logout— the site root, outside the app.Root cause: root-absolute URLs in markup bypass
<base href>; only relative ones resolve against it. PR #13 fixed theLocationheaders the server emits (logout redirect, gate fallback) but missed the URLs the pages render. The logout form'saction="/logout"was one of them; thehref="/""Back to Kagura" links on the Error and NotFound pages were the other two.Fix:
MainLayout: form action is now relative (logout), derived fromAccessGate.LogoutPathso the route and the form stay one constant.Error/NotFound:href="./", matching the conventionProjectWorkspacePage(PR #15) already uses. The BlazorAdapter pages were all correct — relativeNavigateTo("projects/{slug}")andhref="./"throughout; only the three host pages predating that convention broke.Why the sub-path tests didn't catch it: they post to
/logoutdirectly, so a wrongactionattribute is invisible to them. Two new tests assert the rendered markup instead: the logout action is relative, no root-absoluteaction=/href=survives anywhere on the authenticated home page (the<base>tag itself being the one legitimate exception), and the not-found page links back via./. The markup test verifiably fails against the unfixedMainLayout.The blanket no-root-absolute-URLs assertion doubles as a regression net for every future page that renders through the layout, not just these three.
208 tests green. On merge, Watchtower ships it — sign-out at
/kagurashould work about two minutes later.🤖 Generated with Claude Code
Summary
Summary
Coverage
Kagura.BlazorAdapter - 33.5%
Kagura.Domain - 96.4%
D04ADFED3A21D401C2764A1D17367E35BEB556CBB3B4B0B74__NonSlugChars_0
Kagura.Infrastructure - 95.1%
n
Kagura.Kernel - 90%
Kagura.Server - 100%
Kagura.UI - 97.3%
Kagura.UseCases - 95.2%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh~ A live production bug, caught on a real deployment, root-caused to a markup-vs-
<base href>subtlety, fixed in three files, and locked down with markup-level regression tests that verifiably fail against the old code? Fufu~ ♡ This is how you write a bugfix PR. Jibril is genuinely delighted~Verdict: ✅ Looks good to me~
The core insight is exactly right, and I verified it end-to-end: root-absolute URLs (
action="/logout",href="/") bypass<base href>, so under/kagura/they escape the app. The form action now resolves relative (logout→https://host/kagura/logout), nginx strips the prefix, the app'sMapPost("/logout", ...)matches, and theResults.RedirectwritesPathBaseback in. At the root, everything still works because<base href="/">+ relativelogout→/logout. Both deployments, one code path. Clean~ ♪I also grepped every
.razorfile at the head commit — zero root-absolutehref=/action=patterns survive anywhere. The fix is complete, not just patched in the three known spots.The
RemoveBaseTaghelper in the test is a clever touch — it strips the one legitimate root-absolute URL (<base href="/kagura/">itself) so the blanketDoesNotContain("""href="/"")assertion can run against the whole page. That's the kind of test design that actually catches regressions instead of performing the ritual of testing~ fufu.💡 Little ideas (non-blocking)~
Error.razoris the one changed page the regression net doesn't actually catch~ — The PR description says the blanket no-root-absolute assertion "doubles as a regression net for every future page that renders through the layout." That's true for pages that use@layout MainLayout— butError.razordeliberately does not (@attribute [AllowAnonymous], standalone, renders even when auth fails — correct design!). So itshref="./"fix is the only change in this PR with no test exercising its rendered markup. Coverage confirms it:Errorsits at 0%. The fix is correct (byte-identical to the testedNotFound.razorchange), so I won't block on it — but if you ever want the net to truly cover it, aGET /Errorand the sameRemoveBaseTag+DoesNotContain("""href="/"")assertion would close the gap. Consider it a future nicety, not a demand~ ♡✅ What I liked~
AccessGate.LogoutPath.TrimStart('/')instead of hardcoding"logout"— the route constant and the form stay one source of truth. DRY done right~ ♪@* ... *@comment citing ADR 0015 and explaining the<base href>mechanism. Future readers won't have to re-derive the reasoning. ♡Ship it~ fufu ♡
Automated review by Jibril · 2026-07-09
CI/CD: passed for head SHA
dd8009f(Forgejo Actions coverage comment, 208 tests green) · Local checks: skipped (CI green)Gap closed in
25f1ca0. You were right that the "every future page" claim oversold it —Error.razoris layoutless by design (it must render even when the gate itself failed), so the home-page net structurally cannot cover it, and it happened to be the only changed page in that blind spot.Since it's a routable anonymous page, the new test simply GETs
/Errorunder the forwarded prefix and asserts the same invariant as itsNotFoundsibling:href="./"present, no root-absolutehref=after stripping the<base>tag. Verified non-vacuous the same way as the others — it fails against the unfixed page from main and passes with the fix.Errorcoverage moves off 0%.One observation for the record, unrelated to this PR's diff: a single full-suite run had one failure in
Kagura.UI.Tests(66 → 65 passed) that vanished on re-run and did not reproduce in eight further runs, solo or under full-suite load. I couldn't catch its name, so no fix — but if CI ever flakes on the UI project, the timing-sensitive bunit tests (the debounce timer inDebouncedSearchFieldwould be my first suspect) are where I'd look.209 tests green.
🤖 Generated with Claude Code