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 cors headers #516

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

add cors headers #516

wants to merge 2 commits into from

Conversation

fetsorn
Copy link

@fetsorn fetsorn commented May 1, 2024

I needed to clone a soft-serve repo from the browser so I added cors headers.

@fetsorn fetsorn requested a review from aymanbagabas as a code owner May 1, 2024 15:52
Copy link
Member

@aymanbagabas aymanbagabas left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution, Soft Serve uses Gorilla libraries and would prefer to stay that way 🙂

pkg/web/http.go Outdated Show resolved Hide resolved
@fetsorn fetsorn mentioned this pull request May 1, 2024
@fetsorn fetsorn requested a review from aymanbagabas May 1, 2024 16:15
pkg/web/http.go Outdated Show resolved Hide resolved
@fetsorn fetsorn requested a review from aymanbagabas May 1, 2024 17:23
@fetsorn
Copy link
Author

fetsorn commented May 6, 2024

ping @aymanbagabas, should I improve this so you can merge? make all rules "*" perhaps for consistency?

@aymanbagabas
Copy link
Member

ping @aymanbagabas, should I improve this so you can merge? make all rules "*" perhaps for consistency?

Hey @fetsorn, I wonder if we can make the CORS origin field configurable. Without that, it can be a security issue for some users. #516 (comment)

@fetsorn
Copy link
Author

fetsorn commented Jun 13, 2024

I added three lists to the http section of yaml configuration

# The HTTP server configuration.
http:
  # The address on which the HTTP server will listen.
  listen_addr: ":23232"
  
  allowed_headers:
    - Content-Type
    - X-Requested-With
 
  allowed_origins:
     - *
 
  allowed_methods:
     - GET
     - HEAD
     - POST 

@aymanbagabas
Copy link
Member

This looks good @fetsorn! I would move the config to http.cors.headers etc to make it clear that these are CORS config.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
pkg/config/config.go Show resolved Hide resolved
@fetsorn
Copy link
Author

fetsorn commented Jun 17, 2024

I brought out the "cors" configuration struct. I believe that "allowed_headers" is more correct than just "headers" because it corresponds to gorilla's variable`handlers.AllowedHeaders" and the preflight header Access-Control-Allow-Headers.

@fetsorn fetsorn requested a review from aymanbagabas June 17, 2024 14:17
@fetsorn
Copy link
Author

fetsorn commented Jul 2, 2024

@aymanbagabas What can I improve to bring this closer to merge?

Copy link
Member

@aymanbagabas aymanbagabas left a comment

Choose a reason for hiding this comment

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

Please use gofumpt to format your code 🙂

pkg/config/config.go Outdated Show resolved Hide resolved
pkg/config/config.go Outdated Show resolved Hide resolved
pkg/web/server.go Outdated Show resolved Hide resolved
@fetsorn fetsorn requested a review from aymanbagabas July 9, 2024 11:08
@aymanbagabas
Copy link
Member

Hey @fetsorn, could you rebase the PR and any lint issues? This will be released in v0.8.0

Copy link
Member

@aymanbagabas aymanbagabas left a comment

Choose a reason for hiding this comment

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

Would be nice to have tests for this, otherwise, LGTM

Comment on lines 195 to 197
fmt.Sprintf("SOFT_SERVE_HTTP_CORS_ALLOWED_HEADERS=%s", strings.Join(c.HTTP.CORS.AllowedHeaders, "\n")),
fmt.Sprintf("SOFT_SERVE_HTTP_CORS_ALLOWED_ORIGINS=%s", strings.Join(c.HTTP.CORS.AllowedOrigins, "\n")),
fmt.Sprintf("SOFT_SERVE_HTTP_CORS_ALLOWED_METHODS=%s", strings.Join(c.HTTP.CORS.AllowedMethods, "\n")),
Copy link
Member

Choose a reason for hiding this comment

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

caarlos0/env by default separate lists by commas

Suggested change
fmt.Sprintf("SOFT_SERVE_HTTP_CORS_ALLOWED_HEADERS=%s", strings.Join(c.HTTP.CORS.AllowedHeaders, "\n")),
fmt.Sprintf("SOFT_SERVE_HTTP_CORS_ALLOWED_ORIGINS=%s", strings.Join(c.HTTP.CORS.AllowedOrigins, "\n")),
fmt.Sprintf("SOFT_SERVE_HTTP_CORS_ALLOWED_METHODS=%s", strings.Join(c.HTTP.CORS.AllowedMethods, "\n")),
fmt.Sprintf("SOFT_SERVE_HTTP_CORS_ALLOWED_HEADERS=%s", strings.Join(c.HTTP.CORS.AllowedHeaders, ",")),
fmt.Sprintf("SOFT_SERVE_HTTP_CORS_ALLOWED_ORIGINS=%s", strings.Join(c.HTTP.CORS.AllowedOrigins, ",")),
fmt.Sprintf("SOFT_SERVE_HTTP_CORS_ALLOWED_METHODS=%s", strings.Join(c.HTTP.CORS.AllowedMethods, ",")),

Copy link

codecov bot commented Aug 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 53.05%. Comparing base (b06b555) to head (a040d05).
Report is 57 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #516      +/-   ##
==========================================
+ Coverage   51.96%   53.05%   +1.09%     
==========================================
  Files         157      159       +2     
  Lines       13454    11561    -1893     
==========================================
- Hits         6991     6134     -857     
+ Misses       5891     4859    -1032     
+ Partials      572      568       -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@aymanbagabas
Copy link
Member

@fetsorn I'm preparing the release notes for the next release, do you think you can add test cases for this feature to be included in the next release?

@fetsorn
Copy link
Author

fetsorn commented Sep 10, 2024

What tests cases do you imagine would be required? Should I add them to https://github.com/charmbracelet/soft-serve/blob/main/testscript/testdata/http.txtar?

@aymanbagabas
Copy link
Member

What tests cases do you imagine would be required? Should I add them to https://github.com/charmbracelet/soft-serve/blob/main/testscript/testdata/http.txtar?

Yes, or you could add them to a new http-cors.txtar file. You can use the testscript curl command to make requests in the txtar file.
Also, since this changes the config structure, we need to add config tests similar to https://github.com/charmbracelet/soft-serve/pull/557/files#diff-29cea1b5b831c8655c7155f43e2367cec73e66d1e338f8b3c7877a2f339b8811R60

@fetsorn
Copy link
Author

fetsorn commented Sep 11, 2024

What's the window of time until the release?

@aymanbagabas
Copy link
Member

What's the window of time until the release?

Ideally in the next couple of weeks

@aymanbagabas
Copy link
Member

@fetsorn I'm planning on pushing a new release soon. It would be nice to have this PR included in the next release 🙂

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.

2 participants