Page MenuHomePhorge

Set "preconnect" HTTP header when "security.alternate-file-domain" is set
AcceptedPublic

Authored by aklapper on Jun 18 2024, 10:36.

Details

Summary

When a CDN or alternate file domain is configured, reduce perceived latency by resolving DNS and establishing a connection.

See https://developer.mozilla.org/en-US/docs/Web/HTML/Attributes/rel/preconnect and https://we.phorge.it/book/phorge/article/configuring_file_domain/

Closes T15859

Test Plan
  1. Set http://phorge.localhost/config/edit/security.alternate-file-domain/ via ./bin/config set "security.alternate-file-domain" "https://whatever.example.com/"
  2. Open the "Network" tab in your web browser's "Developer Tools" and go to http://phorge.localhost/
  3. Check under "Headers" that "Response Headers" includes the new header Link: <https://whatever.example.com/>; rel="preconnect".

Diff Detail

Repository
rP Phorge
Branch
T15859preFetch (branched from master)
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 1373
Build 1373: arc lint + arc unit

Event Timeline

Should the summary also include https://developer.mozilla.org/en-US/docs/Web/HTML/Attributes/rel/preconnect as a reference? https://developer.mozilla.org/en-US/docs/Web/Performance/dns-prefetch doesn't mention preconnect in the first few paragraphs, so it's somewhat confusing.

Preconnect is only useful if a page needs to make connections to third-party domains. It's unnecessary for AphrontAjaxResponse and redirects, so adding the header to the base class AphrontResponse is excessive. What do you think of moving it to AphrontHTMLResponse?

Good point, thanks! Updated.

