[BUG] v.1.7.3 sameOrigin check issue for localhost over http

[eugenioenko]

Is there an existing issue for this?

• I have searched the existing issues

The issue

It seems like opt.TrustedOrigins is being required to set even though the request does come from the same origin

Current Behavior

The function sameOrigin is comparing a.Scheme == b.Scheme and a.Host == b.Host

csrf/helpers.go

Lines 157 to 158 in 9dd6af1

	func sameOrigin(a, b *url.URL) bool {
	return (a.Scheme == b.Scheme && a.Host == b.Host)

The handler for the CSRF check is using this function to compare r.URL vs r.Header.Get("Origin") here

csrf/csrf.go

Lines 288 to 289 in 9dd6af1

	if !sameOrigin(&requestURL, parsedOrigin) && !slices.Contains(cs.opts.TrustedOrigins, parsedOrigin.Host) {
	r = envError(r, ErrBadOrigin)

The issue is that requestURL.Schema is set to https even when request origin is http because isPlainText in local environment is false

csrf/csrf.go

Lines 271 to 272 in 9dd6af1

	requestURL.Scheme = "https"
	if isPlaintext {

The current fix is to add localhost:8080 as opt.TrustedOrigins but in this case the origin is the same, it shouldn't be required.

Expected Behavior

Requests from the same origin (host + scheme) should not require manually adding entries to opt.TrustedOrigins.

Steps To Reproduce

No response

Anything else?

Solutions seems to be to update the logic to correctly detect plaintext (http) requests in local/dev environments or improve how isPlaintext is set/detected by default.

[FiloSottile]

I believe this was intentional in 9dd6af1, because there is no reliable way to tell if a request came in over HTTPS or HTTP, it assumes it's HTTPS to fail closed unless specified with PlaintextHTTPRequest.

[cirelli94]

I'm sorry if I hijack the topic, but couldn't PlaintextHTTPRequest be an option or perhaps a ready-made middleware to be used, to solve this problem more easily?

[joncalhoun]

I believe this was intentional in 9dd6af1, because there is no reliable way to tell if a request came in over HTTPS or HTTP, it assumes it's HTTPS to fail closed unless specified with PlaintextHTTPRequest.

If this was intentional, wouldn’t this be a breaking change that justifies a version bump?

[cirelli94]

I believe this was intentional in 9dd6af1, because there is no reliable way to tell if a request came in over HTTPS or HTTP, it assumes it's HTTPS to fail closed unless specified with PlaintextHTTPRequest.

If this was intentional, wouldn’t this be a breaking change that justifies a version bump?

Yes, it would justify a version bump, see #186

[PotatoesFall]

It would also justify updating documentation, as the examples in README.md don't include this information.

For example

// PS: Don't forget to pass csrf.Secure(false) if you're developing locally
// over plain HTTP (just don't leave it on in production).

made me think that csrf.Secure(false) was all I needed to try this locally. (I'm here after chasing down the issue for half an hour)

Adding this option (only for dev builds) fixed the issue of course:

csrf.TrustedOrigins([]string{"localhost:8080"})

however that seems weird and clunky, first because I need two options for local development, second because I need to template the port in the string, thirdly because now I can no longer use 127.0.0.1 or serve my dev version to another machine on my network. What is the intended workaround? Adding a middleware that wraps all requests as PlaintextHTTPRequest ?

Personally I would suggest that before the sameOrigin check, the scheme is not forced to "https" if the Secure(false) option has been set. Just my humble opinion as a first-time user of the package, it's possible I misunderstood something.