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

Fix file://c:\some\path\curl.out #2154

Closed
wants to merge 1 commit into from
Closed

Fix file://c:\some\path\curl.out #2154

wants to merge 1 commit into from

Conversation

Jan-E
Copy link
Contributor

@Jan-E Jan-E commented Dec 5, 2017

Sequel to #1187 (comment)

The issue is essentially the same as in the opening post of #1187 (comment)
file://c:\some\path\curl.out
is rejected wih error message:

curl: (3) Invalid file://hostname/, expected localhost or 127.0.0.1 or none

file://c:/some/path/curl.out
is accepted. This is apparently intentional, according to the comment in https://github.com/curl/curl/blob/master/lib/url.c#L2121

   * Additionally, there is an exception for URLs with a Windows drive
   * letter in the authority (which was accidentally omitted from RFC 8089
   * Appendix E, but believe me, it was meant to be there. --MK)

PHP uses the backslash format for file:// URL's. This PR aims to include URL's like file://c:\some\path\curl.out as well.

@Jan-E
Copy link
Contributor Author

Jan-E commented Dec 5, 2017

I have read the remark in #1187 (comment) that backslashes in URL's are forbidden by both [RFC1738] and [RFC3986]. Hence, it would be better to fix the issue in PHP. @weltling intends to do that for the actively maintained PHP versions (PHP 7.1 and PHP 7.2), but the fix will not be backported to versions like PHP 5.6 (which will receive security fixes only). If this PR is merged users of PHP 5.6 can also enjoy the enhancements of the latest curl version.

@Jan-E Jan-E mentioned this pull request Dec 5, 2017
@bagder
Copy link
Member

bagder commented Dec 5, 2017

/cc @phluid61

@bagder bagder added the URL label Dec 5, 2017
@phluid61
Copy link
Contributor

phluid61 commented Dec 5, 2017

tl,dr: LGTM

I will be honest, I wasn't sure how to approach the issue of backslashes when I submitted #2110.

Some observations:

  1. the STARTS_WITH_DRIVE_PREFIX macro only scans as far as ^[A-Z]:, which means it would match any slash, or indeed any character at all, after the colon -- I'm hesitant to touch that, because of the specific use-case for that macro
  2. I included the final anchor (slash or NULL) in the STARTS_WITH_URL_DRIVE_PREFIX macro to ensure it didn't match falsely against random guff like file://x:445/share/file, which could be a legitimate SMB share
  3. It recognises the pipe character (e.g. c|) because that is explicitly handled in lib/file.c
  4. looking at the routine that converts forward slashes to backslashes, it also implicitly accepts backslash path separators in DOS/Windows
  5. in not-Windows, this patch only changes the error message from "bad hostname" to "only MSDOS/Windows" (both CURLE_URL_MALFORMAT), which is either an innocuous change or an improvement

I think 3 and 4 above are enough justification for accepting the patch.

@Jan-E
Copy link
Contributor Author

Jan-E commented Dec 5, 2017

Thanks for the review.

I think 3 and 4 above are enough justification for accepting the patch.

@bagder Could you merge it?

@bagder
Copy link
Member

bagder commented Dec 5, 2017

Merged, thanks!

@bagder bagder closed this in b261c44 Dec 5, 2017
JohnDeHelian pushed a commit to JohnDeHelian/curl that referenced this pull request Dec 7, 2017
... as in "file://c:\some\path\curl.out"

Reviewed-by: Matthew Kerwin
Closes curl#2154
@lock lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019
nbbrooks added a commit to nbbrooks/resource_retriever that referenced this pull request Feb 3, 2020
Pulls in this fix curl/curl#2154

Fixes resource retrieval in Windows
nbbrooks added a commit to nbbrooks/resource_retriever that referenced this pull request Feb 3, 2020
Pulls in this curl fix curl/curl#2154 to fix resource retrieval in Windows
nbbrooks added a commit to nbbrooks/resource_retriever that referenced this pull request Feb 3, 2020
Pulls in curl fix curl/curl#2154 to fix resource retrieval in Windows
wjwwood pushed a commit to ros/resource_retriever that referenced this pull request Feb 3, 2020
Pulls in curl fix curl/curl#2154 to fix resource retrieval in Windows
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

Successfully merging this pull request may close these issues.

None yet

3 participants