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

Add warning regarding "/" in CS2_SERVERNAME #157

Merged
merged 2 commits into from
Mar 21, 2025

Conversation

Peeetman
Copy link
Contributor

Using a "/" character in the CS2_SERVERNAME (e.g., "CS Server 1/3 of My Amazing LAN Party") causes the sed substitution in
https://github.com/joedwards32/CS2/blob/38b2938f7927c5f83e62fe32fe23f37e1f2201cb/bookworm/etc/entry.sh#L103to fail because the default delimiter "/" conflicts with the character in the value. Although the server continues to run, the configuration file ends up with unresolved placeholders (for example, {{SERVER_CHEATS}} remains unchanged). As a result, when players try to connect, they receive a "wrong password" error - even if the password is correct or none was set initially.

How to Reproduce:

  1. take the example docker-compose.yml https://github.com/joedwards32/CS2/blob/38b2938f7927c5f83e62fe32fe23f37e1f2201cb/examples/docker-compose.yml
  2. change the CS2_SERVERNAME to "Server 1/3"
  3. docker compose up
  4. connect to server
  5. wrong password :(

Recommendation:
It is a lot of work to allow all sorts of arbitrary characters. I suggest just adding a warning in both the README and the example docker-compose.yml. I could provide another solution if requested.

Thank you for this amazing docker image and all of your work. It has been serving me and my friends well on our LAN parties <3 This is my first pull request ever created, so I hope the format and description are acceptable.

Copy link
Owner

@joedwards32 joedwards32 left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

I think you can use "/" if you escape them first, e.g. "\/".

Copy link
Owner

@joedwards32 joedwards32 left a comment

Choose a reason for hiding this comment

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

Also, as "/" is being used as the delimiter in all sed substitutions I think it will cause problems for lots of other variables: https://github.com/joedwards32/CS2/blob/main/bookworm/etc/entry.sh#L103

@Peeetman
Copy link
Contributor Author

Peeetman commented Mar 21, 2025

It is technically possible to escape forward slashes / in all environment variables by applying an additional sed command, such as
echo "Server 1/3" | sed 's/\//\\\//g' # results in: Server 1\/3

However, this introduces actually the same issue: users must not use already escaped slashes \/ in their variables, as these will be double-escaped and may lead to unexpected behavior. While it may seem unlikely that someone would intentionally use \/ in a server name, that’s beside the point - the problem remains, and the sed substitution will silently fail or result in incorrect output without any feedback.

Proposed solution:
Warn users that if they really want to use a slash / in any Counter-Strike-related environment variable (e.g., in docker-compose.yml), it must be escaped by prepending a backslash \, as is standard in many configuration formats.

As you already noted, if users really want a slash to appear in their server name, they must escape it manually.
CS2_SERVERNAME="My Amazing Server 1\/3" # results in: My Amazing Server 1/3

I applied this to the current pull request.

Copy link
Owner

@joedwards32 joedwards32 left a comment

Choose a reason for hiding this comment

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

I agree that documenting the requirement is better than trying to auto-escape / characters found in environment variables, getting that right is fiddly.

Maybe one day I will convert this all to python and jinja templates.

Approved. Thank you for your contribution.

@joedwards32 joedwards32 added documentation Improvements or additions to documentation good first issue Good for newcomers labels Mar 21, 2025
@joedwards32 joedwards32 self-assigned this Mar 21, 2025
@joedwards32 joedwards32 merged commit f43f7df into joedwards32:main Mar 21, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation good first issue Good for newcomers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants