Skip to content

fix(devtools): refuse a webserver port the dev server does not own - #165

Open
Decipher wants to merge 1 commit into
fix/oauth-consent-permissionfrom
fix/start-false-success
Open

fix(devtools): refuse a webserver port the dev server does not own#165
Decipher wants to merge 1 commit into
fix/oauth-consent-permissionfrom
fix/start-false-success

Conversation

@Decipher

@Decipher Decipher commented Sep 7, 2026

Copy link
Copy Markdown
Member

.devtools/start reported success when its own PHP server never bound.

Reproduced

A foreign process on the dev port, then .devtools/start:

[TASK] Stopping previously started services, if any.
[TASK] Starting the PHP webserver (docroot: web/).
.
[ OK ] Server started successfully.
[TASK] Checking that the server can serve content.
[ OK ] Server can serve content.
[ OK ] BASE_URL written.
URL       : http://127.0.0.1:8899
Login     : http://127.0.0.1:8899/user/reset/1/...

Exit code 0. Meanwhile, /tmp/quickstart-drupal-php-server.log:

Failed to listen on 127.0.0.1:8899 (reason: Address already in use)

and the pid in the pidfile was already dead. So the script wrote a wrong
BASE_URL into ../.env for the Nuxt side and printed a login link
pointing at a stranger's server.

Why

Both checks asked the port a question only the port could answer:

  • readiness was fsockopen($host, $port), which succeeds against whatever
    holds the port;
  • serving was get_headers() on the same address, likewise.

Neither could tell our server from someone else's. stop_webserver() does
not close the gap, and should not: it deliberately kills only php -S
processes rather than shooting at whatever it finds on the port.

The fix

Two independent nets.

  1. Refuse a port we do not own. After stop_webserver() has cleared our
    own server away, fail if the port is still answering, naming the process
    that holds it. A killed server needs a moment to release the port, so it
    gets a 3-second grace period before it counts as somebody else's.
  2. Wait on the process, not the port. The readiness loop now also reads
    the pidfile and stops as soon as that pid is gone, printing the server
    log. A bind that fails with the port free (a race, a permission problem)
    used to burn the full timeout and then report "Unable to start inbuilt
    PHP server" with no reason; it now reports the reason in about a second.

New helpers: port_is_open(), port_holder(), pid_is_running().

Also fixed while in there: the failure path passed the server log as
sprintf's format string, so a % in a PHP error message would have
broken the error report.

Guardrails, verified in both directions

New test_start_guardrails job. It needs PHP and the scripts only, since
both cases fail before Drupal is touched, so it skips assemble and
provision and runs in seconds.

Check Asserts
A port held by another process is refused non-zero exit, "already in use by another process", and ../.env was not written
A failed bind is reported, not waited out non-zero exit, names the exiting process and the OS reason

A third assertion checks the squatter is still alive afterwards. A guard
that clears the port by killing whatever holds it would pass every other
check here while being worse than the bug; stop_webserver() targets only
php -S processes, and this is what pins that.

Against the fixed scripts all three pass. The happy path was re-run on a
real provisioned backend and is unchanged.

Proving the red direction, and how not to. Non-zero exit is not proof a
guard fired: a crash is also non-zero. A stripped pre-fix copy placed outside
.devtools/ dies on require_once __DIR__ . '/helpers.php' and exits 255
before reaching any of the code under test, which an exit-code assertion
reads as the guard working. So the copy has to sit inside .devtools/
alongside its own helpers.php, and the assertion has to be on the message.
Done that way here, the pre-fix scripts print [ OK ] Server started successfully and reach ENVIRONMENT READY with exit 0, which is the real
failure the guardrail is written against.

The committed guardrail is already message-based in the green direction:
grep -q 'already in use by another process' fails on a crash as well as on
a false success, so a broken script cannot pass it quietly.

Not in this change

find_free_port() still tests a port by connecting to it rather than by
binding it, so it can hand back a port that is unbindable on the target
interface. That is now a loud failure rather than a false success, which is
why it is left alone here.

start also kills whatever server the pidfile names, whatever port is
being requested, so running it with a different WEBSERVER_PORT stops the
server from the previous run. That is defensible as "one server per
checkout", and unchanged, but it is a sharp edge worth knowing about.

@coderabbitai

coderabbitai Bot commented Sep 7, 2026

Copy link
Copy Markdown

Warning

Review limit reached

Next included review available in 59 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Team

Run ID: f36b14e5-3d6d-4ffe-a1b3-29a9071ea009

📥 Commits

Reviewing files that changed from the base of the PR and between 140a9cc and 2103a3c.

📒 Files selected for processing (4)
  • .cspell-project-words.txt
  • .github/workflows/ci.yml
  • drupal/.devtools/helpers.php
  • drupal/.devtools/start

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@codecov

codecov Bot commented Sep 7, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.70%. Comparing base (140a9cc) to head (2103a3c).

Additional details and impacted files
@@                      Coverage Diff                      @@
##           fix/oauth-consent-permission     #165   +/-   ##
=============================================================
  Coverage                         90.70%   90.70%           
=============================================================
  Files                                13       13           
  Lines                              2217     2217           
  Branches                            104      104           
=============================================================
  Hits                               2011     2011           
  Misses                              201      201           
  Partials                              5        5           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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.

1 participant