Skip to content

🩹 fix: Middleware/CORS Remove Scheme Restriction#3168

Merged
ReneWerner87 merged 1 commit intogofiber:v2from
zingi:backport/v2/#3163-cors-middleware-remove-scheme-restriction
Oct 14, 2024
Merged

🩹 fix: Middleware/CORS Remove Scheme Restriction#3168
ReneWerner87 merged 1 commit intogofiber:v2from
zingi:backport/v2/#3163-cors-middleware-remove-scheme-restriction

Conversation

@zingi
Copy link
Copy Markdown
Contributor

@zingi zingi commented Oct 14, 2024

Description

Backport of #3163 to v2.

The Fiber CORS middleware unnecessarily restricts the scheme for the origins that can be configured. As already stated here, the CORS spec does not define any restrictions on the scheme. However, the Fiber CORS middleware restricts the scheme to http or https.

Fixes #3160

Changes introduced

Removes the scheme restriction of the CORS middleware configuration.

  • Benchmarks: Describe any performance benchmarks and improvements related to the changes.
  • Documentation Update: Detail the updates made to the documentation and links to the changed files.
  • Changelog/What's New: Include a summary of the additions for the upcoming release notes.
  • Migration Guide: If necessary, provide a guide or steps for users to migrate their existing code to accommodate these changes.
  • API Alignment with Express: Explain how the changes align with the Express API.
  • API Longevity: Discuss the steps taken to ensure that the new or updated APIs are consistent and not prone to breaking changes.
  • Examples: Provide examples demonstrating the new features or changes in action.

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)
  • Enhancement (improvement to existing features and functionality)
  • Documentation update (changes to documentation)
  • Performance improvement (non-breaking change which improves efficiency)
  • Code consistency (non-breaking change which improves code reliability and robustness)

Checklist

Before you submit your pull request, please make sure you meet these requirements:

  • Followed the inspiration of the Express.js framework for new functionalities, making them similar in usage.
  • Conducted a self-review of the code and provided comments for complex or critical parts.
  • Updated the documentation in the /docs/ directory for Fiber's documentation.
  • Added or updated unit tests to validate the effectiveness of the changes or new features.
  • Ensured that new and existing unit tests pass locally with the changes.
  • Verified that any new dependencies are essential and have been agreed upon by the maintainers/community.
  • Aimed for optimal performance with minimal allocations in the new code.
  • Provided benchmarks for the new code to analyze and improve upon.

Commit formatting

Please use emojis in commit messages for an easy way to identify the purpose or intention of a commit. Check out the emoji cheatsheet here: CONTRIBUTING.md

@zingi zingi requested a review from a team as a code owner October 14, 2024 07:46
@zingi zingi requested review from ReneWerner87, efectn, gaby and sixcolors and removed request for a team October 14, 2024 07:46
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Oct 14, 2024 •
edited
Loading

Walkthrough

The changes in this pull request involve modifications to the normalizeOrigin function in the cors package, specifically removing the validation for the scheme of the provided origin. This allows origins with schemes other than "http" or "https" to be accepted. Additionally, the test cases for the normalizeOrigin function have been updated for improved readability and to include a new test case for the "app" scheme.

Changes

File Change Summary
middleware/cors/utils.go Modified normalizeOrigin to remove scheme validation for origins.
middleware/cors/utils_test.go Updated test cases for Test_normalizeOrigin for clarity and added a case for "app" scheme.

Assessment against linked issues

Objective Addressed Explanation
Allow origins with any protocol (Issue #3160) ✅

Possibly related PRs

Suggested labels

☢️ Bug, 🧹 Updates, v3

Suggested reviewers

  • sixcolors
  • ReneWerner87
  • gaby
  • efectn

🐇 In the land of code, I hop with glee,
No more strict checks, let all schemes be!
From "http" to "app", all can play,
CORS is now friendly, hip-hip-hooray!
With tests all polished, we dance and cheer,
For a smoother Fiber, the future is near! 🌟

📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 6e74114 and 1ef88d0.

📒 Files selected for processing (2)
  • middleware/cors/utils.go (0 hunks)
  • middleware/cors/utils_test.go (1 hunks)
💤 Files with no reviewable changes (1)
  • middleware/cors/utils.go
🧰 Additional context used
🔇 Additional comments (2)
middleware/cors/utils_test.go (2)

16-40: Excellent improvements to test case readability and coverage!

The changes to the Test_normalizeOrigin function are well-implemented:

  1. The use of named fields (origin, expectedValid, expectedOrigin) in the test case structure significantly improves readability and maintainability.
  2. The addition of a new test case for the "app" scheme (line 20) aligns perfectly with the PR objectives to remove scheme restrictions.
  3. The changes are consistently applied across all test cases, maintaining the overall structure and readability of the test suite.

These updates will make it easier for developers to understand and maintain the test cases in the future.

Line range hint 1-15: Consider reviewing unchanged tests for completeness

While the changes to Test_normalizeOrigin are excellent, it's important to ensure that the other tests in this file (Test_matchScheme, Test_normalizeDomain, Benchmark_CORS_SubdomainMatch, and Test_CORS_SubdomainMatch) still cover all necessary cases with the new scheme flexibility.

Specifically:

  1. Test_matchScheme might benefit from additional test cases for non-http/https schemes.
  2. Test_normalizeDomain could include cases with the new flexible schemes.
  3. Test_CORS_SubdomainMatch might need updates to reflect the removal of scheme restrictions.

To assist in this review, you can run the following script to check for potential gaps in test coverage:

This script will help identify areas where additional test cases for flexible schemes might be beneficial.

Also applies to: 41-214

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@gaby gaby changed the title 🩹 Fix: Middleware/CORS Remove Scheme Restriction (gofiber#3160, backport of #3163 to v2) 🩹 fix: Middleware/CORS Remove Scheme Restriction Oct 14, 2024
gaby
gaby approved these changes Oct 14, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@gaby gaby left a comment

Choose a reason for hiding this comment

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

LGTM

@gaby
Copy link
Copy Markdown
Member

gaby commented Oct 14, 2024

@ReneWerner87 We need a backport label 😂

@ReneWerner87 ReneWerner87 added this to the Next Release milestone Oct 14, 2024
@ReneWerner87 ReneWerner87 merged commit 8c84b0f into gofiber:v2 Oct 14, 2024
thylong pushed a commit to thylong/fiber that referenced this pull request Dec 11, 2024
@zingi @aaronz-vipaso @thylong
🩹 fix: Middleware/CORS Remove Scheme Restriction (gofiber#3168)
a8300e0 
🩹 Fix: Middleware/CORS Remove Scheme Restriction (gofiber#3160)

Co-authored-by: Aaron Zingerle <aaron.zingerle@vipaso.io>
@coderabbitai coderabbitai Bot mentioned this pull request Jan 8, 2026
Merged
@campaigner-prod campaigner-prod Bot mentioned this pull request Apr 23, 2026
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gaby gaby gaby approved these changes

@ReneWerner87 ReneWerner87 ReneWerner87 approved these changes

@sixcolors sixcolors Awaiting requested review from sixcolors sixcolors is a code owner automatically assigned from gofiber/maintainers

@efectn efectn Awaiting requested review from efectn efectn is a code owner automatically assigned from gofiber/maintainers

Assignees

No one assigned

Labels

☢️ Bug v2

Projects

None yet

Milestone

v2.52.6

Development

Successfully merging this pull request may close these issues.

4 participants

@zingi @gaby @ReneWerner87 @aaronz-vipaso