Skip to content

Conversation

@garland3
Copy link

@garland3 garland3 commented Oct 7, 2025

This PR addresses three critical security issues:\n\n- Supply chain integrity: Verified generator JAR downloads using SHA-256 (fallback SHA-1). If checksums mismatch, delete the artifact and error.\n- CLI hardening: Sanitize additionalArguments with an allowlist and forbidden token checks. Integrated into command builders.\n- Server concurrency: Refactor mws.Application to use per-request local state, removing shared mutable request/response index.\n\nAlso adds a unit test suite (testSecurityHardening.m) covering checksum computation, CLI sanitization, command building integration, and server state isolation; includes a network-guarded checksum verification test.\n\nNotes:\n- Follows repo conventions; minimal API change.\n- Backwards compatible for typical usage; will error on dangerous CLI arguments.\n- Ready for review and testing in separate environment.

…e server per-request state; add unit tests
@brownemi
Copy link
Member

@garland3 Thanks so much for the PR. For various reasons we won't accept it verbatim but will address the concepts in the coming releases.

  1. Checksum comparison & download, this is used by setup and is documented as deprecated and typically should no longer be used, it is inadequate as is. We'll move forward a 4.0.0 release that will remove setup and this feature in favour of the builtin maven capability.

  2. The CLI sanitation is vital were this to be exposed e.g. via web service or a CI/CD process. Some minor character quibbles apart it makes sense but I think more from an error prevention perspective than robust security. As the MATLAB is a wrapper to the underlying Java libraries which I would also not expect to be robust to injection. We'll add some docs to point out that this should ONLY be exposed to trusted input including the spec itself.

  3. The Application.m suggestion is really interesting and aligns with some other work in our pipeline. We'll come back to this probably in November...

  4. A PR with tests - awesome!

I'll leave the issue open until 1 & 2 are addressed in the coming days, thanks again.

@garland3
Copy link
Author

Great. Good luck. Let me know if you need anything

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