Skip to content

Conversation

@akshaydeo
Copy link
Contributor

@akshaydeo akshaydeo commented Dec 16, 2025

Summary

Update CORS middleware to use the correct path extraction method and expand allowed HTTP methods and headers.

Changes

  • Fixed path extraction in CORS middleware by using string(ctx.Path()) instead of ctx.Request.URI().Path()
  • Added HEAD to the list of allowed HTTP methods in CORS headers
  • Added X-Stainless-Timeout to the list of allowed headers
  • Added a mock logger implementation for testing to prevent real logging during tests

Type of change

  • Bug fix
  • Feature
  • Refactor
  • Documentation
  • Chore/CI

Affected areas

  • Core (Go)
  • Transports (HTTP)
  • Providers/Integrations
  • Plugins
  • UI (Next.js)
  • Docs

How to test

Test the CORS middleware with various origins and verify the correct headers are set:

# Core/Transports
go version
go test ./transports/bifrost-http/handlers/...

Breaking changes

  • Yes
  • No

Related issues

Fixes issues with CORS headers not being properly set for certain HTTP clients that require the X-Stainless-Timeout header and HEAD method.

Security considerations

This change maintains the same security model for CORS, only expanding the allowed methods and headers to support more client implementations.

Checklist

  • I added/updated tests where appropriate
  • I verified builds succeed (Go and UI)
  • I verified the CI pipeline passes locally if applicable

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 16, 2025

Caution

Review failed

The pull request is closed.

📝 Walkthrough

Summary by CodeRabbit

  • Improvements
    • Extended CORS support to include the HEAD HTTP method
    • Added X-Stainless-Timeout to the list of allowed request headers in CORS configuration

✏️ Tip: You can customize this high-level summary in your review settings.

Walkthrough

This PR updates the CORS middleware logging mechanism and extends test coverage by introducing a mock logger implementation. It also updates expected CORS header values in tests to include additional allowed methods and header names for request handling.

Changes

Cohort / File(s) Summary
CORS Middleware Updates
transports/bifrost-http/handlers/middlewares.go
Changed request path logging source from ctx.Request.URI().Path() to string(ctx.Path()) in CorsMiddleware
CORS Middleware Tests
transports/bifrost-http/handlers/middlewares_test.go
Added mockLogger implementation of schemas.Logger interface; integrated mock logger in TestCorsMiddleware_LocalhostOrigins; extended CORS header validation to expect "HEAD" in Access-Control-Allow-Methods and "X-Stainless-Timeout" in Access-Control-Allow-Headers

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

  • Verify the ctx.Path() replacement preserves the logging output equivalently
  • Ensure mockLogger correctly implements all required schemas.Logger interface methods
  • Confirm the updated CORS header expectations align with intended security and compatibility policy

Poem

🐰 A logger hops to cleaner paths,
While test mocks guard the gateway's wrath,
New headers bloom in spring's soft light,
CORS middleware shines more bright! ✨

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch 12-16-fixed_middleware_test_cases

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ef06302 and 3472c29.

📒 Files selected for processing (2)
  • transports/bifrost-http/handlers/middlewares.go (1 hunks)
  • transports/bifrost-http/handlers/middlewares_test.go (3 hunks)

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

Copy link
Contributor Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

@github-actions
Copy link
Contributor

🧪 Test Suite Available

This PR can be tested by a repository admin.

Run tests for PR #1102

@akshaydeo akshaydeo marked this pull request as ready for review December 16, 2025 01:17
@akshaydeo akshaydeo merged commit 777f24b into main Dec 16, 2025
9 of 10 checks passed
@akshaydeo akshaydeo deleted the 12-16-fixed_middleware_test_cases branch December 16, 2025 01:18
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