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

Support wildcard for ExposedHeaders option. #84

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
61 changes: 59 additions & 2 deletions cors.go
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,8 @@ type Options struct {
// Default value is [] but "Origin" is always appended to the list.
AllowedHeaders []string
// ExposedHeaders indicates which headers are safe to expose to the API of a CORS
// API specification
// API specification.
// If the special "*" value is present in the list, all headers will be allowed.
ExposedHeaders []string
// MaxAge indicates how long (in seconds) the results of a preflight request
// can be cached
Expand Down Expand Up @@ -194,6 +195,7 @@ func AllowAll() *Cors {
},
AllowedHeaders: []string{"*"},
AllowCredentials: false,
ExposedHeaders: []string{"*"},
})
}

Expand All @@ -216,12 +218,15 @@ func (c *Cors) Handler(h http.Handler) http.Handler {
} else {
c.logf("Handler: Actual request")
c.handleActualRequest(w, r)
w = &ExposeAllRespWriter{w, false}
h.ServeHTTP(w, r)
}
})
}

// HandlerFunc provides Martini compatible handler
// HandlerFunc provides Martini compatible handler.
// Since a handler isn't wrapped using this func, considering using
// ExposeAllRespWriter for wildcard support.
func (c *Cors) HandlerFunc(w http.ResponseWriter, r *http.Request) {
if r.Method == http.MethodOptions && r.Header.Get("Access-Control-Request-Method") != "" {
c.logf("HandlerFunc: Preflight request")
Expand Down Expand Up @@ -249,6 +254,7 @@ func (c *Cors) ServeHTTP(w http.ResponseWriter, r *http.Request, next http.Handl
} else {
c.logf("ServeHTTP: Actual request")
c.handleActualRequest(w, r)
w = &ExposeAllRespWriter{w, false}
next(w, r)
}
}
Expand Down Expand Up @@ -427,3 +433,54 @@ func (c *Cors) areHeadersAllowed(requestedHeaders []string) bool {
}
return true
}

// ExposeAllRespWriter echos back any headers that are set in the wrapped response writer
// to support the wildcard "*" case for Access-Control-Expose-Headers since
// browsers do not currently have good compatibility.
type ExposeAllRespWriter struct {
Copy link
Owner

Choose a reason for hiding this comment

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

Please do not wrap the response writer. This has the undesired side effect of hiding optional interfaces. Introducing this would silently break many code bases using package.

Copy link
Author

Choose a reason for hiding this comment

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

Yes I have run into that many times, but never seen any good solutions. How do you suggest the headers that are set by the wrapped handler from cors.(*Cors).Handler() by caught to then be applied as they are in here?

Copy link
Owner

Choose a reason for hiding this comment

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

I don't think it's doable, but I'd rather give up on the feature than breaking many users of the lib.

Copy link
Author

Choose a reason for hiding this comment

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

It is prevalent to wrap response writers for middleware of various kinds since there isn't another way to do this, and code that relies on optional interfaces to do a necessary thing (even necessary performance) seems like a bad idea in all the cases I have run across, so personally I would be hesitant to encourage that, but certainly it is a valid point.
If the caller has the opportunity to wrap themselves and make sure the interfaces they want are supported, would that be enough to ease the concern? I can add a field to the options of type http.ResponseWriter, and the caller can wrap the exported type here (rather than implementing it themselves), which also would make it an optional feature. However the argument could also be made that using AllowAll() is already optional ;)

Copy link
Owner

Choose a reason for hiding this comment

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

I do not agree. For some applications, those optional interfaces are the only way to go (think about flusher for instance). It is not the role of a library to encourage or discourage the way the http library is used. The lib must not break any application, or it will lose the trust of its users.

The support for wildcards with Access-Control-Expose-Headers will only increase. So I would prefer to just plan for that and not add tech dept to this lib.

http.ResponseWriter
applied bool
}

func (w *ExposeAllRespWriter) Write(b []byte) (int, error) {
w.setHeaders()
return w.ResponseWriter.Write(b)
}

func (w *ExposeAllRespWriter) WriteHeader(c int) {
w.setHeaders()
w.ResponseWriter.WriteHeader(c)
}

func (w *ExposeAllRespWriter) setHeaders() {
if w.applied {
return
}
w.applied = true

if w.ResponseWriter.Header().Get("Access-Control-Expose-Headers") != "*" {
return
}

var toExpose []string
for k := range w.ResponseWriter.Header() {
switch k {
case
// CORs headers that could be set when Access-Control-Expose-Headers is set
"Access-Control-Allow-Origin", "Access-Control-Allow-Credentials", "Access-Control-Expose-Headers",

// already allowed by spec
"Cache-Control", "Content-Language", "Content-Type", "Expires", "Last-Modified", "Pragma":
continue
default:
toExpose = append(toExpose, k)
}
}

if len(toExpose) == 0 {
w.ResponseWriter.Header().Del("Access-Control-Expose-Headers")
return
}

w.ResponseWriter.Header().Set("Access-Control-Expose-Headers", strings.Join(toExpose, ", "))
}