laravel / valet

A more enjoyable local development experience for Mac.
https://laravel.com/docs/valet
MIT License
2.52k stars 698 forks source link

404 for files in public/ (LaravelValetDriver) #375

Closed skollro closed 7 years ago

skollro commented 7 years ago

I always get a status code 404 for existing files which are directly located under public/ on a fresh install (favicon.ico, robots.txt), but the files are delivered. If I name them differently, it's working with a 200. Files in subdirectories css/, js/ are served properly with a status code 200.

Edit: I figured out that the problem is located in the Nginx config. If I comment out these lines, favicon.ico and robots.txt are served.

location = /favicon.ico { access_log off; log_not_found off; }
location = /robots.txt  { access_log off; log_not_found off; }
simensen commented 7 years ago

I just spent half an hour trying to figure out what was wrong with my favicon for my new spark project served behind valet. I'd be happy to figure out how to patch this if someone could point me in the right direction.

adamwathan commented 7 years ago

I feel like I fixed this on the master branch a few months back, are you running dev-master or latest tag? On Tue, Sep 12, 2017 at 6:08 PM Beau Simensen notifications@github.com wrote:

I just spent half an hour trying to figure out what was wrong with my favicon for my new spark project served behind valet. I'd be happy to figure out how to patch this if someone could point me in the right direction.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/laravel/valet/issues/375#issuecomment-328998407, or mute the thread https://github.com/notifications/unsubscribe-auth/AEH3bFK6c7UK0fCCYvDX-9Uf47iBz2O1ks5shwDLgaJpZM4NYQX5 .

simensen commented 7 years ago

hm. let me see. it looks like i was one version back. the problem i'm having might be slightly different. commenting out the lines mentioned above didn't fix my problem. my problem is that getting /favicon.ico returns the actual image but gives status 404. so if i visit /favicon.ico in the browser directly, it renders just fine.

adamwathan commented 7 years ago

Yeah that should be fixed, but if you are using local SSL on that site you'll need to resecure that site to generate an updated nginx config.

adamwathan commented 7 years ago

Quick trick to regenerate configs for any SSL sites:

valet domain blah
valet domain dev
simensen commented 7 years ago

cool, thanks. i'm not doing local ssl. or rather, wasn't. just tried "valet secure mudskipper-dev" and it worked like a charm. loving valet even more now. :) thanks for your help!

also, for the record, i did an update from valet v2.0.4 to v2.0.5 and it all looks good now.

adamwathan commented 7 years ago

Sweet!

nickdekruijk commented 4 years ago

I'm still having this issue. Just upgraded to v2.8.1 and still favicon.ico and my https://project.test/ is giving a 404 error on favicon, but opening https://project.test/favicon.ico directly does seem to work.

stidges commented 4 years ago

It seems the issue with favicon.ico / robots.txt was fixed in this commit (v2.0.5) but was later re-introduced in #819 (v2.4.1). To work around this I have removed the favicon.ico and robots.txt location blocks from my Nginx configuration.

sesolaga commented 4 years ago

I'm having the same issue.

mattstauffer commented 4 years ago

@stidges Great catch there. That's a fascinating issue... I'm curious how we can get rid of the error logs but also make it work

nickdekruijk commented 4 years ago

I'm not a nginx expert unfortunately but what happens if you only have location = /favicon.ico { access_log off } ? So only turning off access logging but not error log? All sites should have a working favicon these days anyway else they will see 404 errors when the site goes live.

alexmartinfr commented 4 years ago

Also stumbled on this issue today, here is my workaround for now:

Nginx is looking for a favicon.ico file at the root level. Instead, for Laravel projects, it should look in the public folder.

Just add /public on the files paths inside your config file, and run valet restart:

location = /public/favicon.ico { access_log off; log_not_found off; }
location = /public/robots.txt  { access_log off; log_not_found off; }

If you have run valet secure, you'll find your app's config file here: ~/.config/valet/nginx/your_app.test

Otherwise, the default valet nginx config file is located here: /usr/local/etc/nginx/valet/valet.conf

It might make more sense to edit it on a project basis, in case you don't only serve Laravel apps with Valet.

snapey commented 3 years ago

@AlexMartinFR solution worked for me but only after I found a copy of valet.conf in /opt/homebrew/etc/nginx/valet

I guess it depends how you installed

zatorck commented 1 year ago

Hello All.

Nice that I found this thread. I got very similar strange problem only for files located in root of public Symfony project, because it's not working for me on very legacy symfony 3.2 project but only if I do XMLHttpRequest or jQuery, but if I go to link directly in Chrome it works (by clicking on "open in new tab").

euoia commented 1 year ago

I just wanted to comment that this still seems to be an issue as of 3.2.2.

To fix it, I modified ~/.config/valet/nginx/my-site.test and changed:

    location = /favicon.ico { access_log off; log_not_found off; }

to

    location = /public/favicon.ico { access_log off; log_not_found off; }
shengslogar commented 1 year ago

Still having this issue on Laravel Valet 3.3.2. Could we get this issue reopened?

I'm unclear to me what the underlying cause is here but commenting out the location = /favicon.ico line as described in previous comments works for me. It's baffling that the browser 404s while also loading the file at the same time (at least looking at both Chrome and Firefox consoles), and directly accessing the URL works.

Screenshot 2023-03-08 at 20 27 41@2x

simensen commented 1 year ago

Nginx is looking for a favicon.ico file at the root level. Instead, for Laravel projects, it should look in the public folder.

I'm not convinced this is correct. The location context, as I understand it, is already relative to a URL (location vs directory). I think that changing location = /favicon.ico to location = /public/favicon.ico only "works" because you're no longer specifying any special handling for favicon.ico. In other words, it's the same thing as removing the line entirely since you don't have anything in public/public/favicon.ico directory.


I'm not using valet a lot these days, but I still get notified about messages like this because I was dealing with it at one point. I did a quick google search and came up with the following:

https://stackoverflow.com/a/45562682

location = /favicon.ico {
  return 204;
  access_log     off;
  log_not_found  off;
}

It occurred to me that since location is sorta "virtual," what it's doing is slapping some additional metadata around the location that is in addition to the actual file on disk. I have no idea why nginx is both deciding based on this that it will return a 404 and then do the work to get the actual content from disk (wat?) but maybe it makes sense? If you are adding a location context maybe the assumption is that nginx should use the location context to derive most everything about that request from the location?

I wanted to see if there was a more complex return directive that could be aware of whether or not the file actually exists. The best I came up with was this:

https://github.com/vstoykovbg/nginx.conf-examples/blob/master/favicon.ico.md

location = /favicon.ico {
    try_files $uri =404;
    log_not_found off;
    access_log off;
}

This would look to see if the requested file exists and send a 404 only if it does not.


In short, it seems like we have a few options for how we could default this:

Assume everything is good to go and default to 200 or 204 or something

location = /favicon.ico {
  return 204;
  access_log     off;
  log_not_found  off;
}

Pros

Check to see if the file exists and throw a 404 if not

location = /favicon.ico {
  try_files $uri =404;
  access_log     off;
  log_not_found  off;
}

Pros


Honestly, I think the last one, even though a bit "heavier", is probably the behavior everyone expects? It's what I expected for sure. I'd expect that we're telling nginx that for robots.txt and favicon.ico, we know those get asked for a lot, we aren't sure they actually exist, and we don't want to see them in the access log, and we also don't care if they are not there so do not bother showing me an error log about it.

shengslogar commented 1 year ago

Great write up, @simensen (hello, fellow Wisconsinite!).

I agree – the "heavier" behavior you detailed at the end is what I expected. However, while I don't speak fluent nginx, I noticed Laravel Forge uses a very similar configuration, so perhaps something earlier in Valet's config is the culprit? For example, I'm not quite sure how these two blocks are supposed to interact:

location /some-valet-uuid-directory-path/ {
    internal;
    alias /;
    try_files $uri $uri/;
}

location / {
    rewrite ^ "/Users/sheng/.composer/vendor/laravel/valet/server.php" last;
}

Here's Forge's config for reference, which works just fine for favicons:

server {
    listen 443 ssl http2;
    listen [::]:443 ssl http2;
    server_name example.com;
    root /home/forge/example.com/public;

    # FORGE SSL (DO NOT REMOVE!)
    # (He did in fact, remove...)

    add_header X-Frame-Options "SAMEORIGIN";
    add_header X-XSS-Protection "1; mode=block";
    add_header X-Content-Type-Options "nosniff";

    index index.html index.htm index.php;

    charset utf-8;

    # FORGE CONFIG (DO NOT REMOVE!)
    include forge-conf/example.com/server/*;

    location / {
        try_files $uri $uri/ /index.php?$query_string;
    }

    location = /favicon.ico { access_log off; log_not_found off; }
    location = /robots.txt  { access_log off; log_not_found off; }

    access_log off;
    error_log  /var/log/nginx/example.com-error.log error;

    error_page 404 /index.php;

    location ~ \.php$ {
        fastcgi_split_path_info ^(.+\.php)(/.+)$;
        fastcgi_pass unix:/var/run/php/php7.4-fpm.sock;
        fastcgi_index index.php;
        include fastcgi_params;
    }

    location ~ /\.(?!well-known).* {
        deny all;
    }
}

EDIT Oh: just noticing Forge defaults its root to public/ while Valet does not 👀

simensen commented 1 year ago

@shengslogar I wonder if this is a simple order-of-operations issue? Can you try putting the unmodified favicon/robots directives before the root redirect/last directive?

location /some-valet-uuid-directory-path/ {
    internal;
    alias /;
    try_files $uri $uri/;
}

location = /favicon.ico { access_log off; log_not_found off; }
location = /robots.txt  { access_log off; log_not_found off; }

location / {
    rewrite ^ "/Users/sheng/.composer/vendor/laravel/valet/server.php" last;
}

Since that root-level rewrite has last at the end, I'm wondering if those locations are being applied after the fact?

It looks like the way serveStaticFile works (https://github.com/laravel/valet/blob/d1967bbd0c61e528e47a79ed45556e58c2e614cf/cli/Valet/Drivers/ValetDriver.php#L137) is by doing an internal redirect to include that /some-valet-uuid-directory-path prefix.

Maybe it won't matter, but might be worth a try? Would involve not changing the "standard" way to silence favicon and robots without other weird side effects like serving the file with a 404 or NOT serving a file and still having a 200/204 response which would likely confuse people for the same reason.

shengslogar commented 1 year ago

@simensen I think this should work, but only because the final location block would override the previous? Wait... but that doesn't make sense, since location /some-valet-uuid-directory-path/ { } is at the very top... Are they supposed to be merged together? Will try playing with this later today.

shengslogar commented 1 year ago

@simensen Moving those blocks above yields the same weird 404-but-directly-accessible behavior.

markjaquith commented 1 year ago

@shengslogar Nginx doesn't really care how you order the blocks... it has its own rules for matching priority: http://nginx.org/en/docs/http/ngx_http_core_module.html#location

Stetzon commented 10 months ago

for anyone using Laravel Herd (which uses Valet under the hood) the nginx config files are located in ~/Library/Application Support/Herd/config. The favicon lines in question are in herd.conf

NickSdot commented 6 months ago

For me it was simply removing the leading / in the relevant configs..

-location = /favicon.ico { access_log off; log_not_found off; }
+location = favicon.ico { access_log off; log_not_found off; }
-location = /robots.txt  { access_log off; log_not_found off; }
+location = robots.txt  { access_log off; log_not_found off; }