Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(npm): Fix build checks for JavaScript code #1354

Merged
merged 2 commits into from
Dec 6, 2024
Merged

Conversation

harmony7
Copy link
Member

@harmony7 harmony7 commented Dec 4, 2024

When building the project locally I get these errors:

┌──────────────────────────┐
│ 3 Blocking Code Findings │
└──────────────────────────┘
                                            
    npm/@fastly/cli/update.js
     ❱ javascript.lang.security.audit.unsafe-formatstring.unsafe-formatstring
          Detected string concatenation with a non-literal variable in a util.format / console.log function.
          If an attacker injects a format specifier in the string, it will forge the log message. Try to use
          constant values for the format string.                                                            
          Details: https://sg.run/7Y5R                                                                      
                                                                                                            
           77┆ `Response from https://api.github.com/repos/fastly/cli/releases/tags/${tag} was not ok`,
            ⋮┆----------------------------------------
           90┆ `Response from https://api.github.com/repos/fastly/cli/releases/${id}/assets was not ok`,
            ⋮┆----------------------------------------
          123┆ console.error(`Response from ${browser_download_url} was not ok`, archive);
                            
  BLOCKING CODE RULES FIRED:
    javascript.lang.security.audit.unsafe-formatstring.unsafe-formatstring

This happens because the first parameter to console.error() is a format string that can accept printf-like placeholders like %s. Using non-literal values to build this value is unsafe. For example, tag is an untrusted value that is obtained from argv. If we were to craft a value for tag that includes a placeholder such as %O, then we could force the program to dump the internals of the other parameters being passed to console.error().

This PR addresses this issue by inserting a hard-coded literal '%s %o' for the format string.

This is actually a point of confusion with console.log() and friends, because:

  • Most of the time they are used without printf-style placeholders at all
  • The functions are defined so that if they are passed only one parameter, then they display just that value as is, without formatting
  • The functions are defined so that if the first parameter contains no placeholders, then they display a string that is the concatenation of all arguments separated by spaces

See util.format() for details.

@harmony7 harmony7 requested a review from kailan December 4, 2024 03:30
@harmony7
Copy link
Member Author

harmony7 commented Dec 4, 2024

For further context:
This is error output from the semgrep tool, which is a step that only runs during local development (make all or make semgrep). That's why the error does not come up during CI on GitHub.

See #810.

@kpfleming
Copy link
Contributor

For reference, this was reported in Jira in issue CDTOOL-933.

Copy link
Member

@kailan kailan left a comment

Choose a reason for hiding this comment

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

Perfect 👍

@kpfleming kpfleming merged commit 213121a into main Dec 6, 2024
9 checks passed
@kpfleming kpfleming deleted the kats/js-build-errors branch December 6, 2024 15:12
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.

3 participants