I've rebased to fix the merge conflict.

I see 650e4c4 was exactly the same as my second commit (ugh, timing...), so the part about httpAddress being ignored is fixed.

That leaves the behaviour of httpSafePort which is weird; since I've had time to play with the config since I don't think my commit is correct as this port really isn't used most of the time. I think we should start by making that comment clear in the example config.

Hello,

Is this still needed?

I'd need to recheck but I dropped the redundant patch, so this patch being left is still coherent (the first point in top's comment; I edited it to make it clear the second point is not longer relevant)

However as said in October I'm honestly not sure it's correct: as far as I could see there is no example setup that actually relies on the second "httpSafePort", while this patch makes cryptpad listen to it by default which is not something we'd want if it's not used.

So, something still needs to be done, but I can't really help much given I don't understand the premise of that second port -- I think it's meant for trying cryptpad without a reverse proxy at all, which isn't a supported configuration, so the whole code could be ripped off?
Or the code should be fixed to listen to the safe port if and only if it's set in config, because that's not the case here.

I've just updated my instance, this is still definitely needed:
Env.httpSafePort would only be set if neither httpSafeOrigin nor httpSafePort are set in config:

• if neither are set, cryptpad would listen to httpPort+1
• otherwise there is no way to make it listen to this port

In practice cryptpad does not seem to differentiate what comes on which port so this looks like purely a dev option (so running from node directly can bind on two sockets to test different origins), so this is probably just unused, but fix the setting to only listen when the port is explicitly set.

There's a similar problem with the websocketPort which is not disableable at all (it feels weird to use up a port if it's not going to be used in the simple nginx config), but this one really cannot be disabled so I added a line saying that to the config to save whoever comes next some time.
(And while I was here I removed some remnants of NO_SANDBOX which was left unused since fde6f15)

I've updated the patches and tested various combinations (using or not using the ports), this looks much better to me but I'd be happy if there was any cleanup even if these patches aren't used.

Thanks!

Hello,

Is this still needed?

Hello!

I can confirm that this PR is still needed. I tried to set up an instance using the official docker image. I followed all the instructions, read the config with attention but didn't use the provided example nginx.

I set up my sandbox domain to redirect to the httpSafePort, but it didn't work. And when I tried to access to the httpSafePort directly, the page wouldn't even open. It took me an hour and reading the actual server code to finally understand that the app didn't even listen on the httpSafePort. Pointing the sandbox domain to the same port as the main domain was the fix.

All this confusion could have been prevented if I did read with attention the example nginx config. Or with this PR.

@yflory could you give it a look please? :)

Hello,

Thanks for your contribution @martinetd. However, after discussion with our team we've found that:

• Forcing the listening on a httpSafePort isn't needed
• Traffic should be directed to the standard port, both on the main and sandbox domain
• httpSafePort is only useful when you don't have a reverse proxy in front of CryptPad, meant to be used for development setups

We will improve the documentation to better reflect this situation and the requirements of both production and development setup.

Hence, we'll be now closing this PR.

Hi @mathilde-cryptpad
Thanks for the documentation update, I agree the ports aren't needed in most setups and the addition you've merged in staging is appreciated!

Please note though that this PR has real behavior fixes, I don't remember details but according to my last reply the default configuration listens to an extra port we don't want cryptpad to listen to (httpPort +1), and for whoever wants to do developments directly with npm they won't be able to change the port.
I'd like to see at least the former fixed, as that was my main reason for keeping this PR open, even if cryptpad is only listening to localhost it's an inconvenience if it takes up ports that aren't used.
I can update the patches and send a new PR with just that against staging if you prefer.