Repository: twisted/twisted

Issue:
WSGI applications run inside Twisted Web don’t get access to the client port
**Is your feature request related to a problem? Please describe.**
The problem we’re trying to solve is that WSGI applications run inside Twisted Web don’t get access to the client port (though the client address is made available through REMOTE_ADDR). We have an application in which the client’s port is necessary. 

When looking at the [`_WSGIResponse` class](https://github.com/twisted/twisted/blob/11c9f5808da4383678c9ff830d74137efd32d22d/src/twisted/web/wsgi.py#L292), we notice that only `REMOTE_ADDR` is set:
```
        # All keys and values need to be native strings, i.e. of type str in
        # *both* Python 2 and Python 3, so says PEP-3333.
        self.environ = {
            "REQUEST_METHOD": _wsgiString(request.method),
            "REMOTE_ADDR": _wsgiString(request.getClientAddress().host), <------
            "SCRIPT_NAME": _wsgiString(scriptName),
            "PATH_INFO": _wsgiString(pathInfo),
            "QUERY_STRING": _wsgiString(queryString),
            "CONTENT_TYPE": _wsgiString(request.getHeader(b"content-type") or ""),
            "CONTENT_LENGTH": _wsgiString(request.getHeader(b"content-length") or ""),
            "SERVER_NAME": _wsgiString(request.getRequestHostname()),
            "SERVER_PORT": _wsgiString(str(request.getHost().port)),
            "SERVER_PROTOCOL": _wsgiString(request.clientproto),
        }
```

**Describe the solution you'd like**
To provide support for retrieving the remote port as done with the remote IP address. This could be done by updating `environ` to include a new "REMOTE_PORT" attribute as following:
```
        self.environ = {
            "REQUEST_METHOD": _wsgiString(request.method),
            "REMOTE_ADDR": _wsgiString(request.getClientAddress().host), <------
            "REMOTE_PORT": _wsgiString(str(request.getClientAddress().port)),
            "SCRIPT_NAME": _wsgiString(scriptName),
```

**Describe alternatives you've considered**
Besides implementing a monkey patch, another alternative is to try and retrieve the port value from the WSGI context, which might have undesired side affects.

**Additional context**

Discussion:
adiroiban: Thanks Daniel for the PR.

Is this ready for review. No hurry. I just wanted to check the status for this.

Cheers
thinkst-daniel: Thanks for checking @adiroiban, yes it is.
thinkst-daniel: Thanks @adiroiban, I've added a new test for the remote port case (that matches the remote addr case). Let me know if the test is what you had in mind?
exarkun: What spec does this change conform to?  A link to the relevant section might be nice somewhere in here.

adiroiban: As far as I can tell, this is not a standard environment variable https://github.com/joewalnes/websocketd/wiki/Environment-variables#remote_port

There is REMOTE_HOST in the original CGI RFC https://datatracker.ietf.org/doc/html/rfc3875#section-4.1.9

But I found this variable in the IIS implementatin https://learn.microsoft.com/en-us/iis/web-dev-reference/server-variables

The port is not available in Apache HTTPD https://httpd.apache.org/docs/current/mod/mod_cgi.html

But in modern days, I think that it helps to also have the port number ... at least for debug/audit/log
glyph: > There is REMOTE_HOST in the original CGI RFC https://datatracker.ietf.org/doc/html/rfc3875#section-4.1.9

PEP 3333 says

>> The environ parameter is a dictionary object, containing CGI-style environment variables.

therefore it inherits the CGI spec'd version of this variable.
exarkun: > > > The environ parameter is a dictionary object, containing CGI-style environment variables.
> 
> therefore it inherits the CGI spec'd version of this variable.

Just checking - there *is no* CGI spec'd version of this variable, are we on the same page?
adiroiban: > Just checking - there is no CGI spec'd version of this variable, are we on the same page?

That is corect.
I can't find the specification for this variable.

The RFC is only for the `REMOTE_HOST` .

The `REMOTE_PORT` is not documented. 

I can see the draft for the CGI started on 1998 and was finalized in 2004.
I guess that the source IP is not really that important for the majority of use cases.

Since we already expose the peer's host, I think it's safe to also expose the port.
And it can be important information to help with logging / audit / security checks.

yes... it will make the implementation non-portable.


--------

The following variables are standard / part of the RFC for the client-side part of the socket/connection:

* REMOTE_ADDR
* REMOTE_HOST
* REMOTE_IDENT
* REMOTE_USER

For the server-side socket we have:

* SERVER_NAME
* SERVER_PORT
* SERVER_PROTOCOL

glyph: > Just checking - there _is no_ CGI spec'd version of this variable, are we on the same page?

Sorry, I got confused between REMOTE_HOST and REMOTE_PEER.
glyph: OK so here are some references I found for `REMOTE_PORT`.

This book on CGI: https://www.cgi101.com/book/ch3/text.html

"Abyss Web Server For Windows" https://aprelium.com/data/doc/2/abyssws-win-doc-html/cgivars.html

This experimentally-determined environment dump from an Apache 2.4 httpd: https://stackoverflow.com/a/25521262/13564

Which is documented officially as a "variable" you can use in an "expression" https://httpd.apache.org/docs/2.4/expr.html#vars even if this isn't technically CGI

IBM's web server: https://www.ibm.com/docs/en/i/7.2?topic=information-environment-variables

the US Naval Academy's published course on CGI: https://www.usna.edu/Users/cs/adina/teaching/it350/fall2020/lectures/set06-cgi.html

Cloudflare explicitly *not* supporting it: https://community.cloudflare.com/t/no-port-via-server-remote-port/37682/4

Apparently nginx's set of CGI or FastCGI variables are just specified in your own config, however you like? https://www.nginx.com/resources/wiki/start/topics/examples/phpfcgi/ but `fastcgi_param   REMOTE_PORT             $remote_port;` is frequently recommended.

the ASGI specification references it as a part of WSGI compatibility https://asgi.readthedocs.io/en/latest/specs/www.html#wsgi-compatibility

So this seems like while it is not technically standardized anywhere, it is ad-hoc copy/pasted around frequently, and everyone seems to *assume* that it's been inherited from CGI, because some large majority of CGI environments do in fact set this variable, although probably because of a config file that gets distributed with default installs of popular web servers or tutorials for the same.
adiroiban: Does it mean that we can implement it as defacto variable, without having to add any warning extra when someone will use it ? Warning in the sense, that they are using a non-standard variable and that their application might break if they try to migrate to Cloudfare... for example.

I am not very familiar with CGI/WSGI. I know what they are expected to do, but I have never created a CGI application... other that the homeworks for CS classes (a very long time ago) :)

