statamic / ssg

The official Statamic Static Site Generator
230 stars 23 forks source link

TrustedProxies causes issue generating pages with custom routes #40

Closed tao closed 11 months ago

tao commented 3 years ago

I encounter an error with SSG when generating custom pages defined in the routes files, the issue is caused by TrustedProxies middleware in Laravel.

On my site I have a custom view generated for an index for each year.

Route::statamic('articles/{year}', 'articles.years.show', [
    'layout' => 'default',
])->where('year', '[0-9]+');

In my SSG config, I loop through the article years from the start date to the current year and manually include these pages to be generated:

<?php

for ($year = 1972; $year <= date("Y"); $year ++) {
    $yearIndexes[] = "/articles/{$year}";
}

return [

    ...

    /*
    |--------------------------------------------------------------------------
    | Additional URLs
    |--------------------------------------------------------------------------
    |
    | Here you may define a list of additional URLs to be generated,
    | such as manually created routes.
    |
    */

    'urls' => array_merge(
        [
            // other urls to include
        ],
        $yearIndexes,
    ),

When I generate the static website, I encounter this error:

[✔] /articles
Generating /articles/1972...

   TypeError

  Argument 2 passed to Symfony\Component\HttpFoundation\IpUtils::checkIp4() must be of the type string, null g
iven, called in /Users/tao/Code/www.statamic/vendor/symfony/http-foundation/IpUtils.php on line 46

  at vendor/symfony/http-foundation/IpUtils.php:62
     58▕      * @param string $ip IPv4 address or subnet in CIDR notation
     59▕      *
     60▕      * @return bool Whether the request IP matches the IP, or whether the request IP is within the CI
DR subnet
     61▕      */
  ➜  62▕     public static function checkIp4(?string $requestIp, string $ip)
     63▕     {
     64▕         $cacheKey = $requestIp.'-'.$ip;
     65▕         if (isset(self::$checkedIps[$cacheKey])) {
     66▕             return self::$checkedIps[$cacheKey];

      +51 vendor frames
  52  please:37
      Illuminate\Foundation\Console\Kernel::handle(Object(Symfony\Component\Console\Input\ArgvInput), Object(S
ymfony\Component\Console\Output\ConsoleOutput))

I can overcome this error by commenting out the Laravel middleware... but this isn't ideal and wondering if there's a more elegant way to solve this in the SSG code.

For reference this is my TrustedProxies.php file:

class TrustProxies extends Middleware
{
    /**
     * The trusted proxies for this application.
     *
     * @var array
     */
    protected $proxies = '*';

    /**
     * The headers that should be used to detect proxies.
     *
     * @var string
     */
    protected $headers = Request::HEADER_X_FORWARDED_ALL;
}
tao commented 3 years ago

I've created an example repo with this issue.

protected $proxies;

It works fine until you change this line to this:

protected $proxies = '*';
jasonvarga commented 3 years ago

Not saying this can't be fixed, but I'm curious what situation you're in that you need to use a static site and trusted proxies.

tao commented 3 years ago

It's been a couple months so I'm having trouble remembering exactly but it might have come up when I moved my SSG action to a github pipeline because it was slow on my personal computer? And then every night I generate the new pages that have changed in the last 24 hours and re-deploy it to S3.

There is a small chance it did occur on localhost but I see there's been a few merges to SSG this week so I'll review the updates this weekend and see if it's still causing an issue.

jasonvarga commented 3 years ago

I would have thought you'd need to tinker with trusted proxies if you're hosting on somewhere like AWS. If your site exists just to generate a static version, I wouldn't think it was necessary.

It definitely still breaks if I change to protected $proxies = '*'; but I'm wondering if I need to spend effort fixing it if you don't actually need to play with your trusted proxies.

tao commented 3 years ago

I took another look at this... I don't really understand what's causing this issue, especially since (from the example repo I created before) it seems to happen on localhost too. However, this trusted proxies issue is something that happens quite often on Laravel deployments on hosting like Heroku, so it's something I'm sure most developers are familiar with. When I tested it again it generates the / home page but fails on the second /404 page 😕

Maybe it's related somehow to the way SSG does a request for the pages that creates the issue?

I think for now we can probably close this issue and open it up again in the future if it causes an issue for anyone else.

jasonvarga commented 3 years ago

I guess what I'm saying is that if you're hosting your non-static site on Heroku and need to use trusted proxies, sure, that's fine. But then are you also generating the static site on Heroku?

Wouldn't the static version of the site be production?

Is the non-static site you have on Heroku your staging site?

Maybe it's related somehow to the way SSG does a request

It's definitely this. We fake certain parts of the request as if you were requesting the page being generated. There's probably a couple more parts of the request we need to fudge in order for the trusted proxies stuff to work.

tao commented 3 years ago

The static site is our production site and it's hosted as files on CloudFront. We generating it with SSG each night on a GitHub pipeline. However, the bug occurs (with this example repo) on my local machine and I'm just using MacOS & Valet.

I'll be playing with SSG a lot more soon as we're close to launching the site so I'll continue testing (without changing my trusted proxies) and see if I can narrow down the cause of the issue.

abuhurairah-aar commented 2 years ago

Hi @tao , are you able to find a workaround for this?

tao commented 2 years ago

@abuhurairah-aar I haven't had any recent issues with this but I'm not using Heroku anymore either.

I am using $proxies; instead of $proxies = '*'; in my code.

abuhurairah-aar commented 2 years ago

@tao Thanks for the feedback. I've solved the issue by reverted back my $proxies = '**'; to $proxies; like you mentioned, but since my application behind a load balancer, I still need to use $proxies='**' so that I able to configure https properly. Then, whenever I want to run php please ssg:generate i will revert to $proxies; manually. It is cumbersome, but I guess this is the solution for now. Hopefully there's a way to dynamically setup the $proxies through .env file, but I got no luck trying that out yet.

jonassiewertsen commented 1 year ago

I can confirm this behaviour.

jonassiewertsen commented 1 year ago

The request passed in here does have a null value, which does resolve in the shown error.

The code below does show a workaound.

https://github.com/statamic/ssg/compare/master...spiegeltechlab:ssg:trusted-proxy

@jasonvarga

  1. Do you see any security concerns?
  2. I think it should be configurable (ip and if activated at all)
  3. There might be a way which is more clever, but this is the fasted solution I could find.

@tao and @abuhurairah-aar does this workaround solve your issue as well?

After some feedback, I can create a PR of course.

phyroslam commented 1 year ago

I also faced the same issue yesterday and I can confirm @jonassiewertsen 's workaround solves the issue

o1y commented 1 year ago

Thanks @jonassiewertsen, your patch fixes the issue. Would be super happy about a PR which Jason can review then. :)

jonassiewertsen commented 1 year ago

After getting feedback to my questions (see above), I am happy to create a PR.

jegardiner commented 11 months ago

@jonassiewertsen Did this PR ever get created / merged? I can't see it anywhere and I'm still experiencing this problem.

I'm patching manually for now but have to reapply every time I upgrade.

o1y commented 11 months ago

I'm patching manually for now but have to reapply every time I upgrade.

This PR has not been merged yet. You could use composer-patches. This is what I'm doing to have this patch applied everytime I run composer updates.

jegardiner commented 11 months ago

I'm patching manually for now but have to reapply every time I upgrade.

This PR has not been merged yet. You could use composer-patches. This is what I'm doing to have this patch applied everytime I run composer updates.

Thanks. I will give that a try.

Hopefully the PR will be merged soon.

jonassiewertsen commented 11 months ago

No feedback from the core team yet, but I created that PR anyways. I hope it does help.

https://github.com/statamic/ssg/pull/145