Fail gracefully when windows path has quotes or is otherwise invalid

[loosebazooka]

Apparently windows paths are allowed to have " in them, see GoogleCloudPlatform/app-maven-plugin#199

We probably need to fail ignore this a little more gracefully in https://github.com/GoogleCloudPlatform/appengine-plugins-core/blob/master/src/main/java/com/google/cloud/tools/appengine/cloudsdk/PathResolver.java

[patflynn]

I think the actual bug is that we should not let a bad path entry in the path stop the search. Ideally we would log a warning when the path entry is invalid though.

…

On Tue, Jun 27, 2017 at 4:53 PM, Appu Goundan ***@***.***> wrote: Apparently windows paths are allowed to have " in them, see GoogleCloudPlatform/app-maven-plugin#199 <GoogleCloudPlatform/app-maven-plugin#199> We probably need to fail a little more gracefully in https://github.com/ GoogleCloudPlatform/appengine-plugins-core/blob/master/src/ main/java/com/google/cloud/tools/appengine/cloudsdk/PathResolver.java — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#427>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHf5HdV1UUKfe9KsjNmcO346Vxjanl3Fks5sIWuvgaJpZM4OHLP6> .

[chanseokoh]

Apparently windows paths are allowed to have " in them

Yes, AFAIK, a path containing " such as "C:\Users\DPE Admin\Downloads\apache-maven-3.5.0\bin"\gcloud could be a valid path on Windows. It's not a bad path, so why do we want to fail in this case?

[loosebazooka]

I believe I used "fail" incorrectly, but path.get seems to throw an exception.

[elharo]

I'll have to check but I believe the path is invalid. I.e. Windows paths may not contain a double quote. However as @patflynn says we can handle this more gracefully.

[elharo]

https://msdn.microsoft.com/en-us/library/windows/desktop/aa365247(v=vs.85).aspx confirms that the quote is not legal in Windows paths.

[chanseokoh]

The doc says you can't create a directory name or a file name containing a quote, which is pretty much expected. However, the situation is that we have quoting in describing a path (e.g., C:\"Program Files (x86)"\Common Files\), and it's not clear how this should be handled in this context or if it's a client's fault that did not remove the quotes. (Typical Windows applications don't seem to have problems interpreting this path string.)

What is important, I think, is that Windows seems to naturally put quotes when defining path environment variables as @lesv said (GoogleCloudPlatform/app-maven-plugin#199 (comment)), so many users might face the same situation. But it's still not clear if it is a fault of some upper layer that should have stripped out the quotes.

[elharo]

In context this comes directly from the PATH environment variable. Until proven otherwise, I believe this environment variable should not contain quotes. If it does, that's simply an uncommon config error.

[chanseokoh]

@lesv, are you sure setting an env variable on Windows using the Browse... button puts quotes? In my Windows testing (Windows Server 2016) on GCE, it doesn't put quotes. If it still does, which version do you use?

[lesv]

It did - Windows 10.

[lesv]

(I sure didn't put it there)

[elharo]

Can someone find definitive Windows docs on this? It sounds very strange.

[lesv]

Note - the fact I saw the double quotes in when setting the path doesn't mean that it put it there on the result. I also tried removing the quotes with edit, and I continued to get the same error message.

[lesv]

I'm suspecting that whatever is used to split the PATH might be adding the quotes in.

[elharo]

FYI: not normative but suggestive nonetheless: https://stackoverflow.com/questions/34669912/quotes-in-windows-environment-variable-value

[elharo]

and https://serverfault.com/questions/349179/path-variable-and-quotation-marks-windows

[lesv]

Elliotte, I think your going about this backwards.

I just followed a simple process to add a path. It gave me something that broke our code. Even if Microsoft isn't conforming to its docs, it was still a reasonable path for me to have followed, our users are likely to also experience this.

[chanseokoh]

In my Windows testing (Windows Server 2016) on GCE, it doesn't put quotes.

Okay, it didn't put quotes when I was modifying a user PATH env var, but I can see that it does put quotes when modifying the system PATH env var:

The env variable does contain quotes for my new entry ...;"C:\Users\chanseok\Desktop\New folder";... as below:

C:\Users\chanseok\Desktop\appengine-plugins-core>echo %PATH%
C:\Program Files (x86)\Google\Cloud SDK\google-cloud-sdk\bin;C:\ProgramData\Oracle\Java\javapath;C:\windows\system32;C:\windows;C:\windows\System32\Wbem;C:\windows\System32\WindowsPowerShell\v1.0\;C:\ProgramData\GooGet;C:\Program Files (x86)\Google\Cloud SDK\google-cloud-sdk\bin;C:\Program Files\Compute Engine\sysprep;C:\Program Files\Google\Compute Engine\sysprep\;C:\Program Files\Google\Compute Engine\metadata_scripts\;"C:\Users\chanseok\Desktop\New folder";C:\Users\chanseok\AppData\Local\Microsoft\WindowsApps;;

Then I can reproduce this problem (mvnw package in an appengine-plugins-core repo is enough to trigger this):

Tests run: 5, Failures: 0, Errors: 1, Skipped: 3, Time elapsed: 0.015 sec <<< FAILURE!
testResolve(com.google.cloud.tools.appengine.cloudsdk.PathResolverTest)  Time elapsed: 0.015 sec  <<< ERROR!
java.nio.file.InvalidPathException: Illegal char <"> at index 0: "C:\Users\chanseok\Desktop\New folder"\gcloud

What is funny is that, the second time I open the Windows PATH env editor, the editor doesn't show the quotes.

[elharo]

I'd still like to see normative docs on Windows environment variables so we know how to properly parse this, and what other problems might be lurking. Maybe @csells knows?