Implement -Extension parameter in New-TemporaryFile cmdlet #4612

bergmeister · 2017-08-18T11:51:40Z

#4215: Add ability to create a temporary file with a specific extension to New-TemporaryFile
Implemented exactly as described by adding an optional -Extension parameter to specify the extension e.g. as .csv or just csv.
Because of the required change to call different .NET APIs, the cmdlet had to be slightly rewritten to cater for the special case that the file to be created already exists in the temp folder.

…c extension to New-TemporaryFile using a new -Extension parameter.

mklement0 · 2017-08-18T15:05:22Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/NewTemporaryFileCommand.cs

+                    return;
+            }
+
+            if (ShouldProcess(filePath))


While this implicitly fixes the bug in the original code that referred to environment variable TEMP, which can only be assumed to exist in Windows, note that you've changed the logic from reporting the directory path to the file path.

In other words: to remain consistent with how things worked before, it should remain if (ShouldProcess(tempPath))

I guess the logic is: New-TemporaryFile operates on the temp. directory as its target, and creates a new file inside it.

What if: Performing the operation "New-TemporaryFile" on target "C:\Users\jdoe\AppData\Local\Temp".

Ok. Makes sense.

mklement0 · 2017-08-18T15:09:46Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/NewTemporaryFileCommand.cs

+                string fileName = Path.GetRandomFileName();
+                fileName = Path.ChangeExtension(fileName, _extension);
+                filePath = Path.Combine(tempPath, fileName);
+                if(!File.Exists(filePath))


Unfortunately, you can run into race conditions with this approach: Between the time you check for the existence of a file with a given name and a subsequent attempt to create it, someone else could have created such a file, at least hypothetically.

The more robust approach is to attempt to create the file and let that attempt fail if a file with that name happens to exist already (which is what file mode CreateNew does), catch the resulting IOException exception, and try again.

I was aware of this when writing it but did not consider handling it specifically since the global catch would have still handled it and the probability of having a clash of a random file name in such a short time period is really low. However, I liked your idea and as your approach does not seem to have a downside, I implemented your suggestion.

… file could get created matching the random file name in between the point of checking its presence and its creation. Revert SupportsShouldProcess behaviour to report temp path. Those changes were requested in the code review of PR PowerShell#4612

mklement0 · 2017-08-18T17:39:34Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/NewTemporaryFileCommand.cs

                }
-                catch (Exception e)
+
+                if (!creationOfFileSuccessful)
                {
                    WriteError(


Thanks for the quick turnaround.

The following, which predates your PR, may not be worth doing, because it is technically a breaking change, but it's worth pointing out:

Based on the official cmdlet error-reporting guidelines, this should be a (statement)-terminating error: "Cmdlets that can accept only 0-1 input objects and generate only 0-1 output objects should treat errors as terminating errors and generate terminating exceptions." - https://msdn.microsoft.com/en-us/library/ms714412%28v=vs.85%29.aspx

cc @SteveL-MSFT

Thanks for the quick review as well. 👍 You have an interesting point. My thinking is: If one has got a script in which a temporary file gets created, then in order to use it, one would need to use the returned object because otherwise one does not know where the temporary file is. Basically, the cmdlet is only useful if one receives a returned object. I am not sure if this is a case of the users having to check that that their variable is not $null or that the error should be terminating.

I agree that with @mklement0 a terminating error should be thrown. But it would be a breaking change and so we should continue with the existing pattern.

Understood, @PaulHigin.

@bergmeister: Just to clarify: by default, the script would continue to run even if a terminating error were reported with ThrowTerminatingError(), because only the statement is terminated; the difference between reporting a non-terminating vs. a terminating error only matters with respect to -ErrorAction use and try / catch use.

Since the conceptual fog around error handling lifted for me personally only recently, perhaps you'll find this useful too: Our Error Handling, Ourselves - time to fully understand and properly document PowerShell's error handling

Another point is to follow the standard, make the breaking change and "fix" it in our interop module.

@lzybkr Could you please comment - should we make the breaking change or exclude it?

I think a terminating error is appropriate and an unlikely breaking change, but we can make that change in a different PR.

Is there already an issue for the terminating error change?

@SteveL-MSFT Now there is: here
I had a look at the other suggestions about how to improve my tests. I can certainly make those but it would be more important if someone could make a decision or approval whether the currently used algorithm is OK or if not how it should be modified.

dantraMSFT · 2017-08-18T18:05:39Z

test/powershell/Modules/Microsoft.PowerShell.Utility/NewTemporaryFile.Tests.ps1

-        {
-            Remove-Item $tempFile -ErrorAction SilentlyContinue -Force
-        }
+        Remove-Item $tempFile -ErrorAction SilentlyContinue -Force


Suggestion: To make cleanup reliable, use 'try' around the Should statements and place 'Remove-Item' in the finally block.

Ok. Will do.

PaulHigin · 2017-08-18T18:12:13Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/NewTemporaryFileCommand.cs

@@ -12,34 +12,73 @@ namespace Microsoft.PowerShell.Commands
    [OutputType(typeof(System.IO.FileInfo))]
    public class NewTemporaryFileCommand : Cmdlet
    {
+        private string _extension = ".tmp";


Please remove the _extension field and just use the property:

[Parameter(Position = 0)] [ValidateNotNullOrEmpty] public string Extension { get; set; } = ".tmp";

It is CoreFX default - we can exclude this and simply check that the parameter presents.

I was thinking the same but rather followed the existing style. What about a more elegant one-liner?

public string Extension { get; set; } = ".tmp";

We can directly use Path.GetRandomFileName() if the parameter absent.

@iSazonov I am not sure if you meant GetTempFileName() because Path.GetRandomFileName() gives you a random extension and would therefore be a breaking change. If you really meant GetTempFileName then I disagree because this function does not just get you a file name, it also creates it for you, which means we would end up with 2 different ways of creating a temp file with 2 different error handlings...

Yes, I meant that

if (Extension parameter absent) { return Path.GetFileName() } else { Here new code to create a temp file with explicit extension. }

We should follow CoreFX if we can. Here we can. Extension parameter will be used rarely so it is good two code paths.

PaulHigin · 2017-08-18T18:12:22Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/NewTemporaryFileCommand.cs

+                int attempts = 0;
+                bool creationOfFileSuccessful = false;
+
+                // In case the random temporary file already exists, retry 


I am not convinced that this retry loop is necessary. What are the odds that the random file name already exists?

We can use approved pattern

PowerShell/src/System.Management.Automation/utils/PathUtils.cs

Line 420 in 02b5f35

internal static DirectoryInfo CreateTemporaryDirectory()

The odds are low but we could make them really low by using e.g. Guids for the file name instead of Path.GetRandomFileName() because the latter is only 8 characters long...

I think we should follow CoreFX while we can and fix issues there.

PaulHigin · 2017-08-18T18:16:09Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/NewTemporaryFileCommand.cs

                }
-                catch (Exception e)
+
+                if (!creationOfFileSuccessful)
                {
                    WriteError(


I agree that with @mklement0 a terminating error should be thrown. But it would be a breaking change and so we should continue with the existing pattern.

PaulHigin · 2017-08-18T18:19:30Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/NewTemporaryFileCommand.cs

-                            "NewTemporaryFileWriteError",
-                            ErrorCategory.WriteError,
-                            tempPath));
+                            new IOException("Could not find an available temporary file name in {tempPath}."),


This exception message string should go into a resx file so it can be localized. I think this would be the right file: Microsoft.PowerShell.Commands.Utility\resources\UtilityCommonStrings.resx

PaulHigin · 2017-08-18T18:20:19Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/NewTemporaryFileCommand.cs

@@ -12,34 +12,73 @@ namespace Microsoft.PowerShell.Commands
    [OutputType(typeof(System.IO.FileInfo))]
    public class NewTemporaryFileCommand : Cmdlet


Please add a comment section here that describes this public property.

dantraMSFT · 2017-08-18T18:28:52Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/NewTemporaryFileCommand.cs

+        /// </summary>
+        [Parameter(Position = 0)]
+        [ValidateNotNullOrEmpty]
+        public string Extension


Consider validating the value in the setter for valid characters instead of rely only on an exception in your EndProcessing implementation. Something like...

if (!string.IsNullOrEmpty() && value.IndexOfAny(Path.GetInvalidFileNameChars() != -1)
{
throw...
}

…#4612: - Check for invalid characters in Extension parameter - Simplify Extension property and document it in the class documentation. - Use resource strings for error messages - Improved tests to improve the test cleanup using a try-finally approach as suggested in the code review of PR PowerShell#4612. What is left to be discussed is the error handling in the case of a file conflict in the temp folder (i.e. should we retry at all or use a different approach)

mklement0 · 2017-08-18T21:06:58Z

src/Microsoft.PowerShell.Commands.Utility/resources/UtilityCommonStrings.resx

@@ -174,4 +174,10 @@
  <data name="TypeNotSupported" xml:space="preserve">
    <value>'{0}' is not supported in this system.</value>
  </data>
+  <data name="InvalidCharacterInParameter" xml:space="preserve">
+    <value>Parameter '{0}' contains at least one invalid character. It's value is: '{1}'</value>


Please change "it's" to "its".

Oops. Sure.

bergmeister · 2017-08-18T21:49:29Z

I have addressed the main suggestions but would like to wait until we have reached a consensus on the preferred approaches with the issues of the possibility of not being able to create a temp file and how to deal with it

We could use a Guid for the filename to minimise the probability of a file name clash instead of Path.GetRandomFileName(). @iSazonov has suggested to use this existing pattern as an alternative but I struggle to see why as it suffers from the same problem and also does not deal with other exceptions due to e.g. insufficient permissions, disk space. Maybe you can elaborate more on it?
We could simply let it fail and not have a retry mechanism

I think in general it is good that we move away from Get-TempFileName due to it's limitations (see e.g.
here) but I am wondering if this is not a also a case for the coreclr to improve the current implementation and possible offer an extension overload...

I find it great that many people share their thoughts but at the end I would like to have a final word from someone before I make a commit to avoid going back and forth.

PaulHigin · 2017-08-18T22:43:36Z

A GUID based filename would certainly solve the problem but after thinking about it some more I am Ok with the retry loop. It feels like a reasonable compromise to write an error and fail after 10 tries. Also I am fine with initializing the new property with ".tmp". Using GUIDs feels like overkill. But maybe we could add a switch to allow it as an option. Something like:

New-TemporaryFile -GuidBasedName

…xtension parameter in New-TemporaryFile as part of PR PowerShell#4612 In Pester v3 the 'Should Throw' assertion is quite limited, see here: pester/Pester#673 Therefore the example in the guide here was followed: https://github.com/PowerShell/PowerShell/blob/68bcd4b5284bb169c52a570afbfdd1bb70d0de10/docs/testing-guidelines/WritingPesterTests.md

bergmeister · 2017-08-18T23:52:26Z

@PaulHigin
Yes, we could easily add a -GuidBasedName switch parameter without making the code complicated.

Just to recap: Before this PR, the cmdlet was simply calling GetTempFileName and letting it handle the error. When you look at the (Windows) implementation of GetTempFileName here then all it does is call the Interop.Kernel32.GetTempFileNameW method here, which directly calls the Windows API here, which states:

Due to the algorithm used to generate file names, GetTempFileName can perform poorly when creating a large number of files with the same prefix. In such cases, it is recommended that you construct unique file names based on GUIDs.
... This limits GetTempFileName to a maximum of 65,535 unique file names if the lpPathName and lpPrefixString parameters remain the same.
(NB: The API is called with lpPathName are lpPrefixString remaining the same)

To conclude: Without the retry logic, we would have not changed the error behaviour of the cmdlet because it was always just throwing if there is a problem (the Unix implementation of GetTempFileName here is similar and would suffer from the same problem).
The final conclusion seems to be:

The retry mechanism improves the existing behaviour in case of an unfortunate clash and therefore reduces sporadic failures
The Guid based approach would makes it really robust and also make it possible to have more than 65,535 files in the temp folder.

Therefore I also agree that keeping the retry mechanism improves the error behaviour without making breaking changes to its past behaviour and the -GuidBasedName switch would provide a more robust option going forward.

…l#4612

iSazonov · 2017-08-19T20:49:42Z

@PaulHigin

A GUID based filename would certainly solve the problem but after thinking about it some more I am Ok with the retry loop.

Why do you think this problem exists?
Why hasn't this been corrected in .Net Framework in the last 15 years and has not been corrected in CoreFX now?
Do we actually have scripts that create more than 50000 temporary files without removing them?
Are there real support cases on this problem?
Does anyone really need temporary file names based on GUIDs?

If so, right way is - we have to open Issue in CoreFX and fix the problem there without complicating PowerShell Core code.

iSazonov · 2017-08-19T20:59:26Z

test/powershell/Modules/Microsoft.PowerShell.Utility/NewTemporaryFile.Tests.ps1

+        }
+        finally
+        {
+            Remove-Item $tempFile -ErrorAction SilentlyContinue -Force


We use this multiple times - it is good candidate for AfterEach .

Ok. But I will wait with updating the tests for the moment as it is more important to know the final decision first whether the implementation would get accepted or if it requires a fix in corefx first.

iSazonov · 2017-08-20T10:21:56Z

test/powershell/Modules/Microsoft.PowerShell.Utility/NewTemporaryFile.Tests.ps1

+            }
+            catch
+            {
+                $_.FullyQualifiedErrorId | Should Be "NewTemporaryInvalidArgument,Microsoft.PowerShell.Commands.NewTemporaryFileCommand"


Currently we use our pattern:

{ New-TemporaryFile -Extension $invalidFileNameChar -ErrorAction Stop } | ShouldBeErrorId "NewTemporaryInvalidArgument,Microsoft.PowerShell.Commands.NewTemporaryFileCommand"

Ok, can do. I was following the example given in the PowerShell Pester guideline here. Should the guideline then be updated as well?

Yes. Could you please open new Issue for that?

I'd prefer if you open the issue because the person opening an issue or PR is implicitly responsible for arguing and leading the discussion whether the guideline is out of date and/or should be changed. Therefore you are definitely the better person for that.

person opening an issue or PR is implicitly responsible for arguing and leading the discussion

No, we welcome any error report - you don't have to "leading the discussion". Every time you see a bug - feel free to open new Issue (and forget it 😄) MSFT experts will help to open a Issue if the issue is difficult to understand.

iSazonov · 2017-08-20T11:01:53Z

After further thinking, I believe that if the CoreFX algorithm is bad, it must be corrected in CoreFX, not here.
So our code should be simple:

try
{
    var tempFile = GetTempFileName();
    if (parameter Extension present)
    {
         replace extension in tempFile
    }

    WriteObject(tempFile);
}
catch
{
    WriteError(...) or write terminating error;
}

iSazonov · 2017-08-20T11:03:01Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/NewTemporaryFileCommand.cs

        /// </summary>
        protected override void EndProcessing()
        {
+            // Check for invalid characters in extension
+            if (!string.IsNullOrEmpty(Extension) && Extension.IndexOfAny(Path.GetInvalidFileNameChars()) != -1)


I guess it makes sense if we want to fix the problem somehow, but we just write a error message. So it's best to just follow CoreFX, where this check is done and just handle the exception. As a result, we will have only one block try-catch - see my post

bergmeister · 2017-08-20T12:16:07Z

@iSazonov In the beginning I was thinking that a fix in corefx would be nicer as well since this is a common problem that also others have (see e.g. here).
If you look at my links above to the actual implementations, then you can see that it would be trivial to improve the corefx algorithms (Windows & Unix) to have an overload with a non-default extension but it would require more work to improve the file name uniqueness limitation to 65,535 files due to the Windows API that it calls at the end, etc.
The reason why I was hesitating to ask for a change in corefx is because corefx is probably not very happy with adding an overload that is not part of .net standard (which would then also require an implementation of the new overload in Xamarin and full .Net). What do others think?

iSazonov · 2017-08-21T07:32:45Z

Sorry for being unclear - I meant that it is better to fix "the file name uniqueness limitation to 65,535 files" algorithm in CoreFX not extension - adding new overload for extension require .Net Standard change - it is very long process. Also it will be not approved without real business case - mentioned issue is unuseful because don't contains any arguments.

lzybkr

If this is merged, the documentation of New-TemporaryFile will need to be updated as it describes in some detail the current algorithm.

lzybkr · 2017-08-21T19:56:24Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/NewTemporaryFileCommand.cs

+        /// Use a Guid as a file name to allow for more than 65,535 unique file names in the temporary folder.
+        /// </summary>
+        [Parameter()]
+        public SwitchParameter GuidBasedName  { get; set; }


I'm not a fan of this parameter name, maybe -UseGuid?

lzybkr · 2017-08-21T19:58:54Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/NewTemporaryFileCommand.cs

+                        filePath = Path.Combine(tempPath, fileName);
+                        using (new FileStream(filePath, FileMode.CreateNew)) { }
+                        creationOfFileSuccessful = true;
+                        WriteVerbose("Created temporary file {filePath}.");


Shouldn't this be

WriteVerbose($"Created temporary file {filePath}.");

Yes. Thanks

lzybkr · 2017-08-21T20:45:32Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/NewTemporaryFileCommand.cs

                }
-                catch (Exception e)
+
+                if (!creationOfFileSuccessful)
                {
                    WriteError(


I think a terminating error is appropriate and an unlikely breaking change, but we can make that change in a different PR.

lzybkr · 2017-08-21T20:48:12Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/NewTemporaryFileCommand.cs

+                        }
+                        fileName = Path.ChangeExtension(fileName, Extension);
+                        filePath = Path.Combine(tempPath, fileName);
+                        using (new FileStream(filePath, FileMode.CreateNew)) { }


A comment would be good here - explaining why we create the file and do not remove it, similar to Path.GetTempFileName.

lzybkr · 2017-08-21T20:50:29Z

src/Microsoft.PowerShell.Commands.Utility/resources/UtilityCommonStrings.resx

+  <data name="InvalidCharacterInParameter" xml:space="preserve">
+    <value>Parameter '{0}' contains at least one invalid character. Its value is: '{1}'</value>
+  </data>
+  <data name="CouldNotFindTemporaryFilename" xml:space="preserve">


Find doesn't seem quite right, I would use Create, and change the message to Could not create a temporary file name in {0}.

OK, can do. I wasn't 100% happy with the error message myself either.

If we change the text we should rename CouldNotFindTemporaryFilename to CouldNotCreateTemporaryFilename.

lzybkr · 2017-08-21T20:51:59Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/NewTemporaryFileCommand.cs

        /// </summary>
        protected override void EndProcessing()
        {
+            // Check for invalid characters in extension
+            if (!string.IsNullOrEmpty(Extension) && Extension.IndexOfAny(Path.GetInvalidFileNameChars()) != -1)


Extension can't be null or empty because of ValidatenotNorOrEmpty, so this test is not necessary.

lzybkr · 2017-08-21T20:53:31Z

test/powershell/Modules/Microsoft.PowerShell.Utility/NewTemporaryFile.Tests.ps1

+        $tempFile = New-TemporaryFile 'csv' # check that one can also omit the period and parameter name
+        try
+        {
+            Test-Path $tempFile | Should be $true


Shouldn't this be $tempFile | Should Exist?

I was following the pattern of an existing test but you are right that Should Exist is better.

lzybkr · 2017-08-21T20:54:05Z

test/powershell/Modules/Microsoft.PowerShell.Utility/NewTemporaryFile.Tests.ps1

+        try
+        {
+
+            Test-Path $tempFile | Should be $true


Shouldn't this be $tempFile | Should Exist?

lzybkr · 2017-08-21T20:54:54Z

test/powershell/Modules/Microsoft.PowerShell.Utility/NewTemporaryFile.Tests.ps1


-        Test-Path $tempFile | Should be $true
-        $tempFile | Should BeOfType System.IO.FileInfo
+    It "creates a new temporary file with a specific extension using the -Extension parameter" {


This test has repeated code, you should use the -Testcases feature of Pester instead.

mklement0 · 2017-08-23T20:43:01Z

@iSazonov:

As for the business case: Remember that GNU mktemp already allows you to specify a custom extension (suffix), via its far more flexible XXX... templating feature; e.g, something like
mktemp -t XXX.csv yields something like /tmp/kLJ.csv. (BSD mktemp works differently and is less flexible - it only allows you to specify a custom prefix.)
All great technical points.

A simple way of rewriting this PR in order to keep Path.GetTempFileName() in charge is to:

Always start out with filePath = Path.GetTempFileName() - this will ensure the proper permissions.
Only if -Extension was specified:
- Use filePathWithCustomExt = Path.ChangeExtension(filePath, Extension) and then rename that file (File.Move(filePath, filePathWithCustomExt)) in a retry loop.

A retry loop is then only necessary in the -Extension case, which means we have to pick a retry count; picking a high count should be fine, as collisions are unlikely.

iSazonov · 2017-08-24T04:27:52Z

@lzybkr @mklement0 Thanks for the discussion!
Without MSFT CRM 😄 I can not disprove "I do believe there is a real customer need" - so we want -Extension.
About implementation. We could implement simple workaround as @mklement0 suggested to only "close the hole". I prefer "perfect" solution 😄
I have no doubt that the right place is CoreFX. This requires an exhancement of the .Net Standard and subsequent implementation - it is a long process - we should open a Issue in CoreFX.
Today we could implement this in PowerShell Core but we have an Issue to create temp directories either in the cmdlet or in new one. And we see a symmetrical situation - Unix has a richer API than Windows. We again should ask CoreFX to enhance API to create temp directories.
Hence we should create the proposed API and implement it in our Core Facade block to easily migrate to CoreFX implementations later - nothing new - we used such approach in .Net Core 1.0 time and continue currently with 2.0.
Thus we suggest to get Unix mktemp-like functions and re-implement them for both platforms.

mklement0 · 2017-08-24T07:42:24Z

@iSazonov: 👍

I cannot disprove "I do believe there is a real customer need"

Well, I'm a customer, and I have that need.
Q.E.D.
😜

PaulHigin · 2017-08-24T15:55:20Z

@iSazonov
Sorry but I am still unclear on the issues. The latest algorithm in this PR seems Ok to me. It makes ten attempts to a) generate a random file name then b) create the file. Each attempt either succeeds or fails.

But it sounds like the dotNet implementation of file creation is susceptible to race and file access permission errors where a) two processes end up with access to the same temp file and b) file access permission is incorrect and possibly detrimental to one of the processes owning the file causing a security hole.

Is this correct?

So the conclusion is that currently there is no way to safely create a new file until these dotNet layer bugs are fixed. You are suggesting that we create our own native layer file creation for both Windows and Linux platforms that do not have these weaknesses, and then try and get these changes into dotNet. Correct?

Thanks!

iSazonov · 2017-08-24T16:21:25Z

@PaulHigin

The latest algorithm in this PR seems Ok to me

Formally it is Ok. Really it has race and security hole.

until these dotNet layer bugs are fixed.

It is not CoreFX bugs. This is due to the misuse of these functions. With glibc we can get the same - that's why mkstemps() was added.

My suggestion for CoreFX is add something like:

public static string GetTempFileName(string Suffix)
public static string GetTempFileName(string Prefix)
public static string GetTempFileName(string InTempDirectory)
public static string GetTempFileName(string Prefix, string Suffix)
public static string GetTempFileName(string Prefix, string Suffix, int VariableLength)
public static string GetTempFileName(string Prefix, string Suffix, int VariableLength, string InTempDirectory)

// And API to create temp directory

Yes, my suggestion is implement the APIs temporary in PowerShell.

bergmeister · 2017-08-24T20:43:20Z

I think it would be best to modify the existing algorithm using the proposal of @mklement0 , which sounds like an overal improvement and closes the 'hole' (assuming that the rename method does not change the file permissions). I am not sure if this 'hole' is a bit superficial because on Windows any other process can edit the generated temporary file and it would only affect security if a developer directly injected content of the temporary file into other methods in their program or script(please correct me if i'm wrong). Any decent developer should know that any input has the possibility of being a vulnerability for injection methods and therefore needs to assess the risk and potentially defend against it. We should probably have a chat with corefx (maybe we don't have enough understanding of the security model that underlines corefx) but trying to do all the low level stuff ourselves makes it look like to me that we are either overegging the pudding or that corefx is unsafe.

iSazonov · 2017-08-31T05:56:46Z

@bergmeister Sorry for the delay.
Based on @lzybkr and @mklement0 conlusion we can continue to follow the @mklement0 proposal:

A simple way of rewriting this PR in order to keep Path.GetTempFileName() in charge is to:

Always start out with filePath = Path.GetTempFileName() - this will ensure the proper permissions.
Only if -Extension was specified:
- Use filePathWithCustomExt = Path.ChangeExtension(filePath, Extension) and then rename that file (File.Move(filePath, filePathWithCustomExt)) in a retry loop.

…l method Path.GetTempFileName() method by default (because this method takes care of the file creation, which would've let to other issues) but keep the retry logic, which is mainly needed in case the -Extension parameter is used.

bergmeister · 2017-08-31T23:18:33Z

@iSazonov @lzybkr I have committed the change in the algorithm as agreed in the last comment, the only subtle difference was that the retry logic always applies because it was 'free' and allowed to have less and more elegant code this way.

SteveL-MSFT · 2017-09-01T00:20:29Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/NewTemporaryFileCommand.cs

+                            temporaryFilePath = temporaryFileWithCustomExtension;
+                        }
+                        creationOfFileSuccessful = true;
+                        WriteVerbose($"Created temporary file {temporaryFilePath}.");


This should probably be a WriteDebug as that tempfile may be deleted

…sed for temporary work whereby the file probably gets deleted by the user at some point. This was suggested in the code review of PR 4612

iSazonov

Leave a comment

iSazonov · 2017-09-04T06:25:11Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/NewTemporaryFileCommand.cs

+        /// <summary>
+        /// Specify a different file extension other than the default one, which is '.tmp'. The period in this parameter is optional.
+        /// </summary>
+        [Parameter(Position = 0)]


It doesn't make sense for single parameter and block future enhancements. I think we should remove this.

iSazonov · 2017-09-04T06:33:17Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/NewTemporaryFileCommand.cs

+        /// </summary>
+        [Parameter(Position = 0)]
+        [ValidateNotNullOrEmpty]
+        public string Extension { get; set; } = defaultExtension;


I believe we should use simple code route so the initialization should be removed.

iSazonov · 2017-09-04T06:34:30Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/NewTemporaryFileCommand.cs

-            string filePath = null;
-            string tempPath = System.Environment.GetEnvironmentVariable("TEMP");
+            // Check for invalid characters in extension
+            if (Extension.IndexOfAny(Path.GetInvalidFileNameChars()) != -1)


It seems we should move the code in BeginProcessing().

iSazonov · 2017-09-04T07:06:32Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/NewTemporaryFileCommand.cs

+                    try
+                    {
+                        temporaryFilePath = Path.GetTempFileName(); // this already creates the temporary file
+                        if (Extension != defaultExtension)


That is why we must follow the @mklement0 proposal and implement two code routes. Here we do unneeded checks in loop. This loop only makes sense for a very loaded scenarios. If we want to guarantee anything in this cycle, the number of attempts should be greater than 10000 - see Unix implementations.
Also we should exclude creationOfFileSuccessful - if we get file name we should return immediately without extra checks.

if ( !Extension.IsPresent() ) { return Path.GetTempFileName(); } else { return GetFileNameWithExtension(Extention); }

iSazonov · 2017-09-04T07:07:35Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/NewTemporaryFileCommand.cs

+                    }
+                    catch (IOException) // file already exists -> retry
+                    {
+                        attempts++;


Please remove - we do this in cycle header.

iSazonov · 2017-09-04T07:09:26Z

test/powershell/Modules/Microsoft.PowerShell.Utility/NewTemporaryFile.Tests.ps1

 #>

 Describe "NewTemporaryFile" -Tags "CI" {

-    It "creates a new temporary file" {
-        $tempFile = New-TemporaryFile
+    $defaultExtension = '.tmp'


Please move the line to BeforeAll.

iSazonov · 2017-09-04T07:27:00Z

test/powershell/Modules/Microsoft.PowerShell.Utility/NewTemporaryFile.Tests.ps1

        {
-            Remove-Item $tempFile -ErrorAction SilentlyContinue -Force
+            { New-TemporaryFile -Extension $invalidFileNameChar -ErrorAction Stop } | ShouldBeErrorId "NewTemporaryInvalidArgument,Microsoft.PowerShell.Commands.NewTemporaryFileCommand"


Doesn't make sense to test for all invalid chars - enough of one - we should check only the error id.

iSazonov · 2017-09-04T07:28:09Z

test/powershell/Modules/Microsoft.PowerShell.Utility/NewTemporaryFile.Tests.ps1

+            [System.IO.Path]::ChangeExtension($tempFile, $defaultExtension) | Should Not Exist
+            $tempFile | Should BeOfType System.IO.FileInfo
+            $tempFile.Extension | Should be ".csv"
+            $tempFile.FullName.EndsWith($extension) | Should be $true


Please remove the line - previous line already make the check.

The last test is for test cases like .txt.csv because the Extension property returns only .csv in this case

Thanks for clarify.
We use CoreFX method Path.ChangeExtension() to change the extention so we shouldn't have many tests which test CoreFX - one test is enough for our code..

iSazonov · 2017-09-04T07:29:22Z

test/powershell/Modules/Microsoft.PowerShell.Utility/NewTemporaryFile.Tests.ps1

+        @{ extension = '..csv' }
+        @{ extension = 'txt.csv' }
+        @{ extension = '.txt.csv' }
+    )


Please move to It block.

It "creates a new temporary file with a specific extension" -TestCases $extensionParameterTestCases -Test @( @{ extension = 'csv' } @{ extension = '.csv' } @{ extension = '..csv' } @{ extension = 'txt.csv' } @{ extension = '.txt.csv' } ) {

iSazonov · 2017-09-04T07:36:32Z

test/powershell/Modules/Microsoft.PowerShell.Utility/NewTemporaryFile.Tests.ps1

+    AfterEach {
+        if ($null -ne $script:tempFile)
+        {
+            Remove-Item $script:tempFile -ErrorAction SilentlyContinue -Force # variable needs script scope because it gets defined in It block


Please move the comment on separate line.

lzybkr · 2017-09-05T22:09:06Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/NewTemporaryFileCommand.cs

+                    {
+                        temporaryFilePath = Path.GetTempFileName(); // this already creates the temporary file
+                        if (Extension != defaultExtension)
+                        {


We avoid string comparisons using an operator, instead preferring string.Equals and explicitly specify the comparison with either Ordinal or OrdinalIgnoreCase. In this case, Ordinal is probably fine, but we would want a test to make sure File.Move works fine when only the case of the extension differs (e.g. -Extension .TMP.)

One other thought - if we compare the extension of temporaryFilePath with Extension, we can avoid a file move if GetTempFileName changed it's algorithm and the new algorithm matches what the user specifies for Extension - an unlikely scenario I suppose, but I don't see a downside to this code change.

lzybkr · 2017-09-05T22:10:12Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/NewTemporaryFileCommand.cs

+                        if (temporaryFilePath != null)
+                        {
+                            File.Delete(temporaryFilePath);
+                        }


What if this throws? We won't retry.

Should we then simply try File.Delete with an empty catch block instead?

If the retry count is 10, that seems fine. If it was a huge number, then I'd say probably not. A short sleep in the catch might not hurt either, like 10ms or something.

Would it make sense to write a warning if we're not able to delete it?

At least a warning seems reasonable. But I am slightly more leading to actually throwing a (terminating) error because if deleting the temporary file that just got created failed then I suspect that in most cases there is rather a fundamental problem with permissions. WDYT?

-When using the defaultExtension then fall back to the old behaviour without retry logic -In the custom extension retry loop: if file deletion fails in catch block, catch the error and return error -Test improvements and test specifically for .tmp and .TMP (the latter on Unix only)

… done beforehand

bergmeister · 2017-09-30T22:59:47Z

I addressed the main points made in the review:

If the extension is '.tmp', then it goes through the same codepath as before this PR and only otherwise use the new algorithm
cater for failure if file deletion fails in catch block
Usage of Ordinal string comparison
improve tests and add Unix specific test for '.TMP' where we expect the file ending in '.tmp' to not exist on Linux

…en testing against the path.

bergmeister · 2017-10-06T21:39:55Z

I have updated the test to pass since one test did not make sense on Mac due to the the fact that Mac sees files with different casing as the same file similar to Windows.

- Remove position on Extension parameter since this is the only parameter at the moment. - Move validation logic to BeginProcessing - Move constant test variable to BeforeAll block

…o TemporaryFile # Conflicts: #src/Microsoft.PowerShell.Commands.Utility/resources/UtilityCommonStrings.resx

iSazonov · 2018-02-21T16:19:04Z

Currently we have new API request in CoreFX and CoreFX team has post 2.1 plans to enhance the API. Feel free to vote for the API.
I believe we can close the PR as non-critical and wait CoreFX implementation.

PowerShell#4215 Add ability to create a temporary file with a specifi…

bae68f7

…c extension to New-TemporaryFile using a new -Extension parameter.

bergmeister requested review from dantraMSFT, JamesWTruher and PaulHigin as code owners August 18, 2017 11:51

msftclas added the cla-already-signed label Aug 18, 2017

mklement0 reviewed Aug 18, 2017

View reviewed changes

dantraMSFT reviewed Aug 18, 2017

View reviewed changes

PaulHigin suggested changes Aug 18, 2017

View reviewed changes

dantraMSFT reviewed Aug 18, 2017

View reviewed changes

mklement0 reviewed Aug 18, 2017

View reviewed changes

Fixed trivial typo.

6e102da

PowerShell#4215 Add -GuidBasedName switch as proposed in PR PowerShel…

61e5300

…l#4612

iSazonov reviewed Aug 19, 2017

View reviewed changes

iSazonov reviewed Aug 20, 2017

View reviewed changes

lzybkr suggested changes Aug 21, 2017

View reviewed changes

This was referenced Aug 21, 2017

New-TemporaryFile should throw terminating error #4634

Closed

Pester testing guideline for tests against thrown errors #4635

Closed

iSazonov changed the title ~~Implemented #4215: New-TemporaryFile to have -Extension parameter~~ Implement -Extension parameter in New-TemporaryFile cmdlet Aug 24, 2017

daxian-dbw assigned iSazonov Aug 30, 2017

SteveL-MSFT requested changes Sep 1, 2017

View reviewed changes

Call WriteDebug instead of WriteVerbose since this cmdlet is mainly u…

2c3ae88

…sed for temporary work whereby the file probably gets deleted by the user at some point. This was suggested in the code review of PR 4612

SteveL-MSFT approved these changes Sep 1, 2017

View reviewed changes

iSazonov reviewed Sep 4, 2017

View reviewed changes

lzybkr reviewed Sep 5, 2017

View reviewed changes

msftgits removed the cla-already-signed label Sep 26, 2017

PowerShell deleted a comment from msftclas Sep 27, 2017

bergmeister added 3 commits September 30, 2017 22:41

Enhance Unix specific test to pick out the line that is Unix specific

a1d6b2a

Remove redundant check for defaultExtension since this is now already…

c076c9b

… done beforehand

Make test not run on Mac since '.tmp' and '.TMP' would be the same wh…

cafd031

…en testing against the path.

bergmeister added 2 commits October 22, 2017 21:45

Minor improvements to address the reamaining comments:

d43b02a

- Remove position on Extension parameter since this is the only parameter at the moment. - Move validation logic to BeginProcessing - Move constant test variable to BeforeAll block

Merge branch 'master' of https://github.com/PowerShell/PowerShell int…

db3bd43

…o TemporaryFile # Conflicts: #src/Microsoft.PowerShell.Commands.Utility/resources/UtilityCommonStrings.resx

bergmeister closed this Feb 21, 2018

SimonWahlin mentioned this pull request Mar 7, 2018

Add ability to create a temporary file with a specific extension to New-TemporaryFile #4215

Closed

		@@ -12,34 +12,73 @@ namespace Microsoft.PowerShell.Commands
		[OutputType(typeof(System.IO.FileInfo))]
		public class NewTemporaryFileCommand : Cmdlet

Implement -Extension parameter in New-TemporaryFile cmdlet #4612

Implement -Extension parameter in New-TemporaryFile cmdlet #4612

Conversation

bergmeister commented Aug 18, 2017

mklement0 Aug 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bergmeister Aug 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bergmeister Aug 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bergmeister Aug 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bergmeister commented Aug 18, 2017 • edited Loading

PaulHigin commented Aug 18, 2017

bergmeister commented Aug 18, 2017 • edited Loading

iSazonov commented Aug 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bergmeister Aug 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bergmeister Aug 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iSazonov commented Aug 20, 2017

Choose a reason for hiding this comment

bergmeister commented Aug 20, 2017

iSazonov commented Aug 21, 2017

lzybkr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mklement0 commented Aug 23, 2017

iSazonov commented Aug 24, 2017 • edited Loading

mklement0 commented Aug 24, 2017

PaulHigin commented Aug 24, 2017

iSazonov commented Aug 24, 2017

bergmeister commented Aug 24, 2017

iSazonov commented Aug 31, 2017 • edited Loading

bergmeister commented Aug 31, 2017

Choose a reason for hiding this comment

mklement0 Aug 18, 2017 •

edited

Loading

bergmeister Aug 18, 2017 •

edited

Loading

bergmeister Aug 21, 2017 •

edited

Loading

bergmeister Aug 18, 2017 •

edited

Loading

bergmeister commented Aug 18, 2017 •

edited

Loading

bergmeister commented Aug 18, 2017 •

edited

Loading

bergmeister Aug 20, 2017 •

edited

Loading

bergmeister Aug 20, 2017 •

edited

Loading

iSazonov commented Aug 24, 2017 •

edited

Loading

iSazonov commented Aug 31, 2017 •

edited

Loading