fix: add CORS to config, improve test

[fetsorn]

Hotfix for #516, I've corrected the test case and added CORS to the default config.

• prepend c.HTTP.PublicURL to c.HTTP.CORS.AllowedOrigins in the Config.Validate() function.
• make the testscript case more readable
• hint a custom allowed origin in the config
• enable PUT and OPTIONS in default config

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 51.82%. Comparing base (b06b555) to head (237217d).
Report is 122 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #654      +/-   ##
==========================================
- Coverage   51.96%   51.82%   -0.14%     
==========================================
  Files         157      159       +2     
  Lines       13454    13567     +113     
==========================================
+ Hits         6991     7031      +40     
- Misses       5891     5967      +76     
+ Partials      572      569       -3     

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

[aymanbagabas]

We don't need an empty string, an empty list should have the same meaning, no?

[fetsorn]

in gorilla by default an empty list equals to "*"
https://github.com/gorilla/handlers/blob/9c61bd81e701cf500437e1b516b675cdd3b73ca7/cors.go#L117

[aymanbagabas]

We should always prepend the public http origin to the list so that it has at least one item

[fetsorn]

AFAIK before this PR even the public url was not an allowed origin. So I set "" and do not allow any origins to match the original behaviour

Do you think we should make the default less safe but more convenient? If so, I can prepend c.HTTP.PublicURL to c.HTTP.CORS.AllowedOrigins in the Config.Validate() function.

[aymanbagabas]

Right, the Soft Serve public URL would usually be the origin where we expect requests to come from. IMO, it should always be included in the list of allowed origins or be defined in an allowed origins validator.

[aymanbagabas]

Shouldn't an empty slice have the same meaning?

Suggested change

	AllowedOrigins: []string{""},
	AllowedOrigins: []string{},

[fetsorn]

An empty slice fallsback to "*" in gorilla

[aymanbagabas]

What's the behavior of using Soft Serve without a CORS middleware?

[fetsorn]

afaik before #516 added cors middleware all cors/browser requests failed because soft-serve never returned any cors headers

[aymanbagabas]

We don't need an empty string, an empty list should have the same meaning, no?

[aymanbagabas]

Suggested change

	- "" # do not allow any origins
	# - * # Allow all origins don't do this unless you know what you're doing ;)

[fetsorn]

I agree with gorilla docs that returning * is unsafe and not required in any case
https://github.com/gorilla/handlers/blob/9c61bd81e701cf500437e1b516b675cdd3b73ca7/cors.go#L122

So I do not suggest "*" to avoid giving users poor advice.

Do you think it is better to offer a specific example here, like "https://website.com"?

[aymanbagabas]

This case should fail because the origin is not the same i.e. not localhost

[fetsorn]

I believe this case is correct

localhost is not the origin, localhost is the endpoint of the server to which we send a request with an "Origin: https://foo.example" header.
Origin is the value of the Origin header. usually origin is the value of browser's URL bar. Here origin is https://foo.example.

In the first test case, the server by default does not allow any origins, so the response does not have the "Access-Control-Allow-Origin" header.
In the second case the server allows the origin, and replies to OPTIONS with an "Access-Control-Allow-Origin" header. Usually after this the browser checks that the server response contains "Access-Control-Allow-Origin" and proceeds with the POST response.

What can make this test more readable and intuitive in your opinion?

[aymanbagabas]

We also need to allow PUT because LFS uses them. Don't we also need OPTIONS too?

[fetsorn]

I made sure that default behaviour stays the same, by default without cors middleware gorilla only allowed get, head and post. https://github.com/gorilla/handlers/blob/9c61bd81e701cf500437e1b516b675cdd3b73ca7/cors.go#L30

Options is always allowed in gorilla, as an exception. https://github.com/gorilla/handlers/blob/9c61bd81e701cf500437e1b516b675cdd3b73ca7/handlers.go#L19

OK, I'll enable PUT and OPTIONS in default config