OCC-223: Upgrade to Orchard Core 1.8 #412

Psichorex · 2024-02-22T09:23:56Z

Psichorex · 2024-02-26T17:17:35Z

@sarahelsaig Could you please checkout this ZAP report and determine how valid of these errors are? They were not present before now with .NET 8 and OC 1.8? maybe they came to live.

2024-02-26-ZAP-Report-localhost.json

sarahelsaig · 2024-02-26T17:51:01Z

It's nonsense. It tries to inject SQL text into shopping-cart-widget.min.css?v=k7gHY8wfoxBUfO_f_yLhCB7L5QFrXqr1XoCJ8UOqx4I (a static resource) by changing the v argument's value. That argument is not used by the server, it's only there for client side cache busting when the css file changes. So the loading time was "manipulated" by tricking the browser to load an not cached version of the css file. Congrats to the hacker known as ZAP. 🙃

In general I think the "The query time is controllable using parameter value [some SQL injection]" type errors are safe to ignore, because we never handle raw SQL and usually it's like a few hundred millisecond difference which can be simply random fluctuation, one request incidentally triggering .NET garbage collection and the other not, etc

Psichorex · 2024-02-26T18:35:05Z

Okay now I resolved that. The following error still persist though and I also think that this is another close to nonsense error:
2024-02-26 18:17:49.0025 - An exception has occurred while interacting with the page https://localhost:9027/ (My Shop - Orchard Setup). 2024-02-26 18:17:49.0045 - The test failed with the following exception: Shouldly.ShouldAssertException: Shouldly uses your source code to generate its great error messages, build your test project with full debug information to get better error messages The provided expression should be empty but had 1 item and was ["2024-02-26 19:17:27.7620|Default|00-d1384c43efd3c6f22046919e903789ca-d537c6bb36bf1168-00||Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddleware|ERROR|An unhandled exception has occurred while executing the request. System.IO.IOException: A fájlnév, a könyvtárnév vagy a kötetcímke szintaxisa nem megfelelő. : 'C:\Lombiq\OrchardCore.Commerce\test\OrchardCore.Commerce.Tests.UI\bin\Debug\net8.0\Temp\9db19990-aafe-4978-8627-a44ef46e07cd\App\App_Data\Sites\Default\Media\ProductImages\sample-product-image.png\'"] at Lombiq.Tests.UI.Extensions.WebApplicationInstanceExtensions.LogsShouldBeEmptyAsync(IWebApplicationInstance webApplicationInstance, Boolean canContainWarnings, ICollection 1 permittedErrorLines, CancellationToken cancellationToken) at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw() at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task, ConfigureAwaitOptions options) at Lombiq.Tests.UI.Extensions.VerificationUITestContextExtensions.AssertLogsAsync(UITestContext context) at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw() at Lombiq.Tests.UI.Extensions.VerificationUITestContextExtensions.AssertLogsAsync(UITestContext context) at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw() at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task, ConfigureAwaitOptions options) at Lombiq.Tests.UI.Services.UITestExecutionSession.ExecuteAsync(Int32 retryCount, String dumpRootPath) 2024-02-26 18:17:49.2918 INFO > Take page snapshot #01 - FailureDumpPageSnapshot 2024-02-26 18:17:51.3444 WARN - Failed to take CDP snapshot. PageSource snapshot will be taken. OpenQA.Selenium.WebDriverException: Unexpected error creating WebSocket DevTools session. ---> OpenQA.Selenium.WebDriverException: DevTools version is not in the supported range. Desired version=121, range=5. Supported versions: 116, 115, 114, 85 at OpenQA.Selenium.DevTools.DevToolsDomains.MatchDomainsVersion(Int32 desiredVersion, Int32 versionRange) at OpenQA.Selenium.DevTools.DevToolsDomains.InitializeDomains(Int32 protocolVersion, DevToolsSession session, Int32 versionRange) at OpenQA.Selenium.DevTools.DevToolsDomains.InitializeDomains(Int32 protocolVersion, DevToolsSession session) at OpenQA.Selenium.DevTools.DevToolsSession.StartSession(Int32 requestedProtocolVersion) at OpenQA.Selenium.Chromium.ChromiumDriver.<>c__DisplayClass34_1.<<GetDevToolsSession>b__0>d.MoveNext() --- End of stack trace from previous location --- at OpenQA.Selenium.Chromium.ChromiumDriver.GetDevToolsSession(Int32 devToolsProtocolVersion) --- End of inner exception stack trace --- at OpenQA.Selenium.Chromium.ChromiumDriver.GetDevToolsSession(Int32 devToolsProtocolVersion) at OpenQA.Selenium.Chromium.ChromiumDriver.GetDevToolsSession() at Atata.CdpPageSnapshotStrategy.TakeSnapshot(AtataContext context) at Atata.CdpOrPageSourcePageSnapshotStrategy.TakeSnapshot(AtataContext context) 2024-02-26 18:17:51.3485 INFO < Take page snapshot #01 - FailureDumpPageSnapshot (2.056s) >> "01 - FailureDumpPageSnapshot.html" 2024-02-26 18:17:51.3537 - Attempting to stop the Orchard Core instance. 2024-02-26 18:17:51.3584 - The Orchard Core instance was stopped.

So this by the URI that it whines about seems to be fine. I don't have that temp file that it cries about but under the SQLite snapshots this path seems to be fine and image is there.
This one actually gets caught on CI aswell. https://github.com/OrchardCMS/OrchardCore.Commerce/actions/runs/8006384127?pr=412

Can you spot something maybe that I can't? On CI there is a double OrchardCore.Commerce/OrchardCore.Commerce/ in the path that seems fishy but that is not the case locally set it is failing.

Psichorex · 2024-02-27T10:10:32Z

Okay so the previous error is coming from a missing Temp folder. Which I have no idea about why is happening. Also it doesn't break anything.

So I don't actually know why we need Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddleware but the thing is that it is actually running on CI which is not supposed to be Development ENV is also questionable for me.

Piedone · 2024-02-27T20:58:24Z

src/Libraries/OrchardCore.Commerce.Abstractions/Exceptions/FrontendException.cs


 namespace OrchardCore.Commerce.Abstractions.Exceptions;

 [Serializable]
+#pragma warning disable S3925 // "ISerializable" should be implemented correctly
+// Exception(SerializationInfo info, StreamingContext context) is obsolete and should not be called.


Always put these comments above the pragmas.

Also can you use [SuppressMessage] instead? I suggest only using #pragma inside method bodies. It should be avoided on class/property declarations and method signatures, because it's horrible for readability.

sarahelsaig

It generated a bunch of .prettierrc.js files at build. I guess that should be added to the _.gitignore:?

sarahelsaig · 2024-02-28T02:12:03Z

src/Libraries/OrchardCore.Commerce.Abstractions/Exceptions/FrontendException.cs


 namespace OrchardCore.Commerce.Abstractions.Exceptions;

 [Serializable]
+#pragma warning disable S3925 // "ISerializable" should be implemented correctly
+// Exception(SerializationInfo info, StreamingContext context) is obsolete and should not be called.


Also can you use [SuppressMessage] instead? I suggest only using #pragma inside method bodies. It should be avoided on class/property declarations and method signatures, because it's horrible for readability.

sarahelsaig · 2024-02-28T02:54:43Z

The following error still persist though and I also think that this is another close to nonsense error

That's a known error that I was already working on in #410 before the OC 1.8 upgrade complicated things. It's ZAP probing for path traversal exploits, which correctly results in an error log and UITT should permit this. I ended up disabling path traversal tests altogether, because as long as you only use ASP.NET/OC for resources it's not a real security concern and it generates a ton of error logs and weird false positives. Like this case for example. wtf am I supposed to do with this?

To straighten things out, I've merged this branch into issue/OCC-218 and made its PR target your branch. Sadly this make OCC-218 a blocker for this issue, because you need its changes to fix SecurityScanningTests.

Psichorex · 2024-02-28T12:36:40Z

This makes much more sense now. Well it's non sense but it makes sense to me at least :D I will halt the progress here in this case. Please ping me if there are any news on OCC-218. I started chewing my hair due to these Security error logs.

sarahelsaig · 2024-02-28T12:58:01Z

You know times are bad when Roland starts spitting hairballs... 😿

Psichorex · 2024-02-28T13:09:02Z

You know times are bad when Roland starts spitting hairballs... 😿

After doing OC 1.6 - 1.7 - 1.8 upgrades I am hypersensitive to retardedly failing UI tests. At least now I now whener I see a ?= on the internet I should deploy sql injection attac

…agement (#410)

sarahelsaig · 2024-02-28T18:26:30Z

@Psichorex it's good now. Did you need to do anything else or is this ready for review?

Psichorex · 2024-02-28T19:45:39Z

@Psichorex it's good now. Did you need to do anything else or is this ready for review?
It's up for review. Thanks for saving my hair.

Psichorex · 2024-02-28T19:56:29Z

It generated a bunch of .prettierrc.js files at build. I guess that should be added to the _.gitignore:?

Same for me. I love random JS commitables.

sarahelsaig · 2024-02-28T19:59:20Z

Wait, never mind. Please address this first.

Psichorex · 2024-02-28T20:13:52Z

Wait, never mind. Please address this first.

Thats already pushed. CI is running.

sarahelsaig · 2024-02-29T01:58:16Z

src/Libraries/OrchardCore.Commerce.Abstractions/Exceptions/FrontendException.cs

+[SuppressMessage(
+    "Maintainability",
+    "S3925: ISerializable should be implemented correctly",
+    Justification = "Exception(SerializationInfo info, StreamingContext context) is obsolete and should not be called.")]


In this case why isn't this rule turned off altogether? Also based on this issue it seems like if you remove [Serializable] then you don't have to implement the overload with SerializationInfo so the attribute behaves like an opt-in. Also isn't the [Serializable] attribute and the overload you removed related anyway?

That is actually true. [Serializable] can be removed here.

Could be turned off but this is the first time I came across a problem rooting from this so I wuoldn't actually turn it off now that I could just remove it.

I asked about turning it off because the text in the Justification made it sound like you are addressing an outdated analyzer rule by suppressing every occurrence individually. That's not ok even if there is only one case. Since removing [Serializable] did the trick we don't have to worry about it.

sarahelsaig · 2024-02-29T02:02:33Z

src/Libraries/OrchardCore.Commerce.AddressDataType/Abstractions/IAddressFormatter.cs

-    string Format(Address address) =>
-        address is null
-        ? "-"
-        : JoinNotNullAndNotWhiteSpace(
-            Environment.NewLine,
-            address.Department,
-            address.Company,
-            address.StreetAddress1,
-            address.StreetAddress2,
-            JoinNotNullAndNotWhiteSpace(separator: " ", address.City, address.Province, address.PostalCode),
-            address.Region).ToUpperInvariant();
+    string Format(Address address);


Why did you remove the default implementation?

Well this was hard to track :(
So basically this warning is the cause :

The warning wants us to use concrete types. The default implementation was not in the concrete type but the interface. So I moved the default inmplementation to the inheritor class instead of the interface. I thought we don't do implementations in interfaces. Looks pretty wierd. I guess it had a reason though. So it was moved to the DefaultAddressFormatter : IAddressFormatted class.

I looked into the git history of the file and looks like this was from before we took over. I don't think this was ever a good use of the default implementation feature, because this is the only method in the interface so the only reason to create a new class implementing it is to "override" this implementation anyway.
So it was a good thing you did here.

Psichorex added 14 commits February 22, 2024 09:48

Upgrading TargetFramework and OC versions.

1b7fe0d

Making Migrations Async.

cf68cc0

Updating Analyzers.

db8ce36

Making Service methods async and addressing warnings.

139ff32

More ToAsync changes.

298f881

Fixing method implementation.

c345677

Even morer Async fixes.

85ce2af

Addressing warnings.

7aa9f2d

Addressing warnings.

87439dc

More warnings.

26080fd

Fixing tests.

eae5dae

Fixing tests.

470be44

Fixing nuget versions.

b0945fc

Consolidating packages.

c9b3aee

Psichorex added 2 commits February 27, 2024 17:09

Fixing test.

f8a4957

Fixing tests.

92dd809

Piedone reviewed Feb 27, 2024

View reviewed changes

sarahelsaig requested changes Feb 28, 2024

View reviewed changes

OCC-218: Update NuGet packages and migrate to centralized package man…

7669173

…agement (#410)

Using SuppressMessage.

541985a

Merge remote-tracking branch 'origin/issue/OCC-223' into issue/OCC-223

723981b

Piedone mentioned this pull request Feb 29, 2024

Running FullSecurityScanShouldPass locally fails (OCC-218) #404

Closed

sarahelsaig requested changes Feb 29, 2024

View reviewed changes

Removing Attribute.

5443fa6

sarahelsaig merged commit 81a155d into main Feb 29, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OCC-223: Upgrade to Orchard Core 1.8 #412

OCC-223: Upgrade to Orchard Core 1.8 #412

Psichorex commented Feb 22, 2024 •

edited by github-actions bot

Loading

Psichorex commented Feb 26, 2024

sarahelsaig commented Feb 26, 2024

Psichorex commented Feb 26, 2024 •

edited

Loading

Psichorex commented Feb 27, 2024 •

edited

Loading

Piedone Feb 27, 2024

sarahelsaig Feb 28, 2024

sarahelsaig left a comment

sarahelsaig Feb 28, 2024

sarahelsaig commented Feb 28, 2024

Psichorex commented Feb 28, 2024

sarahelsaig commented Feb 28, 2024

Psichorex commented Feb 28, 2024

sarahelsaig commented Feb 28, 2024

Psichorex commented Feb 28, 2024

Psichorex commented Feb 28, 2024

sarahelsaig commented Feb 28, 2024

Psichorex commented Feb 28, 2024

sarahelsaig Feb 29, 2024

Psichorex Feb 29, 2024

Psichorex Feb 29, 2024

sarahelsaig Feb 29, 2024

sarahelsaig Feb 29, 2024

Psichorex Feb 29, 2024

sarahelsaig Feb 29, 2024

OCC-223: Upgrade to Orchard Core 1.8 #412

OCC-223: Upgrade to Orchard Core 1.8 #412

Conversation

Psichorex commented Feb 22, 2024 • edited by github-actions bot Loading

Psichorex commented Feb 26, 2024

sarahelsaig commented Feb 26, 2024

Psichorex commented Feb 26, 2024 • edited Loading

Psichorex commented Feb 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sarahelsaig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sarahelsaig commented Feb 28, 2024

Psichorex commented Feb 28, 2024

sarahelsaig commented Feb 28, 2024

Psichorex commented Feb 28, 2024

sarahelsaig commented Feb 28, 2024

Psichorex commented Feb 28, 2024

Psichorex commented Feb 28, 2024

sarahelsaig commented Feb 28, 2024

Psichorex commented Feb 28, 2024

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

Psichorex commented Feb 22, 2024 •

edited by github-actions bot

Loading

Psichorex commented Feb 26, 2024 •

edited

Loading

Psichorex commented Feb 27, 2024 •

edited

Loading