Properly return 404 when the file does not exist #966

LordGaav · 2021-11-09T18:45:18Z

LordGaav commented

2021-11-09 18:45:18 +00:00

Nginx does not properly return a 404 code when a file does not exist, causing the service worker and/or browser caching to get confused at times. See my comment on this issue for details:

vikunja/api#966 (comment)

Nginx does not properly return a 404 code when a file does not exist, causing the service worker and/or browser caching to get confused at times. See my comment on this issue for details: https://kolaente.dev/vikunja/api/issues/966#issuecomment-18671

LordGaav added 1 commit 2021-11-09 18:45:18 +00:00

continuous-integration/drone/pr Build is passing Details

f18d63f76d Properly return 404 when the file does not exist.

konrad requested changes 2021-11-09 23:02:39 +00:00

nginx.conf

						
				@ -72,3 +72,3 @@

				        location / {

				            root   /usr/share/nginx/html;

				            try_files $uri $uri/ /;

				            try_files $uri $uri/ =404;

konrad commented

2021-11-09 23:02:35 +00:00

Unfortunately this breaks accessing something like /login (or password reset routes, share links etc) directly without going through /.

Unfortunately this breaks accessing something like `/login` (or password reset routes, share links etc) directly without going through `/`.

LordGaav commented

2021-11-10 20:57:37 +00:00

I see, you're right. Unfortunately, when serving with Nginx, the problem that I mentioned in the opening exists (serving 200 OK when actually it should be 404 Not Found).

Is there a way to generate static HTML files for the routes that we need?

I see, you're right. Unfortunately, when serving with Nginx, the problem that I mentioned in the opening exists (serving 200 OK when actually it should be 404 Not Found). Is there a way to generate static HTML files for the routes that we need?

konrad commented

2021-11-10 20:59:57 +00:00

Is there a way to generate static HTML files for the routes that we need?

Maybe, kind of. You could theoretically parse route definitions at router/index.js to figure out a regex map of all routes that are actually required and then put that in the nginx config. But then we might as well think about SSR...

> Is there a way to generate static HTML files for the routes that we need? Maybe, kind of. You could theoretically parse route definitions at `router/index.js` to figure out a regex map of all routes that are actually required and then put that in the nginx config. But then we might as well think about [SSR](https://vitejs.dev/guide/ssr.html)...

LordGaav commented

2021-11-10 21:13:13 +00:00

Maybe we're going about this wrong; we want to avoid returning 200 OK on .js files because that breaks the service worker, any other route doesn't really matter.

konrad commented

2021-11-10 21:19:01 +00:00

mhhhhh nginx can do regex, I'm sure we could modify the location block to do the try_files only work on files that are not ending on .js. Then add another one to keep serving .js files.

mhhhhh nginx can do regex, I'm sure we could modify the `location` block to do the `try_files` only work on files that are not ending on `.js`. Then add another one to keep serving `.js` files.

LordGaav commented

2021-11-10 21:33:45 +00:00

Indeed, see my latest commit. I applied this to my own installation, and everything still seems to work.

konrad commented

2021-11-10 21:34:46 +00:00

Looks good.

dpschen commented

2021-11-10 21:52:15 +00:00

Seems error-prone to list the files individually. E.g. mp3 is missing.

Seems error-prone to list the files individually. E.g. `mp3` is missing.

LordGaav commented

2021-11-10 22:26:13 +00:00

I did not actually find any mp3 files in the deployed Docker container, this list is all the extensions that I did find.

konrad commented

2021-11-10 22:29:09 +00:00

@dpschen do you know a way to generate that list?

dpschen commented

2021-11-10 23:42:18 +00:00

I know that it's possible to prerender which also generates the routes that are static: https://vite-plugin-ssr.com/pre-rendering
Not sure if that helps though.

Regarding the list: I found this after a built in the dist folder using this:

css
html
ico
jpg
js
map
mjs
mp3
png
svg
txt
webmanifest
woff2

I know that it's possible to prerender which also generates the routes that are static: https://vite-plugin-ssr.com/pre-rendering Not sure if that helps though. Regarding the list: I found this after a built in the dist folder using [this](https://stackoverflow.com/a/1842270/15522256): ``` css html ico jpg js map mjs mp3 png svg txt webmanifest woff2 ```

LordGaav added 1 commit 2021-11-10 21:33:12 +00:00

continuous-integration/drone/pr Build is passing Details

71ec28b590 Route all static files except HTML directly to disk or return 404

konrad approved these changes 2021-11-10 21:37:47 +00:00

konrad left a comment

Seems to work, will merge once the CI passes.

konrad merged commit 052cd36085 into main

2021-11-10 21:38:08 +00:00

konrad referenced this issue from a commit

2021-11-10 21:38:08 +00:00

feat: properly return 404 when the file does not exist (#966)

konrad deleted branch feature/do-not-return-200-on-every-request