Properly validate bundle variable names

Fixes 6743

10 files changed, 122 insertions, 25 deletions

diff --git a/src/wix/WixToolset.Core/Common.cs b/src/wix/WixToolset.Core/Common.cs
index 7a7a654c..486c05c7 100644
--- a/src/wix/WixToolset.Core/Common.cs
+++ b/src/wix/WixToolset.Core/Common.cs
@@ -584,6 +584,29 @@ namespace WixToolset.Core
         }
 
         /// <summary>
+        /// Verifies that a value is a legal Bundle Variable/@Name.
+        /// </summary>
+        /// <param name="value">The value to verify.</param>
+        /// <returns>true if the value is an valid Variable/@Name; false otherwise.</returns>
+        public static bool IsBundleVariableName(string value)
+        {
+            if (String.IsNullOrEmpty(value))
+            {
+                return false;
+            }
+
+            for (var i = 0; i < value.Length; ++i)
+            {
+                if (!ValidBundleVariableNameChar(value[i], i == 0))
+                {
+                    return false;
+                }
+            }
+
+            return true;
+        }
+
+        /// <summary>
         /// Get an identifier attribute value and displays an error for an illegal identifier value.
         /// </summary>
         /// <param name="messaging"></param>
@@ -812,5 +835,11 @@ namespace WixToolset.Core
             return ('A' <= c && 'Z' >= c) || ('a' <= c && 'z' >= c) || '_' == c ||
                    (!firstChar && (Char.IsDigit(c) || '.' == c));
         }
+
+        private static bool ValidBundleVariableNameChar(char c, bool firstChar)
+        {
+            return ('A' <= c && 'Z' >= c) || ('a' <= c && 'z' >= c) || '_' == c ||
+                   (!firstChar && Char.IsDigit(c));
+        }
     }
 }
diff --git a/src/wix/WixToolset.Core/CompilerCore.cs b/src/wix/WixToolset.Core/CompilerCore.cs
index 8c676f51..cda6507a 100644
--- a/src/wix/WixToolset.Core/CompilerCore.cs
+++ b/src/wix/WixToolset.Core/CompilerCore.cs
@@ -737,21 +737,26 @@ namespace WixToolset.Core
         }
 
         /// <summary>
-        /// Gets a Bundle variable value and displays an error for an illegal value.
+        /// Gets a bundle variable value and displays an error for an illegal value.
         /// </summary>
         /// <param name="sourceLineNumbers">Source line information about the owner element.</param>
         /// <param name="attribute">The attribute containing the value to get.</param>
         /// <returns>The attribute's value.</returns>
-        public string GetAttributeBundleVariableValue(SourceLineNumber sourceLineNumbers, XAttribute attribute)
+        public Identifier GetAttributeBundleVariableNameIdentifier(SourceLineNumber sourceLineNumbers, XAttribute attribute)
         {
-            string value = this.GetAttributeValue(sourceLineNumbers, attribute);
+            var variableName = this.GetAttributeIdentifier(sourceLineNumbers, attribute);
 
-            if (!String.IsNullOrEmpty(value))
+            if (!String.IsNullOrEmpty(variableName?.Id))
             {
-                this.bundleValidator.ValidateBundleVariableName(sourceLineNumbers, attribute.Parent.Name.LocalName, attribute.Name.LocalName, value);
+                this.bundleValidator.ValidateBundleVariableName(sourceLineNumbers, attribute.Parent.Name.LocalName, attribute.Name.LocalName, variableName.Id);
+
+                if (variableName.Id.StartsWith("Wix", StringComparison.OrdinalIgnoreCase))
+                {
+                    this.messaging.Write(ErrorMessages.ReservedNamespaceViolation(sourceLineNumbers, attribute.Parent.Name.LocalName, attribute.Name.LocalName, "Wix"));
+                }
             }
 
-            return value;
+            return variableName;
         }
 
         /// <summary>
diff --git a/src/wix/WixToolset.Core/CompilerErrors.cs b/src/wix/WixToolset.Core/CompilerErrors.cs
index 10646dfd..1afdfe45 100644
--- a/src/wix/WixToolset.Core/CompilerErrors.cs
+++ b/src/wix/WixToolset.Core/CompilerErrors.cs
@@ -16,6 +16,11 @@ namespace WixToolset.Core
             return Message(sourceLineNumbers, Ids.ReservedValue, "The {0}/@{1} attribute value '{2}' is reserved and cannot be used here. Please choose a different value.", elementName, attributeName, attributeValue);
         }
 
