Add cryptographic algorithm scanning and CycloneDX 1.7 CBOM output#1656
Draft
CortezFrazierJr wants to merge 10 commits intofossas:masterfrom
Draft
Add cryptographic algorithm scanning and CycloneDX 1.7 CBOM output#1656CortezFrazierJr wants to merge 10 commits intofossas:masterfrom
CortezFrazierJr wants to merge 10 commits intofossas:masterfrom
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds a new Rust workspace member 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/App/Fossa/Analyze.hs (1)
451-459:⚠️ Potential issue | 🟠 MajorFlush crypto diagnostics alongside other analyzer results.
maybeCryptoScanResultsis added to analysis but not included in theDiag.flushLogstraversal, so crypto warnings/errors may be missed in normal output.🔧 Suggested fix
traverse_ (Diag.flushLogs SevError SevDebug) [manualSrcUnits, binarySearchResults, dynamicLinkedResults] -- Flush logs using the original Result from VSI. traverse_ (Diag.flushLogs SevError SevDebug) [vsiResults] -- Flush logs from lernie traverse_ (Diag.flushLogs SevError SevDebug) [maybeLernieResults] -- Flush logs from ficus traverse_ (Diag.flushLogs SevError SevDebug) [maybeFicusResults] + -- Flush logs from crypto scan + traverse_ (Diag.flushLogs SevError SevDebug) [maybeCryptoScanResults]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/App/Fossa/Analyze.hs` around lines 451 - 459, Add maybeCryptoScanResults to the diagnostic flushes so crypto warnings/errors are emitted: locate the Diag.flushLogs traversals around vsiResults, maybeLernieResults, and maybeFicusResults in Analyze.hs and include maybeCryptoScanResults in the appropriate traversal (or add a new traverse_ (Diag.flushLogs SevError SevDebug) [maybeCryptoScanResults]) so crypto scan results are flushed just like maybeFicusResults and maybeLernieResults.
🧹 Nitpick comments (17)
test-fixtures/csharp-service/CryptoService.cs (3)
18-26: Authentication tag is discarded.The GCM authentication tag is computed but not returned, making this pattern incomplete for demonstrating proper authenticated encryption. For a more representative fixture, consider returning both the ciphertext and tag.
Suggested improvement
- public static byte[] EncryptAesGcm(byte[] key, byte[] plaintext, byte[] nonce) + public static (byte[] ciphertext, byte[] tag) EncryptAesGcm(byte[] key, byte[] plaintext, byte[] nonce) { var ciphertext = new byte[plaintext.Length]; var tag = new byte[16]; using var gcm = new AesGcm(key, 16); gcm.Encrypt(nonce, plaintext, ciphertext, tag); - return ciphertext; + return (ciphertext, tag); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test-fixtures/csharp-service/CryptoService.cs` around lines 18 - 26, The EncryptAesGcm method computes a 16-byte GCM tag but discards it; update EncryptAesGcm to return both ciphertext and tag (either as a tuple (byte[] ciphertext, byte[] tag) or a single byte[] with tag appended) and adjust the implementation to allocate/return the tag alongside ciphertext (e.g., allocate ciphertext and tag buffers, call gcm.Encrypt(nonce, plaintext, ciphertext, tag), then return both), keeping the method name EncryptAesGcm to locate the change.
1-3: Unused import:System.Text.No types from
System.Text(e.g.,Encoding,StringBuilder) are used in this file.Suggested fix
using System; using System.Security.Cryptography; -using System.Text;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test-fixtures/csharp-service/CryptoService.cs` around lines 1 - 3, Remove the unused using directive for System.Text from CryptoService.cs; locate the top-of-file usings (including System and System.Security.Cryptography) and delete the line "using System.Text;" ensuring no code in the CryptoService class references Encoding or StringBuilder first—if any method uses those types, replace with appropriate alternatives or add the using back only where needed.
7-95: Consider adding additional crypto patterns for parity with the Java fixture.The Java fixture includes several patterns not present here that may be useful for comprehensive scanner testing:
- AES-ECB (FIPS Deprecated) - an insecure mode the scanner should detect
- Ed25519 - modern signature algorithm (available in .NET 5+)
These additions would ensure consistent coverage across ecosystem fixtures for the crypto detection engine.
Example additions
// --- AES-ECB (FIPS Deprecated - insecure) --- public static byte[] EncryptAesEcb(byte[] key, byte[] plaintext) { using var aes = Aes.Create(); aes.Key = key; aes.Mode = CipherMode.ECB; aes.Padding = PaddingMode.PKCS7; using var encryptor = aes.CreateEncryptor(); return encryptor.TransformFinalBlock(plaintext, 0, plaintext.Length); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test-fixtures/csharp-service/CryptoService.cs` around lines 7 - 95, Add AES-ECB and Ed25519 examples to CryptoHelper so the C# fixture matches the Java one: implement a new EncryptAesEcb(byte[] key, byte[] plaintext) method that creates an Aes instance, sets Mode = CipherMode.ECB and Padding = PaddingMode.PKCS7, uses CreateEncryptor/TransformFinalBlock to return ciphertext; and add an Ed25519 example (e.g., GenerateEd25519Key() and a SignEd25519/VerifyEd25519 pair or a single GenerateAndSignEd25519 sample) that uses the platform Ed25519 APIs to generate a keypair and produce/verify a signature; place both methods in the CryptoHelper class alongside existing methods (EncryptAes, EncryptAesGcm, GenerateRsaKey, etc.) so the scanner sees the insecure AES-ECB pattern and the modern Ed25519 pattern.test-fixtures/java-microservice/src/main/java/com/example/CryptoService.java (3)
20-27: AES-GCM: IV must be returned alongside ciphertext for decryption.The method generates a random IV but only returns the ciphertext. Without the IV, decryption is impossible. For a complete demonstration (even as a test fixture), prepend the IV to the output.
Suggested fix
public byte[] encryptAesGcm(byte[] key, byte[] plaintext) throws Exception { Cipher cipher = Cipher.getInstance("AES/GCM/NoPadding"); byte[] iv = new byte[12]; new SecureRandom().nextBytes(iv); GCMParameterSpec spec = new GCMParameterSpec(128, iv); cipher.init(Cipher.ENCRYPT_MODE, new SecretKeySpec(key, "AES"), spec); - return cipher.doFinal(plaintext); + byte[] ciphertext = cipher.doFinal(plaintext); + byte[] result = new byte[iv.length + ciphertext.length]; + System.arraycopy(iv, 0, result, 0, iv.length); + System.arraycopy(ciphertext, 0, result, iv.length, ciphertext.length); + return result; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test-fixtures/java-microservice/src/main/java/com/example/CryptoService.java` around lines 20 - 27, encryptAesGcm currently generates a random IV but only returns ciphertext, making decryption impossible; modify the encryptAesGcm method to return both IV and ciphertext by prepending the 12-byte iv to the ciphertext before returning (use the existing iv and cipher.doFinal(plaintext)), and ensure the caller/decryptor knows to slice off the first 12 bytes as the GCM IV when decrypting (references: encryptAesGcm, iv, GCMParameterSpec, Cipher.doFinal).
64-71: DH and X25519 methods only instantiateKeyAgreementwithout performing actual key exchange.These methods create the
KeyAgreementobject but don't demonstrate the full key agreement flow (init, doPhase, generateSecret). For scanner pattern detection this is sufficient, but if this fixture is also intended as a reference, consider completing the flow.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test-fixtures/java-microservice/src/main/java/com/example/CryptoService.java` around lines 64 - 71, The methods performDhKeyAgreement and performX25519KeyAgreement currently only call KeyAgreement.getInstance and must be expanded to perform a full key-exchange flow: generate a key pair for the local side (use KeyPairGenerator for "DH" and "X25519"), obtain/construct a peer PublicKey (you can generate a second key pair to simulate a peer), call ka.init(localPrivateKey), call ka.doPhase(peerPublicKey, true), and call ka.generateSecret(...) to derive the shared secret; ensure to handle algorithm-specific parameter steps for DH (parameters/spec) and use the appropriate KeyPairGenerator/KeyFactory for X25519 so the code in performDhKeyAgreement and performX25519KeyAgreement demonstrates init, doPhase, and generateSecret end-to-end.
98-103: Consider clearingPBEKeySpecafter use to prevent password from lingering in memory.
PBEKeySpecinternally stores the password as a char array. Callingspec.clearPassword()after key derivation zeroes out the sensitive data.Suggested fix
public byte[] deriveKeyPbkdf2(char[] password, byte[] salt) throws Exception { SecretKeyFactory factory = SecretKeyFactory.getInstance("PBKDF2WithHmacSHA256"); PBEKeySpec spec = new PBEKeySpec(password, salt, 600000, 256); - return factory.generateSecret(spec).getEncoded(); + try { + return factory.generateSecret(spec).getEncoded(); + } finally { + spec.clearPassword(); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test-fixtures/java-microservice/src/main/java/com/example/CryptoService.java` around lines 98 - 103, In deriveKeyPbkdf2, ensure the PBEKeySpec's internal password is zeroed after use by calling spec.clearPassword() in a finally block (or equivalent try/finally) so it always runs even on exceptions; wrap the call to factory.generateSecret(spec).getEncoded() in try and invoke spec.clearPassword() in finally to prevent the password from lingering in memory.test-fixtures/python-web-app/requirements.txt (1)
1-5: Consider pinning exact versions for test reproducibility.Using
>=constraints without upper bounds can lead to non-reproducible test results when new package versions are released. For test fixtures, pinned versions (e.g.,cryptography==42.0.0) ensure consistent behavior across test runs and environments.📌 Suggested approach for version pinning
-cryptography>=42.0.0 -bcrypt>=4.1.0 -argon2-cffi>=23.1.0 -PyJWT>=2.8.0 -flask>=3.0.0 +cryptography==42.0.0 +bcrypt==4.1.0 +argon2-cffi==23.1.0 +PyJWT==2.8.0 +flask==3.0.0This approach ensures that tests behave identically across all environments and over time.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test-fixtures/python-web-app/requirements.txt` around lines 1 - 5, Replace loose >= version specifiers in the test requirements with exact pins to guarantee reproducible test runs; update each dependency line (cryptography, bcrypt, argon2-cffi, PyJWT, flask) to use exact equality pins (e.g., change "cryptography>=42.0.0" to "cryptography==42.0.0") and ensure the requirements.txt only contains the exact versions you validated in CI.test-fixtures/ruby-web-app/app.rb (1)
17-41: Minor: Encryption methods missing IV in return values.Unlike
encrypt_aes_gcmwhich correctly returns{ iv:, data:, tag: }, the other encryption methods generate an IV but don't return it, making decryption impossible. Additionally,encrypt_3desdoesn't generate an IV at all (defaulting to a zero IV).For a test fixture designed for crypto pattern detection, this is acceptable—the scanner will detect the algorithm usage regardless. However, if these are also meant to serve as reference implementations, consider aligning them with the GCM method's complete return structure.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test-fixtures/ruby-web-app/app.rb` around lines 17 - 41, encrypt_aes_cbc, encrypt_chacha, and encrypt_3des currently either discard the generated IV or never generate one, preventing decryption; update each function to generate and return the IV (use cipher.random_iv) and return a consistent hash like { iv:, data:, tag: }—for encrypt_aes_cbc and encrypt_3des return { iv: iv, data: encrypted_data, tag: nil } (CBC/3DES have no auth tag), and for encrypt_chacha return { iv: iv, data: encrypted_data, tag: cipher.auth_tag } after finalizing so callers can decrypt correctly.test-fixtures/php-api/crypto.php (2)
15-24: Consider returning IV alongside CBC/3DES ciphertext for realism.Right now the IV is generated but discarded. Returning it would make these fixture examples closer to executable/round-trip patterns.
Suggested fixture-oriented tweak
-function encrypt_aes_cbc(string $key, string $plaintext): string { +function encrypt_aes_cbc(string $key, string $plaintext): array { $iv = openssl_random_pseudo_bytes(16); - return openssl_encrypt($plaintext, 'aes-256-cbc', $key, OPENSSL_RAW_DATA, $iv); + return ['iv' => $iv, 'data' => openssl_encrypt($plaintext, 'aes-256-cbc', $key, OPENSSL_RAW_DATA, $iv)]; } -function encrypt_3des(string $key, string $plaintext): string { +function encrypt_3des(string $key, string $plaintext): array { $iv = openssl_random_pseudo_bytes(8); - return openssl_encrypt($plaintext, 'des-ede3-cbc', $key, OPENSSL_RAW_DATA, $iv); + return ['iv' => $iv, 'data' => openssl_encrypt($plaintext, 'des-ede3-cbc', $key, OPENSSL_RAW_DATA, $iv)]; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test-fixtures/php-api/crypto.php` around lines 15 - 24, Both encrypt_aes_cbc and encrypt_3des currently generate an IV but drop it; change them to return the IV together with the ciphertext (e.g., return a two-element structure or concatenate IV+ciphertext) so fixtures support realistic round-trip decryption. Update encrypt_aes_cbc and encrypt_3des to include the generated $iv in the return value and adjust callers/tests to expect and use the IV for decryption.
54-60: Qualify the Ed25519 FIPS comment for clarity.The "FIPS Approved" label is technically correct at the algorithm/standard level (Ed25519 is approved via FIPS 186-5 as of February 2023), but the term can be ambiguous in practice. Module validation and runtime implementation are separate considerations—many platforms still require Ed25519 to be explicitly validated in the module or enabled in the runtime to claim FIPS compliance. A slightly qualified comment (e.g., "FIPS-approved algorithm per FIPS 186-5") would make the distinction clear in fixture documentation without overstating the claim.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test-fixtures/php-api/crypto.php` around lines 54 - 60, Update the comment above generate_signing_keypair() to qualify the FIPS claim: change "FIPS Approved" to a clearer phrase such as "FIPS-approved algorithm per FIPS 186-5 (module/runtime validation may be required)" so the fixture notes that Ed25519 is FIPS-approved at the algorithm/standard level but that module validation or runtime configuration may still be necessary; leave the X25519 comment and the function names generate_signing_keypair and generate_box_keypair unchanged.test-fixtures/node-auth-service/auth.js (1)
1-3: Either exercise the declared crypto libraries or remove them from package.json.This fixture declares
argon2,jose, andnode-forgeinpackage.jsonbut doesn't import or use them—onlybcryptand built-incryptoare exercised. This creates ambiguity about fixture intent and may leave API-call detection coverage incomplete. Add a minimal call per unused library (lines 1–3) and export it (lines 98–103), or remove unused dependencies.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test-fixtures/node-auth-service/auth.js` around lines 1 - 3, The fixture declares but doesn't exercise argon2, jose, and node-forge; require each module at the top (e.g., const argon2 = require('argon2'), const { SignJWT } = require('jose'), const forge = require('node-forge')) and add a tiny usage for each (e.g., an async function that hashes a test string with argon2, a function that creates a minimal JWT with SignJWT, and a function that generates a small RSA key or fingerprint with forge); then export those helper functions alongside existing exports (referencing the new argon2/hash function name, the SignJWT wrapper, and the forge helper) so the libraries are actually exercised and detectable by API-call coverage.test-fixtures/node-auth-service/package.json (1)
5-8: Consider pinning fixture dependency versions for reproducibility.While the scanner detects packages by name from the manifest, floating version ranges in test fixtures can make results less predictable across environments. Pinning exact versions is a best practice for fixtures, even though the current tests don't depend on specific versions.
Suggested change
"dependencies": { - "bcrypt": "^5.1.0", - "argon2": "^0.31.0", - "jose": "^5.2.0", - "node-forge": "^1.3.1" + "bcrypt": "5.1.0", + "argon2": "0.31.0", + "jose": "5.2.0", + "node-forge": "1.3.1" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test-fixtures/node-auth-service/package.json` around lines 5 - 8, The fixture's package.json lists floating dependency ranges for bcrypt, argon2, jose, and node-forge which harms reproducibility; update those entries in the dependencies block (the lines containing "bcrypt", "argon2", "jose", and "node-forge") to pinned exact versions (remove the caret and specify the concrete version numbers you want to lock to), then regenerate the lockfile (npm/yarn pnpm) so the fixture and its lockfile remain deterministic across environments.src/App/Fossa/EmbeddedBinary.hs (1)
246-253: Add explicit cross-platform tests for the new embedded binary path.Given the new OS-conditional
embeddedBinaryCryptoScanhandling, add/extend tests that validate extraction and invocation on both Unix and Windows paths.As per coding guidelines, "Code should be thoroughly tested with appropriate unit tests".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/App/Fossa/EmbeddedBinary.hs` around lines 246 - 253, Add unit tests exercising the OS-conditional embeddedBinaryCryptoScan path: write tests that simulate both mingw32_HOST_OS and non-Windows builds to ensure embedFileIfExists is called with the correct file name ("target/release/cryptoscan.exe" for Windows and "target/release/cryptoscan" for Unix), and verify extraction/invocation behavior (e.g., writing to temp file and executing or returning expected ByteString). Use build-time CPP simulation or compile-time test modules to force each branch, mock or stub embedFileIfExists to return known ByteStrings, and assert the downstream logic that consumes embeddedBinaryCryptoScan handles both paths correctly; add these tests alongside EmbeddedBinary.hs tests and name them to reflect Windows vs Unix scenarios.extlib/cryptoscan/src/cyclonedx.rs (1)
206-233: Stabilize CBOM ordering to avoid nondeterministic output.
HashMap/HashSetiteration here can reorder libraries andprovidesentries between runs. Sorting before emit will make output reproducible and test-friendly.♻️ Proposed fix
- for (lib_name, algo_refs) in &library_algorithms { + let mut libs: Vec<_> = library_algorithms.iter().collect(); + libs.sort_by(|a, b| a.0.cmp(b.0)); + for (lib_name, algo_refs) in libs { if seen_libs.contains(lib_name) { continue; } seen_libs.insert(lib_name.clone()); @@ - let unique_algos: Vec<String> = algo_refs - .iter() - .collect::<HashSet<_>>() - .into_iter() - .cloned() - .collect(); + let mut unique_algos: Vec<String> = algo_refs + .iter() + .cloned() + .collect::<HashSet<_>>() + .into_iter() + .collect(); + unique_algos.sort(); @@ - CycloneDxBom { + components.sort_by(|a, b| a.bom_ref.cmp(&b.bom_ref)); + dependencies.sort_by(|a, b| a.dep_ref.cmp(&b.dep_ref)); + + CycloneDxBom {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extlib/cryptoscan/src/cyclonedx.rs` around lines 206 - 233, Iterate libraries in a deterministic order and sort provided-algorithm lists before pushing components/dependencies: instead of iterating library_algorithms directly, collect its keys (or entries) into a Vec, sort that Vec, then loop over the sorted entries (use the existing symbols library_algorithms, seen_libs, lib_name, algo_refs, components, dependencies, BomComponent, BomDependency); produce unique_algos by collecting into a HashSet as before but then convert to a Vec and sort it before assigning to provides so the provides order is stable; remove or simplify seen_libs if you rely on the sorted keys to avoid duplicates.test-fixtures/go-api-server/main.go (1)
100-104: Ignored error fromblake2b.New256.The error return from
blake2b.New256(nil)is discarded. While passingnilfor unkeyed hashing won't error, ignoring errors sets a poor example. Consider handling it or using a comment to explain.♻️ Suggested fix
func HashBLAKE2b(data []byte) []byte { - h, _ := blake2b.New256(nil) + h, _ := blake2b.New256(nil) // nil key for unkeyed hash; error is always nil h.Write(data) return h.Sum(nil) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test-fixtures/go-api-server/main.go` around lines 100 - 104, The call to blake2b.New256(nil) in HashBLAKE2b ignores its error return; update HashBLAKE2b to either (A) handle and propagate the error (change signature to HashBLAKE2b(data []byte) ([]byte, error), check err from blake2b.New256 and return it) or (B) if you intentionally rely on nil being safe, add a clear inline comment and assert the error cannot occur (check err and panic/log.Fatalf with a clear message) so the ignored error is explicit; reference the blake2b.New256 call and the HashBLAKE2b function when making the change.extlib/cryptoscan/tests/integration_test.rs (1)
45-46: Redundant double deserialization.
serde_json::from_slicealready returns aValue, so wrapping it withserde_json::from_valueis unnecessary and performs redundant work.♻️ Suggested fix
- serde_json::from_value(serde_json::from_slice(&output.stdout).expect("invalid JSON")) - .expect("invalid CycloneDX structure") + serde_json::from_slice(&output.stdout).expect("invalid CycloneDX JSON")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extlib/cryptoscan/tests/integration_test.rs` around lines 45 - 46, The code is doing redundant double deserialization by calling serde_json::from_value(serde_json::from_slice(&output.stdout).expect("invalid JSON")).expect("invalid CycloneDX structure"); remove the intermediate from_value step and directly deserialize the bytes from output.stdout into the target CycloneDX structure using serde_json::from_slice(&output.stdout).expect("invalid CycloneDX structure") (i.e., replace the nested call so you only call serde_json::from_slice on output.stdout and handle errors with the final expect), updating any type annotations if necessary to match the expected CycloneDX type.src/App/Fossa/CryptoScan/FipsReport.hs (1)
246-249: UseData.Text.Read.decimalfor Text-native RSA key size parsing.The current implementation uses
reads (Text.unpack ps) :: [(Int, String)], which converts Text to String before parsing. Replace withData.Text.Read.decimalto parse directly on Text. Update the pattern match to check if the unconsumed text is empty (equivalent to the current"").Add
Data.Text.Readto imports if not already present.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/App/Fossa/CryptoScan/FipsReport.hs` around lines 246 - 249, The Text parsing for RSA key sizes should use Data.Text.Read.decimal instead of reads on Text.unpack; in the case that inspects ps replace the reads-based pattern match with decimal (from Data.Text.Read) and match its result form (Right (n, rest) / Left _) and require rest == "" (or Text.null rest) to ensure no unconsumed input; also add Data.Text.Read to the module imports so the decimal function is available.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@extlib/cryptoscan/src/main.rs`:
- Around line 50-53: The match currently treats any unknown cli.format as
cyclonedx (due to the `| _` fallback), silently accepting invalid values; change
the CLI parsing to a typed enum implementing clap::ValueEnum (e.g., a new Format
enum used by `cli.format`) so clap validates allowed values at parse time, then
update the match in main (the `match cli.format.as_str()` block that calls
`serde_json::to_string_pretty(&findings)` and
`cyclonedx::to_cyclonedx_bom(&findings)`) to exhaustively handle only the
defined enum variants (JSON and CycloneDX) and remove the wildcard fallback so
invalid formats are rejected by clap before runtime.
In `@extlib/cryptoscan/src/scanner.rs`:
- Around line 158-160: The deduplication currently retains the first finding per
key, which can preserve lower-confidence items; update the logic in
deduplicate_findings to iterate findings and for each (file_path, algorithm) key
use a HashMap to store the highest-confidence Finding seen (compare Confidence
via a small ranking function or implement Ord for Confidence) and replace the
stored entry when a finding with higher confidence is encountered, then return
the deduplicated values; apply the same pattern to the other similar block
referenced around the 280-296 region and reference the Confidence enum and
deduplicate_findings/Finding key when making the change.
- Around line 184-198: pattern_applies currently only matches by file extension
and therefore skips extensionless manifest files (e.g., Gemfile, Podfile,
Pipfile); update the logic in pattern_applies to treat an empty file_ext as a
valid match when the pattern.file_extensions list contains an empty string (or
other sentinel used for extensionless manifests). Concretely, change the final
check on pattern.file_extensions to return true if any ext == file_ext OR
(file_ext.is_empty() && pattern.file_extensions.iter().any(|ext|
ext.is_empty())), keeping the existing ecosystem match behavior in
pattern_applies.
In `@src/App/Fossa/Analyze.hs`:
- Around line 425-435: The current code collapses scanner Failure and Success
Nothing into Nothing (via join . resultToMaybe), which makes errors appear as
"No crypto findings"; change the handling of the analyzeCryptoScanCBOM result so
you explicitly distinguish Failure from Success Nothing: pattern match on the
diagnostic/Result value returned by Diag.errorBoundaryIO (the maybeCbomBytes
variable/result of analyzeCryptoScanCBOM) and, on Failure, log the error details
(include the diagnostic/error message) and propagate or handle the failure
instead of treating it as "no findings"; on Success (Just bytes) continue to
write the CBOM (sendIO BL.writeFile cbomPath) and on Success Nothing log "No
crypto findings"; apply the same change to the analogous block referenced at
lines 529-534.
In `@src/App/Fossa/Analyze/ScanSummary.hs`:
- Around line 335-338: summarizeCryptoScan's Success branch ignores the `wg`
warnings value and always emits a plain success line; update the branch in
summarizeCryptoScan (pattern matching Success _ (Just (CryptoScanResults
findings))) to also pattern-match or reference `wg` (the warnings list) and,
when `wg` is non-empty, emit a warning-styled summary (e.g., using the same
warningColorCoded/ itemize pattern you use elsewhere) and render the warnings
(use renderCryptoFinding or a dedicated render for warnings) instead of only
showing a bare success message, otherwise keep the normal success output for
empty `wg`.
In `@src/App/Fossa/CryptoScan/FipsReport.hs`:
- Around line 52-53: The current dedupe uses only cryptoAlgorithmName via nubBy,
which collapses distinct variants (e.g., RSA with different key sizes) and can
hide non-compliant items; change the dedupe to use a full algorithm identity
instead of just name — for example compare the complete cryptoFindingAlgorithm
values or build a dedupe key that includes name plus relevant parameters (key
size/mode) and use that in place of the current nubBy; update the same logic
around uniqueAlgos and any similar blocks (lines 65–77) so that
cryptoAlgorithmFipsStatus and related outputs are computed per unique algorithm
variant, not just name.
In `@test-fixtures/elixir-phoenix-app/crypto.ex`:
- Around line 7-30: The AEAD and 3DES calls are using the wrong :crypto APIs;
update encrypt_aes_gcm and encrypt_aes_128_gcm to call
:crypto.crypto_one_time_aead/6 (or /7 if you need the explicit return format)
supplying cipher, key, iv, plaintext, additional_data (nil or ""), and return
the {ciphertext, tag} as appropriate, and update encrypt_chacha to use
:crypto.crypto_one_time_aead/6 with :chacha20_poly1305 (passing AAD and getting
{ciphertext, tag}); replace encrypt_3des (which uses :crypto.block_encrypt) with
:crypto.crypto_one_time/5 for :des_ede3_cbc providing cipher, key, iv,
plaintext, and encrypt flag true so it performs CBC 3DES encryption and returns
the ciphertext.
In `@test-fixtures/php-api/crypto.php`:
- Around line 47-86: The functions currently declare strict return types but
call PHP crypto functions that can return false; update generate_rsa_key (calls
openssl_pkey_new), encrypt_aes_gcm / encrypt_aes_cbc / encrypt_3des (calls
openssl_encrypt and openssl_random_pseudo_bytes) to detect a false return and
fail-fast: check the result of openssl_pkey_new(), openssl_encrypt(), and
openssl_random_pseudo_bytes() and throw a clear Exception (or return a
controlled error value) if they return false so the declared return types
(OpenSSLAsymmetricKey or string) are never violated; keep the same function
signatures but add the guard/exception handling around those calls (refer to
functions generate_rsa_key, encrypt_aes_gcm, encrypt_aes_cbc, encrypt_3des and
the called APIs openssl_pkey_new, openssl_encrypt, openssl_random_pseudo_bytes).
In `@test-fixtures/ruby-web-app/app.rb`:
- Around line 74-76: The generate_ec_key method currently returns an EC object
with curve configured but no keypair because it calls
OpenSSL::PKey::EC.new('prime256v1'); update generate_ec_key to produce a real
keypair by invoking the modern API OpenSSL::PKey::EC.generate('prime256v1') (or,
if you must support older Ruby, call generate_key! on the instance returned by
OpenSSL::PKey::EC.new('prime256v1')), ensuring the method returns an object with
a generated private key.
In `@test-fixtures/rust-crypto-tool/src/main.rs`:
- Around line 57-59: The code uses rand::rngs::OsRng when creating `let mut
csprng = rand::rngs::OsRng;` before calling `SigningKey::generate(&mut csprng)`,
but OsRng is not imported; add the appropriate import (e.g., `use
rand::rngs::OsRng;`) at the top of the file or switch to `use rand_core::OsRng;`
if your Cargo.toml uses rand_core, so `csprng` is a recognized type when passed
into `SigningKey::generate`.
In `@test-fixtures/swift-ios-app/CryptoManager.swift`:
- Around line 81-96: In deriveKeyPbkdf2, don't ignore CCKeyDerivationPBKDF's
return status and avoid force-unwrapping unsafe pointers; instead unwrap
salt.withUnsafeBytes and derivedKey.withUnsafeMutableBytes baseAddress with
guard-let (or use if-let), call CCKeyDerivationPBKDF while capturing its Int32
status, and handle non-kCCSuccess by throwing or returning an error/empty Data
as appropriate for the test fixture; reference the function name
deriveKeyPbkdf2, the CCKeyDerivationPBKDF call, and the
saltBytes.baseAddress/derivedBytes.baseAddress pointers when making these
changes.
---
Outside diff comments:
In `@src/App/Fossa/Analyze.hs`:
- Around line 451-459: Add maybeCryptoScanResults to the diagnostic flushes so
crypto warnings/errors are emitted: locate the Diag.flushLogs traversals around
vsiResults, maybeLernieResults, and maybeFicusResults in Analyze.hs and include
maybeCryptoScanResults in the appropriate traversal (or add a new traverse_
(Diag.flushLogs SevError SevDebug) [maybeCryptoScanResults]) so crypto scan
results are flushed just like maybeFicusResults and maybeLernieResults.
---
Nitpick comments:
In `@extlib/cryptoscan/src/cyclonedx.rs`:
- Around line 206-233: Iterate libraries in a deterministic order and sort
provided-algorithm lists before pushing components/dependencies: instead of
iterating library_algorithms directly, collect its keys (or entries) into a Vec,
sort that Vec, then loop over the sorted entries (use the existing symbols
library_algorithms, seen_libs, lib_name, algo_refs, components, dependencies,
BomComponent, BomDependency); produce unique_algos by collecting into a HashSet
as before but then convert to a Vec and sort it before assigning to provides so
the provides order is stable; remove or simplify seen_libs if you rely on the
sorted keys to avoid duplicates.
In `@extlib/cryptoscan/tests/integration_test.rs`:
- Around line 45-46: The code is doing redundant double deserialization by
calling
serde_json::from_value(serde_json::from_slice(&output.stdout).expect("invalid
JSON")).expect("invalid CycloneDX structure"); remove the intermediate
from_value step and directly deserialize the bytes from output.stdout into the
target CycloneDX structure using
serde_json::from_slice(&output.stdout).expect("invalid CycloneDX structure")
(i.e., replace the nested call so you only call serde_json::from_slice on
output.stdout and handle errors with the final expect), updating any type
annotations if necessary to match the expected CycloneDX type.
In `@src/App/Fossa/CryptoScan/FipsReport.hs`:
- Around line 246-249: The Text parsing for RSA key sizes should use
Data.Text.Read.decimal instead of reads on Text.unpack; in the case that
inspects ps replace the reads-based pattern match with decimal (from
Data.Text.Read) and match its result form (Right (n, rest) / Left _) and require
rest == "" (or Text.null rest) to ensure no unconsumed input; also add
Data.Text.Read to the module imports so the decimal function is available.
In `@src/App/Fossa/EmbeddedBinary.hs`:
- Around line 246-253: Add unit tests exercising the OS-conditional
embeddedBinaryCryptoScan path: write tests that simulate both mingw32_HOST_OS
and non-Windows builds to ensure embedFileIfExists is called with the correct
file name ("target/release/cryptoscan.exe" for Windows and
"target/release/cryptoscan" for Unix), and verify extraction/invocation behavior
(e.g., writing to temp file and executing or returning expected ByteString). Use
build-time CPP simulation or compile-time test modules to force each branch,
mock or stub embedFileIfExists to return known ByteStrings, and assert the
downstream logic that consumes embeddedBinaryCryptoScan handles both paths
correctly; add these tests alongside EmbeddedBinary.hs tests and name them to
reflect Windows vs Unix scenarios.
In `@test-fixtures/csharp-service/CryptoService.cs`:
- Around line 18-26: The EncryptAesGcm method computes a 16-byte GCM tag but
discards it; update EncryptAesGcm to return both ciphertext and tag (either as a
tuple (byte[] ciphertext, byte[] tag) or a single byte[] with tag appended) and
adjust the implementation to allocate/return the tag alongside ciphertext (e.g.,
allocate ciphertext and tag buffers, call gcm.Encrypt(nonce, plaintext,
ciphertext, tag), then return both), keeping the method name EncryptAesGcm to
locate the change.
- Around line 1-3: Remove the unused using directive for System.Text from
CryptoService.cs; locate the top-of-file usings (including System and
System.Security.Cryptography) and delete the line "using System.Text;" ensuring
no code in the CryptoService class references Encoding or StringBuilder first—if
any method uses those types, replace with appropriate alternatives or add the
using back only where needed.
- Around line 7-95: Add AES-ECB and Ed25519 examples to CryptoHelper so the C#
fixture matches the Java one: implement a new EncryptAesEcb(byte[] key, byte[]
plaintext) method that creates an Aes instance, sets Mode = CipherMode.ECB and
Padding = PaddingMode.PKCS7, uses CreateEncryptor/TransformFinalBlock to return
ciphertext; and add an Ed25519 example (e.g., GenerateEd25519Key() and a
SignEd25519/VerifyEd25519 pair or a single GenerateAndSignEd25519 sample) that
uses the platform Ed25519 APIs to generate a keypair and produce/verify a
signature; place both methods in the CryptoHelper class alongside existing
methods (EncryptAes, EncryptAesGcm, GenerateRsaKey, etc.) so the scanner sees
the insecure AES-ECB pattern and the modern Ed25519 pattern.
In `@test-fixtures/go-api-server/main.go`:
- Around line 100-104: The call to blake2b.New256(nil) in HashBLAKE2b ignores
its error return; update HashBLAKE2b to either (A) handle and propagate the
error (change signature to HashBLAKE2b(data []byte) ([]byte, error), check err
from blake2b.New256 and return it) or (B) if you intentionally rely on nil being
safe, add a clear inline comment and assert the error cannot occur (check err
and panic/log.Fatalf with a clear message) so the ignored error is explicit;
reference the blake2b.New256 call and the HashBLAKE2b function when making the
change.
In
`@test-fixtures/java-microservice/src/main/java/com/example/CryptoService.java`:
- Around line 20-27: encryptAesGcm currently generates a random IV but only
returns ciphertext, making decryption impossible; modify the encryptAesGcm
method to return both IV and ciphertext by prepending the 12-byte iv to the
ciphertext before returning (use the existing iv and cipher.doFinal(plaintext)),
and ensure the caller/decryptor knows to slice off the first 12 bytes as the GCM
IV when decrypting (references: encryptAesGcm, iv, GCMParameterSpec,
Cipher.doFinal).
- Around line 64-71: The methods performDhKeyAgreement and
performX25519KeyAgreement currently only call KeyAgreement.getInstance and must
be expanded to perform a full key-exchange flow: generate a key pair for the
local side (use KeyPairGenerator for "DH" and "X25519"), obtain/construct a peer
PublicKey (you can generate a second key pair to simulate a peer), call
ka.init(localPrivateKey), call ka.doPhase(peerPublicKey, true), and call
ka.generateSecret(...) to derive the shared secret; ensure to handle
algorithm-specific parameter steps for DH (parameters/spec) and use the
appropriate KeyPairGenerator/KeyFactory for X25519 so the code in
performDhKeyAgreement and performX25519KeyAgreement demonstrates init, doPhase,
and generateSecret end-to-end.
- Around line 98-103: In deriveKeyPbkdf2, ensure the PBEKeySpec's internal
password is zeroed after use by calling spec.clearPassword() in a finally block
(or equivalent try/finally) so it always runs even on exceptions; wrap the call
to factory.generateSecret(spec).getEncoded() in try and invoke
spec.clearPassword() in finally to prevent the password from lingering in
memory.
In `@test-fixtures/node-auth-service/auth.js`:
- Around line 1-3: The fixture declares but doesn't exercise argon2, jose, and
node-forge; require each module at the top (e.g., const argon2 =
require('argon2'), const { SignJWT } = require('jose'), const forge =
require('node-forge')) and add a tiny usage for each (e.g., an async function
that hashes a test string with argon2, a function that creates a minimal JWT
with SignJWT, and a function that generates a small RSA key or fingerprint with
forge); then export those helper functions alongside existing exports
(referencing the new argon2/hash function name, the SignJWT wrapper, and the
forge helper) so the libraries are actually exercised and detectable by API-call
coverage.
In `@test-fixtures/node-auth-service/package.json`:
- Around line 5-8: The fixture's package.json lists floating dependency ranges
for bcrypt, argon2, jose, and node-forge which harms reproducibility; update
those entries in the dependencies block (the lines containing "bcrypt",
"argon2", "jose", and "node-forge") to pinned exact versions (remove the caret
and specify the concrete version numbers you want to lock to), then regenerate
the lockfile (npm/yarn pnpm) so the fixture and its lockfile remain
deterministic across environments.
In `@test-fixtures/php-api/crypto.php`:
- Around line 15-24: Both encrypt_aes_cbc and encrypt_3des currently generate an
IV but drop it; change them to return the IV together with the ciphertext (e.g.,
return a two-element structure or concatenate IV+ciphertext) so fixtures support
realistic round-trip decryption. Update encrypt_aes_cbc and encrypt_3des to
include the generated $iv in the return value and adjust callers/tests to expect
and use the IV for decryption.
- Around line 54-60: Update the comment above generate_signing_keypair() to
qualify the FIPS claim: change "FIPS Approved" to a clearer phrase such as
"FIPS-approved algorithm per FIPS 186-5 (module/runtime validation may be
required)" so the fixture notes that Ed25519 is FIPS-approved at the
algorithm/standard level but that module validation or runtime configuration may
still be necessary; leave the X25519 comment and the function names
generate_signing_keypair and generate_box_keypair unchanged.
In `@test-fixtures/python-web-app/requirements.txt`:
- Around line 1-5: Replace loose >= version specifiers in the test requirements
with exact pins to guarantee reproducible test runs; update each dependency line
(cryptography, bcrypt, argon2-cffi, PyJWT, flask) to use exact equality pins
(e.g., change "cryptography>=42.0.0" to "cryptography==42.0.0") and ensure the
requirements.txt only contains the exact versions you validated in CI.
In `@test-fixtures/ruby-web-app/app.rb`:
- Around line 17-41: encrypt_aes_cbc, encrypt_chacha, and encrypt_3des currently
either discard the generated IV or never generate one, preventing decryption;
update each function to generate and return the IV (use cipher.random_iv) and
return a consistent hash like { iv:, data:, tag: }—for encrypt_aes_cbc and
encrypt_3des return { iv: iv, data: encrypted_data, tag: nil } (CBC/3DES have no
auth tag), and for encrypt_chacha return { iv: iv, data: encrypted_data, tag:
cipher.auth_tag } after finalizing so callers can decrypt correctly.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockextlib/cryptoscan/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (42)
Cargo.tomlMakefileextlib/cryptoscan/Cargo.tomlextlib/cryptoscan/src/crypto_algorithm.rsextlib/cryptoscan/src/cyclonedx.rsextlib/cryptoscan/src/fips.rsextlib/cryptoscan/src/main.rsextlib/cryptoscan/src/patterns.rsextlib/cryptoscan/src/scanner.rsextlib/cryptoscan/tests/integration_test.rsspectrometer.cabalsrc/App/Fossa/Analyze.hssrc/App/Fossa/Analyze/ScanSummary.hssrc/App/Fossa/Analyze/Types.hssrc/App/Fossa/Config/Analyze.hssrc/App/Fossa/CryptoScan/Analyze.hssrc/App/Fossa/CryptoScan/FipsReport.hssrc/App/Fossa/CryptoScan/SourceUnit.hssrc/App/Fossa/CryptoScan/Types.hssrc/App/Fossa/EmbeddedBinary.hstest-fixtures/csharp-service/CryptoService.cstest-fixtures/csharp-service/Service.csprojtest-fixtures/elixir-phoenix-app/crypto.extest-fixtures/elixir-phoenix-app/mix.exstest-fixtures/go-api-server/go.modtest-fixtures/go-api-server/main.gotest-fixtures/java-microservice/pom.xmltest-fixtures/java-microservice/src/main/java/com/example/CryptoService.javatest-fixtures/node-auth-service/auth.jstest-fixtures/node-auth-service/package.jsontest-fixtures/php-api/composer.jsontest-fixtures/php-api/crypto.phptest-fixtures/python-web-app/app.pytest-fixtures/python-web-app/requirements.txttest-fixtures/ruby-web-app/Gemfiletest-fixtures/ruby-web-app/app.rbtest-fixtures/rust-crypto-tool/Cargo.tomltest-fixtures/rust-crypto-tool/src/main.rstest-fixtures/swift-ios-app/CryptoManager.swifttest-fixtures/swift-ios-app/Podfiletest/App/Fossa/CryptoScan/FipsReportSpec.hstest/App/Fossa/CryptoScan/SourceUnitSpec.hs
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docs/references/subcommands/analyze.md (1)
215-215: Consider surfacing companion crypto flags in the experimental index.At Line 215, only
--x-crypto-scanis listed. Adding a brief note (or additional rows) for--crypto-cbom-outputand--crypto-fips-reportwould improve discoverability from this table.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/references/subcommands/analyze.md` at line 215, Update the experimental flags table to surface the companion crypto flags by adding brief entries (or a short parenthetical note) for --crypto-cbom-output and --crypto-fips-report alongside --x-crypto-scan; reference the same crypto scanning README link used for --x-crypto-scan and include one-line descriptions (e.g., output CBOM path for --crypto-cbom-output and generate FIPS compliance report for --crypto-fips-report) so readers can discover these related options from the table.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@docs/references/subcommands/analyze.md`:
- Line 215: Update the experimental flags table to surface the companion crypto
flags by adding brief entries (or a short parenthetical note) for
--crypto-cbom-output and --crypto-fips-report alongside --x-crypto-scan;
reference the same crypto scanning README link used for --x-crypto-scan and
include one-line descriptions (e.g., output CBOM path for --crypto-cbom-output
and generate FIPS compliance report for --crypto-fips-report) so readers can
discover these related options from the table.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/features/crypto-scanning.mddocs/references/experimental/crypto-scanning/README.mddocs/references/subcommands/analyze.md
✅ Files skipped from review due to trivial changes (2)
- docs/references/experimental/crypto-scanning/README.md
- docs/features/crypto-scanning.md
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/App/Fossa/Analyze.hs (1)
423-427:⚠️ Potential issue | 🟠 MajorDo not treat scanner failures as “no findings”
On Line 423 and Line 523,
join (resultToMaybe ...)collapsesFailureandSuccess Nothinginto the sameNothingpath, so failures are reported as empty results.🔧 Suggested fix
-import Diag.Result (resultToMaybe) +import Diag.Result (Result (Failure, Success), resultToMaybe) ... - case join (resultToMaybe maybeCbomBytes) of - Just bytes -> do + case maybeCbomBytes of + Success _ (Just bytes) -> do sendIO $ BL.writeFile cbomPath bytes logInfo $ "CycloneDX 1.7 CBOM written to: " <> pretty cbomPath - Nothing -> logInfo "No crypto findings to write to CBOM file" + Success _ Nothing -> logInfo "No crypto findings to write to CBOM file" + Failure _ _ -> logInfo "Crypto CBOM generation failed; see diagnostics above" ... when (Config.cryptoFipsReport cfg) $ - case join (resultToMaybe maybeCryptoScanResults) of - Just cryptoResults -> do + case maybeCryptoScanResults of + Success _ (Just cryptoResults) -> do logInfo "" logInfo . renderIt $ renderFipsReport cryptoResults - Nothing -> logInfo "No crypto findings for FIPS report" + Success _ Nothing -> logInfo "No crypto findings for FIPS report" + Failure _ _ -> logInfo "Crypto scan failed; skipping FIPS report"Also applies to: 523-527
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/App/Fossa/Analyze.hs` around lines 423 - 427, The code currently collapses scanner failures and empty successes by using join (resultToMaybe maybeCbomBytes); instead pattern-match on the Result directly so failures are handled separately: replace join (resultToMaybe maybeCbomBytes) with a case on maybeCbomBytes (e.g. Failure err -> logError (show err) or similar; Success Nothing -> logInfo "No crypto findings to write to CBOM file"; Success (Just bytes) -> sendIO $ BL.writeFile cbomPath bytes and logInfo the path). Apply the same change to the other identical block around the code handling the second CBOM (the one at lines ~523-527), ensuring you reference and use sendIO, BL.writeFile, logInfo and the Result value variable (maybeCbomBytes) so failures are not treated as "no findings."
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/App/Fossa/Analyze.hs`:
- Around line 409-423: The crypto CBOM generation path currently runs regardless
of the enableCryptoScan flag, causing analyzeCryptoScanCBOM to execute even when
scans are disabled; update the CBOM handling so it only runs when crypto
scanning is enabled (or when maybeCryptoScanResults indicates a successful
scan). Specifically, modify the block that checks Config.cryptoCbomOutput so it
first guards on enableCryptoScan (or uses maybeCryptoScanResults) before calling
analyzeCryptoScanCBOM, and ensure logging remains consistent with the skip
message in the analyzeWithCryptoScan/enableCryptoScan flow.
---
Duplicate comments:
In `@src/App/Fossa/Analyze.hs`:
- Around line 423-427: The code currently collapses scanner failures and empty
successes by using join (resultToMaybe maybeCbomBytes); instead pattern-match on
the Result directly so failures are handled separately: replace join
(resultToMaybe maybeCbomBytes) with a case on maybeCbomBytes (e.g. Failure err
-> logError (show err) or similar; Success Nothing -> logInfo "No crypto
findings to write to CBOM file"; Success (Just bytes) -> sendIO $ BL.writeFile
cbomPath bytes and logInfo the path). Apply the same change to the other
identical block around the code handling the second CBOM (the one at lines
~523-527), ensuring you reference and use sendIO, BL.writeFile, logInfo and the
Result value variable (maybeCbomBytes) so failures are not treated as "no
findings."
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
spectrometer.cabalsrc/App/Fossa/Analyze.hs
🚧 Files skipped from review as they are similar to previous changes (1)
- spectrometer.cabal
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/App/Fossa/Analyze.hs (1)
451-454:⚠️ Potential issue | 🟡 MinorFlush crypto scan diagnostics like other analyzers.
maybeCryptoScanResultsis collected but never passed throughDiag.flushLogs, so detailed warnings/errors can be missed in normal output.Suggested fix
-- Flush logs from ficus traverse_ (Diag.flushLogs SevError SevDebug) [maybeFicusResults] + -- Flush logs from crypto scan + traverse_ (Diag.flushLogs SevError SevDebug) [maybeCryptoScanResults]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/App/Fossa/Analyze.hs` around lines 451 - 454, The crypto scan diagnostics collected in maybeCryptoScanResults are never flushed; add it to the same flush sequence as the other analyzers by passing maybeCryptoScanResults into Diag.flushLogs (alongside maybeLernieResults and maybeFicusResults). Locate the traverse_ calls that invoke Diag.flushLogs (the ones referencing maybeLernieResults and maybeFicusResults) and include maybeCryptoScanResults in the list so Diag.flushLogs SevError SevDebug is run for it as well.
♻️ Duplicate comments (1)
src/App/Fossa/Analyze.hs (1)
419-430:⚠️ Potential issue | 🟠 MajorGate CBOM generation behind
--x-crypto-scanfor consistent behavior.Line 414 reports crypto scanning is skipped, but Line 423 can still run crypto scanning for CBOM output. This is contradictory and can trigger unexpected scan work.
Suggested fix
- case Config.cryptoCbomOutput cfg of - Just cbomPath -> do + case (enableCryptoScan, Config.cryptoCbomOutput cfg) of + (False, Just _) -> + logInfo "Skipping crypto CBOM output (--x-crypto-scan not set)" + (True, Just cbomPath) -> do maybeCbomBytes <- Diag.errorBoundaryIO . diagToDebug $ Diag.context "crypto-cbom-output" . runStickyLogger SevInfo $ analyzeCryptoScanCBOM basedir case maybeCbomBytes of Success _ (Just bytes) -> do sendIO $ BL.writeFile cbomPath bytes logInfo $ "CycloneDX 1.7 CBOM written to: " <> pretty cbomPath Success _ Nothing -> logInfo "No crypto findings to write to CBOM file" Failure _ _ -> logWarn "Crypto CBOM generation failed; see diagnostics above" - Nothing -> pure () + (_, Nothing) -> pure ()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/App/Fossa/Analyze.hs` around lines 419 - 430, CBOM generation is currently executed unconditionally when Config.cryptoCbomOutput cfg is Just, causing analyzeCryptoScanCBOM to run even when crypto scanning is disabled; modify this block to first check the same crypto-scan flag used to skip scans (the --x-crypto-scan option) and only call analyzeCryptoScanCBOM when crypto scanning is enabled. In practice, wrap the existing case on Config.cryptoCbomOutput cfg with the existing crypto-scan guard (the same condition that prints "crypto scanning is skipped"), or add an explicit check like "when (cryptoScanEnabled cfg) ..." before invoking analyzeCryptoScanCBOM and emitting CBOM logs, and otherwise log/skip CBOM generation consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@extlib/cryptoscan/src/main.rs`:
- Around line 23-25: Replace the loose String option with a strongly-typed enum:
create an enum named Ecosystem (variants: Python, Java, Go, Node, Rust, Auto)
deriving Clone, Debug, and clap::ValueEnum, then change the CLI field ecosystem:
String to ecosystem: Ecosystem and use #[arg(short, long, default_value_t =
Ecosystem::Auto)] so clap will validate inputs at parse time; update any
downstream code that compared or matched on the old string to match on Ecosystem
variants instead.
In `@extlib/cryptoscan/src/scanner.rs`:
- Around line 90-91: The local HashSet named `seen` is filtering out later
matches with the same (algorithm, file, line) before `deduplicate_findings` has
a chance to pick the highest-confidence finding; remove or stop using the
`seen`-based early dedup in the scanner loops and instead collect all raw
matches (allow duplicates through) and let `deduplicate_findings` perform
deduplication by confidence. Specifically, eliminate the `seen`
checks/insertions (the `seen` variable and its use in the match-collection
loops) so the scanner functions push all findings into the collection passed to
`deduplicate_findings`, ensuring `deduplicate_findings` receives all candidates
for proper confidence-based selection.
- Around line 92-102: The loop currently descends into every directory and
relies on should_skip_path(path) to bail out after visiting entries, which
wastes I/O and also fails on Windows because should_skip_path matches on
path.to_string_lossy() with forward slashes. Change the WalkDir pipeline to use
filter_entry(...) (before into_iter()) so directories like node_modules,
__pycache__, .git, target, etc. are pruned and not descended into; and rewrite
should_skip_path(path: &Path) to inspect path components/OsStr names (e.g.,
using path.components() or path.iter().map(|c| c.as_os_str())) and compare each
component against a set of directory names to skip (instead of string pattern
matching), also treat hidden directories by checking
component.to_string_lossy().starts_with('.') so the logic works on both Unix and
Windows path formats.
In `@src/App/Fossa/CryptoScan/FipsReport.hs`:
- Around line 45-49: The functions using guard syntax must be rewritten to use
explicit if/case expressions: replace the guard in compliancePercentage by using
an if totalAlgorithms == 0 then 100 else (approvedCount * 100) `div`
totalAlgorithms; similarly convert coloredPercentage and catWarnings guards into
either nested if/then/else or a case on boolean conditions to preserve the same
branches and return types; rewrite suggestAlternative (multiple guards) into a
case expression that matches on computed predicates (or a cascade of ifs)
preserving evaluation order; for rsaWarning remove pattern guards by matching
the RSA-related value with a case or by extracting fields into local lets and
using if checks. Ensure function names (compliancePercentage, coloredPercentage,
suggestAlternative, catWarnings, rsaWarning) remain unchanged and behavior is
identical.
---
Outside diff comments:
In `@src/App/Fossa/Analyze.hs`:
- Around line 451-454: The crypto scan diagnostics collected in
maybeCryptoScanResults are never flushed; add it to the same flush sequence as
the other analyzers by passing maybeCryptoScanResults into Diag.flushLogs
(alongside maybeLernieResults and maybeFicusResults). Locate the traverse_ calls
that invoke Diag.flushLogs (the ones referencing maybeLernieResults and
maybeFicusResults) and include maybeCryptoScanResults in the list so
Diag.flushLogs SevError SevDebug is run for it as well.
---
Duplicate comments:
In `@src/App/Fossa/Analyze.hs`:
- Around line 419-430: CBOM generation is currently executed unconditionally
when Config.cryptoCbomOutput cfg is Just, causing analyzeCryptoScanCBOM to run
even when crypto scanning is disabled; modify this block to first check the same
crypto-scan flag used to skip scans (the --x-crypto-scan option) and only call
analyzeCryptoScanCBOM when crypto scanning is enabled. In practice, wrap the
existing case on Config.cryptoCbomOutput cfg with the existing crypto-scan guard
(the same condition that prints "crypto scanning is skipped"), or add an
explicit check like "when (cryptoScanEnabled cfg) ..." before invoking
analyzeCryptoScanCBOM and emitting CBOM logs, and otherwise log/skip CBOM
generation consistently.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
extlib/cryptoscan/src/main.rsextlib/cryptoscan/src/scanner.rssrc/App/Fossa/Analyze.hssrc/App/Fossa/Analyze/ScanSummary.hssrc/App/Fossa/CryptoScan/FipsReport.hs
CortezFrazierJr
force-pushed
the
feature/crypto-scanning
branch
from
6e73a28 to
7cd1701
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (3)
src/App/Fossa/CryptoScan/FipsReport.hs (1)
45-49: 🛠️ Refactor suggestion | 🟠 MajorGuard syntax is still present across multiple functions.
Line 47, Line 102, Line 199, Line 239, and Line 254 still rely on guards/pattern-guards. This should be rewritten with
if/caseto match the repo Haskell rules.As per coding guidelines: In Haskell, avoid partial functions, list comprehensions, and match guards.
Also applies to: 100-105, 197-213, 238-248, 253-256
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/App/Fossa/CryptoScan/FipsReport.hs` around lines 45 - 49, The function compliancePercentage (and other functions using guards in this file) currently use guard syntax; rewrite these to use explicit if/then/else or case expressions instead: for compliancePercentage (pattern FipsReportStats{..}) replace the guards that check totalAlgorithms == 0 and the otherwise branch with an if expression that returns 100 when totalAlgorithms == 0 and (approvedCount * 100) `div` totalAlgorithms otherwise; similarly locate other functions that use guards/pattern-guards and convert each guard clause into equivalent if or case logic while preserving the same bindings (e.g., FipsReportStats, approvedCount, totalAlgorithms) and behaviour.extlib/cryptoscan/src/scanner.rs (2)
140-146:⚠️ Potential issue | 🟠 MajorEarly dedup undermines the confidence-based deduplication fix.
The
seenHashSet at lines 90 and 140-146 filters duplicates before they reachdeduplicate_findings, so the HashMap-based "highest confidence wins" logic (lines 304-322) never gets a chance to compare confidence levels. If a Low-confidence pattern matches first, Higher-confidence matches at the same location are dropped.Remove the early dedup to let
deduplicate_findingshandle all deduplication:🔧 Proposed fix
let all_patterns = patterns::build_patterns(); let mut findings = Vec::new(); - let mut seen = HashSet::new(); for entry in WalkDir::new(project_path) ... for mat in pattern.regex.find_iter(&content) { let line_number = content[..mat.start()] .chars() .filter(|&c| c == '\n') .count() + 1; - // Deduplicate: same algorithm + same file - let dedup_key = format!("{}:{}:{}", pattern.algorithm_name, rel_path, line_number); - if seen.contains(&dedup_key) { - continue; - } - seen.insert(dedup_key); - let algorithm = resolve_algorithm(&pattern.algorithm_name, mat.as_str());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extlib/cryptoscan/src/scanner.rs` around lines 140 - 146, The early deduplication using the local `seen` HashSet (creating `dedup_key` from `pattern.algorithm_name`, `rel_path`, `line_number` and using `seen.contains` / `seen.insert`) must be removed so matches are not filtered before reaching `deduplicate_findings`; delete the `dedup_key` creation and the `if seen.contains(&dedup_key) { continue; } seen.insert(dedup_key);` block in the scanner loop, leaving match collection intact so the `deduplicate_findings` function (the HashMap-based highest-confidence logic) can receive all candidates and resolve duplicates.
167-187:⚠️ Potential issue | 🟠 MajorWindows path compatibility issue persists.
should_skip_pathmatches against forward-slash patterns ("/node_modules/"), butpath.to_string_lossy()returns backslashes on Windows. This causes the skip logic to fail silently on Windows, traversing intonode_modules,.git,target, etc.🔧 Proposed cross-platform fix
fn should_skip_path(path: &Path) -> bool { - let path_str = path.to_string_lossy(); - let skip_dirs = [ - "/.git/", - "/node_modules/", - "/__pycache__/", + const SKIP_DIRS: &[&str] = &[ + ".git", "node_modules", "__pycache__", ".venv", "venv", "target", + "dist", "build", ".tox", ".mypy_cache", ".pytest_cache", + "vendor", ".gradle", ".idea", ".vscode", ]; - skip_dirs.iter().any(|d| path_str.contains(d)) + path.iter() + .any(|seg| SKIP_DIRS.iter().any(|skip| seg == std::ffi::OsStr::new(skip))) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extlib/cryptoscan/src/scanner.rs` around lines 167 - 187, The skip logic in should_skip_path fails on Windows because path.to_string_lossy() yields backslashes; normalize or check path components instead. Update should_skip_path to either convert path_str (from path.to_string_lossy()) by replacing '\' with '/' before matching against skip_dirs, or better iterate Path::components()/file_name() and check component names (e.g., "node_modules", ".git", "target") against a set of skip directory names; keep the same skip_dirs list (but without surrounding slashes) and use the normalized comparison so it works cross-platform.
🧹 Nitpick comments (8)
src/App/Fossa/Analyze/Types.hs (1)
15-15: Qualify the new crypto import to match Haskell import rules.Please keep the new
CryptoScanResultsimport qualified for consistency with the repository’s Haskell import guideline.As per coding guidelines: "Use explicit imports in Haskell, qualified with full names".♻️ Proposed refactor
-import App.Fossa.CryptoScan.Types (CryptoScanResults) +import App.Fossa.CryptoScan.Types qualified as CryptoScan.Types (CryptoScanResults) @@ - , cryptoScanResult :: Result (Maybe CryptoScanResults) + , cryptoScanResult :: Result (Maybe CryptoScan.Types.CryptoScanResults)Also applies to: 90-90
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/App/Fossa/Analyze/Types.hs` at line 15, The new import of CryptoScanResults should be made a qualified explicit import to follow the project's Haskell guidelines: change the import of App.Fossa.CryptoScan.Types so it is imported qualified (e.g., import qualified App.Fossa.CryptoScan.Types as CryptoScan) and update usages to reference CryptoScan.CryptoScanResults (or the chosen alias) so all references to CryptoScanResults in Types.hs use the qualified name; ensure the import is explicit and matches other qualified imports in the file (look for existing patterns in Types.hs to pick the correct alias style).src/App/Fossa/Config/Analyze.hs (1)
256-257: PreferTextfor the new CBOM output option fields.The newly added CBOM path fields are
FilePath(String-backed). Consider keeping them asTextat the config boundary and converting only where needed.As per coding guidelines: "In Haskell, use `Text` instead of `String`".♻️ Proposed refactor
- , analyzeCryptoCbomOutput :: Maybe FilePath + , analyzeCryptoCbomOutput :: Maybe Text @@ - , cryptoCbomOutput :: Maybe FilePath + , cryptoCbomOutput :: Maybe Text @@ - <*> optional (strOption (applyFossaStyle <> long "crypto-cbom-output" <> metavar "FILE" <> stringToHelpDoc "Write CycloneDX 1.7 CBOM to FILE (implies --x-crypto-scan)")) + <*> optional (toText <$> strOption (applyFossaStyle <> long "crypto-cbom-output" <> metavar "FILE" <> stringToHelpDoc "Write CycloneDX 1.7 CBOM to FILE (implies --x-crypto-scan)"))Also applies to: 300-301, 377-377
src/App/Fossa/CryptoScan/FipsReport.hs (1)
59-60:nubBydedup is quadratic in two paths.Both stats and rendering dedupe with
nubBy, which scales poorly as findings grow. Consider aSet-based dedupe once and reuse the unique list.Also applies to: 72-73
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/App/Fossa/CryptoScan/FipsReport.hs` around lines 59 - 60, The current dedupe uses nubBy twice (uniqueAlgos and later at lines ~72-73), which is O(n^2); replace both uses by a single Set/Map-based dedupe pass: build a Set (or Map) of keys using dedupeKey over findings once (e.g., fold over findings to collect first occurrence per key), produce one uniqueAlgos list and then reuse it for statuses (map cryptoAlgorithmFipsStatus . cryptoFindingAlgorithm) and for rendering; update references to uniqueAlgos, nubBy and dedupeKey so the dedupe is O(n) and only performed once.test/App/Fossa/CryptoScan/FipsReportSpec.hs (1)
52-62: Add a regression test for same name + different parameter sets.This test currently validates duplicate names only. It should also assert that variants like RSA-1024 and RSA-2048 are counted separately.
As per coding guidelines: Code should be thoroughly tested with appropriate unit tests.🧪 Suggested test addition
+ it "does not deduplicate same-name algorithms with different parameter sets" $ do + let results = + CryptoScanResults + [ (mkFinding "RSA" FipsNotApproved){cryptoFindingAlgorithm = (mkAlgorithm "RSA" FipsNotApproved){cryptoAlgorithmParameterSet = Just "1024"}} + , (mkFinding "RSA" FipsApproved){cryptoFindingAlgorithm = (mkAlgorithm "RSA" FipsApproved){cryptoAlgorithmParameterSet = Just "2048"}} + ] + stats = computeFipsStats results + totalAlgorithms stats `shouldBe` 2 + approvedCount stats `shouldBe` 1 + notApprovedCount stats `shouldBe` 1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/App/Fossa/CryptoScan/FipsReportSpec.hs` around lines 52 - 62, The test in FipsReportSpec currently only checks deduplication by identical name; extend it to cover same base name with different parameters so variants are distinct: add findings via CryptoScanResults using mkFinding for "RSA-1024" and "RSA-2048" (or similar differing parameter names) alongside duplicates like "AES-256-GCM", then call computeFipsStats and assert totalAlgorithms (and approvedCount if relevant) reflect both RSA variants counted separately (i.e., totalAlgorithms increases accordingly) while still deduplicating exact name duplicates.src/App/Fossa/Analyze.hs (1)
410-423: CBOM generation currently triggers a second full crypto scan.
analyzeWithCryptoScanandanalyzeCryptoScanCBOMrun as separate calls, which can significantly increase wall time on large repositories.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/App/Fossa/Analyze.hs` around lines 410 - 423, The code currently may run two full crypto scans because analyzeWithCryptoScan and analyzeCryptoScanCBOM are invoked separately; change the flow so the crypto scan is executed at most once: when enableCryptoScan is true run analyzeWithCryptoScan once (keeping the existing Diag.context/"crypto-scan" and runStickyLogger SevInfo usage) and store its results in maybeCryptoScanResults, then when Config.cryptoCbomOutput is Just cbomPath pass those results into analyzeCryptoScanCBOM (or change analyzeCryptoScanCBOM to accept the scan results) to generate the CBOM instead of calling analyzeCryptoScanCBOM which re-runs the scan; if maybeCryptoScanResults is Nothing (scan disabled or failed) fall back to the current behavior or skip CBOM as appropriate.extlib/cryptoscan/src/scanner.rs (1)
92-102: Usefilter_entryto prune directories early for better performance.The current approach traverses into all directories (including
node_moduleswith thousands of files) and then skips them. Usingfilter_entry()prevents WalkDir from descending into skipped directories entirely.♻️ Proposed optimization
for entry in WalkDir::new(project_path) .into_iter() + .filter_entry(|e| !should_skip_path(e.path())) .filter_map(|e| e.ok()) .filter(|e| e.file_type().is_file()) { let path = entry.path(); - - // Skip hidden directories and common non-source dirs - if should_skip_path(path) { - continue; - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extlib/cryptoscan/src/scanner.rs` around lines 92 - 102, The loop currently uses WalkDir and then skips entries with should_skip_path(path), causing WalkDir to descend into skipped directories; change the iterator to use WalkDir::new(project_path).into_iter().filter_entry(|e| !should_skip_path(e.path())) so directories matched by should_skip_path are pruned before descent, then keep the existing .filter_map(|e| e.ok()).filter(|e| e.file_type().is_file()) logic; ensure should_skip_path(&Path) correctly recognizes directory paths (e.g., node_modules) so filter_entry can prevent descending into them.extlib/cryptoscan/src/cyclonedx.rs (2)
222-227: Non-deterministic ordering of algorithm references.Converting through
HashSetand back produces arbitrary iteration order, which can cause spurious diffs when comparing BOMs across runs. For reproducible output, consider sorting.♻️ Suggested fix for deterministic output
let unique_algos: Vec<String> = algo_refs .iter() .collect::<HashSet<_>>() .into_iter() .cloned() - .collect(); + .collect::<Vec<_>>(); + let mut unique_algos = unique_algos; + unique_algos.sort();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extlib/cryptoscan/src/cyclonedx.rs` around lines 222 - 227, The current creation of unique_algos from algo_refs uses a HashSet which yields non-deterministic ordering; replace that flow with a deterministic approach (e.g., collect into a Vec, deduplicate and then sort, or use a BTreeSet) so unique_algos has a stable order across runs; update the code around the unique_algos variable (and any use of algo_refs) in cyclonedx.rs to perform deterministic deduplication and then call sort() (or build from a BTreeSet) before returning or serializing the BOM.
269-272: Consider handling potential panic on system clock anomalies.
duration_since(UNIX_EPOCH).unwrap()will panic if the system clock is set before 1970, which can occur on misconfigured or embedded systems. For a CLI tool, this is unlikely but worth defensive handling.♻️ Suggested defensive handling
let now = std::time::SystemTime::now() .duration_since(std::time::UNIX_EPOCH) - .unwrap() + .unwrap_or_default() .as_secs();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extlib/cryptoscan/src/cyclonedx.rs` around lines 269 - 272, The current assignment to the variable `now` uses `SystemTime::now().duration_since(UNIX_EPOCH).unwrap()` which can panic if the clock is before 1970; change the call to handle the Result instead (e.g., use a match or unwrap_or_else on `SystemTime::now().duration_since(UNIX_EPOCH)`), log or warn on the Err case, and provide a safe fallback value (for example 0 or the error's duration) when the duration cannot be obtained; update the code around the `now` variable and the `SystemTime::now().duration_since(UNIX_EPOCH)` expression to implement this defensive handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@extlib/cryptoscan/src/fips.rs`:
- Around line 167-175: The RSA branch that currently checks
lower.contains("rsa") only looks for literal "512" or "1024" and incorrectly
approves other sub-2048 sizes (e.g., "rsa-1536"); update the logic in the RSA
handling (the block that inspects lower and returns FipsStatus::NotApproved or
Approved) to parse any numeric key size present in the string (extract digits
via a regex or iterate chars) and treat the key as NotApproved if the parsed
size is < 2048, otherwise Approved; keep returning the same tuple shape
(FipsStatus and Option<String>) and ensure existing messages (e.g., "RSA key
size below 2048 bits; use RSA-2048 or higher") are preserved when returning
NotApproved.
In `@extlib/cryptoscan/src/patterns.rs`:
- Around line 35-36: Remove the unnecessary dead_code suppression from the
actually-used function ecosystem_manifests (it's referenced in scanner.rs) by
deleting the #[allow(dead_code)] above its definition; for ecosystem_extensions
decide whether to keep or drop it — if it's not needed, delete the entire
ecosystem_extensions function, otherwise remove its #[allow(dead_code)] so
future use isn't masked—ensure you only change the attributes or remove the
unused function and do not alter function signatures.
In `@src/App/Fossa/Analyze.hs`:
- Around line 410-417: The crypto and CBOM diagnostic results
(maybeCryptoScanResults, maybeCbomBytes) are not being flushed before fallback
warnings; after you compute each (the Diag.context "crypto-scan" .
runStickyLogger $ analyzeWithCryptoScan basedir call and the analogous CBOM
call) invoke the same diagnostic flush used elsewhere in the file (the flush
function used for other result channels — e.g., the Diag.flush/flushDiagnostics
call or the helper that drains diagToDebug/runStickyLogger output) so that
diagnostics are emitted before fallback warnings are logged.
In `@src/App/Fossa/CryptoScan/FipsReport.hs`:
- Around line 253-256: The pattern match on reads (Text.unpack ps) :: [(Int,
String)] currently falls through to `_ -> []` and silently drops unparsable RSA
parameter sets; change that fallback to emit a warning so uncertain cases aren't
hidden by returning something like `[annotate (color Yellow) $ "- RSA-" <>
pretty ps <> ": Unable to parse key size; compliance uncertain"]` (keep the
existing annotate/color/pretty usage and only replace the `_ -> []` branch),
locating the change where the `reads (Text.unpack ps)` match is performed for
`ps`.
In `@src/App/Fossa/CryptoScan/Types.hs`:
- Around line 42-43: The JSON parsing currently accepts any Ints for numeric
fields (e.g., cryptoFindingLineNumber) without validation; update the FromJSON
implementations for the affected types (the CryptoFinding parser and the other
parsers covering lines ~86-105) to explicitly parse these numeric fields with
withObject/.:, then validate ranges (e.g., ensure cryptoFindingLineNumber >= 0,
enforce any domain-specific min/max for quantum/level fields) and call fail with
a clear message when values are out of range; this ensures invalid or negative
numbers are rejected at parse time rather than accepted silently.
---
Duplicate comments:
In `@extlib/cryptoscan/src/scanner.rs`:
- Around line 140-146: The early deduplication using the local `seen` HashSet
(creating `dedup_key` from `pattern.algorithm_name`, `rel_path`, `line_number`
and using `seen.contains` / `seen.insert`) must be removed so matches are not
filtered before reaching `deduplicate_findings`; delete the `dedup_key` creation
and the `if seen.contains(&dedup_key) { continue; } seen.insert(dedup_key);`
block in the scanner loop, leaving match collection intact so the
`deduplicate_findings` function (the HashMap-based highest-confidence logic) can
receive all candidates and resolve duplicates.
- Around line 167-187: The skip logic in should_skip_path fails on Windows
because path.to_string_lossy() yields backslashes; normalize or check path
components instead. Update should_skip_path to either convert path_str (from
path.to_string_lossy()) by replacing '\' with '/' before matching against
skip_dirs, or better iterate Path::components()/file_name() and check component
names (e.g., "node_modules", ".git", "target") against a set of skip directory
names; keep the same skip_dirs list (but without surrounding slashes) and use
the normalized comparison so it works cross-platform.
In `@src/App/Fossa/CryptoScan/FipsReport.hs`:
- Around line 45-49: The function compliancePercentage (and other functions
using guards in this file) currently use guard syntax; rewrite these to use
explicit if/then/else or case expressions instead: for compliancePercentage
(pattern FipsReportStats{..}) replace the guards that check totalAlgorithms == 0
and the otherwise branch with an if expression that returns 100 when
totalAlgorithms == 0 and (approvedCount * 100) `div` totalAlgorithms otherwise;
similarly locate other functions that use guards/pattern-guards and convert each
guard clause into equivalent if or case logic while preserving the same bindings
(e.g., FipsReportStats, approvedCount, totalAlgorithms) and behaviour.
---
Nitpick comments:
In `@extlib/cryptoscan/src/cyclonedx.rs`:
- Around line 222-227: The current creation of unique_algos from algo_refs uses
a HashSet which yields non-deterministic ordering; replace that flow with a
deterministic approach (e.g., collect into a Vec, deduplicate and then sort, or
use a BTreeSet) so unique_algos has a stable order across runs; update the code
around the unique_algos variable (and any use of algo_refs) in cyclonedx.rs to
perform deterministic deduplication and then call sort() (or build from a
BTreeSet) before returning or serializing the BOM.
- Around line 269-272: The current assignment to the variable `now` uses
`SystemTime::now().duration_since(UNIX_EPOCH).unwrap()` which can panic if the
clock is before 1970; change the call to handle the Result instead (e.g., use a
match or unwrap_or_else on `SystemTime::now().duration_since(UNIX_EPOCH)`), log
or warn on the Err case, and provide a safe fallback value (for example 0 or the
error's duration) when the duration cannot be obtained; update the code around
the `now` variable and the `SystemTime::now().duration_since(UNIX_EPOCH)`
expression to implement this defensive handling.
In `@extlib/cryptoscan/src/scanner.rs`:
- Around line 92-102: The loop currently uses WalkDir and then skips entries
with should_skip_path(path), causing WalkDir to descend into skipped
directories; change the iterator to use
WalkDir::new(project_path).into_iter().filter_entry(|e|
!should_skip_path(e.path())) so directories matched by should_skip_path are
pruned before descent, then keep the existing .filter_map(|e| e.ok()).filter(|e|
e.file_type().is_file()) logic; ensure should_skip_path(&Path) correctly
recognizes directory paths (e.g., node_modules) so filter_entry can prevent
descending into them.
In `@src/App/Fossa/Analyze.hs`:
- Around line 410-423: The code currently may run two full crypto scans because
analyzeWithCryptoScan and analyzeCryptoScanCBOM are invoked separately; change
the flow so the crypto scan is executed at most once: when enableCryptoScan is
true run analyzeWithCryptoScan once (keeping the existing
Diag.context/"crypto-scan" and runStickyLogger SevInfo usage) and store its
results in maybeCryptoScanResults, then when Config.cryptoCbomOutput is Just
cbomPath pass those results into analyzeCryptoScanCBOM (or change
analyzeCryptoScanCBOM to accept the scan results) to generate the CBOM instead
of calling analyzeCryptoScanCBOM which re-runs the scan; if
maybeCryptoScanResults is Nothing (scan disabled or failed) fall back to the
current behavior or skip CBOM as appropriate.
In `@src/App/Fossa/Analyze/Types.hs`:
- Line 15: The new import of CryptoScanResults should be made a qualified
explicit import to follow the project's Haskell guidelines: change the import of
App.Fossa.CryptoScan.Types so it is imported qualified (e.g., import qualified
App.Fossa.CryptoScan.Types as CryptoScan) and update usages to reference
CryptoScan.CryptoScanResults (or the chosen alias) so all references to
CryptoScanResults in Types.hs use the qualified name; ensure the import is
explicit and matches other qualified imports in the file (look for existing
patterns in Types.hs to pick the correct alias style).
In `@src/App/Fossa/CryptoScan/FipsReport.hs`:
- Around line 59-60: The current dedupe uses nubBy twice (uniqueAlgos and later
at lines ~72-73), which is O(n^2); replace both uses by a single Set/Map-based
dedupe pass: build a Set (or Map) of keys using dedupeKey over findings once
(e.g., fold over findings to collect first occurrence per key), produce one
uniqueAlgos list and then reuse it for statuses (map cryptoAlgorithmFipsStatus .
cryptoFindingAlgorithm) and for rendering; update references to uniqueAlgos,
nubBy and dedupeKey so the dedupe is O(n) and only performed once.
In `@test/App/Fossa/CryptoScan/FipsReportSpec.hs`:
- Around line 52-62: The test in FipsReportSpec currently only checks
deduplication by identical name; extend it to cover same base name with
different parameters so variants are distinct: add findings via
CryptoScanResults using mkFinding for "RSA-1024" and "RSA-2048" (or similar
differing parameter names) alongside duplicates like "AES-256-GCM", then call
computeFipsStats and assert totalAlgorithms (and approvedCount if relevant)
reflect both RSA variants counted separately (i.e., totalAlgorithms increases
accordingly) while still deduplicating exact name duplicates.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockextlib/cryptoscan/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (43)
Cargo.tomlMakefiledocs/features/crypto-scanning.mddocs/references/experimental/crypto-scanning/README.mddocs/references/subcommands/analyze.mdextlib/cryptoscan/Cargo.tomlextlib/cryptoscan/src/crypto_algorithm.rsextlib/cryptoscan/src/cyclonedx.rsextlib/cryptoscan/src/fips.rsextlib/cryptoscan/src/main.rsextlib/cryptoscan/src/patterns.rsextlib/cryptoscan/src/scanner.rsextlib/cryptoscan/tests/integration_test.rsspectrometer.cabalsrc/App/Fossa/Analyze.hssrc/App/Fossa/Analyze/ScanSummary.hssrc/App/Fossa/Analyze/Types.hssrc/App/Fossa/Config/Analyze.hssrc/App/Fossa/CryptoScan/Analyze.hssrc/App/Fossa/CryptoScan/FipsReport.hssrc/App/Fossa/CryptoScan/Types.hssrc/App/Fossa/EmbeddedBinary.hstest-fixtures/csharp-service/CryptoService.cstest-fixtures/csharp-service/Service.csprojtest-fixtures/elixir-phoenix-app/crypto.extest-fixtures/elixir-phoenix-app/mix.exstest-fixtures/go-api-server/go.modtest-fixtures/go-api-server/main.gotest-fixtures/java-microservice/pom.xmltest-fixtures/java-microservice/src/main/java/com/example/CryptoService.javatest-fixtures/node-auth-service/auth.jstest-fixtures/node-auth-service/package.jsontest-fixtures/php-api/composer.jsontest-fixtures/php-api/crypto.phptest-fixtures/python-web-app/app.pytest-fixtures/python-web-app/requirements.txttest-fixtures/ruby-web-app/Gemfiletest-fixtures/ruby-web-app/app.rbtest-fixtures/rust-crypto-tool/Cargo.tomltest-fixtures/rust-crypto-tool/src/main.rstest-fixtures/swift-ios-app/CryptoManager.swifttest-fixtures/swift-ios-app/Podfiletest/App/Fossa/CryptoScan/FipsReportSpec.hs
✅ Files skipped from review due to trivial changes (1)
- docs/references/experimental/crypto-scanning/README.md
🚧 Files skipped from review as they are similar to previous changes (15)
- Cargo.toml
- test-fixtures/rust-crypto-tool/Cargo.toml
- extlib/cryptoscan/src/main.rs
- test-fixtures/node-auth-service/package.json
- test-fixtures/ruby-web-app/app.rb
- docs/features/crypto-scanning.md
- test-fixtures/go-api-server/main.go
- test-fixtures/python-web-app/requirements.txt
- test-fixtures/elixir-phoenix-app/mix.exs
- extlib/cryptoscan/Cargo.toml
- src/App/Fossa/CryptoScan/Analyze.hs
- extlib/cryptoscan/tests/integration_test.rs
- test-fixtures/java-microservice/pom.xml
- test-fixtures/php-api/crypto.php
- test-fixtures/elixir-phoenix-app/crypto.ex
Introduce a new experimental feature (--x-crypto-scan) that detects cryptographic algorithm usage across 10 ecosystems (Python, Java, Go, Node, Rust, Ruby, C#, PHP, Swift, Elixir) and produces CycloneDX 1.7 CBOM output for FIPS compliance assessment. Key features: - Rust-based crypto detection engine (extlib/cryptoscan) with pattern matching for imports, API calls, dependency manifests, and config files - Auto-detection of ecosystems present in the project - FIPS 140-3 compliance classification (approved/deprecated/not-approved) - CycloneDX 1.7 CBOM file output (--crypto-cbom-output) - Detailed FIPS compliance report (--crypto-fips-report) with remediation recommendations and key-size warnings - Crypto scan results displayed in the analysis scan summary - 56 passing integration tests covering all 10 ecosystems Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
CortezFrazierJr
force-pushed
the
feature/crypto-scanning
branch
from
7cd1701 to
32f92a9
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
extlib/cryptoscan/src/scanner.rs (1)
91-101:⚠️ Potential issue | 🟠 MajorPrune skipped directories during traversal and avoid slash-based path matching.
Line 91 currently walks entire skipped trees before filtering, and Line 160’s
contains("/.../")matching is brittle for Windows/relative-root paths. This is both a performance and cross-platform correctness risk.♻️ Proposed fix
- for entry in WalkDir::new(project_path) - .into_iter() - .filter_map(|e| e.ok()) - .filter(|e| e.file_type().is_file()) + for entry in WalkDir::new(project_path) + .into_iter() + .filter_entry(|e| !should_skip_path(e.path())) + .filter_map(|e| e.ok()) + .filter(|e| e.file_type().is_file()) { let path = entry.path(); - - // Skip hidden directories and common non-source dirs - if should_skip_path(path) { - continue; - }fn should_skip_path(path: &Path) -> bool { - let path_str = path.to_string_lossy(); - let skip_dirs = [ - "/.git/", - "/node_modules/", - "/__pycache__/", - "/.venv/", - "/venv/", - "/target/", - "/dist/", - "/build/", - "/.tox/", - "/.mypy_cache/", - "/.pytest_cache/", - "/vendor/", - "/.gradle/", - "/.idea/", - "/.vscode/", - ]; - skip_dirs.iter().any(|d| path_str.contains(d)) + const SKIP_DIRS: &[&str] = &[ + ".git", "node_modules", "__pycache__", ".venv", "venv", "target", + "dist", "build", ".tox", ".mypy_cache", ".pytest_cache", + "vendor", ".gradle", ".idea", ".vscode", + ]; + path.iter().any(|seg| { + SKIP_DIRS + .iter() + .any(|skip| seg == std::ffi::OsStr::new(skip)) + }) }Run this read-only check to validate current behavior and expected failure cases:
#!/bin/bash set -euo pipefail echo "== Current scanner traversal + skip implementation ==" rg -n "WalkDir::new\\(project_path\\)|filter_entry|fn should_skip_path|skip_dirs|to_string_lossy\\(|contains\\(" extlib/cryptoscan/src/scanner.rs -A10 -B4 echo echo "== Demonstrate brittle slash-matching behavior ==" python3 <<'PY' cases = [ ("node_modules/pkg/a.js", "/node_modules/"), # relative root path ("/repo/node_modules/pkg/a.js", "/node_modules/"), # unix absolute (r"C:\repo\node_modules\pkg\a.js", "/node_modules/"), # windows-style ] for p, needle in cases: print(f"{p!r} contains {needle!r}: {needle in p}") PYExpected: relative-root and Windows-style cases return
Falsewith current substring strategy.Also applies to: 159-179
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extlib/cryptoscan/src/scanner.rs` around lines 91 - 101, The traversal is walking entire directory trees before filtering (inefficient) and uses brittle slash-based substring checks for skips; change WalkDir::new(project_path).into_iter() to use filter_entry so skipped directories are pruned during descent (i.e., call filter_entry(|e| !should_skip_path(e.path()) ) or similar) and update should_skip_path (and any code that currently does path.to_string_lossy().contains("/node_modules/") or similar) to perform path component checks using Path/OsStr APIs (e.g., path.components() or path.iter() and compare components to "node_modules", "vendor", etc.) to be cross-platform and not rely on "/" substrings; update references in scanner.rs where contains("/.../") is used (around the should_skip_path logic) accordingly.
🧹 Nitpick comments (5)
test-fixtures/rust-crypto-tool/src/main.rs (1)
1-13: Consider adding an explicit “fixture-only / non-compilable” header comment.This would prevent future false-positive review churn on compile-time concerns in this file.
Based on learnings: Test fixtures in
test-fixtures/*are regex pattern-matching targets and do not need to be compilable or runnable code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test-fixtures/rust-crypto-tool/src/main.rs` around lines 1 - 13, Add a short top-of-file comment in this test fixture indicating it's "fixture-only / non-compilable" to avoid compile-time review churn; place it above the existing imports (above the first "use aes_gcm::{Aes256Gcm, KeyInit, Nonce};" line) and clearly state that the file under test-fixtures is intentionally not meant to compile or run (mentioning the file contains example imports like aes_gcm, chacha20poly1305, ed25519_dalek, x25519_dalek, hkdf, argon2, etc. for context).test-fixtures/elixir-phoenix-app/crypto.ex (1)
2-4: Optional: make fixture intent explicit in@moduledoc.Adding fixture-purpose wording here would help avoid repeated runtime-semantics feedback on intentionally synthetic examples.
💡 Optional clarification patch
`@moduledoc` """ - Cryptographic utilities for the Phoenix application. + Cryptographic scanner fixture for Phoenix-style code. + This module is intentionally pattern-focused for regex detection coverage. """Based on learnings: test fixtures are pattern-matching targets and do not need to be functionally runnable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test-fixtures/elixir-phoenix-app/crypto.ex` around lines 2 - 4, The `@moduledoc` in crypto.ex is generic; make the fixture intent explicit by updating the module documentation string (`@moduledoc`) to state this module is a test fixture/synthetic example used for tests (not intended to be production-grade or fully runnable), so consumers and reviewers know its purpose; reference the existing `@moduledoc` and the crypto.ex module to locate and change the documentation text accordingly.test-fixtures/php-api/crypto.php (1)
1-4: Consider adding an explicit “fixture-only” note at the top.A one-line note that this file is intentionally non-production/non-runnable would reduce repeated false-positive review churn.
💡 Optional clarification patch
<?php +// NOTE: Scanner fixture file for regex pattern detection only. +// It intentionally prioritizes API-pattern coverage over runtime correctness. use phpseclib3\Crypt\AES; use phpseclib3\Crypt\RSA;Based on learnings: Test fixtures in
test-fixtures/are syntactic pattern targets for the crypto scanner.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test-fixtures/php-api/crypto.php` around lines 1 - 4, Add a one-line explicit "fixture-only" note at the top of test-fixtures/php-api/crypto.php to indicate this file is intentionally a non-production/runnable test fixture; place the comment before or after the opening <?php tag and reference the file's purpose and that it contains sample crypto usage (AES, RSA) to reduce scanner/review false positives.test-fixtures/swift-ios-app/CryptoManager.swift (1)
1-6: Add a top-level fixture-intent note to prevent future review churn.Consider a short header comment stating this file is a regex-detection fixture (not production crypto guidance), so future reviews don’t re-open runtime-hardening concerns.
Based on learnings: Test fixtures in `test-fixtures/*` are pattern-matching targets for regex-based detection and do not need to be compilable or runnable code.💡 Suggested comment addition
import CryptoKit import CommonCrypto import Foundation +// Test fixture for cryptoscan regex pattern detection. +// Intentionally includes diverse crypto API usages; not production-hardening guidance. struct CryptoManager {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test-fixtures/swift-ios-app/CryptoManager.swift` around lines 1 - 6, Add a short top-of-file fixture-intent comment above the imports stating this file is a regex-detection/test fixture (not production crypto guidance) so future reviewers know it’s for pattern-matching tests; place the note before the existing imports in CryptoManager to avoid churn, mention that the code may be non-compilable and is intentionally a test fixture, and keep the comment concise and clearly labeled (e.g., “TEST FIXTURE — regex detection only”).extlib/cryptoscan/tests/integration_test.rs (1)
45-46: Redundant JSON parsing inrun_scan_cyclonedx.
serde_json::from_slicealready returns aValue, so wrapping it again withserde_json::from_valueis unnecessary. This adds overhead without benefit.♻️ Suggested simplification
fn run_scan_cyclonedx(fixture: &str, ecosystem: &str) -> Value { let output = Command::cargo_bin("cryptoscan") .unwrap() .args(["--path", fixture_path(fixture).to_str().unwrap()]) .args(["--ecosystem", ecosystem]) .args(["--format", "cyclonedx"]) .output() .expect("failed to execute"); assert!(output.status.success(), "scanner exited with error"); - serde_json::from_value(serde_json::from_slice(&output.stdout).expect("invalid JSON")) - .expect("invalid CycloneDX structure") + serde_json::from_slice(&output.stdout).expect("invalid CycloneDX JSON") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extlib/cryptoscan/tests/integration_test.rs` around lines 45 - 46, In run_scan_cyclonedx simplify the redundant JSON parsing: remove the extra serde_json::from_value wrapper and use the Value returned by serde_json::from_slice(&output.stdout) directly (keep the existing expect call for error messaging), e.g., replace the serde_json::from_value(serde_json::from_slice(...).expect("invalid JSON")).expect("invalid CycloneDX structure") pattern with a single serde_json::from_slice(&output.stdout).expect("invalid CycloneDX/CycloneDX JSON") or equivalent so you parse once and preserve clear error text referencing the output variable and run_scan_cyclonedx.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/features/crypto-scanning.md`:
- Line 146: Update the table row text that currently reads "AES-128/192/256 (all
modes except ECB deprecated by 2030)" so it does not imply non-ECB modes are
deprecated; change the wording for the "Symmetric Encryption" / "AES-128/192/256
(all modes except ECB deprecated by 2030)" entry to explicitly state that ECB
mode is the deprecated one (for example: "AES-128/192/256 — ECB mode deprecated
by 2030; use other modes"), preserving the AES variants and the deprecation
deadline.
In `@extlib/cryptoscan/src/cyclonedx.rs`:
- Around line 117-126: The dedup branch that checks seen_algorithms and skips
emitting per-algorithm CBOM currently stops updating detection context; modify
the branch so that even when seen_algorithms.contains(&bom_ref) is true you
still merge/append the finding's detection metadata (fossa:detected-in,
fossa:detection-method, fossa:ecosystem) into the existing component/metadata
entry and into library_algorithms if applicable (use the same keys used when
first inserting), then continue; apply the same change to the analogous block
that spans the logic around lines 154-183 so duplicate detections augment the
existing metadata instead of dropping additional contexts.
In `@extlib/cryptoscan/src/scanner.rs`:
- Around line 139-140: The detection currently passes mat.as_str() into
resolve_algorithm but that value is ignored, so capture-based matches (e.g.,
hashlib.new or imported algorithm groups) are misclassified; update
resolve_algorithm to accept and use the matched_text parameter (remove the
`_matched_text` discard) and implement a helper normalize_detected_algorithm
that maps detected capture variants to canonical algorithm/FIPS labels (handle
patterns like hashlib.new("..."), imported aliases, and group captures), then
call normalize_detected_algorithm from resolve_algorithm for capture-based
rules; apply the same change to the other resolve_algorithm call sites (the
occurrences around the second block mentioned) so all capture-derived strings
influence final classification.
In `@src/App/Fossa/CryptoScan/Analyze.hs`:
- Around line 29-33: The current Aeson.eitherDecode error branch silently logs
and returns Nothing (using warn, toText, pure Nothing), which masks JSON parse
problems; change the Left err branch so it does not downgrade to an empty result
— include the full err text in the log and propagate the failure instead: either
throw an exception (e.g. throwIO or fail) with a descriptive message that
contains toText err, or return an explicit error variant rather than Nothing
according to the surrounding function's result type; update the Left err arm
(the case on Aeson.eitherDecode result) to log the error and propagate it
instead of pure Nothing.
---
Duplicate comments:
In `@extlib/cryptoscan/src/scanner.rs`:
- Around line 91-101: The traversal is walking entire directory trees before
filtering (inefficient) and uses brittle slash-based substring checks for skips;
change WalkDir::new(project_path).into_iter() to use filter_entry so skipped
directories are pruned during descent (i.e., call filter_entry(|e|
!should_skip_path(e.path()) ) or similar) and update should_skip_path (and any
code that currently does path.to_string_lossy().contains("/node_modules/") or
similar) to perform path component checks using Path/OsStr APIs (e.g.,
path.components() or path.iter() and compare components to "node_modules",
"vendor", etc.) to be cross-platform and not rely on "/" substrings; update
references in scanner.rs where contains("/.../") is used (around the
should_skip_path logic) accordingly.
---
Nitpick comments:
In `@extlib/cryptoscan/tests/integration_test.rs`:
- Around line 45-46: In run_scan_cyclonedx simplify the redundant JSON parsing:
remove the extra serde_json::from_value wrapper and use the Value returned by
serde_json::from_slice(&output.stdout) directly (keep the existing expect call
for error messaging), e.g., replace the
serde_json::from_value(serde_json::from_slice(...).expect("invalid
JSON")).expect("invalid CycloneDX structure") pattern with a single
serde_json::from_slice(&output.stdout).expect("invalid CycloneDX/CycloneDX
JSON") or equivalent so you parse once and preserve clear error text referencing
the output variable and run_scan_cyclonedx.
In `@test-fixtures/elixir-phoenix-app/crypto.ex`:
- Around line 2-4: The `@moduledoc` in crypto.ex is generic; make the fixture
intent explicit by updating the module documentation string (`@moduledoc`) to
state this module is a test fixture/synthetic example used for tests (not
intended to be production-grade or fully runnable), so consumers and reviewers
know its purpose; reference the existing `@moduledoc` and the crypto.ex module to
locate and change the documentation text accordingly.
In `@test-fixtures/php-api/crypto.php`:
- Around line 1-4: Add a one-line explicit "fixture-only" note at the top of
test-fixtures/php-api/crypto.php to indicate this file is intentionally a
non-production/runnable test fixture; place the comment before or after the
opening <?php tag and reference the file's purpose and that it contains sample
crypto usage (AES, RSA) to reduce scanner/review false positives.
In `@test-fixtures/rust-crypto-tool/src/main.rs`:
- Around line 1-13: Add a short top-of-file comment in this test fixture
indicating it's "fixture-only / non-compilable" to avoid compile-time review
churn; place it above the existing imports (above the first "use
aes_gcm::{Aes256Gcm, KeyInit, Nonce};" line) and clearly state that the file
under test-fixtures is intentionally not meant to compile or run (mentioning the
file contains example imports like aes_gcm, chacha20poly1305, ed25519_dalek,
x25519_dalek, hkdf, argon2, etc. for context).
In `@test-fixtures/swift-ios-app/CryptoManager.swift`:
- Around line 1-6: Add a short top-of-file fixture-intent comment above the
imports stating this file is a regex-detection/test fixture (not production
crypto guidance) so future reviewers know it’s for pattern-matching tests; place
the note before the existing imports in CryptoManager to avoid churn, mention
that the code may be non-compilable and is intentionally a test fixture, and
keep the comment concise and clearly labeled (e.g., “TEST FIXTURE — regex
detection only”).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockextlib/cryptoscan/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (43)
Cargo.tomlMakefiledocs/features/crypto-scanning.mddocs/references/experimental/crypto-scanning/README.mddocs/references/subcommands/analyze.mdextlib/cryptoscan/Cargo.tomlextlib/cryptoscan/src/crypto_algorithm.rsextlib/cryptoscan/src/cyclonedx.rsextlib/cryptoscan/src/fips.rsextlib/cryptoscan/src/main.rsextlib/cryptoscan/src/patterns.rsextlib/cryptoscan/src/scanner.rsextlib/cryptoscan/tests/integration_test.rsspectrometer.cabalsrc/App/Fossa/Analyze.hssrc/App/Fossa/Analyze/ScanSummary.hssrc/App/Fossa/Analyze/Types.hssrc/App/Fossa/Config/Analyze.hssrc/App/Fossa/CryptoScan/Analyze.hssrc/App/Fossa/CryptoScan/FipsReport.hssrc/App/Fossa/CryptoScan/Types.hssrc/App/Fossa/EmbeddedBinary.hstest-fixtures/csharp-service/CryptoService.cstest-fixtures/csharp-service/Service.csprojtest-fixtures/elixir-phoenix-app/crypto.extest-fixtures/elixir-phoenix-app/mix.exstest-fixtures/go-api-server/go.modtest-fixtures/go-api-server/main.gotest-fixtures/java-microservice/pom.xmltest-fixtures/java-microservice/src/main/java/com/example/CryptoService.javatest-fixtures/node-auth-service/auth.jstest-fixtures/node-auth-service/package.jsontest-fixtures/php-api/composer.jsontest-fixtures/php-api/crypto.phptest-fixtures/python-web-app/app.pytest-fixtures/python-web-app/requirements.txttest-fixtures/ruby-web-app/Gemfiletest-fixtures/ruby-web-app/app.rbtest-fixtures/rust-crypto-tool/Cargo.tomltest-fixtures/rust-crypto-tool/src/main.rstest-fixtures/swift-ios-app/CryptoManager.swifttest-fixtures/swift-ios-app/Podfiletest/App/Fossa/CryptoScan/FipsReportSpec.hs
🚧 Files skipped from review as they are similar to previous changes (23)
- Makefile
- test-fixtures/java-microservice/pom.xml
- test-fixtures/node-auth-service/auth.js
- test-fixtures/php-api/composer.json
- extlib/cryptoscan/src/main.rs
- test-fixtures/python-web-app/requirements.txt
- test-fixtures/go-api-server/go.mod
- test-fixtures/elixir-phoenix-app/mix.exs
- test-fixtures/rust-crypto-tool/Cargo.toml
- Cargo.toml
- src/App/Fossa/EmbeddedBinary.hs
- test-fixtures/python-web-app/app.py
- test-fixtures/csharp-service/Service.csproj
- test-fixtures/ruby-web-app/Gemfile
- extlib/cryptoscan/Cargo.toml
- spectrometer.cabal
- test-fixtures/node-auth-service/package.json
- src/App/Fossa/Analyze.hs
- test-fixtures/swift-ios-app/Podfile
- test/App/Fossa/CryptoScan/FipsReportSpec.hs
- docs/references/experimental/crypto-scanning/README.md
- extlib/cryptoscan/src/crypto_algorithm.rs
- test-fixtures/ruby-web-app/app.rb
Member
|
DM'd Cortez that there is still some work to do. Taking myself off of review until that is done |
…aggregation, parse error handling, and doc wording - scanner.rs: Add normalize_detected_algorithm() to use matched text for refining generic pattern names into specific algorithm variants (e.g., AES -> AES-256-GCM based on matched code context) - cyclonedx.rs: Aggregate detection contexts (detected-in, detection-method, ecosystem) from all findings per algorithm instead of only keeping the first - Analyze.hs: Use fatalText instead of warn+Nothing for JSON parse failures so scanner output-contract regressions surface as hard errors - crypto-scanning.md: Fix wording that implied non-ECB AES modes are deprecated Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ng, add doc entries - FipsReport.hs: Convert all match guards to if/then/else per style guide (compliancePercentage, coloredPercentage, suggestAlternative, catWarnings, rsaWarning). Remove unused matchesAny function. - scanner.rs: Use filter_entry() to prune skipped directories during WalkDir traversal instead of filtering after entry. Replace brittle slash-based path matching with file_name() segment comparison for cross-platform correctness. - analyze.md: Add --crypto-cbom-output and --crypto-fips-report to the experimental flags table for discoverability. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/features/crypto-scanning.md`:
- Around line 76-81: Update the docs to accurately state where FIPS
classification is emitted: replace the claim that FIPS level is in
algorithmProperties with a note that the generator currently sets FIPS info as a
component property named "fossa:fips-status" (while "certification_level"
remains ["none"]); reference the exact fields "algorithmProperties",
"fossa:fips-status", and "certification_level" in the text so integrators know
to read the component property "fossa:fips-status" for FIPS status rather than
looking under algorithmProperties or certification_level.
In `@extlib/cryptoscan/src/scanner.rs`:
- Around line 205-238: The function normalize_detected_algorithm currently
infers key sizes from mode-only matches (e.g., mapping GCM/CBC to AES-256 and
collapsing AES-192 back to "AES"), which corrupts CBOM key-size metadata and
downstream resolution; update normalize_detected_algorithm to stop guessing key
sizes from mode mentions (do not default GCM/CBC to 256 or remap "AES-192" to
generic "AES") and leave plain "AES" size-agnostic, then update
resolve_algorithm to explicitly handle AES-192 variants (add AES-192* arms) and
ensure resolve_algorithm does not treat the generic "AES" as AES-128 later.
Ensure references: normalize_detected_algorithm and resolve_algorithm are the
only places changed so the scanner preserves mode info but not inferred key
sizes.
- Around line 246-256: The RSA size extraction returns format!("RSA-{}", bits)
(see variables name, key_size, bits) but the resolver only handles the "RSA" arm
so RSA-<bits> falls through; either change this block to return "RSA"
(preserving bits in a new field if needed) or update the resolver's matching to
treat "RSA-<bits>" as "RSA" (e.g. match names that equal "RSA" or start_with
"RSA-") so extracted sizes keep their Primitive/security metadata; update all
similar blocks (including the region around lines 275-339) to follow the same
convention.
In `@src/App/Fossa/CryptoScan/Analyze.hs`:
- Around line 29-34: The debug log currently prints the entire decoded payload
via "logDebug $ \"Cryptoscan completed: \" <> pretty (show findings)" which can
expose file paths and source snippets; update the handling after
Aeson.eitherDecode (around the Right findings branch) to avoid dumping
"findings" verbatim—use a terse message or include only a safe metric (e.g.,
number of findings or summary) instead, and keep the fatalText behavior
unchanged; locate the Right findings branch in Analyze.hs and replace the
logDebug call to log a count/summary rather than show findings.
In `@src/App/Fossa/CryptoScan/FipsReport.hs`:
- Around line 233-251: The inner parameter name in catWarnings shadows the outer
binding name from keySizeWarning; rename the catWarnings parameter (for example
to algoName or lowerName) and update all uses inside catWarnings (the pattern
checks and comparisons like "rsa" `Text.isInfixOf` name, "aes-128", paramSet
checks) accordingly so that keySizeWarning, catWarnings, cryptoFindingAlgorithm
and cryptoAlgorithmParameterSet remain the same but there is no shadowing
between the outer name and the inner parameter.
- Around line 147-152: The current categorizeFindings uses foldl which prepends
findings and reverses order; change to use foldr so original input order is
preserved and insert remains O(1) per element: replace foldl categorize
Map.empty with foldr categorize Map.empty in categorizeFindings and keep the
inner categorize (which computes cat via classifyPrimitive
(cryptoAlgorithmPrimitive $ cryptoFindingAlgorithm finding)) inserting the
single-item list for the finding into the Map with Map.insertWith (++) so the
foldr traversal yields the original ordering.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7e2bbcf5-f3ae-455b-9143-b4017e4e2c29
📒 Files selected for processing (6)
docs/features/crypto-scanning.mddocs/references/subcommands/analyze.mdextlib/cryptoscan/src/cyclonedx.rsextlib/cryptoscan/src/scanner.rssrc/App/Fossa/CryptoScan/Analyze.hssrc/App/Fossa/CryptoScan/FipsReport.hs
…, regression test, clock safety - cyclonedx.rs: Sort unique_algos for deterministic BOM output across runs. Use unwrap_or_default() for SystemTime to avoid panic on clock anomalies. - FipsReport.hs: Replace quadratic nubBy with Map-based deduplicateFindings for O(n log n) deduplication. Reuse single dedupe pass in both computeFipsStats and renderFipsReport. - FipsReportSpec.hs: Add regression test verifying RSA-1024 and RSA-2048 are counted as separate algorithm variants (different parameter sets). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…em detection, variable shadowing - Don't guess AES key sizes from mode-only detections (AES-GCM → None, not 256) - Add AES-192 variants for GCM and CBC modes - Add RSA-<bits> dynamic matching in resolve_algorithm fallthrough - Rewrite detect_ecosystems with filter_entry pruning and source extension detection - Fix foldl→foldr in categorizeFindings for proper ordering - Fix variable shadowing in keySizeWarning (name→algoName, name/paramSet→n/ps) - Change debug logging to show finding count instead of full payload - Separate FIPS status from algorithmProperties in CBOM docs Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@extlib/cryptoscan/src/cyclonedx.rs`:
- Around line 132-135: Implement a direct string conversion on the enum types
instead of serializing to JSON: add an as_str(&self) -> &'static str method to
Primitive (and similarly to DetectionMethod) that returns the literal for each
variant, then replace the serde_json::to_value(...) round-trip in the
cyclonedx.rs snippet (where first.algorithm.primitive is used) with
first.algorithm.primitive.as_str().to_string() (or .to_owned()) and keep the
"unknown" fallback only if needed for any non-exhaustive cases.
- Around line 235-241: Replace the collect-into-HashSet-then-sort pattern for
algo_refs with a single BTreeSet-backed collection: stop collecting into a
HashSet and Vec (the code around unique_algos and algo_refs), instead collect
directly into a BTreeSet to both deduplicate and produce sorted iteration, then
convert or iterate from that BTreeSet where unique_algos is used; update
references to unique_algos to expect the BTreeSet or a Vec collected from it as
needed.
- Around line 321-331: The fallback setting month = 12 is unreachable given a
valid days remainder; replace that branch with a defensive assertion or
unreachable marker and a short comment: e.g., after the loop assert month != 0
with debug_assert!(month != 0, "month must be set from months_days loop for
valid days") or use unreachable!("day-of-year must map to a month") to document
invariants; refer to the variables months_days, month and days when making this
change so the loop behavior is preserved and the impossible case is explicitly
asserted rather than silently handled.
- Around line 114-125: Compute bom_ref once per iteration with let bom_ref =
make_bom_ref(&finding.algorithm.name, &finding.algorithm.oid); then reuse it,
cloning where entry consumes the value: call
algo_findings.entry(bom_ref.clone()).or_default().push(finding); and in the
provides branch use
library_algorithms.entry(lib.clone()).or_default().push(bom_ref); referencing
make_bom_ref, algo_findings, library_algorithms, finding.providing_library and
the findings loop so the single bom_ref is reused instead of recomputed.
In `@src/App/Fossa/CryptoScan/FipsReport.hs`:
- Around line 77-94: renderFipsReport is deduplicating findings twice;
deduplicate once into a local (e.g. let deduped = deduplicateFindings findings)
and use that single value for computeFipsStats, categorizeFindings,
renderRemediationTable, and renderKeySizeWarnings. Either change
computeFipsStats to accept pre-deduplicated findings or add a small helper (e.g.
computeFipsStatsFromDedup) that takes the deduped list, then update
renderFipsReport to call that helper instead of letting computeFipsStats call
deduplicateFindings internally.
In `@test/App/Fossa/CryptoScan/FipsReportSpec.hs`:
- Around line 16-72: Add a smoke test for renderFipsReport in the same spec file
so the function is exercised: call renderFipsReport with a small
CryptoScanResults value (e.g., empty results and one with mixed statuses) and
assert it returns a non-empty Text/String (or contains an expected substring
like "FIPS" or a header) and does not throw; reference the renderFipsReport
function and reuse existing helpers (mkFinding / mkAlgorithm /
CryptoScanResults) to construct inputs and keep the test lightweight to ensure
it only verifies successful rendering.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6ea9616b-3115-42c6-86a9-6ad9fa099143
📒 Files selected for processing (3)
extlib/cryptoscan/src/cyclonedx.rssrc/App/Fossa/CryptoScan/FipsReport.hstest/App/Fossa/CryptoScan/FipsReportSpec.hs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@extlib/cryptoscan/src/scanner.rs`:
- Around line 150-171: The call to is_some_and in should_skip_path requires Rust
1.70 but the crate lacks a rust-version; either declare the minimum Rust version
by adding rust-version = "1.70" to extlib/cryptoscan/Cargo.toml, or make the
function compatible with older compilers by replacing the is_some_and usage on
path.file_name().and_then(|s| s.to_str()) with a map_or(false, |name|
SKIP_DIRS.contains(&name)) pattern so SKIP_DIRS and should_skip_path remain the
same.
In `@extlib/cryptoscan/tests/integration_test.rs`:
- Around line 45-46: The test is performing redundant double deserialization by
calling serde_json::from_value on the result of serde_json::from_slice; remove
the unnecessary from_value and deserialize directly from the byte slice
(output.stdout) into the expected CycloneDX type using serde_json::from_slice,
i.e. replace the nested call involving serde_json::from_value and
serde_json::from_slice with a single
serde_json::from_slice::<ExpectedType>(&output.stdout).expect("invalid CycloneDX
structure"), leaving the error message and the source (output.stdout) intact;
reference the symbols serde_json::from_slice, serde_json::from_value and
output.stdout to locate and update the code.
In `@src/App/Fossa/CryptoScan/Analyze.hs`:
- Around line 52-66: Both cryptoScanCommand and cryptoScanCycloneDxCommand
duplicate construction except for the format flag; replace them with a single
parameterized builder (e.g., cryptoScanCommandWithFormat or cryptoScanCommand ::
Format -> BinaryPaths -> Path Abs Dir -> Command) that takes the desired format
string/enum and builds the Command using that format for cmdArgs, then update
call sites (analyzeWithCryptoScan and analyzeCryptoScanCBOM) to call the new
builder with the appropriate format (e.g., JsonFormat / CycloneDxFormat) instead
of the two separate functions; keep cmdName, cmdAllowErr and other args
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: ceaed2be-6582-4a44-a0d6-60feec0a8ae6
📒 Files selected for processing (5)
docs/features/crypto-scanning.mdextlib/cryptoscan/src/scanner.rsextlib/cryptoscan/tests/integration_test.rssrc/App/Fossa/CryptoScan/Analyze.hssrc/App/Fossa/CryptoScan/FipsReport.hs
…lder, smoke tests - Declare rust-version = "1.70" in Cargo.toml for is_some_and compatibility - Add Primitive::as_str() to avoid serde round-trip for string conversion - Reuse bom_ref computation instead of computing twice per loop iteration - Use BTreeSet for combined dedup+sort in CycloneDX dependency generation - Replace defensive month=12 fallback with debug_assert - Fix double deduplication in renderFipsReport (dedupe once, pass to stats) - Extract mkCryptoScanCommand to reduce command builder duplication - Remove redundant double deserialization in integration test - Add renderFipsReport smoke tests for empty and mixed results Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@extlib/cryptoscan/Cargo.toml`:
- Around line 12-18: The Cargo.toml dependencies in extlib/cryptoscan use loose
semver (e.g., serde = "1", clap = "4") which can cause duplicate deps under
resolver = "2"; update the dependency entries in extlib/cryptoscan/Cargo.toml to
use pinned patch versions to match the sibling extlib/millhone crate (at minimum
set serde to "1.0.183" and clap to "4.3.21" and pin serde_json, regex, walkdir,
uuid to exact patch versions used by millhone or your workspace canonical
versions) so all crates resolve the same exact versions.
In `@extlib/cryptoscan/src/crypto_algorithm.rs`:
- Around line 96-104: Add an as_str() on the DetectionMethod enum that returns
the kebab-case static &str for each variant (e.g., DependencyManifest ->
"dependency-manifest", ImportStatement -> "import-statement", ApiCall ->
"api-call", ConfigFile -> "config-file", StringLiteral -> "string-literal") to
mirror Primitive::as_str(); update places that currently call
serde_json::to_value(&detection_method) (e.g., the cyclonedx serialization path)
to use detection_method.as_str() instead to avoid serde_json round-trips and
keep behavior consistent with the serde rename_all rule.
In `@extlib/cryptoscan/src/cyclonedx.rs`:
- Around line 173-182: The code does an unnecessary serde_json::to_value
round-trip to get a string for finding.detection_method; replace that with a
direct call to the new DetectionMethod::as_str() helper (e.g. let method_str =
finding.detection_method.as_str().to_string()) and then use that string for
seen_methods.insert(...) and the BomProperty value when building properties
(referencing finding.detection_method, seen_methods, properties, and
BomProperty). This removes the extra allocation and simplifies the logic while
preserving behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 37e36dae-82d3-4ab1-9f57-7c2ac2df3285
📒 Files selected for processing (7)
extlib/cryptoscan/Cargo.tomlextlib/cryptoscan/src/crypto_algorithm.rsextlib/cryptoscan/src/cyclonedx.rsextlib/cryptoscan/tests/integration_test.rssrc/App/Fossa/CryptoScan/Analyze.hssrc/App/Fossa/CryptoScan/FipsReport.hstest/App/Fossa/CryptoScan/FipsReportSpec.hs
- Pin dependency versions to match millhone for workspace consistency - Add DetectionMethod::as_str() for parity with Primitive::as_str() - Replace serde round-trip for DetectionMethod in cyclonedx.rs Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@extlib/cryptoscan/src/crypto_algorithm.rs`:
- Around line 32-51: The Primitive enum is missing the CycloneDX 1.7 `key-wrap`
value; add a new variant named KeyWrap to the Primitive enum (follow existing
PascalCase convention) and update the Primitive::as_str() implementation to
return "key-wrap" for that variant so key-wrapping algorithms are
serialized/deserialized correctly (update any match arms in
as_str()/FromStr/serde helpers that map enum variants to kebab-case strings).
In `@extlib/cryptoscan/src/cyclonedx.rs`:
- Around line 110-125: The library grouping currently keys library_algorithms by
providing_library only, which collapses providers from different ecosystems;
change the key to include ecosystem (e.g., use a tuple or concatenated string of
(finding.ecosystem, providing_library)) wherever library_algorithms is populated
(see library_algorithms and algo_findings loops and make_bom_ref usage), update
the creation of the bom-ref for library components to incorporate ecosystem
(carry finding.ecosystem into the emitted lib/<name> bom-ref), and adjust
downstream lookups that consume library_algorithms and any emitted library
component identifiers so they use the new ecosystem-aware key/identifier.
- Around line 136-139: When constructing AlgorithmProperties in cyclonedx.rs
(the AlgorithmProperties struct with fields primitive and algorithm_family)
validate the value coming from first.algorithm.algorithm_family against the
CycloneDX 1.7 allowed algorithmFamily enum values before wrapping it in
Some(...); if the value is not in the approved registry (i.e. comes from
resolve_algorithm() fallback), set algorithm_family to None instead of
Some(arbitrary_string). Implement a small helper or lookup that checks the
string against the known CycloneDX algorithmFamily set and use that to decide
whether to assign Some(value) or None when creating AlgorithmProperties.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: d79619de-cc45-445e-9fd9-dcf95524dd4d
📒 Files selected for processing (3)
extlib/cryptoscan/Cargo.tomlextlib/cryptoscan/src/crypto_algorithm.rsextlib/cryptoscan/src/cyclonedx.rs
…ed libs, algorithmFamily validation - Add KeyWrap variant to Primitive enum per CycloneDX 1.7 schema - Scope library identity by (ecosystem, library_name) to prevent cross-ecosystem collisions - Validate algorithmFamily against CycloneDX 1.7 registry, emit None for unknown values Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@extlib/cryptoscan/src/cyclonedx.rs`:
- Around line 110-114: Replace the non-deterministic HashMap usages with ordered
BTreeMap so iteration over library_algorithms and algo_findings is stable:
change the types from HashMap<(String, String), Vec<String>> and HashMap<String,
Vec<&CryptoFinding>> to BTreeMap<(String, String), Vec<String>> and
BTreeMap<String, Vec<&CryptoFinding>> respectively, update the declaration
symbols library_algorithms and algo_findings, import std::collections::BTreeMap,
and ensure any insertion/lookup logic (the code that populates and later
iterates these maps to build components and dependencies) continues to work
unchanged since tuple (String,String) and String implement Ord.
- Around line 270-284: The valid_algorithm_family function currently returns the
input string and uses a broken KNOWN_FAMILIES list; replace KNOWN_FAMILIES with
the official CycloneDX 1.7 canonical enum values (including the 41 missing
entries and fixing case like CAMELLIA) and remove the 13 invalid entries, then
change the membership check to search with .find(|&known|
known.eq_ignore_ascii_case(family)) and return Some(known.to_string()) (i.e.,
the canonical value) instead of the raw input; update references to the
KNOWN_FAMILIES constant and the valid_algorithm_family function accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: a5445d9d-92f1-4bd8-811c-a7b47e4e1e28
📒 Files selected for processing (2)
extlib/cryptoscan/src/crypto_algorithm.rsextlib/cryptoscan/src/cyclonedx.rs
…ithmFamily - Replace HashMap with BTreeMap for algo_findings and library_algorithms to ensure deterministic component/dependency ordering in CBOM output - Replace custom KNOWN_FAMILIES with official CycloneDX 1.7 algorithmFamily enum (78 canonical entries) and return canonical case via .find() Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
extlib/cryptoscan) that scans source code across 10 ecosystems (Python, Java, Go, Rust, Node.js, Ruby, C#/.NET, PHP, Swift, Elixir) for cryptographic algorithm usage via dependency analysis, import patterns, API calls, and config file scanningSourceUnitformat and included in uploads alongside existing dependency analysis--x-crypto-scanflag to enable scanning,--crypto-cbom-output <file>for local CycloneDX 1.7 CBOM export, and--crypto-fips-reportfor FIPS 140-3 compliance reportingfossabinary (same pattern asberkeleydb/millhone) — no separate distribution neededWhat's included
Phase 1 — Rust Crypto Detection Engine (
extlib/cryptoscan/)cryptographic-assetcomponents,cryptoProperties, andprovidesdependenciesPhase 2 — Haskell Integration
CryptoScan.Analyze— invokes embedded binary, parses JSON resultsCryptoScan.SourceUnit— converts findings to FOSSASourceUnitwithcrypto+<algorithm>locatorsCryptoScan.Types— fullFromJSON/ToJSONfor all crypto typesAnalyze.hspipeline: cryptoSourceUnitincluded in upload alongside dependency resultsPhase 3 — CycloneDX 1.7 CBOM Output
--crypto-cbom-output <file>writes standards-compliant CycloneDX 1.7 JSON--format cyclonedxoutput (complete BOM with components, dependencies, metadata)Phase 4 — FIPS Compliance Reporting
--crypto-fips-reportrenders a color-coded compliance summary to stdoutScanSummaryoutputEcosystems supported
Test plan
cargo test -p cryptoscan)SourceUnitconversion and FIPS report renderingfossa analyze --x-crypto-scan --outputproduces valid JSON with crypto SourceUnit--crypto-cbom-output /tmp/cbom.jsonwrites valid CycloneDX 1.7--crypto-fips-reportrenders compliance summary🤖 Generated with Claude Code