From 744cde09b8c4a5338a99cd4af24f81459c82713b Mon Sep 17 00:00:00 2001 From: Rob Mensching Date: Wed, 15 Jun 2022 13:46:33 -0700 Subject: Properly validate bundle variable names Fixes 6743 --- src/wix/WixToolset.Core/Common.cs | 29 +++++++++++++++ src/wix/WixToolset.Core/CompilerCore.cs | 17 ++++++--- src/wix/WixToolset.Core/CompilerErrors.cs | 6 +++ src/wix/WixToolset.Core/Compiler_Bundle.cs | 10 ++--- .../ExtensibilityServices/BundleValidator.cs | 6 +++ .../BadInputFixture.cs | 43 +++++++++++++++++++--- .../FeatureFixture.cs | 12 +++--- .../BundleWithInvalidLocValues.wxs | 4 +- .../BundleWithInvalidLocVariableNames.wxs | 16 ++++++++ .../WixToolsetTest.CoreIntegration.csproj | 4 ++ 10 files changed, 122 insertions(+), 25 deletions(-) create mode 100644 src/wix/test/WixToolsetTest.CoreIntegration/TestData/BundleWithInvalid/BundleWithInvalidLocVariableNames.wxs (limited to 'src') 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 @@ -583,6 +583,29 @@ namespace WixToolset.Core return true; } + /// + /// Verifies that a value is a legal Bundle Variable/@Name. + /// + /// The value to verify. + /// true if the value is an valid Variable/@Name; false otherwise. + 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; + } + /// /// Get an identifier attribute value and displays an error for an illegal identifier value. /// @@ -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 } /// - /// 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. /// /// Source line information about the owner element. /// The attribute containing the value to get. /// The attribute's value. - 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; } /// 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 @@ -154,6 +154,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() { @@ -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 @@ - - + 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 @@ + + + + + + + + + + + + + + + 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 @@ -14,6 +14,10 @@ + + + + -- cgit v1.2.3-55-g6feb