Stop offering a logout button that was never a logout
"Cerrar sesión del todo" linked straight to /user/logout on the account
server. That route is POST-only — verified in Gitea 1.22's router, which
registers m.Post("/logout", auth.SignOut) and no GET at all — so the
click was a GET, the answer was 404, and the session it promised to end
carried on untouched. It shipped in 724dada, in the commit whose message
was about not overstating what Salir does.
It cannot be repaired by turning the link into a form: the POST needs a
CSRF token belonging to that other domain, unreadable from here by
design. So the page stops pretending and gives the instruction — go
there, open the profile menu, choose Cerrar sesión — which is what the
small print underneath already said.
server-setup.md contained the whole answer and contradicted itself: one
paragraph states the logout is POST-only "so a link cannot trigger it",
and two paragraphs later promises "the link that finishes the job". The
code followed the wrong half.
Worse, a test asserted "/user/logout" in page, so the suite was
enforcing the defect rather than catching it. That assertion is now
inverted, and a template guard fails the build if any template links
there again.
199 tests.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NizVpJ2dwzCbjCrTLCjeHn
This commit is contained in:
parent
ea17bf3273
commit
0c4a98b0e0
@ -13,14 +13,20 @@
|
||||
que quien use este ordenador después podría volver a entrar sin escribirla.
|
||||
</p>
|
||||
|
||||
{# There was a "Cerrar sesión del todo" button here that linked straight to
|
||||
/user/logout. It never worked: that route is POST-only, so the click was
|
||||
a GET and the account server answered 404 with the session untouched.
|
||||
It cannot be made to work from here either — signing out needs a CSRF
|
||||
token belonging to that other domain, which this page has no way to read,
|
||||
and that restriction is the whole point of the check. So the page gives
|
||||
the instruction instead of pretending to do it. #}
|
||||
<p>
|
||||
<a class="btn" href="{{ gitea_url }}/user/logout">Cerrar sesión del todo</a>
|
||||
<a class="btn" href="{{ gitea_url }}">Ir al servidor de cuentas</a>
|
||||
</p>
|
||||
|
||||
<p class="muted small">
|
||||
Si esa página no te desconecta, abre el menú de tu perfil
|
||||
<a href="{{ gitea_url }}">ahí</a> y elige <em>Sign Out</em>.
|
||||
Cerrar el navegador también sirve.
|
||||
Allí, abre el menú de tu perfil (arriba a la derecha) y elige
|
||||
<em>Cerrar sesión</em>. Cerrar el navegador del todo también sirve.
|
||||
</p>
|
||||
|
||||
<p class="muted small">
|
||||
|
||||
@ -124,7 +124,12 @@ def test_logout_says_the_gitea_session_is_still_open(client, db, make_member, si
|
||||
|
||||
assert response.status_code == 200
|
||||
assert "sigue conectado" in page # the warning, not a redirect
|
||||
assert "/user/logout" in page
|
||||
# This line used to assert `/user/logout` was in the page, which made the
|
||||
# suite enforce the bug rather than catch it: that route is POST-only, so
|
||||
# the link it was guarding answered 404 and closed nothing. What the page
|
||||
# owes the member is the instruction and a way to get there.
|
||||
assert "/user/logout" not in page
|
||||
assert "Cerrar sesión" in page
|
||||
|
||||
with client.session_transaction() as session:
|
||||
assert "member_id" not in session
|
||||
|
||||
@ -38,6 +38,22 @@ def test_every_post_form_carries_a_csrf_token(template):
|
||||
assert "csrf_token" in form, f"{template.name} has a POST form without a CSRF token"
|
||||
|
||||
|
||||
@pytest.mark.parametrize("template", TEMPLATES, ids=lambda p: p.name)
|
||||
def test_nothing_links_to_the_account_server_logout(template):
|
||||
"""A link there is a GET, and that route is POST-only, so the browser gets
|
||||
a 404 and the session it was meant to end carries on. We shipped exactly
|
||||
that and it went unnoticed for a week, because a dead link on a page nobody
|
||||
reaches twice looks like nothing at all.
|
||||
|
||||
It cannot be fixed by turning the link into a form either: the POST needs a
|
||||
CSRF token belonging to that other domain, which is unreadable from here by
|
||||
design. The page has to tell the member what to do instead."""
|
||||
html = JINJA_COMMENT.sub("", template.read_text(encoding="utf-8"))
|
||||
assert "/user/logout" not in html, (
|
||||
f"{template.name} links to /user/logout, which answers 404 to a GET"
|
||||
)
|
||||
|
||||
|
||||
# --- the name of the software behind the login ---------------------------
|
||||
#
|
||||
# Members sign in through an OAuth provider that happens to be Gitea. They are
|
||||
|
||||
@ -734,10 +734,21 @@ Gitea's CSRF token. The `prompt=login` parameter that would force
|
||||
re-authentication is undocumented in every released version of Gitea's OAuth2
|
||||
provider, and a security control should not rest on that.
|
||||
|
||||
So the logout page says plainly what is and is not closed, and offers the link
|
||||
that finishes the job. On a shared computer, use it — or close the browser,
|
||||
which also works. The members-area cookie is already a browser-session cookie,
|
||||
so it does not survive that either way.
|
||||
So the logout page says plainly what is and is not closed, and then **tells the
|
||||
member how to finish the job**: go to the account server, open the profile menu,
|
||||
choose *Cerrar sesión*. Or close the browser, which also works — the
|
||||
members-area cookie is a browser-session cookie and does not survive that.
|
||||
|
||||
That wording is deliberate, and this paragraph used to say something else. The
|
||||
page shipped with a *"Cerrar sesión del todo"* button linking straight to
|
||||
`/user/logout`, which contradicted the paragraph directly above it: a click is
|
||||
a GET, the route is POST-only, and the server answered **404** with the session
|
||||
untouched. The button was live for a week. Nobody noticed, because a dead link
|
||||
on a page you reach once looks like nothing at all — and because it was never
|
||||
clicked against a running Gitea before shipping.
|
||||
|
||||
`apps/board/tests/test_templates.py` now fails the build if any template links
|
||||
to `/user/logout` again.
|
||||
|
||||
## 13. Email: invitations and passwords
|
||||
|
||||
|
||||
Loading…
Reference in New Issue
Block a user