feat: add validate_resource_server_ip feature flag to config and check to validate - #98
Merged
Merged
Conversation
…k to validate Signed-off-by: romanetar <roman_ag@hotmail.com>
romanetar
force-pushed
the
feature/toggle-ip-addr-whitelisting
branch
from
January 15, 2026 16:42
94cbcb6 to
53fced6
Compare
smarcet
force-pushed
the
main
branch
2 times, most recently
from
February 12, 2026 20:00
ae79f5e to
4b5b726
Compare
📝 WalkthroughWalkthroughA new configuration option Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Strategy as ValidateBearerTokenStrategy
participant Config as Configuration
participant IP as IP Validator
participant Audience as Audience Validator
Client->>Strategy: Bearer token request
Strategy->>Config: Check OAUTH2_VALIDATE_RESOURCE_SERVER_IP
alt Config Flag Enabled
Config-->>Strategy: true
Strategy->>IP: Validate resource server IP<br/>vs request IP
alt IP Valid
IP-->>Strategy: ✓ Match
Strategy->>Audience: Validate token audience
alt Audience Valid
Audience-->>Strategy: ✓ Authorized
Strategy-->>Client: Token accepted
else Audience Invalid
Audience-->>Strategy: ✗ Unauthorized audience
Strategy-->>Client: 401 Unauthorized
end
else IP Invalid
IP-->>Strategy: ✗ IP mismatch
Strategy-->>Client: 401 Unauthorized
end
else Config Flag Disabled
Config-->>Strategy: false
Strategy-->>Client: Token accepted<br/>(skip validation)
end
Estimated Code Review Effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment Tip CodeRabbit can use your project's `phpmd` ruleset to improve the quality of PHP code reviews.You can customize the |
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-98/ This page is automatically updated on each push to this PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.env.example:
- Line 118: The env example and the application default disagree: the env var
OAUTH2_VALIDATE_RESOURCE_SERVER_IP is set to true in .env.example while the app
default in config (OAUTH2_VALIDATE_RESOURCE_SERVER_IP defaulting to false) is
false; make them consistent by either changing the .env.example value to false
to match the current default or updating the config default to true so the
example reflects actual behavior—update the OAUTH2_VALIDATE_RESOURCE_SERVER_IP
entry accordingly and ensure any README or setup notes mirror that choice.
In `@config/oauth2.php`:
- Around line 1-15: The config key 'validate_resource_server_ip' currently
defaults to env('OAUTH2_VALIDATE_RESOURCE_SERVER_IP', false) which disables IP
validation by default; change the default to true so the expression becomes
env('OAUTH2_VALIDATE_RESOURCE_SERVER_IP', true) (preserving existing security
behavior) and update any inline comment if needed to reflect that operators must
explicitly opt out via the OAUTH2_VALIDATE_RESOURCE_SERVER_IP environment
variable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6aa5bcd1-01d1-41fb-b04c-898df3346761
📒 Files selected for processing (4)
.env.exampleapp/Models/OAuth2/ResourceServer.phpapp/libs/OAuth2/GrantTypes/Strategies/ValidateBearerTokenResourceServerStrategy.phpconfig/oauth2.php
| AUTH_PASSWORD_SHAPE_WARNING="Password must include at least one uppercase letter, one lowercase letter, one number, and one special character." | ||
|
|
||
|
|
||
| OAUTH2_VALIDATE_RESOURCE_SERVER_IP=true |
There was a problem hiding this comment.
Inconsistency between example value and config default.
The example sets OAUTH2_VALIDATE_RESOURCE_SERVER_IP=true, but config/oauth2.php defaults to false when the env var is not set. Developers copying this example will get validation enabled, but fresh deployments without explicit configuration will have it disabled.
Consider aligning these: either default to true in the config (preserving existing security behavior) or set the example to false to match the actual default.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.env.example at line 118, The env example and the application default
disagree: the env var OAUTH2_VALIDATE_RESOURCE_SERVER_IP is set to true in
.env.example while the app default in config (OAUTH2_VALIDATE_RESOURCE_SERVER_IP
defaulting to false) is false; make them consistent by either changing the
.env.example value to false to match the current default or updating the config
default to true so the example reflects actual behavior—update the
OAUTH2_VALIDATE_RESOURCE_SERVER_IP entry accordingly and ensure any README or
setup notes mirror that choice.
Comment on lines
+1
to
+15
| <?php | ||
|
|
||
| return [ | ||
|
|
||
| /* | ||
| |-------------------------------------------------------------------------- | ||
| | Validate Resource Server IP Address | ||
| |-------------------------------------------------------------------------- | ||
| | | ||
| | When enabled, validates that the resource server IP address matches | ||
| | the request IP and the access token audience. | ||
| | | ||
| */ | ||
| 'validate_resource_server_ip' => env('OAUTH2_VALIDATE_RESOURCE_SERVER_IP', false), | ||
| ]; |
There was a problem hiding this comment.
Consider defaulting to true to preserve existing security behavior.
This feature flag defaults to false, which disables resource server IP validation. If existing deployments previously relied on this validation (which was unconditional before this PR), upgrading without setting the env var will silently reduce their security posture.
A safer migration path would be to default to true (preserving current behavior) and let operators explicitly opt out if needed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@config/oauth2.php` around lines 1 - 15, The config key
'validate_resource_server_ip' currently defaults to
env('OAUTH2_VALIDATE_RESOURCE_SERVER_IP', false) which disables IP validation by
default; change the default to true so the expression becomes
env('OAUTH2_VALIDATE_RESOURCE_SERVER_IP', true) (preserving existing security
behavior) and update any inline comment if needed to reflect that operators must
explicitly opt out via the OAUTH2_VALIDATE_RESOURCE_SERVER_IP environment
variable.
smarcet
added a commit
that referenced
this pull request
Aug 11, 2026
…ure breadth Two changes to phpunit.xml: - The Application suite's <directory> scan only picks up *Test.php (PHPUnit's default suffix), so the four concrete *TestCase.php protocol suites (OAuth2Protocol, OIDCProtocol, OIDCPasswordless, OpenIdProtocol - 93 tests) were NEVER executed by CI. That is how OIDCProtocolTestCase stayed broken for two years with green builds. They are now listed explicitly. - stopOnFailure=false so a run reports every failure instead of dying on the first one. Also fixes the one test the newly-wired suites surfaced: testResourceServerIntrospectionNotValidIP expected an unconditional 400, but the resource-server IP check became opt-in in #98 (oauth2.validate_resource_server_ip, default off) - the test now enables the flag before asserting the rejection. Full-suite evidence (523 tests): green except 8 pre-existing environment-dependent Turnstile tests that need TEST_USER_EMAIL / TEST_USER_PASSWORD and the Turnstile secrets CI injects (they pass in CI; locally their markTestSkipped guard is defeated by a typed-property TypeError when the env vars are absent).
smarcet
added a commit
that referenced
this pull request
Aug 12, 2026
…OIDC circuit tests (#152) * fix(2fa): validate pending OAuth2 client before redeeming a recovery code verify2FARecovery() skipped the resolveClientFromMemento() guard that verify2FA() applies, so with a pending OAuth2 authorization request whose client no longer exists the single-use recovery code was burned and an IDP session established for an authorization request that could only fail at the /oauth2/auth hop. Apply the same guard before redemption; recovery-code checking itself stays client-agnostic. * refactor(2fa): extract MFA error_code literals into MFAConstants The error_code values emitted by UserController's MFA endpoints were hardcoded strings, duplicated in TwoFactorRateLimitMiddleware::FAILURE_CODES where a silent drift would break the rate-limit failure counting. Tests keep asserting the literal wire values on purpose, pinning the contract. * refactor(2fa): return a typed DTO from getPendingState() instead of an array MFAPendingState (getUserId / getPendingAt / shouldRemember) replaces the string-keyed array, so callers stop scattering 'user_id'/'remember' literals and casts, and the shape is enforced by the type system instead of by convention. * refactor(2fa): serialize recovery-codes standing via a RecoveryCodesStatus DTO verify2FARecovery() and getProfile() each hand-built the recovery_codes_remaining/total/low_threshold payload with their own config() reads and magic defaults. IRecoveryCodeService::getStatus() now returns a RecoveryCodesStatus DTO whose toArray() owns the wire keys, so both call sites merge the same serialized shape. Side effect: the recovery XHR response now also carries recovery_codes_total (additive, ignored by the SPA). * test(2fa): prove the full OIDC consent circuit for verify2FA and verify2FARecovery authorize -> login -> MFA challenge -> verify (OTP / recovery code) -> redirect_url back to the authorization endpoint (rebuilt from the session memento) -> consent screen -> AllowOnce -> authorization code delivered to the client redirect_uri. Locks in that the XHR verify contract composes with the interactive grant's memento round-trip. Note: OIDCProtocolTestCase's password-login circuits (e.g. testAuthCode) predate the MFA gate and post a wrong seed password - broken independently of this change. * test(oidc): repair OIDCProtocolTestCase login circuits (stale password + MFA gate) Two stacked breakages, both predating and unrelated to each individual test: - 021bee3 (jul 2024) changed the TestSeeder passwords from '1qaz2wsx' to '1Qaz2wsx!' without updating this class, so every password login leg has silently failed since - errorLogin() also answers 302, so the post-login assertion kept passing and tests died downstream instead. - The MFA gate now challenges the seeded login user (SuperAdminGroup is in two_factor.enforced_groups), so even a correct password stops at the 2FA challenge. This class exercises the OIDC protocol, not the gate - enforced groups are cleared in prepareForTests(); the gate plus the full authorize -> MFA -> consent -> code circuit live in TwoFactorLoginFlowTest. Result: 29 broken -> 3 (32/35 green). The 3 residuals have distinct pre-existing causes: testConsentLogin and testGetRefreshTokenWithPromptSetToConsentLogin lose the login hint because AuthService::logout()'s Session::flush() (4864f50 / #118) wipes the session-backed security context even when called with clear_security_ctx = false (prompt=login path); testTokenResponseModePost uses max_age=1 and the multi-request dance now takes longer than 1s, forcing a re-login. * fix(auth): honor clear_security_ctx=false across logout()'s session flush The Session::flush() hardening added in #118 wipes the whole session at the end of logout(), including the session-backed security context - even when the caller passed clear_security_ctx = false (the prompt=login re-authentication path in InteractiveGrantType::mustAuthenticateUser()), which broke the login-hint prefill on the login screen for prompt=login OIDC requests. Capture the context before the flush and re-save it after the session ID regenerate; everything else is still flushed, so the #118 hardening stands. * test(oidc): raise testTokenResponseModePost max_age from 1 to 3200 The test exercises response_mode=form_post, not max_age expiry (testMaxAge1AndWait2 owns that) - with max_age=1 the multi-request login+consent dance takes longer than 1s and the final authorize hop forced a re-login instead of delivering the form post. 3200 matches the sibling circuits. OIDCProtocolTestCase is now fully green: 35/35. * test(2fa): negative-path OIDC circuits for verify2FA and verify2FARecovery Six tests inside a pending OIDC authorization-code flow, three per endpoint: - wrong code then correct code: the rejection keeps the pending challenge and the OAuth2 memento alive, and the retry completes the full circuit (consent -> authorization code). - consecutive wrong codes up to the rate-limit threshold: every attempt is 401 without a session, and once the window closes even the CORRECT code answers 429 - brute-forcing inside a pending flow buys no extra attempts. - burned single-use code (used recovery code / redeemed OTP): rejected like any invalid code, and the flow still completes afterwards with a fresh code (new recovery code / resent OTP). * test(2fa): cover the error branches of verify2FA and verify2FARecovery - validator 412s (malformed request, no otp_value / recovery_code) - vanished pending user -> mfa_session_expired + pending state cleared - recovery without a pending challenge -> mfa_session_expired - stale OAuth2 client guard on verify2FA (parity with the recovery test): 412 before the OTP is redeemed - audit failure on the FAILED-verify path stays a clean 401 with the error_code the rate-limit middleware keys on, for both endpoints verify2FA line coverage 82.3% -> 95.2%, verify2FARecovery 82.7% -> 94.2%; the only uncovered lines left are the generic Exception -> 500 catches. * test(ci): actually run the protocol TestCase suites, stop hiding failure breadth Two changes to phpunit.xml: - The Application suite's <directory> scan only picks up *Test.php (PHPUnit's default suffix), so the four concrete *TestCase.php protocol suites (OAuth2Protocol, OIDCProtocol, OIDCPasswordless, OpenIdProtocol - 93 tests) were NEVER executed by CI. That is how OIDCProtocolTestCase stayed broken for two years with green builds. They are now listed explicitly. - stopOnFailure=false so a run reports every failure instead of dying on the first one. Also fixes the one test the newly-wired suites surfaced: testResourceServerIntrospectionNotValidIP expected an unconditional 400, but the resource-server IP check became opt-in in #98 (oauth2.validate_resource_server_ip, default off) - the test now enables the flag before asserting the rejection. Full-suite evidence (523 tests): green except 8 pre-existing environment-dependent Turnstile tests that need TEST_USER_EMAIL / TEST_USER_PASSWORD and the Turnstile secrets CI injects (they pass in CI; locally their markTestSkipped guard is defeated by a typed-property TypeError when the env vars are absent). * refactor(2fa): single home for every MFA string constant MFAConstants now owns all of them: - error codes: the existing three plus mfa_rate_limit and mfa_required. ITwoFactorRateLimitService::RATE_LIMIT_ERROR_CODE and ILoginStrategy::MFA_REQUIRED alias it, so consumers keep their names while the value is defined once. - 2fa_* session keys: previously defined TWICE in production (AbstractMFAChallengeStrategy's private consts and ITwoFactorRateLimitService::PENDING_USER_SESSION_KEY) - both now alias MFAConstants. Also promotes the rate-limit cache-key prefix ('2fa_rate:', previously a sprintf literal in TwoFactorRateLimitService duplicated by the test flush helper) to ITwoFactorRateLimitService::RATE_LIMIT_CACHE_KEY_PREFIX. All ~50 hardcoded literals across TwoFactorLoginFlowTest, AbstractMFAChallengeStrategyTest and EmailOTPMFAChallengeStrategyTest now reference the constants. * test(oidc): extract the seeded password into a SEED_PASSWORD constant The literal appeared at 26 call sites; a seed password change is now a one-line edit, matching TwoFactorLoginFlowTest. The trailing-space login test keeps its spacing explicit around the constant, since that spacing is the subject under test. Suite re-run in idp-app: 35/35, 506 assertions.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ref https://app.clickup.com/t/86b82z68f
Summary by CodeRabbit
New Features
Chores