fix(oagw): handle CORS preflight without tenant context#1423
Open
asmith987 wants to merge 1 commit intocyberfabric:mainfrom
Open
fix(oagw): handle CORS preflight without tenant context#1423asmith987 wants to merge 1 commit intocyberfabric:mainfrom
asmith987 wants to merge 1 commit intocyberfabric:mainfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughPreflight handling moved to the proxy handler: OPTIONS preflight returns a permissive 204 without upstream/tenant resolution. Actual requests enforce origin and method after upstream resolution and may return 403 before forwarding. CORS fields Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler
participant AuthMiddleware
participant ProxyService
participant Upstream
Client->>Handler: OPTIONS + Origin + Access-Control-Request-*
Handler-->>Client: 204 No Content
Note right of Handler: Access-Control-Allow-Origin: <origin>\nAccess-Control-Allow-Methods: <requested>\nAccess-Control-Allow-Headers: <requested>\nVary: Origin, Access-Control-Request-Method, Access-Control-Request-Headers
sequenceDiagram
participant Client
participant AuthMiddleware
participant ProxyService
participant Upstream
Client->>AuthMiddleware: Actual request (Origin + Authorization)
AuthMiddleware->>ProxyService: forward (SecurityContext may be present)
ProxyService->>ProxyService: resolve upstream (uses actual HTTP method)
ProxyService->>ProxyService: validate origin & method vs upstream CORS
alt origin/method not allowed
ProxyService-->>Client: 403 Forbidden (no upstream call)
else allowed
ProxyService->>Upstream: forward request
Upstream-->>ProxyService: response
ProxyService-->>Client: response (CORS headers injected)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
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)
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. Comment |
Review Summary by QodoFix CORS preflight handling without tenant context
WalkthroughsDescription• Preflight requests now return permissive 204 at handler level without upstream resolution • Removed allowed_headers and max_age fields from CORS configuration • Origin and method validation moved to actual requests after upstream resolution • Disallowed origins/methods now rejected with 403 before reaching upstream Diagramflowchart LR
A["Browser Preflight<br/>OPTIONS + Origin + ACRM"] -->|Handler detects| B["Return 204<br/>Echo origin/method/headers"]
C["Actual Request<br/>GET/POST + Origin"] -->|After upstream<br/>resolution| D["Validate origin<br/>& method"]
D -->|Allowed| E["Forward to<br/>upstream"]
D -->|Disallowed| F["403 Forbidden<br/>before upstream"]
E -->|Success| G["Add CORS headers<br/>to response"]
File Changes1. modules/system/api-gateway/src/middleware/auth.rs
|
Code Review by Qodo
1. Preflight bypasses disabled CORS
|
asmith987
force-pushed
the
fix/oagw-cors-preflight
branch
from
5cf93f0 to
bcb5195
Compare
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
testing/e2e/modules/file_parser/generate_file_parser_golden.py (1)
14-14:⚠️ Potential issue | 🟡 MinorDocstring is now outdated.
Line 14 states "Optional bearer token for authentication" but the token is no longer optional — a default is always used. Consider updating to:
E2E_AUTH_TOKEN: Bearer token for authentication (defaults to e2e-token-tenant-a).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testing/e2e/modules/file_parser/generate_file_parser_golden.py` at line 14, Update the outdated docstring for E2E_AUTH_TOKEN to reflect that it is no longer optional and that a default is used; change the description for the E2E_AUTH_TOKEN constant/variable in generate_file_parser_golden.py to something like "Bearer token for authentication (defaults to e2e-token-tenant-a)" so the docstring accurately describes its behavior and default value.testing/e2e/modules/oagw/test_cors.py (1)
8-14:⚠️ Potential issue | 🟠 MajorAdd credentialed-preflight test coverage before supporting
allow_credentials=Trueflows.Per CORS specifications, a preflight response must include
Access-Control-Allow-Credentials: truewhen the actual request will use credentials; browsers will reject the subsequent request if this header is absent. The current test suite only coversallow_credentials=Falsecases, and the handler atmodules/system/oagw/oagw/src/api/rest/handlers/proxy.rs:30-68does not emit this header. If credentialed flows are later needed, this gap will cause failures and won't be caught by existing tests. Add a credentialed-preflight test case or document that credentialed requests are not supported.
🧹 Nitpick comments (4)
modules/system/oagw/oagw/src/domain/cors.rs (1)
131-134: Add direct unit tests for the newly public CORS helper APIs.These helpers are now externally consumable; adding dedicated tests for case-insensitive methods, unknown methods, exact/wildcard origins will prevent regressions.
Also applies to: 191-196
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/system/oagw/oagw/src/domain/cors.rs` around lines 131 - 134, Add focused unit tests for the newly public CORS helpers (e.g., is_method_allowed and the origin helper referenced around the later block) using CorsConfig to prevent regressions: create tests that assert case-insensitive method matching (e.g., "GET" vs "get"), that unknown/invalid methods (where CorsHttpMethod::from_str_loose returns None) yield false, and that origin checks verify exact matches and wildcard patterns; place these in a tests module near cors.rs (or an integration test) and exercise CorsConfig.allowed_methods and allowed_origins to cover positive and negative cases.testing/e2e/conftest.py (1)
18-29: Reintroduce an explicit no-auth fixture path to keep unauth test scenarios possible.Always returning a Bearer header makes
not auth_headersskip patterns unreachable and removes a clean way to exercise unauthenticated flows via shared fixtures.♻️ Suggested tweak
`@pytest.fixture` def auth_headers(): """ Build Authorization headers using E2E_AUTH_TOKEN env var. - - Falls back to a dummy token suitable for the static-authn-plugin - running in ``accept_all`` mode (any non-empty Bearer token is accepted). + Set E2E_DISABLE_AUTH_HEADERS=1 to run unauthenticated scenarios. + Otherwise falls back to local static token if E2E_AUTH_TOKEN is unset. Returns: - dict: Headers dict with Authorization header. + dict: Headers dict (possibly empty for unauthenticated scenarios). """ + if os.getenv("E2E_DISABLE_AUTH_HEADERS") == "1": + return {} token = os.getenv("E2E_AUTH_TOKEN", "e2e-token-tenant-a") return {"Authorization": f"Bearer {token}"}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testing/e2e/conftest.py` around lines 18 - 29, The test suite needs an explicit unauthenticated fixture so tests can exercise no-auth flows; add a new fixture/function named no_auth_headers (in testing/e2e/conftest.py alongside auth_headers) that returns an empty headers mapping (e.g. {} or None) to represent absence of Authorization rather than relying on env tricks, and keep auth_headers unchanged so existing authenticated flows keep working; update any tests that expect a no-auth path to use no_auth_headers.modules/system/oagw/oagw/src/api/rest/handlers/proxy.rs (1)
38-42: Consider the"*"fallback when Origin header is non-UTF8.If
Originexists but contains invalid UTF-8 bytes, the code falls back to"*". While malformed headers are rare, this could inadvertently echo a wildcard origin to the client.A stricter approach would return 400 Bad Request for unparseable Origin headers:
🛡️ Alternative: reject malformed Origin
- let origin = req - .headers() - .get(http::header::ORIGIN) - .and_then(|v| v.to_str().ok()) - .unwrap_or("*"); + let origin = match req.headers().get(http::header::ORIGIN) { + Some(v) => v.to_str().map_err(|_| { + error_response(DomainError::Validation { + detail: "invalid Origin header encoding".into(), + instance: req.uri().path().to_string(), + }) + })?, + None => "*", + };That said, since preflight is explicitly permissive and actual requests still validate origin against the upstream's
allowed_origins, this is defense-in-depth rather than a correctness issue.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/system/oagw/oagw/src/api/rest/handlers/proxy.rs` around lines 38 - 42, The current Origin header handling falls back to "*" when to_str() fails; change it to treat a present-but-unparseable Origin as a client error by returning a 400 Bad Request instead of using the wildcard. In the proxy request handler (where origin is computed from req.headers().get(http::header::ORIGIN)), detect if the header exists but to_str().ok() is None and return an immediate 400 response (with a short error message) rather than setting origin = "*"; keep the existing behavior of using "*" only when the header is completely absent (get(...) returns None).modules/system/oagw/oagw/tests/proxy_integration.rs (1)
3655-3679: Match the specific CORS rejection instead of any error.
response.is_err()will also pass on unrelated pre-upstream failures, so these assertions don’t really prove the new “403 due to CORS” contract. Matching the concrete CORS error/status here would make the regression coverage much tighter.Also applies to: 3681-3748, 3796-3811
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/system/oagw/oagw/tests/proxy_integration.rs` around lines 3655 - 3679, The test currently asserts response.is_err() which is too broad; instead inspect the Result from AppHarness::facade().proxy_request(...) and assert the concrete CORS rejection (HTTP 403 due to CORS) by either pattern-matching the error variant returned (e.g., ProxyError::RequestRejected/ProxyError::CorsRejected) and asserting the status == 403, or if proxy_request returns an Ok<Response>, assert response.status() == StatusCode::FORBIDDEN and body/reason indicates CORS; update cors_actual_request_disallowed_origin_rejected_before_upstream to check that exact condition (and make the analogous changes in the other tests mentioned: the blocks covering lines 3681-3748 and 3796-3811).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@testing/e2e/modules/file_parser/generate_file_parser_golden.py`:
- Line 14: Update the outdated docstring for E2E_AUTH_TOKEN to reflect that it
is no longer optional and that a default is used; change the description for the
E2E_AUTH_TOKEN constant/variable in generate_file_parser_golden.py to something
like "Bearer token for authentication (defaults to e2e-token-tenant-a)" so the
docstring accurately describes its behavior and default value.
---
Nitpick comments:
In `@modules/system/oagw/oagw/src/api/rest/handlers/proxy.rs`:
- Around line 38-42: The current Origin header handling falls back to "*" when
to_str() fails; change it to treat a present-but-unparseable Origin as a client
error by returning a 400 Bad Request instead of using the wildcard. In the proxy
request handler (where origin is computed from
req.headers().get(http::header::ORIGIN)), detect if the header exists but
to_str().ok() is None and return an immediate 400 response (with a short error
message) rather than setting origin = "*"; keep the existing behavior of using
"*" only when the header is completely absent (get(...) returns None).
In `@modules/system/oagw/oagw/src/domain/cors.rs`:
- Around line 131-134: Add focused unit tests for the newly public CORS helpers
(e.g., is_method_allowed and the origin helper referenced around the later
block) using CorsConfig to prevent regressions: create tests that assert
case-insensitive method matching (e.g., "GET" vs "get"), that unknown/invalid
methods (where CorsHttpMethod::from_str_loose returns None) yield false, and
that origin checks verify exact matches and wildcard patterns; place these in a
tests module near cors.rs (or an integration test) and exercise
CorsConfig.allowed_methods and allowed_origins to cover positive and negative
cases.
In `@modules/system/oagw/oagw/tests/proxy_integration.rs`:
- Around line 3655-3679: The test currently asserts response.is_err() which is
too broad; instead inspect the Result from
AppHarness::facade().proxy_request(...) and assert the concrete CORS rejection
(HTTP 403 due to CORS) by either pattern-matching the error variant returned
(e.g., ProxyError::RequestRejected/ProxyError::CorsRejected) and asserting the
status == 403, or if proxy_request returns an Ok<Response>, assert
response.status() == StatusCode::FORBIDDEN and body/reason indicates CORS;
update cors_actual_request_disallowed_origin_rejected_before_upstream to check
that exact condition (and make the analogous changes in the other tests
mentioned: the blocks covering lines 3681-3748 and 3796-3811).
In `@testing/e2e/conftest.py`:
- Around line 18-29: The test suite needs an explicit unauthenticated fixture so
tests can exercise no-auth flows; add a new fixture/function named
no_auth_headers (in testing/e2e/conftest.py alongside auth_headers) that returns
an empty headers mapping (e.g. {} or None) to represent absence of Authorization
rather than relying on env tricks, and keep auth_headers unchanged so existing
authenticated flows keep working; update any tests that expect a no-auth path to
use no_auth_headers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d81768b9-e1fc-46d9-8d89-6c7bdcf70133
📒 Files selected for processing (31)
config/e2e-local.yamlmodules/system/api-gateway/src/middleware/auth.rsmodules/system/oagw/docs/ADR/0006-cors.mdmodules/system/oagw/docs/DESIGN.mdmodules/system/oagw/docs/adr-cors.mdmodules/system/oagw/docs/features/0004-cpt-cf-oagw-feature-proxy-engine.mdmodules/system/oagw/docs/features/0008-cpt-cf-oagw-feature-observability.mdmodules/system/oagw/docs/schemas/route.v1.schema.jsonmodules/system/oagw/docs/schemas/upstream.v1.schema.jsonmodules/system/oagw/oagw-sdk/src/models.rsmodules/system/oagw/oagw/src/api/rest/dto.rsmodules/system/oagw/oagw/src/api/rest/error.rsmodules/system/oagw/oagw/src/api/rest/handlers/proxy.rsmodules/system/oagw/oagw/src/domain/cors.rsmodules/system/oagw/oagw/src/domain/error.rsmodules/system/oagw/oagw/src/domain/model.rsmodules/system/oagw/oagw/src/domain/services/client.rsmodules/system/oagw/oagw/src/domain/services/management.rsmodules/system/oagw/oagw/src/infra/proxy/service.rsmodules/system/oagw/oagw/src/infra/type_provisioning.rsmodules/system/oagw/oagw/tests/proxy_integration.rsmodules/system/oagw/scenarios/INDEX.mdmodules/system/oagw/scenarios/flows/plugin-execution.mdmodules/system/oagw/scenarios/plugins/guards/negative-10.3-cors-credentials-wildcard-rejected-config-validation.mdmodules/system/oagw/scenarios/plugins/guards/positive-10.2-built-cors-handling.mdtesting/e2e/conftest.pytesting/e2e/modules/file_parser/generate_file_parser_golden.pytesting/e2e/modules/oagw/conftest.pytesting/e2e/modules/oagw/test_authz.pytesting/e2e/modules/oagw/test_body_validation.pytesting/e2e/modules/oagw/test_cors.py
💤 Files with no reviewable changes (7)
- modules/system/oagw/oagw/src/infra/type_provisioning.rs
- modules/system/oagw/oagw/src/domain/model.rs
- modules/system/oagw/oagw/src/domain/services/client.rs
- modules/system/oagw/oagw/src/api/rest/dto.rs
- modules/system/oagw/docs/schemas/upstream.v1.schema.json
- modules/system/oagw/oagw-sdk/src/models.rs
- modules/system/oagw/oagw/src/domain/services/management.rs
asmith987
force-pushed
the
fix/oagw-cors-preflight
branch
from
bcb5195 to
1b3aeb0
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/api/api.json (1)
6588-6618:⚠️ Potential issue | 🟠 MajorCORS schema and runtime config are out of sync.
components.schemas.CorsConfigno longer exposes header/max-age fields, but runtime still defines and usesallowed_headersandmax_age_seconds(modules/system/api-gateway/src/config.rsLine 88-124 andmodules/system/api-gateway/src/cors.rsLine 37-56). This creates API contract drift for generated clients and docs.Please either (a) restore these fields in the OpenAPI schema (optionally deprecated), or (b) remove them from runtime in the same change set.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/api/api.json` around lines 6588 - 6618, The OpenAPI CorsConfig schema is missing allowed_headers and max_age_seconds while runtime (modules/system/api-gateway/src/config.rs and modules/system/api-gateway/src/cors.rs) still defines and uses allowed_headers and max_age_seconds, causing a contract drift; fix by either (A) restoring allowed_headers (array[string]) and max_age_seconds (integer) into components.schemas.CorsConfig in docs/api/api.json (mark them deprecated if desired) so the schema matches runtime symbols, or (B) remove the runtime fields/usages named allowed_headers and max_age_seconds from config.rs and cors.rs (and adjust any callers) so the runtime matches the current schema—pick one approach and update both schema and runtime consistently.
🧹 Nitpick comments (3)
docs/api/api.json (1)
8283-8285: Define a concrete schema forProblem.contextto stabilize codegen.
contextcurrently has only a description, so schema generators typically emit an untypedany. If intent is structured context, declare it explicitly.Proposed schema tightening
"context": { + "type": "object", + "additionalProperties": true, "description": "Optional structured context (e.g. `resource_type`, `resource_name`)." },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/api/api.json` around lines 8283 - 8285, The Problem.context entry is only a description and causes codegen to emit untyped any; replace the freeform description with an explicit JSON Schema for Problem.context (an object schema named "Problem.context") that lists expected keys (e.g., "resource_type": { "type":"string" }, "resource_name": { "type":"string" }, any other known fields), sets appropriate "required" flags if needed, and sets "additionalProperties": false (or true if truly extensible) to stabilize code generation and typing for the Problem.context field.modules/system/oagw/oagw/src/domain/cors.rs (1)
131-134: Add unit coverage for the new method gate.
is_method_allowed()is now part of the moved actual-request CORS check, but this module never exercises it directly. A tiny table test for allowed, lowercase, disallowed, and unknown methods would keep this behavior pinned down.🧪 Example coverage
+ #[test] + fn test_is_method_allowed() { + let config = make_config(); + assert!(is_method_allowed(&config, "GET")); + assert!(is_method_allowed(&config, "post")); + assert!(!is_method_allowed(&config, "DELETE")); + assert!(!is_method_allowed(&config, "PROPFIND")); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/system/oagw/oagw/src/domain/cors.rs` around lines 131 - 134, Add unit tests that exercise the new is_method_allowed() gate to pin its behavior: create a small table-driven test in the cors module's test suite that constructs a CorsConfig with a known allowed_methods set and then asserts is_method_allowed(&config, method) for cases including an allowed method (e.g., exactly matched), the same method in lowercase, a disallowed method, and an unknown/invalid method string; use CorsHttpMethod::... variants or strings to drive inputs and assert true/false as appropriate so the behavior for allowed, lowercase, disallowed, and unknown methods is covered.testing/e2e/modules/oagw/test_cors.py (1)
62-90: Please add an actual-request method rejection test.This proves preflight is intentionally permissive, but it never follows up with an actual
DELETEand asserts the403moved from preflight into the real request path. Without that, a broken method-enforcement path can still slip through this suite.🧪 Minimal follow-up test shape
+@pytest.mark.asyncio +async def test_cors_actual_request_disallowed_method_rejected( + oagw_base_url, oagw_headers, mock_upstream_url, mock_upstream, +): + _ = mock_upstream + alias = unique_alias("cors-actual-method-bad") + async with httpx.AsyncClient(timeout=10.0) as client: + upstream = await create_upstream( + client, oagw_base_url, oagw_headers, mock_upstream_url, + alias=alias, cors=CORS_CONFIG, + ) + uid = upstream["id"] + await create_route( + client, oagw_base_url, oagw_headers, uid, ["DELETE"], "/echo", + ) + + resp = await client.delete( + f"{oagw_base_url}/oagw/v1/proxy/{alias}/echo", + headers={**oagw_headers, "origin": "https://app.example.com"}, + ) + assert resp.status_code == 403 + + await delete_upstream(client, oagw_base_url, oagw_headers, uid)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testing/e2e/modules/oagw/test_cors.py` around lines 62 - 90, Add a follow-up actual-request assertion to test_cors_preflight_permissive_echoes_any_method: after the existing OPTIONS preflight, send a real DELETE to f"{oagw_base_url}/oagw/v1/proxy/{alias}/echo" using the same oagw_headers plus "origin": "https://app.example.com" and assert the response.status_code == 403 (and optionally that the body or headers indicate method not allowed); this validates that method enforcement occurs on the real request path even when preflight is permissive. Use the same alias created via create_upstream and the route created with create_route so the DELETE targets the configured proxy route.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config/e2e-local.yaml`:
- Around line 231-235: The token entry named "e2e-token-tenant-b" authenticates
as tenant ID "bbbbbbbb-bbbb-bbbb-bbbb-bbbbbbbbbbbb" which is not present in the
static tenant registry; either add a corresponding tenant object with that
tenant_id under modules.static-tr-plugin.config.tenants in this YAML, or change
the token's subject_tenant_id to one of the tenant IDs already defined in
modules.static-tr-plugin.config.tenants so the token references an existing
configured tenant.
---
Outside diff comments:
In `@docs/api/api.json`:
- Around line 6588-6618: The OpenAPI CorsConfig schema is missing
allowed_headers and max_age_seconds while runtime
(modules/system/api-gateway/src/config.rs and
modules/system/api-gateway/src/cors.rs) still defines and uses allowed_headers
and max_age_seconds, causing a contract drift; fix by either (A) restoring
allowed_headers (array[string]) and max_age_seconds (integer) into
components.schemas.CorsConfig in docs/api/api.json (mark them deprecated if
desired) so the schema matches runtime symbols, or (B) remove the runtime
fields/usages named allowed_headers and max_age_seconds from config.rs and
cors.rs (and adjust any callers) so the runtime matches the current schema—pick
one approach and update both schema and runtime consistently.
---
Nitpick comments:
In `@docs/api/api.json`:
- Around line 8283-8285: The Problem.context entry is only a description and
causes codegen to emit untyped any; replace the freeform description with an
explicit JSON Schema for Problem.context (an object schema named
"Problem.context") that lists expected keys (e.g., "resource_type": {
"type":"string" }, "resource_name": { "type":"string" }, any other known
fields), sets appropriate "required" flags if needed, and sets
"additionalProperties": false (or true if truly extensible) to stabilize code
generation and typing for the Problem.context field.
In `@modules/system/oagw/oagw/src/domain/cors.rs`:
- Around line 131-134: Add unit tests that exercise the new is_method_allowed()
gate to pin its behavior: create a small table-driven test in the cors module's
test suite that constructs a CorsConfig with a known allowed_methods set and
then asserts is_method_allowed(&config, method) for cases including an allowed
method (e.g., exactly matched), the same method in lowercase, a disallowed
method, and an unknown/invalid method string; use CorsHttpMethod::... variants
or strings to drive inputs and assert true/false as appropriate so the behavior
for allowed, lowercase, disallowed, and unknown methods is covered.
In `@testing/e2e/modules/oagw/test_cors.py`:
- Around line 62-90: Add a follow-up actual-request assertion to
test_cors_preflight_permissive_echoes_any_method: after the existing OPTIONS
preflight, send a real DELETE to f"{oagw_base_url}/oagw/v1/proxy/{alias}/echo"
using the same oagw_headers plus "origin": "https://app.example.com" and assert
the response.status_code == 403 (and optionally that the body or headers
indicate method not allowed); this validates that method enforcement occurs on
the real request path even when preflight is permissive. Use the same alias
created via create_upstream and the route created with create_route so the
DELETE targets the configured proxy route.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1a1aa96e-c93c-4b95-8165-efde624c1dda
📒 Files selected for processing (32)
config/e2e-local.yamldocs/api/api.jsonmodules/system/api-gateway/src/middleware/auth.rsmodules/system/oagw/docs/ADR/0006-cors.mdmodules/system/oagw/docs/DESIGN.mdmodules/system/oagw/docs/adr-cors.mdmodules/system/oagw/docs/features/0004-cpt-cf-oagw-feature-proxy-engine.mdmodules/system/oagw/docs/features/0008-cpt-cf-oagw-feature-observability.mdmodules/system/oagw/docs/schemas/route.v1.schema.jsonmodules/system/oagw/docs/schemas/upstream.v1.schema.jsonmodules/system/oagw/oagw-sdk/src/models.rsmodules/system/oagw/oagw/src/api/rest/dto.rsmodules/system/oagw/oagw/src/api/rest/error.rsmodules/system/oagw/oagw/src/api/rest/handlers/proxy.rsmodules/system/oagw/oagw/src/domain/cors.rsmodules/system/oagw/oagw/src/domain/error.rsmodules/system/oagw/oagw/src/domain/model.rsmodules/system/oagw/oagw/src/domain/services/client.rsmodules/system/oagw/oagw/src/domain/services/management.rsmodules/system/oagw/oagw/src/infra/proxy/service.rsmodules/system/oagw/oagw/src/infra/type_provisioning.rsmodules/system/oagw/oagw/tests/proxy_integration.rsmodules/system/oagw/scenarios/INDEX.mdmodules/system/oagw/scenarios/flows/plugin-execution.mdmodules/system/oagw/scenarios/plugins/guards/negative-10.3-cors-credentials-wildcard-rejected-config-validation.mdmodules/system/oagw/scenarios/plugins/guards/positive-10.2-built-cors-handling.mdtesting/e2e/conftest.pytesting/e2e/modules/file_parser/generate_file_parser_golden.pytesting/e2e/modules/oagw/conftest.pytesting/e2e/modules/oagw/test_authz.pytesting/e2e/modules/oagw/test_body_validation.pytesting/e2e/modules/oagw/test_cors.py
💤 Files with no reviewable changes (7)
- modules/system/oagw/oagw/src/infra/type_provisioning.rs
- modules/system/oagw/oagw/src/domain/services/client.rs
- modules/system/oagw/oagw/src/domain/model.rs
- modules/system/oagw/oagw-sdk/src/models.rs
- modules/system/oagw/docs/schemas/upstream.v1.schema.json
- modules/system/oagw/oagw/src/domain/services/management.rs
- modules/system/oagw/oagw/src/api/rest/dto.rs
✅ Files skipped from review due to trivial changes (8)
- modules/system/api-gateway/src/middleware/auth.rs
- modules/system/oagw/docs/features/0004-cpt-cf-oagw-feature-proxy-engine.md
- modules/system/oagw/scenarios/plugins/guards/negative-10.3-cors-credentials-wildcard-rejected-config-validation.md
- modules/system/oagw/docs/features/0008-cpt-cf-oagw-feature-observability.md
- modules/system/oagw/docs/ADR/0006-cors.md
- modules/system/oagw/docs/DESIGN.md
- testing/e2e/modules/oagw/conftest.py
- modules/system/oagw/oagw/src/domain/error.rs
🚧 Files skipped from review as they are similar to previous changes (11)
- testing/e2e/conftest.py
- modules/system/oagw/docs/schemas/route.v1.schema.json
- testing/e2e/modules/oagw/test_authz.py
- modules/system/oagw/oagw/src/api/rest/error.rs
- modules/system/oagw/oagw/src/api/rest/handlers/proxy.rs
- modules/system/oagw/scenarios/plugins/guards/positive-10.2-built-cors-handling.md
- modules/system/oagw/scenarios/flows/plugin-execution.md
- modules/system/oagw/scenarios/INDEX.md
- modules/system/oagw/oagw/src/infra/proxy/service.rs
- modules/system/oagw/docs/adr-cors.md
- modules/system/oagw/oagw/tests/proxy_integration.rs
asmith987
force-pushed
the
fix/oagw-cors-preflight
branch
from
1b3aeb0 to
732c5b6
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/api/api.json (1)
8272-8277:⚠️ Potential issue | 🟠 Major
Problem.instancecontract was weakened and now conflicts with existing consumers.Line 8272 removes
instancefrom required, but current consumers/tests still treat it as present (e.g., proxy integration assertions and SDK error shapes). This creates an API contract drift risk.🔧 Proposed fix
"required": [ "type", "title", "status", - "detail" + "detail", + "instance" ],Also applies to: 8300-8303
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/api/api.json` around lines 8272 - 8277, The Problem schema's required array was weakened by removing "instance", breaking consumers/tests; restore "instance" to the required arrays for the Problem schema entries (the one that currently lists ["type","title","status","detail"] and the second occurrence around the other diff block) so that "Problem.instance" is mandatory again; update both schema definitions referenced (the two required arrays) to include "instance" alongside "type", "title", "status", and "detail" to re-establish the contract.
🧹 Nitpick comments (3)
modules/system/oagw/docs/features/0008-cpt-cf-oagw-feature-observability.md (1)
222-232: Optional: Consider addressing list indentation warnings.Static analysis flagged MD005 (list-indent) warnings on these lines, indicating inconsistent indentation in the numbered list. While markdownlint enforcement is disabled in CI (non-blocking), addressing these could improve readability. Based on learnings, markdownlint issues in docs are non-blocking unless explicitly opted in via CI config.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/system/oagw/docs/features/0008-cpt-cf-oagw-feature-observability.md` around lines 222 - 232, Fix the inconsistent list indentation in the numbered CORS checklist so nested items align uniformly: indent each sub-item (e.g., inst-cors-h-4a and inst-cors-h-4b under inst-cors-h-4, inst-cors-h-6a under inst-cors-h-6, and inst-cors-h-8a/inst-cors-h-8b under inst-cors-h-8) using a consistent indentation level (e.g., 4 spaces per nesting level) and consistent list markers so markdownlint MD005 warnings are resolved and the hierarchy for inst-cors-h-3, inst-cors-h-4, inst-cors-h-5, inst-cors-h-6, inst-cors-h-7, and inst-cors-h-8 is visually and syntactically consistent.docs/api/api.json (1)
8283-8285:Problem.contextshould declare an explicit schema type.Line 8283 adds
contextwith only a description, which makes it unconstrained. If this is intended to be structured context, declare it as an object explicitly.♻️ Suggested schema tightening
"context": { + "type": "object", "description": "Optional structured context (e.g. `resource_type`, `resource_name`)." + ,"additionalProperties": true },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/api/api.json` around lines 8283 - 8285, The Problem.context entry is missing an explicit schema type; update the Problem.context definition in docs/api/api.json to declare it as an object (e.g., add "type": "object") and either define expected properties or set "additionalProperties": true if free-form key/value pairs are allowed so the schema is constrained and validated.modules/system/oagw/oagw/tests/proxy_integration.rs (1)
3655-3679: Cover the new handler path and assert the exact CORS failures.These cases still call
facade().proxy_request(...), so they never hit the 204 preflight short-circuit inmodules/system/oagw/oagw/src/api/rest/handlers/proxy.rs:25-68, which is the main change in this PR. They also only assertis_err(). Please add oneOPTIONStest throughstart_oagw_server()and matchServiceGatewayError::CorsOriginNotAllowed/CorsMethodNotAllowedhere so the contract stays explicit.One way to tighten the assertion pattern
- assert!( - response.is_err(), - "disallowed origin on actual request should be rejected before reaching upstream" - ); + assert!(matches!( + response, + Err(oagw_sdk::error::ServiceGatewayError::CorsOriginNotAllowed { .. }) + ));Also applies to: 3681-3748, 3796-3811
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/system/oagw/oagw/tests/proxy_integration.rs` around lines 3655 - 3679, Update the test to cover the new handler path by adding an OPTIONS request that goes through start_oagw_server instead of facade().proxy_request and assert the specific CORS error variants; specifically, in the test cors_actual_request_disallowed_origin_rejected_before_upstream (and the other affected tests), replace or add an OPTIONS request sent via start_oagw_server (so it hits the 204 preflight handler path) and match the returned error to ServiceGatewayError::CorsOriginNotAllowed or ServiceGatewayError::CorsMethodNotAllowed (as appropriate) rather than only asserting response.is_err(), ensuring the test explicitly checks the CORS failure contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@modules/system/oagw/docs/features/0008-cpt-cf-oagw-feature-observability.md`:
- Around line 218-233: The doc currently mentions adding
Access-Control-Expose-Headers "if configured" but the configuration section only
lists allowed_origins and allowed_methods; decide which of three options to
implement and update the spec accordingly: (1) If expose_headers is supported,
add a clear config field named expose_headers to the configuration requirements
(alongside allowed_origins and allowed_methods) and document its format and
behavior; (2) If expose_headers was removed, delete the “Add
Access-Control-Expose-Headers if configured” note (inst-cors-h-8a) so the
behavior is consistent; or (3) If it is an optional/advanced feature, explicitly
mark expose_headers as optional in the configuration requirements (line ~364)
and reference it from inst-cors-h-8a as optional, including its expected type
and semantics. Ensure references to Access-Control-Expose-Headers and the config
field name expose_headers are consistent across the CORS flow section and the
configuration requirements.
In `@modules/system/oagw/oagw/src/infra/proxy/service.rs`:
- Around line 420-436: The current CORS check runs whenever an Origin header
exists, causing same-origin requests to be treated as cross-origin; change the
guard in the block that uses effective_cors, request_origin and instance_uri so
it only performs CORS validation when the request is actually cross-origin
(i.e., the request Origin does not match the gateway/instance origin), not
merely when the Origin header is present. Concretely, before calling
crate::domain::cors::is_origin_allowed and is_method_allowed, compare the parsed
request_origin to the instance_uri (or use/implement a small helper like
is_same_origin(origin, instance_uri) that compares scheme/host/port) and skip
the origin/method checks when they are same-origin; keep the existing error
returns (DomainError::CorsOriginNotAllowed / CorsMethodNotAllowed) for true
cross-origin failures.
---
Outside diff comments:
In `@docs/api/api.json`:
- Around line 8272-8277: The Problem schema's required array was weakened by
removing "instance", breaking consumers/tests; restore "instance" to the
required arrays for the Problem schema entries (the one that currently lists
["type","title","status","detail"] and the second occurrence around the other
diff block) so that "Problem.instance" is mandatory again; update both schema
definitions referenced (the two required arrays) to include "instance" alongside
"type", "title", "status", and "detail" to re-establish the contract.
---
Nitpick comments:
In `@docs/api/api.json`:
- Around line 8283-8285: The Problem.context entry is missing an explicit schema
type; update the Problem.context definition in docs/api/api.json to declare it
as an object (e.g., add "type": "object") and either define expected properties
or set "additionalProperties": true if free-form key/value pairs are allowed so
the schema is constrained and validated.
In `@modules/system/oagw/docs/features/0008-cpt-cf-oagw-feature-observability.md`:
- Around line 222-232: Fix the inconsistent list indentation in the numbered
CORS checklist so nested items align uniformly: indent each sub-item (e.g.,
inst-cors-h-4a and inst-cors-h-4b under inst-cors-h-4, inst-cors-h-6a under
inst-cors-h-6, and inst-cors-h-8a/inst-cors-h-8b under inst-cors-h-8) using a
consistent indentation level (e.g., 4 spaces per nesting level) and consistent
list markers so markdownlint MD005 warnings are resolved and the hierarchy for
inst-cors-h-3, inst-cors-h-4, inst-cors-h-5, inst-cors-h-6, inst-cors-h-7, and
inst-cors-h-8 is visually and syntactically consistent.
In `@modules/system/oagw/oagw/tests/proxy_integration.rs`:
- Around line 3655-3679: Update the test to cover the new handler path by adding
an OPTIONS request that goes through start_oagw_server instead of
facade().proxy_request and assert the specific CORS error variants;
specifically, in the test
cors_actual_request_disallowed_origin_rejected_before_upstream (and the other
affected tests), replace or add an OPTIONS request sent via start_oagw_server
(so it hits the 204 preflight handler path) and match the returned error to
ServiceGatewayError::CorsOriginNotAllowed or
ServiceGatewayError::CorsMethodNotAllowed (as appropriate) rather than only
asserting response.is_err(), ensuring the test explicitly checks the CORS
failure contract.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 26f51445-bd95-4cca-b2a5-b5bedf451ea1
📒 Files selected for processing (32)
config/e2e-local.yamldocs/api/api.jsonmodules/system/api-gateway/src/middleware/auth.rsmodules/system/oagw/docs/ADR/0006-cors.mdmodules/system/oagw/docs/DESIGN.mdmodules/system/oagw/docs/adr-cors.mdmodules/system/oagw/docs/features/0004-cpt-cf-oagw-feature-proxy-engine.mdmodules/system/oagw/docs/features/0008-cpt-cf-oagw-feature-observability.mdmodules/system/oagw/docs/schemas/route.v1.schema.jsonmodules/system/oagw/docs/schemas/upstream.v1.schema.jsonmodules/system/oagw/oagw-sdk/src/models.rsmodules/system/oagw/oagw/src/api/rest/dto.rsmodules/system/oagw/oagw/src/api/rest/error.rsmodules/system/oagw/oagw/src/api/rest/handlers/proxy.rsmodules/system/oagw/oagw/src/domain/cors.rsmodules/system/oagw/oagw/src/domain/error.rsmodules/system/oagw/oagw/src/domain/model.rsmodules/system/oagw/oagw/src/domain/services/client.rsmodules/system/oagw/oagw/src/domain/services/management.rsmodules/system/oagw/oagw/src/infra/proxy/service.rsmodules/system/oagw/oagw/src/infra/type_provisioning.rsmodules/system/oagw/oagw/tests/proxy_integration.rsmodules/system/oagw/scenarios/INDEX.mdmodules/system/oagw/scenarios/flows/plugin-execution.mdmodules/system/oagw/scenarios/plugins/guards/negative-10.3-cors-credentials-wildcard-rejected-config-validation.mdmodules/system/oagw/scenarios/plugins/guards/positive-10.2-built-cors-handling.mdtesting/e2e/conftest.pytesting/e2e/modules/file_parser/generate_file_parser_golden.pytesting/e2e/modules/oagw/conftest.pytesting/e2e/modules/oagw/test_authz.pytesting/e2e/modules/oagw/test_body_validation.pytesting/e2e/modules/oagw/test_cors.py
💤 Files with no reviewable changes (7)
- modules/system/oagw/oagw/src/infra/type_provisioning.rs
- modules/system/oagw/oagw/src/domain/services/client.rs
- modules/system/oagw/oagw-sdk/src/models.rs
- modules/system/oagw/oagw/src/domain/model.rs
- modules/system/oagw/oagw/src/api/rest/dto.rs
- modules/system/oagw/docs/schemas/upstream.v1.schema.json
- modules/system/oagw/oagw/src/domain/services/management.rs
✅ Files skipped from review due to trivial changes (3)
- modules/system/oagw/docs/features/0004-cpt-cf-oagw-feature-proxy-engine.md
- modules/system/oagw/scenarios/flows/plugin-execution.md
- modules/system/oagw/docs/ADR/0006-cors.md
🚧 Files skipped from review as they are similar to previous changes (14)
- modules/system/oagw/scenarios/plugins/guards/negative-10.3-cors-credentials-wildcard-rejected-config-validation.md
- modules/system/oagw/oagw/src/domain/error.rs
- testing/e2e/modules/oagw/test_body_validation.py
- modules/system/api-gateway/src/middleware/auth.rs
- modules/system/oagw/docs/DESIGN.md
- modules/system/oagw/docs/schemas/route.v1.schema.json
- modules/system/oagw/oagw/src/api/rest/handlers/proxy.rs
- testing/e2e/conftest.py
- testing/e2e/modules/oagw/test_authz.py
- config/e2e-local.yaml
- modules/system/oagw/scenarios/INDEX.md
- modules/system/oagw/scenarios/plugins/guards/positive-10.2-built-cors-handling.md
- modules/system/oagw/docs/adr-cors.md
- testing/e2e/modules/oagw/test_cors.py
modules/system/oagw/docs/features/0008-cpt-cf-oagw-feature-observability.md
Show resolved
Hide resolved
MikeFalcon77
reviewed
Apr 3, 2026
| .header("access-control-allow-origin", origin) | ||
| .header("access-control-allow-methods", requested_method) | ||
| .header("access-control-allow-headers", requested_headers) | ||
| .header("access-control-max-age", "86400") |
Collaborator
There was a problem hiding this comment.
HIGH
Permissive preflight response omits Access-Control-Allow-Credentials: true.
Per the Fetch spec (CORS check, steps 6-8), the browser checks the preflight response for Access-Control-Allow-Credentials: true when the main request uses credentials: 'include'. Without it, browsers reject credentialed cross-origin requests at the preflight stage — breaking any upstream configured with allow_credentials: true.
Add .header("access-control-allow-credentials", "true") here. Since the preflight is intentionally permissive (no upstream config available), always including it is consistent with the echo-everything approach. The actual-request response will enforce the real credentials policy via apply_cors_headers.
asmith987
force-pushed
the
fix/oagw-cors-preflight
branch
from
732c5b6 to
b2958d2
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
modules/system/oagw/oagw/src/infra/proxy/service.rs (1)
420-436:⚠️ Potential issue | 🟠 MajorOnly enforce CORS on true cross-origin requests.
This block still runs whenever an
Originheader is present. Browsers also sendOriginon same-origin POST/PUT/DELETE requests and on WebSocket handshakes, so same-origin traffic can be rejected here whenever the gateway’s own origin is not inallowed_origins. It also means the later response path can add proxy CORS headers to same-origin responses. Gate this on an actual origin comparison (scheme/host/port) instead of raw header presence.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/system/oagw/oagw/src/infra/proxy/service.rs` around lines 420 - 436, The CORS checks in the block referencing effective_cors, request_origin, is_origin_allowed and is_method_allowed are firing on any present Origin header; change this to only enforce when the request is actually cross-origin by parsing and comparing the request Origin against the gateway's own origin (scheme, host, port) rather than raw header presence. Update the logic around request_origin/instance_uri to parse both origins (or use an existing helper) to detect same-origin requests and skip the is_origin_allowed/is_method_allowed checks for same-origin; only call DomainError::CorsOriginNotAllowed or DomainError::CorsMethodNotAllowed when the origins differ (true cross-origin).
🧹 Nitpick comments (4)
docs/api/api.json (1)
8272-8285: Add an explicittype: "object"to theProblem.contextschema.The
contextproperty lacks type specification, which weakens validation and client code generation. Since it holds structured metadata, it should be explicitly typed as an object.♻️ Proposed schema adjustment
"context": { + "type": "object", + "additionalProperties": true, "description": "Optional structured context (e.g. `resource_type`, `resource_name`)." },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/api/api.json` around lines 8272 - 8285, The Problem schema's context property is missing an explicit type; update the Problem.context schema by adding "type": "object" to the context property definition so it is validated as structured metadata (e.g., modify the properties block for "context" inside the Problem schema to include type: "object" and any needed description/constraints). Ensure the change targets the "Problem" schema's "context" property entry.modules/system/oagw/oagw/tests/proxy_integration.rs (3)
3671-3678: Assert specific error variant instead of justis_err().The test verifies the request fails but doesn't confirm it fails for the correct reason. Without asserting the specific error variant (
CorsOriginNotAllowed), this test could pass if the request fails due to an unrelated issue (e.g., route not found, validation error).♻️ Proposed fix to assert specific error variant
let response = h .facade() .proxy_request(h.security_context().clone(), req) .await; - assert!( - response.is_err(), - "disallowed origin on actual request should be rejected before reaching upstream" - ); + match response { + Err(err) => assert!( + matches!( + err, + oagw_sdk::error::ServiceGatewayError::CorsOriginNotAllowed { .. } + ), + "expected CorsOriginNotAllowed, got: {err:?}" + ), + Ok(resp) => panic!( + "expected CorsOriginNotAllowed error, got response with status {}", + resp.status() + ), + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/system/oagw/oagw/tests/proxy_integration.rs` around lines 3671 - 3678, The test currently only checks response.is_err() after calling facade().proxyRequest (proxy_request) which can hide unrelated failures; replace that generic assertion with a specific assertion that response is an Err containing the CorsOriginNotAllowed variant (e.g., match or matches! against the error enum variant used by proxy_request, such as OagwError::CorsOriginNotAllowed) so the test ensures the failure reason is CORS origin rejection rather than some other error from proxy_request/facade().
3804-3811: Assert specific error variant for non-matching origin.For consistency with the other CORS rejection tests, this should also verify the
CorsOriginNotAllowedvariant.♻️ Proposed fix
let response = h .facade() .proxy_request(h.security_context().clone(), req) - .await; - assert!( - response.is_err(), - "non-matching origin should be rejected with 403" - ); + .await; + match response { + Err(err) => assert!( + matches!( + err, + oagw_sdk::error::ServiceGatewayError::CorsOriginNotAllowed { .. } + ), + "expected CorsOriginNotAllowed for non-matching origin, got: {err:?}" + ), + Ok(resp) => panic!( + "expected CorsOriginNotAllowed error, got response with status {}", + resp.status() + ), + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/system/oagw/oagw/tests/proxy_integration.rs` around lines 3804 - 3811, The test currently only asserts that proxy_request returned an error for a non-matching origin; change the assertion to verify the specific CORS rejection variant (CorsOriginNotAllowed) instead. Update the check around the call to h.facade().proxy_request(h.security_context().clone(), req).await (the response variable) to assert that the Err matches the CorsOriginNotAllowed variant (e.g., by using pattern matching or an expect_err and matching the error enum variant) so the test aligns with other CORS rejection tests.
3740-3748: Assert specific error variant for method rejection test.Same issue as above — the test should verify
CorsMethodNotAllowedrather than just checkingis_err().♻️ Proposed fix
let response = h .facade() .proxy_request(h.security_context().clone(), req) .await; - assert!( - response.is_err(), - "disallowed method on actual request should be rejected before reaching upstream" - ); + match response { + Err(err) => assert!( + matches!( + err, + oagw_sdk::error::ServiceGatewayError::CorsMethodNotAllowed { .. } + ), + "expected CorsMethodNotAllowed, got: {err:?}" + ), + Ok(resp) => panic!( + "expected CorsMethodNotAllowed error, got response with status {}", + resp.status() + ), + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/system/oagw/oagw/tests/proxy_integration.rs` around lines 3740 - 3748, The test currently only checks response.is_err(); instead assert that the error is the specific CorsMethodNotAllowed variant by unwrapping the Err and matching its variant — call h.facade().proxy_request(...).await, capture the Result into response, then extract the error (e.g. let err = response.expect_err("...");) and assert matches!(err, ErrorType::CorsMethodNotAllowed) or assert_eq!(err, ErrorType::CorsMethodNotAllowed) using the actual error enum used in the code; update the assertion in proxy_integration.rs to reference the exact CorsMethodNotAllowed variant instead of is_err().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@modules/system/oagw/oagw/src/infra/proxy/service.rs`:
- Around line 420-436: The CORS checks in the block referencing effective_cors,
request_origin, is_origin_allowed and is_method_allowed are firing on any
present Origin header; change this to only enforce when the request is actually
cross-origin by parsing and comparing the request Origin against the gateway's
own origin (scheme, host, port) rather than raw header presence. Update the
logic around request_origin/instance_uri to parse both origins (or use an
existing helper) to detect same-origin requests and skip the
is_origin_allowed/is_method_allowed checks for same-origin; only call
DomainError::CorsOriginNotAllowed or DomainError::CorsMethodNotAllowed when the
origins differ (true cross-origin).
---
Nitpick comments:
In `@docs/api/api.json`:
- Around line 8272-8285: The Problem schema's context property is missing an
explicit type; update the Problem.context schema by adding "type": "object" to
the context property definition so it is validated as structured metadata (e.g.,
modify the properties block for "context" inside the Problem schema to include
type: "object" and any needed description/constraints). Ensure the change
targets the "Problem" schema's "context" property entry.
In `@modules/system/oagw/oagw/tests/proxy_integration.rs`:
- Around line 3671-3678: The test currently only checks response.is_err() after
calling facade().proxyRequest (proxy_request) which can hide unrelated failures;
replace that generic assertion with a specific assertion that response is an Err
containing the CorsOriginNotAllowed variant (e.g., match or matches! against the
error enum variant used by proxy_request, such as
OagwError::CorsOriginNotAllowed) so the test ensures the failure reason is CORS
origin rejection rather than some other error from proxy_request/facade().
- Around line 3804-3811: The test currently only asserts that proxy_request
returned an error for a non-matching origin; change the assertion to verify the
specific CORS rejection variant (CorsOriginNotAllowed) instead. Update the check
around the call to h.facade().proxy_request(h.security_context().clone(),
req).await (the response variable) to assert that the Err matches the
CorsOriginNotAllowed variant (e.g., by using pattern matching or an expect_err
and matching the error enum variant) so the test aligns with other CORS
rejection tests.
- Around line 3740-3748: The test currently only checks response.is_err();
instead assert that the error is the specific CorsMethodNotAllowed variant by
unwrapping the Err and matching its variant — call
h.facade().proxy_request(...).await, capture the Result into response, then
extract the error (e.g. let err = response.expect_err("...");) and assert
matches!(err, ErrorType::CorsMethodNotAllowed) or assert_eq!(err,
ErrorType::CorsMethodNotAllowed) using the actual error enum used in the code;
update the assertion in proxy_integration.rs to reference the exact
CorsMethodNotAllowed variant instead of is_err().
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 778510e8-1f8b-492d-a2ac-b9573da97d7c
📒 Files selected for processing (32)
config/e2e-local.yamldocs/api/api.jsonmodules/system/api-gateway/src/middleware/auth.rsmodules/system/oagw/docs/ADR/0006-cors.mdmodules/system/oagw/docs/DESIGN.mdmodules/system/oagw/docs/adr-cors.mdmodules/system/oagw/docs/features/0004-cpt-cf-oagw-feature-proxy-engine.mdmodules/system/oagw/docs/features/0008-cpt-cf-oagw-feature-observability.mdmodules/system/oagw/docs/schemas/route.v1.schema.jsonmodules/system/oagw/docs/schemas/upstream.v1.schema.jsonmodules/system/oagw/oagw-sdk/src/models.rsmodules/system/oagw/oagw/src/api/rest/dto.rsmodules/system/oagw/oagw/src/api/rest/error.rsmodules/system/oagw/oagw/src/api/rest/handlers/proxy.rsmodules/system/oagw/oagw/src/domain/cors.rsmodules/system/oagw/oagw/src/domain/error.rsmodules/system/oagw/oagw/src/domain/model.rsmodules/system/oagw/oagw/src/domain/services/client.rsmodules/system/oagw/oagw/src/domain/services/management.rsmodules/system/oagw/oagw/src/infra/proxy/service.rsmodules/system/oagw/oagw/src/infra/type_provisioning.rsmodules/system/oagw/oagw/tests/proxy_integration.rsmodules/system/oagw/scenarios/INDEX.mdmodules/system/oagw/scenarios/flows/plugin-execution.mdmodules/system/oagw/scenarios/plugins/guards/negative-10.3-cors-credentials-wildcard-rejected-config-validation.mdmodules/system/oagw/scenarios/plugins/guards/positive-10.2-built-cors-handling.mdtesting/e2e/conftest.pytesting/e2e/modules/file_parser/generate_file_parser_golden.pytesting/e2e/modules/oagw/conftest.pytesting/e2e/modules/oagw/test_authz.pytesting/e2e/modules/oagw/test_body_validation.pytesting/e2e/modules/oagw/test_cors.py
💤 Files with no reviewable changes (7)
- modules/system/oagw/oagw/src/infra/type_provisioning.rs
- modules/system/oagw/oagw/src/domain/model.rs
- modules/system/oagw/oagw-sdk/src/models.rs
- modules/system/oagw/oagw/src/api/rest/dto.rs
- modules/system/oagw/oagw/src/domain/services/client.rs
- modules/system/oagw/docs/schemas/upstream.v1.schema.json
- modules/system/oagw/oagw/src/domain/services/management.rs
✅ Files skipped from review due to trivial changes (2)
- modules/system/oagw/docs/features/0004-cpt-cf-oagw-feature-proxy-engine.md
- modules/system/oagw/scenarios/plugins/guards/negative-10.3-cors-credentials-wildcard-rejected-config-validation.md
🚧 Files skipped from review as they are similar to previous changes (14)
- modules/system/oagw/oagw/src/api/rest/error.rs
- modules/system/oagw/docs/schemas/route.v1.schema.json
- testing/e2e/modules/oagw/test_authz.py
- modules/system/oagw/oagw/src/domain/error.rs
- modules/system/oagw/docs/DESIGN.md
- testing/e2e/modules/oagw/conftest.py
- modules/system/oagw/oagw/src/api/rest/handlers/proxy.rs
- testing/e2e/conftest.py
- testing/e2e/modules/file_parser/generate_file_parser_golden.py
- modules/system/oagw/docs/adr-cors.md
- modules/system/oagw/scenarios/flows/plugin-execution.md
- modules/system/oagw/scenarios/INDEX.md
- modules/system/oagw/docs/ADR/0006-cors.md
- testing/e2e/modules/oagw/test_body_validation.py
Browser preflight (OPTIONS) requests carry no credentials, so upstream resolution fails. Preflight now returns a permissive 204 at the handler level; origin and method enforcement moves to the actual request path after upstream resolution. Removes allowed_headers and max_age from CorsConfig (max_age hardcoded to 86400, header validation unnecessary with permissive preflight). Signed-off-by: Andre Smith <andre.smith+oss@acronis.com> Signed-off-by: Andre Smith <andre.smith@acronis.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
asmith987
force-pushed
the
fix/oagw-cors-preflight
branch
from
b2958d2 to
84bc327
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
modules/system/oagw/oagw/src/infra/proxy/service.rs (1)
420-436:⚠️ Potential issue | 🟠 MajorDon't treat every
Originheader as cross-origin.Line 422 gates this block on header presence alone. Browsers also send
Originon same-origin POST/PUT/DELETE requests and WebSocket handshakes, so this can return 403 for legitimate same-origin traffic whenever the gateway's own origin is not inallowed_origins. Gate the origin/method checks on an actual cross-origin comparison instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/system/oagw/oagw/src/infra/proxy/service.rs` around lines 420 - 436, The current CORS gating treats any present Origin header as cross-origin; change the logic in the block that references effective_cors, request_origin, is_origin_allowed, and is_method_allowed so you only enforce origin/method checks when the request is actually cross-origin (compare the parsed Origin value against the gateway's own origin/instance URI or the request host+scheme+port to determine same-origin). If the Origin matches the gateway's origin consider it same-origin and skip the CorsOriginNotAllowed/CorsMethodNotAllowed checks; otherwise run crate::domain::cors::is_origin_allowed and is_method_allowed and return the DomainError variants when appropriate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@modules/system/oagw/docs/DESIGN.md`:
- Around line 279-280: The table row incorrectly attributes CORS enforcement to
the guard plugin `gts.x.core.oagw.guard_plugin.v1~x.core.oagw.cors.v1`; update
the DESIGN.md row to either remove CORS from the guard-plugin table or relabel
it as "Core CORS (not a guard-plugin)" and change the description to state that
preflight handling is implemented in api/rest/handlers/proxy.rs and
origin/method validation is implemented in infra/proxy/service.rs, so readers
know the actual implementation locations and responsibilities.
In `@modules/system/oagw/docs/features/0008-cpt-cf-oagw-feature-observability.md`:
- Around line 218-232: The spec omits the CORS method-rejection path implemented
in infra/proxy/service.rs: add a step after Origin validation (e.g., after
inst-cors-h-5 / before inst-cors-h-7) that checks the request HTTP method
against configured allowed_methods and, if not allowed, returns 403 (similar to
inst-cors-h-6 for origin); update the numbered flow, DoD and acceptance-criteria
entries (also update the corresponding mentions around lines 364-365 and
438-440) to include this allowed_methods rejection and its 403 response.
In `@modules/system/oagw/oagw/tests/proxy_integration.rs`:
- Around line 3671-3679: The test currently only asserts that proxy_request
returned an error; change it to match the concrete CORS rejection (expecting the
CorsOriginNotAllowed or CorsMethodNotAllowed variant) by inspecting the Err
returned from h.facade().proxy_request(h.security_context().clone(), req).await
and pattern-matching or using assert_eq! / matches! against the specific
variant; additionally assert that no requests reached the upstream by checking
the test helper's request capture (via h’s upstream/guard request counter or
request list) to ensure upstream received zero requests. Apply the same changes
to the other occurrences noted (around lines 3740-3747 and 3804-3811).
In `@testing/e2e/conftest.py`:
- Around line 18-29: The helper auth_headers currently always returns a truthy
dict because it uses os.getenv with a default token; change it to return None
when E2E_AUTH_TOKEN is not set so existing tests that call `if not auth_headers:
pytest.skip(...)` still skip. Concretely, in the auth_headers function read
E2E_AUTH_TOKEN without a default (e.g., token = os.getenv("E2E_AUTH_TOKEN")),
return None when token is falsy, and only return {"Authorization": f"Bearer
{token}"} when a token exists; this preserves behavior expected by tests like
testing/e2e/modules/file_parser/test_file_parser_xlsx_pptx.py.
In `@testing/e2e/modules/oagw/test_cors.py`:
- Around line 23-31: The OPTIONS tests currently send oagw_headers and use
routes that already allow the tested method, which can mask handler-level
bypass; update test_cors_preflight_fully_permissive and
test_cors_preflight_permissive_echoes_any_method to omit oagw_headers (make the
preflight anonymous), request an HTTP method that the targeted route does NOT
allow (pick a method not in that route's allowed methods so route-level method
checks would fail if invoked), and add an additional case with an Origin value
that is NOT in the allowlist to verify the handler-level permissive preflight
still echoes/permits correctly; adjust the corresponding assertions to expect
permissive 204 behavior for the anonymous preflight and include the new
non-allowlisted Origin case (apply same changes to the tests around lines
68-97).
---
Duplicate comments:
In `@modules/system/oagw/oagw/src/infra/proxy/service.rs`:
- Around line 420-436: The current CORS gating treats any present Origin header
as cross-origin; change the logic in the block that references effective_cors,
request_origin, is_origin_allowed, and is_method_allowed so you only enforce
origin/method checks when the request is actually cross-origin (compare the
parsed Origin value against the gateway's own origin/instance URI or the request
host+scheme+port to determine same-origin). If the Origin matches the gateway's
origin consider it same-origin and skip the
CorsOriginNotAllowed/CorsMethodNotAllowed checks; otherwise run
crate::domain::cors::is_origin_allowed and is_method_allowed and return the
DomainError variants when appropriate.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f2cf5509-a9d1-4bf7-91a2-98f0bbaa739b
📒 Files selected for processing (32)
config/e2e-local.yamldocs/api/api.jsonmodules/system/api-gateway/src/middleware/auth.rsmodules/system/oagw/docs/ADR/0006-cors.mdmodules/system/oagw/docs/DESIGN.mdmodules/system/oagw/docs/adr-cors.mdmodules/system/oagw/docs/features/0004-cpt-cf-oagw-feature-proxy-engine.mdmodules/system/oagw/docs/features/0008-cpt-cf-oagw-feature-observability.mdmodules/system/oagw/docs/schemas/route.v1.schema.jsonmodules/system/oagw/docs/schemas/upstream.v1.schema.jsonmodules/system/oagw/oagw-sdk/src/models.rsmodules/system/oagw/oagw/src/api/rest/dto.rsmodules/system/oagw/oagw/src/api/rest/error.rsmodules/system/oagw/oagw/src/api/rest/handlers/proxy.rsmodules/system/oagw/oagw/src/domain/cors.rsmodules/system/oagw/oagw/src/domain/error.rsmodules/system/oagw/oagw/src/domain/model.rsmodules/system/oagw/oagw/src/domain/services/client.rsmodules/system/oagw/oagw/src/domain/services/management.rsmodules/system/oagw/oagw/src/infra/proxy/service.rsmodules/system/oagw/oagw/src/infra/type_provisioning.rsmodules/system/oagw/oagw/tests/proxy_integration.rsmodules/system/oagw/scenarios/INDEX.mdmodules/system/oagw/scenarios/flows/plugin-execution.mdmodules/system/oagw/scenarios/plugins/guards/negative-10.3-cors-credentials-wildcard-rejected-config-validation.mdmodules/system/oagw/scenarios/plugins/guards/positive-10.2-built-cors-handling.mdtesting/e2e/conftest.pytesting/e2e/modules/file_parser/generate_file_parser_golden.pytesting/e2e/modules/oagw/conftest.pytesting/e2e/modules/oagw/test_authz.pytesting/e2e/modules/oagw/test_body_validation.pytesting/e2e/modules/oagw/test_cors.py
💤 Files with no reviewable changes (7)
- modules/system/oagw/oagw/src/infra/type_provisioning.rs
- modules/system/oagw/oagw-sdk/src/models.rs
- modules/system/oagw/oagw/src/domain/model.rs
- modules/system/oagw/oagw/src/api/rest/dto.rs
- modules/system/oagw/docs/schemas/upstream.v1.schema.json
- modules/system/oagw/oagw/src/domain/services/management.rs
- modules/system/oagw/oagw/src/domain/services/client.rs
✅ Files skipped from review due to trivial changes (2)
- modules/system/oagw/scenarios/plugins/guards/negative-10.3-cors-credentials-wildcard-rejected-config-validation.md
- docs/api/api.json
🚧 Files skipped from review as they are similar to previous changes (15)
- modules/system/oagw/scenarios/flows/plugin-execution.md
- modules/system/api-gateway/src/middleware/auth.rs
- modules/system/oagw/docs/features/0004-cpt-cf-oagw-feature-proxy-engine.md
- testing/e2e/modules/oagw/test_body_validation.py
- modules/system/oagw/oagw/src/domain/error.rs
- testing/e2e/modules/file_parser/generate_file_parser_golden.py
- testing/e2e/modules/oagw/conftest.py
- config/e2e-local.yaml
- modules/system/oagw/oagw/src/api/rest/error.rs
- modules/system/oagw/scenarios/INDEX.md
- modules/system/oagw/docs/schemas/route.v1.schema.json
- modules/system/oagw/oagw/src/api/rest/handlers/proxy.rs
- modules/system/oagw/scenarios/plugins/guards/positive-10.2-built-cors-handling.md
- modules/system/oagw/docs/adr-cors.md
- modules/system/oagw/docs/ADR/0006-cors.md
modules/system/oagw/docs/features/0008-cpt-cf-oagw-feature-observability.md
Show resolved
Hide resolved
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.
Browser preflight (OPTIONS) requests carry no credentials, so upstream resolution fails. Preflight now returns a permissive 204 at the handler level; origin and method enforcement moves to the actual request path after upstream resolution. Removes allowed_headers and max_age from CorsConfig (max_age hardcoded to 86400, header validation unnecessary with permissive preflight).
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Documentation
Tests / Chores