Hub Integration for Centralized Config for Analysis #588
Conversation
WalkthroughAdds profile-driven analysis and a new Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as kantra CLI
participant Hub as Hub Server
participant Auth as Auth Store
participant FS as File System
User->>CLI: kantra config login
CLI->>User: prompt host/username/password
User-->>CLI: credentials
CLI->>Hub: POST /auth/login (credentials)
Hub-->>CLI: 200 {access, refresh}
CLI->>Auth: store tokens (~/.kantra/auth.json)
Auth->>FS: write auth.json
CLI-->>User: login success
sequenceDiagram
actor User
participant CLI as kantra CLI
participant Auth as Auth Store
participant Hub as Hub Server
participant FS as File System
User->>CLI: kantra config sync --url <repo>
CLI->>Auth: load tokens
Auth-->>CLI: tokens
alt token expired
CLI->>Hub: POST /auth/refresh
Hub-->>CLI: new token
CLI->>Auth: update tokens
end
CLI->>Hub: GET /hub/applications?repo=<url>
Hub-->>CLI: application list
CLI->>Hub: GET /hub/applications/{id}/profiles
Hub-->>CLI: profiles metadata
CLI->>Hub: GET /hub/profiles/{id}/download
Hub-->>CLI: tar.gz
CLI->>FS: extract to ./<application>/profiles (safe paths)
FS-->>CLI: extraction complete
CLI-->>User: sync finished
sequenceDiagram
actor User
participant CLI as kantra analyze
participant ProfileMgr as pkg/profile
participant Analyzer as Analyze runtime
participant FS as File System
User->>CLI: kantra analyze --profile-dir ./profiles
CLI->>ProfileMgr: FindSingleProfile / UnmarshalProfile
ProfileMgr->>FS: read profile.yaml and rules/*
FS-->>ProfileMgr: profile data and rules list
ProfileMgr-->>CLI: ProfileSettings
CLI->>Analyzer: applyProfileSettings (input, mode, rules, selectors)
Analyzer-->>User: run analysis with profile-applied settings
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
eemcmullan
changed the title
eemcmullan
force-pushed
the
central-config
branch
4 times, most recently
from
ae1c865 to
1acd681
Compare
eemcmullan
changed the title
eemcmullan
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/analyze.go (1)
100-116: Reconsider required flag behavior when--profileis set (and keep output explicit).In
PreRunE:if !list-sources && !list-targets && !list-providers && !cmd.Flags().Lookup("list-languages").Changed && !cmd.Flags().Lookup("profile").Changed { cmd.MarkFlagRequired("input") cmd.MarkFlagRequired("output") ... } if cmd.Flags().Lookup("list-languages").Changed { cmd.MarkFlagRequired("input") } ... err := analyzeCmd.Validate(cmd.Context(), cmd)With this change:
- Supplying
--profilemeans neither--inputnor--outputare required by Cobra.Validatehappily accepts an emptyoutput(it ends up asfilepath.Abs(""), i.e. the current working directory), sokantra analyze --profile ...can write analysis output into whatever directory the user happens to be in, without an explicit--output.- The docs still show explicit
--outputfor profile usage, and default-profile usage describeskantra analyze --output <output-dir>without changing the required-flag logic for that case.For a safer and more predictable UX, consider:
- Still requiring
--outputwhenever you are doing a real analysis (i.e., unless you’re only listing something), regardless of whether--profileis set.- Allowing
--inputto be omitted only when it can be unambiguously derived from the profile path (either explicit--profileor auto-discovered default), and ideally doing that before validation so that the derived input goes through the same stat/type checks as a CLI‑provided one.At a minimum, it would be good to decide whether “implicit output to CWD when
--profileis used without--output” is intended; if not, makingoutputrequired even when--profileis set would be a small targeted change.Also applies to: 352-511
🧹 Nitpick comments (6)
cmd/config.go (1)
286-311: Handle “no matching application” explicitly
getApplicationFromHubreturns a zero‑valueapi.Applicationwhen no app matchess.url, which then results in requests against/applications/0/...and a less clear downstream error.Consider:
- Returning an explicit error when
appsis empty or no element matchess.url.- Optionally handling “multiple matches” (e.g., first match vs. error).
This will make CLI failures easier to understand for users.
cmd/login.go (2)
135-146: Refine TLS settings and default scheme for Hub connectionsA couple of possible improvements:
- Default scheme
When the user omits a scheme, you currently prepend
http://:if !strings.HasPrefix(l.host, "http://") && !strings.HasPrefix(l.host, "https://") { l.host = "http://" + l.host }Given you already support TLS and many hubs will be HTTPS‑only, consider defaulting to
https://instead (or at least documenting the expectation).
- Insecure TLS min version
In both
performLoginandRefreshToken, the “insecure” mode sets:TLSClientConfig: &tls.Config{InsecureSkipVerify: true},If you want to satisfy stricter security scanners, you could also set
MinVersion: tls.VersionTLS13while retaining theInsecureSkipVerifyescape hatch for self‑signed/dev hubs, keeping in mind any compatibility needs with older deployments.Also applies to: 188-197
285-307: Consider defaulting refresh host from stored tokens
Refreshrequires the host to be passed as an argument even thoughLoadStoredTokensreturns aLoginResponsethat can contain aHostfield. For UX, you could fall back tostoredTokens.Hostwhenl.hostis empty, allowingkantra login --refreshto work without repeating the host.This is purely a convenience improvement; current behavior is correct but a bit strict.
cmd/profile_test.go (2)
168-320: MakeexpectedInputcomputation OS‑agnostic and aligned with implementation.In
TestAnalyzeCommand_setSettingsFromProfile,expectedInputis derived via:expectedInput := strings.Split(cmd.profile, ".konveyor")[0] expectedInput = strings.TrimSuffix(expectedInput, "/")Two issues:
- This assumes POSIX separators (
/). On Windows paths likeC:\app\.konveyor\..., the trailing\is never stripped, soexpectedInputwill not match the value computed insetSettingsFromProfile, which uses index math rather thanSplit.- The tests implicitly assume
cmd.profilecontains.konveyorplus aprofile.yamlsuffix, while the runtime path passed via--profileis a directory and is later joined with"profile.yaml"inanalyze.go. That mismatch makes it harder to catch regressions in the real CLI path handling.Consider:
- Deriving the expected input using
filepath.Dir/filepath.Cleanfrom the profile file path (mirroring whatever logic you settle on insetSettingsFromProfile), and- Adding a test that mimics the actual
analyzeflow:profileset to the directory passed via--profile, thensetSettingsFromProfile(filepath.Join(profileDir, "profile.yaml"), cobraCmd).
376-537: Tighten tests around rules/profile discovery and avoid OS‑specific error fragments.The
getRulesInProfileandfindSingleProfiletests are generally solid, but a few small improvements would help:
- For
"profile with multiple rule directories", the test uses a specialif tt.name == ...branch and ignoreswantRules. That makeswantRulesmisleading; either remove it for that case or assert equality againstwantRulesfor consistency.- Several tests assert error substrings like
"no such file or directory". That string is OS‑dependent and may not be stable across platforms. As long as you're only targeting Unix‑like CI this is fine, but to keep tests portable it's safer to assert on your own wrapped message prefix (e.g."failed to stat profiles directory") rather than the underlyingostext.These are test‑only concerns and can be tackled opportunistically.
Also applies to: 539-730
cmd/config_test.go (1)
53-125: Avoid asserting on raw OS error text in tests.Several table entries use
errMsgvalues like"no such file or directory"or"failed to delete tar file", and then assert:if tt.errMsg != "" && !strings.Contains(err.Error(), tt.errMsg) { ... }The wrapped prefixes like
"failed to stat application path"are stable, but the"no such file or directory"portion comes from the OS and can vary by platform (e.g. Windows). That makes the tests brittle if you ever run them off Linux.Consider:
- Asserting only on your own prefix (
"failed to stat application path","failed to open tar file", etc.), and- Dropping OS‑specific substrings from the expectations.
Behavior stays fully tested while making the suite more portable.
Also applies to: 157-235, 618-686, 874-903, 1013-1069, 1127-1138
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (10)
cmd/analyze-bin.go(1 hunks)cmd/analyze.go(5 hunks)cmd/config.go(1 hunks)cmd/config_test.go(1 hunks)cmd/login.go(1 hunks)cmd/profile.go(1 hunks)cmd/profile_test.go(1 hunks)cmd/root.go(1 hunks)docs/profiles.md(1 hunks)go.mod(5 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-08T15:06:52.056Z
Learnt from: abrugaro
Repo: konveyor/kantra PR: 524
File: cmd/dump-rules.go:0-0
Timestamp: 2025-08-08T15:06:52.056Z
Learning: In konveyor/kantra cmd/dump-rules.go (Go/Cobra), the dump-rules command should not use package-level variables for flags (output, overwrite). Flags must be defined as local variables inside NewDumpRulesCommand to avoid shared mutable state across commands/tests.
Applied to files:
cmd/profile_test.go
🧬 Code graph analysis (2)
cmd/root.go (1)
cmd/config.go (1)
NewConfigCmd(39-78)
cmd/analyze.go (1)
cmd/profile.go (1)
Profiles(15-15)
🪛 ast-grep (0.40.0)
cmd/config.go
[warning] 222-222: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{InsecureSkipVerify: true}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
cmd/login.go
[warning] 140-140: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{InsecureSkipVerify: true}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
[warning] 193-193: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{InsecureSkipVerify: true}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
🪛 markdownlint-cli2 (0.18.1)
docs/profiles.md
180-180: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
233-233: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
240-240: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 OSV Scanner (2.2.4)
go.mod
[HIGH] 77-77: github.com/golang-jwt/jwt/v4 4.5.0: Improper error handling in ParseWithClaims and bad documentation may cause dangerous situations in github.com/golang-jwt/jwt
(GO-2024-3250)
[HIGH] 77-77: github.com/golang-jwt/jwt/v4 4.5.0: Excessive memory allocation during header parsing in github.com/golang-jwt/jwt
(GO-2025-3553)
[HIGH] 77-77: github.com/golang-jwt/jwt/v4 4.5.0: Bad documentation of error handling in ParseWithClaims can lead to potentially dangerous situations
[HIGH] 77-77: github.com/golang-jwt/jwt/v4 4.5.0: jwt-go allows excessive memory allocation during header parsing
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build & test from commit
🔇 Additional comments (6)
cmd/analyze-bin.go (1)
299-303: Progress cancellation sequencing looks consistentExplicitly cancelling and then waiting on
progressDoneonly when progress mode is enabled is fine, givenprogressCancelshould be non‑nil in that mode and is already safely deferred earlier. No changes needed here.cmd/config.go (1)
392-471: Tar extraction looks safe and path‑traversal resistantNice job on
extractTarFile: detecting gzip vs plain tar, creating parent directories, and explicitly rejecting entries whose resolved path escapesdestDirvia:if !strings.HasPrefix(targetPath, filepath.Clean(destDir)+string(os.PathSeparator)) { return fmt.Errorf("invalid file path in tar: %s", header.Name) }This is a solid defense against path traversal.
cmd/login.go (2)
81-133: Interactive login flow is straightforward and robustPrompting for missing host/username/password, validating URL with
url.ParseRequestURI, and logging + wrapping failures makes this flow solid for CLI use. Storing tokens afterward viastoreTokenswith 0600 permissions is appropriate.
310-325: Automatic token refresh integration looks good
hubClient.refreshStoredTokenreuses the existing login refresh logic and persists updated tokens, which keeps the hub client and CLI login flows consistent. Good reuse ofloginCommand.RefreshTokenandstoreTokens.cmd/root.go (1)
66-74: Config command wiring into root CLI looks correctRegistering
NewConfigCmd(logger)alongside the other subcommands is consistent with the existing pattern and cleanly exposes the new config/sync/login functionality from the root command.cmd/profile.go (1)
85-115: Profile rules and discovery helpers look good; behavior matches docs.
getRulesInProfileandfindSingleProfile:
- Correctly treat missing
rules/or missingprofile.yamlas “no rules/profile” rather than errors.- Error only on unexpected filesystem problems (e.g. non-directory
rulesor non-directory profiles root).- Match the documented behavior that a default profile is only used when exactly one
profile.yamlis present under.konveyor/profiles.No changes needed here; the behavior is solid and error handling is appropriate.
Also applies to: 117-156
Signed-off-by: Emily McMullan <[email protected]>
Signed-off-by: Emily McMullan <[email protected]>
Signed-off-by: Emily McMullan <[email protected]>
eemcmullan
force-pushed
the
central-config
branch
from
1acd681 to
0333565
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (9)
docs/profiles.md (2)
134-136: Clarify that--profileexpects a profile directory, not the profiles root.The example
kantra analyze --profile myapp/.konveyor/profilespoints to the.konveyor/profilesroot, but the implementation expects a directory containingprofile.yaml(e.g.,myapp/.konveyor/profiles/profile-1). This will fail at runtime.Update the example to:
-kantra analyze --profile myapp/.konveyor/profiles +kantra analyze --profile myapp/.konveyor/profiles/profile-1
217-219: Example command missing required--inputflag.The command
kantra analyze --output <output-dir>omits--input, which is required unless the profile mechanism auto-derives it. This may confuse users if the current implementation still requires explicit input.Either add
--inputto the example or clarify that input is derived from the profile location.go.mod (1)
77-77: Address HIGH-severity vulnerability ingithub.com/golang-jwt/jwt/v4 v4.5.0.CVE-2025-30204 (CVSS 7.5) affects versions < 4.5.2 due to excessive memory allocation during header parsing. This is a transitive dependency likely pulled by
github.com/konveyor/tackle2-hub.Upgrade the direct dependencies that pull in jwt/v4 to versions that include
github.com/golang-jwt/jwt/v4 >= 4.5.2, or explicitly override:go get github.com/golang-jwt/jwt/[email protected] go mod tidyThen verify with:
go list -m all | grep github.com/golang-jwt/jwt/v4cmd/config.go (2)
123-126: Fix logr usage: avoid printf-style format string.
logr.Logger.Infodoes not supportfmtformatting. The%swill be logged literally.-c.log.Info("no profile directories found in: %s", profilesDir) +c.log.Info("no profile directories found", "path", profilesDir)
224-228: Consider setting TLSMinVersionfor insecure mode.When
insecureis true, onlyInsecureSkipVerifyis configured. Static analysis recommends settingMinVersion: tls.VersionTLS13to align with modern security defaults.tr := &http.Transport{ - TLSClientConfig: &tls.Config{InsecureSkipVerify: true}, + TLSClientConfig: &tls.Config{ + InsecureSkipVerify: true, + MinVersion: tls.VersionTLS13, + }, }Note: This may break compatibility with older Hub instances using TLS 1.2. Verify target Hub TLS requirements before applying.
cmd/config_test.go (2)
503-518: Test uses non-standardAuthenticationheader instead ofAuthorization.This test validates the wrong header name. Tackle2 Hub expects
Authorization: Bearer <token>per RFC 6750, notAuthentication.Update the test to use the correct header:
serverResponse: func(w http.ResponseWriter, r *http.Request) { - auth := r.Header.Get("Authentication") + auth := r.Header.Get("Authorization") if auth != "Bearer test-token" {
1077-1078: Test expects wrong error message.The test expects "failed to open tar file" but
extractTarFilereturnsos.Open's error directly (e.g., "no such file or directory"), not a wrapped message.-errMsg: "failed to open tar file", +errMsg: "no such file or directory",cmd/profile.go (2)
17-32: The issues from the previous review remain unaddressed.This method still checks
a.profile == ""(line 18) instead of validating thepathparameter. Whenpathis provided buta.profileis empty (e.g., auto-discovered profiles), the method returnsnil, nil, which will cause nil-pointer dereferences in calling code.
34-82: The issues from the previous review remain unaddressed.This method still uses
a.profile(lines 39-44) instead of thepathparameter for directory computation, and the substring matha.profile[:konveyorIndex-1]will panic if.konveyorappears at the start of the path (konveyorIndex == 0).
🧹 Nitpick comments (2)
docs/profiles.md (1)
180-184: Add language identifiers to fenced code blocks.Static analysis flagged these blocks for missing language specifiers. Using
textsatisfies markdownlint MD040.-``` +```text URL: https://hub.myapp.com username: <username> password: 12345Apply similar changes to the error message blocks at lines 233 and 240. Also applies to: 233-235, 240-242 </blockquote></details> <details> <summary>cmd/config.go (1)</summary><blockquote> `500-504`: **Missing error handling after `io.Copy`.** If `io.Copy` fails, the file is closed but the error from `io.Copy` may be lost if `outFile.Close()` also returns an error. The `outFile.Close()` error is currently ignored. ```diff _, err = io.Copy(outFile, tarReader) -outFile.Close() -if err != nil { - return err -} +closeErr := outFile.Close() +if err != nil { + return err +} +if closeErr != nil { + return closeErr +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
cmd/analyze.go(5 hunks)cmd/config.go(1 hunks)cmd/config_test.go(1 hunks)cmd/login.go(1 hunks)cmd/profile.go(1 hunks)cmd/profile_test.go(1 hunks)cmd/root.go(1 hunks)docs/profiles.md(1 hunks)go.mod(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/root.go
- cmd/profile_test.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-08T15:06:52.056Z
Learnt from: abrugaro
Repo: konveyor/kantra PR: 524
File: cmd/dump-rules.go:0-0
Timestamp: 2025-08-08T15:06:52.056Z
Learning: In konveyor/kantra cmd/dump-rules.go (Go/Cobra), the dump-rules command should not use package-level variables for flags (output, overwrite). Flags must be defined as local variables inside NewDumpRulesCommand to avoid shared mutable state across commands/tests.
Applied to files:
cmd/config.gocmd/profile.go
🧬 Code graph analysis (1)
cmd/config_test.go (2)
cmd/config.go (2)
NewConfigCmd(46-85)NewSyncCmd(135-184)cmd/profile.go (1)
Profiles(15-15)
🪛 ast-grep (0.40.0)
cmd/config.go
[warning] 225-225: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{InsecureSkipVerify: true}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
cmd/login.go
[warning] 135-135: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{InsecureSkipVerify: true}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
[warning] 188-188: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{InsecureSkipVerify: true}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
🪛 markdownlint-cli2 (0.18.1)
docs/profiles.md
180-180: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
233-233: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
240-240: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 OSV Scanner (2.3.0)
go.mod
[HIGH] 77-77: github.com/golang-jwt/jwt/v4 4.5.0: Improper error handling in ParseWithClaims and bad documentation may cause dangerous situations in github.com/golang-jwt/jwt
(GO-2024-3250)
[HIGH] 77-77: github.com/golang-jwt/jwt/v4 4.5.0: Excessive memory allocation during header parsing in github.com/golang-jwt/jwt
(GO-2025-3553)
[HIGH] 77-77: github.com/golang-jwt/jwt/v4 4.5.0: Bad documentation of error handling in ParseWithClaims can lead to potentially dangerous situations
[HIGH] 77-77: github.com/golang-jwt/jwt/v4 4.5.0: jwt-go allows excessive memory allocation during header parsing
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build & test from commit
🔇 Additional comments (7)
cmd/analyze.go (2)
150-181: LGTM with one suggestion: improve error logging clarity.The profile handling logic correctly validates the profile path and delegates to
setSettingsFromProfile. The fallback to discover a default profile is appropriate.One minor improvement: the error at line 171 is logged but execution continues silently. Consider distinguishing between "no profile found" (expected) and actual errors.
profilePath, err := analyzeCmd.findSingleProfile(profilesDir) if err != nil { - analyzeCmd.log.Error(err, "did not find valid profile in default path") + analyzeCmd.log.V(5).Info("no default profile found", "path", profilesDir, "reason", err.Error()) }
105-106: Verify profile-only analysis flow works without--input.When
--profileis specified without--input, the required flag validation is skipped (lines 105-106). EnsuresetSettingsFromProfilecorrectly derives the input path from the profile location beforeValidateruns.cmd/login.go (5)
24-39: LGTM!The authentication data structures are well-designed with appropriate JSON tags for the login flow.
238-257: LGTM!Token storage follows security best practices with restrictive file permissions (0600) and directory permissions (0700).
259-278: LGTM!Token loading is implemented correctly with helpful error messages for missing authentication files.
280-295: LGTM!Token refresh logic correctly reuses the login infrastructure and persists the new tokens.
297-316: Verify the handling of tokens without expiration claims.Line 315 returns
false(not expired) when a token has noexpclaim, meaning such tokens are considered valid indefinitely. Ensure this aligns with the Hub's token format and security requirements.If tokens without expiration should be treated as expired, apply this diff:
if exp, ok := claims["exp"]; ok { if expFloat, ok := exp.(float64); ok { expTime := time.Unix(int64(expFloat), 0) return time.Now().After(expTime), nil } return false, fmt.Errorf("invalid expiration claim format in token") } - return false, nil + // Tokens without expiration claims should be treated as expired for security + return true, nil
| if l.insecure { | ||
| tr := &http.Transport{ | ||
| TLSClientConfig: &tls.Config{InsecureSkipVerify: true}, | ||
| } | ||
| client.Transport = tr | ||
| } |
There was a problem hiding this comment.
Add minimum TLS version to strengthen cryptographic security.
The TLS configuration on line 136 uses InsecureSkipVerify without specifying MinVersion. While InsecureSkipVerify is intended for development/testing, the configuration should still enforce modern TLS versions to prevent downgrade attacks.
As per static analysis hints, apply this diff:
if l.insecure {
tr := &http.Transport{
- TLSClientConfig: &tls.Config{InsecureSkipVerify: true},
+ TLSClientConfig: &tls.Config{
+ InsecureSkipVerify: true,
+ MinVersion: tls.VersionTLS12,
+ },
}
client.Transport = tr
}Consider using tls.VersionTLS13 if compatibility with TLS 1.2 servers is not required.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if l.insecure { | |
| tr := &http.Transport{ | |
| TLSClientConfig: &tls.Config{InsecureSkipVerify: true}, | |
| } | |
| client.Transport = tr | |
| } | |
| if l.insecure { | |
| tr := &http.Transport{ | |
| TLSClientConfig: &tls.Config{ | |
| InsecureSkipVerify: true, | |
| MinVersion: tls.VersionTLS12, | |
| }, | |
| } | |
| client.Transport = tr | |
| } |
🧰 Tools
🪛 ast-grep (0.40.0)
[warning] 135-135: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{InsecureSkipVerify: true}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
🤖 Prompt for AI Agents
In cmd/login.go around lines 134 to 139 the TLS config sets InsecureSkipVerify
without a MinVersion; update the tls.Config to also set MinVersion to a modern
TLS constant (e.g. tls.VersionTLS13 or tls.VersionTLS12 if TLS 1.2 compatibility
is required) so even in insecure/dev mode the client enforces a minimum TLS
version and reduces risk of downgrade attacks.
| if l.insecure { | ||
| tr := &http.Transport{ | ||
| TLSClientConfig: &tls.Config{InsecureSkipVerify: true}, | ||
| } | ||
| client.Transport = tr | ||
| } |
There was a problem hiding this comment.
Add minimum TLS version to strengthen cryptographic security.
The TLS configuration on line 189 has the same issue as performLogin - missing MinVersion while using InsecureSkipVerify.
Apply this diff:
if l.insecure {
tr := &http.Transport{
- TLSClientConfig: &tls.Config{InsecureSkipVerify: true},
+ TLSClientConfig: &tls.Config{
+ InsecureSkipVerify: true,
+ MinVersion: tls.VersionTLS12,
+ },
}
client.Transport = tr
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if l.insecure { | |
| tr := &http.Transport{ | |
| TLSClientConfig: &tls.Config{InsecureSkipVerify: true}, | |
| } | |
| client.Transport = tr | |
| } | |
| if l.insecure { | |
| tr := &http.Transport{ | |
| TLSClientConfig: &tls.Config{ | |
| InsecureSkipVerify: true, | |
| MinVersion: tls.VersionTLS12, | |
| }, | |
| } | |
| client.Transport = tr | |
| } |
🧰 Tools
🪛 ast-grep (0.40.0)
[warning] 188-188: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{InsecureSkipVerify: true}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
🤖 Prompt for AI Agents
In cmd/login.go around lines 187 to 192, the TLS config created when l.insecure
is true sets InsecureSkipVerify but omits MinVersion; update the TLSClientConfig
to include MinVersion: tls.VersionTLS12 (or tls.VersionTLS13 if you prefer to
require TLS 1.3) to enforce a minimum protocol version when skipping
verification; ensure the crypto/tls package is imported if not already.
0333565 to
2e41ba0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (13)
cmd/config.go (3)
224-229: Add TLSMinVersionto strengthen cryptographic security.The TLS configuration sets
InsecureSkipVerifywithout specifyingMinVersion. Even in insecure mode, setting a minimum TLS version helps prevent downgrade attacks.if insecure { tr := &http.Transport{ - TLSClientConfig: &tls.Config{InsecureSkipVerify: true}, + TLSClientConfig: &tls.Config{ + InsecureSkipVerify: true, + MinVersion: tls.VersionTLS12, + }, } client.Transport = tr }
123-126: Fix logr usage: avoid printf-style format string.
logr.Logger.Infodoes not supportfmt-style formatting. The%swill be logged literally andprofilesDirwill be treated as a key without a value.- c.log.Info("no profile directories found in: %s", profilesDir) + c.log.Info("no profile directories found", "path", profilesDir)
344-349: Potential issue: URL mismatch returns empty struct silently.When
apps[0].Repository.URL != s.url, the function returns an emptyapi.Application{}without error. This could cause confusion downstream whenapplication.Resource.IDis 0.Since the Hub already filtered by URL, this check may be redundant. Consider either removing it or returning an explicit error on mismatch:
var application api.Application if apps[0].Repository.URL == s.url { application = apps[0] +} else { + return api.Application{}, fmt.Errorf("application URL mismatch: expected %s, got %s", s.url, apps[0].Repository.URL) }cmd/login.go (4)
134-139: Add minimum TLS version to strengthen cryptographic security.if l.insecure { tr := &http.Transport{ - TLSClientConfig: &tls.Config{InsecureSkipVerify: true}, + TLSClientConfig: &tls.Config{ + InsecureSkipVerify: true, + MinVersion: tls.VersionTLS12, + }, } client.Transport = tr }
187-192: Add minimum TLS version to strengthen cryptographic security.Same issue as
performLogin- missingMinVersionin TLS configuration.
112-114: Defaulting to HTTP is insecure for credential transmission.Prepending
http://when no scheme is provided transmits credentials in plaintext.if !strings.HasPrefix(l.host, "http://") && !strings.HasPrefix(l.host, "https://") { - l.host = "http://" + l.host + l.host = "https://" + l.host }
206-211: SetContent-Typeheader for JSON request body.The request has a JSON body but only sets
Acceptheader, notContent-Type.req, err := http.NewRequest("POST", refreshURL, bytes.NewBuffer(jsonData)) if err != nil { return nil, err } + req.Header.Set("Content-Type", "application/json") req.Header.Set("Accept", "application/json")cmd/config_test.go (4)
375-395: Test will fail without stored authentication tokens.
getHubClient()callsnewHubClientWithOptions()which callsloadStoredTokens(). If no tokens are stored, this returns an error. The test doesn't set up stored tokens, so it will fail in clean environments.Either mock
loadStoredTokensor pre-populate token storage before testing.
627-695: Test data format mismatch: YAML test data but JSON parser.
parseApplicationsFromHubusesjson.Unmarshal, but test cases provide YAML-formatted strings (variable namedyamlData). Update test data to use JSON format and renameyamlDatatojsonData:{ name: "valid YAML with applications", - yamlData: ` -- id: 1 - name: "App1" - repository: - url: "https://github.com/example/app1" -`, + jsonData: `[{"id": 1, "name": "App1", "repository": {"url": "https://github.com/example/app1"}}]`, wantErr: false, expected: 1, },
1145-1147: Test expects wrong error message.
deleteTarFilereturnsos.Remove's error directly, not "failed to delete tar file".- errMsg: "failed to delete tar file", + errMsg: "no such file or directory",
504-518: Test uses non-standard header name.The production code was fixed to use
Authorization, but this test still checks forAuthenticationat line 506. The test should be updated to match:{ name: "check authentication header with token", serverResponse: func(w http.ResponseWriter, r *http.Request) { - auth := r.Header.Get("Authentication") + auth := r.Header.Get("Authorization") if auth != "Bearer test-token" {cmd/profile.go (2)
17-32: Usepathparameter consistently instead ofa.profilefield.The function checks
a.profile == ""(line 18) but reads frompathparameter (line 21). When called for an auto-discovered profile,a.profilemay be empty whilepathis valid, causing incorrect early return.func (a *analyzeCommand) unmarshalProfile(path string) (*api.AnalysisProfile, error) { - if a.profile == "" { + if path == "" { return nil, nil }
39-48: Potential panic with negative slice bound; usefilepath.Dirinstead.If
a.profilestarts with.konveyor(e.g., relative path.konveyor/profiles/foo/profile.yaml),konveyorIndexis 0 anda.profile[:konveyorIndex-1]panics with a negative slice bound.Use
filepath.Dirchain for robust path handling and operate on thepathparameter:- konveyorIndex := strings.Index(a.profile, ".konveyor") - if konveyorIndex == -1 { - return fmt.Errorf("profile path does not contain .konveyor directory: %s", a.profile) - } - // get dir before .konveyor/ - locationDir := a.profile[:konveyorIndex-1] + // path: .../app/.konveyor/profiles/<name>/profile.yaml + profDir := filepath.Dir(path) // .../app/.konveyor/profiles/<name> + profilesDir := filepath.Dir(profDir) // .../app/.konveyor/profiles + konveyorDir := filepath.Dir(profilesDir) // .../app/.konveyor + locationDir := filepath.Dir(konveyorDir) // .../app
🧹 Nitpick comments (1)
cmd/config.go (1)
496-504: Consider limiting file size during extraction to prevent resource exhaustion.The
io.Copyat line 500 has no size limit. A malicious or corrupted archive could contain extremely large files that exhaust disk space. While the Hub is a trusted source, defense-in-depth suggests limiting extraction.- _, err = io.Copy(outFile, tarReader) + // Limit extraction to prevent decompression bombs (e.g., 1GB max per file) + const maxFileSize = 1 << 30 // 1GB + _, err = io.CopyN(outFile, tarReader, maxFileSize) + if err == io.EOF { + err = nil // Expected when file is smaller than limit + }Alternatively, compare against
header.Sizebefore copying.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Dockerfile(2 hunks)cmd/config.go(1 hunks)cmd/config_test.go(1 hunks)cmd/login.go(1 hunks)cmd/profile.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-08T15:06:52.056Z
Learnt from: abrugaro
Repo: konveyor/kantra PR: 524
File: cmd/dump-rules.go:0-0
Timestamp: 2025-08-08T15:06:52.056Z
Learning: In konveyor/kantra cmd/dump-rules.go (Go/Cobra), the dump-rules command should not use package-level variables for flags (output, overwrite). Flags must be defined as local variables inside NewDumpRulesCommand to avoid shared mutable state across commands/tests.
Applied to files:
cmd/config.gocmd/profile.go
🧬 Code graph analysis (2)
cmd/config.go (4)
cmd/login.go (1)
NewLoginCmd(51-79)cmd/profile.go (1)
Profiles(15-15)cmd/settings.go (1)
Config(26-35)cmd/static-report.go (1)
Application(16-24)
cmd/config_test.go (2)
cmd/config.go (2)
NewConfigCmd(46-85)NewSyncCmd(135-184)cmd/profile.go (1)
Profiles(15-15)
🪛 ast-grep (0.40.0)
cmd/config.go
[warning] 225-225: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{InsecureSkipVerify: true}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
cmd/login.go
[warning] 135-135: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{InsecureSkipVerify: true}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
[warning] 188-188: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{InsecureSkipVerify: true}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build & test from commit
🔇 Additional comments (14)
Dockerfile (2)
15-16: LGTM!The CGO prerequisites installation for Linux builds is appropriate. Using
build-essentialandlibsqlite3-devenables sqlite3 support for the Linux target.
41-57: Cross-compilation with CGO_ENABLED=1 requires additional toolchains for darwin and windows targets.Enabling CGO for darwin and windows targets (
GOOS=darwin,GOOS=windows) while building on Linux requires cross-compilation toolchains:osxcrosswith macOS SDK for darwin, andmingw-w64for windows. The current Dockerfile only installsbuild-essentialandlibsqlite3-dev(Linux native tools), so these cross-platform builds will fail at compile time unless the toolchains and appropriateCC/CXXenvironment variables are configured elsewhere.Verify whether:
- These darwin/windows binaries are actually built and tested in CI, or if they are unused targets
- CGO is required for all three platforms or only for Linux
- Cross-compiler toolchains are installed in a multi-stage build not shown in the snippet
If CGO is only needed for Linux, set
CGO_ENABLED=0for darwin/windows builds.cmd/config.go (6)
23-44: LGTM!The struct definitions for
configCommand,syncCommand, andhubClientare well-organized with appropriate fields for their respective responsibilities.
46-85: LGTM!The
NewConfigCmdfunction follows Cobra conventions properly. Flags are correctly bound to the command struct instance rather than package-level variables, which aligns with the learnings about avoiding shared mutable state.
135-184: LGTM!The
NewSyncCmdfunction is well-structured with proper validation, required flags, and a clear workflow for syncing Hub profiles.
186-199: LGTM!The validation logic properly checks that the application path is provided, exists, and is a directory.
241-287: LGTM!The request handling with token refresh logic is well-implemented. The retry mechanism for expired tokens and 401 responses provides robust authentication handling.
440-513: LGTM!The tar extraction implementation is secure with proper path traversal protection (lines 481-483), handles both plain and gzip-compressed archives, and includes appropriate resource cleanup.
cmd/config_test.go (1)
22-155: LGTM!Comprehensive test coverage for command construction and validation logic with proper table-driven tests and cleanup.
cmd/profile.go (2)
84-114: LGTM!The function properly handles edge cases (empty input, non-existent directory) and correctly collects rule subdirectory paths.
116-154: LGTM!The function correctly implements the single-profile discovery logic with appropriate handling for edge cases. The behavior of returning empty string for 0 or multiple profiles is documented in comments.
cmd/login.go (3)
24-79: LGTM!The login command setup and data structures are well-designed, following Cobra patterns with appropriate flag handling.
238-278: LGTM!Token storage uses appropriate restrictive permissions (0700 for directory, 0600 for file) to protect sensitive authentication data.
297-316: LGTM!The token expiry check correctly uses
ParseUnverifiedsince only the expiry time is needed, not signature validation. The handling of edge cases (empty token, missing exp claim) is reasonable.
eemcmullan
force-pushed
the
central-config
branch
from
2e41ba0 to
cc4a090
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
cmd/config.go (3)
106-133: Fix logrInfousage: avoid printf-style format string
logr.Logger.Infodoesn’t usefmt-style formatting. This call:c.log.Info("no profile directories found in: %s", profilesDir)logs the literal
%sand treatsprofilesDiras a key. Pass it as structured data instead.- if len(profileDirs) == 0 { - c.log.Info("no profile directories found in: %s", profilesDir) - return nil - } + if len(profileDirs) == 0 { + c.log.Info("no profile directories found", "path", profilesDir) + return nil + }
212-235: Add TLSMinVersionin insecure mode to avoid weak protocol downgradeWhen
insecureis true, the TLS config only setsInsecureSkipVerifyand leaves the minimum TLS version at default. Even for “insecure” mode, it’s safer to pin a modern minimum TLS version to avoid downgrades.- if insecure { - tr := &http.Transport{ - TLSClientConfig: &tls.Config{InsecureSkipVerify: true}, - } - client.Transport = tr - } + if insecure { + tr := &http.Transport{ + TLSClientConfig: &tls.Config{ + InsecureSkipVerify: true, + MinVersion: tls.VersionTLS13, // or tls.VersionTLS12 if you need TLS 1.2 compatibility + }, + } + client.Transport = tr + }This also mirrors the guidance already applied (or planned) in
cmd/login.gofor login/refresh clients.
314-350: Avoid returning a zero-valuedapi.Applicationwhen URLs don’t matchIf exactly one application is returned but
apps[0].Repository.URL != s.url, the function returns a zero-valuedapi.Applicationwith no error. This is confusing for callers and can lead to downstream use ofapplication.Resource.ID == 0(e.g., calling/applications/0/...).Given that the Hub query is already filtered by
repository.url='%s', you can safely return the single element, or explicitly error when no match is found.- if len(apps) == 0 { - return api.Application{}, fmt.Errorf("no applications found in Hub for URL: %s", urlRepo) - - // TODO handle multiple applications later - } else if len(apps) > 1 { - return api.Application{}, fmt.Errorf("multiple applications found in Hub for URL: %s", urlRepo) - } - var application api.Application - if apps[0].Repository.URL == s.url { - application = apps[0] - } - - return application, nil + if len(apps) == 0 { + return api.Application{}, fmt.Errorf("no applications found in Hub for URL: %s", urlRepo) + } + // TODO handle multiple applications later + if len(apps) > 1 { + return api.Application{}, fmt.Errorf("multiple applications found in Hub for URL: %s", urlRepo) + } + + // Query is already filtered by repository.url, so return the single result. + return apps[0], nilcmd/login.go (3)
81-128: Defaulting to plain HTTP for login is insecure; prefer HTTPSWhen the user omits a scheme,
Loginprependshttp://:if !strings.HasPrefix(l.host, "http://") && !strings.HasPrefix(l.host, "https://") { l.host = "http://" + l.host }This sends credentials in cleartext by default, which is a serious security risk. The default should be HTTPS; users who explicitly specify
http://can still opt into insecure transport.- if !strings.HasPrefix(l.host, "http://") && !strings.HasPrefix(l.host, "https://") { - l.host = "http://" + l.host - } + if !strings.HasPrefix(l.host, "http://") && !strings.HasPrefix(l.host, "https://") { + // Default to HTTPS; users can explicitly opt into http:// if needed. + l.host = "https://" + l.host + }
130-181: Add TLSMinVersioninperformLoginfor stronger crypto (even in insecure mode)In
performLogin, the TLS config used whenl.insecureis true setsInsecureSkipVerifybut leavesMinVersionat defaults, which can allow weaker protocol versions than desired.- if l.insecure { - tr := &http.Transport{ - TLSClientConfig: &tls.Config{InsecureSkipVerify: true}, - } - client.Transport = tr - } + if l.insecure { + tr := &http.Transport{ + TLSClientConfig: &tls.Config{ + InsecureSkipVerify: true, + MinVersion: tls.VersionTLS13, // or tls.VersionTLS12 for wider compatibility + }, + } + client.Transport = tr + }The rest of the login flow (timeouts, error handling, and token extraction) looks solid.
183-236: Align refresh client TLS settings and set JSONContent-TypeheaderTwo things in
RefreshToken:
- TLS config again sets
InsecureSkipVerifywithout aMinVersion, same concern asperformLogin.- The POST with a JSON body sets
Acceptbut notContent-Type. Some servers requireContent-Type: application/jsonfor JSON payloads.- if l.insecure { - tr := &http.Transport{ - TLSClientConfig: &tls.Config{InsecureSkipVerify: true}, - } - client.Transport = tr - } + if l.insecure { + tr := &http.Transport{ + TLSClientConfig: &tls.Config{ + InsecureSkipVerify: true, + MinVersion: tls.VersionTLS13, // or tls.VersionTLS12 if needed + }, + } + client.Transport = tr + } @@ - req, err := http.NewRequest("POST", refreshURL, bytes.NewBuffer(jsonData)) + req, err := http.NewRequest("POST", refreshURL, bytes.NewBuffer(jsonData)) @@ - req.Header.Set("Accept", "application/json") + req.Header.Set("Content-Type", "application/json") + req.Header.Set("Accept", "application/json")
🧹 Nitpick comments (3)
cmd/config.go (3)
87-104: Improve error message when profiles directory is missing (optional)When
profilesDirdoes not exist,Validatereturns the rawos.Staterror, which will look like a low-level “no such file or directory” path error. Consider returning a more user-focused message that mentions the expected.konveyor/profileslocation under the provided application path.- profilesDir := filepath.Join(c.listProfiles, Profiles) - if _, err := os.Stat(profilesDir); os.IsNotExist(err) { - return err - } + profilesDir := filepath.Join(c.listProfiles, Profiles) + if _, err := os.Stat(profilesDir); os.IsNotExist(err) { + return fmt.Errorf("no profiles directory found at %s; expected %s under the application path", profilesDir, Profiles) + }
363-383: Minor: fix typo in method name for clarity
getProfilesFromHubApplicatonis misspelled (“Applicaton”). Renaming it togetProfilesFromHubApplicationwould improve readability and searchability. Since it’s an unexported method, the impact is limited to local call sites.-func (s *syncCommand) getProfilesFromHubApplicaton(appID int) ([]api.AnalysisProfile, error) { +func (s *syncCommand) getProfilesFromHubApplication(appID int) ([]api.AnalysisProfile, error) {and update the single caller in
RunE.
385-438: Clarify success log message to point at extracted directory
downloadToFiledeletes the tarfile after extraction but logs success using the tar path:log.Info("profile bundle downloaded successfully", "path", outputPath)At this point
outputPathno longer exists; the useful artifact isextractDir.- extractDir := strings.TrimSuffix(outputPath, ".tar") + extractDir := strings.TrimSuffix(outputPath, ".tar") @@ - log.Info("profile bundle downloaded successfully", "path", outputPath) + log.Info("profile bundle downloaded successfully", "path", extractDir)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Dockerfile(2 hunks)cmd/config.go(1 hunks)cmd/config_test.go(1 hunks)cmd/login.go(1 hunks)cmd/profile.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- cmd/profile.go
- cmd/config_test.go
- Dockerfile
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-08T15:06:52.056Z
Learnt from: abrugaro
Repo: konveyor/kantra PR: 524
File: cmd/dump-rules.go:0-0
Timestamp: 2025-08-08T15:06:52.056Z
Learning: In konveyor/kantra cmd/dump-rules.go (Go/Cobra), the dump-rules command should not use package-level variables for flags (output, overwrite). Flags must be defined as local variables inside NewDumpRulesCommand to avoid shared mutable state across commands/tests.
Applied to files:
cmd/config.go
🧬 Code graph analysis (1)
cmd/config.go (3)
cmd/login.go (1)
NewLoginCmd(51-79)cmd/profile.go (1)
Profiles(15-15)cmd/static-report.go (1)
Application(16-24)
🪛 ast-grep (0.40.0)
cmd/config.go
[warning] 225-225: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{InsecureSkipVerify: true}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
cmd/login.go
[warning] 135-135: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{InsecureSkipVerify: true}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
[warning] 188-188: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{InsecureSkipVerify: true}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build & test from commit
🔇 Additional comments (9)
cmd/config.go (5)
46-85: Config command wiring and flag handling look solidThe
configroot command wiring, subcommand registration, and the use of struct fields (rather than package-level vars) for flags are all in good shape and align with prior guidance in this repo.
135-199: Sync command flag validation/use is reasonable
syncwiring and validation look consistent: required flags are enforced via Cobra,Validatechecksapplication-pathis a directory, and--insecureis correctly taken from the parent persistent flag. The overall structure ofRunEand error propagation is clear.
241-287: Token refresh and retry flow looks correctThe
doRequestWithRetrylogic—loading stored tokens, checking expiry, refreshing on expiry or 401, and then retrying once—is coherent and avoids infinite recursion. Error paths (e.g., refresh failures or bad JWT claims) properly abort the request instead of sending an unauthenticated call.
440-513: Extraction logic and path validation look robustThe tar extraction correctly:
- Detects optional gzip compression,
- Enforces a prefix check on
targetPathto prevent directory traversal,- Creates parent directories safely and applies file modes.
Symlinks and other non-regular entries are effectively ignored rather than followed, which is a safe default for untrusted bundles.
515-521: Tar deletion helper is straightforward
deleteTarFileis a minimal wrapper aroundos.Removewith proper error propagation and is used defensively indownloadToFile(logging on failure without failing the whole operation). No changes needed.cmd/login.go (4)
51-79: Login command wiring and flag usage look goodThe
loginCobra command is wired cleanly, arguments are mapped predictably to fields, and--insecureis stored on the struct (not as a package var), consistent with earlier project conventions.
238-278: Token storage and loading semantics look correctStoring tokens under
~/.kantra/auth.jsonwith directory mode0700and file mode0600is a good default, andloadStoredTokenscorrectly distinguishes between “no stored auth” and other I/O errors before unmarshaling. This integrates cleanly with the hub client logic incmd/config.go.
280-295: Reuse of login flow for token refresh is reasonable
hubClient.refreshStoredTokenreuses theloginCommand’sRefreshTokenandstoreTokensmethods, ensuring refresh behavior stays consistent with the login CLI without duplicating logic. The method respects the hub client’sinsecureandhostsettings and correctly persists new tokens.
297-315: JWT expiry parsing is cautious and favors safety
isTokenExpired:
- Treats empty tokens as expired,
- Parses the JWT without validating signature just to inspect
exp,- Treats parse/claim format errors as failures (caller gets an error),
- Considers tokens without an
expclaim as non-expiring.This is a reasonable balance for a CLI client; callers already handle errors distinctly from “expired” and will not silently accept malformed tokens.
eemcmullan
force-pushed
the
central-config
branch
from
cc4a090 to
72c2aca
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Dockerfile (1)
33-42: Consider build performance implications of osxcross compilation.The osxcross build (lines 33–40) is a heavyweight multi-step process involving SDK download and compilation that runs on every image build. This will add significant time to the build pipeline. While the cleanup at line 40 is good for image size, consider:
- Whether this layer can be cached or moved to a separate builder image (e.g., a pre-built
osxcross-builderimage on a registry)- Whether build frequency justifies the time cost or if cross-compilation for macOS is critical for every build
This is noted as context for operational planning; no code change is strictly necessary.
cmd/config.go (1)
452-452: Verify Seek return value.The call
tarFile.Seek(0, 0)at line 452 ignores the return value. While this is typically safe for local files, it's better practice to check the error return.Apply this diff:
var reader io.Reader = tarFile -tarFile.Seek(0, 0) +if _, err := tarFile.Seek(0, 0); err != nil { + return err +} header := make([]byte, 3)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Dockerfile(3 hunks)cmd/config.go(1 hunks)cmd/config_test.go(1 hunks)cmd/login.go(1 hunks)cmd/profile.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/config_test.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-08T15:06:52.056Z
Learnt from: abrugaro
Repo: konveyor/kantra PR: 524
File: cmd/dump-rules.go:0-0
Timestamp: 2025-08-08T15:06:52.056Z
Learning: In konveyor/kantra cmd/dump-rules.go (Go/Cobra), the dump-rules command should not use package-level variables for flags (output, overwrite). Flags must be defined as local variables inside NewDumpRulesCommand to avoid shared mutable state across commands/tests.
Applied to files:
cmd/config.gocmd/profile.go
🪛 ast-grep (0.40.0)
cmd/config.go
[warning] 225-225: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{InsecureSkipVerify: true}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
cmd/login.go
[warning] 135-135: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{InsecureSkipVerify: true}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
[warning] 188-188: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{InsecureSkipVerify: true}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build & test from commit
🔇 Additional comments (4)
Dockerfile (4)
15-31: Good cross-compilation toolchain foundation.The dependency installation appropriately includes sqlite headers, native compiler tools, Windows cross-compiler (mingw), and macOS build prerequisites.
68-68: Cross-compilation flags are well-configured.CGO is properly enabled across all platforms, and the per-platform compiler toolchain overrides (CC/CXX for Darwin and Windows) are appropriate for cross-compilation. The GOARCH parameter enables parameterized architecture builds.
One minor observation: the Windows build (line 80) only sets CC but not CXX. This is likely acceptable for pure C dependencies, but confirm that the Windows mingw build does not require C++ support.
Also applies to: 74-74, 80-80
62-62: Platform-specific naming is consistent and correct.The output binaries are now explicitly named by platform (kantra for Linux, darwin-kantra for macOS, windows-kantra.exe for Windows), and all three are properly copied into the final image. The default GOARCH=amd64 is appropriate.
Also applies to: 84-84, 108-108
33-40: Verify osxcross build produces expected compiler path and SDK download stability.The osxcross build hardcodes a verification check for
x86_64-apple-darwin20.4-clang(line 39), which is later used in Darwin and downstream build commands. Confirm that:
- The osxcross build consistently produces this exact compiler binary at the expected path
- The MacOSX11.3.sdk download URL is stable or has a documented fallback
- Build failures from missing SDK downloads are caught early with clear error messages
cmd/analyze.go
Outdated
| } | ||
|
|
||
| // get profile options for analysis | ||
| if analyzeCmd.profile != "" { |
There was a problem hiding this comment.
nit: if profile is a dir can we make it profileDir
There was a problem hiding this comment.
Does this mean that I must have a profile? Do we create one if none exist?
There was a problem hiding this comment.
An analyze command will work without --profile specified. And the default profile dir is created during the sync process. It expects a Hub profile to be under <app>/.konveyor/profiles/. We may need to update some of this once we add the ability to configure analysis from a settings profile.
cmd/analyze.go
Outdated
| } | ||
| } else { | ||
| // check for a single profile in default path | ||
| profilesDir := filepath.Join(analyzeCmd.input, Profiles) |
There was a problem hiding this comment.
Are profiles stored inside the application directory?
And if so how does this work with batch
eemcmullan
force-pushed
the
central-config
branch
from
72c2aca to
e2e4042
Compare
Signed-off-by: Emily McMullan <[email protected]>
eemcmullan
force-pushed
the
central-config
branch
from
e2e4042 to
39ff60e
Compare
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (55.46%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #588 +/- ##
==========================================
+ Coverage 36.24% 38.79% +2.55%
==========================================
Files 34 37 +3
Lines 4277 4923 +646
==========================================
+ Hits 1550 1910 +360
- Misses 2555 2780 +225
- Partials 172 233 +61 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (7)
pkg/profile/profile.go (1)
74-84: Potential panic with relative paths starting with.konveyor.When
pathis a relative path like.konveyor/profiles/foo/profile.yaml,konveyorIndexis0, andpath[:konveyorIndex-1]evaluates topath[:-1], causing a panic due to negative slice bounds.Additionally, if
UnmarshalProfilereturnsnil(e.g., whenpathis empty), the code continues and will dereferencenilwhen accessingprofile.Mode,profile.Scope, etc.Consider using
filepath.Dirchain for robust path handling and add nil check:func SetSettingsFromProfile(path string, cmd *cobra.Command, settings *ProfileSettings) error { profile, err := UnmarshalProfile(path) if err != nil { return err } - konveyorIndex := strings.Index(path, ".konveyor") - if konveyorIndex == -1 { - return fmt.Errorf("profile path does not contain .konveyor directory: %s", path) + if profile == nil { + return nil + } + if !strings.Contains(path, ".konveyor") { + return fmt.Errorf("profile path does not contain .konveyor directory: %s", path) + } + // Derive app root: profile.yaml -> profile dir -> profiles dir -> .konveyor -> app root + profileDir := filepath.Dir(path) + profilesDir := filepath.Dir(profileDir) + konveyorDir := filepath.Dir(profilesDir) + locationDir := filepath.Dir(konveyorDir) + if locationDir == "." { + locationDir, _ = os.Getwd() } - // get dir before .konveyor/ - locationDir := path[:konveyorIndex-1]This aligns with the past review suggestion to use
filepath.Dirinstead of string slicing.cmd/config/login.go (4)
112-114: Defaulting to HTTP transmits credentials in plaintext.The code prepends
http://when no scheme is provided, but per past review the fix should default tohttps://. The past review comment indicates this was addressed in commit 2e41ba0, but the current code still showshttp://.Please verify if this was intentionally reverted or if the fix was lost:
#!/bin/bash # Check git history for this line git log --oneline -n 10 -- cmd/config/login.go git show 2e41ba0 --stat 2>/dev/null || echo "Commit not found"
134-139: Add minimum TLS version to TLS configuration.The
TLSClientConfigat line 136 is missingMinVersion. WhileInsecureSkipVerifyis for development/testing, setting a minimum TLS version prevents downgrade attacks.Apply this diff:
if l.insecure { tr := &http.Transport{ - TLSClientConfig: &tls.Config{InsecureSkipVerify: true}, + TLSClientConfig: &tls.Config{ + InsecureSkipVerify: true, + MinVersion: tls.VersionTLS12, + }, } client.Transport = tr }
187-192: Add minimum TLS version to TLS configuration in RefreshToken.Same issue as
performLogin- the TLS configuration is missingMinVersion.Apply this diff:
if l.insecure { tr := &http.Transport{ - TLSClientConfig: &tls.Config{InsecureSkipVerify: true}, + TLSClientConfig: &tls.Config{ + InsecureSkipVerify: true, + MinVersion: tls.VersionTLS12, + }, } client.Transport = tr }
206-211: Missing Content-Type header for JSON request body.The
RefreshTokenfunction setsAcceptheader but notContent-Type, even though the request has a JSON body (marshaled at line 201). The past review comment indicates this was addressed in commit e2e4042, but the current code appears to be missing it.Apply this diff:
req, err := http.NewRequest("POST", refreshURL, bytes.NewBuffer(jsonData)) if err != nil { return nil, err } + req.Header.Set("Content-Type", "application/json") req.Header.Set("Accept", "application/json")cmd/config/config.go (2)
123-126: Fix logr usage: avoid printf-style format string.
logr.Logger.Infodoes not usefmtformatting. The%swill be logged literally, andprofilesDirwill be treated as a key without a value.Apply this diff:
if len(profileDirs) == 0 { - c.log.Info("no profile directories found in: %s", profilesDir) + c.log.Info("no profile directories found", "path", profilesDir) return nil }
355-360: Return an error when application URL doesn't match.When
apps[0].Repository.URL != s.url, the function returns an emptyApplication{}without error. Downstream code usingapplication.IDwill get 0, which could cause confusing behavior or API errors.var application profile.Application - if apps[0].Repository != nil && apps[0].Repository.URL == s.url { - application = apps[0] + if apps[0].Repository == nil || apps[0].Repository.URL != s.url { + return profile.Application{}, fmt.Errorf("application repository URL mismatch: expected %s", s.url) } + application = apps[0] return application, nil
🧹 Nitpick comments (5)
pkg/profile/profile_test.go (2)
590-622: Consider using validation functions instead of test name comparisons.The logic that switches on
tt.name(e.g.,if tt.name == "rules directory with mixed files and directories") is fragile and will break if test names are renamed. Other tests in this file correctly usevalidatefunctions in the test struct.Refactor to use validation functions consistent with the pattern used elsewhere:
tests := []struct { name string setupFunc func() (string, func(), error) wantRules []string wantErr bool profileDir string errMsg string + validate func([]string, *testing.T) }{
1079-1090: Consider simplifying by passingProfileSettingsdirectly to validate.The pattern of copying settings back to
mockAnalyzeCommandand then validating the mock is indirect. Thevalidatefunction could simply accept*ProfileSettingsdirectly.- validate func(*mockAnalyzeCommand, *testing.T) + validate func(*ProfileSettings, *testing.T) }{ // ... validate: func(settings *ProfileSettings, t *testing.T) { - if cmd.mode != string(provider.SourceOnlyAnalysisMode) { + if settings.Mode != string(provider.SourceOnlyAnalysisMode) {cmd/config/login.go (1)
297-316: Consider tokens withoutexpclaim as expired.If a token lacks an
expclaim, the function returnsfalse, nil(not expired). For security, tokens without an expiry should be treated as expired to force re-authentication.if exp, ok := claims["exp"]; ok { if expFloat, ok := exp.(float64); ok { expTime := time.Unix(int64(expFloat), 0) return time.Now().After(expTime), nil } return false, fmt.Errorf("invalid expiration claim format in token") } - return false, nil + // Tokens without expiry claim should be treated as expired for security + return true, nilcmd/config/config.go (1)
507-515: Consider limiting extracted file size to prevent resource exhaustion.The tar extraction uses
io.Copywithout size limits. A malicious tar archive could contain extremely large files that exhaust disk space or memory.Consider using
io.LimitReaderor similar to cap individual file sizes:+const maxExtractFileSize = 100 * 1024 * 1024 // 100MB limit + case tar.TypeReg: parentDir := filepath.Dir(targetPath) err = os.MkdirAll(parentDir, 0755) if err != nil { return err } + if header.Size > maxExtractFileSize { + return fmt.Errorf("file %s exceeds maximum size limit", header.Name) + } outFile, err := os.Create(targetPath) if err != nil { return err } - _, err = io.Copy(outFile, tarReader) + _, err = io.Copy(outFile, io.LimitReader(tarReader, maxExtractFileSize)) outFile.Close()cmd/config/config_test.go (1)
376-417: Consider extracting auth file setup/cleanup to a helper function.The auth file backup, mock token creation, and cleanup pattern is repeated across many tests (TestSyncCommand_getHubClient, TestHubClient_doRequest, TestSyncCommand_getApplicationFromHub, etc.). Extracting this to a helper would reduce duplication and ensure consistent test setup.
Example helper:
func setupTestAuth(t *testing.T) (cleanup func()) { t.Helper() homeDir, err := os.UserHomeDir() if err != nil { t.Fatalf("Failed to get home directory: %v", err) } kantraDir := filepath.Join(homeDir, ".kantra") authFile := filepath.Join(kantraDir, "auth.json") var backupData []byte var hasBackup bool if data, err := os.ReadFile(authFile); err == nil { backupData = data hasBackup = true } validJWT := "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJleHAiOjE4OTM0NTYwMDB9.Hs_ZQhwq7Uy9E7VzTpSKNqvWUdKKYKxWJhUlNhqJGKE" testAuth := LoginResponse{ Host: "http://test-host", Token: validJWT, RefreshToken: "test-refresh-token", } os.MkdirAll(kantraDir, 0755) authData, _ := json.Marshal(testAuth) os.WriteFile(authFile, authData, 0600) return func() { if hasBackup { os.WriteFile(authFile, backupData, 0600) } else { os.Remove(authFile) } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
cmd/analyze.go(7 hunks)cmd/config/config.go(1 hunks)cmd/config/config_test.go(1 hunks)cmd/config/login.go(1 hunks)cmd/root.go(2 hunks)go.mod(2 hunks)pkg/profile/application.go(1 hunks)pkg/profile/profile.go(1 hunks)pkg/profile/profile_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-08T15:06:52.056Z
Learnt from: abrugaro
Repo: konveyor/kantra PR: 524
File: cmd/dump-rules.go:0-0
Timestamp: 2025-08-08T15:06:52.056Z
Learning: In konveyor/kantra cmd/dump-rules.go (Go/Cobra), the dump-rules command should not use package-level variables for flags (output, overwrite). Flags must be defined as local variables inside NewDumpRulesCommand to avoid shared mutable state across commands/tests.
Applied to files:
cmd/config/config.gocmd/analyze.gopkg/profile/profile.gocmd/root.go
🧬 Code graph analysis (5)
cmd/config/config.go (3)
cmd/config/login.go (1)
NewLoginCmd(51-79)pkg/profile/profile.go (1)
Profiles(14-14)pkg/profile/application.go (2)
Application(4-8)Repository(14-16)
cmd/analyze.go (1)
pkg/profile/profile.go (5)
Profiles(14-14)FindSingleProfile(154-192)ProfileSettings(47-55)LabelSelector(38-41)SetSettingsFromProfile(74-120)
cmd/root.go (1)
cmd/config/config.go (1)
NewConfigCmd(46-85)
cmd/config/config_test.go (3)
cmd/config/config.go (2)
NewConfigCmd(46-85)NewSyncCmd(135-183)cmd/config/login.go (1)
LoginResponse(29-35)pkg/profile/application.go (2)
Application(4-8)Repository(14-16)
cmd/config/login.go (1)
cmd/settings.go (1)
Config(26-35)
🪛 ast-grep (0.40.0)
cmd/config/config.go
[warning] 236-236: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{InsecureSkipVerify: true}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
cmd/config/login.go
[warning] 135-135: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{InsecureSkipVerify: true}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
[warning] 188-188: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{InsecureSkipVerify: true}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build & test from commit
🔇 Additional comments (16)
go.mod (2)
11-11: Good upgrade to jwt/v5 addressing prior security concerns.The upgrade from
jwt/v4(which had CVE-2025-30204) tojwt/v5 v5.2.2as a direct dependency is the correct approach for the new authentication features being added in this PR.
22-22: LGTM - terminal dependency for secure password input.Adding
golang.org/x/termas a direct dependency is appropriate for the login command's password input functionality.pkg/profile/application.go (1)
1-16: Clean data structure definitions for Hub integration.The types are well-structured with appropriate JSON/YAML tags. Using a pointer for
Repositorycorrectly enables theomitemptybehavior for optional fields.pkg/profile/profile.go (3)
57-72: LGTM - UnmarshalProfile implementation is straightforward.The function correctly handles empty paths and properly returns errors from file reading and YAML parsing.
122-152: LGTM - Robust handling of rules directory.The function properly handles empty paths, non-existent directories, and correctly filters for subdirectories only.
154-192: LGTM - Clean profile discovery logic.The function handles edge cases well: non-existent directories, files instead of directories, multiple profiles, and missing
profile.yamlfiles. The design choice to return empty string (not error) for ambiguous cases is sensible.pkg/profile/profile_test.go (1)
1-12: LGTM - Test file imports are appropriate.All imports are utilized within the test file.
cmd/root.go (2)
15-15: LGTM - Config package import for new subcommand.
74-74: Clean registration of the config command.The config command is registered following the same pattern as other subcommands, maintaining consistency with the existing codebase.
cmd/analyze.go (3)
151-182: LGTM - Profile loading logic is well-structured.The profile handling correctly:
- Validates the profile directory exists and is a directory
- Loads
profile.yamlfrom the specified directory- Falls back to searching the default
.konveyor/profilespath when no--profile-diris provided- Applies settings only if a valid profile is found
The error at line 172 is logged but doesn't block execution, which allows analysis to proceed without a profile in the default path.
1621-1648: Clean separation of profile settings handling.The
createProfileSettingsandapplyProfileSettingshelper functions provide a clean interface between the analyze command and the profile package. The bidirectional mapping is consistent.
106-107: Verify validation behavior when--profile-diris provided without explicit--input.When
--profile-diris provided without--input, validation inPreRunE(line 124) may fail with empty input before the profile settings are applied inRunE. Ensure that theValidatemethod either:
- Allows empty input when
--profile-diris set, or- Defers input validation until after the profile is loaded and its input path is applied.
Confirm that this flow prevents validation errors for valid profile-based configurations.
cmd/config/config.go (1)
248-298: LGTM - Token refresh and retry logic is well-implemented.The
doRequestWithRetryfunction correctly:
- Checks token expiry before making requests
- Refreshes expired tokens proactively
- Retries on 401 Unauthorized with token refresh
- Uses
isRetryflag to prevent infinite retry loopscmd/config/config_test.go (3)
23-52: LGTM - Well-structured test suite.The test suite provides good coverage:
- Command initialization and flag verification
- Validation logic for various input scenarios
- Hub client behavior including TLS options
- HTTP response handling
- Tar/gzip extraction
- Integration test for end-to-end flow
870-888: Test codifies potentially problematic behavior.This test expects
expectedAppID: 0when the application URL doesn't match, but this behavior may cause downstream issues. If the implementation is updated to return an error instead (as suggested in config.go review), this test case should be updated to expect an error.
300-307: Test expects error message that may not be returned by implementation.The test at lines 300-307 expects error message "application path is required" for empty
applicationPath. However, according to the review comment,syncCommand.Validate()callssetDefaultApplicationPath()whenapplicationPathis empty (see config.go lines 186-192), which uses the current working directory as default instead of returning an error. This test may not fail as expected.
| if insecure { | ||
| tr := &http.Transport{ | ||
| TLSClientConfig: &tls.Config{InsecureSkipVerify: true}, | ||
| } | ||
| client.Transport = tr | ||
| } |
There was a problem hiding this comment.
Add minimum TLS version to strengthen cryptographic security.
The TLS configuration is missing MinVersion, which could allow downgrade attacks even when InsecureSkipVerify is enabled for testing.
Apply this diff:
if insecure {
tr := &http.Transport{
- TLSClientConfig: &tls.Config{InsecureSkipVerify: true},
+ TLSClientConfig: &tls.Config{
+ InsecureSkipVerify: true,
+ MinVersion: tls.VersionTLS12,
+ },
}
client.Transport = tr
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if insecure { | |
| tr := &http.Transport{ | |
| TLSClientConfig: &tls.Config{InsecureSkipVerify: true}, | |
| } | |
| client.Transport = tr | |
| } | |
| if insecure { | |
| tr := &http.Transport{ | |
| TLSClientConfig: &tls.Config{ | |
| InsecureSkipVerify: true, | |
| MinVersion: tls.VersionTLS12, | |
| }, | |
| } | |
| client.Transport = tr | |
| } |
🧰 Tools
🪛 ast-grep (0.40.0)
[warning] 236-236: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{InsecureSkipVerify: true}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
🤖 Prompt for AI Agents
In cmd/config/config.go around lines 235 to 240, the TLS config created when
insecure is true lacks a MinVersion setting; update the tls.Config to include a
minimum TLS version (e.g., set MinVersion to tls.VersionTLS12 or
tls.VersionTLS13 using the tls package constants) so even when
InsecureSkipVerify is enabled the connection refuses older TLS versions; modify
the TLSClientConfig to include MinVersion: tls.VersionTLS12.
Signed-off-by: Emily McMullan <[email protected]>
eemcmullan
force-pushed
the
central-config
branch
from
a04b98c to
77698ef
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (7)
cmd/config/login.go (3)
134-139: Add minimum TLS version to prevent protocol downgrade attacks.The TLS configuration sets
InsecureSkipVerifybut doesn't specifyMinVersion. Even for insecure/development scenarios, enforcing a minimum TLS version (e.g., TLS 1.2) helps prevent downgrade attacks.Apply this diff as suggested in the previous review:
if l.insecure { tr := &http.Transport{ - TLSClientConfig: &tls.Config{InsecureSkipVerify: true}, + TLSClientConfig: &tls.Config{ + InsecureSkipVerify: true, + MinVersion: tls.VersionTLS12, + }, } client.Transport = tr }
187-192: Add minimum TLS version to RefreshToken's TLS configuration.Same issue as in
performLogin- the TLS configuration is missingMinVersion.Apply this diff:
if l.insecure { tr := &http.Transport{ - TLSClientConfig: &tls.Config{InsecureSkipVerify: true}, + TLSClientConfig: &tls.Config{ + InsecureSkipVerify: true, + MinVersion: tls.VersionTLS12, + }, } client.Transport = tr }
206-210: Set Content-Type header for JSON request body in RefreshToken.The POST request at Line 206 sends a JSON body but only sets the
Acceptheader. AddContent-Type: application/jsonto ensure the server correctly interprets the request body.Apply this diff:
req, err := http.NewRequest("POST", refreshURL, bytes.NewBuffer(jsonData)) if err != nil { return nil, err } + req.Header.Set("Content-Type", "application/json") req.Header.Set("Accept", "application/json")pkg/profile/profile.go (1)
79-84: Panic risk: negative slice bound when path starts with.konveyor.If
pathis a relative path like.konveyor/profiles/foo/profile.yaml, thenkonveyorIndexis0, andpath[:konveyorIndex-1]attempts to slice to index-1, causing a runtime panic.Consider using
filepath.Dirchain instead of string manipulation:- konveyorIndex := strings.Index(path, ".konveyor") - if konveyorIndex == -1 { - return fmt.Errorf("profile path does not contain .konveyor directory: %s", path) - } - // get dir before .konveyor/ - locationDir := path[:konveyorIndex-1] + // Derive app root from profile path: .../app/.konveyor/profiles/<name>/profile.yaml + profDir := filepath.Dir(path) // .../.konveyor/profiles/<name> + profilesDir := filepath.Dir(profDir) // .../.konveyor/profiles + konveyorDir := filepath.Dir(profilesDir) // .../.konveyor + locationDir := filepath.Dir(konveyorDir) // app root + + // Validate that path contains .konveyor structure + if !strings.Contains(path, ".konveyor") { + return fmt.Errorf("profile path does not contain .konveyor directory: %s", path) + }cmd/config/config.go (3)
87-133: Fixlogr.Logger.Infousage inListProfiles
logr.Logger.Infois key/value based, notfmt-formatted. This call:c.log.Info("no profile directories found in: %s", profilesDir)will log the
%sliterally and treatprofilesDiras a key without value.Consider:
- if len(profileDirs) == 0 { - c.log.Info("no profile directories found in: %s", profilesDir) - return nil - } + if len(profileDirs) == 0 { + c.log.Info("no profile directories found", "path", profilesDir) + return nil + }This matches the structured logging style used elsewhere in the file.
223-246: AddMinVersionto TLS config even in insecure modeWhen
insecureis true, the TLS config setsInsecureSkipVerifybut leavesMinVersionat the Go default. Static analysis flagged this as allowing older TLS versions than necessary.To harden the client, set an explicit minimum TLS version (e.g. TLS 1.3, or 1.2 if you must support older Hub deployments):
- if insecure { - tr := &http.Transport{ - TLSClientConfig: &tls.Config{InsecureSkipVerify: true}, - } - client.Transport = tr - } + if insecure { + tr := &http.Transport{ + TLSClientConfig: &tls.Config{ + InsecureSkipVerify: true, + MinVersion: tls.VersionTLS13, // or tls.VersionTLS12 if needed + }, + } + client.Transport = tr + }This keeps the “insecure” behavior (no cert verification) while still refusing legacy protocol versions.
325-361: Avoid returning a zero-valuedApplicationon URL mismatchIn
getApplicationFromHub, when exactly one app is returned but itsRepository.URLdoes not match, the function returns a zero-valuedprofile.Applicationandnilerror:var application profile.Application if apps[0].Repository != nil && apps[0].Repository.URL == s.url { application = apps[0] } return application, nilCallers then see
ID == 0and proceed to/applications/0/analysis/profiles, which will typically fail with a less clear error.Instead, always either return a matching application or an explicit error:
- var application profile.Application - if apps[0].Repository != nil && apps[0].Repository.URL == s.url { - application = apps[0] - } - - return application, nil + app := apps[0] + if app.Repository == nil || app.Repository.URL != urlRepo { + return profile.Application{}, fmt.Errorf("no application matching repository URL: %s", urlRepo) + } + + return app, nilThis also makes use of the
urlRepoparameter and ensures callers never get a silently invalid application.
🧹 Nitpick comments (1)
pkg/profile/profile_test.go (1)
531-555: Permission test may be flaky when run as root.The "permission denied on rules directory" test creates a directory with mode
0000, but this test will pass (not error) when run as root since root can read any directory regardless of permissions. Consider skipping this test when running as root.{ name: "permission denied on rules directory", setupFunc: func() (string, func(), error) { + // Skip if running as root since root ignores permissions + if os.Getuid() == 0 { + return "", func() {}, fmt.Errorf("skip: running as root") + } tmpDir, err := os.MkdirTemp("", "test-profile-") if err != nil { return "", nil, err }And update the test runner to handle the skip:
if err != nil && strings.Contains(err.Error(), "skip:") { t.Skip(err.Error()) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
cmd/analyze.go(7 hunks)cmd/config/config.go(1 hunks)cmd/config/config_test.go(1 hunks)cmd/config/login.go(1 hunks)cmd/root.go(2 hunks)go.mod(2 hunks)pkg/profile/application.go(1 hunks)pkg/profile/profile.go(1 hunks)pkg/profile/profile_test.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- go.mod
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/root.go
- pkg/profile/application.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-08T15:06:52.056Z
Learnt from: abrugaro
Repo: konveyor/kantra PR: 524
File: cmd/dump-rules.go:0-0
Timestamp: 2025-08-08T15:06:52.056Z
Learning: In konveyor/kantra cmd/dump-rules.go (Go/Cobra), the dump-rules command should not use package-level variables for flags (output, overwrite). Flags must be defined as local variables inside NewDumpRulesCommand to avoid shared mutable state across commands/tests.
Applied to files:
cmd/config/config.gocmd/analyze.gopkg/profile/profile.go
🧬 Code graph analysis (3)
cmd/config/config_test.go (5)
cmd/config/config.go (2)
NewConfigCmd(46-85)NewSyncCmd(135-183)pkg/profile/profile.go (2)
Profiles(14-14)AnalysisProfile(16-22)cmd/config/login.go (1)
LoginResponse(29-35)pkg/profile/application.go (2)
Application(4-8)Repository(14-16)cmd/root.go (1)
Execute(79-94)
cmd/analyze.go (1)
pkg/profile/profile.go (5)
Profiles(14-14)FindSingleProfile(154-192)ProfileSettings(47-55)LabelSelector(38-41)SetSettingsFromProfile(74-120)
cmd/config/login.go (1)
cmd/settings.go (1)
Config(26-35)
🪛 ast-grep (0.40.0)
cmd/config/config.go
[warning] 236-236: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{InsecureSkipVerify: true}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
cmd/config/login.go
[warning] 135-135: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{InsecureSkipVerify: true}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
[warning] 188-188: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{InsecureSkipVerify: true}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build & test from commit
🔇 Additional comments (19)
cmd/config/login.go (1)
297-316: LGTM - Token expiration check implementation is correct.The
isTokenExpiredfunction correctly handles edge cases: empty tokens return expired, usesParseUnverifiedappropriately for client-side expiry checks, and properly handles theexpclaim. The use ofjwt.WithoutClaimsValidation()is appropriate here since we only need to read the expiry without validating signatures.pkg/profile/profile.go (3)
57-72: LGTM - UnmarshalProfile implementation is correct.The function properly handles empty paths, reads the file, and unmarshals YAML content. Error handling is appropriate.
122-152: LGTM - GetRulesInProfile implementation is robust.The function correctly handles empty paths, non-existent directories, validates that the rules path is a directory, and only returns subdirectories (not files).
154-192: LGTM - FindSingleProfile implementation handles edge cases well.The function gracefully handles non-existent directories, validates directory type, and correctly returns empty string (not error) when zero or multiple profiles are found. This allows callers to proceed without a profile when appropriate.
pkg/profile/profile_test.go (2)
1-270: LGTM - Test coverage for UnmarshalProfile is comprehensive.The tests cover key scenarios: empty path, valid profiles with various field combinations, empty files, and error cases like directory-instead-of-file. The test structure with setup/cleanup functions ensures proper resource management.
627-830: LGTM - FindSingleProfile tests cover important edge cases.Tests appropriately verify single profile detection, multiple profiles (returns empty), non-existent paths, file-instead-of-directory errors, and empty directories.
cmd/analyze.go (3)
1621-1648: LGTM - Profile settings helpers are well-structured.
createProfileSettingsproperly initializes the settings struct from current command state, andapplyProfileSettingscorrectly applies the profile-derived settings back to the command fields.
349-353: Validate signature updated correctly to accept cmd parameter.The
Validatemethod now accepts*cobra.Commandwhich enables profile-aware flag checking. This change integrates well with the profile system.
106-108: Unable to verify profile-dir interaction—repository access required.The verification could not be completed due to inability to access the repository containing the changes under review. Web search indicates that the current konveyor/kantra project does not expose a
--profile-dirflag, suggesting this may be a proposed addition in the PR under review.To properly verify the reviewer's concerns about the interaction between
--profile-dir,input, andoutputrequirements, direct access to the modified code is necessary to inspect:
- The
SetSettingsFromProfileimplementation and whether it setsoutput- The
ProfileSettingsstruct definition- The validation logic at line 466+
The reviewer's questions about expected behavior remain unresolved without examining the actual implementation.
cmd/config/config_test.go (5)
375-437: LGTM - getHubClient test properly manages auth file lifecycle.The test correctly backs up any existing auth file, creates test auth data with a valid (far-future) JWT, and restores the original file in cleanup. This prevents test pollution of user's actual credentials.
586-601: LGTM - Authorization header test uses correct standard header name.The test now correctly checks for
Authorizationheader withBearertoken format, aligning with the Tackle2 Hub API specification.
710-793: LGTM - parseApplicationsFromHub tests use proper JSON format.Test data correctly uses JSON format matching what
json.Unmarshalexpects. The variable naming and test cases are clear and comprehensive.
1156-1307: LGTM - Tar extraction tests cover both plain and gzipped formats.Tests properly verify extraction of regular tar files and gzipped tar files, plus error handling for non-existent files. The setup functions correctly create valid tar archives for testing.
1508-1539: LGTM - Integration test validates end-to-end command execution.The integration test properly sets up a temporary directory structure with profiles and executes the config command with
--list-profilesflag, verifying the full command flow works correctly.cmd/config/config.go (5)
46-85: Command wiring and local flag state look good
NewConfigCmdkeeps flag backing fields on a command-scoped struct (configCommand) instead of package-level globals, which avoids shared mutable state across commands/tests and aligns well with how other commands in this repo are being refactored.Based on learnings, this avoids the global-flag issue seen in earlier commands.
135-183: Optionally handleMarkFlagRequired("url")error
syncCommand.MarkFlagRequired("url")can fail (e.g., flag renamed), but the error is ignored, which could hide configuration bugs during development.If you want a bit more robustness without changing behavior for users, you could at least log failures:
- syncCommand.Flags().StringVar(&syncCmd.url, "url", "", "url of the remote application repository") - syncCommand.MarkFlagRequired("url") + syncCommand.Flags().StringVar(&syncCmd.url, "url", "", "url of the remote application repository") + if err := syncCommand.MarkFlagRequired("url"); err != nil { + log.Error(err, "failed to mark url flag as required") + }[ suggest_optional_refactor ]
252-323: Auth/refresh flow and HTTP error handling look consistentThe combination of
doRequestWithRetry,readResponseBody, andcheckAuthenticationcleanly:
- Applies the bearer token if present.
- Refreshes on token expiry or a single 401 retry.
- Returns HTTP errors with body text for context.
No issues spotted here from a flow/correctness perspective.
374-415: Profile listing and bundle download flow looks sound
getProfilesFromHubApplicationanddownloadProfileBundlefollow the same hubClient patterns as the application lookup, with clear error propagation and directory preparation before download. Nothing blocking here.
451-532: Tar extraction code handles gzip/uncompressed and path traversal safely
extractTarFilecorrectly:
- Detects gzip vs plain tar by magic bytes.
- Ensures all extracted paths stay under
destDirusing a prefix check.- Creates intermediate directories and applies file modes.
Combined with
deleteTarFile, this gives a solid and safe bundle extraction path for profile archives.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
pkg/profile/profile_test.go (3)
242-267: Consider verifying slice contents, not just lengths.The current assertions only check slice lengths (e.g.,
len(got.Scope.Packages.Included)), which could miss bugs where slices have the correct length but contain wrong values. Consider adding value comparisons:if len(got.Scope.Packages.Included) != len(tt.wantProfile.Scope.Packages.Included) { t.Errorf("UnmarshalProfile() Scope.Packages.Included length = %v, want %v", len(got.Scope.Packages.Included), len(tt.wantProfile.Scope.Packages.Included)) +} else { + for i := range got.Scope.Packages.Included { + if got.Scope.Packages.Included[i] != tt.wantProfile.Scope.Packages.Included[i] { + t.Errorf("UnmarshalProfile() Scope.Packages.Included[%d] = %v, want %v", i, got.Scope.Packages.Included[i], tt.wantProfile.Scope.Packages.Included[i]) + } + } }Alternatively, use a reflection-based deep comparison library like
cmp.Difffromgithub.com/google/go-cmp/cmpfor cleaner assertions.
590-622: Refactor test validation to avoid string matching on test names.The validation logic uses string comparison on test names to determine how to validate results. This is fragile—renaming a test breaks the validation logic.
Consider adding a validation function field to the test struct:
tests := []struct { name string setupFunc func() (string, func(), error) wantRules []string wantErr bool profileDir string errMsg string + validate func(got []string, t *testing.T) }{Then replace the name-based conditionals with:
-if tt.name == "rules directory with mixed files and directories" { - if len(got) != 2 { - // validation logic - } -} else if tt.name == "profile with rules" { - // more validation logic -} else if tt.wantRules == nil && got != nil { +if tt.validate != nil { + tt.validate(got, t) +} else if tt.wantRules == nil && got != nil {
1245-1275: Permission-based error tests may be platform-dependent.The test removes read permissions from directories to trigger permission errors. This approach works on Unix-like systems but may behave differently on Windows where permission models differ. Consider adding a build tag or skip condition for platforms where this test pattern is unreliable:
if runtime.GOOS == "windows" { t.Skip("Permission-based tests not reliable on Windows") }cmd/config/config_test.go (2)
54-156: Nice coverage of filesystem validation; consider avoiding brittle string matches on OS errorsThe validation and list‑profiles tests exercise a good matrix of cases: empty values, valid dirs, missing subdirs, non‑existent paths, and “file vs directory” confusion, and similar checks in the tar/delete tests. That’s valuable coverage.
The only nit is relying on substrings like
"no such file or directory"/"is not a directory"in assertions. These come fromoserror messages and can vary between platforms or Go versions, making tests a bit brittle.If you want to future‑proof, consider using predicates instead of string containment for these cases (e.g.,
os.IsNotExist(err)or checking the sentinel error type where applicable), and then just assert that an error occurred.Also applies to: 158-266, 291-373, 1275-1277, 1344-1346
439-504:TestNewHubClientWithOptionsdepends on real stored auth and often skips; consider seeding test auth instead
TestNewHubClientWithOptionscallsnewHubClientWithOptionsand skips when the error indicates “no stored authentication found” or “stored authentication is invalid”. In a clean CI or dev environment this likely means the test never truly exercises the code under test.Given you already have patterns elsewhere for writing a
LoginResponseinto auth storage, it would be more deterministic to:
- Point auth loading at a temp HOME/config dir (as suggested in the other comment), and
- Seed that location with a known‑good
LoginResponsebefore callingnewHubClientWithOptions.That way the test will consistently validate host population, HTTP client init, timeout, and TLS settings without relying on a developer or CI job having logged in beforehand.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/config/config_test.go(1 hunks)pkg/profile/profile_test.go(1 hunks)
🧰 Additional context used
🪛 Gitleaks (8.30.0)
cmd/config/config_test.go
[high] 394-394: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.
(jwt)
[high] 531-531: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.
(jwt)
[high] 588-588: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.
(jwt)
[high] 816-816: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.
(jwt)
[high] 955-955: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.
(jwt)
[high] 1399-1399: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.
(jwt)
[high] 1588-1588: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.
(jwt)
[high] 1681-1681: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.
(jwt)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build & test from commit
🔇 Additional comments (3)
pkg/profile/profile_test.go (1)
1-1398: Comprehensive test coverage - well done!This test file provides excellent coverage of the profile package functionality with:
- Table-driven tests for maintainability
- Proper cleanup using deferred functions
- Edge cases including empty inputs, invalid data, and permission errors
- Clear test names describing scenarios
The test structure follows Go testing best practices and provides good confidence in the profile package implementation.
cmd/config/config_test.go (2)
23-52: Config/sync command wiring and basic integration tests look goodThe tests for
NewConfigCmd,NewSyncCmd, and the basicconfigintegration plussetDefaultApplicationPathcover the expectedUse/Shortstrings, flag wiring, subcommands, and a simple execution path without over-specifying behavior. This is a solid, maintainable surface-level check for the new CLI entry points.Also applies to: 268-289, 1508-1540, 1542-1559
642-708: Hub/profile/tar/download/refresh tests are thorough and balancedThe remaining tests around
readResponseBody, application/profile parsing, profile retrieval, tar download/extraction/deletion, profile bundle download, authentication checks, retry behavior, and token refresh cover both happy paths and key error scenarios (HTTP 4xx/5xx, empty data, missing profiles, missing apps).They collectively give good confidence in the new Hub/profile sync flow without over‑mocking internals. No changes needed here from a test-logic perspective.
Also applies to: 710-793, 795-932, 934-1065, 1067-1154, 1156-1307, 1309-1376, 1378-1506, 1561-1658, 1660-1760, 1762-1831
eemcmullan
force-pushed
the
central-config
branch
from
d729d7c to
5e10870
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
cmd/config/config_test.go (1)
401-442: Tests still manipulate real~/.kantra/auth.json; use isolated temp HOME instead.Multiple tests (this one and those at lines 532-576, 818-861, 957-1000, 1401-1444, 1584-1656, 1683-1725) backup/restore the real user's auth file. This pattern is fragile:
- Interrupted tests can leave auth corrupted
- Tests cannot run in parallel
- Behavior depends on developer's existing auth state
As flagged in a past review, use a throwaway HOME directory:
func setupTestAuth(t *testing.T) (cleanup func()) { homeDir := t.TempDir() t.Setenv("HOME", homeDir) kantreDir := filepath.Join(homeDir, ".kantra") os.MkdirAll(kantreDir, 0755) validJWT := generateMockJWT(time.Now().Add(1 * time.Hour)) testAuth := LoginResponse{ Host: "http://test-host", Token: validJWT, RefreshToken: "test-refresh-token", } authData, _ := json.Marshal(testAuth) os.WriteFile(filepath.Join(kantreDir, "auth.json"), authData, 0600) return func() {} // t.TempDir handles cleanup }
🧹 Nitpick comments (2)
pkg/profile/profile_test.go (2)
531-555: Permission-denied tests may fail when running as root.Tests using
os.Chmod(dir, 0000)to trigger permission errors will not work when the test runs as root (common in CI Docker containers), since root bypasses permission checks.Consider skipping these tests when running as root:
{ name: "permission denied on rules directory", setupFunc: func() (string, func(), error) { + if os.Getuid() == 0 { + return "", func() {}, nil // Will be skipped + } tmpDir, err := os.MkdirTemp("", "test-profile-") // ... }, wantRules: nil, wantErr: true, errMsg: "permission denied", },And in the test runner:
if tt.name == "permission denied on rules directory" && os.Getuid() == 0 { t.Skip("Skipping permission test when running as root") }Also applies to: 1244-1276, 1314-1344, 1346-1368
590-622: Consider simplifying validation logic with a helper function.The inline validation with case-specific checks (
tt.name == "rules directory with mixed...") is harder to maintain. Each test case could define its own validation function similar toTestSetSettingsFromProfile.This is a minor readability improvement that could be deferred:
// In test struct validate func([]string, *testing.T) // In test case { name: "profile with rules", // ... validate: func(got []string, t *testing.T) { if len(got) != 2 { t.Errorf("expected 2 rules, got %d", len(got)) } // ... }, }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/config/config_test.go(1 hunks)pkg/profile/profile_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
cmd/config/config_test.go (5)
cmd/config/config.go (2)
NewConfigCmd(46-85)NewSyncCmd(135-183)pkg/profile/profile.go (2)
Profiles(14-14)AnalysisProfile(16-22)cmd/config/login.go (1)
LoginResponse(29-35)pkg/profile/application.go (2)
Application(4-8)Repository(14-16)cmd/root.go (1)
Execute(79-94)
🪛 Gitleaks (8.30.0)
cmd/config/config_test.go
[high] 611-611: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.
(jwt)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build & test from commit
🔇 Additional comments (8)
cmd/config/config_test.go (2)
24-47: Good approach using dynamically generated mock JWTs.The
generateMockJWThelper properly creates structurally valid JWTs with configurable expiration for testing, avoiding hardcoded tokens that would trigger secret scanners repeatedly.
1179-1241: LGTM: Thorough tar extraction tests with good coverage.Tests cover plain tar, gzipped tar, and non-existent file scenarios. The setup functions properly create valid tar archives with correct headers.
pkg/profile/profile_test.go (6)
1-12: LGTM: Clean imports appropriate for test requirements.All imports are used:
osandfilepathfor file operations,stringsfor error message checks,logrfor mock logger,providerfor mode constants, andcobrafor command mocking.
14-270: LGTM: Comprehensive unmarshalling tests with good edge case coverage.Tests cover empty paths, valid profiles, profiles with all fields, empty files, partial profiles, and directory-as-file errors. The field-by-field assertions ensure proper deserialization.
272-388: LGTM: Solid integration tests for profile settings application.Tests properly verify that profile data populates
ProfileSettingscorrectly, including mode derivation, library analysis flags, and label selectors. The use of mock Cobra commands with proper flag setup is appropriate.
627-830: LGTM: Comprehensive profile discovery tests.Good coverage of single profile, multiple profiles, missing directories, and various edge cases. The validation correctly checks for the expected path suffix.
1095-1105: LGTM: Mock struct appropriately mirrors production type for testing.The
mockAnalyzeCommandprovides the minimal fields needed to verify settings application without coupling to the realanalyzeCommandimplementation.
1109-1235: LGTM: Good error handling coverage.Tests verify that invalid YAML files and rules-as-file scenarios properly propagate errors with expected messages.
eemcmullan
force-pushed
the
central-config
branch
from
5e10870 to
225a300
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
docs/profiles.md (2)
209-209: Clarify--profileflag: specify profile directory, not parent.konveyor/profiles.This example still shows passing the parent
.konveyor/profilesdirectory, but past review comments indicate that--profileexpects a path to a specific profile directory containingprofile.yaml(e.g.,myapp/.konveyor/profiles/profile-1), not the parent root.Either update the example to use a specific profile subdirectory or adjust the implementation to auto-discover profiles when given the parent directory.
292-292: Add--inputflag or document input derivation from profile.This example omits the
--inputflag. If the analyze command requires an explicit input path even when using a profile, this command will fail. Either:
- Add
--input(or-i .) to the example, or- Document that the input is automatically derived from the profile location.
cmd/config/config.go (2)
124-124: Fix logr usage: use key-value pairs instead of printf-style format.
logr.Logger.Infodoes not support printf-style format strings. This call will log%sliterally and treatprofilesDiras a key without a value.Use key-value pairs:
-c.log.Info("no profile directories found in: %s", profilesDir) +c.log.Info("no profile directories found", "path", profilesDir)
235-240: Add minimum TLS version to prevent downgrade attacks.The TLS configuration lacks
MinVersion, which could allow protocol downgrade attacks even in insecure mode.Apply this diff:
if insecure { tr := &http.Transport{ - TLSClientConfig: &tls.Config{InsecureSkipVerify: true}, + TLSClientConfig: &tls.Config{ + InsecureSkipVerify: true, + MinVersion: tls.VersionTLS12, + }, } client.Transport = tr }
🧹 Nitpick comments (1)
docs/profiles.md (1)
254-258: Add language identifiers to fenced code blocks.Several fenced code blocks (prompts, error messages) lack language identifiers, causing markdownlint violations (MD040). Mark these blocks with a language identifier such as
textto satisfy linting tools.Apply this pattern to each unlabeled block:
-``` +```text Host: https://hub.myapp.com Username: myusername Password: [hidden]Also applies to: 307-309, 315-317, 322-324, 331-333, 339-341, 346-348 </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: CodeRabbit UI **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 5e1087052dd60c10dffcb70d585d18b5cb244e7f and 225a30064a0212ddc75a7cf57c876ff7d91a7e20. </details> <details> <summary>📒 Files selected for processing (4)</summary> * `cmd/config/config.go` (1 hunks) * `cmd/config/config_test.go` (1 hunks) * `docs/profiles.md` (1 hunks) * `pkg/profile/profile_test.go` (1 hunks) </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (1)</summary> * pkg/profile/profile_test.go </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🧠 Learnings (1)</summary> <details> <summary>📚 Learning: 2025-08-08T15:06:52.056Z</summary>Learnt from: abrugaro
Repo: konveyor/kantra PR: 524
File: cmd/dump-rules.go:0-0
Timestamp: 2025-08-08T15:06:52.056Z
Learning: In konveyor/kantra cmd/dump-rules.go (Go/Cobra), the dump-rules command should not use package-level variables for flags (output, overwrite). Flags must be defined as local variables inside NewDumpRulesCommand to avoid shared mutable state across commands/tests.**Applied to files:** - `cmd/config/config.go` </details> </details><details> <summary>🧬 Code graph analysis (2)</summary> <details> <summary>cmd/config/config.go (2)</summary><blockquote> <details> <summary>pkg/profile/profile.go (1)</summary> * `Profiles` (14-14) </details> <details> <summary>pkg/profile/application.go (2)</summary> * `Application` (4-8) * `Repository` (14-16) </details> </blockquote></details> <details> <summary>cmd/config/config_test.go (4)</summary><blockquote> <details> <summary>cmd/config/login.go (1)</summary> * `LoginResponse` (29-35) </details> <details> <summary>cmd/config/config.go (2)</summary> * `NewConfigCmd` (46-85) * `NewSyncCmd` (135-183) </details> <details> <summary>pkg/profile/profile.go (1)</summary> * `Profiles` (14-14) </details> <details> <summary>pkg/profile/application.go (2)</summary> * `Application` (4-8) * `Repository` (14-16) </details> </blockquote></details> </details><details> <summary>🪛 ast-grep (0.40.0)</summary> <details> <summary>cmd/config/config.go</summary> [warning] 236-236: MinVersion` is missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. Add `MinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3. Context: tls.Config{InsecureSkipVerify: true} Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES] https://owasp.org/Top10/A02_2021-Cryptographic_Failures (missing-ssl-minversion-go) </details> </details> <details> <summary>🪛 Gitleaks (8.30.0)</summary> <details> <summary>cmd/config/config_test.go</summary> [high] 577-577: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data. (jwt) </details> </details> <details> <summary>🪛 markdownlint-cli2 (0.18.1)</summary> <details> <summary>docs/profiles.md</summary> 189-189: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 254-254: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 307-307: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 315-315: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 322-322: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 331-331: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 339-339: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 346-346: Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> </details> <details> <summary>⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)</summary> * GitHub Check: Build & test from commit </details> <details> <summary>🔇 Additional comments (2)</summary><blockquote> <details> <summary>cmd/config/config_test.go (1)</summary><blockquote> `1-1662`: **Comprehensive test coverage with proper isolation.** The test suite is well-structured and thorough: - Uses `setupTempAuth` helper to isolate HOME environment and avoid touching real auth files - Covers all command surfaces: config, sync, hub client, authentication flows - Tests both happy paths and error scenarios - Includes integration tests for end-to-end workflows Past review issues (JWT handling, JSON vs YAML, error message expectations, auth isolation) appear to have been addressed in previous commits. </blockquote></details> <details> <summary>cmd/config/config.go (1)</summary><blockquote> `1-534`: **Well-structured implementation with proper error handling.** The config command implementation demonstrates good practices: - Lazy initialization of hub client with proper caching - Token management with automatic refresh on expiry or 401 responses - Path traversal protection in tar extraction (line 494) - Proper file permissions from tar headers - Clean function decomposition for testability Two unresolved issues from past reviews remain: - Line 124: Incorrect logr usage (flagged separately) - Lines 235-240: Missing TLS MinVersion (flagged separately) </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
docs/profiles.md
Outdated
| For example: `.konveyor/profiles/profile-1/profile.yaml`. | ||
| If a single profile is found here, it will be used by default for analysis configuration | ||
| options. You can also use the `kantra analyze --profile` option to pass in a valid | ||
| profile path. |
There was a problem hiding this comment.
👍 on documenting profile directories.
eemcmullan
force-pushed
the
central-config
branch
from
225a300 to
704a6ac
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
docs/profiles.md (2)
189-199: Add languages to fenced code blocks to satisfy markdownlint (MD040).Several fences lack a language (directory tree, interactive prompt, and error-message examples). For example:
- Directory structure under “Directory Structure”
- Hub prompt snippet (
Host/Username/Password)- Error examples in Troubleshooting
Consider marking them as
text:```text my-application/ ├── src/ ...Host: https://hub.myapp.com Username: myusername Password: [hidden]Error: failed to stat profile at path /path/to/profilesThis will clear MD040 and make the intent clearer. Also applies to: 254-258, 307-348 --- `95-115`: **Align config/analyze flag docs with the current CLI (`list` subcommand, `--profile-dir`).** The docs still describe: - `kantra config --list-profiles <path>` as a flag on the main `config` command, and - `kantra analyze --profile` as the way to pass a profile, but the implementation now exposes: - a `config list` subcommand with a `--profile-dir` flag (application directory), and - profile integration wired via profile directories (and `FindSingleProfile`) rather than a `--profile` flag. To avoid confusing users: - Update “Main Config Command” to describe the `login`, `sync`, and `list` subcommands instead of a `--list-profiles` flag, and show examples like `kantra config list --profile-dir /path/to/app`. - Replace the reference to ``kantra analyze --profile`` with the actual flag name (`--profile-dir`) and be explicit that it expects an application directory containing `.konveyor/profiles` (or that it will auto-discover a single profile under `.konveyor/profiles/`). This also addresses the earlier ambiguity about whether you pass the `.konveyor/profiles` root vs a specific profile subdirectory. Also applies to: 180-212 </blockquote></details> <details> <summary>cmd/config/config.go (2)</summary><blockquote> `145-160`: **Fix logr usage: avoid printf-style format string in `ListProfiles`.** `logr.Logger.Info` ignores `fmt` formatting; this call: ```go l.log.Info("no profile directories found in: %s", profilesDir)will log the
%sliterally and treatprofilesDiras a key without a value. Pass the path as structured key/value instead:- if len(profileDirs) == 0 { - l.log.Info("no profile directories found in: %s", profilesDir) - return nil - } + if len(profileDirs) == 0 { + l.log.Info("no profile directories found", "path", profilesDir) + return nil + }
257-275: Set a minimum TLS version in the insecure TLS configuration.The current code uses
InsecureSkipVerify: truewithout settingMinVersion, relying on Go's defaults. Go's best practices recommend explicitly settingMinVersiontotls.VersionTLS12(or higher) even whenInsecureSkipVerifyis necessary for test/self-signed environments.if insecure { tr := &http.Transport{ - TLSClientConfig: &tls.Config{InsecureSkipVerify: true}, + TLSClientConfig: &tls.Config{ + InsecureSkipVerify: true, + MinVersion: tls.VersionTLS12, + }, } client.Transport = tr }This prevents negotiation of obsolete TLS versions while preserving the ability to accept self-signed certificates in development.
cmd/config/config_test.go (1)
568-581: Fix auth-header test so it actually validates the Authorization header and drop the hard-coded JWT literal.In
TestHubClient_doRequest, case"check authentication header with token":
setupTempAuthwrites a dynamically generated JWT fromgenerateMockJWT(...)intoauth.json, sodoRequestwill sendAuthorization: Bearer <dynamic-token>.- The test server compares against a hard-coded
expectedAuthJWT string that will never equal the dynamic token, so it always returns400 Bad Request.- The test still passes because
doRequestdoesn’t treat 4xx as an error and the test never asserts on the status code.This both weakens the test (it doesn’t actually confirm the header) and reintroduces a JWT-looking literal that static analysis flags.
Consider rewriting this case to validate the header format instead of a specific token and to assert the 2xx status, e.g.:
{ name: "check authentication header with token", serverResponse: func(w http.ResponseWriter, r *http.Request) { - auth := r.Header.Get("Authorization") - expectedAuth := "Bearer eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJleHAiOjE4OTM0NTYwMDB9.Hs_ZQhwq7Uy9E7VzTpSKNqvWUdKKYKxWJhUlNhqJGKE" - if auth == expectedAuth { - w.WriteHeader(http.StatusOK) - w.Write([]byte(`{"authenticated": true}`)) - } else { - // Return a different error code to avoid triggering token refresh - w.WriteHeader(http.StatusBadRequest) - w.Write([]byte("Bad Request")) - } + auth := r.Header.Get("Authorization") + if strings.HasPrefix(auth, "Bearer ") && len(auth) > len("Bearer ") { + w.WriteHeader(http.StatusOK) + w.Write([]byte(`{"authenticated": true}`)) + } else { + w.WriteHeader(http.StatusUnauthorized) + w.Write([]byte("Missing or invalid Authorization header")) + } }, path: "/auth", acceptHeader: "application/json", wantErr: false, },and then, after
doRequest, assert thatresp.StatusCode == http.StatusOKfor this case. That way the test genuinely verifies that a non-empty Bearer token is being sent, and the static JWT constant can be removed.Also applies to: 611-628
🧹 Nitpick comments (3)
pkg/profile/profile_test.go (1)
880-910: Consider avoiding hard-coded OS error message substrings in tests.Several cases assert that errors contain strings like
"no such file or directory"or"permission denied". These messages can vary across platforms and filesystems, making tests brittle outside a Linux-like environment.Where practical, prefer
errors.Isor helpers such asos.IsNotExist/os.IsPermission, or compare againstos.ErrNotExist.Error()instead of literal substrings. This keeps the tests portable while still verifying the right failure mode.Also applies to: 1151-1202, 1237-1275, 1308-1368
cmd/config/config_test.go (2)
1125-1237: Reduce dependence on literal filesystem error messages in helper tests.Tests like
TestExtractTarFile(“non-existent tar file should fail”) andTestDeleteTarFile(“delete non-existent file should fail”) assert thaterr.Error()contains"no such file or directory". As with the profile tests, these strings can vary across OSes.If you want these helpers to be portable, consider using
errors.Is(err, os.ErrNotExist)or checkingos.IsNotExist(err)instead of matching the platform-specific text.Also applies to: 1269-1306
1470-1500: Avoid touching real.konveyor/profilesunder the current working directory in integration tests.In the second subtest of
TestConfigCommandIntegration, the test:
- Uses
os.Getwd()ascurrentDir.- Creates
profilesDir := filepath.Join(currentDir, profile.Profiles)and thendefer os.RemoveAll(profilesDir).If someone runs
go testfrom a directory that already has a real.konveyor/profilestree, this will delete it. To keep tests hermetic and side-effect free, it would be safer to run this integration scenario entirely under a temp directory (similar to the first subtest), e.g., bychdiring intot.TempDir()for the duration or by pointing--profile-dirat a temp app root.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cmd/config/config.go(1 hunks)cmd/config/config_test.go(1 hunks)docs/profiles.md(1 hunks)pkg/profile/profile_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-08T15:06:52.056Z
Learnt from: abrugaro
Repo: konveyor/kantra PR: 524
File: cmd/dump-rules.go:0-0
Timestamp: 2025-08-08T15:06:52.056Z
Learning: In konveyor/kantra cmd/dump-rules.go (Go/Cobra), the dump-rules command should not use package-level variables for flags (output, overwrite). Flags must be defined as local variables inside NewDumpRulesCommand to avoid shared mutable state across commands/tests.
Applied to files:
cmd/config/config.go
🧬 Code graph analysis (3)
cmd/config/config.go (3)
cmd/config/login.go (1)
NewLoginCmd(51-79)pkg/profile/profile.go (2)
Profiles(14-14)AnalysisProfile(16-22)pkg/profile/application.go (2)
Application(4-8)Repository(14-16)
pkg/profile/profile_test.go (1)
pkg/profile/profile.go (11)
AnalysisProfile(16-22)AnalysisMode(24-26)AnalysisScope(33-36)PackageSelector(28-31)AnalysisRules(43-45)LabelSelector(38-41)UnmarshalProfile(57-72)ProfileSettings(47-55)SetSettingsFromProfile(74-120)GetRulesInProfile(122-152)FindSingleProfile(154-192)
cmd/config/config_test.go (4)
cmd/config/login.go (1)
LoginResponse(29-35)cmd/config/config.go (3)
NewConfigCmd(51-82)NewListCmd(88-119)NewSyncCmd(169-217)pkg/profile/profile.go (2)
Profiles(14-14)AnalysisProfile(16-22)pkg/profile/application.go (2)
Application(4-8)Repository(14-16)
🪛 ast-grep (0.40.0)
cmd/config/config.go
[warning] 270-270: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{InsecureSkipVerify: true}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
🪛 Gitleaks (8.30.0)
cmd/config/config_test.go
[high] 615-615: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.
(jwt)
🪛 markdownlint-cli2 (0.18.1)
docs/profiles.md
189-189: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
254-254: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
307-307: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
315-315: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
322-322: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
331-331: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
339-339: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
346-346: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build & test from commit
🔇 Additional comments (1)
pkg/profile/profile_test.go (1)
14-388: Nice thorough coverage of profile parsing and settings behavior.The table-driven tests around
UnmarshalProfileandSetSettingsFromProfileexercise a wide range of scenarios (empty path, partial fields, invalid YAML, missing.konveyor, rules discovery), which should make regressions in profile handling much easier to catch.
| return profile.Application{}, err | ||
| } | ||
|
|
||
| apps, err := parseApplicationsFromHub(string(body)) |
There was a problem hiding this comment.
we probably don't need to convert to string to convert back to []byte
shawn-hurley
left a comment
shawn-hurley
left a comment
There was a problem hiding this comment.
VISACK mostly looks right to me, small nit to change if you want
Signed-off-by: Emily McMullan <[email protected]>
eemcmullan
force-pushed
the
central-config
branch
from
704a6ac to
8bb195e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
cmd/config/login.go (3)
134-139: Add minimum TLS version to prevent downgrade attacks.The TLS configuration is missing
MinVersion, which allows connections to downgrade to TLS 1.0 or 1.1 even whenInsecureSkipVerifyis set for testing purposes.Apply this diff:
if l.insecure { tr := &http.Transport{ - TLSClientConfig: &tls.Config{InsecureSkipVerify: true}, + TLSClientConfig: &tls.Config{ + InsecureSkipVerify: true, + MinVersion: tls.VersionTLS12, + }, } client.Transport = tr }Based on static analysis hints.
191-196: Add minimum TLS version to prevent downgrade attacks.Same TLS configuration issue as
performLogin- missingMinVersionsetting.Apply this diff:
if l.insecure { tr := &http.Transport{ - TLSClientConfig: &tls.Config{InsecureSkipVerify: true}, + TLSClientConfig: &tls.Config{ + InsecureSkipVerify: true, + MinVersion: tls.VersionTLS12, + }, } client.Transport = tr }Based on static analysis hints.
214-219: Set Content-Type header for JSON request body.The refresh request includes a JSON body (marshaled at line 209) but only sets the
Acceptheader, notContent-Type. The Hub API may requireContent-Type: application/json.Apply this diff:
req, err := http.NewRequest("POST", refreshURL, bytes.NewBuffer(jsonData)) if err != nil { return nil, err } + req.Header.Set("Content-Type", "application/json") req.Header.Set("Accept", "application/json")cmd/config/config.go (3)
158-158: Fix logr usage - avoid printf-style formatting.
logr.Logger.Infodoesn't support printf-style format strings. This will log%sliterally and treatprofilesDiras a key without a value.Apply this diff:
- l.log.Info("no profile directories found in: %s", profilesDir) + l.log.Info("no profile directories found", "path", profilesDir)
269-274: Add minimum TLS version to prevent downgrade attacks.The TLS configuration is missing
MinVersion, which could allow downgrade attacks even whenInsecureSkipVerifyis enabled for testing.Apply this diff:
if insecure { tr := &http.Transport{ - TLSClientConfig: &tls.Config{InsecureSkipVerify: true}, + TLSClientConfig: &tls.Config{ + InsecureSkipVerify: true, + MinVersion: tls.VersionTLS12, + }, } client.Transport = tr }Based on static analysis hints.
389-394: Fix nil-pointer dereference risk in URL mismatch error.The
elsebranch at line 393 attempts to accessapps[0].Repository.URLeven whenRepositoryisnil. The condition on line 390 short-circuits with&&, so ifRepositoryisnil, the else branch executes and tries to dereference the nil pointer in the error message.Apply this diff:
var application profile.Application - if apps[0].Repository != nil && apps[0].Repository.URL == s.url { + app := apps[0] + if app.Repository == nil { + return profile.Application{}, fmt.Errorf("application from Hub has no repository for URL: %s", s.url) + } + if app.Repository.URL != s.url { + return profile.Application{}, fmt.Errorf("URL mismatch: expected %s, got %s", s.url, app.Repository.URL) + } + - application = apps[0] - } else { - return profile.Application{}, fmt.Errorf("URL mismatch: expected %s, got %s", s.url, apps[0].Repository.URL) - } - - return application, nil + return app, nil
🧹 Nitpick comments (1)
docs/profiles.md (1)
189-189: Add language specifiers to fenced code blocks.Multiple fenced code blocks are missing language identifiers, which triggers markdownlint warnings. Add
textas the language specifier for these error message and prompt examples.Apply this pattern to all flagged blocks:
-``` +```text Error: failed to stat profile at path /path/to/profiles ```This affects lines 189, 254, 307, 315, 322, 331, 339, and 346.
Based on static analysis hints.
Also applies to: 254-254, 307-307, 315-315, 322-322, 331-331, 339-339, 346-346
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
cmd/config/config.go(1 hunks)cmd/config/config_test.go(1 hunks)cmd/config/login.go(1 hunks)docs/profiles.md(1 hunks)pkg/profile/profile_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-08T15:06:52.056Z
Learnt from: abrugaro
Repo: konveyor/kantra PR: 524
File: cmd/dump-rules.go:0-0
Timestamp: 2025-08-08T15:06:52.056Z
Learning: In konveyor/kantra cmd/dump-rules.go (Go/Cobra), the dump-rules command should not use package-level variables for flags (output, overwrite). Flags must be defined as local variables inside NewDumpRulesCommand to avoid shared mutable state across commands/tests.
Applied to files:
cmd/config/config.go
🧬 Code graph analysis (3)
cmd/config/config.go (4)
cmd/config/login.go (1)
NewLoginCmd(51-79)pkg/profile/profile.go (2)
Profiles(14-14)AnalysisProfile(16-22)cmd/settings.go (1)
Config(26-35)pkg/profile/application.go (2)
Application(4-8)Repository(14-16)
pkg/profile/profile_test.go (1)
pkg/profile/profile.go (8)
AnalysisProfile(16-22)AnalysisMode(24-26)LabelSelector(38-41)UnmarshalProfile(57-72)ProfileSettings(47-55)SetSettingsFromProfile(74-120)GetRulesInProfile(122-152)FindSingleProfile(154-192)
cmd/config/config_test.go (5)
cmd/config/login.go (1)
LoginResponse(29-35)cmd/config/config.go (3)
NewConfigCmd(51-82)NewListCmd(88-119)NewSyncCmd(169-217)pkg/profile/profile.go (2)
Profiles(14-14)AnalysisProfile(16-22)pkg/profile/application.go (2)
Application(4-8)Repository(14-16)cmd/root.go (1)
Execute(79-94)
🪛 ast-grep (0.40.0)
cmd/config/config.go
[warning] 270-270: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{InsecureSkipVerify: true}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
cmd/config/login.go
[warning] 135-135: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{InsecureSkipVerify: true}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
[warning] 192-192: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{InsecureSkipVerify: true}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
🪛 Gitleaks (8.30.0)
cmd/config/config_test.go
[high] 615-615: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.
(jwt)
🪛 markdownlint-cli2 (0.18.1)
docs/profiles.md
189-189: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
254-254: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
307-307: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
315-315: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
322-322: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
331-331: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
339-339: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
346-346: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build & test from commit
🔇 Additional comments (2)
pkg/profile/profile_test.go (1)
1-1398: LGTM - Comprehensive test coverage.The test suite provides excellent coverage of the profile package functionality with well-structured table-driven tests, appropriate edge cases, and good error path validation.
cmd/config/config_test.go (1)
1-1731: LGTM - Excellent test coverage with proper isolation.The test suite provides comprehensive coverage with well-isolated tests using temporary HOME directories. The JWT token flagged by static analysis at line 615 is a synthetic test token and is a false positive.
3 participants
Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.