Skip to content
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

SumoLogic SDK fails to work #28

Closed
arafatmohammed opened this issue Apr 2, 2018 · 33 comments · Fixed by #73
Closed

SumoLogic SDK fails to work #28

arafatmohammed opened this issue Apr 2, 2018 · 33 comments · Fixed by #73

Comments

@arafatmohammed
Copy link

Hi,

We're using Sumologic SDK (https://github.com/SumoLogic/js-logging-sdk) and get the following error:

Failed to load https://endpoint1.collection.us2.sumologic.com/receiver/v1/http/ZaVnC4dhaV3...0SiTUGo7o9dxNm5 (index):1bJHzJr80EcmGJlbSlcdn1GYn8ooa==: 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:3000' is therefore not allowed access.

Please help us fix this.

@billsaysthis
Copy link
Contributor

Hi, can you post sample code that reproduces your issue? Maybe use one of the demo pages as a starting point.

@soederpop
Copy link

soederpop commented Apr 3, 2018

@arafatmohammed and I are working on this.

I'm creating an instance of the logger with the following function:

const SumoLogic = require('sumo-logger')

export function createLogger(options = {}) {
  options = {
    endpoint:
      'https://endpoint1.collection.us2.sumologic.com/receiver/v1/http/__SECRET_REMOVED__',
    interval: 10000, // Send messages in batches every 10 seconds
    sourceName: 'MyApp',
    sessionKey: 'MyUUID',
    hostName: 'localhost:3000',
    sourceCategory: `appName/web/local`,
   ...options
  }

  const sumo = new SumoLogic(options)

  if (typeof window !== 'undefined') {
    window.addEventListener('beforeunload', () => sumo.flushLogs())
  }

  return sumo
}

This code is bundled by Webpack. I experienced the same issue with the script tag.

We attempted to resolve The CORS error that I receive from the browser by configuring the hosted collector _sourceHost value, but it may be possible we didn't do this correctly? Is this something that we need to configure?

Ideally I'd like to provide a comma separated list of host names. I believe right now we added just "localhost:3000"

Appreciate any advice, thanks.

@soederpop
Copy link

FYI I found this:

function createCORSRequest(method, url) {
  var xhr = new XMLHttpRequest();
  if ("withCredentials" in xhr) {

    // Check if the XMLHttpRequest object has a "withCredentials" property.
    // "withCredentials" only exists on XMLHTTPRequest2 objects.
    xhr.open(method, url, true);

  } else if (typeof XDomainRequest != "undefined") {

    // Otherwise, check if XDomainRequest.
    // XDomainRequest only exists in IE, and is IE's way of making CORS requests.
    xhr = new XDomainRequest();
    xhr.open(method, url);

  } else {

    // Otherwise, CORS is not supported by the browser.
    xhr = null;

  }
  return xhr;
}

var xhr = createCORSRequest('GET', url);
if (!xhr) {
  throw new Error('CORS not supported');
}

I'm not sure how up to date this is, but it looks like the way this SDK creates its network request might fail in IE anyway.

@soederpop
Copy link

I tried hand crafting the request with xhr and using the axios module (which is super legit), and it still fails due to the lack of CORS headers being sent by the server response

