Merge pull request #334 from Lombiq/issue/TDEAL-16

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

@@ -18,6 +18,12 @@ namespace Lombiq.Tests.UI.Samples.Tests;
 // sure to also check out the corresponding documentation page:
 // https://github.com/Lombiq/UI-Testing-Toolbox/blob/dev/Lombiq.Tests.UI/Docs/SecurityScanning.md.
 
+// Most common alerts can be resolved by using the OrchardCoreBuilder.ConfigureSecurityDefaultsWithStaticFiles()
+// extension method from Lombiq.HelpfulLibraries.OrchardCore. It's worth enabling in in your Program and then verifying
+// that everything still works on the site before really getting into security scanning. If you experience any problems
+// related to Content-Security-Policy, take a look at the documentation of IContentSecurityPolicyProvider and
+// ContentSecurityPolicyAttribute to adjust the permissions, because these defaults are rather strict out of the box.
+
 // Note that security scanning has cross-platform support, but due to the limitations of virtualization under Windows in
 // GitHub Actions, these tests won't work there. They'll work on a Windows desktop though.
 public class SecurityScanningTests : UITestBase
@@ -31,44 +37,50 @@ public SecurityScanningTests(ITestOutputHelper testOutputHelper)
 
     // We're running one of ZAP's built-in scans, the Baseline scan. This, as the name suggests, provides some
     // rudimentary security checks. While you can start with this, we recommend running the Full Scan, for which there
-    // similarly is an extension method as well.
+    // similarly is an extension method as well. This can take a very long time. If you want to have a broader scan in
+    // your CI test runs, use the RunAndConfigureAndAssertFullSecurityScanForContinuousIntegrationAsync extension method
+    // instead. It applies some limitations to the full scan, including time limits to the active scan portion that
+    // normally takes the longest.
 
     // If you're new to security scanning, starting with exactly this is probably a good idea. Most possibly your app
     // will fail the scan, but don't worry! You'll get a nice report about the findings in the failure dump.
     [Fact]
     public Task BasicSecurityScanShouldPass() =>
-        ExecuteTestAfterSetupAsync(context => context.RunAndAssertBaselineSecurityScanAsync());
+        ExecuteTestAfterSetupAsync(
+            context => context.RunAndAssertBaselineSecurityScanAsync(),
+            // You should configure the assertion that checks the app logs to accept some common cases that only should
+            // appear during security scanning. If you launch a full scan, this is automatically configured by the
+            // RunAndConfigureAndAssertFullSecurityScanForContinuousIntegrationAsync extension method.
+            changeConfiguration: configuration => configuration.UseAssertAppLogsForSecurityScan());
 
     // Time for some custom configuration! While this scan also runs the Baseline scan, it does this with several
     // adjustments:
-    // - Also runs ZAP's Ajax Spider (https://www.zaproxy.org/docs/desktop/addons/ajax-spider/automation/). This is
-    //   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.
-    //   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
-    //   disable rules more specifically instead of the whole scan.
-    // - Configures sign in with a user account. This is what the scan will start with. With the Blog recipe it doesn't
-    //   matter too much, since nothing on the frontend will change, but you can use this to scan authenticated features
-    //   too. Note that since ZAP uses its own spider, not the browser accessed by the test, user sessions are not
-    //   shared, so such an explicit sign in is necessary.
+    // - Also runs ZAP's Ajax Spider (https://www.zaproxy.org/docs/desktop/addons/ajax-spider/automation/). Usually this
+    //   is only necessary for sites that are single page applications (SPA).
+    // - Excludes certain URLs from the scan completely. Use this if you don't want ZAP to process those pages at all.
+    // - Disables one of ZAP's passive scan rules for the whole scan.
+    // - Also disables a rule but only for the /about page. Use this to disable rules more specifically instead of the
+    //   whole scan.
+    // - Configures sign in with a user account. This is what the scan will start with. This doesn't matter much with
+    //   the Blog recipe, because nothing on the frontend will change. You can use this to scan authenticated features
+    //   too. This is necessary because ZAP uses its own spider so it doesn't share session or cookies with the browser.
     // - The assertion on the scan results is custom. Use this if you (conditionally) want to assert on the results
     //   differently from the global context.Configuration.SecurityScanningConfiguration.AssertSecurityScanResult. The
     //   default there is "no scanning alert is allowed"; we expect some alerts here.
+    // - The suppressions are not actually necessary here. The BasicSecurityScanShouldPass works fine without them. They
+    //   are only present to illustrate the type of adjustments you may want for your own site.
     [Fact]
     public Task SecurityScanWithCustomConfigurationShouldPass() =>
         ExecuteTestAfterSetupAsync(
             context => context.RunAndAssertBaselineSecurityScanAsync(
                 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)));
+                sarifLog => sarifLog.Runs[0].Results.Count.ShouldBeInRange(17, 22)),
+            changeConfiguration: configuration => configuration.UseAssertAppLogsForSecurityScan());
 
     // Let's get low-level into ZAP's configuration now. While the .NET configuration API of the Lombiq UI Testing
     // Toolbox covers the most important ways to configure ZAP, sometimes you need more. For this, you have complete
@@ -110,7 +122,8 @@ public Task SecurityScanWithCustomAutomationFrameworkPlanShouldPass() =>
                         // more pages to be scanned.
                         spiderParameters.Add("maxDepth", "8");
                     }),
-                sarifLog => SecurityScanningConfiguration.AssertSecurityScanHasNoAlerts(context, sarifLog)));
+                sarifLog => SecurityScanningConfiguration.AssertSecurityScanHasNoAlerts(context, sarifLog)),
+            changeConfiguration: configuration => configuration.UseAssertAppLogsForSecurityScan());
 
     // Overriding the default setup so we can have a simpler site, simplifying the security scan for the purpose of this
     // demo. For a real app's security scan you needn't (shouldn't) do this though; always run the scan on the actual
@@ -141,13 +154,6 @@ protected override Task ExecuteTestAfterSetupAsync(
             {
                 configuration.HtmlValidationConfiguration.RunHtmlValidationAssertionOnAllPageChanges = false;
 
-                // Note how we specify an assertion too. This is because ZAP actually notices a few security issues with
-                // vanilla Orchard Core. These, however, are more like artifacts of running the app locally and out of
-                // the box without any real configuration. So, to make the tests pass, we need to override the default
-                // assertion that would fail the test if any issue is found.
-
-                // Don't do this at home! Fix the issues instead. This is only here to have a smoother demo.
-                configuration.SecurityScanningConfiguration.AssertSecurityScanResult = (_, _) => { };
                 // Check out the rest of SecurityScanningConfiguration too!
 
                 await changeConfigurationAsync(configuration);

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/Controllers/ErrorController.cs

@@ -9,7 +9,7 @@ namespace Lombiq.Tests.UI.Shortcuts.Controllers;
 [DevelopmentAndLocalhostOnly]
 public class ErrorController : Controller
 {
-    public const string ExceptionMessage = "This action causes an exception!";
+    public const string ExceptionMessage = "This action intentionally causes an exception!";
 
     // This only happens in the CI for some reason.
     [SuppressMessage("Usage", "CA1822:Mark members as static", Justification = "It's a controller action.")]

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.5.tdeal-16" />
   </ItemGroup>
 
 </Project>

Lombiq.Tests.UI/Docs/SecurityScanning.md

@@ -19,8 +19,28 @@ 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`:
+
+```yaml
+  build-and-test:
+    name: Build and Test
+    # See https://github.com/Lombiq/GitHub-Actions/blob/dev/.github/workflows/build-and-test-orchard-core.yml.
+    uses: Lombiq/GitHub-Actions/.github/workflows/build-and-test-orchard-core.yml@dev
+    with:
+      machine-types: '["windows-latest"]'
+      test-filter: "FullyQualifiedName!~SecurityScanningTests"
+```
+
 ## Troubleshooting
 
+- Most common alerts in Orchard Core can be resolved by [using the extension method in Lombiq Helpful Libraries](https://github.com/Lombiq/Helpful-Libraries/blob/dev/Lombiq.HelpfulLibraries.OrchardCore/Docs/Security.md) like this: `orchardCoreBuilder.ConfigureSecurityDefaults(allowInlineStyle: true)`.
 - 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).
 - If an alert is a false positive, follow [the official docs](https://www.zaproxy.org/faq/how-do-i-handle-a-false-positive/). You can use the [`alertFilter` job](https://www.zaproxy.org/docs/desktop/addons/alert-filters/automation/) to ignore alerts in very specific conditions. You can also access this via the .NET configuration API.
 - ZAP didn't find everything in your app? By default, ZAP has a crawl depth of 5 for its standard spider and 10 for its AJAX spider. Set `maxDepth` (and `maxChildren`) [for `spider`](https://www.zaproxy.org/docs/desktop/addons/automation-framework/job-spider/) and `maxCrawlDepth` [for `spiderAjax`](https://www.zaproxy.org/docs/desktop/addons/ajax-spider/automation/).

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/ShouldlyExtensions.cs

@@ -1,4 +1,8 @@
+using System;
+using System.Collections.Generic;
 using System.Diagnostics.CodeAnalysis;
+using System.Linq;
+using System.Text.Json;
 
 namespace Shouldly;
 
@@ -17,4 +21,41 @@ public static void ShouldBeAsString(this object actual, object expected, string
 
         actualText.ShouldBe(expected?.ToString(), customMessage);
     }
+
+    /// <summary>
+    /// Filters the provided <paramref name="enumerable"/> by the <paramref name="condition"/>. If the result is not
+    /// empty, a <see cref="ShouldAssertException"/> is thrown. A JSON serialized string of the results is provided as
+    /// the custom message of the exception.
+    /// </summary>
+    /// <remarks><para>
+    /// This extension method is similar to <c>enumerable.ShouldNotContain(condition)</c>, but it offers much better
+    /// developer experience because all the offending items are listed right there in the exception message. The
+    /// overhead is minimal during non-exceptional operation (only the creation of an empty list), and it's well worth
+    /// the added clarity during failure.
+    /// </para></remarks>
+    public static void ShouldBeEmptyWhen<TItem>(
+        this IEnumerable<TItem> enumerable,
+        Func<TItem, bool> condition,
+        JsonSerializerOptions jsonSerializerOptions = null) =>
+        enumerable.ShouldBeEmptyWhen<TItem, TItem>(condition, messageTransform: null, jsonSerializerOptions);
+
+    /// <inheritdoc cref="ShouldBeEmptyWhen{TItem}"/>
+    /// <param name="messageTransform">
+    /// When not <see langword="null"/>, the results are transformed with this method before they
+    /// are serialized into JSON.
+    /// </param>
+    public static void ShouldBeEmptyWhen<TItem, TMessage>(
+        this IEnumerable<TItem> enumerable,
+        Func<TItem, bool> condition,
+        Func<TItem, TMessage> messageTransform,
+        JsonSerializerOptions jsonSerializerOptions = null)
+    {
+        var results = enumerable.Where(condition).ToList();
+        if (!results.Any()) return;
+
+        var message = messageTransform == null
+            ? JsonSerializer.Serialize(results, jsonSerializerOptions)
+            : JsonSerializer.Serialize(results.Select(messageTransform), jsonSerializerOptions);
+        results.ShouldBeEmpty(message); // This will always throw at this point.
+    }
 }

Lombiq.Tests.UI/Extensions/TypedRouteUITestContextExtensions.cs

@@ -62,6 +62,28 @@ public static string GetRelativeUrlOfAction<TController>(
             .CreateFromExpression(actionExpressionAsync.StripResult(), additionalArguments, CreateServiceProvider(context))
             .ToString();
 
+    /// <summary>
+    /// Gets the absolute URL generated by <see cref="TypedRoute"/> for the <paramref name="actionExpression"/> in the
+    /// <typeparamref name="TController"/>.
+    /// </summary>
+    public static Uri GetAbsoluteUrlOfAction<TController>(
+        this UITestContext context,
+        Expression<Action<TController>> actionExpression,
+        params (string Key, object Value)[] additionalArguments)
+        where TController : ControllerBase =>
+        context.GetAbsoluteUri(context.GetRelativeUrlOfAction(actionExpression, additionalArguments));
+
+    /// <summary>
+    /// Gets the absolute URL generated by <see cref="TypedRoute"/> for the <paramref name="actionExpressionAsync"/> in
+    /// the <typeparamref name="TController"/>.
+    /// </summary>
+    public static Uri GetAbsoluteUrlOfAction<TController>(
+        this UITestContext context,
+        Expression<Func<TController, Task>> actionExpressionAsync,
+        params (string Key, object Value)[] additionalArguments)
+        where TController : ControllerBase =>
+        context.GetAbsoluteUri(context.GetRelativeUrlOfAction(actionExpressionAsync, additionalArguments));
+
     private static IServiceProvider CreateServiceProvider(UITestContext context)
     {
         var services = new ServiceCollection();