Skip to content

DTFPCHG-178: Fixing snyk issue #19

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.

Sign up for GitHub

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

Jump to bottom
Open
pannepu wants to merge 1 commit into
base: main
Choose a base branch
from
Open

DTFPCHG-178: Fixing snyk issue #19

pannepu wants to merge 1 commit into from

Conversation

@pannepu
Copy link

@pannepu pannepu commented May 22, 2023 •
edited
Loading

Fixing snyk issue

JIRA ID | Git Issue

DTFPCHG-178

Change Type

  • Bug fix (is this a backward compatible change?)
  • New feature (is this a backward compatible change?)
  • Breaking change (is this a non-backward compatible change?)
  • DB changes (add scripts in the sql/migrations/{date} folder)

Change Impact

Summary

  • The SAST tool [Snyk] has identified security risks with code snippet in this repository.
  • http.request method is being used in lib/api-requestor.js file which is the root cause for this issue.
  • We are mainly using this for local testing since we do not support http in the Prod environment.
  • Removed the support for making the request using http protocol.
  • At any point in time if we want to continue testing in our local, we can go ahead and add this code snippet.

Demo

image
image
image

Notes

  • Updated the lib/api-requestor.js file to remove the support for http protocol.

Testing Instructions

  • Try to submit a dispute in our local environment using http protocol. Configure the required properties in the lib/index.js
  • Observe that we would be able to submit the test dispute as long as http protocol is supported in our SDK.
  • Now go ahead and remove the http protocol support from lib/api-requestor.js file and try submitting a test dispute.
  • Observe that the request would fail with the above mentioned error (in the screenshot).

@jagadeeshdeepak-pp
Copy link

jagadeeshdeepak-pp commented May 22, 2023

@pannepu can you document this and attach it to the setup page here?
https://engineering.paypalcorp.com/confluence/display/RIAC/CH+-+Local+Setup

jagadeeshdeepak-pp
jagadeeshdeepak-pp reviewed May 22, 2023
View reviewed changes
lib/api-requestor.js
if (this._chargehound.options.protocol === 'https://') {
req = https.request(reqOpts).setTimeout(this.getTimeout())
connectEv = 'secureConnect'
} else {
Copy link

@jagadeeshdeepak-pp jagadeeshdeepak-pp May 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we not see if the env is local or not and then use the protocol based on that?

Copy link
Author

@pannepu pannepu May 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@pannepu
Copy link
Author

pannepu commented May 23, 2023

@pannepu can you document this and attach it to the setup page here?
https://engineering.paypalcorp.com/confluence/display/RIAC/CH+-+Local+Setup

Done.
Document Link: https://engineering.paypalcorp.com/confluence/display/RIAC/Snyk+Issue+in+chargehound-node+project

aggarwalvinay
aggarwalvinay reviewed May 23, 2023
View reviewed changes
lib/api-requestor.js Outdated
req = https.request(reqOpts).setTimeout(this.getTimeout())
connectEv = 'secureConnect'
} else {
} else if ((this._chargehound.options.protocol === 'http://') && (this._chargehound.options.host === 'localhost')) {
Copy link

@aggarwalvinay aggarwalvinay May 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

python dont have else if. please change it to elif

Copy link

@aggarwalvinay aggarwalvinay May 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with deepak, can we check current values of protocol for all environment (local, stage, prod) and use it accordingly here instead of relying on protocol value

Copy link
Author

@pannepu pannepu May 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

python dont have else if. please change it to elif

These changes are done in javascript file.

aggarwalvinay
aggarwalvinay reviewed May 23, 2023
View reviewed changes
lib/index.js Outdated
const CHARGEHOUND_HOST = 'api.chargehound.com'
const CHARGEHOUND_BASE_PATH = '/v1/'
const CHARGEHOUND_TIMEOUT = 60 * 1000
const CHARGEHOUND_ENV = 'prod'
Copy link

@aggarwalvinay aggarwalvinay May 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const CHARGEHOUND_ENV = 'prod'
const CHARGEHOUND_DEFAULT_ENV = 'prod'

@pannepu
Fixing snyk issue
ce7c12e 
Fixing snyk issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ansonipaypal ansonipaypal Awaiting requested review from ansonipaypal

@yusharma-pp yusharma-pp Awaiting requested review from yusharma-pp

2 more reviewers

@aggarwalvinay aggarwalvinay aggarwalvinay approved these changes

@jagadeeshdeepak-pp jagadeeshdeepak-pp jagadeeshdeepak-pp approved these changes

Reviewers whose approvals may not affect merge requirements

At least 2 approving reviews are required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@pannepu @jagadeeshdeepak-pp @aggarwalvinay