speck added inline comments.
src/aphront/response/AphrontResponse.php
115 ↗(On Diff #2121)

Should this ensure cdn ends with ‘/‘? Unclear if that’s becessary

src/aphront/response/AphrontResponse.php
115 ↗(On Diff #2121)

Testing locally, a trailing slash wasn't needed: I set security.alternate-file-domain to a non-existing domain like https://whatever.example.com and images/JS did not load as expected, and I set it to existing https://phab.wmfusercontent.org/ and could see in the browser's Developer Tools' network tab that it loaded resources from that domain.

What if $cdn has some evil characters? Do we have some escaping, or can it cause anything bad, or do we not care? Looks like we're including a bunch of other config as-is here, so maybe it's ok.

can it cause anything bad

If the domain is invalid or does not host Phorge assets: Failing to load assets like CSS/images/JS as the resulting URIs are invalid.
If the domain is valid: In theory you could craft some evil JS if path and filename are what's expected by Phorge. But that is nothing to fix in this very setting which covers an alternative domain name only and can do nothing about content hosted on that domain.

Still, if an admin sets /config/edit/security.alternate-file-domain/ in the Phorge installation they are supposed to maintain to a domain not fully under their control or to include evil characters then I'd be way more worried about having system administrators who seem to have no clue what they are doing when pushing or tweaking random knobs in a piece of software.

src/aphront/response/AphrontResponse.php
115 ↗(On Diff #2121)

My previous reply likely missed your actual point about trailing slashes: Both
./bin/config set "security.alternate-file-domain" "https://phab.wmfusercontent.org/"
and
./bin/config set "security.alternate-file-domain" "https://phab.wmfusercontent.org"
expose the very same behavior in the "network" browser tab (some expected CSP failures etc).

src/aphront/response/AphrontResponse.php
3 ↗(On Diff #2121)

What do you think of moving the new header to AphrontHTMLResponse, or another class that better represents a document load in web browsers? AphrontResponse is too generic and covers many types of requests, in which Ajax and resource requests generally don't need the preconnect, because they generally do not set off new connections to a different domain.

What if $cdn has some evil characters? Do we have some escaping

Premising that probably no need to escape headers:

https://stackoverflow.com/a/5677844/3451846

But yes, probably need to escape the URI inside <...>. The example is limited to the path, but probably relevant also for the domain itself:

https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Link#encoding_urls

After 15 minutes of reading stuff around on the web, I think we need a little help. Waiting for answers (and waiting for downvotes lol).

https://stackoverflow.com/q/78671273/3451846

probably need to escape the URI

I'm not sure it matters for now as Phorge does not seem to accept non-ascii domains anyway (with a confusing error message):

[acko@fedora phorge (T15859preFetch *$|u+1)]$ ./bin/config set "security.alternate-file-domain" "https://苗条.example.com"
Usage Exception: Config option 'security.alternate-file-domain' is invalid. The URI must start with 'http://' or 'https://'.
[acko@fedora phorge (T15859preFetch *$|u+1)]$ ./bin/config set "security.alternate-file-domain" "https://áéexample.com"
Usage Exception: Config option 'security.alternate-file-domain' is invalid. The URI must start with 'http://' or 'https://'.

I could imagine something like $cdn = preg_replace_callback('/[^\x20-\x7f]/', function($match) { return rawurlencode($match[0]); }, $cdn); to work. Untested because above.

aklapper added inline comments.
src/aphront/response/AphrontResponse.php
3 ↗(On Diff #2121)

Yes, that makes a lot of sense. Thanks, going to cover in the next revision

aklapper marked an inline comment as done.

Move CDN check from generic AphrontResponse to more correct AphrontHTMLResponse.

I confirm this still sets the HTTP Header.

Looks like installing php-xdebug forces me to set 'xdebug.mode' to 'coverage' in 'php.ini' to pass arc unit, no matter what...

Would anyone be willing to give this another review? :)

I'm not sure it matters for now as Phorge does not seem to accept non-ascii domains anyway (with a confusing error message):

[acko@fedora phorge (T15859preFetch *$|u+1)]$ ./bin/config set "security.alternate-file-domain" "https://苗条.example.com"
Usage Exception: Config option 'security.alternate-file-domain' is invalid. The URI must start with 'http://' or 'https://'.
[acko@fedora phorge (T15859preFetch *$|u+1)]$ ./bin/config set "security.alternate-file-domain" "https://áéexample.com"
Usage Exception: Config option 'security.alternate-file-domain' is invalid. The URI must start with 'http://' or 'https://'.

Good catch. Phorge does not currently support these scaring esoteric domains that MAY need a bigger test plan. In fact, the utility PhutilURI is clearly not supporting "internationalized domain names" right now, as I've now described in T16048.

Whenever T16048 is open or closed, Phorge demonstrated to already validate configured URI(s) before using them here, so, this patch itself is safe.

Also, I've tested that creating this local alternative:

http://苗条asdasasasd.phorge.localhost/

That, once encoded, it's this:

http://xn--asdasasasd-mr0vl07w.phorge.localhost/

And that can be configured right now with this command:

./bin/config set security.alternate-file-domain http://xn--asdasasasd-mr0vl07w.phorge.localhost/
/etc/hosts
127.0.0.1  xn--asdasasasd-mr0vl07w.phorge.localhost
<VirtualHost *:80>

    ServerName phorge.localhost
    ServerAlias xn--asdasasasd-mr0vl07w.phorge.localhost

    DocumentRoot /var/www/phorge/webroot

    RewriteEngine on
    RewriteRule ^(.*)$          /index.php?__path__=$1  [B,L,QSA]

    php_admin_value post_max_size 32M

</VirtualHost>

Again, this is just an attempt to screw up my local installation and be aggressive here. Don't try since T16048 is not implemented.


Another hypotetical problem already covered: somebody may say: «what if my Phorge is served by phorge.example.com and I've set security.alternate-file-domain? for my use case, isn't it better to avoid to generate a duplicate Link????? UHM UHM????». Answer: dear hypotetical user, this potential workflow has no sense, since that config is designed to have a different domain, or null.

Therefore, this is safe, tested, approved, and probably already ready for future improvement e.g. T16048. Thaanks!

This revision is now accepted and ready to land.Thu, May 1, 21:43

(Plus, a domain 0 is nonsense, so the well-known PHP pitfall of if($something) is excluded here - double-slam-accept)