Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 27 additions & 3 deletions packages/scan/src/__tests__/code-rules.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -869,12 +869,36 @@ describe('misconfiguration, weak crypto and injection', () => {
it('flags an XPath expression with an inner quote around the interpolation', () => {
// The `"`-delimited f-string contains a `'`; a class that excluded both
// quotes stopped before the interpolation and missed it.
expect(ruleIds('a.py', "expr = f\"//user[name/text()='{username}']\"")).toContain(
'py-xpath-injection',
);
const src = [
'from lxml import etree',
'expr = f"//user[name/text()=\'{username}\']"',
].join('\n');
expect(ruleIds('a.py', src)).toContain('py-xpath-injection');
expect(ruleIds('a.py', 'result = doc.xpath("//user[name=$n]", n=username)')).toEqual([]);
});

it('does not read the // of a URL scheme as an XPath step', () => {
// The commonest false positive by far: an f-string holding a URL. `//`
// after a scheme is not a descendant-or-self step, and a file that does no
// XML at all cannot be evaluating XPath. Both halves are asserted here
// because either one alone still let the other shape through.
const urls = [
'url = f"https://{host}/v1/{path}"',
'dsn = f"postgres://postgres:{pw}@{host}:5432/{db}"',
"req = f'redis://{host}:{port}/0'",
].join('\n');
expect(ruleIds('a.py', urls)).not.toContain('py-xpath-injection');

// Even with lxml in the file, a URL is still a URL.
expect(ruleIds('a.py', ['from lxml import etree', 'url = f"https://{host}/v1"'].join('\n'))).toEqual(
[],
);

// And a real XPath in a file that never imports an XML library is out of
// scope, the same way the Java XXE rule joins on its parser imports.
expect(ruleIds('a.py', 'expr = f"//user[@id={uid}]"')).toEqual([]);
});

it('flags a NoSQL query built from request data', () => {
expect(ruleIds('a.js', 'return users.find(req.body.filter);')).toContain('js-nosql-injection');
expect(ruleIds('a.js', 'return users.find({ _id: sanitize(req.params.id) });')).toEqual([]);
Expand Down
24 changes: 24 additions & 0 deletions packages/scan/src/__tests__/false-positives.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,30 @@ describe('fixture credentials in test files', () => {
expect(ruleIds('tests/db.test.js', dsn)).toContain('secret-database-url');
});

it('exempts a DSN whose password slot is a Python f-string or a <pw> usage line', () => {
// The code that BUILDS a connection string holds the password in a
// variable; the interpolation is the proof it is not a credential. Both
// shapes came off one pull request -- the f-string from the tool that
// assembles the DSN, the angle brackets from the usage comment above it.
const templated = [
'url = f"postgres://postgres:{pw}@{host}:5432/{db}"',
"dsn = f'mysql://{user}:{password}@{host}/{name}'",
'# CLOUD_DB_URL=postgres://postgres.<ref>:<pw>@aws-0-<region>.pooler.supabase.com:5432/postgres',
];
for (const line of templated) {
expect(ruleIds('dev2/dev2-site', line), line).not.toContain('secret-database-url');
}

// A literal password in the same slot is still material, and a templated
// username beside a real password stays reported: only the password slot
// decides it.
expect(ruleIds('dev2/sites.d/site.json', 'postgres://postgres:7f3a9c21e4@db.internal:5432/app'))
.toContain('secret-database-url');
expect(ruleIds('a.py', 'url = f"postgres://{user}:hunter2hunter2@{host}/app"')).toContain(
'secret-database-url',
);
});

it('does not read a fixture stem out of the middle of a word', () => {
expect(isTestFixtureValue("const x = 'a'", 'latestbuildsecret')).toBe(false);
expect(isTestFixtureValue("const x = 'a'", 'contestwinner2024')).toBe(false);
Expand Down
15 changes: 13 additions & 2 deletions packages/scan/src/code-rules.ts
Original file line number Diff line number Diff line change
Expand Up @@ -330,6 +330,14 @@ const XXE_GUARD =
const XML_PARSING_FILE =
/\b(?:javax\.xml|org\.xml\.sax|org\.w3c\.dom|org\.jdom2?|org\.dom4j|XmlPullParser|DocumentBuilderFactory|DocumentBuilder|SAXParserFactory|SAXParser|XMLInputFactory|XMLReaderFactory|XMLReader|SAXBuilder|SAXReader)\b/;

/**
* Python only evaluates XPath through lxml or ElementTree; nothing else in the
* standard library takes an XPath expression. Joining on the file keeps the
* rule off the far more common f-string that simply holds a URL -- see the
* `://` exclusion in the pattern for the second half of that defence.
*/
const PY_XPATH_FILE = /\b(?:lxml|xml\.etree|ElementTree|etree|defusedxml|xpath)\b/i;

/** A sink that executes whatever string reaches it. */
const CODE_SINK =
/\bglobalThis\s*\[|\bconstructor\b|\beval\b|\bFunction\b|\brun\s*\(|\bvm\s*\.\s*run/;
Expand Down Expand Up @@ -1456,9 +1464,12 @@ export const CODE_RULES: readonly CodeRule[] = [
severity: 'high',
languages: ['python'],
// Per-quote, so an inner `'` in a `"`-delimited f-string (`text()='{x}'`)
// does not truncate the match before the interpolation.
// does not truncate the match before the interpolation. `(?<!:)` keeps the
// `//` of a URL scheme out of it: `f"https://{host}/v1"` is a URL, not a
// descendant-or-self step, and it was by far the commonest match here.
pattern:
/\bf"[^"\n]*(?:\/\/|\/\w+\[)[^"\n]*\{[^}\n]+\}|\bf'[^'\n]*(?:\/\/|\/\w+\[)[^'\n]*\{[^}\n]+\}/,
/\bf"[^"\n]*(?:(?<!:)\/\/|\/\w+\[)[^"\n]*\{[^}\n]+\}|\bf'[^'\n]*(?:(?<!:)\/\/|\/\w+\[)[^'\n]*\{[^}\n]+\}/,
fileRequires: PY_XPATH_FILE,
},
{
id: 'py-fast-password-hash',
Expand Down
17 changes: 13 additions & 4 deletions packages/scan/src/secret-rules.ts
Original file line number Diff line number Diff line change
Expand Up @@ -207,11 +207,20 @@ export const SECRET_RULES: readonly SecretRule[] = [
*/
/**
* A template expression, in the syntaxes that reach a connection string: Go
* and Handlebars `{{…}}`, shell and JavaScript `${…}`, bare `$VAR`, and
* printf `%s`/`%d`/`%v`. Anchored to the end of the credential segment so it
* has to fill the slot rather than merely appear somewhere near it.
* and Handlebars `{{…}}`, shell and JavaScript `${…}`, bare `$VAR`, printf
* `%s`/`%d`/`%v`, the single brace of a Python f-string or `str.format`
* (`{pw}`), and the `<pw>` of a usage line in a comment or README. Anchored to
* the end of the credential segment so it has to fill the slot rather than
* merely appear somewhere near it.
*
* The single brace and the angle brackets are the two that a tool building
* DSNs actually writes — `f"postgres://postgres:{pw}@{host}/{db}"` is the code
* that assembles a connection string, not a credential sitting in a file. Note
* the angle-bracket form cannot be left to the `<[a-z-]+>` entry in
* KNOWN_PLACEHOLDERS below: that one is `\b`-anchored, and there is no word
* boundary between the `:` of the DSN and the `<` that follows it.
*/
const INTERPOLATION = String.raw`(?:\{\{[^}]*\}\}|\$\{[^}]*\}|\$[A-Za-z_]\w*|%[sdv])`;
const INTERPOLATION = String.raw`(?:\{\{[^}]*\}\}|\$\{[^}]*\}|\$[A-Za-z_]\w*|%[sdv]|\{[^{}\s'"@/]+\}|<[A-Za-z_][\w.-]*>)`;

const KNOWN_PLACEHOLDERS = [
// Deliberately NOT here: AWS's published documentation key/secret pair
Expand Down
Loading