Skip to content

FreeScout: Keep administrators to the proxy - #996

Open
obenland wants to merge 3 commits into
WordPress:trunkfrom
obenland:add/freescout-admin-proxy
Open

obenland wants to merge 3 commits into
WordPress:trunkfrom
obenland:add/freescout-admin-proxy

Conversation

@obenland

@obenland obenland commented Oct 2, 2026 •

Copy link
Copy Markdown
Member

Requires administrators to use the helpdesk through the proxy. Agents are still allowed in from anywhere, since most of them don't have proxy access.

How it works

  • The signal comes from nginx. It sets a WPORG_PROXIED_REQUEST FastCGI param to 1 for the proxy's IP addresses and 0 for everyone else, the same check WPORG_PROXIED_REQUEST makes on WordPress.org. The proxy IP list stays with Systems instead of being copied into the module.
  • Enforcement is in RequireWordPressOrgLogin. When WPORG_SSO_REQUIRE_PROXY=true, an administrator's request without the param set to 1 ends their session. They're sent to the login page with an error, and polls get a 401. This applies however they logged in, break-glass included, so a stolen admin session doesn't work from outside the proxy.
  • A missing param fails closed. If nginx doesn't pass the param, nobody counts as proxied, and an error is logged once an hour. WPORG_SSO_REQUIRE_PROXY=false lets administrators back in.
  • Behavior is unchanged until the variable is set. It defaults to false.

Deploying

  1. Systems add the geo map and the fastcgi_param on the FreeScout VM (example in Modules/WPOrgSSO/README.md). The map needs the proxy's IPv6 addresses as well as IPv4.
  2. Set WPORG_SSO_REQUIRE_PROXY=true.

Testing

npm run freescout:test (from environments/). The new ProxyTest covers:

  • proxied administrators staying logged in;
  • logging out administrators who are unproxied, missing the param, or only sending a spoofed header;
  • polls getting a 401;
  • an SSO login from outside the proxy;
  • agents being unaffected;
  • nothing happening until the option is on.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features
    • Administrators can be required to access the app through a configured proxy, regardless of how they signed in. Requests without the expected proxy marker log administrators out; API polling requests receive a 401 response. Agents are unaffected.
    • Proxy enforcement is optional and disabled by default. Requests are not considered proxied if the required marker is missing or appears only in a request header.
  • Documentation
    • Added setup guidance for enabling proxy enforcement and configuring nginx to mark proxied requests.

With WPORG_SSO_REQUIRE_PROXY=true, WPOrgSSO logs administrators out of any
request that nginx doesn't mark as proxied with the WPORG_PROXIED_REQUEST
FastCGI param, however they logged in. A session stolen from an administrator
then doesn't work from outside the proxy. Agents aren't affected.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Copilot AI balanced review requested due to automatic review settings October 2, 2026 16:29

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@github-actions

github-actions Bot commented Oct 2, 2026

Copy link
Copy Markdown

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props obenland.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@coderabbitai

coderabbitai Bot commented Oct 2, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 68895f96-6a9b-42f1-a5f2-9831b7450752

📥 Commits

Reviewing files that changed from the base of the PR and between 26be950 and 8a0d5bb.

📒 Files selected for processing (2)
  • freescout.wordpress.net/Modules/WPOrgSSO/Providers/WPOrgSSOServiceProvider.php
  • freescout.wordpress.net/Modules/WPOrgSSO/README.md
💤 Files with no reviewable changes (1)
  • freescout.wordpress.net/Modules/WPOrgSSO/Providers/WPOrgSSOServiceProvider.php
🚧 Files skipped from review as they are similar to previous changes (1)
  • freescout.wordpress.net/Modules/WPOrgSSO/README.md

Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review.


📝 Walkthrough

Walkthrough

WPOrgSSO adds an optional proxy requirement for administrator access. When enabled, middleware logs out administrators whose requests lack the nginx proxy marker. The change adds configuration, provider checks, logout responses, deployment instructions, and tests.

Changes

Administrator proxy access

Layer / File(s) Summary
Proxy configuration and request marker
freescout.wordpress.net/Modules/WPOrgSSO/Config/config.php, freescout.wordpress.net/Modules/WPOrgSSO/Providers/WPOrgSSOServiceProvider.php, freescout.wordpress.net/Modules/WPOrgSSO/README.md, freescout.wordpress.net/README.md
Adds the require_proxy setting, defaulting to false. The provider checks whether the nginx server variable equals '1' and logs a configuration error at most once per 60 seconds when the variable is absent. The documentation describes the setting and nginx configuration.
Administrator request enforcement
freescout.wordpress.net/Modules/WPOrgSSO/Http/Middleware/RequireWordPressOrgLogin.php, freescout.wordpress.net/Modules/WPOrgSSO/tests/ProxyTest.php
When proxy enforcement is enabled, the middleware logs out administrators whose requests are not marked as proxied. It invalidates the session and returns a JSON 401 or redirects to login. Tests cover proxied and non-proxied requests, missing markers, header handling, agents, and disabled enforcement.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Feature

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant RequireWordPressOrgLogin
  participant WPOrgSSOServiceProvider
  participant Session
  Client->>RequireWordPressOrgLogin: Send administrator request
  RequireWordPressOrgLogin->>WPOrgSSOServiceProvider: Check proxy requirement
  WPOrgSSOServiceProvider-->>RequireWordPressOrgLogin: Return configuration and marker result
  RequireWordPressOrgLogin->>Session: Log out administrator and invalidate session
  RequireWordPressOrgLogin-->>Client: Return JSON 401 or login redirect
Loading

Merge Risk: ⚪ Minimal · up to 8a0d5

The proxy requirement appears mergeable after normal checks, provided nginx is configured before the setting is enabled.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 26be9

The application adds a fail-closed control for administrator web requests, including sessions created through emergency login. No introduced bypass was established. Successful deployment still depends on trustworthy proxy classification, and coverage of privileged endpoints outside the web middleware remains unconfirmed.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The affected authorization scope is administrator web access to this helpdesk. An incorrect infrastructure classification could admit requests carrying administrator authority or deny all administrators; agent access deliberately remains unchanged. A broader cross-service privilege expansion was not established.

Security Findings and Attack Paths

  • observed — The reviewed application path does not accept a same-named client header as the proxy marker, and rejects missing markers. Tests represent these cases and an unproxied existing administrator session. These controls counter a direct header-spoofing bypass but do not verify deployed nginx address classification.

Trust Boundaries and Controls

  • observed — nginx owns origin classification, while WPOrgSSO trusts the resulting FastCGI value and combines it with authenticated administrator identity. Enforcement is registered on the web group; the SAML ACS and metadata routes remain in an open, throttled group. Production forwarding trust and privileged non-web route coverage remain unresolved.

Resilience and Maintainability Implications

  • inferred — Repeated covered requests cannot rely solely on an earlier successful login: each authenticated administrator request rechecks proxy status. Rejecting one request terminates its dispatch and invalidates its session. This does not establish cancellation of already-running requests or enforcement for independently authenticated non-web endpoints.

Hardening Proposals

  • proposed — Before activation, validate the deployed trusted-address configuration with proxied, unproxied, spoofed-forwarding-header, IPv4 and IPv6 requests. Confirm that privileged core and private-module endpoints receive this policy or an equivalent control, and preserve an infrastructure-level recovery path for administrator lockout.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 16 functions across 4 files. (1 skipped: 1…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title identifies the main change: requiring administrator access through the proxy. It is concise and related to the pull request, although the wording is slightly awkward.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

obenland and others added 2 commits October 2, 2026 13:01
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants