Fix removal of domains with schema #2229
Conversation
In at least one case a customer of ours added a domain including a schema, as in “http://domain.tld/”. That is obviously incorrect, but then they couldn't remove it using the dokku command: $ dokku domains:remove app7 http://domain.tld/ sed: -e expression #1, char 9: unknown command: `/' This patch changes the sed command to use the "\CregexpC" syntax. The character used is a vertical pipe, which is far less likely to be used in a URL. After the change: $ dokku domains:remove app7 http://domain.tld/ -----> Removed http://domain.tld/ from app7 Signed-off-by: Michael Hanselmann <hansmi@vshn.ch>
|
The patch seems reasonable. However, the use case raises a question. Given the domains assigned to an app are byte for byte inserted into nginx configuration, is a domain with a scheme actually a valid argument to the |
|
I can't find any docs that state having a scheme is valid. We should probably validate and toss them. I also think that trailing slash shouldn't be there either. |
|
Yeah, there shouldn't be a scheme or any slashes. Just a FQDN. Currently it is possible to insert incorrect strings without there being a working option to remove them again, hence the patch. |
|
Ah ok understood. Thank you for clarifying (again 😉). I missed the initial bit about that being "obviously incorrect". Sorry for the confusion here. |
|
@hansmi do you mind working on a pr to ensure people don't add invalid strings? |
|
@josegonzalez Unfortunately I can't work on this at the present time. I have opened issue #2231, though. |
|
Thanks for filing! |
In at least one case a customer of ours added a domain including
a schema, as in “http://domain.tld/”. That is obviously incorrect, but
then they couldn't remove it using the dokku command:
This patch changes the sed command to use the "\CregexpC" syntax. The
character used is a vertical pipe, which is far less likely to be used
in a URL. After the change: