Archived
Merge pull request 'Four admin forms had no CSRF token, so they 403'd in silence' (#15) from admin-csrf-forms into main
This commit is contained in:
@@ -50,7 +50,7 @@
|
|||||||
</li>
|
</li>
|
||||||
</ul>
|
</ul>
|
||||||
|
|
||||||
<form method="post" action="/admin/categories" class="mt-3 flex gap-2">
|
<form method="post" th:action="@{/admin/categories}" class="mt-3 flex gap-2">
|
||||||
<label class="flex-1">
|
<label class="flex-1">
|
||||||
<span class="sr-only">New category</span>
|
<span class="sr-only">New category</span>
|
||||||
<input class="field" name="name" placeholder="New category" required>
|
<input class="field" name="name" placeholder="New category" required>
|
||||||
@@ -67,7 +67,7 @@
|
|||||||
the location your phone put in them) and converted on upload, so this can take a few seconds each.
|
the location your phone put in them) and converted on upload, so this can take a few seconds each.
|
||||||
</p>
|
</p>
|
||||||
|
|
||||||
<form method="post" action="/admin/items" enctype="multipart/form-data" class="mt-3 space-y-3">
|
<form method="post" th:action="@{/admin/items}" enctype="multipart/form-data" class="mt-3 space-y-3">
|
||||||
<div class="grid gap-2 sm:grid-cols-[1fr_12rem]">
|
<div class="grid gap-2 sm:grid-cols-[1fr_12rem]">
|
||||||
<label class="block">
|
<label class="block">
|
||||||
<span class="sr-only">What is it?</span>
|
<span class="sr-only">What is it?</span>
|
||||||
|
|||||||
@@ -51,7 +51,7 @@
|
|||||||
No tables yet. The catering page will tell people to call instead until there is one.
|
No tables yet. The catering page will tell people to call instead until there is one.
|
||||||
</p>
|
</p>
|
||||||
|
|
||||||
<form method="post" action="/admin/catering/tables" class="mt-4 flex gap-2">
|
<form method="post" th:action="@{/admin/catering/tables}" class="mt-4 flex gap-2">
|
||||||
<label class="flex-1">
|
<label class="flex-1">
|
||||||
<span class="sr-only">New table</span>
|
<span class="sr-only">New table</span>
|
||||||
<input class="field" name="name" placeholder="New table, e.g. Graduation parties" required>
|
<input class="field" name="name" placeholder="New table, e.g. Graduation parties" required>
|
||||||
@@ -66,7 +66,7 @@
|
|||||||
<h2 class="card-heading">Under the whole page</h2>
|
<h2 class="card-heading">Under the whole page</h2>
|
||||||
<p class="mt-1 text-sm text-bakery-600">Terms that apply whichever table someone is reading.</p>
|
<p class="mt-1 text-sm text-bakery-600">Terms that apply whichever table someone is reading.</p>
|
||||||
|
|
||||||
<form method="post" action="/admin/catering/notes" class="mt-3 space-y-2">
|
<form method="post" th:action="@{/admin/catering/notes}" class="mt-3 space-y-2">
|
||||||
<label th:each="note : ${menu.notes}" class="block">
|
<label th:each="note : ${menu.notes}" class="block">
|
||||||
<span class="sr-only">Note</span>
|
<span class="sr-only">Note</span>
|
||||||
<textarea class="field min-h-[3.25rem]" rows="2" name="notes" th:text="${note}"></textarea>
|
<textarea class="field min-h-[3.25rem]" rows="2" name="notes" th:text="${note}"></textarea>
|
||||||
|
|||||||
@@ -34,8 +34,16 @@
|
|||||||
<a href="/products" class="btn-secondary">View the site</a>
|
<a href="/products" class="btn-secondary">View the site</a>
|
||||||
<!--/* A real form post: the platform's logout expects one, and it also ends the Authentik
|
<!--/* A real form post: the platform's logout expects one, and it also ends the Authentik
|
||||||
session — a link would leave you signed in at the identity provider and straight back in
|
session — a link would leave you signed in at the identity provider and straight back in
|
||||||
on the next click. */-->
|
on the next click.
|
||||||
<form method="post" action="/logout">
|
|
||||||
|
th:action, NOT action, AND THAT IS NOT COSMETIC. Thymeleaf inserts the hidden _csrf input
|
||||||
|
while it processes a th:action; a plain action= is passed through untouched, so the form
|
||||||
|
posts with no token and Spring Security answers 403. That renders the site's error page and
|
||||||
|
logs NOTHING at ERROR, so the form simply appears to be broken with no trace anywhere. It
|
||||||
|
is how "Add something new" silently refused to create a product, and how this button
|
||||||
|
silently refused to sign anyone out. AdminPagesTest counts forms against tokens so a plain
|
||||||
|
action= cannot come back. */-->
|
||||||
|
<form method="post" th:action="@{/logout}">
|
||||||
<button type="submit" class="btn-secondary">Sign out</button>
|
<button type="submit" class="btn-secondary">Sign out</button>
|
||||||
</form>
|
</form>
|
||||||
</div>
|
</div>
|
||||||
|
|||||||
@@ -101,6 +101,33 @@ class AdminPagesTest {
|
|||||||
assertThat(multipart.getMaxFileSize()).isNotEqualTo(DataSize.ofMegabytes(1));
|
assertThat(multipart.getMaxFileSize()).isNotEqualTo(DataSize.ofMegabytes(1));
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* EVERY form on an admin page has to carry a CSRF token, and four of them did not.
|
||||||
|
*
|
||||||
|
* <p>Thymeleaf inserts the hidden {@code _csrf} input while it processes a {@code th:action}. A form
|
||||||
|
* written with a plain {@code action=} is passed through untouched, so it posts without a token and
|
||||||
|
* Spring Security answers 403 — which renders the site's error page and logs nothing at ERROR, so the
|
||||||
|
* failure was invisible from the outside. "Add something new" was one of the four, which is why no
|
||||||
|
* product could be created.
|
||||||
|
*
|
||||||
|
* <p>Counted rather than named, so a form added later is covered by this test without anyone
|
||||||
|
* remembering to extend it.
|
||||||
|
*/
|
||||||
|
@Test
|
||||||
|
void everyAdminFormCarriesACsrfToken() throws Exception {
|
||||||
|
for (String page : new String[] {"/admin", "/admin/catering"}) {
|
||||||
|
String html = mvc.perform(get(page).with(user("morissa")))
|
||||||
|
.andExpect(status().isOk())
|
||||||
|
.andReturn().getResponse().getContentAsString();
|
||||||
|
|
||||||
|
long forms = html.split("<form", -1).length - 1;
|
||||||
|
long tokens = html.split("name=\"_csrf\"", -1).length - 1;
|
||||||
|
assertThat(forms).as("%s should have forms to check", page).isPositive();
|
||||||
|
assertThat(tokens).as("%s: every one of its %d forms needs a token", page, forms)
|
||||||
|
.isEqualTo(forms);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
@Test
|
@Test
|
||||||
void theCatalogueScreenShowsWhatIsOnThePageWithItsPhotos() throws Exception {
|
void theCatalogueScreenShowsWhatIsOnThePageWithItsPhotos() throws Exception {
|
||||||
mvc.perform(get("/admin").with(user("morissa")))
|
mvc.perform(get("/admin").with(user("morissa")))
|
||||||
|
|||||||
Reference in New Issue
Block a user