Skip to content

Debug mode: Marks UMB-DEBUG cookie as HttpOnly and Secure#21032

Merged
AndyButland merged 6 commits intomainfrom
v17/bugfix/web-profiling-httponly-cookie
Dec 4, 2025
Merged

Debug mode: Marks UMB-DEBUG cookie as HttpOnly and Secure#21032
AndyButland merged 6 commits intomainfrom
v17/bugfix/web-profiling-httponly-cookie

Conversation

@iOvergaard
Copy link
Contributor

@iOvergaard iOvergaard commented Dec 2, 2025

Summary

This PR addresses a security scanning alert about the debug profiling cookie and improves the overall cookie handling:

  • Fixes security issue: The UMB-DEBUG cookie now respects the UseHttps setting instead of hardcoding Secure=false
  • Adds httpOnly flag: Prevents JavaScript access to the cookie (XSS mitigation)
  • Uses SameSite=Strict: Limits cookie to same-site requests only
  • Converts to session cookie: No longer persists for 1 year; deleted when browser closes
  • Refactors to use ICookieManager: Improves encapsulation and consistency
  • Adds status verification: Frontend now verifies the cookie was actually set and warns users if it fails (e.g., cross-site scenario)

Fixes https://github.com/umbraco/Umbraco-CMS/security/code-scanning/1730

Changes

Backend:

  • Replaced direct HttpContext.Response.Cookies usage with ICookieManager
  • Cookie now uses secure: _globalSettings.UseHttps instead of secure: false
  • Added httpOnly: true and sameSiteMode: "Strict"
  • Removed 1-year expiration (now a session cookie)

Frontend:

  • Added verification that the profiler status was actually changed
  • Shows warning notification when toggle fails (explains same-site limitation)
  • Added loading state during toggle operation
  • Replaced unsafeHTML with umb-localize components

Note on SameSite=Strict

Because the cookie uses SameSite=Strict, it only works when the BackOffice and front-end share the same domain. For cross-site setups (e.g., headless), users can use ?umbDebug=true query string or X-UMB-DEBUG header instead.

Test steps

Cross-site scenario (should show warning)

  1. Run the BackOffice using Vite's dev server (npm run dev in the client folder)
  2. Navigate to Settings → Performance Profiling
  3. Toggle the "Activate the profiler by default" switch ON
  4. Expected: A warning notification appears explaining the cookie could not be set because the BackOffice and front-end are on different URLs

Same-site scenario (should work)

  1. Run Umbraco normally (not using Vite's dev server)
  2. Navigate to Settings → Performance Profiling
  3. Toggle the "Activate the profiler by default" switch ON
  4. Expected: Toggle stays ON, no warning
  5. Toggle the switch OFF
  6. Expected: Toggle stays OFF, no warning
  7. Close and reopen the browser
  8. Expected: Toggle is OFF (session cookie was deleted)

🤖 Generated with Claude Code

@iOvergaard iOvergaard marked this pull request as ready for review December 3, 2025 08:34
Copilot AI review requested due to automatic review settings December 3, 2025 08:34
@iOvergaard iOvergaard changed the title Debug mode: Marks UMB-DEBUG cookie as HttpOnly Debug mode: Marks UMB-DEBUG cookie as HttpOnly and Secure Dec 3, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR addresses a security vulnerability by improving the security configuration of the UMB-DEBUG cookie used for performance profiling. The changes ensure the cookie follows security best practices (HttpOnly, Secure based on settings, SameSite=Strict, session-only) and refactors the implementation to use ICookieManager for better encapsulation. The frontend is enhanced with proper verification that cookie operations succeed and provides user feedback when they fail due to cross-site restrictions.

Key Changes:

  • Improved cookie security settings (HttpOnly, dynamic Secure flag, SameSite=Strict, session cookie)
  • Refactored backend to use ICookieManager abstraction instead of direct cookie manipulation
  • Enhanced frontend with status verification and user-friendly error notifications

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
src/Umbraco.Web.Common/Repositories/WebProfilerRepository.cs Refactored to use ICookieManager with secure cookie settings (HttpOnly, dynamic Secure, SameSite=Strict) and removed persistent expiration
src/Umbraco.Web.UI.Client/src/packages/performance-profiling/dashboard-performance-profiling.element.ts Added verification of profiler status changes, loading states, error notifications, and replaced unsafeHTML with umb-localize components
src/Umbraco.Web.UI.Client/src/assets/lang/en.ts Added new error message localization strings and updated description to mention same-site limitation

Copy link
Contributor

@AndyButland AndyButland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good @iOvergaard and all works as expected. Toggle status is retained, the cookie is set with the appropriate settings and it works to enabled debug mode when browsing the front-end of the website.

I'll cherry-pick this back for 16.5 too.

@AndyButland AndyButland merged commit cb45437 into main Dec 4, 2025
27 of 28 checks passed
@AndyButland AndyButland deleted the v17/bugfix/web-profiling-httponly-cookie branch December 4, 2025 07:55
AndyButland pushed a commit that referenced this pull request Dec 4, 2025
* fix: sets profiling cookie to httpOnly and strict in order to run non-secure

* fix: adds extra message to explain when you can set a cookie

* fix: simplify cookie explanation comment in WebProfilerRepository

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>

* fix: checks that the profiler is actually enabled and/or disabled and warns the user if that is not the case

* Update src/Umbraco.Web.UI.Client/src/assets/lang/en.ts

Co-authored-by: Copilot <[email protected]>

---------

Co-authored-by: Claude <[email protected]>
Co-authored-by: Copilot <[email protected]>
@AndyButland
Copy link
Contributor

@iOvergaard - I've cherry-picked this over for 16.5 in the commit you can see linked above. I just had to rework how the context was consumed on the client. I think it's all good and have tested it out, but if you want to take a look please do.

@iOvergaard
Copy link
Contributor Author

All good, @AndyButland !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants