Archived
Four admin forms had no CSRF token, so they 403'd in silence
"Add something new" could not create a product. It was not the upload limit raised in #14: the post never got as far as multipart parsing. Thymeleaf inserts the hidden _csrf input while it processes a th:action. Five forms were written with a plain action= instead, so Thymeleaf passed the tag through untouched and they posted with no token. Spring Security answers 403, Spring renders the site's own error page, and NOTHING is logged at ERROR -- which is why the app log was completely clean while the form was broken. From the outside it looked like the upload silently failed; the request was rejected before any of our code ran. The four that mattered: add an item, add a category, add a catering table, edit the page notes. The fifth was Sign out in the admin layout, on every admin page, which had been failing the same way. Counted rather than named in the test -- /admin renders 89 forms and carried 86 tokens -- so a form added later is covered without anyone remembering to extend it. Two notes on the previous change while this is fresh. The 403 probe I used to "verify" the raised upload limit proved nothing: CSRF is checked before the body is parsed, which is exactly why a 5 KB and a 3 MB body both came back 403. And the @ExceptionHandler added in #14 only catches an over-sized file if Tomcat has not already thrown during parameter parsing, which it does when a filter reads a parameter first -- so it is narrower than its comment claims. Left alone here rather than widened on a guess; the size path needs a real over-sized upload to characterise.
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