Skip to content
5 changes: 4 additions & 1 deletion packages/runner/src/lib/IssueFound.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,10 @@ import { SecTesterError } from '@sectester/core';
import { Issue } from '@sectester/scan';

export class IssueFound extends SecTesterError {
constructor(public readonly issue: Issue, formatter: Formatter) {
constructor(
public readonly issue: Issue,
formatter: Formatter
) {
super(`Target is vulnerable\n\n${formatter.format(issue)}`);
}
}
168 changes: 163 additions & 5 deletions packages/scan/src/DefaultScans.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,7 @@ describe('DefaultScans', () => {
});

afterEach(() =>
reset<typeof ci | ApiClient | Configuration>(
mockedApiClient,
mockedCi,
mockedConfiguration
)
reset<ApiClient | Configuration>(mockedApiClient, mockedConfiguration)
Copy link
Member

Choose a reason for hiding this comment

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

Now the tests is gonna behave incorrectly, CI mock is not being reset.

);

describe('createScan', () => {
Expand Down Expand Up @@ -70,6 +66,168 @@ describe('DefaultScans', () => {

expect(result).toEqual({ id });
});

it('should transform broken_access_control test with single auth object into backend compatible format', async () => {
const response = new Response(JSON.stringify({ id }));

when(mockedConfiguration.name).thenReturn('test');
when(mockedConfiguration.version).thenReturn('1.0');
when(mockedCi.name).thenReturn('github');

when(
mockedApiClient.request(
'/api/v1/scans',
deepEqual({
method: 'POST',
headers: {
'content-type': 'application/json'
},
body: JSON.stringify({
projectId,
name: 'test',
entryPointIds: [entryPointId],
tests: ['xss', 'broken_access_control'],
testMetadata: {
broken_access_control: {
authObjectId: [null, 'auth-id-123']
}
},
info: {
source: 'utlib',
provider: 'github',
client: {
name: 'test',
version: '1.0'
}
}
})
})
)
).thenResolve(response);

const result = await scans.createScan({
projectId,
name: 'test',
entryPointIds: [entryPointId],
tests: [
'xss',
{
name: 'broken_access_control' as const,
options: { auth: 'auth-id-123' }
}
]
});

expect(result).toEqual({ id });
});

it('should transform broken_access_control test with two auth objects into backend compatible format', async () => {
const response = new Response(JSON.stringify({ id }));
const config = {
projectId,
name: 'test scan',
entryPointIds: [entryPointId],
tests: [
{
name: 'broken_access_control' as const,
options: { auth: ['auth-id-1', 'auth-id-2'] as [string, string] }
}
]
};

when(mockedConfiguration.name).thenReturn('test');
when(mockedConfiguration.version).thenReturn('1.0');
when(mockedCi.name).thenReturn('github');

when(
mockedApiClient.request(
'/api/v1/scans',
deepEqual({
method: 'POST',
headers: {
'content-type': 'application/json'
},
body: JSON.stringify({
projectId,
name: 'test scan',
entryPointIds: [entryPointId],
tests: ['broken_access_control'],
testMetadata: {
broken_access_control: {
authObjectId: ['auth-id-1', 'auth-id-2']
}
},
info: {
source: 'utlib',
provider: 'github',
client: {
name: 'test',
version: '1.0'
}
}
})
})
)
).thenResolve(response);

const result = await scans.createScan(config);

expect(result).toEqual({ id });
});

it('should throw error when broken_access_control test has no auth option', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

This and tests below are no longer relevant due to https://github.com/NeuraLegion/sectester-js/pull/270/changes#r2692255661

const config = {
projectId,
name: 'test scan',
entryPointIds: [entryPointId],
tests: [
{
name: 'broken_access_control' as const,
options: {} as any
}
]
};

await expect(scans.createScan(config)).rejects.toThrow(
'Auth option is required for broken_access_control test'
);
});

it('should throw error when broken_access_control test has empty auth array', async () => {
const config = {
projectId,
name: 'test scan',
entryPointIds: [entryPointId],
tests: [
{
name: 'broken_access_control' as const,
options: { auth: [] as any }
}
]
};

await expect(scans.createScan(config)).rejects.toThrow(
'broken_access_control test auth option must be either a string or a tuple of two strings'
);
});

it('should throw error when broken_access_control test has auth array with 3 elements', async () => {
const config = {
projectId,
name: 'test scan',
entryPointIds: [entryPointId],
tests: [
{
name: 'broken_access_control' as const,
options: { auth: ['auth-id-1', 'auth-id-2', 'auth-id-3'] as any }
}
]
};

await expect(scans.createScan(config)).rejects.toThrow(
'broken_access_control test auth option must be either a string or a tuple of two strings'
);
});
});

describe('listIssues', () => {
Expand Down
73 changes: 72 additions & 1 deletion packages/scan/src/DefaultScans.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { Scans } from './Scans';
import { Issue, ScanConfig, ScanState } from './models';
import { BrokenAccessControlTest } from './models/Tests';
import { inject, injectable } from 'tsyringe';
import { ApiClient, ApiError, Configuration } from '@sectester/core';
import ci from 'ci-info';
Expand All @@ -19,7 +20,7 @@ export class DefaultScans implements Scans {
'content-type': 'application/json'
},
body: JSON.stringify({
...config,
...this.transformConfig(config),
info: {
source: 'utlib',
provider: ci.name,
Expand Down Expand Up @@ -80,4 +81,74 @@ export class DefaultScans implements Scans {

return result;
}

private transformConfig(config: ScanConfig): Record<string, unknown> {
if (!config.tests) {
return { ...config };
}

const { mappedTests, testMetadata } = this.mapTests(config.tests);
const { tests: originalTests, ...restConfig } = config;

if (Object.keys(testMetadata).length > 0) {
const result: Record<string, unknown> = {
...restConfig,
tests: mappedTests,
testMetadata
};

return result;
}

return { ...config };
Comment on lines +86 to +103
Copy link
Member

Choose a reason for hiding this comment

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

The intent is unclear from both the name and implementation. Rename the method to reflect its role and avoid imperative style.

Suggested change
if (!config.tests) {
return { ...config };
}
const { mappedTests, testMetadata } = this.mapTests(config.tests);
const { tests: originalTests, ...restConfig } = config;
if (Object.keys(testMetadata).length > 0) {
const result: Record<string, unknown> = {
...restConfig,
tests: mappedTests,
testMetadata
};
return result;
}
return { ...config };
const mapped = config.tests.map(test => this.mapTest(test));
const tests = mapped.map(t => t.name);
const testMetadata = mapped.reduce<TestMetadata | undefined>(
(acc, { metadata }) =>
metadata
? { ...acc, ...metadata }
: acc,
undefined
);
return {
...config,
tests,
...(testMetadata && { testMetadata }),
};

}

private mapTests(tests: ScanConfig['tests']) {
Copy link
Member

Choose a reason for hiding this comment

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

Too complex due to a lot of mutations. Methods should be pure, e.g.

private mapTest(test: Test): {
  name: string;
  metadata?: TestMetadata;
} {
  if (typeof test === 'string') {
    return { name: test };
  }

  switch (test.name) {
    case 'broken_access_control':
      return mapBrokenAccessControl(test);

    default:
      throw new Error(`Unsupported configurable test: ${test.name}`);
  }
}

const mappedTests: string[] = [];
const testMetadata: Record<string, unknown> = {};

if (!tests) {
throw new Error('Scan config should have tests defined');
}

for (const test of tests) {
if (typeof test === 'string') {
mappedTests.push(test);
continue;
}

if (test.name === 'broken_access_control') {
Copy link
Member

Choose a reason for hiding this comment

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

This check is technically redundant today, compiler knows the only single test with options

this.mapBrokenAccessControlTest(test, mappedTests, testMetadata);
} else {
throw new Error(`Unsupported configurable test: ${test.name}`);
}
}

return { mappedTests, testMetadata };
}

private mapBrokenAccessControlTest(
test: BrokenAccessControlTest,
mappedTests: string[],
testMetadata: Record<string, unknown>
) {
if (!test.options?.auth) {
Copy link
Member

Choose a reason for hiding this comment

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

Lets TS enforce correctness instead of runtime checks

throw new Error('Auth option is required for broken_access_control test');
}

const { auth } = test.options;
if (
typeof auth !== 'string' &&
(!Array.isArray(auth) || auth.length !== 2)
Comment on lines +141 to +142
Copy link
Member

Choose a reason for hiding this comment

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

ditto

) {
throw new Error(
`${test.name} test auth option must be either a string or a tuple of two strings`
);
}

mappedTests.push(test.name);
testMetadata[test.name] = {
authObjectId: typeof auth === 'string' ? [null, auth] : [auth[0], auth[1]]
};
}
}
101 changes: 101 additions & 0 deletions packages/scan/src/ScanSettings.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -187,5 +187,106 @@ describe('ScanSettings', () => {
name: expect.stringMatching(/^.{1,199}…$/)
});
});

it('should handle broken access control test with string auth', () => {
// arrange
const testConfig = {
name: 'broken_access_control' as const,
options: {
auth: 'auth-object-id'
}
};
const settings: ScanSettingsOptions = {
tests: [testConfig],
target: { url: 'https://example.com' }
};

// act
const result = new ScanSettings(settings);

// assert
expect(result.tests).toEqual([testConfig]);
});

it('should handle broken access control test with tuple auth', () => {
// arrange
const testConfig = {
name: 'broken_access_control' as const,
options: {
auth: ['key', 'value'] as [string, string]
}
};
const settings: ScanSettingsOptions = {
tests: [testConfig],
target: { url: 'https://example.com' }
};

// act
const result = new ScanSettings(settings);

// assert
expect(result.tests).toEqual([testConfig]);
});

it('should deduplicate string tests', () => {
// arrange
const testName = 'xss';
const settings: ScanSettingsOptions = {
tests: [testName, testName],
target: { url: 'https://example.com' }
};

// act
const result = new ScanSettings(settings);

// assert
expect(result.tests).toEqual([testName]);
});

it('should not allow duplicated tests with options', () => {
// arrange
const testConfig1 = {
name: 'broken_access_control' as const,
options: {
auth: 'auth1'
}
};
const testConfig2 = {
name: 'broken_access_control' as const,
options: {
auth: 'auth2'
}
};
const settings: ScanSettingsOptions = {
tests: [testConfig1, testConfig2],
target: { url: 'https://example.com' }
};

// act & assert
expect(() => new ScanSettings(settings)).toThrow(
'Duplicate test configuration found: broken_access_control'
);
});

it('should handle mixed string and configurable tests', () => {
// arrange
const stringTest = 'xss';
const testWithOptions = {
name: 'broken_access_control' as const,
options: {
auth: 'auth-object-id'
}
};
const settings: ScanSettingsOptions = {
tests: [stringTest, testWithOptions],
target: { url: 'https://example.com' }
};

// act
const result = new ScanSettings(settings);

// assert
expect(result.tests).toEqual([stringTest, testWithOptions]);
});
});
});
Loading
Loading