This is the same {@code problem} flash the refusals above use, so the editor reads it in the + * same place on the same page. Before it existed, an over-sized file escaped as the container's own + * parsing error: a 500, and then a second failure forwarding to {@code /error}, because that forward + * re-parsed the same too-large request. Reaching this handler at all depends on {@code + * spring.servlet.multipart.resolve-lazily} — parsed eagerly, the throw happens before any handler + * method is chosen and there is nothing here to catch it. + * + *
The flash map is written directly rather than through {@code RedirectAttributes}, which is not + * an argument Spring supplies to an {@code @ExceptionHandler}. + */ + @ExceptionHandler(MaxUploadSizeExceededException.class) + public String photoTooBig(HttpServletRequest request) { + RequestContextUtils.getOutputFlashMap(request).put("problem", + "That photo is too large. Anything up to 15 MB is fine — a photo straight off a phone " + + "normally is — and we resize it here, so there is no need to shrink it first."); + return "redirect:/admin"; + } } diff --git a/src/main/resources/application.yaml b/src/main/resources/application.yaml index 3585f49..b5c514d 100644 --- a/src/main/resources/application.yaml +++ b/src/main/resources/application.yaml @@ -11,6 +11,23 @@ spring: open-in-view: false flyway: enabled: true + servlet: + multipart: + # THIS BLOCK IS THE WHOLE REASON PHOTO UPLOADS FAILED. Unset, Boot defaults to a 1 MB max-file-size, + # and a photo off a phone is 3-12 MB — so every real upload died with FileSizeLimitExceededException + # before it reached ProductPhotoService, which exists precisely to resize "whatever came off a phone". + # The resizing pipeline could never run on the input it was written for. + max-file-size: 15MB + # The file input is `multiple`, so one submit can carry several photos; this bounds the whole request + # rather than each part. Four full-size photos at once is a realistic morning's worth of new stock. + max-request-size: 60MB + # Parse when the controller asks for the files, not while Tomcat is reading parameters. Eagerly, an + # oversize part throws from inside the container's parameter parsing, which no @ExceptionHandler can + # reach — the request dies as a 500 and then the forward to /error re-parses and throws again (the + # "Exception Processing [ErrorPage...]" pairs in the log). Lazily, it surfaces as a + # MaxUploadSizeExceededException during argument binding, where AdminController can catch it. + resolve-lazily: true + mail: host: ${SMTP_SERVER:localhost} port: ${SMTP_PORT:25} @@ -29,6 +46,13 @@ spring: ssl: trust: ${SMTP_SERVER:localhost} +server: + tomcat: + # Read and discard the rest of an over-sized body instead of resetting the connection, so the browser + # actually receives the redirect and the message rather than "connection reset". Only reachable now for + # a genuinely enormous file, but that is exactly when a clear answer matters. + max-swallow-size: -1 + platform: web: spa: diff --git a/src/main/resources/static/apple-touch-icon.png b/src/main/resources/static/apple-touch-icon.png new file mode 100644 index 0000000..157e788 Binary files /dev/null and b/src/main/resources/static/apple-touch-icon.png differ diff --git a/src/main/resources/static/favicon.ico b/src/main/resources/static/favicon.ico new file mode 100644 index 0000000..3bbd8ba Binary files /dev/null and b/src/main/resources/static/favicon.ico differ diff --git a/src/main/resources/static/favicon.svg b/src/main/resources/static/favicon.svg new file mode 100644 index 0000000..be44651 --- /dev/null +++ b/src/main/resources/static/favicon.svg @@ -0,0 +1,37 @@ + diff --git a/src/main/resources/templates/fragments/head.html b/src/main/resources/templates/fragments/head.html index e8c5924..f2851c6 100644 --- a/src/main/resources/templates/fragments/head.html +++ b/src/main/resources/templates/fragments/head.html @@ -13,8 +13,19 @@ - - + + + + diff --git a/src/test/java/com/itsthevine/web/AdminPagesTest.java b/src/test/java/com/itsthevine/web/AdminPagesTest.java index 066072c..67ceaa2 100644 --- a/src/test/java/com/itsthevine/web/AdminPagesTest.java +++ b/src/test/java/com/itsthevine/web/AdminPagesTest.java @@ -1,5 +1,6 @@ package com.itsthevine.web; +import static org.assertj.core.api.Assertions.assertThat; import static org.hamcrest.Matchers.containsString; import static org.springframework.security.test.web.servlet.request.SecurityMockMvcRequestPostProcessors.csrf; import static org.springframework.security.test.web.servlet.request.SecurityMockMvcRequestPostProcessors.user; @@ -13,7 +14,9 @@ import static org.springframework.test.web.servlet.result.MockMvcResultMatchers. import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.servlet.autoconfigure.MultipartProperties; import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.util.unit.DataSize; import org.springframework.boot.testcontainers.service.connection.ServiceConnection; import org.springframework.security.test.web.servlet.setup.SecurityMockMvcConfigurers; import org.springframework.test.web.servlet.MockMvc; @@ -79,6 +82,25 @@ class AdminPagesTest { .build(); } + /** + * The bug this guards was an ABSENCE: nothing configured multipart, so Boot's 1 MB default applied and + * every photo off a phone was rejected by the container before {@code ProductPhotoService} — the class + * whose whole job is resizing phone photos — could see it. A default is exactly the kind of thing that + * comes back silently, so the numbers are asserted rather than trusted. + */ + @Test + void photosOffAPhoneFitInsideTheUploadLimits() { + MultipartProperties multipart = context.getBean(MultipartProperties.class); + + assertThat(multipart.getMaxFileSize()).isEqualTo(DataSize.ofMegabytes(15)); + assertThat(multipart.getMaxRequestSize()).isEqualTo(DataSize.ofMegabytes(60)); + // Without this the throw happens inside the container's parameter parsing, where no + // @ExceptionHandler can reach it — which is what made an over-sized photo a 500. + assertThat(multipart.isResolveLazily()).isTrue(); + // The default is 1 MB. If this ever passes, the fix has been undone. + assertThat(multipart.getMaxFileSize()).isNotEqualTo(DataSize.ofMegabytes(1)); + } + @Test void theCatalogueScreenShowsWhatIsOnThePageWithItsPhotos() throws Exception { mvc.perform(get("/admin").with(user("morissa"))) diff --git a/src/test/java/com/itsthevine/web/AdminUploadRefusalTest.java b/src/test/java/com/itsthevine/web/AdminUploadRefusalTest.java new file mode 100644 index 0000000..1375712 --- /dev/null +++ b/src/test/java/com/itsthevine/web/AdminUploadRefusalTest.java @@ -0,0 +1,56 @@ +package com.itsthevine.web; + +import static org.hamcrest.Matchers.containsString; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.mock; +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.multipart; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.flash; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.redirectedUrl; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; + +import org.junit.jupiter.api.Test; +import org.springframework.mock.web.MockMultipartFile; +import org.springframework.test.web.servlet.MockMvc; +import org.springframework.test.web.servlet.setup.MockMvcBuilders; +import org.springframework.web.multipart.MaxUploadSizeExceededException; + +/** + * A photo too big for the limit comes back as a sentence, not a stack trace. + * + *
Photo uploads used to fail at 1 MB — no multipart limits were configured, so Boot's default applied + * (see {@code AdminPagesTest#photosOffAPhoneFitInsideTheUploadLimits}, which pins the numbers). Raising + * them fixes the everyday case; this covers what the editor sees when a file really is too large, because + * what happened before was a 500 followed by a second failure forwarding to {@code /error} — that forward + * re-parsed the same over-sized request and threw again. + * + *
Standalone rather than a booted context, and the throw is staged from the service rather than from a + * genuinely huge upload, because MockMvc does not enforce the container's multipart limits — there is no + * way to provoke the real parse failure here. What that leaves worth asserting is the wiring: that the + * handler catches this exception type, writes the same {@code problem} flash the other refusals use, and + * redirects instead of rendering an error page. Whether the exception can reach a handler at all is a + * property of {@code resolve-lazily}, which is asserted separately. + */ +class AdminUploadRefusalTest { + + @Test + void anOversizePhotoIsRefusedOnThePageRatherThanAsAStackTrace() throws Exception { + Catalogue catalogue = mock(Catalogue.class); + doThrow(new MaxUploadSizeExceededException(15_728_640L)) + .when(catalogue).addPhotos(eq(7L), any()); + + MockMvc mvc = MockMvcBuilders.standaloneSetup(new AdminController(catalogue)).build(); + + mvc.perform(multipart("/admin/items/7/photos") + .file(new MockMultipartFile("photos", "cake.jpg", "image/jpeg", new byte[] {1, 2, 3}))) + .andExpect(status().is3xxRedirection()) + .andExpect(redirectedUrl("/admin")) + // The same flash key the domain's own refusals use, so it lands in the same place on the + // page, and it names the limit rather than saying "invalid". + .andExpect(flash().attribute("problem", containsString("too large"))) + .andExpect(flash().attribute("problem", containsString("15 MB"))) + // It should not tell the baker to go and shrink the photo: resizing is this app's job. + .andExpect(flash().attribute("problem", containsString("resize it here"))); + } +} diff --git a/src/test/java/com/itsthevine/web/SiteControllerTest.java b/src/test/java/com/itsthevine/web/SiteControllerTest.java index 1ed50bd..7296b5d 100644 --- a/src/test/java/com/itsthevine/web/SiteControllerTest.java +++ b/src/test/java/com/itsthevine/web/SiteControllerTest.java @@ -60,6 +60,34 @@ class SiteControllerTest { mvc = MockMvcBuilders.webAppContextSetup(context).build(); } + /** + * The tab icon used to be the 1000x1000 logo PNG, which is 71 KB fetched to paint 16 pixels and, being + * a hairline drawing, arrived as a grey smudge at that size. The order matters and is the reason this + * asserts position: a browser takes the LAST icon format it understands, so the .ico has to come first + * or Chrome settles for the bitmap instead of the SVG. + */ + @Test + void theTabIconIsAnIconRatherThanTheFullLogo() throws Exception { + String head = mvc.perform(get("/")).andExpect(status().isOk()) + .andReturn().getResponse().getContentAsString(); + + assertThat(head).contains(""); + assertThat(head).contains(""); + assertThat(head).contains(""); + assertThat(head.indexOf("/favicon.ico")).isLessThan(head.indexOf("/favicon.svg")); + // The 1000x1000 logos are no longer offered as icons anywhere. + assertThat(head).doesNotContain("rel=\"icon\" media="); + assertThat(head).doesNotContain("logo_dark.png"); + } + + @Test + void theIconFilesAreActuallyServed() throws Exception { + // A link to a 404 is worse than no link: the browser shows its default and caches the miss. + for (String icon : new String[] {"/favicon.svg", "/favicon.ico", "/apple-touch-icon.png"}) { + mvc.perform(get(icon)).andExpect(status().isOk()); + } + } + @Test void everyPageStatesItsOwnTitleAndDescription() throws Exception { // One generic shell for every page was the SPA's problem, and the reason a controller used to