Skip to content

fix: prevent double sudo in parseCommandsByLineForSudo#9781

Open
jeroenvdmeer wants to merge 1 commit intocoollabsio:v4.xfrom
jeroenvdmeer:fix/parse-commands-double-sudo
Open

fix: prevent double sudo in parseCommandsByLineForSudo#9781
jeroenvdmeer wants to merge 1 commit intocoollabsio:v4.xfrom
jeroenvdmeer:fix/parse-commands-double-sudo

Conversation

@jeroenvdmeer
Copy link
Copy Markdown

Changes

  • Ran into an issue that the proxy did not start when the server had access to a different account than the root account.
  • Fixed a bug in bootstrap/helpers/sudo.php where parseCommandsByLineForSudo() inadvertently generated double sudo sudo commands.
  • The operator replacements for &&, ||, and | were naive string replacements. If a previous step already prepended sudo (e.g. && sudo chown -R ...), the third pass would turn this into && sudo sudo chown .... This causes execution to fail when PTY is required.
  • This PR adds a negative lookahead to the preg_replace statements for &&, ||, and |, ensuring sudo is only injected if the subsequent token is not already sudo.
  • Updated existing tests that previously documented the buggy double sudo behavior as expected, and added comprehensive new regression tests for all three operator variants.

Additional Context for Documentation:

  1. During debugging on an Ubuntu 24.04 machine we identified that CIS-hardened servers with Defaults log_input, log_output or requiretty in sudoers will cause programmatic Coolify deployments to fail silently because sudo allocates a PTY and consumes the bash heredoc stdin. It is highly recommended to add !use_pty, !log_input, and !log_output to the Coolify user's sudoers configuration to ensure smooth proxy startups and queue job executions.
  2. Furthermore, /data/coolify/proxy/ must be owned by the SSH user executing Coolify, otherwise the SSH session will encounter permission denied errors when attempting to write docker-compose.yml.

Issues

Category

  • Bug fix
  • Improvement
  • New feature
  • Adding new one click service
  • Fixing or updating existing one click service

Preview

N/A - backend logic change.

AI Assistance

  • AI was NOT used to create this PR
  • AI was used (please describe below)

If AI was used:

  • Tools used: Claude Code
  • How extensively: AI was used to debug the execution traces of the parseCommandsByLineForSudo pipeline, write the negative lookahead regular expressions, update the Pest tests, and format the PR description. I (human) thoroughly verified the findings and architecture on my VPS.

Testing

  • Automated Tests: Wrote and updated Pest regression tests in tests/Unit/ParseCommandsByLineForSudoTest.php confirming the negative lookahead correctly prevents double sudo injection.
  • Manual Verification: Deployed the patch manually inside a running Coolify container on a CIS Level 1-hardened Ubuntu 24.04 VPS. Verified that clicking "Start Proxy" in the UI successfully generates the correct single sudo execution chain, creates the docker-compose.yml with the correct ownership via the SSH session, and successfully starts the coolify-proxy container without exiting silently.

Contributor Agreement

Important

  • I have read and understood the contributor guidelines. If I have failed to follow any guideline, I understand that this PR may be closed without review.
  • I have searched existing issues and pull requests (including closed ones) to ensure this isn't a duplicate.
  • I have tested all the changes thoroughly with a local development instance of Coolify and I am confident that they will work as expected when a maintainer tests them.

@Iisyourdad
Copy link
Copy Markdown
Contributor

Howdy, this needs a couple of fixes before merge.

First, this PR currently targets v4.x, Coolify PRs should target the branch next.

Second, the new tests currently don't work. After Pest sanitizes the operator-only names. Running the test file hits a fatal redeclare for the still injects sudo after &&/|| when next token is not sudo cases because && and || both collapse out of the generated test method name. Please rename those tests using words like and operator / or operator.

There is also a behavior mismatch under those tests: several fixtures already start with sudo, but parseCommandsByLineForSudo() still prefixes every non-exempt line with sudo in the first pass. For example, a fixture like sudo docker stop proxy 2>/dev/null || true becomes sudo sudo docker stop ... before the operator replacement logic runs. Either the helper should explicitly skip lines already starting with sudo, or the tests should avoid expecting that behavior.

When I ran the test I got the following.

PHP Fatal error: Cannot redeclare generated Pest test method

Generated method:
  P\Tmp\coolifypr9781\tests\Unit\ParseCommandsByLineForSudoTest
    ::__pest_evaluable_still_injects_sudo_after____when_next_token_is_not_sudo()

Location:
  /home/tyler/Coollabsio/coolify/vendor/pestphp/pest/src/Factories/TestCaseFactory.php(170) : eval()'d code
  line 510

Also Coolify only supports installation and full operation as a root user, see https://coolify.io/docs/get-started/installation#_1-prepare-your-server

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Non-root user breaks proxy

2 participants