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

CORSAdaptor should support CORS requests with "Origin" header #483

Closed
jthann opened this issue Jan 27, 2022 · 3 comments · Fixed by #498
Closed

CORSAdaptor should support CORS requests with "Origin" header #483

jthann opened this issue Jan 27, 2022 · 3 comments · Fixed by #498
Labels
enhancement New feature or request

Comments

@jthann
Copy link
Collaborator

jthann commented Jan 27, 2022

The current builtin filter CORSAdaptor implementation just handle the preflight OPTIONS request in CORS not-so-simple request and do nothing for other CORS request.
The CORS divide request into two category: simple request and not-so-simple request.
For not-so-simple request,after OPTIONS preflight request passed,later request also should be handled through http Origin request Header and add response header Access-Control-Allow-Origin.
For simple request it doest't have preflight request and should also be handled through http Origin request Header and add response header Access-Control-Allow-Origin

So the right way do this as following pseudo-code show:

func handle(req http.Request, resp http.Response) {
         //is it a CORS request
       isCorsRequest:= req.getHeader("Origin")!=""
       if !isCorsRequest {             
            callNextFilter()  //Execute next filter in pipeline and return
            return
       }
       isPreflight:= req.Method()=="OPTIONS" && req.getHeader("Access-Control-Request-Method")!=""
       //is it a CORS preflight request
       if isPreflight {
            // valid CORS config and add all CORS response headers
            corsHandle(req, resp, isPreflight)
            return //here should break the whole request pipeline and just respond to client
       }
       if isCorsRequest {
            //valid CORS config and just add Access-Control-Allow-Origin response header
            corsHandle(req, resp)
            callNextFilter()  /here should execute next filter in pipeline
            return
       }
}

By the way we should add MaxAge config in CORSAdaptor spec

We can also see java spring framework source code org.springframework.web.cors.DefaultCorsProcessor and org.springframework.web.cors.CorsUtils as references in artifact 'org.springframework:spring-web:5.3.15'

W3C reference: https://www.w3.org/TR/2020/SPSD-cors-20200602/
One Chinese blog that clarify CORS standard: https://www.ruanyifeng.com/blog/2016/04/cors.html

jthann added a commit to jthann/easegress that referenced this issue Jan 27, 2022
@samutamm samutamm added the enhancement New feature or request label Jan 27, 2022
@samutamm samutamm changed the title The current builtin filter CORSAdaptor implementation is something wrong CORSAdaptor should support CORS requests with "Origin" header Jan 27, 2022
@samutamm
Copy link
Contributor

samutamm commented Jan 27, 2022

Thanks for opening the issue @jthann. The current implementation matches the documentation of CORSAdaptor. It aims to handle the CORS-preflight request (the one with OPTIONS method, a.k.a not-so-simple request) and nothing more.
However I think that your initiative is good; it could also support CORS request (the one with Origin header, a.k.a simple request). Also adding MaxAge is good idea. Your design above seems ok and could be simplified to:

func (a *CORSAdaptor) Handle(ctx context.HTTPContext) string {
    result := a.handle(ctx)
    return ctx.CallNextHandler(result) # next filter is called here
}

func (a *CORSAdaptor) handle(ctx context.HTTPContext) string {
    r := ctx.Request()
    w := ctx.Response()
    method := r.Method()
    isCorsRequest := r.Header().Get("Origin") != ""
    if !isCorsRequest {
      return "" # calls next filter
    }
    isPreflight := method == http.MethodOptions && r.Header().Get("Access-Control-Request-Method") != ""
    a.cors.HandlerFunc(w.Std(), r.Std())
    if isPreflight {
      return resultPreflighted # pipeline jumpIf skips following filters
    }
   return "" # calls next filter
}

We need to also verify that this change is somehow backward compatible and don't break existing Pipelines.

Background

@samutamm
Copy link
Contributor

samutamm commented Jan 27, 2022

To make this backward compatible and to ensure this don't break any existing logic, you could do following:

  • add new boolean flag, for example supportCORSRequest. By default it's false.
  • during runtime, if the flag is false, use exactly the same logic as before (only CORS pre-flight request handled). If it's true, use the logic that deals both with CORS pre-flight and CORS request.

Then by default this change doesn't break anything

@jthann
Copy link
Collaborator Author

jthann commented Jan 27, 2022

okay,the code refactored is more clean and concise and backward compatible way is good. In a word the current implementation before enhancement is useless in our biz using Easegress as proxy for different domain: web frontend page domain and api service domain at the same time

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants