Conversation
WalkthroughAdds a Reader abstraction for file/password input in internal/certinfo, updates cert/key/CA loading APIs and all callers (internal/requests and cmd) to accept a Reader, adds extensive tests and testdata, and updates linting/gitignore/devenv entries. (26 words) Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as cmd (certinfo/requests)
participant Requests as internal/requests
participant Certinfo as internal/certinfo
participant Reader as certinfo.Reader
participant FS as FileSystem/TTY
CLI->>Requests: SetCaPoolFromFile(path, fileReader)
Requests->>Certinfo: GetRootCertsFromFile(path, fileReader)
Certinfo->>Reader: ReadFile(path) / ReadPassword(fd)
Reader->>FS: OS read or TTY prompt
FS-->>Reader: bytes / password / error
Reader-->>Certinfo: bytes / password / error
Certinfo->>Certinfo: parse/decrypt -> x509.CertPool / private key / error
Certinfo-->>Requests: CertPool / key / error
Requests-->>CLI: apply pool / propagate error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)internal/requests/requests.go (2)
🔇 Additional comments (2)
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}
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)
internal/requests/requests_test.go (1)
25-31: Fix contradictory assertion flow: you call bothrequire.Errorandrequire.NoErrorfor the sameerr.
This should be one or the other.rmc, err := NewRequestsMetaConfig() - if err != nil { - require.Error(t, err, "error when calling NewRequestsMetaConfig()") - } - - require.NoError(t, err) + require.NoError(t, err, "error when calling NewRequestsMetaConfig()")
🧹 Nitpick comments (12)
.golangci.yml (1)
57-62: Configuration looks good, but note potential linter redundancy.The new cyclomatic complexity rule is properly formatted and uses the same 15-threshold as the existing gocyclo and cognitive-complexity checks. However, you now have two checks analyzing cyclomatic complexity: gocyclo (line 28–29) and revive's cyclomatic rule (lines 57–62) with identical thresholds. This redundancy is harmless but could be worth consolidating in a future refactoring if you prefer a single source of truth.
.gitignore (1)
1-6: Harden ignore rules for cert/key fixtures (current*Cert*is too narrow).
If you generate/store PEMs/keys underinternal/certinfo/testdata, consider ignoring by extension to prevent accidental commits (while still keeping README/sample text tracked).internal/requests/testdata -internal/certinfo/testdata/*Cert* +internal/certinfo/testdata/*.pem +internal/certinfo/testdata/*.crt +internal/certinfo/testdata/*.key +internal/certinfo/testdata/*Cert*cmd/root.go (1)
25-48: Prefer storingfileReaderas the interface type (certinfo.Reader) to keep DI flexible.
Makes swapping mocks/stubs in cmd tests easier and avoids method-set surprises ifInputReaderever uses pointer receivers.var ( @@ - fileReader certinfo.InputReader + fileReader certinfo.Reader = certinfo.InputReader{} )internal/certinfo/certinfo_test.go (2)
22-29:mockReader.ReadFilemutates state that won’t persist (value receiver); simplify.
State isn’t used; just return the error.type mockReader struct { - readError error } func (mr mockReader) ReadFile(name string) ([]byte, error) { - mr.readError = fmt.Errorf("unable to read file %s", name) - return nil, mr.readError + return nil, fmt.Errorf("unable to read file %s", name) }
41-60: Consider loosening error assertions + resolve TODO before merging if it’s meant to gate behavior.
assert.Equal(err.Error())can become flaky if upstream wraps/changes messages.Also applies to: 62-65
internal/requests/requests_test.go (1)
891-894: (Future hardening) Consider settingtls.Config.MinVersionin tests once legacy compatibility is less critical.
Based on learnings, this has been a worthwhile follow-up in this repo when feasible.Also applies to: 998-1001, 1160-1162
internal/certinfo/certinfo_handlers.go (1)
113-116: Design inconsistency: PrintData uses package-levelinputReaderinstead of injected reader.The
GetCertsFromBundlecall uses the package-levelinputReadervariable, while other methods in this package (e.g.,SetCaPoolFromFile,SetCertsFromFile) accept aReaderparameter for dependency injection. This inconsistency makesPrintDataharder to test in isolation.Consider accepting a
Readerparameter inPrintDataor onCertinfoConfigto maintain consistent DI across the package.internal/certinfo/common_handlers_test.go (1)
357-369: Platform-specific error message may cause test fragility.The expected error
"inappropriate ioctl for device"is a Unix-specific message fromterm.ReadPasswordwhen stdin is not a terminal. This test may fail on Windows or in certain CI environments with different error messages.Consider using
require.Error(t, err)without asserting the exact message, or useerrors.Is/errors.Asto check for a specific error type if available.internal/certinfo/main_test.go (4)
88-88: Unused variable declared.
RSASampleCertBundleFileis declared but never initialized or used in the test setup functions. Consider removing it or implementing the bundle file generation if needed for tests.
148-163: Consider handling key generation errors.Ignoring the error from
RSAGenerateKeyon line 152 could cause nil pointer issues if key generation fails unexpectedly. For test reliability, consider logging or panicking on setup failures.- RSASampleCertKey, _ = RSAGenerateKey(2048) + var err error + RSASampleCertKey, err = RSAGenerateKey(2048) + if err != nil { + panic(fmt.Sprintf("failed to generate RSA sample cert key: %v", err)) + }
189-203: Same suggestion: handle key generation errors.Line 193 ignores the error from
RSAGenerateKey. Consider the same improvement as suggested forgenerateRSACertificateData.
265-267: Minor typo in comment."udpate" should be "update".
- // in case of CA cert we udpate the template with the proper fields + // in case of CA cert we update the template with the proper fields
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (18)
internal/certinfo/testdata/ecdsa-crt.pemis excluded by!**/*.peminternal/certinfo/testdata/ecdsa-encrypted-broken-private-key.pemis excluded by!**/*.peminternal/certinfo/testdata/ecdsa-encrypted-private-key.pemis excluded by!**/*.peminternal/certinfo/testdata/ecdsa-plaintext-private-key.pemis excluded by!**/*.peminternal/certinfo/testdata/ed25519-crt.pemis excluded by!**/*.peminternal/certinfo/testdata/ed25519-encrypted-broken-private-key.pemis excluded by!**/*.peminternal/certinfo/testdata/ed25519-encrypted-private-key.pemis excluded by!**/*.peminternal/certinfo/testdata/ed25519-plaintext-private-key.pemis excluded by!**/*.peminternal/certinfo/testdata/rsa-pkcs1-crt.pemis excluded by!**/*.peminternal/certinfo/testdata/rsa-pkcs1-csr.pemis excluded by!**/*.peminternal/certinfo/testdata/rsa-pkcs1-encrypted-broken-private-key.pemis excluded by!**/*.peminternal/certinfo/testdata/rsa-pkcs1-encrypted-private-key.pemis excluded by!**/*.peminternal/certinfo/testdata/rsa-pkcs1-plaintext-private-key.pemis excluded by!**/*.peminternal/certinfo/testdata/rsa-pkcs8-crt.pemis excluded by!**/*.peminternal/certinfo/testdata/rsa-pkcs8-csr.pemis excluded by!**/*.peminternal/certinfo/testdata/rsa-pkcs8-encrypted-broken-private-key.pemis excluded by!**/*.peminternal/certinfo/testdata/rsa-pkcs8-encrypted-private-key.pemis excluded by!**/*.peminternal/certinfo/testdata/rsa-pkcs8-plaintext-private-key.pemis excluded by!**/*.pem
📒 Files selected for processing (16)
.gitignore(1 hunks).golangci.yml(1 hunks)cmd/certinfo.go(1 hunks)cmd/requests.go(1 hunks)cmd/root.go(2 hunks)devenv.nix(1 hunks)internal/certinfo/certinfo.go(5 hunks)internal/certinfo/certinfo_handlers.go(1 hunks)internal/certinfo/certinfo_test.go(1 hunks)internal/certinfo/common_handlers.go(5 hunks)internal/certinfo/common_handlers_test.go(1 hunks)internal/certinfo/main_test.go(1 hunks)internal/certinfo/testdata/README.md(1 hunks)internal/certinfo/testdata/sample-text.txt(1 hunks)internal/requests/requests.go(1 hunks)internal/requests/requests_test.go(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: xenOs76
Repo: xenOs76/https-wrench PR: 10
File: internal/requests/requests_test.go:918-997
Timestamp: 2025-11-29T14:05:35.000Z
Learning: In the https-wrench tool test suite (xenOs76/https-wrench repository), the user wants to be reminded in the future about potentially adding MinVersion enforcement to TLS configurations in test code, once legacy server compatibility becomes less critical.
📚 Learning: 2025-11-15T17:16:55.075Z
Learnt from: xenOs76
Repo: xenOs76/https-wrench PR: 5
File: devenv.nix:569-569
Timestamp: 2025-11-15T17:16:55.075Z
Learning: In the xenOs76/https-wrench repository, the devenv.nix test environment is local-only without GitHub CI integration, so automatic dependency updates in enterTest are acceptable for the maintainer's workflow.
Applied to files:
devenv.nix
🧬 Code graph analysis (9)
internal/certinfo/certinfo_handlers.go (1)
internal/certinfo/common_handlers.go (1)
GetCertsFromBundle(99-139)
internal/certinfo/certinfo_test.go (2)
internal/certinfo/certinfo.go (1)
NewCertinfoConfig(71-82)internal/certinfo/main_test.go (2)
RSACaCertKeyFile(75-75)RSACaCertFile(79-79)
internal/requests/requests_test.go (2)
internal/certinfo/certinfo.go (1)
InputReader(45-47)internal/requests/requests.go (1)
NewRequestsMetaConfig(132-143)
internal/certinfo/main_test.go (1)
internal/certinfo/certinfo.go (1)
Reader(40-43)
internal/certinfo/common_handlers_test.go (1)
internal/certinfo/common_handlers.go (5)
GetRootCertsFromFile(68-84)GetRootCertsFromString(86-97)GetCertsFromBundle(99-139)GetKeyFromFile(246-262)IsPrivateKeyEncrypted(143-158)
cmd/root.go (1)
internal/certinfo/certinfo.go (1)
InputReader(45-47)
internal/certinfo/common_handlers.go (1)
internal/certinfo/certinfo.go (1)
Reader(40-43)
internal/requests/requests.go (2)
internal/certinfo/certinfo.go (1)
Reader(40-43)internal/certinfo/common_handlers.go (1)
GetRootCertsFromFile(68-84)
internal/certinfo/certinfo.go (1)
internal/certinfo/common_handlers.go (3)
GetRootCertsFromFile(68-84)GetCertsFromBundle(99-139)GetKeyFromFile(246-262)
🪛 Gitleaks (8.30.0)
internal/certinfo/testdata/README.md
[high] 34-67: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
[high] 80-112: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
[high] 122-144: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
🔇 Additional comments (19)
devenv.nix (1)
36-45: Good update: exclude new certinfo testdata from git-hooks.
Keeps local hooks from scanning/generated fixture churn.internal/certinfo/testdata/sample-text.txt (1)
1-3: Benign fixture content looks fine.cmd/requests.go (1)
89-96: Reader injection wiring looks correct at the CLI boundary.internal/requests/requests_test.go (1)
151-155: DI update looks good: passingcertinfo.InputReaderintoSetCaPoolFromFile.internal/requests/requests.go (1)
168-182: LGTM!The signature update correctly accepts an injected
Readerand properly propagates it tocertinfo.GetRootCertsFromFile. The error handling and CA pool assignment remain intact.internal/certinfo/certinfo.go (1)
84-127: LGTM!The updated method signatures correctly accept the
Readerinterface for dependency injection, and the implementations properly delegate to the corresponding helper functions with the injected reader.internal/certinfo/common_handlers_test.go (2)
1-11: Comprehensive test suite for certinfo handlers.Good coverage of error paths and success paths across multiple key types (RSA PKCS1/PKCS8, ECDSA, ED25519) and encryption states. The use of
t.Setenvfor environment variables and appropriatet.Parallel()usage in read-only subtests is well done.
398-497: Good edge case coverage forcertMatchPrivateKey.The tests cover multiple key types (RSA PKCS1/PKCS8, ECDSA, ED25519) and properly handle nil inputs for both certificate and key parameters.
cmd/certinfo.go (1)
74-88: LGTM!The method calls correctly pass the injected
fileReader(which implements thecertinfo.Readerinterface) to the updated signatures. The existing error-handling pattern (print and continue) is preserved, allowing partial success when some certificate sources fail.internal/certinfo/common_handlers.go (5)
68-84: LGTM!The dependency injection pattern for
Readeris well implemented. Input validation for empty path and proper error wrapping are in place.
86-97: LGTM!Consistent input validation pattern and improved error messaging.
160-181: LGTM!Clean implementation of passphrase retrieval with proper dependency injection for testability.
198-244: LGTM!Comprehensive private key parsing with support for multiple formats. The fallback chain (PKCS8 → PKCS1 → EC) is appropriate.
246-262: LGTM!Consistent input validation and proper delegation to
ParsePrivateKeywith the injected reader.internal/certinfo/main_test.go (5)
18-36: LGTM!Clean type definitions for test templates and mock readers that implement the
Readerinterface.
129-146: LGTM!Mock implementations correctly satisfy the
Readerinterface for testing error paths.
235-304: LGTM!Well-structured certificate generation utility with proper handling of both CA and leaf certificates. The reference to the source blog post is helpful.
306-343: LGTM!Helper functions are well-implemented. The
errors.Joinpattern increateTmpFileWithContentproperly aggregates file close errors.
91-127: Test exit code not propagated - CI may not detect failures.
m.Run()returns an exit code that should be passed toos.Exit(). Without this, test failures may not be properly reported to CI systems. Additionally, placingdeferafterm.Run()is unconventional.Apply this diff to properly handle the exit code:
func TestMain(m *testing.M) { fmt.Printf("Certinfo TestMain - check test data dir: %s\n", testdataDir) if errDataDir := os.Mkdir(testdataDir, os.ModePerm); errDataDir != nil { fmt.Println(errDataDir) } systemCertPool, _ = x509.SystemCertPool() caCertPool = x509.NewCertPool() generateRSACaData() caCertPool.AppendCertsFromPEM(RSACaCertPEM) generateRSACertificateData() - m.Run() - - // Cleanup - defer func() { - filesToDel := []string{ + code := m.Run() + + // Cleanup + filesToDel := []string{ + RSACaCertKeyFile, + RSACaCertFile, + RSASampleCertFile, + RSASampleCertKeyFile, + } + for _, fileToDel := range filesToDel { + err := os.Remove(fileToDel) + if err != nil { + fmt.Printf( + "unable to remove file %s: %s\n", + fileToDel, + err.Error(), + ) + } + } + + os.Exit(code) - RSACaCertKeyFile, - RSACaCertFile, - RSASampleCertFile, - RSASampleCertKeyFile, - } - for _, fileToDel := range filesToDel { - err := os.Remove(fileToDel) - if err != nil { - fmt.Printf( - "unable to remove file %s: %s", - fileToDel, - err.Error(), - ) - } - } - }() }⛔ Skipped due to learnings
Learnt from: xenOs76 Repo: xenOs76/https-wrench PR: 3 File: internal/requests/main_test.go:219-341 Timestamp: 2025-11-08T15:01:21.772Z Learning: In Go 1.15 and later, TestMain does not need to explicitly call os.Exit(). The test runner automatically handles the exit code returned by m.Run(). Calling os.Exit() explicitly in TestMain is redundant and flagged by golangci-lint's revive rule "redundant-test-main-exit".
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
internal/certinfo/common_handlers.go (1)
99-102: Inconsistent parameter name in error message (previously flagged).The error message references
caBundlePathbut the parameter is namedcertBundlePath.func GetCertsFromBundle(certBundlePath string, fileReader Reader) ([]*x509.Certificate, error) { if certBundlePath == emptyString { - return nil, errors.New("empty string provided as caBundlePath") + return nil, errors.New("empty string provided as certBundlePath") }
🧹 Nitpick comments (5)
internal/certinfo/common_handlers_test.go (3)
3-10: Multiple assertion libraries in use.The test file imports three different assertion libraries (
assert/v2,go-cmp, andtestify/require). While this works, consider standardizing on one or two libraries for consistency across the test suite.
345-378: Address or remove the commented-out test.This commented-out test for
ParsePrivateKeystdin password read error should either be implemented properly or removed. Leaving commented code in the codebase creates maintenance burden.Would you like me to help implement this test case using the mock readers, or should we open an issue to track this as future work?
380-390: Consider expanding test coverage forIsPrivateKeyEncrypted.The current test only covers the "No PEM encoded file" error case. Consider adding tests for:
- Encrypted PKCS8 keys (returns
true)- Encrypted PKCS1/EC keys with DEK-Info header (returns
true)- Plaintext keys (returns
false)- Unrecognized key types (returns error)
internal/certinfo/common_handlers.go (1)
256-261: Address the TODO: MakepwEnvKeyconfigurable.The TODO indicates that
pwEnvKeyshould be passed as a function argument rather than using the hardcodedprivateKeyPwEnvVar. This would improve flexibility for callers needing different environment variable names.Would you like me to help refactor this to accept
pwEnvKeyas a parameter, or should we open an issue to track this?internal/certinfo/main_test.go (1)
125-131: Confusing mock naming:MockInputReader.ReadFilealways fails.
MockInputReadersuggests a successful input reader, but itsReadFilemethod always returns an error. Meanwhile,MockErrReaderdoes the same. Consider renaming for clarity:
MockInputReader→MockPasswordReader(only provides password successfully)- Or document the intended behavior
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
internal/certinfo/certinfo.go(5 hunks)internal/certinfo/certinfo_test.go(1 hunks)internal/certinfo/common_handlers.go(5 hunks)internal/certinfo/common_handlers_test.go(1 hunks)internal/certinfo/main_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/certinfo/certinfo_test.go
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: xenOs76
Repo: xenOs76/https-wrench PR: 10
File: internal/requests/requests_test.go:918-997
Timestamp: 2025-11-29T14:05:35.000Z
Learning: In the https-wrench tool test suite (xenOs76/https-wrench repository), the user wants to be reminded in the future about potentially adding MinVersion enforcement to TLS configurations in test code, once legacy server compatibility becomes less critical.
🧬 Code graph analysis (4)
internal/certinfo/common_handlers_test.go (1)
internal/certinfo/common_handlers.go (5)
GetRootCertsFromFile(68-84)GetRootCertsFromString(86-97)GetCertsFromBundle(99-139)GetKeyFromFile(246-267)IsPrivateKeyEncrypted(143-158)
internal/certinfo/common_handlers.go (1)
internal/certinfo/certinfo.go (1)
Reader(40-43)
internal/certinfo/main_test.go (1)
internal/certinfo/certinfo.go (1)
Reader(40-43)
internal/certinfo/certinfo.go (1)
internal/certinfo/common_handlers.go (3)
GetRootCertsFromFile(68-84)GetCertsFromBundle(99-139)GetKeyFromFile(246-267)
🔇 Additional comments (9)
internal/certinfo/common_handlers_test.go (3)
12-61: LGTM – Thorough test coverage forGetRootCertsFromFile.Good coverage of error paths (empty string, unreadable file, wrong file type) and validation of successful certificate import using
cmp.Diff.
147-343: Excellent test coverage forGetKeyFromFile.Comprehensive tests covering RSA (PKCS1/PKCS8), ECDSA, and ED25519 keys in both plaintext and encrypted formats, including error scenarios for wrong passwords and broken keys.
433-532: Good coverage forcertMatchPrivateKey.Tests cover RSA (PKCS1/PKCS8), ECDSA, ED25519 matching, mismatches, and nil inputs.
internal/certinfo/certinfo.go (3)
54-61: Previous critical bug has been fixed.The
ReadFilemethod now correctly returns the actual error fromos.ReadFileinstead of silently swallowing it.
39-46: Reader abstraction looks good.The
Readerinterface enables dependency injection for file and password I/O, improving testability. The emptyInputReaderstruct with unnamed receivers follows Go idioms for interface satisfaction without state.
80-95: LGTM – Clean integration offileReaderintoSetCaPoolFromFile.The method properly delegates to
GetRootCertsFromFilewith the injected reader.internal/certinfo/common_handlers.go (2)
68-84: LGTM –GetRootCertsFromFileproperly uses injected reader.Clean implementation with proper input validation and error handling.
198-228: LGTM –ParsePrivateKeyproperly propagatespwReader.The function correctly passes the reader to
getPassphraseIfNeededfor password retrieval.internal/certinfo/main_test.go (1)
237-306:GenerateCertificateis well-structured.Good implementation with proper serial number generation, template handling for CA vs leaf certificates, and PEM encoding.
…d the key's password
…ose errors from deferred function
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
internal/certinfo/main_test.go (1)
143-182: Stop discarding cert/key generation errors in setup helpers.
Right now failures can silently cascade into confusing test results.-func generateRSACertificateData() { +func generateRSACertificateData() error { var err error @@ - RSASampleCertKey, _ = RSAGenerateKey(2048) + RSASampleCertKey, err = RSAGenerateKey(2048) + if err != nil { + return err + } @@ RSASampleCertKeyFile, err = createTmpFileWithContent( @@ if err != nil { - fmt.Println(err) + return err } @@ - RSASampleCertPEM, RSASampleCertParent, _ = GenerateCertificate( - rsaSampleCertTpl, - ) + RSASampleCertPEM, RSASampleCertParent, err = GenerateCertificate(rsaSampleCertTpl) + if err != nil { + return err + } @@ RSASampleCertFile, err = createTmpFileWithContent( @@ if err != nil { - fmt.Print(err) + return err } + return nil } -func generateRSACaData() { +func generateRSACaData() error { var err error @@ - RSACaCertKey, _ = RSAGenerateKey(2048) + RSACaCertKey, err = RSAGenerateKey(2048) + if err != nil { + return err + } @@ RSACaCertKeyFile, err = createTmpFileWithContent( @@ if err != nil { - fmt.Println(err) + return err } @@ - RSACaCertPEM, RSACaCertParent, _ = GenerateCertificate( - rsaCaCertTpl, - ) + RSACaCertPEM, RSACaCertParent, err = GenerateCertificate(rsaCaCertTpl) + if err != nil { + return err + } @@ RSACaCertFile, err = createTmpFileWithContent( @@ if err != nil { - fmt.Print(err) + return err } + return nil }Also applies to: 184-221
🧹 Nitpick comments (3)
internal/certinfo/common_handlers.go (2)
198-206: Don’t ignoreIsPrivateKeyEncryptederrors / avoid double decode.
You alreadypem.Decodeabove; you can derive encryption fromkeyBlockand keep errors meaningful.
68-83: Add path context when returning raw read errors.
GetKeyFromFilecurrently returns theReadFileerror without indicating which path failed.Also applies to: 99-107, 246-254
internal/certinfo/main_test.go (1)
301-322: Consider 0600 perms for private key fixtures (even in tests).
0644makes key material world-readable on *nix.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
internal/certinfo/common_handlers.go(5 hunks)internal/certinfo/common_handlers_test.go(1 hunks)internal/certinfo/main_test.go(1 hunks)internal/requests/requests_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/certinfo/common_handlers_test.go
- internal/requests/requests_test.go
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: xenOs76
Repo: xenOs76/https-wrench PR: 10
File: internal/requests/requests_test.go:918-997
Timestamp: 2025-11-29T14:05:35.000Z
Learning: In the https-wrench tool test suite (xenOs76/https-wrench repository), the user wants to be reminded in the future about potentially adding MinVersion enforcement to TLS configurations in test code, once legacy server compatibility becomes less critical.
📚 Learning: 2025-11-08T15:01:21.772Z
Learnt from: xenOs76
Repo: xenOs76/https-wrench PR: 3
File: internal/requests/main_test.go:219-341
Timestamp: 2025-11-08T15:01:21.772Z
Learning: In Go 1.15 and later, TestMain does not need to explicitly call os.Exit(). The test runner automatically handles the exit code returned by m.Run(). Calling os.Exit() explicitly in TestMain is redundant and flagged by golangci-lint's revive rule "redundant-test-main-exit".
Applied to files:
internal/certinfo/main_test.go
📚 Learning: 2025-11-29T14:05:35.000Z
Learnt from: xenOs76
Repo: xenOs76/https-wrench PR: 10
File: internal/requests/requests_test.go:918-997
Timestamp: 2025-11-29T14:05:35.000Z
Learning: In the https-wrench tool test suite (xenOs76/https-wrench repository), the user wants to be reminded in the future about potentially adding MinVersion enforcement to TLS configurations in test code, once legacy server compatibility becomes less critical.
Applied to files:
internal/certinfo/main_test.go
🧬 Code graph analysis (2)
internal/certinfo/main_test.go (1)
internal/certinfo/certinfo.go (1)
Reader(40-43)
internal/certinfo/common_handlers.go (1)
internal/certinfo/certinfo.go (1)
Reader(40-43)
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/certinfo/common_handlers.go (1)
168-256: Don’t ignoreIsPrivateKeyEncryptederrors; it can mask unsupported key types.
Line 216 discards the error, so an “unrecognized private key type” can later collapse into the generic “unsupported key format or invalid password” (Line 255).- isEncrypted, _ := IsPrivateKeyEncrypted(keyPEM) + isEncrypted, encErr := IsPrivateKeyEncrypted(keyPEM) + if encErr != nil { + return nil, encErr + }Also, the nil-reader error in
getPassphraseIfNeeded(Line 174) could be more specific (e.g., “nil Reader provided for passphrase input”) to aid callers.
♻️ Duplicate comments (1)
internal/certinfo/main_test.go (1)
114-221: Fail fast in fixture generation; currently multiple errors are ignored.
Examples:x509.SystemCertPool()error ignored (Line 114),RSAGenerateKeyerror ignored (Lines 147, 188),GenerateCertificateerror ignored (Lines 169-171, 205-207), and file creation errors are only printed (Lines 155-181, 196-220). This can produce confusing panics or silently invalid test inputs.Suggested shape: make
generateRSACaData()/generateRSACertificateData()returnerrorandpanic/os.Exit(1)inTestMainon failure, e.g.:-func generateRSACertificateData() { +func generateRSACertificateData() error { @@ - RSASampleCertKey, _ = RSAGenerateKey(2048) + RSASampleCertKey, err = RSAGenerateKey(2048) + if err != nil { return err } @@ - if err != nil { fmt.Println(err) } + if err != nil { return err } @@ - RSASampleCertPEM, RSASampleCertParent, _ = GenerateCertificate(rsaSampleCertTpl) + RSASampleCertPEM, RSASampleCertParent, err = GenerateCertificate(rsaSampleCertTpl) + if err != nil { return err } @@ - if err != nil { fmt.Print(err) } + if err != nil { return err } + return nil }
🧹 Nitpick comments (4)
internal/certinfo/common_handlers.go (2)
68-88: Reader-based CA loading looks solid; consider including path in the “append certs” error.
Right now “unable to create CertPool from file” (Line 84) loses which file failed. Minor but helpful for CLI UX/logs.- if ok := rootCAPool.AppendCertsFromPEM(certsFromFile); !ok { - return nil, errors.New("unable to create CertPool from file") - } + if ok := rootCAPool.AppendCertsFromPEM(certsFromFile); !ok { + return nil, fmt.Errorf("unable to create CertPool from file %q", caBundlePath) + }
258-286: Key loading wiring is fine; wrap ReadFile error with filename for debugging.
GetKeyFromFilereturns the rawReadFileerror (Line 273), which may not include the path.keyPEM, err := inputReader.ReadFile(keyFilePath) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to read key file %q: %w", keyFilePath, err) }internal/certinfo/main_test.go (2)
125-141: SimplifyMockErrReader.ReadPassword(the closure adds no value).
Line 138-140 can just directly return the error.func (MockErrReader) ReadPassword(fd int) ([]byte, error) { - return func(_ int) ([]byte, error) { - return []byte{}, errors.New("mockErrReader: unable to read password") - }(fd) + _ = fd + return nil, errors.New("mockErrReader: unable to read password") }
38-64: Preferfilepath.Joinfor testdata paths (Windows-friendly).
Currently uses string concatenation with “/” (e.g., Line 39-63).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/certinfo/common_handlers.go(4 hunks)internal/certinfo/main_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: xenOs76
Repo: xenOs76/https-wrench PR: 10
File: internal/requests/requests_test.go:918-997
Timestamp: 2025-11-29T14:05:35.000Z
Learning: In the https-wrench tool test suite (xenOs76/https-wrench repository), the user wants to be reminded in the future about potentially adding MinVersion enforcement to TLS configurations in test code, once legacy server compatibility becomes less critical.
📚 Learning: 2025-11-08T15:01:21.772Z
Learnt from: xenOs76
Repo: xenOs76/https-wrench PR: 3
File: internal/requests/main_test.go:219-341
Timestamp: 2025-11-08T15:01:21.772Z
Learning: In Go 1.15 and later, TestMain does not need to explicitly call os.Exit(). The test runner automatically handles the exit code returned by m.Run(). Calling os.Exit() explicitly in TestMain is redundant and flagged by golangci-lint's revive rule "redundant-test-main-exit".
Applied to files:
internal/certinfo/main_test.go
📚 Learning: 2025-11-29T14:05:35.000Z
Learnt from: xenOs76
Repo: xenOs76/https-wrench PR: 10
File: internal/requests/requests_test.go:918-997
Timestamp: 2025-11-29T14:05:35.000Z
Learning: In the https-wrench tool test suite (xenOs76/https-wrench repository), the user wants to be reminded in the future about potentially adding MinVersion enforcement to TLS configurations in test code, once legacy server compatibility becomes less critical.
Applied to files:
internal/certinfo/main_test.go
🧬 Code graph analysis (2)
internal/certinfo/main_test.go (1)
internal/certinfo/certinfo.go (1)
Reader(40-43)
internal/certinfo/common_handlers.go (1)
internal/certinfo/certinfo.go (1)
Reader(40-43)
🔇 Additional comments (1)
internal/certinfo/main_test.go (1)
87-123: The current code correctly handles test exit codes — no changes needed.Go 1.15+ (and your project uses 1.24.9) automatically propagates the exit code returned by
m.Run()inTestMain. Callingm.Run()and returning without explicitos.Exit()is the correct pattern and is what golangci-lint's revive ruleredundant-test-main-exitexpects. The cleanup defer block executes properly before the function returns, allowing the test runner to use the correct exit code. Addingos.Exit(m.Run())would unnecessarily override this behavior and trigger the linter.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/certinfo/main_test.go (1)
87-123: LGTM! TestMain setup is well-structured.The defer cleanup is correctly placed before
m.Run(),MkdirAllensures idempotent directory creation, and the panic-on-error provides fail-fast behavior. The absence ofos.Exit()aligns with Go 1.15+ best practices. Based on learnings, explicitos.Exit()is redundant in modern Go.Optional: Consider handling systemCertPool initialization error.
Line 114 ignores the error from
x509.SystemCertPool(). If loading the system cert pool fails, tests might exhibit unexpected behavior. Consider logging or handling this error to avoid confusion during test failures.- systemCertPool, _ = x509.SystemCertPool() + var err error + systemCertPool, err = x509.SystemCertPool() + if err != nil { + fmt.Printf("Warning: failed to load system cert pool: %v\n", err) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/certinfo/main_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: xenOs76
Repo: xenOs76/https-wrench PR: 10
File: internal/requests/requests_test.go:918-997
Timestamp: 2025-11-29T14:05:35.000Z
Learning: In the https-wrench tool test suite (xenOs76/https-wrench repository), the user wants to be reminded in the future about potentially adding MinVersion enforcement to TLS configurations in test code, once legacy server compatibility becomes less critical.
📚 Learning: 2025-11-08T15:01:21.772Z
Learnt from: xenOs76
Repo: xenOs76/https-wrench PR: 3
File: internal/requests/main_test.go:219-341
Timestamp: 2025-11-08T15:01:21.772Z
Learning: In Go 1.15 and later, TestMain does not need to explicitly call os.Exit(). The test runner automatically handles the exit code returned by m.Run(). Calling os.Exit() explicitly in TestMain is redundant and flagged by golangci-lint's revive rule "redundant-test-main-exit".
Applied to files:
internal/certinfo/main_test.go
📚 Learning: 2025-11-29T14:05:35.000Z
Learnt from: xenOs76
Repo: xenOs76/https-wrench PR: 10
File: internal/requests/requests_test.go:918-997
Timestamp: 2025-11-29T14:05:35.000Z
Learning: In the https-wrench tool test suite (xenOs76/https-wrench repository), the user wants to be reminded in the future about potentially adding MinVersion enforcement to TLS configurations in test code, once legacy server compatibility becomes less critical.
Applied to files:
internal/certinfo/main_test.go
🧬 Code graph analysis (1)
internal/certinfo/main_test.go (1)
internal/certinfo/certinfo.go (1)
Reader(40-43)
🔇 Additional comments (4)
internal/certinfo/main_test.go (4)
125-141: LGTM! Mock implementations are clear and appropriate.The mock readers provide distinct behaviors for testing success and failure paths:
MockInputReader: Password reading succeeds, file reading failsMockErrReader: Both operations fail
230-299: LGTM! Certificate generation logic is correct.The function properly handles both CA and non-CA certificate generation:
- For CA certificates, the template is rebuilt with appropriate constraints (lines 265-279), then
certParent = &templateat line 280 correctly creates a self-signed certificate- Serial number generation, validity periods, and key usage are properly configured
- PEM encoding and certificate parsing are straightforward
The CA parent template assignment order issue from past reviews has been resolved.
301-322: LGTM! Proper error handling and secure file permissions.The function correctly uses:
- Named return values enabling the deferred close error to propagate via
errors.Join- File permissions
0o600for private key files (least-privilege)- Appropriate error handling throughout
Past review concerns about
errors.Joinpropagation and file permissions have been addressed.
324-343: LGTM! RSA helper functions are well-implemented.Both functions are straightforward and correct:
RSAGenerateKeyproperly handles key generation errorsRSAPrivateKeyToPEMcorrectly marshals PKCS1 and encodes to PEM format
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
internal/certinfo/common_handlers_test.go (1)
383-393: Consider expanding test coverage forIsPrivateKeyEncrypted.Currently only tests the PEM decode failure path. Consider adding test cases for the other branches: encrypted PKCS8 keys, encrypted/unencrypted EC and RSA keys, and unrecognized key types.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/certinfo/common_handlers_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: xenOs76
Repo: xenOs76/https-wrench PR: 10
File: internal/requests/requests_test.go:918-997
Timestamp: 2025-11-29T14:05:35.000Z
Learning: In the https-wrench tool test suite (xenOs76/https-wrench repository), the user wants to be reminded in the future about potentially adding MinVersion enforcement to TLS configurations in test code, once legacy server compatibility becomes less critical.
🧬 Code graph analysis (1)
internal/certinfo/common_handlers_test.go (1)
internal/certinfo/common_handlers.go (7)
GetRootCertsFromFile(68-88)GetRootCertsFromString(90-101)GetCertsFromBundle(103-147)GetKeyFromFile(258-286)ParsePrivateKey(210-256)IsPrivateKeyEncrypted(151-166)printKeyInfo(37-55)
🔇 Additional comments (9)
internal/certinfo/common_handlers_test.go (9)
3-11: LGTM on imports.The combination of
require(for fatal preconditions),assert(for non-fatal checks), andcmp.Diff(for detailed structural comparisons) is reasonable and common in Go test suites.
13-71: Good test coverage forGetRootCertsFromFile.Tests cover empty input, file read errors, invalid file content, successful import validation, and nil reader handling. The use of
cmp.Difffor pool comparison provides clear failure output.
73-102: LGTM!Tests cover the expected error cases (empty string, invalid cert string) and validates successful import against the expected cert pool.
104-164: LGTM!Comprehensive coverage including error paths for empty input, read failures, invalid content, and nil reader, plus validation of successful certificate parsing.
283-307: Good test structure for env-var dependent tests.Running subtests sequentially (not parallel) is the correct approach when using
t.Setenv(). The test covers a comprehensive matrix of key formats and encryption states.
340-356: LGTM!Clean validation of successful key import with structural comparison.
358-381: LGTM!Well-documented test that verifies error propagation from passphrase reading through
ParsePrivateKey.
449-482: LGTM!Good coverage of mismatch scenarios including nil certificate and nil key edge cases.
512-535: LGTM!Good integration-style test that validates the full flow: loading certs and keys from files, then verifying they match. Covers the main key types (RSA PKCS1/PKCS8, ECDSA, ED25519).
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
internal/certinfo/main_test.go (3)
185-222: Return errors from CA data generation.Similar to
generateRSACertificateData, this function ignores errors from key and certificate generation (lines 189, 206) and only prints file creation errors (lines 198, 220).Apply the same pattern as suggested for
generateRSACertificateData: change the function signature to returnerrorand propagate all failures to the caller.
88-124: Test setup should fail fast on errors.Lines 118 and 121 call
generateRSACaData()andgenerateRSACertificateData()without checking for errors. These functions print errors (lines 157, 181, 198, 220) but allow TestMain to continue, which can cause tests to run with incomplete or broken test data.Consider modifying these functions to return errors and panic in TestMain if setup fails:
- generateRSACaData() + if err := generateRSACaData(); err != nil { + panic(fmt.Sprintf("failed to generate RSA CA data: %v", err)) + } caCertPool.AppendCertsFromPEM(RSACaCertPEM) - generateRSACertificateData() + if err := generateRSACertificateData(); err != nil { + panic(fmt.Sprintf("failed to generate RSA certificate data: %v", err)) + }Then update
generateRSACaDataandgenerateRSACertificateDatato returnerrorand propagate failures from key generation and certificate creation.
144-183: Return errors from test data generation.Errors from
RSAGenerateKey(line 148) andGenerateCertificate(line 170) are ignored, and errors fromcreateTmpFileWithContent(lines 157, 181) are only printed. This means test setup failures are silently ignored, causing tests to run with invalid data.Modify the function to return an error and propagate all failures:
-func generateRSACertificateData() { +func generateRSACertificateData() error { var err error // RSA Certificate - RSASampleCertKey, _ = RSAGenerateKey(2048) + RSASampleCertKey, err = RSAGenerateKey(2048) + if err != nil { + return fmt.Errorf("failed to generate RSA sample key: %w", err) + } RSASampleCertKeyPEM = RSAPrivateKeyToPEM(RSASampleCertKey) RSASampleCertKeyFile, err = createTmpFileWithContent( testdataDir, "RSASampleCertKey", RSASampleCertKeyPEM, ) if err != nil { - fmt.Println(err) + return fmt.Errorf("failed to create RSA sample key file: %w", err) } rsaSampleCertTpl := certificateTemplate{ cn: "RSA Testing Sample Certificate", isCA: false, key: RSASampleCertKey, caKey: RSACaCertKey, parent: RSACaCertParent, dnsNames: []string{"example.com", "example.net", "example.de"}, ipAddresses: []net.IP{net.ParseIP("::1"), net.ParseIP("127.0.0.1")}, } - RSASampleCertPEM, RSASampleCertParent, _ = GenerateCertificate( + RSASampleCertPEM, RSASampleCertParent, err = GenerateCertificate( rsaSampleCertTpl, ) + if err != nil { + return fmt.Errorf("failed to generate RSA sample certificate: %w", err) + } RSASampleCertPEMString = string(RSASampleCertPEM) RSASampleCertFile, err = createTmpFileWithContent( testdataDir, "RSASampleCert", []byte(RSASampleCertPEMString), ) if err != nil { - fmt.Print(err) + return fmt.Errorf("failed to create RSA sample cert file: %w", err) } + return nil }Similar changes should be made to
generateRSACaData(lines 185-222).
🧹 Nitpick comments (2)
internal/certinfo/main_test.go (2)
126-132: Consider clarifying mock naming.
MockInputReader.ReadFilealways returns an error, which might be confusing given the "InputReader" name suggests successful reading. If this mock is specifically for testing error paths, consider renaming it toMockErrInputReaderor similar for clarity.
138-142: SimplifyReadPasswordimplementation.The immediately invoked function expression (IIFE) is unnecessary here.
func (MockErrReader) ReadPassword(fd int) ([]byte, error) { - return func(_ int) ([]byte, error) { - return []byte{}, errors.New("mockErrReader: unable to read password") - }(fd) + return []byte{}, errors.New("mockErrReader: unable to read password") }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
internal/certinfo/testdata/rsa-pkcs8-broken-crt.pemis excluded by!**/*.pem
📒 Files selected for processing (2)
internal/certinfo/common_handlers_test.go(1 hunks)internal/certinfo/main_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: xenOs76
Repo: xenOs76/https-wrench PR: 10
File: internal/requests/requests_test.go:918-997
Timestamp: 2025-11-29T14:05:35.000Z
Learning: In the https-wrench tool test suite (xenOs76/https-wrench repository), the user wants to be reminded in the future about potentially adding MinVersion enforcement to TLS configurations in test code, once legacy server compatibility becomes less critical.
📚 Learning: 2025-11-29T14:05:35.000Z
Learnt from: xenOs76
Repo: xenOs76/https-wrench PR: 10
File: internal/requests/requests_test.go:918-997
Timestamp: 2025-11-29T14:05:35.000Z
Learning: In the https-wrench tool test suite (xenOs76/https-wrench repository), the user wants to be reminded in the future about potentially adding MinVersion enforcement to TLS configurations in test code, once legacy server compatibility becomes less critical.
Applied to files:
internal/certinfo/common_handlers_test.gointernal/certinfo/main_test.go
📚 Learning: 2025-11-08T15:01:21.772Z
Learnt from: xenOs76
Repo: xenOs76/https-wrench PR: 3
File: internal/requests/main_test.go:219-341
Timestamp: 2025-11-08T15:01:21.772Z
Learning: In Go 1.15 and later, TestMain does not need to explicitly call os.Exit(). The test runner automatically handles the exit code returned by m.Run(). Calling os.Exit() explicitly in TestMain is redundant and flagged by golangci-lint's revive rule "redundant-test-main-exit".
Applied to files:
internal/certinfo/main_test.go
🧬 Code graph analysis (2)
internal/certinfo/common_handlers_test.go (2)
internal/certinfo/common_handlers.go (6)
GetRootCertsFromFile(68-88)GetRootCertsFromString(90-101)GetCertsFromBundle(103-147)GetKeyFromFile(258-286)ParsePrivateKey(210-256)IsPrivateKeyEncrypted(151-166)internal/certinfo/certinfo.go (1)
Reader(40-43)
internal/certinfo/main_test.go (1)
internal/certinfo/certinfo.go (1)
Reader(40-43)
🔇 Additional comments (12)
internal/certinfo/common_handlers_test.go (9)
1-11: LGTM!The imports are appropriate for the testing requirements. Using
testify/requirefor fatal assertions andalecthomas/assertfor non-fatal assertions is a reasonable pattern, andgo-cmpis well-suited for deep comparisons of certificate structures.
13-71: LGTM!Comprehensive test coverage for
GetRootCertsFromFileincluding:
- Empty string validation
- File read error handling
- Invalid certificate content detection
- Nil Reader guard
- Successful import validation using
cmp.DiffThe test structure is clean and covers all error paths from the implementation.
73-102: LGTM!Good test coverage for
GetRootCertsFromStringwith proper validation of both error conditions and successful import.
104-175: LGTM!Well-structured table-driven tests covering all error paths including the broken certificate case. The
CertImportValidationsubtest properly validates the returned certificate slice.
177-349: LGTM!Excellent comprehensive test coverage for
GetKeyFromFileincluding:
- Multiple key formats (RSA PKCS1/PKCS8, ECDSA, ED25519)
- Encrypted and plaintext key variants
- Wrong password and broken key scenarios
- Proper use of
t.Setenvfor test isolationThe main test loop correctly avoids
t.Parallel()due to environment variable modifications, while the independent subtests properly use parallel execution.
351-367: LGTM!Good complementary test that validates the returned key matches the expected fixture using deep comparison.
369-404: LGTM!Good targeted tests for error propagation paths. The
ParsePrivateKeytest effectively validates error handling when passphrase reading fails, with clear comments explaining the test strategy.
406-458: LGTM!Good test coverage for
getPassphraseIfNeededincluding the platform-agnostic error assertion usingrequire.ErrorContains(addressing the previous review feedback about OS-specific error messages).
516-567: LGTM!Comprehensive test coverage for
certMatchPrivateKeywith multiple key formats (RSA PKCS1/PKCS8, ECDSA, ED25519). The tests properly load certificates and keys before validating the matching logic.internal/certinfo/main_test.go (3)
231-300: LGTM!The certificate generation logic correctly handles both CA and non-CA certificates. The CA parent template is properly assigned after the template is constructed (line 281), and error handling is comprehensive throughout.
302-323: LGTM!The function correctly uses named return values to enable error joining in the deferred close, and file permissions (0o600) are appropriate for private key files.
325-344: LGTM!Both helper functions are correctly implemented with appropriate error handling in
RSAGenerateKeyand proper PEM encoding inRSAPrivateKeyToPEM.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/requests/requests.go (1)
231-269: Use the provided writer parameter for consistent output.Lines 254 and 262 hardcode
os.Stdoutfor certificate info output, while the rest ofPrintResponseDebugcorrectly uses thewparameter. This inconsistency reduces testability and flexibility.Apply this diff to use the provided writer consistently:
for i, cert := range resp.TLS.PeerCertificates { fmt.Fprintf(w, "Certificate %d:\n", i) - certinfo.PrintCertInfo(cert, 1, os.Stdout) + certinfo.PrintCertInfo(cert, 1, w) } for i, chain := range resp.TLS.VerifiedChains { fmt.Fprintf(w, "Verified Chain %d:\n", i) for j, cert := range chain { fmt.Fprintf(w, " Cert %d:\n", j) - certinfo.PrintCertInfo(cert, 2, os.Stdout) + certinfo.PrintCertInfo(cert, 2, w) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/certinfo/common_handlers.go(5 hunks)internal/certinfo/common_handlers_test.go(1 hunks)internal/requests/requests.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/certinfo/common_handlers.go
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: xenOs76
Repo: xenOs76/https-wrench PR: 10
File: internal/requests/requests_test.go:918-997
Timestamp: 2025-11-29T14:05:35.000Z
Learning: In the https-wrench tool test suite (xenOs76/https-wrench repository), the user wants to be reminded in the future about potentially adding MinVersion enforcement to TLS configurations in test code, once legacy server compatibility becomes less critical.
📚 Learning: 2025-11-29T14:05:35.000Z
Learnt from: xenOs76
Repo: xenOs76/https-wrench PR: 10
File: internal/requests/requests_test.go:918-997
Timestamp: 2025-11-29T14:05:35.000Z
Learning: In the https-wrench tool test suite (xenOs76/https-wrench repository), the user wants to be reminded in the future about potentially adding MinVersion enforcement to TLS configurations in test code, once legacy server compatibility becomes less critical.
Applied to files:
internal/certinfo/common_handlers_test.go
🧬 Code graph analysis (2)
internal/requests/requests.go (2)
internal/certinfo/certinfo.go (1)
Reader(40-43)internal/certinfo/common_handlers.go (2)
GetRootCertsFromFile(69-89)PrintCertInfo(19-35)
internal/certinfo/common_handlers_test.go (1)
internal/certinfo/common_handlers.go (7)
GetRootCertsFromFile(69-89)GetRootCertsFromString(91-102)GetCertsFromBundle(104-148)GetKeyFromFile(259-287)ParsePrivateKey(211-257)IsPrivateKeyEncrypted(152-167)PrintCertInfo(19-35)
🔇 Additional comments (11)
internal/requests/requests.go (1)
168-182: LGTM! Reader abstraction properly integrated.The signature change correctly adds the
fileReaderparameter to support the Reader abstraction pattern, enabling better testability and dependency injection for file operations.internal/certinfo/common_handlers_test.go (10)
14-72: LGTM!Comprehensive test coverage for
GetRootCertsFromFilewith proper error handling validation, successful import verification usingcmp.Diff, and nil reader checks.
74-103: LGTM!Well-structured test covering error cases and successful cert pool import validation.
105-176: LGTM!Excellent table-driven test design covering multiple error scenarios and successful cert bundle parsing with validation against expected fixtures.
178-350: LGTM!Comprehensive table-driven test suite covering multiple key formats (RSA PKCS1/PKCS8, ECDSA, ED25519), encryption scenarios, error paths, and proper environment variable handling for password testing.
352-368: LGTM!Focused test validating successful RSA key import with proper comparison using
cmp.Diff.
370-393: LGTM!Good edge case test validating error propagation when password reading fails for encrypted keys.
395-405: LGTM!Focused test validating error handling for non-PEM encoded input.
407-459: LGTM!Comprehensive test coverage for passphrase handling with proper platform-agnostic error assertions (using
require.ErrorContains) and validation of both success and error paths.
461-515: LGTM!Well-structured tests covering certificate-key mismatch scenarios, nil inputs, and error conditions with appropriate assertions.
517-568: LGTM!Excellent end-to-end validation testing certificate-private-key matching across multiple key types (RSA PKCS1/PKCS8, ECDSA, ED25519).
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 participant
Summary by CodeRabbit
Tests
Chores
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.