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

Provide a way to specify a regex for allowed origins #4963

Closed
minwoox opened this issue Jun 16, 2023 · 9 comments
Closed

Provide a way to specify a regex for allowed origins #4963

minwoox opened this issue Jun 16, 2023 · 9 comments
Milestone

Comments

@minwoox
Copy link
Member

minwoox commented Jun 16, 2023

Currently, we use the exact match for the allowed origin:

} else if (!isNullOrigin && policy.origins().contains(lowerCaseOrigin) &&

This can be a problem if there are tremendous subdomains that are frequently changed because users cannot specify all of them.

@minwoox minwoox added this to the 1.25.0 milestone Jun 16, 2023
@seonWKim
Copy link
Contributor

Please assign this to me ~ 🥺

@minwoox
Copy link
Member Author

minwoox commented Jun 16, 2023

It's all yours. Thanks! 😆

@seonWKim
Copy link
Contributor

seonWKim commented Jun 18, 2023

@minwoox Thank you for assigning the issue to me 😄 . I have a question about the issue.

It seems that armeria allow users to configure CORS in below ways:

  • CorsService's builder(String...origins)
  • CorsDecorator annotation's origins

Should we provide features that allow users to configure origins using regex by adding additional field such as originPatterns or just consider the origin String as a regex expression ?

@minwoox
Copy link
Member Author

minwoox commented Jun 19, 2023

We are specifying the origin when we create the builder so I prefer to specify the regex when creating a builder:

CorsService.builderForRegex("^https:\\/\\/.*example.com$")
           .allowRequestMethods(HttpMethod.POST, HttpMethod.GET)
           .andForOriginRegex("^https:\\/\\/.*example2.com$")
           .allowRequestMethods(HttpMethod.GET)
           .and()
           .newDecorator()));
// Use Pattern
Pattern regex = ...
CorsService.builderForRegex(regex)
           .allowRequestMethods(HttpMethod.POST, HttpMethod.GET)
           ...

We can also specify the regex string to @CorsDecoror

@CorsDecorator(
  originRegex = "^https:\\/\\/.*example.com$",
  pathPatterns = "glob:/**/configured",
  allowedRequestMethods = HttpMethod.GET
)

We can also add a predicate for the builder only. (Not for the annotation)

Predicate predicate = ...
CorsService.builder(predicate)
           .allowRequestMethods(HttpMethod.POST, HttpMethod.GET)

@seonWKim
Copy link
Contributor

Thank you for your detailed comment. I want to make sure that I understand what features are needed 😄 .
Are below features the ones that should be implemented ?

  1. Add methods to indicate regex in the CorsService and CorsServiceBuilder (e.g. builderForRegex, andForOriginRegex and some overloaded methods)
  2. Add originRegex for CorsDecorator annotaion
  3. Accept Predicate in CorsService.builder(...) method.

@minwoox
Copy link
Member Author

minwoox commented Jun 19, 2023

Yes, exactly. 😄 We also need to add those methods to CorsPolicy:

class CorsService {

    public static CorsServiceBuilder builder(Predicate<String> predicate) {...}
    // Need to consider a better name.
    public static CorsServiceBuilder builderForOriginRegex(String regex) {...}
    public static CorsServiceBuilder builderForOriginRegex(Pattern regex) {...}
}

class CorsServiceBuilder {

    CorsServiceBuilder(Predicate<String> predicate) {...}
    CorsServiceBuilder(Pattern regex) {...}

    public ChainedCorsPolicyBuilder andForOriginRegex(String regex) {...}
    public ChainedCorsPolicyBuilder andForOriginRegex(Pattern regex) {...}
}

class CorsPolicy {
    public static CorsPolicyBuilder builder(Predicate<String> predicate) {...}
    public static CorsPolicyBuilder builderForOriginRegex(String regex) {...}
    public static CorsPolicyBuilder builderForOriginRegex(Pattern regex) {...}
}

class CorsPolicyBuilder {

    CorsPolicyBuilder(Predicate<String> predicate) {...}
    CorsPolicyBuilder(Pattern regex) {...}
}

public @interface CorsDecorator {
    String[] origins();
    String originRegex(); // We could probably raise an exception if both origins and originRegex are specified.
    ...
}

