Conversation
idk if this works yet, will have to change some things. this is just the foundation for oauth.
so that it can work on both environments
it only prints the auth token for now, but ideally we would want to store this auth token inside the OS keyring which is secure. the idea is not set in stone yet, we may use something other than the keyring
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ✏️ Tip: You can disable in-progress messages and the fortune message in your review settings. Tip You can disable the changed files summary in the walkthrough.Disable the ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Comment |
|
Note Docstrings generation - SUCCESS |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
cmd/server/.env.example (1)
1-8:⚠️ Potential issue | 🟡 MinorFix dotenv-linter ordering and trailing newline warnings.
Reordering keys and adding a final blank line will clear the current linter warnings.🧹 Suggested reorder + trailing newline
DB_HOST= -DB_USER= -DB_PASSWORD= DB_NAME= DB_PORT= -PORT= +DB_USER= +DB_PASSWORD= +GOOGLE_CLIENT_ID= +GOOGLE_CLIENT_SECRET= +PORT= -GOOGLE_CLIENT_ID= -GOOGLE_CLIENT_SECRET= +🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/server/.env.example` around lines 1 - 8, The .env.example file has dotenv-linter warnings for key ordering and missing trailing newline; reorder the environment keys (DB_HOST, DB_NAME, DB_PASSWORD, DB_PORT, DB_USER, GOOGLE_CLIENT_ID, GOOGLE_CLIENT_SECRET, PORT) into the linter-expected order (e.g., alphabetical or project convention) and add a final blank line at the end of the file so the file ends with a newline.internal/cli/commands/login.go (2)
44-55:⚠️ Potential issue | 🔴 CriticalReplace fmt.Sprintf with json.Marshal and add early return on request error to prevent panic.
Line 44 uses
fmt.Sprintfto build JSON, which will produce invalid JSON ifpasswordcontain quotes or backslashes. Lines 49–53 contain a more critical issue: the error fromhttp.NewRequestis caught and logged but execution continues, causing a nil pointer panic on line 53 whenreq.Header.Set()is called on a nil receiver.Proposed fix
import ( "bytes" + "encoding/json" "fmt" "log" "net/http" "time" @@ - jsonStr := []byte(fmt.Sprintf(`{"email":"%s","password":"%s"}`, email, password)) - payload := bytes.NewBuffer(jsonStr) + body, err := json.Marshal(map[string]string{ + "email": email, + "password": password, + }) + if err != nil { + fmt.Println("failed to encode login payload") + return + } + payload := bytes.NewBuffer(body) @@ req, err := http.NewRequest("POST", "http://localhost:8080/api/v1/auth/login", payload) if err != nil { fmt.Println("error on creating new POST req for login") + return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cli/commands/login.go` around lines 44 - 55, The code builds JSON with fmt.Sprintf (jsonStr) and continues after a failed http.NewRequest, risking invalid JSON and a nil-pointer panic; replace the fmt.Sprintf/json byte construction with encoding/json's json.Marshal on a struct or map (used to create payload/bytes.NewBuffer) and after calling http.NewRequest check err and return or exit immediately (do not call req.Header.Set when req is nil); update references to jsonStr, payload, client, req and the surrounding login logic in login.go accordingly.
29-31:⚠️ Potential issue | 🟠 MajorMask password input to avoid terminal echo.
fmt.Scanlnechoes the password to the terminal, creating both a security and UX risk for a login flow.🔒 Proposed fix to read password without echo
import ( "bytes" "fmt" "log" "net/http" + "os" + "strings" "time" "github.com/spf13/cobra" + "golang.org/x/term" ) @@ fmt.Printf("Password: ") - fmt.Scanln(&password) + pwdBytes, err := term.ReadPassword(int(os.Stdin.Fd())) + if err != nil { + fmt.Println("failed to read password") + return + } + fmt.Println() + password = strings.TrimSpace(string(pwdBytes))Note: The same vulnerability exists in
internal/cli/commands/register.goand should be addressed with the same fix.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cli/commands/login.go` around lines 29 - 31, Replace the insecure echoed input via fmt.Scanln(&password) with a no-echo read using term.ReadPassword (from golang.org/x/term) on the stdin file descriptor, keep the existing prompt (fmt.Printf("Password: ")), convert the returned byte slice to a string (trimming newline) and handle any read error; update imports to include "os" and "golang.org/x/term". Apply the same change to the analogous password input in register.go so neither login nor register echoes passwords.
🧹 Nitpick comments (5)
internal/server/utils/exchange.go (1)
45-83: Consider pruning expired entries to avoid unbounded growth.
Entries are only removed on Consume; repeated login attempts that never complete can leave expired entries in memory.♻️ Suggested pruning during Save
func (store *cliExchangeStore) Save(exchangeCode, state, token string, ttl time.Duration) { if exchangeCode == "" || state == "" || token == "" { return } store.mu.Lock() defer store.mu.Unlock() // store.mu.Unlock() is called before Save() returns + now := time.Now() + for code, entry := range store.entries { + if now.After(entry.expiresAt) { + delete(store.entries, code) + } + } + store.entries[exchangeCode] = cliExchangeEntry{ token: token, state: state, - expiresAt: time.Now().Add(ttl), + expiresAt: now.Add(ttl), } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/utils/exchange.go` around lines 45 - 83, The store currently only deletes entries on Consume, which lets expired cliExchangeEntry items accumulate; update cliExchangeStore.Save to prune expired entries before inserting: while holding store.mu lock (same lock used in Save), iterate store.entries and delete any entry where time.Now().After(entry.expiresAt), then insert the new entry as before; ensure you keep the existing nil/empty checks for exchangeCode/state/token and maintain thread-safety by doing pruning and insertion under the same lock used in Save.internal/cli/utils/exchange_test.go (1)
157-303: Consider avoiding globalhttp.DefaultTransportoverrides in tests.
Mutating the global transport can cause flakiness if tests become parallelized; prefer injecting a client or package-level var inExchangeServerCode.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cli/utils/exchange_test.go` around lines 157 - 303, Tests mutate global http.DefaultTransport which can cause flakiness; modify ExchangeServerCode to accept an *http.Client parameter (or use a package-level variable like var httpClient = http.DefaultClient) and use that client for the request instead of the global defaults, then update the tests to supply a custom http.Client with a RoundTripper mocking transport (or set the package-level httpClient in tests) so you no longer override http.DefaultTransport; ensure symbols referenced are ExchangeServerCode and any new package-level httpClient variable or the new function signature that takes *http.Client.internal/cli/commands/login.go (1)
46-47: Avoid hardcoded API base URL in the CLI.
Hardcoding localhost makes the command unusable against non-local deployments; consider reading a base URL from config/env with a localhost default.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cli/commands/login.go` around lines 46 - 47, The code currently hardcodes "http://localhost:8080" when creating the request in login.go (see the http.NewRequest call); change it to read a base API URL from configuration or an environment variable (e.g. API_BASE_URL) with "http://localhost:8080" as the default, then build the full request URL by joining that base with "/api/v1/auth/login" before calling http.NewRequest; keep using the existing http.Client (client variable) and its Timeout. Ensure you validate or normalize the base URL (trailing slash handling) so the constructed URL is correct.internal/cli/utils/exchange.go (1)
55-62: Make the API base URL configurable instead of hardcoding localhost.
Both the auth URL and exchange endpoint are fixed to localhost, which blocks non-local deployments and tests.🔧 Suggested helper for configurable base URL
import ( "bytes" "crypto/rand" "encoding/hex" "encoding/json" "fmt" "io" "net/http" "net/url" + "os" "os/exec" "strings" "time" ) @@ +func apiBaseURL() string { + if v := strings.TrimSpace(os.Getenv("ENVLINK_API_URL")); v != "" { + return v + } + return "http://localhost:8080" +} + func BuildServerGoogleAuthURL(callbackURL, state string) string { - baseURL := "http://localhost:8080/api/v1/auth/google" + baseURL := apiBaseURL() + "/api/v1/auth/google" values := url.Values{} values.Set("cli_callback", callbackURL) values.Set("cli_state", state) @@ - req, err := http.NewRequest("POST", "http://localhost:8080/api/v1/auth/cli/exchange", bytes.NewBuffer(payloadBytes)) + req, err := http.NewRequest("POST", apiBaseURL()+"/api/v1/auth/cli/exchange", bytes.NewBuffer(payloadBytes))Also applies to: 127-128
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cli/utils/exchange.go` around lines 55 - 62, BuildServerGoogleAuthURL currently hardcodes the API base URL to http://localhost:8080 which prevents non-local deployments and tests; change it to read a configurable base URL (e.g., from an environment variable or shared config) with a sensible default and use that when building the URL. Update BuildServerGoogleAuthURL to accept or obtain the server base (instead of the hardcoded baseURL) and do the same for the corresponding exchange endpoint function (e.g., BuildServerGoogleExchangeURL) so both auth and exchange endpoints use the configurable SERVER_BASE_URL defaulting to localhost if unset.internal/cli/commands/register.go (1)
88-155: OAuth flow implementation is well-structured.The local callback server pattern, timeout handling, and cleanup logic are implemented correctly. The use of channels for async communication is appropriate.
One minor note: printing the access token (line 154) should be removed or replaced with secure storage before production use.
Consider removing or replacing line 154 with secure token storage:
fmt.Println("Google authentication successful.") - fmt.Println("Token:", token.AccessToken) + // TODO: Store token securely for future API calls🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cli/commands/register.go` around lines 88 - 155, The code prints the OAuth access token directly in registerUsingGoogle (fmt.Println("Token:", token.AccessToken)); remove that direct printing and instead securely persist the token (e.g., call an existing credential/secret store helper or add a secure save function such as SaveAuthToken/StoreToken and pass token.AccessToken), or if no storage is available yet, drop the print and return a success message only; ensure you update registerUsingGoogle to call the secure storage method (or omit printing) after receiving token from cliutils.ExchangeServerCode and before exiting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/test.yml:
- Around line 18-21: Update the GitHub Actions step that uses
actions/setup-go@v4 to the current supported major version: replace the uses
reference "actions/setup-go@v4" with "actions/setup-go@v6" (or
"actions/setup-go@v5" if your runners are older/self-hosted and don’t meet
runner v2.327.1+); keep the existing go-version setting (go-version: '1.24') and
ensure your CI runners meet the v6 requirement before switching.
In `@cmd/server/main.go`:
- Around line 18-21: The call to godotenv.Load() currently triggers log.Fatalf
on any error (via log.Fatalf), which crashes in production when no .env file
exists; change the handling so missing .env is non-fatal: call godotenv.Load()
and if it returns an error, detect if it's "file not found" (or simply treat it
as optional) and only log a warning/info (e.g., log.Printf) and continue, while
still treating other unexpected errors as fatal; update the block around
godotenv.Load() and log.Fatalf to perform this conditional handling.
In `@internal/cli/commands/register.go`:
- Line 67: Replace the hardcoded "http://localhost:8080" in the register command
(the http.NewRequest call in internal/cli/commands/register.go) with a
configurable base URL (e.g., read ENVLINK_SERVER_URL with a fallback to
"http://localhost:8080" or accept a CLI flag) and build the full request URL by
joining the base URL and "/api/v1/auth/register" (handle trailing/leading
slashes to avoid double slashes); ensure the register command uses this base URL
when creating the HTTP request so the CLI can target different environments.
- Around line 52-62: The snippet in register.go prints sensitive debug messages
for the variables email and password; remove the fmt.Println calls that output
"email provided"/"email not provided" and "password provided"/"password not
provided" so the CLI no longer leaks credential presence. If visibility is
needed for debugging, replace these prints with a conditional debug log using
the existing verbose/trace flag or a logger (guarded by that flag) instead of
unconditional fmt.Println, and reference the email and password variables in the
register command code to locate the removals.
- Around line 64-71: The code in register.go builds JSON via fmt.Sprintf into
jsonStr and does not return after NewRequest error; replace the manual string
interpolation with proper JSON marshaling (e.g., create a struct or map for
email/password and use json.Marshal to produce payload), create payload with
bytes.NewBuffer(marshaledBytes), and handle the http.NewRequest error by logging
and returning (or exiting) immediately so code does not continue with a nil req;
update references around jsonStr, payload, client, and req accordingly.
In `@internal/server/api/auth/auth.go`:
- Around line 19-37: The NewAuth function currently passes the raw key into
sessions.NewCookieStore without validating its strength; add validation for the
key parameter (used when calling sessions.NewCookieStore) to ensure it is at
least 32 bytes (recommend 32+ bytes) and non-empty, returning an error from
NewAuth if the key is too short or missing; alternatively, document/offer
generating a secure random 32-byte key if key is empty, but do not call
sessions.NewCookieStore until the key passes this validation and use the
validated []byte(key) when creating store.
In `@internal/server/api/auth/controllers.go`:
- Around line 32-47: In postLogin, remove the credential logging (delete the
fmt.Println(requestBody.Email) and fmt.Println(requestBody.Password)) and stop
returning the full authRequestBody; after calling controller.repo.Login(...),
return a sanitized response (e.g., an OK message or a struct containing only
non-sensitive fields such as email or a generated token) instead of the original
requestBody so the password is not leaked; ensure authRequestBody and postLogin
continue to validate/login via controller.repo.Login but never include or log
the password in responses or logs.
- Around line 49-64: In postRegister, remove the credential logging and stop
returning the raw authRequestBody (which contains the password); specifically
delete the fmt.Println(requestBody.Email) and fmt.Println(requestBody.Password)
calls and instead return a sanitized response (e.g., success message or user
identifier/email only) so the password from authRequestBody is never included in
the JSON response; ensure controller.repo.Register(requestBody.Email,
requestBody.Password) still runs but only non-sensitive fields (or a generic
success payload) are sent back from postRegister.
- Around line 127-142: The HTML response in the handler builds a string with
user-controlled fields (user.Name, user.Email, user.Provider) and is vulnerable
to XSS; update the HTML generation in the controller (the code that constructs
html := fmt.Sprintf(...)) to escape those values before interpolation (e.g.,
call html.EscapeString on user.Name, user.Email and user.Provider or use Go's
html/template to render safely) so the response writes only escaped content to
c.Data/http response.
In `@internal/server/utils/exchange.go`:
- Around line 112-115: WriteCLIAuthContext currently sets CLI cookies with
secure=false which exposes callback/state over HTTP in production; update it to
detect whether the current request is secure (check c.Request.TLS != nil and
also respect the X-Forwarded-Proto header for proxies) and set the cookie Secure
flag accordingly. Modify the calls that set cliCallbackCookieName and
cliStateCookieName so they pass Secure: isProdAndRequestIsHTTPS (true only when
running in prod and the request is HTTPS via TLS or X-Forwarded-Proto ==
"https"), keeping HttpOnly and other params unchanged; use the same detection
logic used elsewhere for auth session (mirror auth.go behavior) to ensure
cookies are only sent over secure connections in production.
---
Outside diff comments:
In `@cmd/server/.env.example`:
- Around line 1-8: The .env.example file has dotenv-linter warnings for key
ordering and missing trailing newline; reorder the environment keys (DB_HOST,
DB_NAME, DB_PASSWORD, DB_PORT, DB_USER, GOOGLE_CLIENT_ID, GOOGLE_CLIENT_SECRET,
PORT) into the linter-expected order (e.g., alphabetical or project convention)
and add a final blank line at the end of the file so the file ends with a
newline.
In `@internal/cli/commands/login.go`:
- Around line 44-55: The code builds JSON with fmt.Sprintf (jsonStr) and
continues after a failed http.NewRequest, risking invalid JSON and a nil-pointer
panic; replace the fmt.Sprintf/json byte construction with encoding/json's
json.Marshal on a struct or map (used to create payload/bytes.NewBuffer) and
after calling http.NewRequest check err and return or exit immediately (do not
call req.Header.Set when req is nil); update references to jsonStr, payload,
client, req and the surrounding login logic in login.go accordingly.
- Around line 29-31: Replace the insecure echoed input via fmt.Scanln(&password)
with a no-echo read using term.ReadPassword (from golang.org/x/term) on the
stdin file descriptor, keep the existing prompt (fmt.Printf("Password: ")),
convert the returned byte slice to a string (trimming newline) and handle any
read error; update imports to include "os" and "golang.org/x/term". Apply the
same change to the analogous password input in register.go so neither login nor
register echoes passwords.
---
Nitpick comments:
In `@internal/cli/commands/login.go`:
- Around line 46-47: The code currently hardcodes "http://localhost:8080" when
creating the request in login.go (see the http.NewRequest call); change it to
read a base API URL from configuration or an environment variable (e.g.
API_BASE_URL) with "http://localhost:8080" as the default, then build the full
request URL by joining that base with "/api/v1/auth/login" before calling
http.NewRequest; keep using the existing http.Client (client variable) and its
Timeout. Ensure you validate or normalize the base URL (trailing slash handling)
so the constructed URL is correct.
In `@internal/cli/commands/register.go`:
- Around line 88-155: The code prints the OAuth access token directly in
registerUsingGoogle (fmt.Println("Token:", token.AccessToken)); remove that
direct printing and instead securely persist the token (e.g., call an existing
credential/secret store helper or add a secure save function such as
SaveAuthToken/StoreToken and pass token.AccessToken), or if no storage is
available yet, drop the print and return a success message only; ensure you
update registerUsingGoogle to call the secure storage method (or omit printing)
after receiving token from cliutils.ExchangeServerCode and before exiting.
In `@internal/cli/utils/exchange_test.go`:
- Around line 157-303: Tests mutate global http.DefaultTransport which can cause
flakiness; modify ExchangeServerCode to accept an *http.Client parameter (or use
a package-level variable like var httpClient = http.DefaultClient) and use that
client for the request instead of the global defaults, then update the tests to
supply a custom http.Client with a RoundTripper mocking transport (or set the
package-level httpClient in tests) so you no longer override
http.DefaultTransport; ensure symbols referenced are ExchangeServerCode and any
new package-level httpClient variable or the new function signature that takes
*http.Client.
In `@internal/cli/utils/exchange.go`:
- Around line 55-62: BuildServerGoogleAuthURL currently hardcodes the API base
URL to http://localhost:8080 which prevents non-local deployments and tests;
change it to read a configurable base URL (e.g., from an environment variable or
shared config) with a sensible default and use that when building the URL.
Update BuildServerGoogleAuthURL to accept or obtain the server base (instead of
the hardcoded baseURL) and do the same for the corresponding exchange endpoint
function (e.g., BuildServerGoogleExchangeURL) so both auth and exchange
endpoints use the configurable SERVER_BASE_URL defaulting to localhost if unset.
In `@internal/server/utils/exchange.go`:
- Around line 45-83: The store currently only deletes entries on Consume, which
lets expired cliExchangeEntry items accumulate; update cliExchangeStore.Save to
prune expired entries before inserting: while holding store.mu lock (same lock
used in Save), iterate store.entries and delete any entry where
time.Now().After(entry.expiresAt), then insert the new entry as before; ensure
you keep the existing nil/empty checks for exchangeCode/state/token and maintain
thread-safety by doing pruning and insertion under the same lock used in Save.
Comment on lines
+18
to
+21
| - name: Set up Go | ||
| uses: actions/setup-go@v4 | ||
| with: | ||
| go-version: '1.24' |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
actions/setup-go GitHub Actions latest version supported
💡 Result:
- Latest supported major version:
actions/setup-go@v6(current recommended major). It upgrades the action runtime to Node.js 24 and includes updated Go toolchain/version resolution behavior. [1] - Runner requirement for v6: your GitHub Actions runner must be actions/runner v2.327.1 or later (needed for running Node24-based actions). [1][2]
- If you can’t meet that runner minimum (common on older self-hosted runners / some GHES setups): use
actions/setup-go@v5instead. [1]
Example
- uses: actions/setup-go@v6
with:
go-version: '1.23.x'[1] (github.com)
[2] (github.com)
Citations:
Update actions/setup-go to the latest supported major version.
actions/setup-go@v4 is outdated and no longer recommended. The latest supported major version is v6, which upgrades the runtime to Node.js 24 and includes updated Go toolchain resolution. Note that v6 requires GitHub Actions runner v2.327.1 or later; if you're using older self-hosted runners or GitHub Enterprise Server, use v5 instead.
✅ Suggested change
- name: Set up Go
- uses: actions/setup-go@v4
+ uses: actions/setup-go@v6
with:
go-version: '1.24'📝 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.
Suggested change
| - name: Set up Go | |
| uses: actions/setup-go@v4 | |
| with: | |
| go-version: '1.24' | |
| - name: Set up Go | |
| uses: actions/setup-go@v6 | |
| with: | |
| go-version: '1.24' |
🧰 Tools
🪛 actionlint (1.7.11)
[error] 19-19: the runner of "actions/setup-go@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/test.yml around lines 18 - 21, Update the GitHub Actions
step that uses actions/setup-go@v4 to the current supported major version:
replace the uses reference "actions/setup-go@v4" with "actions/setup-go@v6" (or
"actions/setup-go@v5" if your runners are older/self-hosted and don’t meet
runner v2.327.1+); keep the existing go-version setting (go-version: '1.24') and
ensure your CI runners meet the v6 requirement before switching.
Comment on lines
+18
to
+21
| err := godotenv.Load() | ||
| if err != nil { | ||
| log.Fatalf("Could not load environment variables!") | ||
| } |
There was a problem hiding this comment.
godotenv.Load() will cause fatal error in production where .env file doesn't exist.
In production deployments (like Railway), environment variables are typically set directly in the environment, not via a .env file. The current code will crash on startup if the file is absent.
🐛 Proposed fix to handle missing .env gracefully
err := godotenv.Load()
if err != nil {
- log.Fatalf("Could not load environment variables!")
+ log.Println("No .env file found, using environment variables directly")
}📝 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.
Suggested change
| err := godotenv.Load() | |
| if err != nil { | |
| log.Fatalf("Could not load environment variables!") | |
| } | |
| err := godotenv.Load() | |
| if err != nil { | |
| log.Println("No .env file found, using environment variables directly") | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/server/main.go` around lines 18 - 21, The call to godotenv.Load()
currently triggers log.Fatalf on any error (via log.Fatalf), which crashes in
production when no .env file exists; change the handling so missing .env is
non-fatal: call godotenv.Load() and if it returns an error, detect if it's "file
not found" (or simply treat it as optional) and only log a warning/info (e.g.,
log.Printf) and continue, while still treating other unexpected errors as fatal;
update the block around godotenv.Load() and log.Fatalf to perform this
conditional handling.
Comment on lines
+52
to
+62
| if email != "" { | ||
| fmt.Println("email provided") | ||
| } else { | ||
| fmt.Println("email not provided") | ||
| } | ||
|
|
||
| if password != "" { | ||
| fmt.Println("password provided") | ||
| } else { | ||
| fmt.Println("password not provided") | ||
| } |
There was a problem hiding this comment.
Remove debug print statements before merging.
These verbose prints leak information about whether credentials were provided and add noise to the CLI output.
🧹 Proposed fix to remove debug prints
- if email != "" {
- fmt.Println("email provided")
- } else {
- fmt.Println("email not provided")
- }
-
- if password != "" {
- fmt.Println("password provided")
- } else {
- fmt.Println("password not provided")
- }📝 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.
Suggested change
| if email != "" { | |
| fmt.Println("email provided") | |
| } else { | |
| fmt.Println("email not provided") | |
| } | |
| if password != "" { | |
| fmt.Println("password provided") | |
| } else { | |
| fmt.Println("password not provided") | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/cli/commands/register.go` around lines 52 - 62, The snippet in
register.go prints sensitive debug messages for the variables email and
password; remove the fmt.Println calls that output "email provided"/"email not
provided" and "password provided"/"password not provided" so the CLI no longer
leaks credential presence. If visibility is needed for debugging, replace these
prints with a conditional debug log using the existing verbose/trace flag or a
logger (guarded by that flag) instead of unconditional fmt.Println, and
reference the email and password variables in the register command code to
locate the removals.
Comment on lines
+64
to
+71
| jsonStr := []byte(fmt.Sprintf(`{"email":"%s","password":"%s"}`, email, password)) | ||
| payload := bytes.NewBuffer(jsonStr) | ||
| client := &http.Client{Timeout: 10 * time.Second} | ||
| req, err := http.NewRequest("POST", "http://localhost:8080/api/v1/auth/register", payload) | ||
|
|
||
| if err != nil { | ||
| fmt.Println("error on creating new POST req for register") | ||
| } |
There was a problem hiding this comment.
JSON injection vulnerability and missing early return on error.
- String interpolation into JSON is unsafe - special characters in email/password (quotes, backslashes) will break or inject malicious JSON.
- After error on line 70, execution continues and
reqis nil, causing a panic on line 73.
🐛 Proposed fix using proper JSON marshaling and early return
+import "encoding/json"
+
func registerUsingEmailPassword() {
var email string
var password string
fmt.Printf("Email: ")
fmt.Scanln(&email)
fmt.Printf("Password: ")
fmt.Scanln(&password)
- jsonStr := []byte(fmt.Sprintf(`{"email":"%s","password":"%s"}`, email, password))
- payload := bytes.NewBuffer(jsonStr)
+ data := map[string]string{"email": email, "password": password}
+ jsonBytes, err := json.Marshal(data)
+ if err != nil {
+ fmt.Println("error encoding request body")
+ return
+ }
+ payload := bytes.NewBuffer(jsonBytes)
client := &http.Client{Timeout: 10 * time.Second}
req, err := http.NewRequest("POST", "http://localhost:8080/api/v1/auth/register", payload)
if err != nil {
fmt.Println("error on creating new POST req for register")
+ return
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/cli/commands/register.go` around lines 64 - 71, The code in
register.go builds JSON via fmt.Sprintf into jsonStr and does not return after
NewRequest error; replace the manual string interpolation with proper JSON
marshaling (e.g., create a struct or map for email/password and use json.Marshal
to produce payload), create payload with bytes.NewBuffer(marshaledBytes), and
handle the http.NewRequest error by logging and returning (or exiting)
immediately so code does not continue with a nil req; update references around
jsonStr, payload, client, and req accordingly.
| jsonStr := []byte(fmt.Sprintf(`{"email":"%s","password":"%s"}`, email, password)) | ||
| payload := bytes.NewBuffer(jsonStr) | ||
| client := &http.Client{Timeout: 10 * time.Second} | ||
| req, err := http.NewRequest("POST", "http://localhost:8080/api/v1/auth/register", payload) |
There was a problem hiding this comment.
Hardcoded localhost:8080 URL won't work against production server.
The CLI needs to support configurable server URLs for different environments.
Consider adding a configuration mechanism (environment variable, config file, or CLI flag) for the server base URL:
// Example: use environment variable with fallback
baseURL := os.Getenv("ENVLINK_SERVER_URL")
if baseURL == "" {
baseURL = "http://localhost:8080"
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/cli/commands/register.go` at line 67, Replace the hardcoded
"http://localhost:8080" in the register command (the http.NewRequest call in
internal/cli/commands/register.go) with a configurable base URL (e.g., read
ENVLINK_SERVER_URL with a fallback to "http://localhost:8080" or accept a CLI
flag) and build the full request URL by joining the base URL and
"/api/v1/auth/register" (handle trailing/leading slashes to avoid double
slashes); ensure the register command uses this base URL when creating the HTTP
request so the CLI can target different environments.
Comment on lines
+19
to
+37
| func NewAuth(port string, domain string, key string, isProd bool) error { | ||
| googleClientId := os.Getenv("GOOGLE_CLIENT_ID") | ||
| if googleClientId == "" { | ||
| return errors.New("Google Client Id was not provided!") | ||
| } | ||
|
|
||
| googleClientSecret := os.Getenv("GOOGLE_CLIENT_SECRET") | ||
| if googleClientSecret == "" { | ||
| return errors.New("Google Client Secret was not provided!") | ||
| } | ||
|
|
||
| store := sessions.NewCookieStore([]byte(key)) | ||
| store.Options = &sessions.Options{ | ||
| Path: "/", // cookie is valid for all paths on the host | ||
| MaxAge: 86400 * 30, | ||
| HttpOnly: true, | ||
| Secure: isProd, | ||
| SameSite: http.SameSiteLaxMode, | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
gorilla sessions CookieStore recommended key length authentication encryption
💡 Result:
For gorilla/sessions.CookieStore (i.e., sessions.NewCookieStore(keyPairs ...[]byte)), keys are provided in pairs: (authentication key, encryption key). The encryption key is optional (can be nil/omitted), but the authentication key is required. [1]
Recommended / required key lengths
- Authentication key (HMAC): recommended 32 or 64 bytes. [1]
- Encryption key (AES, optional): must be 16, 24, or 32 bytes (AES-128 / AES-192 / AES-256). [1][2]
How to generate keys
- Use
securecookie.GenerateRandomKey(n)to generate strong random keys of the required lengths, and persist them so cookies remain decodable across restarts. [2]
Validate session key strength before creating the cookie store.
NewCookieStore([]byte(key)) accepts the key as an HMAC authentication key without validation. A minimum of 32 bytes is recommended for authentication keys in gorilla/sessions to ensure adequate session integrity. Empty or very short keys significantly weaken the security of session signatures.
🛡️ Suggested change
func NewAuth(port string, domain string, key string, isProd bool) error {
+ if len(key) < 32 {
+ return errors.New("session key must be at least 32 bytes")
+ }
googleClientId := os.Getenv("GOOGLE_CLIENT_ID")
if googleClientId == "" {
return errors.New("Google Client Id was not provided!")
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/server/api/auth/auth.go` around lines 19 - 37, The NewAuth function
currently passes the raw key into sessions.NewCookieStore without validating its
strength; add validation for the key parameter (used when calling
sessions.NewCookieStore) to ensure it is at least 32 bytes (recommend 32+ bytes)
and non-empty, returning an error from NewAuth if the key is too short or
missing; alternatively, document/offer generating a secure random 32-byte key if
key is empty, but do not call sessions.NewCookieStore until the key passes this
validation and use the validated []byte(key) when creating store.
Comment on lines
+32
to
+47
| func (controller *AuthController) postLogin(c *gin.Context) { | ||
| var requestBody authRequestBody | ||
| if err := c.BindJSON(&requestBody); err != nil { | ||
| c.IndentedJSON(http.StatusBadRequest, gin.H{"error": "invalid request body"}) | ||
| return | ||
| } | ||
|
|
||
| if _, err := controller.repo.Login(requestBody.Email, requestBody.Password); err != nil { | ||
| c.IndentedJSON(http.StatusInternalServerError, gin.H{"error": "failed to login"}) | ||
| return | ||
| } | ||
|
|
||
| fmt.Println(requestBody.Email) | ||
| fmt.Println(requestBody.Password) | ||
| c.IndentedJSON(http.StatusOK, requestBody) | ||
| } |
There was a problem hiding this comment.
Security issues: logging credentials and returning password in response.
- Lines 44-45: Logging email and password to stdout exposes credentials in logs.
- Line 46: Returning the full request body (including password) in the response leaks sensitive data.
🔐 Proposed fix to remove credential logging and sanitize response
func (controller *AuthController) postLogin(c *gin.Context) {
var requestBody authRequestBody
if err := c.BindJSON(&requestBody); err != nil {
c.IndentedJSON(http.StatusBadRequest, gin.H{"error": "invalid request body"})
return
}
if _, err := controller.repo.Login(requestBody.Email, requestBody.Password); err != nil {
c.IndentedJSON(http.StatusInternalServerError, gin.H{"error": "failed to login"})
return
}
- fmt.Println(requestBody.Email)
- fmt.Println(requestBody.Password)
- c.IndentedJSON(http.StatusOK, requestBody)
+ c.IndentedJSON(http.StatusOK, gin.H{"message": "login successful"})
}📝 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.
Suggested change
| func (controller *AuthController) postLogin(c *gin.Context) { | |
| var requestBody authRequestBody | |
| if err := c.BindJSON(&requestBody); err != nil { | |
| c.IndentedJSON(http.StatusBadRequest, gin.H{"error": "invalid request body"}) | |
| return | |
| } | |
| if _, err := controller.repo.Login(requestBody.Email, requestBody.Password); err != nil { | |
| c.IndentedJSON(http.StatusInternalServerError, gin.H{"error": "failed to login"}) | |
| return | |
| } | |
| fmt.Println(requestBody.Email) | |
| fmt.Println(requestBody.Password) | |
| c.IndentedJSON(http.StatusOK, requestBody) | |
| } | |
| func (controller *AuthController) postLogin(c *gin.Context) { | |
| var requestBody authRequestBody | |
| if err := c.BindJSON(&requestBody); err != nil { | |
| c.IndentedJSON(http.StatusBadRequest, gin.H{"error": "invalid request body"}) | |
| return | |
| } | |
| if _, err := controller.repo.Login(requestBody.Email, requestBody.Password); err != nil { | |
| c.IndentedJSON(http.StatusInternalServerError, gin.H{"error": "failed to login"}) | |
| return | |
| } | |
| c.IndentedJSON(http.StatusOK, gin.H{"message": "login successful"}) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/server/api/auth/controllers.go` around lines 32 - 47, In postLogin,
remove the credential logging (delete the fmt.Println(requestBody.Email) and
fmt.Println(requestBody.Password)) and stop returning the full authRequestBody;
after calling controller.repo.Login(...), return a sanitized response (e.g., an
OK message or a struct containing only non-sensitive fields such as email or a
generated token) instead of the original requestBody so the password is not
leaked; ensure authRequestBody and postLogin continue to validate/login via
controller.repo.Login but never include or log the password in responses or
logs.
Comment on lines
+49
to
+64
| func (controller *AuthController) postRegister(c *gin.Context) { | ||
| var requestBody authRequestBody | ||
| if err := c.BindJSON(&requestBody); err != nil { | ||
| c.IndentedJSON(http.StatusBadRequest, gin.H{"error": "invalid request body"}) | ||
| return | ||
| } | ||
|
|
||
| if _, err := controller.repo.Register(requestBody.Email, requestBody.Password); err != nil { | ||
| c.IndentedJSON(http.StatusInternalServerError, gin.H{"error": "failed to register"}) | ||
| return | ||
| } | ||
|
|
||
| fmt.Println(requestBody.Email) | ||
| fmt.Println(requestBody.Password) | ||
| c.IndentedJSON(http.StatusOK, requestBody) | ||
| } |
There was a problem hiding this comment.
Same security issues as postLogin: credential logging and password in response.
🔐 Proposed fix
func (controller *AuthController) postRegister(c *gin.Context) {
var requestBody authRequestBody
if err := c.BindJSON(&requestBody); err != nil {
c.IndentedJSON(http.StatusBadRequest, gin.H{"error": "invalid request body"})
return
}
if _, err := controller.repo.Register(requestBody.Email, requestBody.Password); err != nil {
c.IndentedJSON(http.StatusInternalServerError, gin.H{"error": "failed to register"})
return
}
- fmt.Println(requestBody.Email)
- fmt.Println(requestBody.Password)
- c.IndentedJSON(http.StatusOK, requestBody)
+ c.IndentedJSON(http.StatusCreated, gin.H{"message": "registration successful"})
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/server/api/auth/controllers.go` around lines 49 - 64, In
postRegister, remove the credential logging and stop returning the raw
authRequestBody (which contains the password); specifically delete the
fmt.Println(requestBody.Email) and fmt.Println(requestBody.Password) calls and
instead return a sanitized response (e.g., success message or user
identifier/email only) so the password from authRequestBody is never included in
the JSON response; ensure controller.repo.Register(requestBody.Email,
requestBody.Password) still runs but only non-sensitive fields (or a generic
success payload) are sent back from postRegister.
Comment on lines
+127
to
+142
| html := fmt.Sprintf(` | ||
| <!doctype html> | ||
| <html> | ||
| <head><meta charset="utf-8"><title>Auth successful</title></head> | ||
| <body> | ||
| <h1>Authentication successful</h1> | ||
| <p><strong>Name:</strong> %s</p> | ||
| <p><strong>Email:</strong> %s</p> | ||
| <p><strong>Provider:</strong> %s</p> | ||
| <p>You can close this window and return to the CLI.</p> | ||
| </body> | ||
| </html> | ||
| `, user.Name, user.Email, user.Provider) | ||
|
|
||
| c.Data(http.StatusOK, "text/html; charset=utf-8", []byte(html)) | ||
| } |
There was a problem hiding this comment.
XSS vulnerability: user-controlled data rendered directly in HTML.
user.Name, user.Email, and user.Provider are interpolated into HTML without escaping. A malicious OAuth provider response or manipulated profile could inject scripts.
🔐 Proposed fix using html.EscapeString
+import "html"
+
html := fmt.Sprintf(`
<!doctype html>
<html>
<head><meta charset="utf-8"><title>Auth successful</title></head>
<body>
<h1>Authentication successful</h1>
<p><strong>Name:</strong> %s</p>
<p><strong>Email:</strong> %s</p>
<p><strong>Provider:</strong> %s</p>
<p>You can close this window and return to the CLI.</p>
</body>
</html>
- `, user.Name, user.Email, user.Provider)
+ `, html.EscapeString(user.Name), html.EscapeString(user.Email), html.EscapeString(user.Provider))📝 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.
Suggested change
| html := fmt.Sprintf(` | |
| <!doctype html> | |
| <html> | |
| <head><meta charset="utf-8"><title>Auth successful</title></head> | |
| <body> | |
| <h1>Authentication successful</h1> | |
| <p><strong>Name:</strong> %s</p> | |
| <p><strong>Email:</strong> %s</p> | |
| <p><strong>Provider:</strong> %s</p> | |
| <p>You can close this window and return to the CLI.</p> | |
| </body> | |
| </html> | |
| `, user.Name, user.Email, user.Provider) | |
| c.Data(http.StatusOK, "text/html; charset=utf-8", []byte(html)) | |
| } | |
| html := fmt.Sprintf(` | |
| <!doctype html> | |
| <html> | |
| <head><meta charset="utf-8"><title>Auth successful</title></head> | |
| <body> | |
| <h1>Authentication successful</h1> | |
| <p><strong>Name:</strong> %s</p> | |
| <p><strong>Email:</strong> %s</p> | |
| <p><strong>Provider:</strong> %s</p> | |
| <p>You can close this window and return to the CLI.</p> | |
| </body> | |
| </html> | |
| `, html.EscapeString(user.Name), html.EscapeString(user.Email), html.EscapeString(user.Provider)) | |
| c.Data(http.StatusOK, "text/html; charset=utf-8", []byte(html)) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/server/api/auth/controllers.go` around lines 127 - 142, The HTML
response in the handler builds a string with user-controlled fields (user.Name,
user.Email, user.Provider) and is vulnerable to XSS; update the HTML generation
in the controller (the code that constructs html := fmt.Sprintf(...)) to escape
those values before interpolation (e.g., call html.EscapeString on user.Name,
user.Email and user.Provider or use Go's html/template to render safely) so the
response writes only escaped content to c.Data/http response.
Comment on lines
+112
to
+115
| func WriteCLIAuthContext(c *gin.Context, callbackURL, state string) { | ||
| c.SetCookie(cliCallbackCookieName, url.QueryEscape(callbackURL), cliCookieTTLSeconds, "/", "", false, true) | ||
| c.SetCookie(cliStateCookieName, state, cliCookieTTLSeconds, "/", "", false, true) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n internal/server/utils/exchange.go | head -150Repository: jasutiin/envlink
Length of output: 5234
🏁 Script executed:
rg "SetCookie" internal/server/utils/ -B 2 -A 2Repository: jasutiin/envlink
Length of output: 916
🏁 Script executed:
rg "secure.*:=" internal/server/ -B 2 -A 2Repository: jasutiin/envlink
Length of output: 42
🏁 Script executed:
grep -r "X-Forwarded-Proto" internal/Repository: jasutiin/envlink
Length of output: 42
🏁 Script executed:
find . -name "main.go" -o -name "*server*.go" | head -20Repository: jasutiin/envlink
Length of output: 98
🏁 Script executed:
rg "TLS|ListenAndServeTLS" internal/server/ -B 2 -A 2Repository: jasutiin/envlink
Length of output: 829
🏁 Script executed:
rg "gin.Engine|Router" internal/server/ --type go -B 2 -A 5 | head -60Repository: jasutiin/envlink
Length of output: 1299
🏁 Script executed:
rg "Request.TLS\|X-Forwarded" . --type goRepository: jasutiin/envlink
Length of output: 42
🏁 Script executed:
cat -n cmd/server/main.goRepository: jasutiin/envlink
Length of output: 1764
🏁 Script executed:
find . -name "*.go" -type f | xargs rg "ListenAndServe" -B 5 -A 5Repository: jasutiin/envlink
Length of output: 44
🏁 Script executed:
rg "isProd" internal/server/ -B 2 -A 2Repository: jasutiin/envlink
Length of output: 682
🏁 Script executed:
grep -r "secure.*true\|Secure.*true" internal/server/ --type goRepository: jasutiin/envlink
Length of output: 177
🏁 Script executed:
cat -n internal/server/api/auth/auth.go | head -80Repository: jasutiin/envlink
Length of output: 1676
🏁 Script executed:
rg "WriteCLIAuthContext|ClearCLIAuthContext" internal/server/ -B 3 -A 3Repository: jasutiin/envlink
Length of output: 2829
🏁 Script executed:
cat -n internal/server/api/auth/controller.go | grep -A 10 -B 10 "WriteCLIAuthContext\|ClearCLIAuthContext"Repository: jasutiin/envlink
Length of output: 130
Secure CLI auth cookies in production.
secure=false allows cookies carrying OAuth state and callback URLs to transmit over HTTP in production, exposing the CLI authentication flow. The main auth session already correctly uses Secure: isProd (auth.go:35), but CLI cookies ignore the production flag.
The fix detects HTTPS using c.Request.TLS or the X-Forwarded-Proto header (for reverse proxies like Railway), ensuring cookies are only sent over secure connections in production:
Proposed fix
func WriteCLIAuthContext(c *gin.Context, callbackURL, state string) {
+ secure := c.Request.TLS != nil || strings.EqualFold(c.GetHeader("X-Forwarded-Proto"), "https")
- c.SetCookie(cliCallbackCookieName, url.QueryEscape(callbackURL), cliCookieTTLSeconds, "/", "", false, true)
- c.SetCookie(cliStateCookieName, state, cliCookieTTLSeconds, "/", "", false, true)
+ c.SetCookie(cliCallbackCookieName, url.QueryEscape(callbackURL), cliCookieTTLSeconds, "/", "", secure, true)
+ c.SetCookie(cliStateCookieName, state, cliCookieTTLSeconds, "/", "", secure, true)
}
func ClearCLIAuthContext(c *gin.Context) {
+ secure := c.Request.TLS != nil || strings.EqualFold(c.GetHeader("X-Forwarded-Proto"), "https")
- c.SetCookie(cliCallbackCookieName, "", -1, "/", "", false, true)
- c.SetCookie(cliStateCookieName, "", -1, "/", "", false, true)
+ c.SetCookie(cliCallbackCookieName, "", -1, "/", "", secure, true)
+ c.SetCookie(cliStateCookieName, "", -1, "/", "", secure, true)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/server/utils/exchange.go` around lines 112 - 115,
WriteCLIAuthContext currently sets CLI cookies with secure=false which exposes
callback/state over HTTP in production; update it to detect whether the current
request is secure (check c.Request.TLS != nil and also respect the
X-Forwarded-Proto header for proxies) and set the cookie Secure flag
accordingly. Modify the calls that set cliCallbackCookieName and
cliStateCookieName so they pass Secure: isProdAndRequestIsHTTPS (true only when
running in prod and the request is HTTPS via TLS or X-Forwarded-Proto ==
"https"), keeping HttpOnly and other params unchanged; use the same detection
logic used elsewhere for auth session (mirror auth.go behavior) to ensure
cookies are only sent over secure connections in production.
coderabbitai bot
added a commit
that referenced
this pull request
Feb 23, 2026
Docstrings generation was requested by @jasutiin. * #1 (comment) The following files were modified: * `cmd/server/main.go` * `internal/cli/commands/register.go` * `internal/cli/root.go` * `internal/cli/utils/exchange.go` * `internal/server/api/auth/auth.go` * `internal/server/api/auth/controllers.go` * `internal/server/api/auth/router.go` * `internal/server/database/database.go` * `internal/server/utils/exchange.go`
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
New Features
Tests
Chores