fix: add safety check to redirects from external file URL uploads (#15458)

### What
Fixes an `uploads` related security issue where redirects are able
bypass URL validation in external file uploads.
### Why
When uploading files via an external URL, HTTP redirects (3xx responses)
were automatically followed. This meant an potential attacker could
provide a public URL that redirected to internal services, bypassing the
security checks that only ran on the initial URL.
### How
- Added `redirect: 'manual'` to `safeFetch()` to prevent automatic
redirect following
- Modified `getExternalFile()` to manually handle redirects with
re-validation at each step
- Limited redirects to maximum of 3x to prevent infinite loops
- Tests added to `uploads` test suite
#### Acknowledgement
Big thank you to `r3dbrothers1` for the responsible disclosure.

Author: JessRynkar

packages/payload/src/uploads/getExternalFile.ts

@@ -37,38 +37,55 @@ export const getExternalFile = async ({ data, req, uploadConfig }: Args): Promis
 cookie: cookies.join(';'),
 }
 
-// Check if URL is allowed because of skipSafeFetch allowList
-constskipSafeFetch: boolean=
-uploadConfig.skipSafeFetch===true
- ? uploadConfig.skipSafeFetch
- : Array.isArray(uploadConfig.skipSafeFetch)&&
-isURLAllowed(fileURL,uploadConfig.skipSafeFetch)
-
-// Check if URL is allowed because of pasteURL allowList
-constisAllowedPasteUrl: boolean|undefined=
-uploadConfig.pasteURL&&
-uploadConfig.pasteURL.allowList&&
-isURLAllowed(fileURL,uploadConfig.pasteURL.allowList)
-
 letres
-if(skipSafeFetch||isAllowedPasteUrl){
-// Allowed
-res=awaitfetch(fileURL,{
-credentials: 'include',
- headers,
-method: 'GET',
-})
-}else{
-// Default
-res=awaitsafeFetch(fileURL,{
-credentials: 'include',
- headers,
-method: 'GET',
-})
+letredirectCount=0
+constmaxRedirects=3
+
+while(redirectCount<=maxRedirects){
+constskipSafeFetch: boolean=
+uploadConfig.skipSafeFetch===true
+ ? uploadConfig.skipSafeFetch
+ : Array.isArray(uploadConfig.skipSafeFetch)&&
+isURLAllowed(fileURL,uploadConfig.skipSafeFetch)
+
+constisAllowedPasteUrl: boolean|undefined=
+uploadConfig.pasteURL&&
+uploadConfig.pasteURL.allowList&&
+isURLAllowed(fileURL,uploadConfig.pasteURL.allowList)
+
+if(skipSafeFetch||isAllowedPasteUrl){
+res=awaitfetch(fileURL,{
+credentials: 'include',
+ headers,
+method: 'GET',
+redirect: 'manual',
+})
+}else{
+// Default
+res=awaitsafeFetch(fileURL,{
+credentials: 'include',
+ headers,
+method: 'GET',
+})
+}
+
+if(res.status>=300&&res.status<400){
+redirectCount++
+if(redirectCount>maxRedirects){
+thrownewAPIError(`Too many redirects (max ${maxRedirects})`,403)
+}
+constlocation=res.headers.get('location')
+if(location){
+fileURL=newURL(location,fileURL).toString()
+continue
+}
+}
+
+break
 }
 
-if(!res.ok){
-thrownewAPIError(`Failed to fetch file from ${fileURL}`,res.status)
+if(!res||!res.ok){
+thrownewAPIError(`Failed to fetch file from ${fileURL}`,res?.status)
 }
 
 constdata=awaitres.arrayBuffer()

packages/payload/src/uploads/safeFetch.ts

@@ -85,6 +85,7 @@ export const safeFetch = async (...args: Parameters<typeof undiciFetch>) => {
 returnawaitundiciFetch(url,{
  ...options,
 dispatcher: safeDispatcher,
+redirect: 'manual',// Prevent automatic redirects
 })
 }catch(error){
 if(errorinstanceofError){

test/uploads/int.spec.ts

@@ -1598,13 +1598,123 @@ describe('Collections - Uploads', () => {
 expect(arrayBuffer.byteLength).toBe(1)
 })
 })
+
+describe('External File Upload - Redirect Blocking',()=>{
+constvalidPNG=Buffer.from(
+'89504e470d0a1a0a0000000d494844520000000100000001'+
+'0806000000ifad8300000010494441541865000000018001'+
+'ffa500051f37dbba0000000049454e44ae426082',
+'hex',
+)
+
+conststartServer=async(server: ReturnType<typeofcreateServer>): Promise<number>=>{
+returnnewPromise<number>((resolve)=>{
+server.listen(0,'0.0.0.0',()=>{
+resolve((server.address()asAddressInfo).port)
+})
+})
+}
+
+it('should block malicious redirect',async()=>{
+constinternalServer=createServer((req,res)=>{
+res.writeHead(200,{'Content-Type': 'text/plain'})
+res.end('SECRET_CREDENTIALS')
+})
+
+constinternalServerPort=awaitstartServer(internalServer)
+
+constattackerServer=createServer((req,res)=>{
+res.writeHead(302,{
+Location: `http://127.0.0.1:${internalServerPort}/secret`,
+})
+res.end()
+})
+
+constattackerServerPort=awaitstartServer(attackerServer)
+
+try{
+awaitexpect(
+payload.create({
+collection: mediaSlug,
+data: {
+filename: 'malicious.jpg',
+url: `http://127.0.0.1:${attackerServerPort}/image.jpg`,
+},
+}),
+).rejects.toThrow()
+}finally{
+attackerServer.close()
+internalServer.close()
+}
+})
+
+it('should allow legitimate redirects within allowlist',async()=>{
+constedgeServer=createServer((req,res)=>{
+res.writeHead(200,{
+'Content-Type': 'image/png',
+'Content-Length': validPNG.length.toString(),
+})
+res.end(validPNG)
+})
+
+constedgeServerPort=awaitstartServer(edgeServer)
+
+constcdnServer=createServer((req,res)=>{
+res.writeHead(302,{Location: `http://127.0.0.1:${edgeServerPort}/image.png`})
+res.end()
+})
+
+constcdnServerPort=awaitstartServer(cdnServer)
+
+try{
+constdoc=awaitpayload.create({
+collection: allowListMediaSlug,
+data: {
+filename: 'cdn-image.png',
+url: `http://127.0.0.1:${cdnServerPort}/image.png`,
+},
+})
+
+expect(doc.filename).toBe('cdn-image.png')
+expect(doc.mimeType).toBe('image/png')
+}finally{
+cdnServer.close()
+edgeServer.close()
+}
+})
+
+it('should not allow infinite redirect loops',async()=>{
+letredirectServerPort: number
+
+constredirectServer=createServer((req,res)=>{
+res.writeHead(302,{Location: `http://127.0.0.1:${redirectServerPort}/loop`})
+res.end()
+})
+
+redirectServerPort=awaitstartServer(redirectServer)
+
+try{
+awaitexpect(
+payload.create({
+collection: allowListMediaSlug,
+data: {
+filename: 'loop.png',
+url: `http://127.0.0.1:${redirectServerPort}/loop`,
+},
+}),
+).rejects.toThrow(/Toomanyredirects/)
+}finally{
+redirectServer.close()
+}
+})
+})
 })
 
 asyncfunctionfileExists(fileName: string): Promise<boolean>{
 try{
 awaitstat(fileName)
 returntrue
-}catch(err){
+}catch(_err){
 returnfalse
 }
 }