Escape reserved path characters in the remote images post-transform

[jayaddison]

Feature or Bugfix

• Bugfix

Purpose

• Allow remote image URLs that contain port number specifications to be saved correctly during the post-transform steps on Windows.

Detail

• Add the colon character (:) to the dictionary of character-to-path mappings in the ImageDownloader class; it isn't considered a valid path component character on Windows, meaning that attempts to create directories that include it in the name fail.

Relates

• Resolves Remote images: download of URIs containing port number details on Windows fails. #12100.

[jayaddison]

Perhaps this isn't ideal, particularly because it would affect the on-disk image download storage layout for non-Windows hosts. In particular: for hosts that had previously successfully image downloads in a http/localhost:7777/foo/bar directory path, then after this change, they would not discover that existing image content.

[jayaddison]

Perhaps this isn't ideal, particularly because it would affect the on-disk image download storage layout for non-Windows hosts. In particular: for hosts that had previously successfully image downloads in a http/localhost:7777/foo/bar directory path, then after this change, they would not discover that existing image content.

I've adjusted the logic to retain the colon symbol on POSIX systems, meaning that cache re-use should be possible there. Existing cached content will not have existed on Windows systems for these URLs, because the directory names would have been considered invalid and not created in the first place.

I still think we should consider replacing this logic entirely with requests-cache or similar, as I've mentioned in the issue thread (#12100). However, that feels like a more significant change, and should be a separate pull request - either as an alternative to this approach, or a subsequent follow-up.

[AA-Turner]

I've added all of the reserved characters from https://learn.microsoft.com/en-us/windows/win32/fileio/naming-a-file. This may be overkill, but better safe than sorry.

A

[jayaddison]

I've added all of the reserved characters from https://learn.microsoft.com/en-us/windows/win32/fileio/naming-a-file. This may be overkill, but better safe than sorry.

A

Thanks @AA-Turner. One thing I am slightly concerned about is that the : (and existing &) character is technically valid within the path segment of a URI -- so there's some, albeit maybe obscure, potential for websites that contain both foo/bar/image.png and foo:bar/image.png to resolve to the same image download cache filename here.

For long filenames, that's avoided due to the use of a sha1 on the complete URI - but that doesn't cover all cases.

I'd figured that perhaps adding colon-only, and only on Windows (where it hadn't been previously supported) would be a cautious improvement here -- but also that it could make sense to try to solve the problem more comprehensively in the not-too-distant future by using a well-tested library to handle all of this for us.

(you may already have understood / inferred all that, but I'd like to make sure)

[AA-Turner]

We could simply always use the sha1, or a hybrid whereby we use the sanitised URI Authority and path + the sha of the full path?

A

[jayaddison]

We could simply always use the sha1, or a hybrid whereby we use the sanitised URI Authority and path + the sha of the full path?

Ok, yep - it looks like I misunderstood some of the code; the sha1 for dirname (the post-translation node URI) occurs whenever the resulting string length is greater than 32. That's going to be the vast majority of cases, I think - especially because despite the variable name dirname, the value does include the filename part of the path (foo.png) (right?).

In that case, I think we should indeed always use sha1 on dirname -- removing the length conditional check -- and we should use the existing translations from v7.2.6 and earlier (? and &) so that cache re-use is maximized.

Does that seem safe and reasonable? (I'm not completely confident myself yet, this is relatively new code to me)