We also need to make WebSockerServiceBuilder support the Predicate. We don't have to support the regex because unlike CorsDecorator, it doesn't have an annotation which the value should be specified in String.

class WebSocketServiceBuilder {
    public WebSocketServiceBuilder allowedOrigins(Predicate<String> predicate) {...}
}

@seonWKim
Copy link
Contributor

seonWKim commented Jun 21, 2023

Hi @minwoox , thank you for the super detailed and wonderful explanation. I wonder what policy we should take.

  • Allow users to specify origins, originRegex and predicate at the same time.
    • If so, when we verify a origin, what's the policy for verifying allowed origins ?
      • user requested origins should match: origins AND originRegex AND predicate
      • user requested origins should match: origins OR originRegex OR predicate
  • Allow users to specify only one of origins, originRegex or `predicate.

Based on the comment you wrote in below code, it seems that we might not want users to specify both origins and originRegex at the same time.

public @interface CorsDecorator {
    String[] origins();
    String originRegex(); // We could probably raise an exception if both origins and originRegex are specified.
    ...
}

@minwoox
Copy link
Member Author

minwoox commented Jun 22, 2023

Allow users to specify only one of origins, originRegex or `predicate.

This is what I'm saying. regex or Predicate can cover multiple domains so I think we don't have to provide a way to chain those conditions. Predicate even has its own and and or methods, so users can chain them if they want.

@jrhee17 jrhee17 modified the milestones: 1.25.0, 1.26.0 Aug 15, 2023
@minwoox minwoox modified the milestones: 1.26.0, 1.27.0 Oct 11, 2023
@ikhoon ikhoon modified the milestones: 1.27.0, 1.28.0 Jan 16, 2024
jrhee17 pushed a commit that referenced this issue Apr 4, 2024
… for CORS allowed origins (#4982)

Motivation:

Allow users to specify CORS allowed origins by regular expression or
`Predicate<String>`

Modifications:

Users can specify allowed origins by using regular expression or
`Predicate<String>` like below.

<b>CorsDecorator</b> 
```
@CorsDecorator(
        originRegex = "http://example.*",
        allowedRequestMethods = HttpMethod.GET
)
``` 

<b>`CorsService`'s `builderForOriginRegex` or `builder(Predicate<String>
originPredicate)`</b>
```
  sb.service("/cors15", myService.decorate(
          CorsService.builderForOriginRegex("http://example.*")
                     .allowRequestMethods(HttpMethod.GET)
                     .newDecorator()));

  sb.service("/cors17", myService.decorate(
          CorsService.builder(origin -> origin.contains("example") || origin.contains("line"))
                     .allowRequestMethods(HttpMethod.GET)
                     .newDecorator()));
```

or per route 
```
sb.annotatedService("/cors18", new Object() {
                        @get("/index1")
                        public void index1() {}

                        @post("/index2")
                        public void index2() {}

                        @delete("/index3")
                        public void index3() {}
                    }, CorsService.builder()
                                  .andForOriginRegex("http://example.*")
                                  .route("/cors18/index1")
                                  .allowRequestMethods(HttpMethod.GET)
                                  .and()
                                  .andForOriginRegex(Pattern.compile(".*line.*"))
                                  .route("/cors18/index2")
                                  .allowRequestMethods(HttpMethod.POST)
                                  .and()
                                  .andForOrigin((origin) -> origin.contains("armeria"))
                                  .route("/cors18/index3")
                                  .allowRequestMethods(HttpMethod.DELETE)
                                  .and()
                                  .newDecorator()
```

<b>`WebSocketServiceBuilder`'s `allowedOrigins(Predicate<String>
originPredicate>`</b>
```
 sb.route()
    .path("/chat")
    .build(WebSocketService.builder(new CustomWebSocketServiceHandler())
                           .allowedOrigins(origin -> origin.contains("armeria"))
                           .build());
```

Result:

- Closes #<#4963>. (If this
resolves the issue.)
- Users can use regular expression or `Predicate<String>` to specify
allowed origins

---------

Co-authored-by: minwoox <songmw725@gmail.com>
Co-authored-by: Ikhun Um <ih.pert@gmail.com>
@jrhee17
Copy link
Contributor

jrhee17 commented Apr 8, 2024

closed by #4982

@jrhee17 jrhee17 closed this as completed Apr 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants