Handle symlinking edge cases #1660

withinboredom · 2025-06-18T19:39:18Z

This one is interesting — though I’m not sure the best way to provide a test. I will have to look into maybe an integration test because it is a careful dance between how we resolve paths in the Caddy module vs. workers. I looked into making a proper change (literally using the same logic everywhere), but I think it is best to wait until #1646 is merged.

But anyway, this change deals with some interesting edge cases. I will use gherkin to describe them:

Feature: FrankenPHP symlinked edge cases
  Background: 
    Given a `test` folder
    And a `public` folder linked to `test`
    And a worker script located at `test/index.php`
    And a `test/nested` folder
    And a worker script located at `test/nested/index.php`
  Scenario: neighboring worker script
    Given frankenphp located in the test folder
    When I execute `frankenphp php-server --listen localhost:8080 -w index.php` from `public`
    Then I expect to see the worker script executed successfully
  Scenario: nested worker script
    Given frankenphp located in the test folder
    When I execute `frankenphp --listen localhost:8080 -w nested/index.php` from `public`
    Then I expect to see the worker script executed successfully
  Scenario: outside the symlinked folder
    Given frankenphp located in the root folder
    When I execute `frankenphp --listen localhost:8080 -w public/index.php` from the root folder
    Then I expect to see the worker script executed successfully
  Scenario: specified root directory
    Given frankenphp located in the root folder
    When I execute `frankenphp --listen localhost:8080 -w public/index.php -r public` from the root folder
    Then I expect to see the worker script executed successfully

Trying to write that out in regular English would be more complex IMHO.

These scenarios should all pass now with this PR.

AlliBalliBaba · 2025-06-18T20:44:43Z

worker.go

+	absFileName, err := filepath.EvalSymlinks(o.fileName)
+	if err != nil {
+		return nil, fmt.Errorf("worker filename is invalid %q: %w", o.fileName, err)
+	}


I like it 👍 explicitly checking if a worker exists is something I actually wanted to do for a long time

Or wait, evalSymlinks will just evaluate symlinks. In that case it would also be nice to error right away if there's no file there. Can also be done in another PR though

it errors if the file doesn't exist (I had to update a failing test -- haven't pushed the commit yet)

AlliBalliBaba · 2025-06-18T20:49:05Z

caddy/module.go

+		//If we do not have a resolved document root, then we cannot resolve the symlink of our cwd because it may
+		//resolve to a different directory than the one we are currently in.
+		//This is especially important if there are workers running.
+		documentRootOption = frankenphp.WithRequestDocumentRoot(documentRoot, false)


Not checking for symlinks here seems like it might break some setups. Definitely would be nice to test all the edge cases somehow (should be possible with caddy_tests?).

Tbh though, I don't know what the current edge cases with symlinks are

I’m pretty sure we don’t want to evaluate symlinks here. In the case where a document root was expressly given (such as prod setups), we will never even enter this branch. In the case where a document root isn’t given, we need to be compatible with how we resolve workers. There is an edge case here that I didn’t cover, and I’m working out how to handle it: chained symlinks.

I suspect I’ll have to go back to the drawing board.

henderkes · 2025-06-19T13:04:01Z

do the edge cases only affect workers, not regular php requests?

caddy/module.go

worker.go

withinboredom · 2025-06-25T20:24:43Z

@henderkes yes, only workers. I should have a nice test suite this weekend. Manually testing this is annoying.

henderkes · 2025-12-29T10:17:20Z

@withinboredom should I pick this up or do you have something stashed away to commit?

withinboredom · 2025-12-31T08:46:22Z

TBH, I forgot this existed. If you want to take a gander, go for it!

henderkes · 2025-12-31T13:04:33Z

In adding a test manually after AI generated the bunch, of course the manually generated one errors. Edge case when using relative worker scripts:

localhost
php {
    root /path/to/symlink
    worker index.php
}

dunglas · 2025-12-31T23:08:31Z

caddy/caddy_test.go

+//	Given frankenphp located in the test folder
+//	When I execute `frankenphp php-server --listen localhost:8080 -w index.php` from `public`
+//	Then I expect to see the worker script executed successfully
+func TestSymlinkNeighboringWorkerScript(t *testing.T) {


Couldn't you use subtests instead a separated tests?

I'll refactor it tomorrow, thanks for the link.

Oh and Happy New Year!

Happy new year!! And thanks.

dunglas · 2025-12-31T23:11:28Z

testdata/symlinks/public

@@ -0,0 +1 @@
+test


Suggested change

test

test

This is a symlink, not a text file. I don't think we should be changing the content

testdata/symlinks/test/document-root.php

testdata/symlinks/test/index.php

testdata/symlinks/test/nested/index.php

Co-authored-by: Kévin Dunglas <[email protected]> Signed-off-by: Marc <[email protected]>

withinboredom added 2 commits June 18, 2025 21:10

fix #1637

581859b

add a comment

fd60f48

withinboredom requested review from AlliBalliBaba and dunglas June 18, 2025 19:39

AlliBalliBaba reviewed Jun 18, 2025

View reviewed changes

dunglas requested changes Jun 25, 2025

View reviewed changes

caddy/module.go Outdated Show resolved Hide resolved

worker.go Outdated Show resolved Hide resolved

henderkes added 2 commits December 31, 2025 12:35

Merge remote-tracking branch 'php/main' into HEAD

2227900

@dunglas suggestion

137f74c

henderkes added 3 commits December 31, 2025 14:07

add tests (last one is failing)

96a3692

fix case of relative worker script in a symlinked dir

ce674a9

oopsie with libphp, I wish we didn't hardcode -lphp :/

d1cec49

henderkes requested review from AlliBalliBaba and dunglas December 31, 2025 13:39

dunglas approved these changes Dec 31, 2025

View reviewed changes

henderkes and others added 3 commits January 1, 2026 00:54

Update testdata/symlinks/test/document-root.php

efc5627

Co-authored-by: Kévin Dunglas <[email protected]> Signed-off-by: Marc <[email protected]>

Update testdata/symlinks/test/index.php

eb6a4d9

Co-authored-by: Kévin Dunglas <[email protected]> Signed-off-by: Marc <[email protected]>

Update testdata/symlinks/test/nested/index.php

7de7f55

Co-authored-by: Kévin Dunglas <[email protected]> Signed-off-by: Marc <[email protected]>

Handle symlinking edge cases #1660

Are you sure you want to change the base?

Handle symlinking edge cases #1660

Uh oh!

Conversation

withinboredom commented Jun 18, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

henderkes commented Jun 19, 2025

Uh oh!

Uh oh!

Uh oh!

withinboredom commented Jun 25, 2025

Uh oh!

henderkes commented Dec 29, 2025

Uh oh!

withinboredom commented Dec 31, 2025

Uh oh!

henderkes commented Dec 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

henderkes commented Dec 31, 2025 •

edited

Loading