+        public static Message IllegalBundleVariableName(SourceLineNumber sourceLineNumbers, string elementName,string attributeName, string value)
+        {
+            return Message(sourceLineNumbers, Ids.IllegalBundleVariableName, "The {0}/@{1} attribute's value, '{2}', is not a legal bundle variable name. Identifiers may contain ASCII characters A-Z, a-z, digits, or underscores (_). Every identifier must begin with either a letter or an underscore.", elementName, attributeName, value);
+        }
+
         public static Message IllegalName(SourceLineNumber sourceLineNumbers, string parentElement, string name)
         {
             return Message(sourceLineNumbers, Ids.IllegalName, "The Tag/@Name attribute value, '{1}', contains invalid filename identifiers. The Tag/@Name may have defaulted from the {0}/@Name attrbute. If so, use the Tag/@Name attribute to provide a valid filename. Any character except for the follow may be used: \\ ? | > < : / * \".", parentElement, name);
@@ -38,6 +43,7 @@ namespace WixToolset.Core
 
             IllegalName = 6601,
             ExampleRegid = 6602,
+            IllegalBundleVariableName = 6603,
         } // 5400-5499 and 6600-6699 were the ranges for Dependency and Tag which are now in Core between CompilerWarnings and CompilerErrors.
     }
 }
diff --git a/src/wix/WixToolset.Core/Compiler_Bundle.cs b/src/wix/WixToolset.Core/Compiler_Bundle.cs
index f3fc0271..3ff8216b 100644
--- a/src/wix/WixToolset.Core/Compiler_Bundle.cs
+++ b/src/wix/WixToolset.Core/Compiler_Bundle.cs
@@ -3659,7 +3659,7 @@ namespace WixToolset.Core
         {
             var sourceLineNumbers = Preprocessor.GetSourceLineNumbers(node);
             var hidden = false;
-            string name = null;
+            Identifier name = null;
             var persisted = false;
             string value = null;
             string typeValue = null;
@@ -3677,7 +3677,7 @@ namespace WixToolset.Core
                             }
                             break;
                         case "Name":
-                            name = this.Core.GetAttributeBundleVariableValue(sourceLineNumbers, attrib);
+                            name = this.Core.GetAttributeBundleVariableNameIdentifier(sourceLineNumbers, attrib);
                             break;
                         case "Persisted":
                             if (YesNoType.Yes == this.Core.GetAttributeYesNoValue(sourceLineNumbers, attrib))
@@ -3706,10 +3706,6 @@ namespace WixToolset.Core
             {
                 this.Core.Write(ErrorMessages.ExpectedAttribute(sourceLineNumbers, node.Name.LocalName, "Name"));
             }
-            else if (name.StartsWith("Wix", StringComparison.OrdinalIgnoreCase))
-            {
-                this.Core.Write(ErrorMessages.ReservedNamespaceViolation(sourceLineNumbers, node.Name.LocalName, "Name", "Wix"));
-            }
 
             if (hidden && persisted)
             {
@@ -3722,7 +3718,7 @@ namespace WixToolset.Core
 
             if (!this.Core.EncounteredError)
             {
-                this.Core.AddSymbol(new WixBundleVariableSymbol(sourceLineNumbers, new Identifier(AccessModifier.Section, name))
+                this.Core.AddSymbol(new WixBundleVariableSymbol(sourceLineNumbers, name)
                 {
                     Value = value,
                     Type = type,
diff --git a/src/wix/WixToolset.Core/ExtensibilityServices/BundleValidator.cs b/src/wix/WixToolset.Core/ExtensibilityServices/BundleValidator.cs
index 0149fe94..44b3ea93 100644
--- a/src/wix/WixToolset.Core/ExtensibilityServices/BundleValidator.cs
+++ b/src/wix/WixToolset.Core/ExtensibilityServices/BundleValidator.cs
@@ -153,6 +153,12 @@ namespace WixToolset.Core.ExtensibilityServices
 
                 return false;
             }
+            else if (!Common.IsBundleVariableName(variableName))
+            {
+                this.Messaging.Write(CompilerErrors.IllegalBundleVariableName(sourceLineNumbers, elementName, attributeName, variableName));
+
+                return false;
+            }
             else if (BuiltinBundleVariables.Contains(variableName))
             {
                 var illegalValues = CreateValueList(ValueListKind.Or, BuiltinBundleVariables);
diff --git a/src/wix/test/WixToolsetTest.CoreIntegration/BadInputFixture.cs b/src/wix/test/WixToolsetTest.CoreIntegration/BadInputFixture.cs
index ab38e9ae..815c5ccd 100644
--- a/src/wix/test/WixToolsetTest.CoreIntegration/BadInputFixture.cs
+++ b/src/wix/test/WixToolsetTest.CoreIntegration/BadInputFixture.cs
@@ -155,6 +155,42 @@ namespace WixToolsetTest.CoreIntegration
         }
 
         [Fact]
+        public void CannotBuildBundleWithLocVariableNames()
+        {
+            var folder = TestData.Get(@"TestData");
+
+            using (var fs = new DisposableFileSystem())
+            {
+                var baseFolder = fs.GetFolder();
+                var intermediateFolder = Path.Combine(baseFolder, "obj");
+
+                var result = WixRunner.Execute(new[]
+                {
+                    "build",
+                    Path.Combine(folder, "BundleWithInvalid", "BundleWithInvalidLocVariableNames.wxs"),
+                    "-loc", Path.Combine(folder, "BundleWithInvalid", "BundleWithInvalidLocValues.wxl"),
+                    "-bindpath", Path.Combine(folder, ".Data"),
+                    "-bindpath", Path.Combine(folder, "DecompileSingleFileCompressed"),
+                    "-bindpath", Path.Combine(folder, "SimpleBundle", "data"),
+                    "-intermediateFolder", intermediateFolder,
+                    "-o", Path.Combine(baseFolder, @"bin\test.exe")
+                });
+
+                var messages = result.Messages.Select(m => m.ToString()).ToList();
+                messages.Sort();
+
+                WixAssert.CompareLineByLine(new[]
+                {
+                    "The SetVariable/@Variable attribute's value, '!(loc.BuiltinBurnVariableName)', is not a legal bundle variable name. Identifiers may contain ASCII characters A-Z, a-z, digits, or underscores (_). Every identifier must begin with either a letter or an underscore.",
+                    "The Variable/@Name attribute was not found; it is required.",
+                    "The Variable/@Name attribute's value, '!(loc.BuiltinBurnVariableName)', is not a legal identifier. Identifiers may contain ASCII characters A-Z, a-z, digits, underscores (_), or periods (.). Every identifier must begin with either a letter or an underscore.",
+                }, messages.ToArray());
+
+                Assert.Equal(6603, result.ExitCode);
+            }
+        }
+
+        [Fact]
         public void GuardsAgainstVariousBundleValuesFromLoc()
         {
             var folder = TestData.Get(@"TestData");
@@ -176,23 +212,20 @@ namespace WixToolsetTest.CoreIntegration
                     "-o", Path.Combine(baseFolder, @"bin\test.exe")
                 });
 
-                Assert.InRange(result.ExitCode, 2, Int32.MaxValue);
-
                 var messages = result.Messages.Select(m => m.ToString()).ToList();
                 messages.Sort();
 
                 WixAssert.CompareLineByLine(new[]
                 {
-                    "*Search/@Condition contains the built-in Variable 'WixBundleAction', which is not available when it is evaluated. (Unavailable Variables are: 'WixBundleAction'.). Rewrite the condition to avoid Variables that are never valid during its evaluation.",
                     "Bundle/@Condition contains the built-in Variable 'WixBundleInstalled', which is not available when it is evaluated. (Unavailable Variables are: 'RebootPending', 'WixBundleAction', or 'WixBundleInstalled'.). Rewrite the condition to avoid Variables that are never valid during its evaluation.",
                     "ExePackage/@DetectCondition contains the built-in Variable 'WixBundleAction', which is not available when it is evaluated. (Unavailable Variables are: 'WixBundleAction'.). Rewrite the condition to avoid Variables that are never valid during its evaluation.",
-                    "The *Search/@Variable attribute's value, 'WixBundleInstalled', is one of the illegal options: 'AdminToolsFolder', 'AppDataFolder', 'CommonAppDataFolder', 'CommonFiles64Folder', 'CommonFilesFolder', 'CompatibilityMode', 'Date', 'DesktopFolder', 'FavoritesFolder', 'FontsFolder', 'InstallerName', 'InstallerVersion', 'LocalAppDataFolder', 'LogonUser', 'MyPicturesFolder', 'NativeMachine', 'NTProductType', 'NTSuiteBackOffice', 'NTSuiteDataCenter', 'NTSuiteEnterprise', 'NTSuitePersonal', 'NTSuiteSmallBusiness', 'NTSuiteSmallBusinessRestricted', 'NTSuiteWebServer', 'PersonalFolder', 'Privileged', 'ProgramFiles64Folder', 'ProgramFiles6432Folder', 'ProgramFilesFolder', 'ProgramMenuFolder', 'RebootPending', 'SendToFolder', 'ServicePackLevel', 'StartMenuFolder', 'StartupFolder', 'System64Folder', 'SystemFolder', 'TempFolder', 'TemplateFolder', 'TerminalServer', 'UserLanguageID', 'UserUILanguageID', 'VersionMsi', 'VersionNT', 'VersionNT64', 'WindowsFolder', 'WindowsVolume', 'WixBundleAction', 'WixBundleCommandLineAction', 'WixBundleForcedRestartPackage', 'WixBundleElevated', 'WixBundleInstalled', 'WixBundleProviderKey', 'WixBundleTag', or 'WixBundleVersion'.",
                     "The CommandLine/@Condition attribute's value '=' is not a valid bundle condition.",
                     "The MsiPackage/@InstallCondition attribute's value '=' is not a valid bundle condition.",
                     "The MsiProperty/@Condition attribute's value '=' is not a valid bundle condition.",
-                    //"The Variable/@Name attribute's value, 'WixBundleInstalled', is one of the illegal options: 'AdminToolsFolder', 'AppDataFolder', 'CommonAppDataFolder', 'CommonFiles64Folder', 'CommonFilesFolder', 'CompatibilityMode', 'Date', 'DesktopFolder', 'FavoritesFolder', 'FontsFolder', 'InstallerName', 'InstallerVersion', 'LocalAppDataFolder', 'LogonUser', 'MyPicturesFolder', 'NativeMachine', 'NTProductType', 'NTSuiteBackOffice', 'NTSuiteDataCenter', 'NTSuiteEnterprise', 'NTSuitePersonal', 'NTSuiteSmallBusiness', 'NTSuiteSmallBusinessRestricted', 'NTSuiteWebServer', 'PersonalFolder', 'Privileged', 'ProgramFiles64Folder', 'ProgramFiles6432Folder', 'ProgramFilesFolder', 'ProgramMenuFolder', 'RebootPending', 'SendToFolder', 'ServicePackLevel', 'StartMenuFolder', 'StartupFolder', 'System64Folder', 'SystemFolder', 'TempFolder', 'TemplateFolder', 'TerminalServer', 'UserLanguageID', 'UserUILanguageID', 'VersionMsi', 'VersionNT', 'VersionNT64', 'WindowsFolder', 'WindowsVolume', 'WixBundleAction', 'WixBundleCommandLineAction', 'WixBundleForcedRestartPackage', 'WixBundleElevated', 'WixBundleInstalled', 'WixBundleProviderKey', 'WixBundleTag', or 'WixBundleVersion'.",
                     "The 'REINSTALLMODE' MsiProperty is controlled by the bootstrapper and cannot be authored. (Illegal properties are: 'ACTION', 'ADDLOCAL', 'ADDSOURCE', 'ADDDEFAULT', 'ADVERTISE', 'ALLUSERS', 'REBOOT', 'REINSTALL', 'REINSTALLMODE', or 'REMOVE'.) Remove the MsiProperty element.",
                 }, messages.ToArray());
+
+                Assert.Equal(1159, result.ExitCode);
             }
         }
     }
