TDEAL-16: ZAP improvements

Lombiq.Tests.UI.Samples/Tests/CustomZapAutomationFrameworkPlan.yml

@@ -42,21 +42,11 @@ jobs:
     enableTags: false
     disableAllRules: false
   rules:
-  - id: 10035
-    name: "Strict-Transport-Security Header"
-    threshold: "off"
-  - id: 10038
-    name: "Content Security Policy (CSP) Header Not Set"
-    threshold: "off"
-  - id: 10020
-    name: "Anti-clickjacking Header"
-    threshold: "off"
-  - id: 10037
-    name: "Server Leaks Information via \"X-Powered-By\" HTTP Response Header Field(s)"
-    threshold: "off"
-  - id: 10021
-    name: "X-Content-Type-Options Header Missing"
-    threshold: "off"
+    # This is required for <script> blocks which OC uses extensively. The rule may be removed when OC starts to provide
+    # cryptographic nonce for these script blocks (see https://github.com/OrchardCMS/OrchardCore/issues/13389).
+    - id: 10055
+      name: "script-src includes unsafe-inline"
+      threshold: "off"
   name: "passiveScan-config"
   type: "passiveScan-config"
 - parameters: {}

Lombiq.Tests.UI.Samples/Tests/SecurityScanningTests.cs

@@ -45,8 +45,8 @@ public Task BasicSecurityScanShouldPass() =>
     //   usually not just unnecessary for a website that's not an SPA, but also slows the scan down by a lot. However,
     //   if you have an SPA, you need to use it.
     // - Excludes certain URLs from the scan completely. Use this if you don't want ZAP to process certain URLs at all.
-    // - Disables the "Server Leaks Information via "X-Powered-By" HTTP Response Header Field(s)" alert of ZAP's passive
-    //   scan for the whole scan. This is because by default, Orchard Core sends an "X-Powered-By: OrchardCore" header.
+    // - Disables the The response does not include either Content-Security-Policy with 'frame-ancestors' directive."
+    //   alert of ZAP's passive scan for the whole scan. This is because by default, Orchard Core sends an "X-Powered-By: OrchardCore" header.
     //   If you want airtight security, you might want to turn this off, but for the sake of example we just ignore the
     //   alert here.
     // - Also disables the "Content Security Policy (CSP) Header Not Set" rule but only for the /about page. Use this to
@@ -65,7 +65,7 @@ public Task SecurityScanWithCustomConfigurationShouldPass() =>
                 configuration => configuration
                     ////.UseAjaxSpider() // This is quite slow so just showing you here but not running it.
                     .ExcludeUrlWithRegex(".*blog.*")
-                    .DisablePassiveScanRule(10037, "Server Leaks Information via \"X-Powered-By\" HTTP Response Header Field(s)")
+                    .DisablePassiveScanRule(10020, "The response does not include either Content-Security-Policy with 'frame-ancestors' directive.")
                     .DisableScanRuleForUrlWithRegex(".*/about", 10038, "Content Security Policy (CSP) Header Not Set")
                     .SignIn(),
                 sarifLog => sarifLog.Runs[0].Results.Count.ShouldBeLessThan(34)));

Lombiq.Tests.UI.Shortcuts/Controllers/AccountController.cs

@@ -22,9 +22,8 @@ public AccountController(UserManager<IUser> userManager, SignInManager<IUser> us
     [AllowAnonymous]
     public async Task<IActionResult> SignInDirectly(string userName)
     {
-        var user = await _userManager.FindByNameAsync(userName);
-
-        if (user == null) return NotFound();
+        if (string.IsNullOrWhiteSpace(userName)) userName = "admin";
+        if (await _userManager.FindByNameAsync(userName) is not { } user) return NotFound();
 
         await _userSignInManager.SignInAsync(user, isPersistent: false);
 

Lombiq.Tests.UI.Shortcuts/Lombiq.Tests.UI.Shortcuts.csproj

@@ -50,7 +50,7 @@
   </ItemGroup>
 
   <ItemGroup Condition="'$(NuGetBuild)' == 'true'">
-    <PackageReference Include="Lombiq.HelpfulLibraries.OrchardCore" Version="8.1.0" />
+    <PackageReference Include="Lombiq.HelpfulLibraries.OrchardCore" Version="8.1.1-alpha.4.tdeal-16" />
   </ItemGroup>
 
 </Project>

Lombiq.Tests.UI/Docs/SecurityScanning.md

@@ -19,6 +19,23 @@ You can create detailed security scans of your app with [Zed Attack Proxy (ZAP)]
 - While ZAP is fully managed for you, Docker needs to be available and running to host the ZAP instance. On your development machine, you can install [Docker Desktop](https://www.docker.com/products/docker-desktop/).
 - The full scan of a website with even just 1-200 pages can take 5-10 minutes. So, be careful to fine-tune the ZAP configuration to make it suitable for your app.
 
+## Limitations
+
+On Windows-based GitHub runners the security tests always fail with the following error:
+
+> The `docker.exe pull softwaresecurityproject/zap-stable:2.14.0 --quiet` command failed with the output below.
+> no matching manifest for windows/amd64 10.0.20348 in the manifest list entries.
+
+This is because the Docker installation is configured to use Windows images, while the [ZAP docker image](https://hub.docker.com/r/softwaresecurityproject/zap-stable/tags) is only available for Linux. If you rely on our [Lombiq GitHub Actions](https://github.com/Lombiq/GitHub-Actions) then you can configure it like this to disable a test, in this case `SecurityScanningTests`:
+```
+  call-build-and-test-workflow-windows:
+    name: Build and Test
+    uses: Lombiq/GitHub-Actions/.github/workflows/build-and-test-orchard-core.yml@dev
+    with:
+      machine-types: '["windows-latest"]'
+      test-filter: "FullyQualifiedName!~SecurityScanningTests"
+```
+
 ## Troubleshooting
 
 - If you're unsure what happens in a scan, run the [ZAP desktop app](https://www.zaproxy.org/download/) and load the Automation Framework plan's YAML file into it. If you use the default scans, then these will be available under the build output directory (like _bin/Debug_) under _SecurityScanning/AutomationFrameworkPlans_. Then, you can open and run them as demonstrated [in this video](https://youtu.be/PnCbIAnauD8?si=u0vi63Uvv9wZINzb&t=1173).

Lombiq.Tests.UI/Extensions/BrowserUITestContextExtensions.cs

@@ -62,4 +62,35 @@ public static async Task<T> FetchWithBrowserContextAsync<T>(
 
         return await processResponseAsync(response);
     }
+
+    /// <summary>
+    /// Performs a <paramref name="task"/> with app log assertion temporarily disabled. But first <see
+    /// cref="OrchardCoreUITestExecutorConfiguration.AssertAppLogsAsync"/> is used to ensure no unrelated errors are
+    /// masked by this activity. Afterwards it's used again to verify the results, and if it fails then the logs are
+    /// cleared out.
+    /// </summary>
+    /// <returns>A task indicating whether there were anything in the app logs that failed the assertion.</returns>
+    public static async Task<bool> DoWithoutAppLogAssertionAsync(this UITestContext context, Func<Task> task)
+    {
+        // Verify that the app logs are fine right now, then suppress logs for the duration of the task.
+        await context.Configuration.AssertAppLogsAsync(context.Application);
+        var assertAppLogsAsync = context.Configuration.AssertAppLogsAsync;
+        context.Configuration.AssertAppLogsAsync = _ => Task.CompletedTask;
+
+        await task();
+
+        // Restore the app log assertion and determine if it failed. Clear the logs if failure occurred.
+        context.Configuration.AssertAppLogsAsync = assertAppLogsAsync;
+        try
+        {
+            await context.Configuration.AssertAppLogsAsync(context.Application);
+        }
+        catch
+        {
+            context.ClearLogs();
+            return true;
+        }
+
+        return false;
+    }
 }

Lombiq.Tests.UI/Extensions/WebApplicationInstanceExtensions.cs

@@ -1,6 +1,8 @@
 using Lombiq.Tests.UI.Services;
 using Shouldly;
 using System;
+using System.Collections.Generic;
+using System.Linq;
 using System.Threading;
 using System.Threading.Tasks;
 
@@ -12,19 +14,35 @@ public static class WebApplicationInstanceExtensions
     /// Asserting that the logs should be empty. When they aren't the Shouldly exception will contain the logs'
     /// contents.
     /// </summary>
+    /// <param name="permittedErrorLines">
+    /// If not <see langword="null"/> or empty, each line is split and any lines containing <c>|ERROR|</c> will be
+    /// ignored if they contain any string from this collection (case-insensitive).
+    /// </param>
     public static async Task LogsShouldBeEmptyAsync(
         this IWebApplicationInstance webApplicationInstance,
         bool canContainWarnings = false,
+        ICollection<string> permittedErrorLines = null,
         CancellationToken cancellationToken = default)
     {
         if (cancellationToken == default) cancellationToken = CancellationToken.None;
+        permittedErrorLines ??= Array.Empty<string>();
 
         var logOutput = await webApplicationInstance.GetLogOutputAsync(cancellationToken);
 
         if (canContainWarnings)
         {
-            logOutput.ShouldNotContain("|ERROR|");
             logOutput.ShouldNotContain("|FATAL|");
+
+            var errorLines = logOutput
+                .SplitByNewLines()
+                .Where(line => line.Contains("|ERROR|"));
+
+            if (permittedErrorLines.Any())
+            {
+                errorLines = errorLines.Where(line => !permittedErrorLines.Any(line.ContainsOrdinalIgnoreCase));
+            }
+
+            errorLines.ShouldBeEmpty();
         }
         else
         {

Lombiq.Tests.UI/Helpers/DirectoryHelper.cs

@@ -41,4 +41,25 @@ public static void SafelyDeleteDirectoryIfExists(string path, int maxTryCount =
             }
         }
     }
+
+    /// <summary>
+    /// Creates a directory with the given path and a numeric suffix from 1 to <see cref="int.MaxValue"/>. This means if
+    /// the <paramref name="path"/> is <c>c:\MyDirectory</c> then it will first attempt to create <c>c:\MyDirectory1</c>
+    /// but if that already exists it will try to create <c>c:\MyDirectory2</c> instead, and so on.
+    /// </summary>
+    /// <param name="path">The base path to use when constructing the final path.</param>
+    /// <returns>The final path created.</returns>
+    public static string CreateEnumeratedDirectory(string path)
+    {
+        for (var i = 1; i < int.MaxValue; i++)
+        {
+            var newPath = path + i.ToTechnicalString();
+            if (Directory.Exists(newPath)) continue;
+
+            Directory.CreateDirectory(newPath);
+            return newPath;
+        }
+
+        throw new InvalidOperationException("Couldn't create a new directory within the integer space.");
+    }
 }

Lombiq.Tests.UI/Lombiq.Tests.UI.csproj

@@ -103,9 +103,9 @@
 
   <ItemGroup Condition="'$(NuGetBuild)' == 'true'">
     <PackageReference Include="Lombiq.Tests" Version="2.2.5" />
-    <PackageReference Include="Lombiq.HelpfulLibraries.Cli" Version="8.1.0" />
-    <PackageReference Include="Lombiq.HelpfulLibraries.OrchardCore" Version="8.1.0" />
-    <PackageReference Include="Lombiq.HelpfulLibraries.Refit" Version="8.1.0" />
+    <PackageReference Include="Lombiq.HelpfulLibraries.Cli" Version="8.1.1-alpha.4.tdeal-16" />
+    <PackageReference Include="Lombiq.HelpfulLibraries.OrchardCore" Version="8.1.1-alpha.4.tdeal-16" />
+    <PackageReference Include="Lombiq.HelpfulLibraries.Refit" Version="8.1.1-alpha.4.tdeal-16" />
     <PackageReference Include="Lombiq.Npm.Targets" Version="1.4.0" />
   </ItemGroup>
 

Lombiq.Tests.UI/SecurityScanning/AutomationFrameworkPlans/Baseline.yml

@@ -42,9 +42,24 @@ jobs:
     enableTags: false
     disableAllRules: false
   rules:
+  # This needs to be disabled during UI testing, because a local app needn't use HSTS. It's also something commonly
+  # configured outside the app, like in Cloudflare.
   - id: 10035
     name: "Strict-Transport-Security Header"
     threshold: "off"
+  # This is required for <script> blocks which OC uses extensively. The rule may be removed when OC starts to provide
+  # cryptographic nonce for these script blocks (see https://github.com/OrchardCMS/OrchardCore/issues/13389).
+  - id: 10055
+    name: "script-src includes unsafe-inline"
+    threshold: "off"
+  # This is a low-risk alert, enforcing it is undesirable for branding.
+  - id: 10037
+    name: "Server Leaks Information via 'X-Powered-By' HTTP Response Header Field(s)"
+    threshold: "off"
+  # This rule generates false positives on UUIDs or random numeric sequences.
+  - id: 10062
+    name: "The response contains Personally Identifiable Information, such as CC number, SSN and similar sensitive data."
+    threshold: "high"
   name: "passiveScan-config"
   type: "passiveScan-config"
 - parameters: {}

Lombiq.Tests.UI/SecurityScanning/AutomationFrameworkPlans/FullScan.yml

@@ -42,9 +42,24 @@ jobs:
     enableTags: false
     disableAllRules: false
   rules:
+  # This needs to be disabled during UI testing, because a local app needn't use HSTS. It's also something commonly
+  # configured outside the app, like in Cloudflare.
   - id: 10035
     name: "Strict-Transport-Security Header"
     threshold: "off"
+  # This is required for <script> blocks which OC uses extensively. The rule may be removed when OC starts to provide
+  # cryptographic nonce for these script blocks (see https://github.com/OrchardCMS/OrchardCore/issues/13389).
+  - id: 10055
+    name: "script-src includes unsafe-inline"
+    threshold: "off"
+  # This is a low-risk alert, enforcing it is undesirable for branding.
+  - id: 10037
+    name: "Server Leaks Information via 'X-Powered-By' HTTP Response Header Field(s)"
+    threshold: "off"
+  # This rule generates false positives on UUIDs or random numeric sequences.
+  - id: 10062
+    name: "The response contains Personally Identifiable Information, such as CC number, SSN and similar sensitive data."
+    threshold: "high"
   name: "passiveScan-config"
   type: "passiveScan-config"
 - parameters: {}

Lombiq.Tests.UI/SecurityScanning/AutomationFrameworkPlans/GraphQL.yml

@@ -42,9 +42,24 @@ jobs:
     enableTags: false
     disableAllRules: false
   rules:
+  # This needs to be disabled during UI testing, because a local app needn't use HSTS. It's also something commonly
+  # configured outside the app, like in Cloudflare.
   - id: 10035
     name: "Strict-Transport-Security Header"
     threshold: "off"
+  # This is required for <script> blocks which OC uses extensively. The rule may be removed when OC starts to provide
+  # cryptographic nonce for these script blocks (see https://github.com/OrchardCMS/OrchardCore/issues/13389).
+  - id: 10055
+    name: "script-src includes unsafe-inline"
+    threshold: "off"
+  # This is a low-risk alert, enforcing it is undesirable for branding.
+  - id: 10037
+    name: "Server Leaks Information via 'X-Powered-By' HTTP Response Header Field(s)"
+    threshold: "off"
+  # This rule generates false positives on UUIDs or random numeric sequences.
+  - id: 10062
+    name: "The response contains Personally Identifiable Information, such as CC number, SSN and similar sensitive data."
+    threshold: "high"
   name: "passiveScan-config"
   type: "passiveScan-config"
 - parameters:

Lombiq.Tests.UI/SecurityScanning/AutomationFrameworkPlans/OpenAPI.yml

@@ -42,9 +42,24 @@ jobs:
     enableTags: false
     disableAllRules: false
   rules:
+  # This needs to be disabled during UI testing, because a local app needn't use HSTS. It's also something commonly
+  # configured outside the app, like in Cloudflare.
   - id: 10035
     name: "Strict-Transport-Security Header"
     threshold: "off"
+  # This is required for <script> blocks which OC uses extensively. The rule may be removed when OC starts to provide
+  # cryptographic nonce for these script blocks (see https://github.com/OrchardCMS/OrchardCore/issues/13389).
+  - id: 10055
+    name: "script-src includes unsafe-inline"
+    threshold: "off"
+  # This is a low-risk alert, enforcing it is undesirable for branding.
+  - id: 10037
+    name: "Server Leaks Information via 'X-Powered-By' HTTP Response Header Field(s)"
+    threshold: "off"
+  # This rule generates false positives on UUIDs or random numeric sequences.
+  - id: 10062
+    name: "The response contains Personally Identifiable Information, such as CC number, SSN and similar sensitive data."
+    threshold: "high"
   name: "passiveScan-config"
   type: "passiveScan-config"
 - parameters: {}