add cors headers

[fetsorn]

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

[aymanbagabas]

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

[fetsorn]

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

[aymanbagabas]

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]

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]

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

[fetsorn]

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]

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

[aymanbagabas]

Please use gofumpt to format your code 🙂

[aymanbagabas]

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

[aymanbagabas]

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

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 51.80%. Comparing base (b06b555) to head (dacc251).
Report is 122 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #516      +/-   ##
==========================================
- Coverage   51.96%   51.80%   -0.16%     
==========================================
  Files         157      159       +2     
  Lines       13454    13562     +108     
==========================================
+ Hits         6991     7026      +35     
- Misses       5891     5967      +76     
+ Partials      572      569       -3     

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

[aymanbagabas]

@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]

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]

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]

What's the window of time until the release?

[aymanbagabas]

What's the window of time until the release?

Ideally in the next couple of weeks

[aymanbagabas]

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

[fetsorn]

What test cases are missing do you think? @aymanbagabas

Currently there are tests for:

• parsing CORS headers, origins and methods from config or env
• requesting OPTIONS fails with default config
• requesting OPTIONS passes with a CORS config

[aymanbagabas]

Thank you @fetsorn for your amazing work here! We will push a new release soon