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

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants
@Jan-E
Contributor

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

This comment has been minimized.

Show comment Hide comment
@Jan-E

Jan-E Dec 5, 2017

Contributor

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.

Contributor

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 referenced this pull request in winlibs/cURL Dec 5, 2017

Closed

Curl 7.57.0 #7

@bagder

This comment has been minimized.

Show comment Hide comment
@bagder

bagder Dec 5, 2017

Member

/cc @phluid61

Member

bagder commented Dec 5, 2017

/cc @phluid61

@bagder bagder added the URL label Dec 5, 2017

@phluid61

This comment has been minimized.

Show comment Hide comment
@phluid61

phluid61 Dec 5, 2017

Contributor

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.

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

This comment has been minimized.

Show comment Hide comment
@Jan-E

Jan-E Dec 5, 2017

Contributor

Thanks for the review.

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

@bagder Could you merge it?

Contributor

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

This comment has been minimized.

Show comment Hide comment
@bagder

bagder Dec 5, 2017

Member

Merged, thanks!

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

URL: tolerate backslash after drive letter for FILE:
... as in "file://c:\some\path\curl.out"

Reviewed-by: Matthew Kerwin
Closes #2154
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment