Improve proxy security (#100) #134

toplenboren · 2024-04-01T23:36:57Z

Fixes #100

I added some small improvements to proxy.ts service

I've implemented:

Do not allow POST / PUT and pretty much everything besides GET
Do not allow requests to reserved ips
Do not allow anything besides http or https
Remove shady headers
Added a handy README note
Test that init real target http server and proxy and test end to end user

I asked a few questions in #100. Let's use issue to discuss planned changes

Motivation

We do not want our proxy to be abused and banned, thus all the security stuff. Check #100 for details

Deps

I needed a package to check whether IP is reserved or not. Thought that we do not want to bloat our code with this stuff. I found three packages: isMartianPacket, bogon and isReservedIp

I found is-reserved-ip code to be strange and bogon to have one more dependency 👍

I decided to pick isMartianPacket.Package seems to be maintained, and I found the lack of types (Can implement this myself 🥨) and lack of adoption as main drawback (thou need to say that many people just introduce code for this task in their projects).

Checklist (Gonna be checked once we are out of draft state)

proxy/README.md

proxy/package.json

proxy/index.ts

proxy/proxy.ts

ai · 2024-04-09T20:40:09Z

proxy/test/proxy.test.ts

+  )
+})
+
+describe('proxy tests', async () => {


We don’t use single describe per file in other tests.

Can we have a single describe here? The reason is that I want to utilise before and after so that I can init and destroy http server.

If I ditch single describe, then I'd have to write a lot of:

await initTestHttpServer( 'proxy', createProxyServer({ hostnameWhitelist: ['localhost'], silentMode: true }) ) await initTestHttpServer('target', targetServer) let proxyServerUrl = '' let targetServerUrl = '' before(() => { let proxy = getTestHttpServer('proxy') if (proxy) { proxyServerUrl = proxy.baseUrl } let target = getTestHttpServer('target') if (target) { targetServerUrl = target.baseUrl } if (!target || !proxy) { throw new Error( "Couldn't set up target server or proxy server. Please check out 'proxy.test.js'" ) } })

Maybe there is a way to support before outside the single describe?

You can use before/after without description.

Just use beforeEach or beforeAll from node:test.

proxy/test/proxy.test.ts

proxy/test/utils.ts

proxy/README.md

ai · 2024-04-12T12:39:22Z

proxy/proxy.ts

+          targetResponse.headers.get('content-type') ?? 'text/plain'
+      })
+      sent = true
+      res.write(await targetResponse.text())


Do you want to also improve a performance a little?

Why do we need to first create a full copy of the HTTP document in the proxy memory and only then send it to the user? Instead, on receiving first bites we can send them to the client. It improves performance and memory consumption.

https://developer.mozilla.org/en-US/docs/Web/API/Streams_API/Using_readable_streams

proxy/proxy.ts

ai · 2024-04-12T12:46:33Z

proxy/proxy.ts

+      let targetResponse = await fetch(url, {
+        headers: {
+          ...(req.headers as HeadersInit),
+          host: new URL(url).host


Should we write client’s IP to avoid using proxy to hide IP? There is X-Forwarded-For header for that.

Later we will add privacy mechanism for paid users.

Agreed. Let me take some time to research usecases and write tests :D

ai · 2024-04-12T12:59:07Z

proxy/proxy.ts

+      })
+      sent = true
+      res.write(await targetResponse.text())
+      res.end()


There is another nice abuse protection system. We can limit the size of response https://github.com/gridaco/cors.sh/blob/main/services/proxy.cors.sh/src/limit/payload-limit.ts

…tpServer functions

…stom classes

toplenboren · 2024-04-20T13:28:59Z

Hey, @ai I've done some cool stuff! Can you please re-review my PR?

Done:

Introduced timeout feature
Refactored error handling: All Bad Requests will not be looged and will return 400 to user
Fixed minor issues, such as README.md structure or typescript errors

Questions:

I have a question about single describe in tests. Please check it
I have a question about configuration

Left to do:

Debounce
Hide client IP
Limit size of response
Introduce partial return for optimizations

ai · 2024-04-20T16:41:11Z

proxy/proxy.ts

@@ -0,0 +1,126 @@
+// @ts-ignore


Let’s define simple types in global.d.ts. It should be a few lines just for API which we use.

ai · 2024-04-20T16:41:56Z

proxy/proxy.ts

+    silentMode?: boolean
+  } = {}
+): http.Server => {
+  // Main toggle for production features


The arguments are very simple to explain them in comments (in contrast with different hacks)

ai · 2024-04-20T16:42:23Z

proxy/README.md

+## Abuse Protection
+
+- Proxy allows only GET requests
+- Proxy do not allow requests to reserved ip addresses like `127.0.0.1`


Suggested change

- Proxy do not allow requests to reserved ip addresses like `127.0.0.1`

- Proxy do not allow requests to in-cloud IP addresses like `127.0.0.1`

ai · 2024-04-20T16:42:38Z

proxy/README.md

+
+- Proxy allows only GET requests
+- Proxy do not allow requests to reserved ip addresses like `127.0.0.1`
+- Proxy allows only http or https


Suggested change

- Proxy allows only http or https

- Proxy allows only HTTP or HTTPS protocols

ai · 2024-04-20T16:43:29Z

proxy/README.md

+
+## Test Strategy
+
+Proxy is tested using e2e testing. To write tests `initTestHttpServer` should be used


Suggested change

Proxy is tested using e2e testing. To write tests `initTestHttpServer` should be used

To test proxy we emulate the real HTTP servers (end-to-end testing).

Docs are for juniors.

ai · 2024-04-20T16:43:58Z

proxy/proxy.ts

+  }
+}
+
+export const createProxyServer = (


Let’s use function if it is a function

ai · 2024-04-20T16:45:23Z

proxy/proxy.ts

+
+      let requestUrl = new URL(url)
+      if (!hostnameWhitelist.includes(requestUrl.hostname)) {
+        // Do not allow requests to addresses that are reserved:


Suggested change

// Do not allow requests to addresses that are reserved:

// Do not allow requests to addresses inside our cloud:

We should explain “why we did it” not “what we did”

ai · 2024-04-20T16:49:20Z

proxy/proxy.ts

+        }
+      }
+
+      // Remove all cookie headers and host header from request


Suggested change

// Remove all cookie headers and host header from request

// Remove all cookie headers so they will not be set on proxy domain

// Replace Host header to target

ai · 2024-04-20T16:51:39Z

proxy/test/utils.ts

@@ -0,0 +1,68 @@
+//  Use this function if you need to init local http server for testing. Express is supported. Server will be closed automatically after tests finish


We don’t need docs here since the test is a good example

…r isMartianIp package

ai · 2024-04-27T21:34:03Z

I am merging this because I need it for other task

ai reviewed Apr 2, 2024

View reviewed changes

proxy/README.md Outdated Show resolved Hide resolved

ai reviewed Apr 2, 2024

View reviewed changes

proxy/README.md Outdated Show resolved Hide resolved

ai reviewed Apr 2, 2024

View reviewed changes

proxy/package.json Outdated Show resolved Hide resolved

toplenboren force-pushed the toplenboren-proxy branch from c2fa283 to 3ef4209 Compare April 8, 2024 13:15

toplenboren changed the title ~~[draft] Improve proxy security (#100)~~ Improve proxy security (#100) Apr 8, 2024

toplenboren requested a review from ai April 8, 2024 22:23