This leads me to believe that there is a configuration problem with our http collector, and not with this SDK library. ( Although once we get it working I will need to confirm that IE works, if not I'm happy to submit a PR with a fix)

If you have any helpful documentation for us to be able to configure sumologic on our end to support CORS requests, I think that will fix our issue.

@billsaysthis
Copy link
Contributor

Hi @soederpop , thanks for jumping in. I don't have that documentation myself but will ask internally and get back to you. PRs are definitely welcome!

@latkin
Copy link

latkin commented Apr 3, 2018

The backend that powers HTTP sources is intended to be CORS-aware. If we can nail down a repro that shows a clear issue then certainly we can look into a change server-side.

HTTP source endpoints are intended to be open to requests from anywhere, so we allow all origins from CORS perspective. So whatever value the client populates in the request Origin header the server should echo back in the response Access-Control-Allow-Origin header.

Example simulating CORS preflight request:

curl 'https://endpoint1.collection.us2.sumologic.com/receiver/v1/http/<redacted>==' -v -X OPTIONS -H "Origin: foobar123" -H "Access-Control-Request-Method: POST"

// reply
< Access-Control-Allow-Credentials: true
< Access-Control-Allow-Headers: X-Requested-With,Content-Type,Accept,Origin,Content-Encoding,X-Sumo-Host,X-Sumo-Category,X-Sumo-Name,X-Sumo-Client,X-Sumo-Metadata,X-Sumo-Dimensions
< Access-Control-Allow-Methods: GET,POST,HEAD,OPTIONS
< Access-Control-Allow-Origin: foobar123
< Access-Control-Max-Age: 86400

Note that the client must populate the Origin header in order for the request to be considered for CORS. If Origin is omitted then the server won't set CORS headers in its response. Maybe you are hitting this?

@soederpop
Copy link

soederpop commented Apr 4, 2018

Thanks @billsaysthis @latkin for your replies.

The only mention I am able to find on Sumologic's website regarding CORS mentions the need to set the Origin header.

Chrome and Firefox ( at least ) automatically set the Origin header for you.

See https://stackoverflow.com/questions/15988323/cors-and-origin-header

In either case, I've confirmed that the Origin header is being sent in the preflight OPTIONS request.

I have seen this error reported before when there is a server side error, however the OPTIONS request returns a status 200, so I can't be sure that is what is happening.

screenshot 2018-04-03 23 20 16_preview

I've tried both POST and GET requests. Manually using xhr and the axios library, in addition to using this library. They all fail for the same reason.

I'm leaning toward this being a problem on the server side or a configuration option we're missing.

@soederpop
Copy link

FYI, it works fine from CURL, so I think we can rule out the server side / configuration scenario.

@soederpop
Copy link

soederpop commented Apr 4, 2018

I figured it out.

In my code which is bundled by webpack, it uses your node.js build instead of the browser build. Since the node.js build uses axios, and axios (despite being amazing in all other respects) has a very nasty bug in it:

See axios/axios#385

This bug causes axios default headers to be shared by all instances of axios (WTF?!)

So my requests that were being made using axios by your library were sending an authorization header, since my other client which uses axios sets that default header. My requests to the sumo logic endpoint were sending an authorization header and this was causing the request to "fail" on Sumologic's end, even though the preflight OPTIONS request returned 200.

chrome reports this as a CORS failure, because it assumed that a 200 response means it was normal, and the response didn't include the access control headers.

I configured webpack to use the lib/sumologic.logger.min.js file instead of lib/SumoLogger.js

now i'm getting logs, so we can close this issue.

@soederpop
Copy link

I'm still worried about the XHR usage in the browser build not being compatible with IE, but I assume if that was the case you would have stepped on it already. I'll do some further testing and report back.

I've created a PR which will configure your library to do the right thing for Webpack builds that target the browser.

Thanks for your help.

@clementallen
Copy link
Contributor

@soederpop @billsaysthis I've done a bit of looking into the axios issue and it looks like a PR to fix it is very close to being merged! I'll keep an eye on the PR and when it's merged and published I'll update Axios in the logger. Would be great for you to check then if it solves your problem @soederpop

axios/axios#1395

@billsaysthis
Copy link
Contributor

Sounds like a good plan to me, thanks @clementallen !

@billsaysthis
Copy link
Contributor

That PR got merged Tuesday, listed to be part of their 0.19 release (https://github.com/axios/axios/projects/3), which has several tickets still open.

@billsaysthis
Copy link
Contributor

Axios team not moving fast on this :(

@billsaysthis
Copy link
Contributor

Still not much movement.

@billsaysthis
Copy link
Contributor

0.19 is (finally) in beta, closing this as we can do no more.

@clementallen
Copy link
Contributor

@billsaysthis surely this should be kept open until 0.19 is released and updated in the sdk?

@billsaysthis
Copy link
Contributor

Fair enough

@billsaysthis billsaysthis reopened this Nov 7, 2018
@billsaysthis
Copy link
Contributor

Axios 0.19 still not GA 😢

@clementallen
Copy link
Contributor

Been no movement on the Axios repo for over four months now...

@billsaysthis
Copy link
Contributor

Here is the issue with single maintainer projects:

axios/axios#1965 (comment)

@OzairP
Copy link
Contributor

OzairP commented May 8, 2019

I think it may be worth just to bump axios to the beta version. Really can't wait this long for them to update their project.

@billsaysthis
Copy link
Contributor

I'm not sure. From reading the recent-ish posts they are now working on 1.0, with no news on 0.19 despite repeated requests for an update.

@clementallen what do you think?

@clementallen
Copy link
Contributor

I know how frustrating it is when changes are taking so long but in the name of stability I don't think we should be using any beta versions of libraries.

@OzairP
Copy link
Contributor

OzairP commented May 9, 2019

That is a fair response, however it has been more than a year since the Axios PR fixing the bug causing this issue was merged (axios/axios#1395). Axios 0.19.0 beta was release 10 months ago. I think switching HTTP libraries should be considered.

I haven't looked too much into the repository but what is the difficulty of swapping Axios out for another library?

There has recently been a vulnerability in Axios too so I am not very keen on using Axios myself.
https://snyk.io/blog/a-denial-of-service-vulnerability-discovered-in-the-axios-javascript-package-affecting-all-versions-of-the-popular-http-client/

@OzairP
Copy link
Contributor

OzairP commented May 9, 2019

Okay just took a look quickly, in at least SumoLogger.js there are only 2 instances of using Axios so it should be no effort to swap it out. What do you think about removing the Axios dependency entirely and just using HTTP Fetch? Availability wise, most consumers are using WebPack and Babel should automatically polyfill it.

I can make this contribution if this sounds good to the maintainers.

@billsaysthis
Copy link
Contributor

My main concern is support for IE11, otherwise I'm open to a PR. @clementallen what do you think?

@clementallen
Copy link
Contributor

As much as I hate IE11 we do still have to consider it unfortunately. Also, this has to work with Node which I don't believe has native fetch support?

@clementallen
Copy link
Contributor

Certainly up for moving on from Axios if we find a good solution

@OzairP
Copy link
Contributor

OzairP commented May 9, 2019

The most popular alternatives to Axios if you don't want to use Fetch is SuperAgent and Request.

Personally, I think SA is the best pick since it's API is the most closest to Axios. It supports IE11 out of the box as well, but IE9 & 10 with some polyfills.

@clementallen
Copy link
Contributor

Superagent gets my vote too, only slightly larger than Axios and seems to have all the features we need. Providing @billsaysthis is happy I think we should go ahead with migrating over

@billsaysthis
Copy link
Contributor

@OzairP if you want to send a PR I'm good with the switch.

@OzairP
Copy link
Contributor

OzairP commented May 13, 2019

@billsaysthis @clementallen I'm working on the PR, trying to make it backwards compatible however I do not think it is possible for users who rely on the promise wrapped Axios response that is returned by #log and #flushLogs.

All but 1 field in the Axios response object are easily mapped. The field config is a custom Axios config that is not compatible with SuperAgent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants