Cannot edit uploaded image when hosting Payload CMS behind a reverse proxy

[felskov]

Link to reproduction

No response

Describe the Bug

The issue occurs because we're running the application behind a reverse proxy (nginx), which causes the getExternalFile fetch request to stall and time out. This happens due to bad forwarding of headers here:

const res = await fetch(fileURL, {
  credentials: 'include',
  headers: {
    ...req.headers,
  },
  method: 'GET',
})

When we dug into the issue, we realized that this caused you to forward all headers from the incoming request to edit the image. That request is a POST-request, which includes a request body. Therefore that request also contained a "Content-Length" header. But obviously we don't actually send a request body with the above GET-request, and therefore nginx never served the request (it kept waiting for a request body that never came).

I think you might want to be more explicit about which headers to forward (seemingly only cookies are relevant here?), so as to avoid causing issues with applications behind load balancers / reverse proxies / other?

To Reproduce

1. Host Payload CMS behind a nginx reverse proxy
2. Upload an image
3. Try to edit the image

Payload Version

2.11.1

Adapters and Plugins

@payloadcms/plugin-cloud-storage/s3

[swytch]

Can confirm that this also affects uploads to S3 without the reverse proxy. It pops up as a different error:

FetchError: request to https://prod-public.s3.ap-southeast-2.amazonaws.com/media/image.jpg failed, reason: Hostname/IP does not match certificate's altnames: Host: localhost. is not in the cert's altnames: DNS:s3-ap-southeast-2.amazonaws.com, ...snip

Removing the headers block entirely from the fetch request lets us crop, and fixes the issue.

This might need to be configurable in the cases where you're relying on payload to provide access control, but if you have disablePayloadAccessControl: true, requests are made directly to a public S3 bucket both out of payload and your frontend.

In might be just enough to rely on the disablePayloadAccessControl config setting in this case?

[HarleySalas]

I'm also having this issue, hosting as a docker container with traefik

[denolfe]

@felskov Are you able to provide a simplified version of your nginx config, so I can try and recreate?

I'm not against offering additional configuration on the forwarded headers.

[felskov]

@denolfe Sure, it looks something like this:

server {
	server_name _;
	listen 80;

	client_max_body_size 10M;

	gzip		on;
	gzip_static	on;
	gzip_vary	on;
	gzip_proxied	any;
	gzip_types	text/plain text/css application/json application/javascript application/x-javascript text/javascript text/xml application/xml application/rss+xml application/atom+xml application/rdf+xml;

	location / {
		proxy_pass		http://127.0.0.1:8080; # or whatever port payload is listening for internally
		proxy_set_header	Upgrade $http_upgrade;
		proxy_set_header	Host $http_host;
		proxy_http_version	1.1;
		proxy_set_header	X-Forwarded-For $proxy_add_x_forwarded_for;
	}
}

Note however that we're using certbot for SSL termination on the proxy as well, I've not included that here :)

I know it's none of my business, but personally I would start from looking into a more selective forwarding of headers. It seems to me to carry lots of potential issues that you forward all headers "as is". I'm assuming it's to preserve authorization, but why not select the actually relevant headers and forward only those? Potentially wrapped in an "authorizedFetch" if this is something that's done in many places throughout the implementation.

[glettl]

this also happens for local files.

In this case, getExternalFiles should never be called.

try {
            if (url && url.startsWith('/') && !disableLocalStorage) {
                const filePath = `${staticPath}/${filename}`;
                const response = await (0, _getFileByPath.default)(filePath);
                file = response;
                overwriteExistingFiles = true;
            } else if (filename && url) {
                file = await (0, _getExternalFile.getExternalFile)({
                    req,
                    data: data
                });
                overwriteExistingFiles = true;
            }
        } catch (err) {
            throw new _errors.FileUploadError(req.t);
        }

url.startsWith('/') check is always false, as also local-file urls start with https.

I created a patch that checks if the url starts with serverURL and use the local file if thats true.

        const isLocalFile = url && url.startsWith(config.serverUrl);
        try {
            if (url && isLocalFile && !disableLocalStorage) {
                const filePath = `${staticPath}/${filename}`;
                const response = await (0, _getFileByPath.default)(filePath);
                file = response;
                overwriteExistingFiles = true;
            } else if (filename && url) {
                file = await (0, _getExternalFile.getExternalFile)({
                    req,
                    data: data
                });
                overwriteExistingFiles = true;
            }
        } catch (err) {
            throw new _errors.FileUploadError(req.t);
        }

Not sure if this covers all cases, but works for us

[prowsejeremy]

Just chiming in on this one as it is also an issue in my implementation. As @swytch stated here removing the following headers resolves this issue for me:

const getExternalFile = async ({ data, req })=>{
        ...
        const res = await fetch(fileURL, {
            credentials: 'include',
            // headers: {
            //     ...req.headers
            // },
            method: 'GET'
        });
        ...
};

Unsure of what is being sent in these headers to cause an issue in the first place.

Here is the output of the req headers and response for a "failed" and "successful" response respectively:

Note that the headers in the following output are NOT being included in the request.

[felskov]

@denolfe Any plans on handling this issue? It's very cumbersome for us that we effectively need to re-upload images to change focal point on them :( Is there anything we can do to help speed up a potential fix?

[prowsejeremy]

@denolfe following up on this one. I think I've tracked down the issue. When logging out the headers being passed to the payload/src/uploads/getExternalFile.ts > getExternalFile (line 11) function I can see that the "host" value for the external file is set to the site url where it should be set to the external hosting provider for the media resource, something like this:

req.headers = {
   host: 'my-bucket.s3.amazon.com', // currently set to 'my-payload-url.tld'
   ...
}

I would imagine this value should be fetched from the configuration object for the cloud storage plugin - or from some other global setting if this function is used elsewhere.

As another example, setting up an env variable of S3_HOSTNAME and updating the fetch request in the above function to:

    ...

    const res = await fetch(fileURL, {
        credentials: 'include',
        headers: {
            ...req.headers,
            host: process.env.S3_HOSTNAME
        },
        method: 'GET'
    })

    ...

This results in the request completing successfully in my testing, however I can't imagine setting a separate env variable as such is the best way to approach this.

It'd be great to get this addressed asap, seems to have been sitting for some time now and is a breaking issue for external media items.

[denolfe]

I will take another look a this one

[felskov]

@denolfe I still stand by my initial thoughts, that it seems like a code smell to unconditionally forward incoming request headers. I think the comment thread proves that it opens you up to a myriad of potential edge cases depending on the tech stack used to host Payload CMS.

My suggestion still stands to identify and conditionally forward only relevant headers (seemingly only cookies for authorization). Personally I wouldn't even forward incoming cookies for authz, but rather issue a signed token specifically for the internal request being made - that way internal processes aren't dependent on impersonating end-users.

In the meanwhile for anybody interested, we've been able to successfully use patch-package to resolve the issue on our end, with the following patch against v2.11.1:

diff --git a/node_modules/payload/dist/uploads/getExternalFile.js b/node_modules/payload/dist/uploads/getExternalFile.js
index 9ade232..641adfb 100644
--- a/node_modules/payload/dist/uploads/getExternalFile.js
+++ b/node_modules/payload/dist/uploads/getExternalFile.js
@@ -62,7 +62,7 @@ const getExternalFile = async ({ data, req })=>{
         const res = await fetch(fileURL, {
             credentials: 'include',
             headers: {
-                ...req.headers
+                "cookies": req.headers["cookies"]
             },
             method: 'GET'
         });

[denolfe]

I'm leaning toward adding an additional configurable property to the plugin called headerFilter or similar that accepts the req and would return headers. This function would be used if defined, otherwise fall back to the current functionality.

Look at this PR, it looks like previously it was sending no headers and was changed to send all headers.

Thoughts? @felskov @prowsejeremy

[felskov]

It would work for my use case.

However, I still stand by what I said in thinking you should generally refrain from unconditional forwarding of headers. Your fix will allow me to debug the method for my specific stack, but it's going to impact people in non-obvious ways, and if they're not aware of the possibility that issues stem from unwanted headers being forwarded, chances are that it will be hard to figure out how to solve them.

In our use case, the request stalled and eventually timed out. It really wasn't easy to draw the conclusion that the issue was caused by request headers, we spent a lot of time looking into the source code and trying out potential fixes, so we would still have wasted a lot of valuable time debugging the issue, even with a headerFilter option.

In the end it's your call, but from everything I've seen, I'd still go with our proposed solution of forwarding only cookies header as per the patch posted above. It would effectively solve all issues reported on this ticket as far as I can tell, and it wouldn't require user/developer involvement/configuration to begin with.

[prowsejeremy]

@denolfe My only other thoughts were to trace back the initial request being passed to getExternalFile and ensure the headers that are being passed down the line are accurate and relevant to the request being made. The initial headers set for the update operation appear to be the ones passed down the line to getExternalFile and I struggle to see how they would be relevant to this request.

All that to say I'm inclined to agree with @felskov on this one. I'm unsure on the reasoning behind forwarding the headers in these requests, however if they are needed for the likes of security then obviously those relevant portions of the header object should be forwarded. Noting that this function is tied into both the create and update collection operations. From what I have tested there is no issue with either operation when these headers are removed or modified as mentioned.

[castlemilk]

@denolfe , I've tried your latest merge PR and now get error:

TypeError: headers.forEach is not a function
    at uploadConfig (/Users/benebsworth/projects/shorted/cms/node_modules/payload/src/uploads/getExternalFile.ts:59:5)
    at getExternalFile
...

Is the error in getExternalFile.ts, where it should be say:

function headersToObject(headers) {
  const headersObj = {}
  Object.entries(headers).forEach(([key, value]) => {
    // If the header value is an array, join its elements into a single string
    if (Array.isArray(value)) {
      headersObj[key] = value.join(', ')
    } else {
      headersObj[key] = value
    }
  })
  return headersObj
}

With this patch locally applied I now have GCS uploads working with edits 🥳 🙏

[denolfe]

@castlemilk Some fixes were pushed. Please upgrade to 2.14.2 and let me know if you still see this issue.

[jnicewander]

Upgrading Payload to 2.14.2 appears to have fixed it for us on our AWS Elastic Beanstalk deployments.

[github-actions[bot]]

This issue has been automatically locked. Please open a new issue if this issue persists with any additional detail.