Dnn.Platform icon indicating copy to clipboard operation
Dnn.Platform copied to clipboard

[Bug]: Faulty MoveTabAfter and MoveTabBefore logic

Open timi-ty opened this issue 9 months ago • 1 comments

Is there an existing issue for this?

  • [x] I have searched the existing issues

What happened?

The procedures: MoveTabBefore and MoveTabAfter, in https://github.com/dnnsoftware/Dnn.Platform/blob/develop/DNN%20Platform/Website/Providers/DataProviders/SqlDataProvider/DotNetNuke.Data.SqlDataProvider do not work correctly in case several tabs (pages) of a parent have the same TabOrder value. It is then not possible to insert a tab into the series of pages with the same TabOrder value.

Steps to reproduce?

  1. Go to the content tab on the admin panel
  2. Click on 'Pages'
  3. Create two or more pages with the same TabOrder value (change the TabOrder value within the database as this change cannot be performed explicitly from the UI)
  4. Move a page of a different TabOrder value, between two pages of the same TabOrder value.
  5. Refresh the site

Current Behavior

Home, 404 Error Page, Page One, and Page Two have the same TabOrder value. Attempting to move Activity Feed with a different TabOrder value, between them, fails.

The page with a different TabOrder value (Activity Feed) is moved above all tabs with the same TabOrder value.

Image Image Image

Expected Behavior

Activity Feed should be in between 404 Error Page and Page One

Relevant log output


Anything else?

No response

Affected Versions

9.13.6 (latest release)

What browsers are you seeing the problem on?

Chrome, Microsoft Edge

Code of Conduct

  • [x] I agree to follow this project's Code of Conduct

timi-ty avatar Feb 17 '25 16:02 timi-ty

Do you know what caused you to get into a scenario where the TabOrder of multiple pages was the same? As you noted, there's not a UI that should allow you to get into that scenario.

bdukes avatar Feb 17 '25 16:02 bdukes

@bdukes we found the root cause of this. TDLR; it can only happen when more than one move procedure is called concurrently which is possible if two or more site admins happen to be moving pages at the same time.

Root Cause Analysis: DNN TabOrder Race Condition

Why Do Duplicate TabOrder Values Exist?

Given the requested fix presumes that duplicates may be present, we asked:
“Why should sibling pages ever share the same TabOrder value in the first place?”

Answer: This should not occur during sequential execution. Duplicate TabOrder values only arise when concurrent MovePage calls target the same reference position and interleave so that both place a page at the identical calculated TabOrder.


Abnormal Nature of the Failure

Duplicates are an extremely rare failure mode because they:

  • Require precise timing of concurrent operations targeting the same reference page.
  • Are non‑deterministic, depending on network latency and request scheduling.
Image

Normal dbo.Tabs state. No duplicate tab order values under the same parent+portal.


Formal Proof: Sequential Execution Cannot Create Duplicates

A single‑threaded sequence of MovePage operations proceeds as follows:

  1. Remove source page (TabOrder = -1).
  2. Read the target position.
  3. Create gap by applying +2 increments.
  4. Insert the page into the new gap.

Because each operation updates a distinct range before inserting, sequential execution cannot produce duplicates.


Race Condition Mechanism (Concurrent Execution Required)

When two threads operate at the same time against the same reference page, they can each read the same TabOrder and compute the same destination:

T1: Thread A & B read TabOrder = X from the same reference page
T2: Thread A & B create a gap at position X + 2
T3: Thread A & B both set their page to TabOrder = X + 2
Result: Duplicate TabOrder values

This outcome is enabled by ASP.NET’s multi‑threaded environment and a lack of concurrency control inside the stored procedures.


Vulnerable Code

File: 07.03.03.SqlDataProvider
Procedures: MoveTabBefore, MoveTabAfter, MoveTabToParent

SET @TabOrder = (SELECT TabOrder FROM dbo.Tabs WHERE TabID = @AfterTabId)
UPDATE dbo.Tabs SET TabOrder = TabOrder + 2 WHERE TabOrder > @TabOrder
UPDATE dbo.Tabs SET TabOrder = @TabOrder + 2 WHERE TabID = @TabId

There is no locking to prevent multiple threads from reading the same @TabOrder and writing the same destination value.


Evidence

  • Original DB State
Image
  • Run Sequential Tests (covers all edge cases for how the procedures may be invoked)
Image
  • DB State After Sequential Tests
Image
  • Run Concurrent Tests (covers all edge cases for how the procedures may be invoked)
Image
  • DB State After Concurrent Tests
Image

Test Results

  1. Sequential test: No duplicates created.
  2. Concurrent test: Successfully created duplicate TabOrder values.
  3. Database verification: Confirmed multiple pages with identical TabOrder.

Technical Findings

  • Duplicate TabOrder values can only be created through concurrent MovePage calls.
  • ASP.NET provides a multi‑threaded request environment that makes the race condition possible.
  • The stored procedures lack concurrency control mechanisms.
  • The issue exists in procedures unchanged since version 07.03.03.

Conclusion

The presence of duplicate TabOrder values is not an expected steady state. It occurs only due to a race condition in the MovePage stored procedures when multiple operations target the same reference position simultaneously. This violates the assumption of TabOrder uniqueness but requires very specific timing to manifest.


timi-ty avatar Jul 28 '25 14:07 timi-ty

Done in #6633

valadas avatar Jul 30 '25 02:07 valadas