diff --git a/src/wix/test/WixToolsetTest.CoreIntegration/FeatureFixture.cs b/src/wix/test/WixToolsetTest.CoreIntegration/FeatureFixture.cs
index d23e37f6..51c83618 100644
--- a/src/wix/test/WixToolsetTest.CoreIntegration/FeatureFixture.cs
+++ b/src/wix/test/WixToolsetTest.CoreIntegration/FeatureFixture.cs
@@ -32,13 +32,15 @@ namespace WixToolsetTest.CoreIntegration
                     "-o", msiPath
                 });
 
-                Assert.Equal(267, result.ExitCode);
+                var messages = result.Messages.Select(m => m.ToString()).ToList();
+                messages.Sort();
 
-                var errors = result.Messages.Where(m => m.Level == MessageLevel.Error);
-                Assert.Equal(new[]
+                WixAssert.CompareLineByLine(new[]
                 {
-                    267
-                }, errors.Select(e => e.Id).ToArray());
+                    "Found orphaned Component 'filit6MyH46zIGKsPPPXDZDfeNrfVY'.  If this is a Package, every Component must have at least one parent Feature.  To include a Component in a Module, you must include it directly as a Component element of the Module element or indirectly via ComponentRef, ComponentGroup, or ComponentGroupRef elements.",
+                }, messages.ToArray());
+
+                Assert.Equal(267, result.ExitCode);
             }
         }
 
