Skip to content

Conversation

@niStee
Copy link

@niStee niStee commented Dec 25, 2025

  • Add DevSkim CI job that runs on PRs to main and weekly (Tue 05:40 UTC).
  • Minimal permissions (actions/contents read, security-events write); uploads SARIF to the Security tab via github/codeql-action/upload-sarif.
  • Uses microsoft/DevSkim-Action@v1 and actions/checkout@v4. (Renovate will pin SHAs if configured Add conservative Renovate configuration with gated majors #5116; can pin manually if preferred.)

Testing/impact: Workflow-only change; no runtime code touched.

@github-advanced-security
Copy link

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

@CommandMC
Copy link
Member

From your recent PRs it seems you're quite deep into the field of software security. For the less experienced people (that's me), could you outline what it is that you're trying to add, and why it is helpful for us specifically (this goes for all of your PRs, not just this one)?

@niStee
Copy link
Author

niStee commented Dec 30, 2025

@CommandMC @arielj – I realize I've generated a lot of noise with PRs and Issues in the last days. My apologies, I got a bit carried away when the new scanners started flagging real vulnerabilities!

To clean this up, I'm going to pause all new PRs and Issues immediately. I've logged the specific bugs (Command Injection, XSS) as separate Issues just so they don't get lost, but I won't spam you with more.

If you feel the tooling PRs (Renovate/pnpm-audit) are too heavy right now, just let me know and I'll close them. I'm happy to pivot and focus purely on a single PR to fix the critical bugs I found instead.

Does that sound like a better way forward?

@arielj
Copy link
Collaborator

arielj commented Dec 30, 2025

what are the critical bugs you found?

@arielj
Copy link
Collaborator

arielj commented Dec 30, 2025

I do think that it's a good idea to explain what each tool added in these PRs is actually doing, like... what are the kind of security issues that DevSkim would find? what is OSV-Scanner supposed to find?

I don't think it's useful to add tools just to add them with no clear reason (at least I don't know what they do)

@niStee
Copy link
Author

niStee commented Dec 30, 2025

Good questions, @arielj! I'm happy to wait for @CommandMC to share their thoughts on the general strategy. In the meantime, here's what I found:

  1. Critical bugs found (by CodeQL):
  • Command injection (critical): src/backend/utils.ts spawns a shell to expand variables. A malicious path could execute arbitrary commands.
  • XSS/phishing (high): WebView allows open redirects via query parameters.
  1. What the tools do:
  • DevSkim: Performs static analysis for unsafe coding patterns (e.g. weak cryptography).
  • OSV-Scanner: Checks dependencies for known vulnerabilities.

I have logged the specific bugs as issues so that we can track them. I'll hold off on the rest until we agree on the plan!

@arielj
Copy link
Collaborator

arielj commented Dec 30, 2025

I have to ask, are these comments being generated by an AI? cause it sounds like it

@arielj
Copy link
Collaborator

arielj commented Dec 30, 2025

what's the difference between OSV-Scanner (that checks known vulnerabilities) and pnpm audit which also checks known vulnerabilities?

@arielj
Copy link
Collaborator

arielj commented Dec 30, 2025

I have logged the specific bugs as issues so that we can track them

can you link those issues? there are not issues created by you in this repo

@CommandMC
Copy link
Member

Critical bugs found (by CodeQL):

If these are the "bugs" found by CodeQL, I do not want to integrate it here

Command injection (critical): src/backend/utils.ts spawns a shell to expand variables. A malicious path could execute arbitrary commands.

  1. Please point to the exact line this issue is on. We are not specifying the shell property in any calls to exec/spawn
  2. Heroic runs locally on your own machine, so the user being able to run commands by specifying a malicious path is a non-issue

XSS/phishing (high): WebView allows open redirects via query parameters.

  1. I'm not sure I understand the issue. If a website wants to redirect you to another one, it can just do that. If a website wants to open a new window, we explicitly allow that (since it's required for alternative identity providers on login)
  2. We open fixed, safe URLs (e.g. the Epic/GOG/Amazon storefronts) in Webviews. They are not going to do anything malicious. If something else is compromised on your system (e.g. you cannot trust your DNS server), that's not something Heroic can fix

Please take some time to actually use the launcher before reporting security issues

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