Skip to content

fix: Address three security findings from Fortify scan - #1101

Open
jmadhur87 wants to merge 7 commits into
fortify:dev/v3.xfrom
jmadhur87:mjain6/FAAVulnFix3
Open

jmadhur87 wants to merge 7 commits into
fortify:dev/v3.xfrom
jmadhur87:mjain6/FAAVulnFix3

Conversation

@jmadhur87

@jmadhur87 jmadhur87 commented Sep 22, 2026 •

Copy link
Copy Markdown
Contributor

Fixes three issues reported by a Fortify scan:
2 Path Manipulation and 1 Privacy Violation

  • Report directory deletion followed symbolic links, so a symlink inside the target directory could cause files outside it to be
    deleted. Now uses the existing FileUtils.deleteRecursive(), which does not follow links.

  • CSV output wrote values starting with =, +, -, @, tab, CR or LF as-is, which spreadsheet apps treat as formulas. These values are
    now prefixed with a single quote. Numbers are unaffected.

  • The FVDL description parser recursed without any depth limit, so a deeply nested description in an FPR could cause a StackOverflowError. Nesting is now capped, with remaining content rendered as plain text.

@jmadhur87
jmadhur87 requested a review from rsenden September 23, 2026 10:36
}

/** Neutralize CSV formula injection by prefixing affected text values with a single quote. */
private static ObjectNode escapeFormulas(ObjectNode formattedRecord) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

  • Whether we should escape formula-like characters depends on the consumer. For example, if user loads CSV file into Excel, we should escape formulas, but we shouldn't escape if CSV file is consumed by some automation that consumes the records as-is without interpreting formulas. Probably best way to handle this is through the --style option, i.e., add new styles to control whether output should be escaped. Need to think whether these styles should be CSV-specific, like [no-]csv-escape or [no-]formula-escape, or more generic like [no-]escape. For safety, escaping enabled should be the default.
  • Current implementation operates at textual nodes only, not the final output string values. For example, it doesn't handle POJONodes for which the contents eventually get rendered as a string that could potentially represent a formula
  • Current implementation modifies the existing ObjectNode, which could cause issues if it's also used elsewhere (like if output is simultaneously being written to an fcli variable)
  • Current implementation causes an extra iteration over the fields, which may have a small performance impact

For the latter three, probably better to use a custom ObjectMapper/Serializer/SerializerModifier to escape the output as it's being written by Jackson.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants