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

TypeError: Cannot read property 'headers' of undefined at getPaginated #142

Closed
rohan-deshpande opened this issue Aug 8, 2018 · 37 comments · Fixed by #160
Closed

TypeError: Cannot read property 'headers' of undefined at getPaginated #142

rohan-deshpande opened this issue Aug 8, 2018 · 37 comments · Fixed by #160

Comments

@rohan-deshpande
Copy link

rohan-deshpande commented Aug 8, 2018

Error is occurring at RequestHelper.js:116:44 when attempting the following

const gitlab = new GitLab({ url, token });

describe('projects', () => {
  it('should fetch projects as an array', done => {
    gitlab
      .Projects
      .all()
      .then(projects => {
        console.log(projects);
        done();
      })
      .catch(console.log);
  });
});

Note that I can access the projects via postman with

{{gitlab}}/api/v4/projects?private_token={{token}}
@jdalrymple
Copy link
Owner

hmm, ill look!

@jdalrymple
Copy link
Owner

What version of the package are you using? And do you have problems running any of the other functions?

@rohan-deshpande
Copy link
Author

rohan-deshpande commented Aug 8, 2018

Ah sorry, 3.7.0, nothing really seems to work, I keep getting this error

@jdalrymple
Copy link
Owner

Hmm thats really odd... Looking!

@jdalrymple
Copy link
Owner

Could you check if you get the same problem in an earlier version of the package?

@rohan-deshpande
Copy link
Author

rohan-deshpande commented Aug 8, 2018

Can do, let's just make sure this is not a user error first though, are you expecting the url prop passed to the constructor to contain the /api/v4 suffix or are you appending it? I wasn't really clear about this.

Edit: same issue in 3.6.0 this makes me think that it's something I'm doing incorrectly...

@jdalrymple
Copy link
Owner

I append it internally, the url just need to be the base url, like: https://gitlab.com

@rohan-deshpande
Copy link
Author

rohan-deshpande commented Aug 8, 2018

Heh, okay, it's a certificate issue. I'm behind a proxy. Logging the err inside that method yields

name: 'RequestError',
  message: 'Error: unable to get local issuer certificate',
  cause:
   { Error: unable to get local issuer certificate
       at TLSSocket.<anonymous> (_tls_wrap.js:1098:38)
       at emitNone (events.js:105:13)
       at TLSSocket.emit (events.js:207:7)
       at TLSSocket._finishInit (_tls_wrap.js:628:8)
       at TLSWrap.ssl.onhandshakedone (_tls_wrap.js:458:38) code: 'UNABLE_TO_GET_ISSUER_CERT_LOCALLY' },

Basically.

@jdalrymple
Copy link
Owner

😂 woo, glad we figured it out

@rohan-deshpande
Copy link
Author

rohan-deshpande commented Aug 8, 2018

Hmm wonder how I can get around it... bit weird that it doesn't happen when hitting the API directly

@jdalrymple
Copy link
Owner

hmm, could it be an issue with the Request module? Can you access the API directly through the Request module ?

@rohan-deshpande
Copy link
Author

Do you mean the RequestHelper class?

@jdalrymple
Copy link
Owner

Nope, i mean with the Request Library: https://www.npmjs.com/package/request

@rohan-deshpande
Copy link
Author

Ah right so you're using this to perform the requests, let me try.

@jdalrymple
Copy link
Owner

yup! It will help narrow down where the error is coming from

@rohan-deshpande
Copy link
Author

Okay yeah, happens there too. I can get around it by passing an option to the call like

const rejectUnauthorized = false;

request({ url, rejectUnauthorized });

Are all options passed to the constructor passed down to the request call? If so then passing this as an option should just work.

@jdalrymple
Copy link
Owner

No the options are not passed down :( If there another way around that problem?

@rohan-deshpande
Copy link
Author

rohan-deshpande commented Aug 9, 2018

I don't think so. It's a legit use case imo because in some corporate environments, non standard certs are used or certificates are not managed by the developers themselves making it hard to have any kind of control over them.

Given that GitLab focuses on self-hosting, I think this would be a good option to include. Let me know if you think so and I'll see if I can get a PR done.

@jdalrymple
Copy link
Owner

Hmm, i agree. How does this flag work with other request modules? For example we use the XMLHttpRequest module as well in this library ? (Eventually to be refactored out to support any request module the user would like #98

@rohan-deshpande
Copy link
Author

rohan-deshpande commented Aug 9, 2018

Any reason why you are using two libraries? Why not just use one?

@jdalrymple
Copy link
Owner

I started with the Request module because it was one of the most used/well tested request modules i could find. The XML request module was added after to support xhr request for the browser. A better library to upgrade to may be cross-fetch, which handles all of these cases. Before switching the main request library around, id like to do more research to at least try and maintain the same API if possible instead of having to update everything

@rohan-deshpande
Copy link
Author

Ah okay makes sense. I think node-fetch might be the best option then because the implementation is identical to the browser spec.

@jdalrymple
Copy link
Owner

Meh, this library was build specifically for node, so whether it maintains adherence to the browser spec is less important. As long as it is consistent s'all good. Another one another contributor suggested was axios?

@rohan-deshpande
Copy link
Author

rohan-deshpande commented Aug 9, 2018

I wouldn't use Axios personally. Fetch is the way forward.

Get what you're saying about it being node specific, but supporting browsers is also nice because it means people can use your lib to create some cool UIs.

@MartinBenninger
Copy link
Contributor

I think this is actually an issue with the node-gitlab library and not just the request package.

We use this library in the browser with the XMLHttpRequester and we get the same error whenever a request fails.

TypeError: Cannot read property 'headers' of undefined

This happens for example when making a request to a repository that the user does not have access to. Previously the real HTTP error was passed to the error function.

I think that it's coming from the bit of code that tries to get the sleep time from the 'retry-after' header.

const sleepTime = parseInt(err.response.headers['retry-after'], 10);

Let me know if you need any more information. I can also try to make a small sample to reproduce the error.

@jdalrymple
Copy link
Owner

jdalrymple commented Aug 9, 2018 via email

@jetersen
Copy link
Contributor

jetersen commented Aug 11, 2018

@rohan-deshpande and @MartinBenninger even still you should be able to include the extra certs into NodeJS this is how it should be solved you don't want to start down the dark path of ignore certs.

Added this to our package.json and the pem includes all our internal certs 😄 Has been working great for us.

"scripts": {
    "start": "NODE_EXTRA_CA_CERTS=./secrets/3ShapeCA.pem node bot.js"
},

https://nodejs.org/api/cli.html#cli_node_extra_ca_certs_file

@jdalrymple
Copy link
Owner

There are two problems in this issue lol 1. Certificates but @Casz has pointed out how to handle that, and 2. The sleep propagating header not found errors. This second one im working on now!

@jdalrymple
Copy link
Owner

Pushed up a fix for the headers

@rohan-deshpande
Copy link
Author

I don’t think the suggested solution works if your certificates are borked. Which mine are, and I have no control over them. It’s not a viable solution for everyone where as passing the request option is.

I’ve fallen back to just hitting the API directly with request for now and it’s all working fine.

@jdalrymple
Copy link
Owner

Hmm, I guess so eh. I'll add the extra param then to the init logic.

@jdalrymple jdalrymple reopened this Aug 15, 2018
jdalrymple pushed a commit that referenced this issue Aug 15, 2018
# [3.9.0](3.8.0...3.9.0) (2018-08-15)

### Bug Fixes

* Fix error while throwing an error in RequestHelper ([#156](#156)) ([177d7fd](177d7fd))
* Handling errors before retrying request ([#142](#142)) [skip-ci] ([bc3b366](bc3b366))
* Linting Master ([#157](#157)) ([ab14ed7](ab14ed7))

### Features

* Add deploy keys enable functionality ([#155](#155)) thanks to [Michael Matzka](https://github.com/mimaidms ) ([66547ad](66547ad))
@jetersen
Copy link
Contributor

jetersen commented Aug 15, 2018

@rohan-deshpande you really ought to handle certs in the way that I suggested above. This other way is just an excuse.
All you have to do is take your company's public root CA, you can grab this one through Chrome, no need to contact your IT department and put it into the pem file.

If you want to include more you simply add them all into one pem file, there is really no excuse to not doing it.

image
image
Save it as DER x509

Convert x509 to PEM

You can also run: openssl s_client -connect HOST:PORT -showcerts to get the certificate chain and in there you should be able to find the public root CA PEM format in your chain

@rohan-deshpande
Copy link
Author

Of course I’ve already tried this. The certs are not actually valid. That’s the problem. I don’t think it’s helpful to assume that everything is as it should be in the myriad of corporate mess that others have to deal with.

@jdalrymple
Copy link
Owner

Although i agree with @Casz , I see your point. If the user of the library is ok with ignoring certs that's their business.

@rohan-deshpande
Copy link
Author

I agree 100% in principle as well. It’s just that these things aren’t always as they should be.

jdalrymple pushed a commit that referenced this issue Aug 15, 2018
Allows to be set service wide or on individual request
fixes #142
jdalrymple pushed a commit that referenced this issue Aug 15, 2018
# [3.10.0](3.9.0...3.10.0) (2018-08-15)

### Features

* Expose reject unauthorized in request helper ([#160](#160)) ([01a2ce2](01a2ce2)), closes [#142](#142)
@jdalrymple
Copy link
Owner

🎉 This issue has been resolved in version 3.10.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@rohan-deshpande
Copy link
Author

Nice one, thanks.

mdsb100 pushed a commit to mdsb100/node-gitlab that referenced this issue Aug 17, 2018
# [3.9.0](jdalrymple/gitbeaker@3.8.0...3.9.0) (2018-08-15)

### Bug Fixes

* Fix error while throwing an error in RequestHelper ([jdalrymple#156](jdalrymple#156)) ([177d7fd](jdalrymple@177d7fd))
* Handling errors before retrying request ([jdalrymple#142](jdalrymple#142)) [skip-ci] ([bc3b366](jdalrymple@bc3b366))
* Linting Master ([jdalrymple#157](jdalrymple#157)) ([ab14ed7](jdalrymple@ab14ed7))

### Features

* Add deploy keys enable functionality ([jdalrymple#155](jdalrymple#155)) thanks to [Michael Matzka](https://github.com/mimaidms ) ([66547ad](jdalrymple@66547ad))
mdsb100 pushed a commit to mdsb100/node-gitlab that referenced this issue Aug 17, 2018
Allows to be set service wide or on individual request
fixes jdalrymple#142
mdsb100 pushed a commit to mdsb100/node-gitlab that referenced this issue Aug 17, 2018
# [3.10.0](jdalrymple/gitbeaker@3.9.0...3.10.0) (2018-08-15)

### Features

* Expose reject unauthorized in request helper ([jdalrymple#160](jdalrymple#160)) ([01a2ce2](jdalrymple@01a2ce2)), closes [jdalrymple#142](jdalrymple#142)
EvHaus pushed a commit to EvHaus/node-gitlab-api that referenced this issue Jan 29, 2019
After upgrading my version of `node-gitlab` I noticed that I the API broke as result of these error messages: `Error: self signed certificate`. I was really confused why this was happening even though I had `process.env.NODE_TLS_REJECT_UNAUTHORIZED = '0';` set in my file.

Turns out this work jdalrymple#142 added a special flag to Gitlab's BaseModel to specifically handle rejections of certificates.

This PR adds documentation for it so that others running into this issue won't waste their time going down the same rabbit hole I went on.
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.

4 participants