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
feat: Set CORS headers #4990
feat: Set CORS headers #4990
Conversation
Signed-off-by: Joyce Anne Piscos <joycepiscos@gmail.com>
Signed-off-by: Joyce Anne Piscos <joycepiscos@gmail.com>
This reverts commit 9b148a7. Signed-off-by: Joyce Anne Piscos <joycepiscos@gmail.com>
Signed-off-by: Joyce Anne Piscos <joycepiscos@gmail.com>
server/apiserver/argoserver.go
Outdated
return &httpServer | ||
} | ||
|
||
// configureCORS is a middleware for setting CORS headers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Any reason to not insert this next to the x-frames-options code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just want this middleware close to the callers. Should I move it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Look for where we already add headers suck as X-Frame-Options
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay found it. Should I not use the middleware and just call w.Header().Set()
in static.go
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved it near the x-frame-options code, kindly check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joyciep After this change, are the CORS configured also for /api/
URI? If I am reading it right, it is only used for static.NewFilesServer
, isn't it?
Signed-off-by: Joyce Anne Piscos <joycepiscos@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Set
Access-Control-Allow-Origin
header by passing the allowed origins via--access-control-allow-origin
flagFixes #4984
Checklist: