New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CORS not working with Client4 #557

Closed
iampeter opened this Issue Jul 3, 2018 · 16 comments

Comments

Projects
None yet
2 participants
@iampeter
Copy link

iampeter commented Jul 3, 2018

Summary

CORS seem to fail with Client4 although mattermost-server has AllowCorsFrom set to "*"

Environment Information

  • Chrome 65
  • Webapp
  • Mattermost Server 5.0.0

Steps to reproduce

Try to call Client4.getMe()

Expected behavior

Getting the results from the server.

Observed behavior

Chrome says: Failed to load http://localhost:8065/api/v4/users/me: Response to preflight request doesn't pass access control check: The value of the 'Access-Control-Allow-Credentials' header in the response is '' which must be 'true' when the request's credentials mode is 'include'. Origin 'http://localhost:8081' is therefore not allowed access.

A simple test posted here:
mattermost/mattermost-server#8687
using fetch (fetch('http://localhost:8065/api/v4/system/ping')) works.

@lieut-data

This comment has been minimized.

Copy link
Member

lieut-data commented Jul 3, 2018

I can reproduce this:

> git clone https://github.com/mattermost/mattermost-redux
> pushd mattermost-redux
> npm install babel
> make bundle
> popd
> npm install babel-polyfill

At this point, I created an index.html locally as a sibling to the mattermost-redux folder, using the ping API for consistency with the other example:

<!DOCTYPE html>
<html>
    <head>
        <title>Title</title>
        <script src="node_modules/babel-polyfill/dist/polyfill.js"></script>
        <script src="mattermost-redux/lib/mattermost.client4.js"></script>
    </head>
    <body>
        <script>
            const test = async () => {
                const client = new Mattermost.client4.default();
                client.setUrl('http://localhost:8065/');
                const response = await client.ping()
                console.log(response);
            };

            try {
                test();
            } catch (e) {
                console.log(e);
            }
        </script>
    </body>
</html>

and got:

Failed to load http://localhost:8065//api/v4/system/ping?time=1530652466421: Response to preflight request doesn't pass access control check: The value of the 'Access-Control-Allow-Credentials' header in the response is '' which must be 'true' when the request's credentials mode is 'include'. Origin 'null' is therefore not allowed access.

It turns out that mattermost-server isn't actually returning * when AllowCorsFrom is set to same, but rather looks for an Origin header and responds with that instead. Normally, the browser sends the Origin header automatically, but it looks like it's empty for file:// assets. The server in turn, having no value for the header, just returns an empty string and Chrome rejects the actual API call after the pre-flight OPTIONS check fails.

What I can't actually explain, now, is why the bare fetch call in the previous test worked at all, since it seems like CORS isn't even applying in that case.

@lieut-data

This comment has been minimized.

Copy link
Member

lieut-data commented Jul 3, 2018

Oh, I see, it's because mattermost-redux passes credentials: 'include' to the fetch call that triggers the CORS check. I can reproduce the issue on the original test case if I do same.

@lieut-data

This comment has been minimized.

Copy link
Member

lieut-data commented Jul 3, 2018

It looks like the server is correct in /not/ sending *, since according to discussions and the wikipedia article on this:

The value of "*" is special in that it does not allow requests to supply credentials, meaning it does not allow HTTP authentication, client-side SSL certificates, or cookies to be sent in the cross-domain request.[8]

So it wouldn't work anyway. Given that the Origin header can only be set by the user-agent, it may simply not be possible to work around this for file:// assets.

You'll probably need to front your local assets behind /something/ serving them up on localhost, e.g. as rudimentary as described:

> cd ~/files
> php -S localhost:8000
@iampeter

This comment has been minimized.

Copy link
Author

iampeter commented Jul 4, 2018

@lieut-data I am not serving as file://, everything is served via http

@iampeter

This comment has been minimized.

Copy link
Author

iampeter commented Jul 4, 2018

@lieut-data I have changed the setting from "*" to "localhost:8081" (calling app), and now I'm getting:

Failed to load http://localhost:8065/api/v4/users/me: Response to preflight request doesn't pass access control check: 
No 'Access-Control-Allow-Origin' header is present on the requested resource. 
Origin 'http://localhost:8081' is therefore not allowed access. 
If an opaque response serves your needs, set the request's mode to 'no-cors' to fetch the resource with CORS disabled.
@lieut-data

This comment has been minimized.

Copy link
Member

lieut-data commented Jul 4, 2018

@iampeter, hmm. With the setting changed back to *, can you capture the headers emitted in the OPTIONS request from your browser? I'm specifically interested in the value of the Origin header emitted by your user agent.

@iampeter

This comment has been minimized.

Copy link
Author

iampeter commented Jul 4, 2018

@lieut-data here you go:

OPTIONS /api/v4/users/me HTTP/1.1
Host: localhost:8065
Connection: keep-alive
Pragma: no-cache
Cache-Control: no-cache
Access-Control-Request-Method: GET
Origin: http://localhost:8081
User-Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/65.0.3325.162 Safari/537.36
Access-Control-Request-Headers: authorization,x-requested-with
Accept: */*
Referer: http://localhost:8081/suggest
Accept-Encoding: gzip, deflate, br
Accept-Language: pl-PL,pl;q=0.9,en-US;q=0.8,en;q=0.7
@lieut-data

This comment has been minimized.

Copy link
Member

lieut-data commented Jul 4, 2018

Thanks for your patience, @iampeter -- indeed, the server is omitting the required Access-Control-Allow-Credentials response in this case. It looks like I got lost in unrelated details to miss the key part of your original report ;P

We'll get this one fixed!

@lieut-data

This comment has been minimized.

Copy link
Member

lieut-data commented Jul 5, 2018

I'm tracking the requisite server investigation and fix here.

@iampeter

This comment has been minimized.

Copy link
Author

iampeter commented Jul 5, 2018

@lieut-data thanks, and I am also wondering how soon do you plan to take to this, because it is sort of urgent for me :)

@lieut-data

This comment has been minimized.

Copy link
Member

lieut-data commented Jul 5, 2018

I'd imagine the soonest we can evaluate a fix for this would be Mattermost 5.2, scheduled for August 16th.

Are you running against a stock Mattermost install or a development instance? It's easy enough to make the change locally if you're iterating on development -- let me know if I can help with that. I've marked this as a security ticket (not in a bug sense but a feature sense), and hope to have the team make a decision on the best default behaviour for all customers going forward.

@iampeter

This comment has been minimized.

Copy link
Author

iampeter commented Jul 5, 2018

@lieut-data I'm running a stock one, but can switch to a custom build.

@iampeter

This comment has been minimized.

Copy link
Author

iampeter commented Jul 6, 2018

@lieut-data is there a proper way to make build a team edition without docker?

getting lots of errors.

@iampeter

This comment has been minimized.

Copy link
Author

iampeter commented Jul 6, 2018

@lieut-data ok building release-5.0 worked fine for me, it was only master that caused errors

@lieut-data

This comment has been minimized.

Copy link
Member

lieut-data commented Jul 7, 2018

@iampeter, cool! Do you need help making the specific changes, or have you already figured that part out?

@iampeter

This comment has been minimized.

Copy link
Author

iampeter commented Jul 8, 2018

@lieut-data thanks, I added it myself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment