nickdeis / eslint-plugin-no-secrets

An eslint plugin to find strings that might be secrets/credentials
MIT License
137 stars 5 forks source link

additionalRegexes doesn't seem to handle pattern matching whitespaces #36

Open CyberRoute opened 1 week ago

CyberRoute commented 1 week ago

Hello, I have trying to use the plugin to target secrets that don't have an actual prefix within the string e.g. AKIAIOSFODNN7EXAMPLE. So when I try to add regexes like /.+\["'\]x-auth-key\["'\]\\s\*\\\]\\s\*=\\s\*\["'\]\[a-zA-Z0-9\]{37}\["'\]/i the pattern won't match. The issue seems in here https://github.com/nickdeis/eslint-plugin-no-secrets/blob/master/index.js#L66 the splitIntoTokens function splits the input string on each specified delimiter, which includes spaces. This means that if your regex patterns rely on finding specific sequences of characters that contain whitespace (e.g., API Key: XXXXXXXXXX), this splitting will separate the words and may break the intended regex match. Not 100% sure this is an intended behaviour, but seems odd. Thanks in advance

nickdeis commented 1 week ago

Hey @CyberRoute, First off, thanks for filing an issue. What's strange is that splitIntoTokens should only be used for the entropy calculation and addtionalRegexes are part of the patternReport, which uses the function checkRegexes. However, I'm not 100% sure. What I'm going to do is take your example and write a unit test. Once again, thank you. I use this extension in my own work so I like to know when it doesn't work. Nick

CyberRoute commented 1 week ago

Thanks man! For the prompt response I am curious to know your findings! Was going to attempt a pr but I preferred reaching out first

nickdeis commented 1 week ago

Hey @CyberRoute,

If you have a PR or want to make the change, I always appreciate it and will work collaboratively with you to make sure your PR gets in while keeping backwards compatibility. My goal with any opensource repo is to increase that contributor count.

So I've written a test, and I wanted to confirm one thing with you: You are trying to match something like this, right?

const X_AUTH_KEY = '"X-Auth-Key": "c2547eb745079dac9320b638f5e225cf483cc5cfdda41"'

Best, Nick

CyberRoute commented 1 week ago

hi @nickdeis what I am trying to match look like: init.headers['X-Auth-Key'] = 'ab18332788b7866eb236237c853e820841234'; what I have tried to do so far is creating a new test case that looks like this:

const X_AUTH_KEY_TEST = `
  init.headers['X-Auth-Key'] = 'ab18332788b7866eb236237c853e820841234';
`;
 {
        code: X_AUTH_KEY_TEST,
        options: [{ additionalRegexes: { "Cloudflare token": /[a-zA-Z0-9]{37}/ }}],
        errors: [PATTERN_MATCH_MSG]
},

the above passes but if I change to:

 {
        code: X_AUTH_KEY_TEST,
        options: [{ additionalRegexes: { "Cloudflare token": /.+\["'\]x-auth-key\["'\]\\s\*\\\]\\s\*=\\s\*\["'\]\[a-zA-Z0-9\]{37}\["'\]/i }}],
        errors: [PATTERN_MATCH_MSG]
      },

I start getting the failure, which so far I haven't managed to solve:

generatedMessage: false,
  code: 'ERR_ASSERTION',
  actual: 0,
  expected: 1,
  operator: 'strictEqual'
}
nickdeis commented 5 days ago

Hey @CyberRoute , Okay, now I can clearly see why this isn't working. So part of the reason I created this over truffleHog was that I wanted to look in the AST for high entropy strings + patterns. Since ESLint works with the AST,

init.headers['X-Auth-Key'] = 'ab18332788b7866eb236237c853e820841234';

Ends up being three or four tokens. However, this whole conversation is giving me a lot of very cool ideas.

No Literal Strings For Certain Properties (harder)

This could either be a new rule or a config option, but essentially

init.headers['X-Auth-Key'] = process.env.X_AUTH_KEY;
init.headers['X-Auth-Key'] = `x-auth-key: ${process.env.X_AUTH_KEY}`;

would be allowed, but

init.headers['X-Auth-Key'] = 'ab18332788b7866eb236237c853e820841234';
init.headers['X-Auth-Key'] = "ab18332788b7866eb236237c853e820841234";

would not be allowed if the user configured X-Auth-Key to not allow literal strings.

Have a rule that applies patterns to the whole file (easier)

I could probably do this quite easily via the top level Program node + scanning the comments.

Honestly both seem like useful options.

Let me know what you think and thanks for the help, Nick

CyberRoute commented 4 days ago

Hey @nickdeis thanks for looking into this. I would pick to easier :)

nickdeis commented 3 days ago

Hey @CyberRoute, I'll hopefully get to this real soon (Friday or Saturday) but I like that idea too since it will allow people to fill in any gaps that AST analysis can't find. I still might do the harder option but I want to really think about the design on that one. Thank You! Nick

CyberRoute commented 3 days ago

Cool that sounds great @nickdeis ! I am not really a typescript/javascript guy, so I am looking forward to learn! thanks so much for looking into this