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

[eugenioenko]

Is there an existing issue for this?

• [x] 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 https://github.com/gorilla/csrf/blob/9dd6af1f6d30fc79fb0d972394deebdabad6b5eb/helpers.go#L157-L158

The handler for the CSRF check is using this function to compare r.URL vs r.Header.Get("Origin") here https://github.com/gorilla/csrf/blob/9dd6af1f6d30fc79fb0d972394deebdabad6b5eb/csrf.go#L288-L289

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

https://github.com/gorilla/csrf/blob/9dd6af1f6d30fc79fb0d972394deebdabad6b5eb/csrf.go#L271-L272

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 9dd6af1f6d30fc79fb0d972394deebdabad6b5eb, 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 https://github.com/gorilla/csrf/issues/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.