FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Update UNC path handling by monkeyiq · Pull Request #2591 · simplesamlphp/simplesamlphp · GitHub

/ simplesamlphp Public

Update UNC path handling - #2591

Draft
monkeyiq wants to merge 3 commits into
simplesamlphp:simplesamlphp-2.5from
monkeyiq:2025/feb/unc-path-and-resolvePath
ConversationCommits (3)ChecksFiles changed
Draft

Update UNC path handling#2591
monkeyiq wants to merge 3 commits into
simplesamlphp:simplesamlphp-2.5from
monkeyiq:2025/feb/unc-path-and-resolvePath

Conversation

monkeyiq commented Feb 5, 2026

Copy link
Copy Markdown
Contributor

This

\\some-pc\some-share\some.key

Should result in

var_dump($ret);
string(29) "//some-pc/some-share/some.key"

Which according to
#2586 should be able to be loaded.

I only have a partial php win env so it needs verification against a real SSP install.

Update UNC path handling
125eebb 
This

```
\\some-pc\some-share\some.key
```

Should result in

```
var_dump($ret);
string(29) "//some-pc/some-share/some.key"
```

Which according to
simplesamlphp#2586
should be able to be loaded.

I only have a partial php win env so it needs verification against a
real SSP install.
monkeyiq mentioned this pull request Feb 5, 2026
Open

tvdijen commented Feb 5, 2026

Copy link
Copy Markdown
Member

If this works, we should capture this case in a unit test

monkeyiq commented Feb 5, 2026

Copy link
Copy Markdown
Contributor Author

I don't know if folks are running the httpd build for win most or something else these days. From my perspective apache makes it easier if that is commonish. I guess if I installed httpd at some stage and maybe set it up to rsync the code over to that box it would perhaps help with win testing.

test case
dc94e11 
These for absolute path on Win

monkeyiq commented Feb 11, 2026

Copy link
Copy Markdown
Contributor Author

I now have a simple test case for this on windows. I have used a hack based on DIRECTORY_SEPARATOR to know if we are running on Win, perhaps there is a DEFINE or something that is more direct to use here instead.

tvdijen reviewed Feb 12, 2026
View reviewed changes
Comment thread tests/src/SimpleSAML/Utils/SystemTest.php

/**
*/
public function testUNCpaths(): void

tvdijen Feb 12, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This is not right. If the separator is \\ then the UNC-path should also use backslashes?

monkeyiq Feb 13, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think the UNC path should, I was looking at first at the comment from the original issue which mentions that php 8.2 seems to allow //server/path as well and work #2586 (comment)

If we are looking to make the returned path use '\' then we could change the rebuild code at

simplesamlphp/src/SimpleSAML/Utils/System.php

Line 163 in 5cae09a

$ret .= '/';

and see what else falls over with that and try to get those ripple effects under control again.

tvdijen reviewed Feb 12, 2026
View reviewed changes
Comment thread tests/src/SimpleSAML/Utils/SystemTest.php Outdated

monkeyiq commented Feb 13, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

I guess I should look at installing apache on the win box for testing too. I assume nobody else on the core team is looking to test / run things on a win machine.

tvdijen commented Feb 13, 2026

Copy link
Copy Markdown
Member

[..] I assume nobody else on the core team is looking to test / run things on a win machine.

I don't have anything available that we can use right now. I intend to replace and improve my home lab, but with RAM prices going through the roof it'll have to wait.. Almost a thousand euro's for 64 GB :'(

tvdijen force-pushed the simplesamlphp-2.5 branch from d380e5a to e0d2141 Compare March 2, 2026 20:49
tvdijen force-pushed the simplesamlphp-2.5 branch 4 times, most recently from 2e9bad0 to 20f3d33 Compare March 14, 2026 12:31

monkeyiq commented Apr 23, 2026

Copy link
Copy Markdown
Contributor Author

I am moving this to a draft until I have a win install to test this on and/or feedback from the initial bug report is received.

monkeyiq marked this pull request as draft April 23, 2026 00:12
tvdijen force-pushed the simplesamlphp-2.5 branch from 1e5392e to 593b7f3 Compare June 1, 2026 16:38
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

tvdijen tvdijen left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

Back | FazBrowse Home | New Git URL