diff --git a/src/wix/test/WixToolsetTest.CoreIntegration/TestData/BundleWithInvalid/BundleWithInvalidLocValues.wxs b/src/wix/test/WixToolsetTest.CoreIntegration/TestData/BundleWithInvalid/BundleWithInvalidLocValues.wxs
index 504f6e48..00263f49 100644
--- a/src/wix/test/WixToolsetTest.CoreIntegration/TestData/BundleWithInvalid/BundleWithInvalidLocValues.wxs
+++ b/src/wix/test/WixToolsetTest.CoreIntegration/TestData/BundleWithInvalid/BundleWithInvalidLocValues.wxs
@@ -12,7 +12,7 @@
                 <CommandLine Condition="!(loc.NonsenseExecuteCondition)" />
             </ExePackage>
         </Chain>
-        <Variable Name="!(loc.BuiltinBurnVariableName)" Value="1" />
-        <SetVariable Id="Builtin" Condition="!(loc.NonsenseDetectCondition)" Variable="!(loc.BuiltinBurnVariableName)" Value="1" />
+        <!--<Variable Name="FOO" Value="1" />
+        <SetVariable Id="Builtin" Condition="!(loc.NonsenseDetectCondition)" Variable="FOO" Value="1" />-->
     </Bundle>
 </Wix>
diff --git a/src/wix/test/WixToolsetTest.CoreIntegration/TestData/BundleWithInvalid/BundleWithInvalidLocVariableNames.wxs b/src/wix/test/WixToolsetTest.CoreIntegration/TestData/BundleWithInvalid/BundleWithInvalidLocVariableNames.wxs
new file mode 100644
index 00000000..a210f03f
--- /dev/null
+++ b/src/wix/test/WixToolsetTest.CoreIntegration/TestData/BundleWithInvalid/BundleWithInvalidLocVariableNames.wxs
@@ -0,0 +1,16 @@
+<Wix xmlns="http://wixtoolset.org/schemas/v4/wxs">
+    <Bundle Name="BundleWithInvalidUpgradeCode" Condition="!(loc.NonsenseGlobalCondition)"
+            Version="1.0.0.0" Manufacturer="Example Corporation" UpgradeCode="{F2A56B65-2105-44C8-A532-A93A8C169D07}">
+        <BootstrapperApplication>
+            <BootstrapperApplicationDll SourceFile="fakeba.dll" />
+        </BootstrapperApplication>
+        <Chain>
+            <MsiPackage SourceFile="example.msi" >
+                <MsiProperty Name="!(loc.BuiltinMsiPropertyName)" Value="1" />
+            </MsiPackage>
+            <ExePackage DetectCondition="DoSomething" UninstallArguments="-uninstall" SourceFile="burn.exe" />
+        </Chain>
+        <Variable Name="!(loc.BuiltinBurnVariableName)" Value="1" />
+        <SetVariable Id="Builtin" Condition="!(loc.NonsenseDetectCondition)" Variable="!(loc.BuiltinBurnVariableName)" Value="1" />
+    </Bundle>
+</Wix>
diff --git a/src/wix/test/WixToolsetTest.CoreIntegration/WixToolsetTest.CoreIntegration.csproj b/src/wix/test/WixToolsetTest.CoreIntegration/WixToolsetTest.CoreIntegration.csproj
index 996858d1..211db06a 100644
--- a/src/wix/test/WixToolsetTest.CoreIntegration/WixToolsetTest.CoreIntegration.csproj
+++ b/src/wix/test/WixToolsetTest.CoreIntegration/WixToolsetTest.CoreIntegration.csproj
@@ -15,6 +15,10 @@
   </ItemGroup>
 
   <ItemGroup>
+    <None Remove="TestData\BundleWithInvalid\BundleWithInvalidLocVariableNames.wxs" />
+  </ItemGroup>
+
+  <ItemGroup>
     <ProjectReference Include="..\..\WixToolset.Core\WixToolset.Core.csproj" />
     <ProjectReference Include="..\..\WixToolset.Core.Burn\WixToolset.Core.Burn.csproj" />
     <ProjectReference Include="..\..\WixToolset.Core.WindowsInstaller\WixToolset.Core.WindowsInstaller.csproj" />