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

feat(cli): Secret scan #438

Merged
merged 20 commits into from
Sep 17, 2024
Merged

Conversation

Sambit003
Copy link
Contributor

Description

Give a summary of the change that you have made

Fixes #[ISSUENO]

Dependencies

Mention any dependencies/packages used

Future Improvements

Mention any improvements to be done in future related to any file/feature

Mentions

Mention and tag the people

Screenshots of relevant screens

Add screenshots of relevant screens

Developer's checklist

  • My PR follows the style guidelines of this project
  • I have performed a self-check on my work

If changes are made in the code:

  • I have followed the coding guidelines
  • My changes in code generate no new warnings
  • My changes are breaking another fix/feature of the project
  • I have added test cases to show that my feature works
  • I have added relevant screenshots in my PR
  • There are no UI/UX issues

Documentation Update

  • This PR requires an update to the documentation at docs.keyshade.xyz
  • I have made the necessary updates to the documentation, or no documentation changes are required.

Copy link
Contributor

PR Reviewer Guide 🔍

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Key issues to review

Possible Security Issue
The JWT regex may be too permissive and could potentially match non-JWT strings. Consider tightening the regex pattern to more strictly match valid JWT formats.

Performance Concern
The Cloudflare regex is commented out due to conflicts with other rules. This needs to be resolved to ensure Cloudflare tokens can be properly detected.

Code Smell
The scan command uses a lot of console.log statements for output. Consider using a logging library or creating a dedicated output handler for better maintainability.

Copy link
Contributor

PR Code Suggestions ✨

CategorySuggestion                                                                                                                                    Score
Enhancement
Improve the specificity of the Grafana API Key regex pattern

Consider using a more specific regex pattern for the Grafana API Key. The current
pattern might match some non-Grafana keys. A more specific pattern could include a
fixed prefix or a specific character count.

packages/secret-scan/src/rules/grafana.ts [6]

-/eyJrIjoi[A-Za-z0-9]{70,400}={0,2}/,  		   // Grafana API Key regex
+/eyJrIjoiGRA[A-Za-z0-9]{70,400}={0,2}/,  		   // Grafana API Key regex
 
  • Apply this suggestion
Suggestion importance[1-10]: 7

Why:

7
Implement a dynamic approach to automatically test all available secret detection rules

Instead of manually calling testSecret for each rule, consider using a dynamic
approach to automatically test all available rules. This would make it easier to add
new rules without modifying the test file.

packages/secret-scan/src/test/secret.test.ts [311-316]

-it(testcaseTitleTemplate('Grafana Key'), () => {
-  testSecret(grafana.testcases)
-});
-it(testcaseTitleTemplate('Authress Key'), () => {
-  testSecret(authress.testcases)
+const rules = { grafana, authress /* ... other rules ... */ };
+Object.entries(rules).forEach(([ruleName, rule]) => {
+  it(testcaseTitleTemplate(`${ruleName} Key`), () => {
+    testSecret(rule.testcases);
+  });
 });
 
  • Apply this suggestion
Suggestion importance[1-10]: 7

Why:

7
Add word boundaries to the Facebook Access Token regex for more precise matching

Consider using a more specific regex pattern for the Facebook Access Token. The
current pattern might match unintended strings due to its broad nature.

packages/secret-scan/src/rules/facebook.ts [6]

-/\d{15,16}([|%])[0-9a-z\-_]{27,40}/i,  		// Facebook Access Token regex
+/\b\d{15,16}([|%])[0-9a-z\-_]{27,40}\b/i,  		// Facebook Access Token regex
 
  • Apply this suggestion
Suggestion importance[1-10]: 7

Why:

7
Maintainability
Break down the complex regex pattern into more manageable parts

The regex pattern for Authress API keys is quite complex and might be hard to
maintain. Consider breaking it down into smaller, more manageable parts using named
capture groups or separate regex patterns for different parts of the key.

packages/secret-scan/src/rules/authress.ts [11]

-/(?:sc|ext|scauth|authress)_[a-z0-9]{5,30}\.[a-z0-9]{4,6}\.acc[_-][a-z0-9-]{10,32}\.[a-z0-9+/_=-]{30,120}/i
+const prefix = /(?:sc|ext|scauth|authress)_/;
+const middle = /[a-z0-9]{5,30}\.[a-z0-9]{4,6}\.acc[_-][a-z0-9-]{10,32}\./;
+const suffix = /[a-z0-9+/_=-]{30,120}/;
+return [new RegExp(`${prefix.source}${middle.source}${suffix.source}`, 'i')];
 
  • Apply this suggestion
Suggestion importance[1-10]: 7

Why:

7
Refactor the denylist object creation for better maintainability

Consider using a more maintainable approach for defining the denylist object, such
as grouping related items or using a factory function.

packages/secret-scan/src/denylist.ts [75-215]

-const denylist: SecretConfig = {
-  private_key: private_key(),
-  openAI: openAI(),
-  pypi: pypi(),
-  sendgrid: sendgrid(),
-  square_OAuth: square_OAuth(),
-  stripe: stripe(),
-  telegram_token: telegram_token(),
-  twilio: twilio(),
-  npm: npm(),
-  mailchimp: mailchimp(),
-  jwt: jwt(),
-  ip_public: ip_public(),
-  github: github(),
-  discord: discord(),
-  artifactory: artifactory(),
-  aws: aws(),
-  algolia: algolia(),
-  alibaba: alibaba(),
-  adafruit: adafruit(),
-  adobe: adobe(),
-  age: age(),
-  airtable: airtable(),
-  asana: asana(),
-  atlassian: atlassian(),
-  authress: authress(),
-  beamer: beamer(),
-  bitbucket: bitbucket(),
-  bittrex: bittrex(),
-  clojars: clojars(),
-  codecov: codecov(),
-  coinbase: coinbase(),
-  confluent: confluent(),
-  contentful: contentful(),
-  databricks: databricks(),
-  datadog: datadog(),
-  definednetworking: definednetworking(),
-  digitalocean: digitalocean(),
-  doppler: doppler(),
-  dropbox: dropbox(),
-  duffel: duffel(),
-  dynatrace: dynatrace(),
-  easypost: easypost(),
-  facebook: facebook(),
-  flutterwave: flutterwave(),
-  frameio: frameio(),
-  gitlab: gitlab(),
-  grafana: grafana(),
-  harness: harness(),
-  hashicorp: hashicorp(),
-  heroku: heroku(),
-  hubspot: hubspot(),
-  huggingface: huggingface(),
-  infracost: infracost(),
-  intra42: intra42(),
-  linear: linear(),
-  lob: lob(),
-  planetscale: planetscale(),
-  postman: postman(),
-  prefect: prefect(),
-  pulumi: pulumi(),
-  readme: readme(),
-  rubygems: rubygems(),
-  scalingo: scalingo(),
-  sendinblue: sendinblue(),
-  shippo: shippo(),
-  shopify: shopify(),
-  sidekiq: sidekiq()
-}
+const createDenylist = (): SecretConfig => {
+  const services = {
+    private_key, openAI, pypi, sendgrid, square_OAuth, stripe, telegram_token,
+    twilio, npm, mailchimp, jwt, ip_public, github, discord, artifactory, aws,
+    algolia, alibaba, adafruit, adobe, age, airtable, asana, atlassian, authress,
+    beamer, bitbucket, bittrex, clojars, codecov, coinbase, confluent, contentful,
+    databricks, datadog, definednetworking, digitalocean, doppler, dropbox, duffel,
+    dynatrace, easypost, facebook, flutterwave, frameio, gitlab, grafana, harness,
+    hashicorp, heroku, hubspot, huggingface, infracost, intra42, linear, lob,
+    planetscale, postman, prefect, pulumi, readme, rubygems, scalingo, sendinblue,
+    shippo, shopify, sidekiq
+  };
 
+  return Object.entries(services).reduce((acc, [key, value]) => {
+    acc[key] = value();
+    return acc;
+  }, {} as SecretConfig);
+};
+
+const denylist: SecretConfig = createDenylist();
+
  • Apply this suggestion
Suggestion importance[1-10]: 7

Why:

7
Simplify the complex IP address regex by breaking it down into smaller parts

The current regex for public IP addresses is complex and might be hard to maintain.
Consider simplifying it or breaking it down into smaller, more manageable parts.

packages/secret-scan/src/rules/ip_public.ts [7]

-/(?<![\w.])((?!(192\.168\.|127\.|10\.|172\.(?:1[6-9]|2[0-9]|3[01])))(?:(?:25[0-5]|2[0-4][0-9]|1[0-9][0-9]|[1-9][0-9]|[0-9])\.){3}(?:25[0-5]|2[0-4][0-9]|1[0-9][0-9]|[1-9][0-9]|[0-9])(?:\:\d{1,5})?)(?![\w.])/
+const privateIPRanges = /^(192\.168\.|127\.|10\.|172\.(1[6-9]|2[0-9]|3[01]))/;
+const ipAddressPattern = /\b(?:(?:25[0-5]|2[0-4][0-9]|1[0-9][0-9]|[1-9][0-9]|[0-9])\.){3}(?:25[0-5]|2[0-4][0-9]|1[0-9][0-9]|[1-9][0-9]|[0-9])(?::\d{1,5})?\b/;
 
+function isPublicIP(ip: string): boolean {
+  return ipAddressPattern.test(ip) && !privateIPRanges.test(ip);
+}
+
+return [new RegExp(ipAddressPattern.source, 'g')];
+
  • Apply this suggestion
Suggestion importance[1-10]: 7

Why:

7
Performance
Use a more efficient data structure for storing found secrets

Consider using a more efficient data structure like Set instead of an array for
foundSecrets to improve performance when checking for duplicates.

apps/cli/src/commands/scan.command.ts [63]

-const foundSecrets = []
+const foundSecrets = new Set()
 
  • Apply this suggestion
Suggestion importance[1-10]: 7

Why:

7
Use asynchronous file operations to improve performance

Consider using async/await for file operations to improve performance and avoid
blocking the event loop.

apps/cli/src/commands/scan.command.ts [68]

-const content = readFileSync(file, 'utf8').split(/\r?\n/)
+const content = (await fs.promises.readFile(file, 'utf8')).split(/\r?\n/)
 
  • Apply this suggestion
Suggestion importance[1-10]: 7

Why:

7
Best practice
Use explicit typing for function parameters

Consider using a more specific type for options instead of implicitly typing it
based on usage.

apps/cli/src/commands/scan.command.ts [40]

-const { currentChanges, file } = options
+const { currentChanges, file } = options as { currentChanges?: boolean; file?: string }
 
  • Apply this suggestion
Suggestion importance[1-10]: 7

Why:

7
Compatibility
Simplify Huggingface token regex patterns for better compatibility

The current regex patterns for Huggingface tokens use lookahead and lookbehind
assertions, which might not be supported in all JavaScript environments. Consider
using a simpler regex pattern that achieves the same result.

packages/secret-scan/src/rules/huggingface.ts [7-10]

-/(?:^|[\\'"` >=:])(hf_[a-zA-Z]{34})(?:$|[\\'"` <])/,
+/\b(hf_[a-zA-Z]{34})\b/,
 
 // Huggingface Organization Access Token Regex
-/(?:^|[\\'"` >=:\(,)])(api_org_[a-zA-Z]{34})(?:$|[\\'"` <\),])/
+/\b(api_org_[a-zA-Z]{34})\b/
 
  • Apply this suggestion
Suggestion importance[1-10]: 7

Why:

7
Possible issue
Add a specific prefix to the Heroku API key regex for more accurate matching

The current regex for Heroku API keys might match other UUID-like strings that are
not necessarily Heroku keys. Consider adding a prefix or specific pattern that is
unique to Heroku API keys.

packages/secret-scan/src/rules/heroku.ts [7]

-/\b[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}\b/
+/\bheroku_[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}\b/i
 
  • Apply this suggestion
Suggestion importance[1-10]: 7

Why:

7

@rajdip-b rajdip-b changed the title feat(cli): Secret scan feature merge feat(cli): Secret scan Sep 17, 2024
@rajdip-b rajdip-b enabled auto-merge (squash) September 17, 2024 05:21
@rajdip-b rajdip-b disabled auto-merge September 17, 2024 05:21
@rajdip-b rajdip-b merged commit 85cb8ab into keyshade-xyz:develop Sep 17, 2024
5 of 9 checks passed
Kiranchaudhary537 pushed a commit to Kiranchaudhary537/keyshade that referenced this pull request Oct 13, 2024
Co-authored-by: kriptonian1 <shymal.sawan@gmail.com>
rajdip-b pushed a commit that referenced this pull request Oct 24, 2024
## [2.6.0](v2.5.0...v2.6.0) (2024-10-24)

### 🚀 Features

* **api:**  Add icon and remove description field from workspace ([#435](#435)) ([a99c0db](a99c0db))
* **api-client:** Added workspace-membership and related tests ([#452](#452)) ([6a1c091](6a1c091))
* **api-client:** Create controller for User module ([#484](#484)) ([f9d8e83](f9d8e83))
* **api:** Add prod env schema in env file ([#436](#436)) ([21c3004](21c3004))
* **api:** Add resend otp implementation ([#445](#445)) ([4dc6aa1](4dc6aa1))
* **api:** Fetch total count of environments, [secure]s and variables in project ([#434](#434)) ([0c9e50a](0c9e50a))
* **api:** Replace `projectId` with `name` and `slug` in workspace-role response.  ([#432](#432)) ([af06071](af06071))
* **cli:** Add functionality to operate on Secrets ([#504](#504)) ([1b4bf2f](1b4bf2f))
* **cli:** Add project command ([#451](#451)) ([70448e1](70448e1))
* **cli:** Add workspace operations ([#441](#441)) ([ed38d22](ed38d22))
* **cli:** implement commands to get, list, update, and delete, workspace roles ([#469](#469)) ([957ea8d](957ea8d))
* **cli:** Implemented pagination support ([#453](#453)) ([feb1806](feb1806))
* **cli:** Secret scan ([#438](#438)) ([85cb8ab](85cb8ab))
* **cli:** Update environment command outputs ([f4af874](f4af874))
* **platform:** Clearing email field after waitlisting the user email ([#481](#481)) ([256d659](256d659))
* Remove project IDs from workspace role export data and update tests ([#448](#448)) ([8fdb328](8fdb328))
* **web:** Configured extra check for waitlisted users already in the list and created toast message for them ([#492](#492)) ([2ddd0ef](2ddd0ef))
* **web:** show the toast only when email add successfully ([#490](#490)) ([783c411](783c411))

### 🐛 Bug Fixes

* **api,api-client:** Add environmentSlug in multiple places across the variable module ([#468](#468)) ([d970aff](d970aff))
* **api:** Replace the id with slug in the global-search service ([#455](#455)) ([74804b1](74804b1))
* **platform:** Fixed duplicate Google Logo UI fix  ([#450](#450)) ([fb0d6f7](fb0d6f7))
* resolve footer website name cut-off or overlap issue ([#444](#444)) ([fe03ba2](fe03ba2))
* **web:** Horizontal Scrolling issue on the website ([#440](#440)) ([655177b](655177b))

### 📚 Documentation

* Add documentation for environment in CLI ([#462](#462)) ([dad7394](dad7394))
* Add documentation for project in CLI ([#466](#466)) ([341fb32](341fb32))
* Add documentation for scan in CLI ([#461](#461)) ([72281e6](72281e6))
* Add documentation for workspace command ([#464](#464)) ([4aad8a2](4aad8a2))
* Add instructions for resetting the local Prisma database ([#495](#495)) ([#501](#501)) ([b07ea17](b07ea17))
* Added docker support documentation ([#465](#465)) ([bc04be4](bc04be4))
* Added documentation for running the platform ([#473](#473)) ([8b8386b](8b8386b))
* Added missing mappings to pages ([5de9fd8](5de9fd8))
* Fix Documentation Hyperlink and update expired Discord invite link ([#496](#496)) ([5a10e39](5a10e39))
* Updated CLI docs ([#460](#460)) ([c7e0f13](c7e0f13))

### 🔧 Miscellaneous Chores

* Add more logging to Sentry init ([#470](#470)) ([de4925d](de4925d))
* **api:** Optimise API docker image size ([#360](#360)) ([ea40dc1](ea40dc1))
* **api:** Updated lockfile ([a968e78](a968e78))
* **CI:** Add [secure] scan validation ([f441262](f441262))
* **cli:** Update controller invocation in environment commands ([#477](#477)) ([596bd1a](596bd1a))
* Minor changes to variables ([fe01ca6](fe01ca6))
* **[secure]-scan:** Failing lint issues ([#507](#507)) ([48f45df](48f45df))
* **[secure]-scan:** Formatted files ([5884833](5884833))
* Update .env.example ([70ad4f7](70ad4f7))
* Updated scripts ([9eb76a7](9eb76a7))
* **web:** email validation ([#487](#487)) ([e8e737a](e8e737a))
@rajdip-b
Copy link
Member

🎉 This PR is included in version 2.6.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

3 participants