Skip to content

Commit a7c93d1

Browse files
NEW(a4d): @W-17617362@: Send in context range dependent upon the rule
1 parent c9a50a7 commit a7c93d1

9 files changed

+832
-58
lines changed

.github/workflows/validate-pr.yml

+1-1
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ jobs:
2121
# Define regex patterns for different types of PR titles
2222
MAIN2DEV_REGEX="^MAIN2DEV[[:space:]]*:?[[:space:]]*@W-[[:digit:]]{8,9}@.*MERGING.+[[:digit:]]{1,2}\.[[:digit:]]{1,2}\.[[:digit:]]{1,2}.*"
2323
RELEASE2MAIN_REGEX="^RELEASE[[:space:]]*:?[[:space:]]*@W-[[:digit:]]{8,9}@.+"
24-
PR_INTO_DEV_OR_RELEASE_REGEX="^(FIX|CHANGE|NEW)([[:space:]]*\([a-zA-Z]+\))?[[:space:]]*:?[[:space:]]*@W-[[:digit:]]{8,9}@.+"
24+
PR_INTO_DEV_OR_RELEASE_REGEX="^(FIX|CHANGE|NEW)([[:space:]]*\([^)]+\))?[[:space:]]*:?[[:space:]]*@W-[[:digit:]]{8,9}@.+"
2525
2626
# Validate PR title based on base_ref and head_ref
2727
if [[ "$base_ref" == "dev" && "${{ startsWith(github.head_ref, 'm2d/') }}" == "true" && ! "$title_upper" =~ $MAIN2DEV_REGEX ]]; then

src/lib/agentforce/agentforce-violation-fixer.ts

+16-11
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ import {makePrompt, GUIDED_JSON_SCHEMA, LLMResponse, PromptInputs} from './llm-p
1010
import {LLMService, LLMServiceProvider} from "../external-services/llm-service";
1111
import {Logger} from "../logger";
1212
import {extractRuleName} from "../diagnostics";
13-
import {A4D_SUPPORTED_RULES} from "./supported-rules";
13+
import {A4D_SUPPORTED_RULES, RuleInfo, ViolationContextScope} from "./supported-rules";
1414
import {RangeExpander} from "../range-expander";
1515
import {FixSuggestion} from "../fix-suggestion";
1616
import {messages} from "../messages";
@@ -34,23 +34,28 @@ export class AgentforceViolationFixer {
3434
try {
3535
const llmService: LLMService = await this.llmServiceProvider.getLLMService();
3636

37-
const rangeExpander: RangeExpander = new RangeExpander();
38-
39-
const violationLinesRange: vscode.Range = rangeExpander.expandToCompleteLines(document, diagnostic.range);
37+
const ruleName: string = extractRuleName(diagnostic);
38+
const ruleInfo: RuleInfo = A4D_SUPPORTED_RULES.get(ruleName);
39+
if (!ruleInfo) {
40+
// Should never get called since suggestFix should only be called on supported rules
41+
throw new Error(`Unsupported rule: ${ruleName}`);
42+
}
4043

41-
// NOTE: We currently assume that the range of the context of the code to replace is equal to the range of
42-
// the violating lines. This won't work for the rules that need additional context, so we have a
43-
// TODO to update the context with W-17617362.
44-
const contextRange: vscode.Range = violationLinesRange;
44+
const rangeExpander: RangeExpander = new RangeExpander(document);
45+
const violationLinesRange: vscode.Range = rangeExpander.expandToCompleteLines(diagnostic.range);
46+
let contextRange: vscode.Range = violationLinesRange; // This is the default: ViolationContextScope.ViolationScope
47+
if (ruleInfo.violationContextScope === ViolationContextScope.ClassScope) {
48+
contextRange = rangeExpander.expandToClass(diagnostic.range);
49+
} else if (ruleInfo.violationContextScope === ViolationContextScope.MethodScope) {
50+
contextRange = rangeExpander.expandToMethod(diagnostic.range);
51+
}
4552

46-
// Generate the prompt
47-
const ruleName: string = extractRuleName(diagnostic);
4853
const promptInputs: PromptInputs = {
4954
codeContext: document.getText(contextRange),
5055
violatingLines: document.getText(violationLinesRange),
5156
violationMessage: diagnostic.message,
5257
ruleName: ruleName,
53-
ruleDescription: A4D_SUPPORTED_RULES.get(ruleName)
58+
ruleDescription: ruleInfo.description
5459
};
5560
const prompt: string = makePrompt(promptInputs);
5661

src/lib/agentforce/supported-rules.ts

+124-27
Original file line numberDiff line numberDiff line change
@@ -1,31 +1,128 @@
1-
// Until we move to using the node api of Code Analyzer v5, we would either have to get the rule descriptions
2-
// from the CLI, or from a hard coded map. For now, we just hard code the rule descriptions for the rules we support.
3-
// TODO: Replace with a more scalable solution
4-
export const A4D_SUPPORTED_RULES: Map<string, string> = new Map([
1+
/**
2+
* The scope of the context that a rule should send into the LLM
3+
*/
4+
export enum ViolationContextScope {
5+
// The class scope is used when we need to send in all the lines associated with the class that contains the violation
6+
ClassScope = 'ClassScope',
7+
8+
// The method scope is used when we need to send in all the lines associated with the method that contains the violation
9+
MethodScope = 'MethodScope',
10+
11+
// The violation scope is used when it is sufficient to just send in the violating lines without additional context
12+
ViolationScope = 'ViolationScope'
13+
}
14+
15+
/**
16+
* Rule information used for A4D Quick fixes to associate a rule to a description and {@link ViolationContextScope}
17+
*/
18+
export type RuleInfo = {
19+
description: string
20+
violationContextScope: ViolationContextScope
21+
}
22+
23+
/**
24+
* Map containing the rules that we support with A4D Quick Fix to the associated {@link RuleInfo} instance
25+
*
26+
* Note: Until we move to using the node api of Code Analyzer v5, we would either have to get the rule descriptions
27+
* from the CLI, or from a hard coded map. For now, we just hard code them for the rules we support.
28+
* // TODO: Replace with a more scalable solution
29+
*/
30+
export const A4D_SUPPORTED_RULES: Map<string, RuleInfo> = new Map([
31+
532
/* === Rules from rule selector: 'pmd:Recommended:ErrorProne:Apex' === */
6-
['AvoidDirectAccessTriggerMap', 'Avoid directly accessing Trigger.old and Trigger.new as it can lead to a bug. Triggers should be bulkified and iterate through the map to handle the actions for each item separately.'],
7-
['AvoidHardcodingId', 'When deploying Apex code between sandbox and production environments, or installing Force.com AppExchange packages, it is essential to avoid hardcoding IDs in the Apex code. By doing so, if the record IDs change between environments, the logic can dynamically identify the proper data to operate against and not fail.'],
8-
['AvoidNonExistentAnnotations', 'Apex supported non existent annotations for legacy reasons. In the future, use of such non-existent annotations could result in broken apex code that will not compile. This will prevent users of garbage annotations from being able to use legitimate annotations added to Apex in the future.'],
9-
['EmptyCatchBlock', 'Empty Catch Block finds instances where an exception is caught, but nothing is done. In most circumstances, this swallows an exception which should either be acted on or reported.'],
10-
['EmptyIfStmt', 'Empty If Statement finds instances where a condition is checked but nothing is done about it.'],
11-
['EmptyStatementBlock', 'Empty block statements serve no purpose and should be removed.'],
12-
['EmptyTryOrFinallyBlock', 'Avoid empty try or finally blocks - what\'s the point?'],
13-
['EmptyWhileStmt', 'Empty While Statement finds all instances where a while statement does nothing. If it is a timing loop, then you should use Thread.sleep() for it; if it is a while loop that does a lot in the exit expression, rewrite it to make it clearer.'],
14-
['InaccessibleAuraEnabledGetter', 'In the Summer \'21 release, a mandatory security update enforces access modifiers on Apex properties in Lightning component markup. The update prevents access to private or protected Apex getters from Aura and Lightning Web Components.'],
15-
['MethodWithSameNameAsEnclosingClass', 'Non-constructor methods should not have the same name as the enclosing class.'],
16-
['OverrideBothEqualsAndHashcode', 'Override both `public Boolean equals(Object obj)`, and `public Integer hashCode()`, or override neither. Even if you are inheriting a hashCode() from a parent class, consider implementing hashCode and explicitly delegating to your superclass. This is especially important when Using Custom Types in Map Keys and Sets.'],
17-
['TestMethodsMustBeInTestClasses', 'Test methods marked as a testMethod or annotated with @IsTest, but not residing in a test class should be moved to a proper class or have the @IsTest annotation added to the class. Support for tests inside functional classes was removed in Spring-13 (API Version 27.0), making classes that violate this rule fail compile-time. This rule is mostly usable when dealing with legacy code.'],
33+
['AvoidDirectAccessTriggerMap', {
34+
description: 'Avoid directly accessing Trigger.old and Trigger.new as it can lead to a bug. Triggers should be bulkified and iterate through the map to handle the actions for each item separately.',
35+
violationContextScope: ViolationContextScope.ViolationScope
36+
}],
37+
['AvoidHardcodingId', {
38+
description: 'When deploying Apex code between sandbox and production environments, or installing Force.com AppExchange packages, it is essential to avoid hardcoding IDs in the Apex code. By doing so, if the record IDs change between environments, the logic can dynamically identify the proper data to operate against and not fail.',
39+
violationContextScope: ViolationContextScope.ViolationScope
40+
}],
41+
['AvoidNonExistentAnnotations', {
42+
description: 'Apex supported non existent annotations for legacy reasons. In the future, use of such non-existent annotations could result in broken apex code that will not compile. This will prevent users of garbage annotations from being able to use legitimate annotations added to Apex in the future.',
43+
violationContextScope: ViolationContextScope.ViolationScope
44+
}],
45+
['EmptyCatchBlock', {
46+
description: 'Empty Catch Block finds instances where an exception is caught, but nothing is done. In most circumstances, this swallows an exception which should either be acted on or reported.',
47+
violationContextScope: ViolationContextScope.MethodScope
48+
}],
49+
['EmptyIfStmt', {
50+
description: 'Empty If Statement finds instances where a condition is checked but nothing is done about it.',
51+
violationContextScope: ViolationContextScope.MethodScope
52+
}],
53+
['EmptyStatementBlock', {
54+
description: 'Empty block statements serve no purpose and should be removed.',
55+
violationContextScope: ViolationContextScope.MethodScope
56+
}],
57+
['EmptyTryOrFinallyBlock', {
58+
description: 'Avoid empty try or finally blocks - what\'s the point?',
59+
violationContextScope: ViolationContextScope.ViolationScope
60+
}],
61+
['EmptyWhileStmt', {
62+
description: 'Empty While Statement finds all instances where a while statement does nothing. If it is a timing loop, then you should use Thread.sleep() for it; if it is a while loop that does a lot in the exit expression, rewrite it to make it clearer.',
63+
violationContextScope: ViolationContextScope.ViolationScope
64+
}],
65+
['InaccessibleAuraEnabledGetter', {
66+
description: 'In the Summer \'21 release, a mandatory security update enforces access modifiers on Apex properties in Lightning component markup. The update prevents access to private or protected Apex getters from Aura and Lightning Web Components.',
67+
violationContextScope: ViolationContextScope.MethodScope
68+
}],
69+
['MethodWithSameNameAsEnclosingClass', {
70+
description: 'Non-constructor methods should not have the same name as the enclosing class.',
71+
violationContextScope: ViolationContextScope.ViolationScope
72+
}],
73+
['OverrideBothEqualsAndHashcode', {
74+
description: 'Override both `public Boolean equals(Object obj)`, and `public Integer hashCode()`, or override neither. Even if you are inheriting a hashCode() from a parent class, consider implementing hashCode and explicitly delegating to your superclass. This is especially important when Using Custom Types in Map Keys and Sets.',
75+
violationContextScope: ViolationContextScope.ViolationScope
76+
}],
77+
['TestMethodsMustBeInTestClasses', {
78+
description: 'Test methods marked as a testMethod or annotated with @IsTest, but not residing in a test class should be moved to a proper class or have the @IsTest annotation added to the class. Support for tests inside functional classes was removed in Spring-13 (API Version 27.0), making classes that violate this rule fail compile-time. This rule is mostly usable when dealing with legacy code.',
79+
violationContextScope: ViolationContextScope.ViolationScope
80+
}],
81+
1882

1983
/* === Rules from rule selector: 'pmd:Recommended:Security:Apex' === */
20-
['ApexBadCrypto', 'The rule makes sure you are using randomly generated IVs and keys for `Crypto` calls. Hard-wiring these values greatly compromises the security of encrypted data.'],
21-
['ApexCRUDViolation', 'The rule validates you are checking for access permissions before a SOQL/SOSL/DML operation. Since Apex runs by default in system mode not having proper permissions checks results in escalation of privilege and may produce runtime errors. This check forces you to handle such scenarios. Since Winter \'23 (API Version 56) you can enforce user mode for database operations by using `WITH USER_MODE`...'],
22-
['ApexCSRF', 'Having DML operations in Apex class constructor or initializers can have unexpected side effects: By just accessing a page, the DML statements would be executed and the database would be modified. Just querying the database is permitted. In addition to constructors and initializers, any method called `init` is checked as well. Salesforce Apex already protects against this scenario and raises a runtime...'],
23-
['ApexDangerousMethods', 'Checks against calling dangerous methods. For the time being, it reports: * Against `FinancialForce`\'s `Configuration.disableTriggerCRUDSecurity()`. Disabling CRUD security opens the door to several attacks and requires manual validation, which is unreliable. * Calling `System.debug` passing sensitive data as parameter, which could lead to exposure of private data.'],
24-
['ApexInsecureEndpoint', 'Checks against accessing endpoints under plain **http**. You should always use **https** for security.'],
25-
['ApexOpenRedirect', 'Checks against redirects to user-controlled locations. This prevents attackers from redirecting users to phishing sites.'],
26-
['ApexSharingViolations', 'Detect classes declared without explicit sharing mode if DML methods are used. This forces the developer to take access restrictions into account before modifying objects.'],
27-
['ApexSOQLInjection', 'Detects the usage of untrusted / unescaped variables in DML queries.'],
28-
['ApexSuggestUsingNamedCred', 'Detects hardcoded credentials used in requests to an endpoint. You should refrain from hardcoding credentials: * They are hard to mantain by being mixed in application code * Particularly hard to update them when used from different classes * Granting a developer access to the codebase means granting knowledge of credentials, keeping a two-level access is not possible. * Using different...'],
29-
['ApexXSSFromEscapeFalse', 'Reports on calls to `addError` with disabled escaping. The message passed to `addError` will be displayed directly to the user in the UI, making it prime ground for XSS attacks if unescaped.'],
30-
['ApexXSSFromURLParam', 'Makes sure that all values obtained from URL parameters are properly escaped / sanitized to avoid XSS attacks.'],
84+
['ApexBadCrypto', {
85+
description: 'The rule makes sure you are using randomly generated IVs and keys for `Crypto` calls. Hard-wiring these values greatly compromises the security of encrypted data.',
86+
violationContextScope: ViolationContextScope.MethodScope
87+
}],
88+
['ApexCRUDViolation', {
89+
description: 'The rule validates you are checking for access permissions before a SOQL/SOSL/DML operation. Since Apex runs by default in system mode not having proper permissions checks results in escalation of privilege and may produce runtime errors. This check forces you to handle such scenarios. Since Winter \'23 (API Version 56) you can enforce user mode for database operations by using `WITH USER_MODE`...',
90+
violationContextScope: ViolationContextScope.ViolationScope
91+
}],
92+
['ApexCSRF', {
93+
description: 'Having DML operations in Apex class constructor or initializers can have unexpected side effects: By just accessing a page, the DML statements would be executed and the database would be modified. Just querying the database is permitted. In addition to constructors and initializers, any method called `init` is checked as well. Salesforce Apex already protects against this scenario and raises a runtime...',
94+
violationContextScope: ViolationContextScope.ViolationScope
95+
}],
96+
['ApexDangerousMethods', {
97+
description: 'Checks against calling dangerous methods. For the time being, it reports: * Against `FinancialForce`\'s `Configuration.disableTriggerCRUDSecurity()`. Disabling CRUD security opens the door to several attacks and requires manual validation, which is unreliable. * Calling `System.debug` passing sensitive data as parameter, which could lead to exposure of private data.',
98+
violationContextScope: ViolationContextScope.ViolationScope
99+
}],
100+
['ApexInsecureEndpoint', {
101+
description: 'Checks against accessing endpoints under plain **http**. You should always use **https** for security.',
102+
violationContextScope: ViolationContextScope.MethodScope
103+
}],
104+
['ApexOpenRedirect', {
105+
description: 'Checks against redirects to user-controlled locations. This prevents attackers from redirecting users to phishing sites.',
106+
violationContextScope: ViolationContextScope.MethodScope
107+
}],
108+
['ApexSharingViolations', {
109+
description: 'Detect classes declared without explicit sharing mode if DML methods are used. This forces the developer to take access restrictions into account before modifying objects.',
110+
violationContextScope: ViolationContextScope.ViolationScope
111+
}],
112+
['ApexSOQLInjection', {
113+
description: 'Detects the usage of untrusted / unescaped variables in DML queries.',
114+
violationContextScope: ViolationContextScope.MethodScope
115+
}],
116+
['ApexSuggestUsingNamedCred', {
117+
description: 'Detects hardcoded credentials used in requests to an endpoint. You should refrain from hardcoding credentials: * They are hard to mantain by being mixed in application code * Particularly hard to update them when used from different classes * Granting a developer access to the codebase means granting knowledge of credentials, keeping a two-level access is not possible. * Using different...',
118+
violationContextScope: ViolationContextScope.MethodScope
119+
}],
120+
['ApexXSSFromEscapeFalse', {
121+
description: 'Reports on calls to `addError` with disabled escaping. The message passed to `addError` will be displayed directly to the user in the UI, making it prime ground for XSS attacks if unescaped.',
122+
violationContextScope: ViolationContextScope.ViolationScope
123+
}],
124+
['ApexXSSFromURLParam', {
125+
description: 'Makes sure that all values obtained from URL parameters are properly escaped / sanitized to avoid XSS attacks.',
126+
violationContextScope: ViolationContextScope.ViolationScope
127+
}],
31128
]);

0 commit comments

Comments
 (0)