Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fastcgi: ensure SERVER_PORT is always set #4572

Merged
merged 1 commit into from
Mar 2, 2022

Conversation

ttys3
Copy link
Contributor

@ttys3 ttys3 commented Feb 12, 2022

as tools.ietf.org/html/rfc3875#section-4.1.15

4.1.15.  SERVER_PORT

   The SERVER_PORT variable MUST be set to the TCP/IP port number on
   which this request is received from the client.  This value is used
   in the port part of the Script-URI.

      SERVER_PORT = server-port
      server-port = 1*digit

   Note that this variable MUST be set, even if the port is the default
   port for the scheme and could otherwise be omitted from a URI.

related problem caused by this: when works with xdebug , "Cannot accept external Xdebug connection: Cannot evaluate expression '$_SERVER['SERVER_PORT']'"

@CLAassistant
Copy link

CLAassistant commented Feb 12, 2022

CLA assistant check
All committers have signed the CLA.

@ttys3 ttys3 force-pushed the fix-fcgi-empty-SERVER_PORT branch from be9d667 to 240f70c Compare February 12, 2022 11:39
@ttys3 ttys3 changed the title fix(fastcgi): SERVER_PORT variable MUST be set even it is omitted from a URI by client) fix(fastcgi): SERVER_PORT variable MUST be set (even it is omitted from a URI by client) Feb 12, 2022
@francislavoie francislavoie changed the title fix(fastcgi): SERVER_PORT variable MUST be set (even it is omitted from a URI by client) fastcgi: SERVER_PORT variable MUST be set (even it is omitted from a URI by client) Feb 12, 2022
@francislavoie
Copy link
Member

Hmm. The reason we left it out is because we don't really know that the request came in on ports 80 or 443 if the port is empty. For example, Caddy could be bound to a unix socket and receive the request that way, in which case the port would also be empty. The fastcgi RFC makes the assumption that the server always receives the request over TCP, but that's not always the case (also, HTTP3 is UDP for example).

But realistically, it's probably more helpful to PHP apps to fill it in than leave it out, even if it's not strictly accurate.

@francislavoie francislavoie added the bug 🐞 Something isn't working label Feb 12, 2022
@francislavoie francislavoie added this to the v2.5.0 milestone Feb 12, 2022
@francislavoie francislavoie changed the title fastcgi: SERVER_PORT variable MUST be set (even it is omitted from a URI by client) fastcgi: ensure SERVER_PORT is always set Feb 12, 2022
@francislavoie francislavoie added the under review 🧐 Review is pending before merging label Feb 12, 2022
Copy link
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks; we can probably do something like this, but a few things need to happen if we're going to rigorously comply with spec.

The SERVER_PORT variable MUST be set to the TCP/IP port number on
which this request is received from the client.

  1. We need to verify that the port is an actual number.
  2. We need to verify the request is received over TCP/IP, not UDP or unix sockets.
  3. Maybe set it to 0 or -1 otherwise, I dunno. The spec doesn't really accommodate the modern Web.

… port for the scheme (which omiited from a URI by client)
@francislavoie francislavoie force-pushed the fix-fcgi-empty-SERVER_PORT branch from 240f70c to 3349d23 Compare February 15, 2022 23:55
Copy link
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After some discussion with Francis in Slack, I'm convinced we can go ahead and merge this change (I might slightly tweak it afterward). My main concerns were spec compliance, but apparently nobody really follows this spec anyway, and the spec is ambiguous because it assumes TCP/IP is the only transport/routing that exists, but nowadays that's not true (unix sockets, etc).

Another concern I had was assuming HTTP on port 80 and HTTPS on port 443. There are many reasons HTTP/HTTPS are served on different ports and where the port may not appear in the request (for example, request comes in on the default HTTPS port 443, so not specified in the URL, but then the router forwards it to another port). Francis told me the PHP application doesn't really care, it just wants to know what port the client connected on. Some PHP applications even erroneously check this variable for 80 to assume HTTP, or 443 to assume HTTPS.

I don't love setting this to a value that can very easily be wrong (as opposed to just leaving it unset). So maybe we should clarify in the docs that this value is set for widest compatibility, not correctness.

@mholt mholt merged commit de490c7 into caddyserver:master Mar 2, 2022
@mholt mholt added discussion 💬 The right solution needs to be found needs docs ✍️ Requires documentation changes and removed bug 🐞 Something isn't working under review 🧐 Review is pending before merging labels Mar 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion 💬 The right solution needs to be found needs docs ✍️ Requires documentation changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants