VSCode plugin does not provide a fully working replacement

[martijn-coolminds]

Describe the bug
When using the VSCode plugin and triggering a rule, the offered replacement isn't parsed properly.
When DS144886 is triggered for instance, I get a replacement suggestion of +

So: $_REQUEST would need to be replaced with $_GET but the suggestion is $_REQUESTGET (screenshot attached).
I don't think it's the rule itself, but rather a plugin issue?

To Reproduce
Steps to reproduce the behavior:

1. Create a new PHP file in VSCode
2. Write <?php $_REQUEST['id'] = 1;
3. Place cursor at beginning
4. See that the wrong replacement is suggested ($_REQUESTGET, $__REQUESTPOST) instead of $_GET and $_POST

Expected behavior
$_GET and $_POST being suggested.

Screenshots

Versions(please complete the following information):

• OS: Windows 11 (10.0.22631)
• Devskim Version 1.0.33
• VSCode 1.89.0 (2024-05-01)

[gfs]

Thanks for the report. Definitely a bug. I suspect with the replacement pattern in the rule.

[martijn-coolminds]

Putting the language server in verbose mode gives the following traces (extracted a single one);

[Trace - 2:55:22 PM] Received notification 'textDocument/publishDiagnostics'.
Params: {
    "uri": "file:///c:/projects/test.php",
    "version": 5,
    "diagnostics": [
        {
            "range": {
                "start": {
                    "line": 2,
                    "character": 0
                },
                "end": {
                    "line": 2,
                    "character": 9
                }
            },
            "severity": 2,
            "code": "MS-CST-E.vscode-devskim: DS144886",
            "source": "DevSkim Language Server",
            "message": "$_REQUEST combines POST, GET, and cookie values in one array, making it easy for an attacker to modify a POST or cookie value by instead putting it in a GET and sending the URL to the victim"
        }
    ]
}


[Trace - 2:55:22 PM] Received notification 'devskim/fileversion'.
Params: {
    "version": 5,
    "fileName": "file:///c:/projects/test.php"
}


[Trace - 2:55:22 PM] Received notification 'devskim/codefixmapping'.
Params: {
    "version": 5,
    "diagnostic": {
        "range": {
            "start": {
                "line": 2,
                "character": 0
            },
            "end": {
                "line": 2,
                "character": 9
            }
        },
        "severity": 2,
        "code": "MS-CST-E.vscode-devskim: DS144886",
        "source": "DevSkim Language Server",
        "message": "$_REQUEST combines POST, GET, and cookie values in one array, making it easy for an attacker to modify a POST or cookie value by instead putting it in a GET and sending the URL to the victim"
    },
    "replacement": "$_REQUESTGET",
    "fileName": "file:///c:/projects/test.php",
    "friendlyString": "Replace with $_REQUESTGET",
    "matchStart": 9,
    "matchEnd": 18,
    "isSuppression": false
}

That correspondents to the following rule specification

 "fix_its": [
      {
        "name": "Change to $_GET",
        "type": "RegexReplace",
        "replacement": "$_GET",
        "pattern": {
          "pattern": "\\$_REQUEST",
          "type": "regex",
          "scopes": [
            "code"
          ],
          "_comment": ""
        }
      },

I think the definition literally says "Replace $_REQUEST with $_GET" in this case, so the replacement pattern in the rule shouldn't be the problem. I'm thinking it's more in the plugin itself?

Looking at the trace message it seems to have the correct cursor range.

        "range": {
            "start": {
                "line": 2,
                "character": 0
            },
            "end": {
                "line": 2,
                "character": 9
            }

So I'm assuming the vscode plugin's replacement logic mixes it up. I'm not setup nor experienced enough to build it / run it.
But i'm suspecting either the codeAction section here;

DevSkim/DevSkim-VSCode-Plugin/client/devSkimFixer.ts

Line 43 in ba39d7a

provideCodeActions(document: vscode.TextDocument, range: vscode.Range | vscode.Selection, context: vscode.CodeActionContext, token: vscode.CancellationToken): vscode.CodeAction[] {

Or (most likely) the createFix function here doesn't work properly by either getting the wrong value or executing the wrong replacement function here

DevSkim/DevSkim-VSCode-Plugin/client/devSkimFixer.ts

Line 60 in ba39d7a

private createFix(document: vscode.TextDocument, diagnostic: vscode.Diagnostic, codeFix: CodeFixMapping): vscode.CodeAction

[gfs]

I was able to dig into this a bit today and was able to replicate the same behavior using the analyze/fix commands in the DevSkim CLI.

For context, the extension acts as a pretty thin wrapper around the language server which uses the same APIs as the CLI to perform analysis and generate fixes. Fixes are generated by this method from a code snippet and rule - even for fixes consumed by the extension:

DevSkim/DevSkim-DotNet/Microsoft.DevSkim/DevSkimRuleProcessor.cs

Lines 85 to 102 in ba39d7a

	public static string? Fix(string text, CodeFix fixRecord)
	{
	string? result = null;
	if (fixRecord?.FixType is { } and FixType.RegexReplace)
	{
	if (fixRecord.Pattern is { } fixPattern)
	{
	Regex? regex = SearchPatternToRegex(fixPattern);
	if (regex is { })
	{
	result = regex.Replace(text, fixRecord.Replacement ?? string.Empty);
	}
	}
	}
	return result;
	}

The issue appears to be with the $_ sequence in the replacements specified in the fixes for this rule.

DevSkim/rules/default/security/frameworks/php.json

Lines 25 to 38 in ba39d7a

	"fix_its": [
	{
	"name": "Change to $_GET",
	"type": "RegexReplace",
	"replacement": "$_GET",
	"pattern": {
	"pattern": "\\$_REQUEST",
	"type": "regex",
	"scopes": [
	"code"
	],
	"_comment": ""
	}
	},

Since the RuleProcessor generates fixes using the Regex.Replace API the replacement text is interpreted by the Regex engine and substitutions are applied. By coincidence, $_ means 'insert the entire matched sequence' from the Regex, thus appending $_REQUEST to GET for $_REQUESTGET.

The fix was to update the rule to add an extra $ so the $_ sequence is interpreted literally and not as a substitution. See #616.

[martijn-coolminds]

Thanks for figuring this out! Seems legit, if I replace it manually in the json on my machine, it works 👍

[gfs]

Thanks @martijn-coolminds for confirming!