Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for wildcard '*' in cors domains #563

Merged
merged 5 commits into from
Nov 19, 2023

Conversation

Sagebati
Copy link
Contributor

@Sagebati Sagebati commented Apr 28, 2023

Hi !

This new feature is especially useful for applications that utilize multiple subdomains, as it eliminates the need to manually configure CORS for each subdomain. By simply adding the wildcard (*) to your CORS configuration, you can enable cross-domain requests for all subdomains of a given domain. I replaced the HashSet::contains search with a Regex containing all the domains (example:https://regex101.com/r/ZEhBy4/2). I'm aware of the allow_origin_fn, but in opinion this is ergonomic. The PR is almost ready to be merged only need a review of the unwrap used.

Any feedback is welcome.

@Sagebati Sagebati changed the title Add support for wildsward '*' in cors domains Add support for wildcard '*' in cors domains Apr 28, 2023
@attila-lin
Copy link
Collaborator

Hi, @Sagebati , Thank you for your mr.

Would you please add some testcase and fix the CI?

@Sagebati

This comment was marked as duplicate.

@Sagebati

This comment was marked as outdated.

@Sagebati

This comment was marked as outdated.

@Sagebati
Copy link
Contributor Author

Seems like the CI passes

Copy link
Contributor

@DDtKey DDtKey left a comment

Choose a reason for hiding this comment

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

Just a suggestion: consider to use a wildmatch crate

@Sagebati
Copy link
Contributor Author

Sagebati commented May 31, 2023

I didn't knew this crate, I could rewrite the feature to use it. The change implies that we will have a Vec<WildMatch> for each url, instead of only one regex, but it's easier to maintain. Let met know your preferences

@sunli829
Copy link
Collaborator

sunli829 commented Jun 6, 2023

What do you think if it could be better like below? @Sagebati

pub struct Cors {
    allow_credentials: bool,
    allow_origins: HashSet<HeaderValue>,
    allow_origins_wildcard: Vec<WildMatch>, //////// <<<<<<<<<
    allow_origins_fn: Option<Arc<dyn Fn(&str) -> bool + Send + Sync>>,
    allow_headers: HashSet<HeaderName>,
    allow_methods: HashSet<Method>,
    expose_headers: HashSet<HeaderName>,
    max_age: i32,
}

impl Cors {
    fn allow_origin_to_regex(mut self, wildcard: impl AsRef<str>) -> Self {
        self.allow_origins_wildcard.push(WildMatch::new(wildcard));
        self
    }
}

Added the functionality to specify wildcard '*'  in cors domains
@Sagebati
Copy link
Contributor Author

Sagebati commented Jun 6, 2023

I rewrote the PR to use the wildmatch crate, let me know if there is anything to review
Thanks for your time

@sunli829
Copy link
Collaborator

sunli829 commented Jun 7, 2023

Could you help to add a test case? 🙂 @Sagebati

@sunli829
Copy link
Collaborator

Thanks! @Sagebati 🙂

@sunli829 sunli829 merged commit 80d31ca into poem-web:master Nov 19, 2023
8 of 9 checks passed
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.

4 participants