glyph: > Does it mean that we can implement it as defacto variable, without having to add any warning extra when someone will use it ? Warning in the sense, that they are using a non-standard variable and that their application might break if they try to migrate to Cloudfare... for example.

Yeah, I think we can just document it. People seem to mostly expect it, and they can deal with Cloudflare when they use Cloudflare (although don't use Cloudflare, use Fastly…)
thinkst-daniel: 


> > Does it mean that we can implement it as defacto variable, without having to add any warning extra when someone will use it ? Warning in the sense, that they are using a non-standard variable and that their application might break if they try to migrate to Cloudfare... for example.
> 
> Yeah, I think we can just document it. People seem to mostly expect it, and they can deal with Cloudflare when they use Cloudflare (although don't use Cloudflare, use Fastly…)

Thanks for all extra info and references @glyph. As you've pointed out, it is possible get `REMOTE_PORT` by specifying it in an NGINX config too. So we would just like to do the same from the Twisted side.

`REMOTE_PORT` is helpful in cases where requests are coming from behind a PAT (Port Address Translation).
thinkst-daniel: Hey @adiroiban, I just want to double-check if there is any other changes or updates I should do here? Thanks!
thinkst-daniel: > Changes look good.
> 
> The missing part requested by Glyph
> 
> > Yeah, I think we can just document it.
> 
> As this is a non-standard variable, I think it would help to have it documented in the docstring.
> 
> Maybe here ?
> 
> https://github.com/twisted/twisted/blob/63df84e454978bd7a2d57ed292913384ca352e1a/src/twisted/web/wsgi.py#L533-L545
> 
> Other than that, I think that we can merge this.
> 
> Thanks!

Thanks @adiroiban let me know if that doc was what you had in mind.
adiroiban: Thanks for the update. I left a comment, but I think that also your version is ok.

Please review and let me know what you think.

We can merge this, even with your version.

I think that we all agree that the variable should be available to the applications.

If documentation is not right, this can be updated in a follow up PR.

Thanks again and sorry for the delay.

----------

A final rant :)

WSGI is not my jam and I try to avoind using threads.
I am not sure why we have thread based web app development in Twisted.
I am using twisted so that I don't have to use threads.


Patch:
diff --git a/src/twisted/newsfragments/12096.feature b/src/twisted/newsfragments/12096.feature
new file mode 100644
index 0000000000..a07999d034
--- /dev/null
+++ b/src/twisted/newsfragments/12096.feature
@@ -0,0 +1,1 @@
+twisted.web.wsgi request environment now contains the peer port number as `REMOTE_PORT`.
\ No newline at end of file
diff --git a/src/twisted/web/wsgi.py b/src/twisted/web/wsgi.py
index 43227f40e3..29d1de8f2e 100644
--- a/src/twisted/web/wsgi.py
+++ b/src/twisted/web/wsgi.py
@@ -287,9 +287,11 @@ class _WSGIResponse:
 
         # All keys and values need to be native strings, i.e. of type str in
         # *both* Python 2 and Python 3, so says PEP-3333.
+        remotePeer = request.getClientAddress()
         self.environ = {
             "REQUEST_METHOD": _wsgiString(request.method),
-            "REMOTE_ADDR": _wsgiString(request.getClientAddress().host),
+            "REMOTE_ADDR": _wsgiString(remotePeer.host),
+            "REMOTE_PORT": _wsgiString(str(remotePeer.port)),
             "SCRIPT_NAME": _wsgiString(scriptName),
             "PATH_INFO": _wsgiString(pathInfo),
             "QUERY_STRING": _wsgiString(queryString),
@@ -535,6 +537,9 @@ class WSGIResource:
     An L{IResource} implementation which delegates responsibility for all
     resources hierarchically inferior to it to a WSGI application.
 
+    The C{environ} argument passed to the application, includes the
+    C{REMOTE_PORT} key to complement the C{REMOTE_ADDR} key.
+
     @ivar _reactor: An L{IReactorThreads} provider which will be passed on to
         L{_WSGIResponse} to schedule calls in the I/O thread.
 


Test patch:
diff --git a/src/twisted/web/test/test_wsgi.py b/src/twisted/web/test/test_wsgi.py
index b6be4f0f4b..ccb736c6bf 100644
--- a/src/twisted/web/test/test_wsgi.py
+++ b/src/twisted/web/test/test_wsgi.py
@@ -715,7 +715,11 @@ class EnvironTests(WSGITestsMixin, TestCase):
         The C{'REMOTE_ADDR'} key of the C{environ} C{dict} passed to the
         application contains the address of the client making the request.
         """
-        d = self.render("GET", "1.1", [], [""])
+
+        def channelFactory():
+            return DummyChannel(peer=IPv4Address("TCP", "192.168.1.1", 12344))
+
+        d = self.render("GET", "1.1", [], [""], channelFactory=channelFactory)
         d.addCallback(self.environKeyEqual("REMOTE_ADDR", "192.168.1.1"))
 
         return d
@@ -735,6 +739,20 @@ class EnvironTests(WSGITestsMixin, TestCase):
 
         return d
 
+    def test_remotePort(self):
+        """
+        The C{'REMOTE_PORT'} key of the C{environ} C{dict} passed to the
+        application contains the port of the client making the request.
+        """
+
+        def channelFactory():
+            return DummyChannel(peer=IPv4Address("TCP", "192.168.1.1", 12344))
+
+        d = self.render("GET", "1.1", [], [""], channelFactory=channelFactory)
+        d.addCallback(self.environKeyEqual("REMOTE_PORT", "12344"))
+
+        return d
+
     def test_headers(self):
         """
         HTTP request headers are copied into the C{environ} C{dict} passed to