[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