Skip to content

Add security CI workflow#684

Draft
taylorbell57 wants to merge 2 commits into
mainfrom
add-security-ci
Draft

Add security CI workflow#684
taylorbell57 wants to merge 2 commits into
mainfrom
add-security-ci

Conversation

@taylorbell57

Copy link
Copy Markdown
Contributor

Summary

  • add a security workflow with pip-audit and Bandit jobs
  • pin GitHub Actions by commit SHA
  • fix high-severity Bandit findings and simple medium findings

Testing

  • python -m py_compile exoctk/utils.py exoctk/contam_visibility/make_contam_traces.py exoctk/modelgrid.py
  • python -c "import yaml, pathlib; yaml.safe_load(pathlib.Path('.github/workflows/security.yml').read_text())"
  • /Users/tbell/miniconda3/envs/exoctk/bin/python -m bandit -r exoctk -x exoctk/tests -lll

@taylorbell57

Copy link
Copy Markdown
Contributor Author

Resolves #567

@taylorbell57 taylorbell57 linked an issue Jun 16, 2026 that may be closed by this pull request
@york-stsci

Copy link
Copy Markdown
Collaborator

Looking at the bandit results, in order,

  1. This is, in fact, not an issue. The potential issue is that in theory someone could put an arbitrary string that does anything SQL-based in the TAP query. However, the vectors for that are RA/DEC (which are limited to floating-point values). However, these values are taken from a coordinate object, and converted using astropy unit conversion, so they will always be floating-point numbers. If they weren't, the code would crash before it got to this point.
  2. Same. More so, the function here is only called by a test function. There is no way to call it from the website.
  3. See line 41
  4. See line 41
  5. See line 41
  6. See line 41
  7. Yes, this is a potential issue. It's not easy to exploit, because you would need to wait for a query to be running (okay, easy enough), get that query's task ID (same), and then break in to the container or VM and write files with the appropriate names that are malicious and then don't get overwritten but do get loaded on task completion. If you are already in the VM with permission to write arbitrary files, using that access to compromise the web server would be unlikely to give you substantially more access than you already have. The reason pickle is used here is that I wasn't able to get any non-pickle form of serialization to work with the data, but we could give that another try.
  8. Same as 115
  9. Same as 115
  10. Same as 115
  11. Yes, we are doing that. I don't know how to fix it while still letting the website be seen at all.
  12. This isn't guaranteed to be text-based data, but it's still coming out of a table that's generated internally, and the potentially compromised database is the logging database sqlite file, which wouldn't really hurt us.
  13. Same as 164
  14. Again, this requires the attacker to have access to the filesystem already. I don't know whether we could serialize this in non-binary form, but if we can that might be good.

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.

Libraries with vulnerabilities should be updated

2 participants