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

urlapi: UNC paths in file: URLs on Windows #7366

Closed
wants to merge 1 commit into from

Conversation

sergio-nsk
Copy link
Contributor

  • file://host.name/path/file.txt is a valid UNC path \host.name\path\files.txt
    to a non-local file transformed into URI (RFC 8089 Appendix E.3)

  • UNC paths on other OSs must be smb: URLs

  • file://host.name/path/file.txt is working in browsers, should work in cURL also

@ghost
Copy link

ghost commented Jul 8, 2021

Congratulations 🎉. DeepCode analyzed your code in 3.084 seconds and we found no issues. Enjoy a moment of no bugs ☀️.

👉 View analysis in DeepCode’s Dashboard | Configure the bot

👉 The DeepCode service and API will be deprecated in August, 2021. Here is the information how to migrate. Thank you for using DeepCode 🙏 ❤️ !

If you are using our plugins, you might be interested in their successors: Snyk's JetBrains plugin and Snyk's VS Code plugin.

@bagder bagder added URL Windows Windows-specific labels Jul 8, 2021
@bagder
Copy link
Member

bagder commented Jul 8, 2021

file://host.name/path/file.txt is working in browsers, should work in cURL also

And does it with this change?

There's no test case updates in this PR, which probably explains the test failures on Windows.

Shouldn't docs/URL-SYNTAX.md also be updated accordingly?

@sergio-nsk
Copy link
Contributor Author

And does it with this change?

Yes, it does. I tested it locally in my environment using windows shared paths like file://192.168.1.25/shared/path/...

There's no test case updates in this PR, which probably explains the test failures on Windows.

I should look at the broken tests. Perhaps the tests expecting the previous CURLUE_MALFORMED_INPUT fail now.

Shouldn't docs/URL-SYNTAX.md also be updated accordingly?

Perhaps yes, should be updated.

@sergio-nsk sergio-nsk force-pushed the curlapi branch 2 times, most recently from b0c8b6b to 6468e00 Compare July 10, 2021 00:02
@bagder
Copy link
Member

bagder commented Jul 30, 2021

This seems to have broken test 2080 on many windows builds?

@sergio-nsk
Copy link
Contributor Author

sergio-nsk commented Jul 30, 2021

This seems to have broken test 2080 on many windows builds?

file://dev/null is a valid UNC path \\dev\null, thus I tried to skip this test on Windows, but it did not help:

*** 17,22 ****
--- 17,26 ----
 <server>
 none
 </server>
+ <!-- valid UNC path on win32 -->
+ <features>
+ !Win32
+ </features>
  <name>
 config file with overly long option
  </name>

Where am I wrong?
It seems !Win32 must be !win32. I have updates the change set.

@bagder
Copy link
Member

bagder commented Jul 30, 2021

file://dev/null is a valid UNC path

You mean invalid ?

Why is that an invalid URL? It seems to match the syntax. The test case will never actually use the URL for anything but the parsing of the URL should succeed...

@sergio-nsk
Copy link
Contributor Author

sergio-nsk commented Jul 30, 2021

file://dev/null is malformed file: URL in Unix. This is a valid file: URL in Windows.
Valid paths in Unix: file:///dev/null, a traditional file URI for a local file, or file:/dev/null, the minimalistic file URI for a local file.
file://dev/null is a UNC filename in Windows, encodes a host name dev and a share name null, a file path is \\dev\null.
cURL accepts only file://localhost/null or file://127.0.0.1/null as a valid file: URL with a host in Unix. Any host should be accepted in Windows.

@bagder
Copy link
Member

bagder commented Jul 30, 2021

Hm, wrong of me, the URL parser fails (and should fail) on the URL already without your PR... The test case assumes it!

@bagder
Copy link
Member

bagder commented Jul 30, 2021

You should not disable test 2080 on Windows. You're just hiding a problem as it should work on there too.

@sergio-nsk
Copy link
Contributor Author

sergio-nsk commented Jul 30, 2021

Hm, wrong of me, the URL parser fails (and should fail) on the URL already without your PR... The test case assumes it!

Right, although file://dev/null is a syntactically valid URL, it is useless in Unix with a host name dev, thus the parser returns CURLE_URL_MALFORMAT and the test ensures this. But the path \\dev\null is valid in Windows and the parser returns CURLE_OK in Windows in my PR.

@sergio-nsk
Copy link
Contributor Author

sergio-nsk commented Jul 30, 2021

You should not disable test 2080 on Windows. You're just hiding a problem as it should work on there too.

Is it possible to make a conditional errorcode?

<errorcode>
3
</errorcode>

I can add a test data test7366 for Windows based on test 2080 with

<errorcode>
0
</errorcode>

@bagder
Copy link
Member

bagder commented Aug 8, 2021

You should rather modify the URL so that it fails the same independently of operating system!

@sergio-nsk
Copy link
Contributor Author

I will look what URL in the test can be suitable. I am afraid, a new URL will get a different code flow than expected with using file://dev/null in the test.

@bagder
Copy link
Member

bagder commented Aug 15, 2021

Why? You just have to make the URL parse fail consistently on all platforms. How about perhaps adding a white space? file://de v/null"

@sergio-nsk
Copy link
Contributor Author

sergio-nsk commented Aug 15, 2021

Good idea! Whitespaces may be disallowed in file-host names in UNC path, so file://de v/null should work for the test case. I will check it.

@sergio-nsk sergio-nsk force-pushed the curlapi branch 3 times, most recently from 28a49ec to 5e7bc09 Compare August 16, 2021 04:28
@bagder
Copy link
Member

bagder commented Aug 17, 2021

Lovely!

How about adding a few tests of this to lib1560.c as well within #ifdef win32 ? We need to make sure that the basics work (and keep working in the future) and that some obvious bad formats are rejected.

@bagder bagder self-assigned this Aug 17, 2021
@sergio-nsk
Copy link
Contributor Author

sergio-nsk commented Aug 17, 2021

I will do it soon.

Regular DOS or Windows file URIs with vertical line characters in the drive letter construct. For example:

  • file:///c|/path/to/file
  • file:/c|/path/to/file
  • file:c|/path/to/file

@bagder, would you like cURL to support such URIs? The top web browsers support such file: URIs.

@bagder
Copy link
Member

bagder commented Aug 18, 2021

@bagder, would you like cURL to support such URIs? The top web browsers support such file: URIs.

To my knowledge users haven't asked for this - I can't recall anyone at least. Neither UNC paths nor support for | in the file URLs, so this is not something we must have... It also seems sensible to do the | handling in a separate PR.

- file://host.name/path/file.txt is a valid UNC path \\host.name\path\files.txt
  to a non-local file transformed into URI (RFC 8089 Appendix E.3)

- UNC paths on other OSs must be smb: URLs
@bagder bagder added the feature-window A merge of this requires an open feature window label Sep 6, 2021
@bagder bagder closed this in 4b99762 Sep 27, 2021
@bagder
Copy link
Member

bagder commented Sep 27, 2021

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-window A merge of this requires an open feature window URL Windows Windows-specific
Development

Successfully merging this pull request may close these issues.

None yet

2 participants