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
📝 WalkthroughWalkthroughThis PR introduces OAuth authentication support (specifically Google), restructures the CLI command system into a dedicated package, refactors server authentication to support OAuth flows with local CLI exchange code mechanisms, and adds necessary dependencies. Changes span CI/CD, configuration, CLI, server auth, and utilities. Changes
Sequence DiagramssequenceDiagram
actor User
participant CLI as CLI Process
participant LocalSrv as Local HTTP Server
participant Browser as Browser
participant GoogleAuth as Google OAuth
participant API as envlink API
User->>CLI: Run register (Google)
CLI->>CLI: Generate state, start listener on 127.0.0.1:random
CLI->>LocalSrv: CreateLocalServer(callback handler)
CLI->>GoogleAuth: Open auth URL in browser (state param)
CLI->>LocalSrv: Wait for callback result (2 min timeout)
User->>Browser: Authenticate with Google
Browser->>GoogleAuth: Consent
GoogleAuth->>Browser: Redirect to localhost callback
Browser->>LocalSrv: GET /?exchange_code=XXX&state=YYY
LocalSrv->>LocalSrv: Validate state, extract exchange_code
LocalSrv->>CLI: Send CallbackResult via channel
LocalSrv-->>Browser: Success HTML
CLI->>API: POST /api/v1/auth/cli/exchange {exchange_code, state}
API->>API: Validate & consume exchange code
API-->>CLI: {token: access_token}
CLI->>User: Print success + token
sequenceDiagram
actor User
participant CLI as CLI
participant API as envlink API
participant DB as Database
User->>CLI: Run register (Email)
CLI->>User: Prompt email
CLI->>User: Prompt password
User->>CLI: Enter credentials
CLI->>API: POST /api/v1/auth/register {email, password}
API->>DB: repository.Register(email, password)
DB-->>API: Success/Error
API-->>CLI: 2xx or error status
CLI->>User: Print success or failure
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes The diff spans multiple distinct subsystems (CLI restructuring, server auth refactoring, OAuth integration, utilities), introduces new OAuth flows with state management and timeout handling, reorganizes package structure with wide-reaching import changes, and adds non-trivial logic for local HTTP servers, callback validation, and exchange code TTL management. While many changes follow consistent refactoring patterns, the OAuth functionality and restructuring across multiple domains require careful, context-aware review. Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
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.
| 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
+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`
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
internal/server/database/database.go (1)
13-33:⚠️ Potential issue | 🔴 CriticalFail fast on DB open and migration errors instead of silently continuing startup.
The
CreateDB()function (line 29) andAutoMigrate()function (line 36-37) both log errors without returning them. If the database connection fails or migration fails, the application continues with an unusable or partially initialized state, causing runtime failures later.Both functions must return errors and the single call site in
cmd/server/main.gomust handle those errors to stop startup on failure.Proposed fix
-func CreateDB() *gorm.DB { +func CreateDB() (*gorm.DB, error) { err := godotenv.Load() if err != nil { fmt.Println("error loading .env file") } host := os.Getenv("DB_HOST") user := os.Getenv("DB_USER") password := os.Getenv("DB_PASSWORD") name := os.Getenv("DB_NAME") port := os.Getenv("DB_PORT") dsn := fmt.Sprintf("host=%s user=%s password=%s dbname=%s port=%s sslmode=require", host, user, password, name, port) db, err := gorm.Open(postgres.Open(dsn), &gorm.Config{}) if err != nil { - fmt.Println("failed to open db") + return nil, fmt.Errorf("failed to open db: %w", err) } - return db + return db, nil } -func AutoMigrate(db *gorm.DB) { +func AutoMigrate(db *gorm.DB) error { + if db == nil { + return fmt.Errorf("db is nil") + } if err := db.AutoMigrate(&auth.User{}); err != nil { - fmt.Println("migrate failed:", err) + return fmt.Errorf("migrate failed: %w", err) } + return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/database/database.go` around lines 13 - 33, Change CreateDB and AutoMigrate to return errors instead of just logging: update CreateDB signature to return (*gorm.DB, error), replace fmt.Println/implicit continues with returning a descriptive error when godotenv.Load or gorm.Open fails (including the wrapped error), and update AutoMigrate to return error from db.AutoMigrate; then update the caller in cmd/server/main.go to check these returned errors and abort startup (log and os.Exit or return) on failure so the app fails fast on DB connection or migration problems.internal/cli/commands/login.go (2)
49-53:⚠️ Potential issue | 🔴 CriticalMissing return after error causes nil pointer dereference.
When
http.NewRequestfails, the code prints an error but continues execution to line 53 wherereq.Header.Setis called on anilrequest, causing a panic.🐛 Proposed fix
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") + log.Fatalf("Error creating login request: %v", err) }🤖 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 49 - 53, The code calls http.NewRequest and prints an error but does not stop execution, so req can be nil and the subsequent req.Header.Set causes a panic; update the error handling after http.NewRequest to return (or otherwise exit the function) on err instead of only printing, ensuring you do not call req.Header.Set when err != nil—locate the http.NewRequest call and the following req.Header.Set usage and add an early return or proper error propagation using the existing function's error/exit pattern.
44-45:⚠️ Potential issue | 🟠 MajorJSON injection vulnerability via unsanitized user input.
Using
fmt.Sprintfto construct JSON with raw user input allows injection. Iftest"injection), the JSON structure breaks, potentially causing parse errors or unexpected behavior.🛡️ Proposed fix using json.Marshal
+import "encoding/json" + func login() { var email string; var password string; // ... input reading ... - jsonStr := []byte(fmt.Sprintf(`{"email":"%s","password":"%s"}`, email, password)) - payload := bytes.NewBuffer(jsonStr) + loginData := map[string]string{"email": email, "password": password} + jsonStr, err := json.Marshal(loginData) + if err != nil { + log.Fatalf("Error marshalling login data: %v", err) + } + payload := bytes.NewBuffer(jsonStr)🤖 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 - 45, The code builds JSON with fmt.Sprintf using unsanitized email/password (jsonStr := []byte(fmt.Sprintf(...))) which allows injection; replace that with a safe json.Marshal call: create a struct or map like payloadObj := map[string]string{"email": email, "password": password} (or a small struct type), call b, err := json.Marshal(payloadObj), handle the error, then set payload := bytes.NewBuffer(b); update the login function that currently defines jsonStr/payload to use json.Marshal instead to correctly escape inputs.
♻️ Duplicate comments (1)
internal/server/api/auth/controllers.go (1)
33-45:⚠️ Potential issue | 🔴 CriticalDo not return
authRequestBodyfrom successful auth endpoints.Line 45 and Line 60 still serialize
Passwordback to the caller. Return only non-sensitive fields or a generic success payload.🔐 Proposed fix
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, requestBody) + c.IndentedJSON(http.StatusOK, gin.H{"message": "login successful"}) } 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 } - c.IndentedJSON(http.StatusOK, requestBody) + c.IndentedJSON(http.StatusCreated, gin.H{"message": "registration successful"}) }Also applies to: 48-60
🤖 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 33 - 45, The postLogin handler currently returns the entire authRequestBody (including Password) on success; change postLogin (and the similar endpoint that returns authRequestBody) to never serialize Password back to the client — instead return a minimal non-sensitive payload (e.g., user ID/email and/or an auth token or a generic success message). Locate postLogin, the authRequestBody type and the controller.repo.Login call, remove/omit Password from the response, and construct a response struct or gin.H with only safe fields before calling c.IndentedJSON.
🧹 Nitpick comments (4)
internal/server/database/database.go (1)
7-7: Avoid coupling the database layer to the API package model.
internal/server/databaseimportinginternal/server/api/auth.Userties persistence schema to API-layer types (seeinternal/server/api/auth/types.go:1-13). Consider moving the DB model to a dedicated domain/persistence package and mapping API structs separately.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/database/database.go` at line 7, The database package currently imports and uses the API-layer type auth.User which couples persistence to the API; create a new dedicated persistence/domain model (e.g., a DBUser struct in a new package like internal/server/model or internal/server/persistence) and move any DB-specific tags/fields there, update internal/server/database to use that DBUser instead of auth.User, and add mapping functions to convert between DBUser and api auth.User (place converters near the API boundary, e.g., in internal/server/api/auth or the new model package) so the database layer no longer depends on internal/server/api/auth.User.internal/server/api/auth/auth.go (1)
35-42: Consider adding an encryption key for session confidentiality.
NewCookieStorecurrently receives only an authentication key, so session cookies are signed but not encrypted. Session data will be readable (though tamper-proof) in the browser. If session data may contain sensitive information, consider passing a second 32-byte encryption key.💡 Example with encryption key
-func NewAuth(port string, domain string, key string, isProd bool) error { +func NewAuth(port string, domain string, authKey string, encryptKey string, isProd bool) error { // key needs to be 32-bytes as per NewCookieStore - if len(key) < 32 { + if len(authKey) < 32 { return errors.New("session key must be at least 32 bytes") } + if encryptKey != "" && len(encryptKey) != 32 { + return errors.New("encryption key must be exactly 32 bytes for AES-256") + } // ... - store := sessions.NewCookieStore([]byte(key)) + var store *sessions.CookieStore + if encryptKey != "" { + store = sessions.NewCookieStore([]byte(authKey), []byte(encryptKey)) + } else { + store = sessions.NewCookieStore([]byte(authKey)) + }🤖 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 35 - 42, NewCookieStore is only given a single auth key so cookies are signed but not encrypted; if session payloads may contain sensitive data, generate/provide a 32-byte encryption key as the second argument to sessions.NewCookieStore (e.g., pass sessions.NewCookieStore([]byte(authKey), []byte(encKey))) and ensure the encryption key is securely generated/stored and used when creating the store referenced by store (and keep existing store.Options including Secure=isProd). Update any key-loading logic so both keys are derived/loaded (auth key and 32-byte enc key) before calling NewCookieStore.internal/cli/commands/login.go (1)
32-42: Remove debug prints before merging.These conditional prints revealing whether email/password were provided appear to be development artifacts and should be removed.
♻️ Proposed removal
- 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/login.go` around lines 32 - 42, Remove the temporary debug prints that print whether credentials were provided in internal/cli/commands/login.go: delete the two if/else blocks that output "email provided"/"email not provided" and "password provided"/"password not provided" (the checks using the variables email and password) so the command no longer emits development-only messages; ensure no other similar debug fmt.Println calls remain in the same function or file.internal/server/utils/exchange.go (1)
16-85: In-memory exchange store lacks cleanup of expired entries.The
cliExchangeStoreonly removes entries on successful consumption. Expired or unconsumed entries will accumulate indefinitely, causing a memory leak under sustained load or failed auth attempts.Consider adding periodic cleanup:
♻️ Suggested approach
+func (store *cliExchangeStore) cleanup() { + store.mu.Lock() + defer store.mu.Unlock() + now := time.Now() + for code, entry := range store.entries { + if now.After(entry.expiresAt) { + delete(store.entries, code) + } + } +} func newCLIExchangeStore() *cliExchangeStore { - return &cliExchangeStore{entries: make(map[string]cliExchangeEntry)} + store := &cliExchangeStore{entries: make(map[string]cliExchangeEntry)} + go func() { + ticker := time.NewTicker(5 * time.Minute) + for range ticker.C { + store.cleanup() + } + }() + return store }🤖 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 16 - 85, The store currently leaks expired entries because entries are only removed on Consume; add periodic cleanup to cliExchangeStore by implementing a cleanupExpired method that locks store.mu, iterates store.entries and deletes entries whose expiresAt is before time.Now(), run this cleanup on a ticker (e.g., every minute) started from newCLIExchangeStore (or a dedicated startCleanup goroutine) so PendingCLIExchanges automatically prunes expired entries; ensure the ticker goroutine uses the store.mu for deletions and consider exposing a stop mechanism if shutdown is needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/server/.env.example`:
- Around line 5-8: The .env.example is missing the COOKIE_SESSION_KEY variable
required by auth.NewAuth() in cmd/server/main.go; add a line
"COOKIE_SESSION_KEY=" to the example file and include a short comment if desired
explaining it's a random secret used for session signing so reviewers know to
set a secure value before running the server.
In `@internal/cli/commands/register.go`:
- Around line 148-149: The code currently prints the raw OAuth token (the
fmt.Println lines that print "Token:" and token.AccessToken) to stdout; remove
those prints and instead persist the token securely (e.g., call a
SaveToken/StoreCredentials helper or write to the existing config with
restrictive permissions like 0600 or use the OS keyring). Update the
registration flow in the function handling Google auth to only output a success
message ("Google authentication successful.") and invoke the secure storage
routine with the token variable rather than emitting token.AccessToken to the
terminal.
In `@internal/cli/utils/exchange.go`:
- Around line 55-61: The helper BuildServerGoogleAuthURL hardcodes
"http://localhost:8080"; change it to use a configurable server base URL (passed
in or fetched from the CLI config/env) and build the auth endpoint from that
base. Concretely: thread a serverBaseURL into BuildServerGoogleAuthURL (or read
it via a shared GetServerBaseURL helper used by cmd/server/main.go), replace
baseURL := "http://localhost:8080" with the provided serverBaseURL, and update
any other endpoint helpers in this file (the other helper at line ~127) to
construct their URLs from the same serverBaseURL so the CLI works when the API
host/port changes.
- Around line 165-170: The OpenInBrowser helper currently only runs Windows
commands; update OpenInBrowser to detect OS via runtime.GOOS and call the
platform-appropriate opener: use "rundll32" + "url.dll,FileProtocolHandler" /
"cmd /c start" for windows, "open" for darwin (macOS), and "xdg-open" for linux;
ensure you import runtime and build the exec.Command call per-OS and return its
Start() error so the OAuth flow can open the browser on all platforms.
In `@internal/server/api/auth/controllers.go`:
- Around line 72-88: The CLI cookie/context is never cleared on non-CLI starts
or when CompleteUserAuth returns an error, allowing stale CLI flow to persist;
update the auth handler to call serverutils.ClearCLIAuthContext(c) (or
equivalent) whenever cli_callback and cli_state are both empty (i.e., non-CLI
requests) and before every early return after calling CompleteUserAuth (the
error path that currently returns before cleanup), and keep
serverutils.WriteCLIAuthContext(c, cliCallback, cliState) only when both values
are present and valid; this ensures stale CLI cookies are removed on normal
browser auth starts and on callback failures.
- Around line 109-120: The CLI redirect branch currently falls through to the
generic 200 success page when serverutils.NewExchangeCode or
serverutils.BuildCLIRedirectURL fails; update the handler so that if
NewExchangeCode returns an error or BuildCLIRedirectURL returns an error you
immediately return an explicit HTTP error (e.g., c.String or c.JSON with
http.StatusInternalServerError and a short message) instead of continuing to the
success flow. Keep the existing save/clear logic
(serverutils.PendingCLIExchanges.Save and serverutils.ClearCLIAuthContext) only
on successful path and ensure errors from NewExchangeCode or BuildCLIRedirectURL
cause an immediate response to the client so no empty/missing exchange_code is
sent to the CLI.
- Around line 170-173: The getLogoutProvider handler ignores the error returned
by gothic.Logout, causing silent logout failures; update getLogoutProvider to
capture the error from gothic.Logout (in function getLogoutProvider), check if
err != nil, log the error (using the controller's logger or gin.Context), and
return an appropriate error response (e.g., set a 500 or 400 status and JSON
error message) instead of unconditionally setting Location and issuing the
redirect; only set the Location header and write the TemporaryRedirect when
logout succeeds, and ensure any session-store deletion failures are surfaced to
the client or logged for debugging.
- Around line 35-37: Replace calls to c.BindJSON with c.ShouldBindJSON in the
HTTP handlers postLogin, postRegister, and postCLIExchange so binding errors are
returned instead of automatically aborting; update the error checks that follow
each binding (the existing if err != nil { c.IndentedJSON(...); return }) to
remain as-is so you can send custom error responses after ShouldBindJSON returns
the error, referencing the BindJSON occurrences inside postLogin, postRegister,
and postCLIExchange to locate and change them.
---
Outside diff comments:
In `@internal/cli/commands/login.go`:
- Around line 49-53: The code calls http.NewRequest and prints an error but does
not stop execution, so req can be nil and the subsequent req.Header.Set causes a
panic; update the error handling after http.NewRequest to return (or otherwise
exit the function) on err instead of only printing, ensuring you do not call
req.Header.Set when err != nil—locate the http.NewRequest call and the following
req.Header.Set usage and add an early return or proper error propagation using
the existing function's error/exit pattern.
- Around line 44-45: The code builds JSON with fmt.Sprintf using unsanitized
email/password (jsonStr := []byte(fmt.Sprintf(...))) which allows injection;
replace that with a safe json.Marshal call: create a struct or map like
payloadObj := map[string]string{"email": email, "password": password} (or a
small struct type), call b, err := json.Marshal(payloadObj), handle the error,
then set payload := bytes.NewBuffer(b); update the login function that currently
defines jsonStr/payload to use json.Marshal instead to correctly escape inputs.
In `@internal/server/database/database.go`:
- Around line 13-33: Change CreateDB and AutoMigrate to return errors instead of
just logging: update CreateDB signature to return (*gorm.DB, error), replace
fmt.Println/implicit continues with returning a descriptive error when
godotenv.Load or gorm.Open fails (including the wrapped error), and update
AutoMigrate to return error from db.AutoMigrate; then update the caller in
cmd/server/main.go to check these returned errors and abort startup (log and
os.Exit or return) on failure so the app fails fast on DB connection or
migration problems.
---
Duplicate comments:
In `@internal/server/api/auth/controllers.go`:
- Around line 33-45: The postLogin handler currently returns the entire
authRequestBody (including Password) on success; change postLogin (and the
similar endpoint that returns authRequestBody) to never serialize Password back
to the client — instead return a minimal non-sensitive payload (e.g., user
ID/email and/or an auth token or a generic success message). Locate postLogin,
the authRequestBody type and the controller.repo.Login call, remove/omit
Password from the response, and construct a response struct or gin.H with only
safe fields before calling c.IndentedJSON.
---
Nitpick comments:
In `@internal/cli/commands/login.go`:
- Around line 32-42: Remove the temporary debug prints that print whether
credentials were provided in internal/cli/commands/login.go: delete the two
if/else blocks that output "email provided"/"email not provided" and "password
provided"/"password not provided" (the checks using the variables email and
password) so the command no longer emits development-only messages; ensure no
other similar debug fmt.Println calls remain in the same function or file.
In `@internal/server/api/auth/auth.go`:
- Around line 35-42: NewCookieStore is only given a single auth key so cookies
are signed but not encrypted; if session payloads may contain sensitive data,
generate/provide a 32-byte encryption key as the second argument to
sessions.NewCookieStore (e.g., pass sessions.NewCookieStore([]byte(authKey),
[]byte(encKey))) and ensure the encryption key is securely generated/stored and
used when creating the store referenced by store (and keep existing
store.Options including Secure=isProd). Update any key-loading logic so both
keys are derived/loaded (auth key and 32-byte enc key) before calling
NewCookieStore.
In `@internal/server/database/database.go`:
- Line 7: The database package currently imports and uses the API-layer type
auth.User which couples persistence to the API; create a new dedicated
persistence/domain model (e.g., a DBUser struct in a new package like
internal/server/model or internal/server/persistence) and move any DB-specific
tags/fields there, update internal/server/database to use that DBUser instead of
auth.User, and add mapping functions to convert between DBUser and api auth.User
(place converters near the API boundary, e.g., in internal/server/api/auth or
the new model package) so the database layer no longer depends on
internal/server/api/auth.User.
In `@internal/server/utils/exchange.go`:
- Around line 16-85: The store currently leaks expired entries because entries
are only removed on Consume; add periodic cleanup to cliExchangeStore by
implementing a cleanupExpired method that locks store.mu, iterates store.entries
and deletes entries whose expiresAt is before time.Now(), run this cleanup on a
ticker (e.g., every minute) started from newCLIExchangeStore (or a dedicated
startCleanup goroutine) so PendingCLIExchanges automatically prunes expired
entries; ensure the ticker goroutine uses the store.mu for deletions and
consider exposing a stop mechanism if shutdown is needed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e2cf8894-f99f-4c2e-a4d9-3a0114b540ca
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (29)
.github/workflows/test.ymlcmd/server/.env.examplecmd/server/main.gogo.modinternal/cli/commands/login.gointernal/cli/commands/projects.gointernal/cli/commands/pull.gointernal/cli/commands/push.gointernal/cli/commands/register.gointernal/cli/commands/store.gointernal/cli/register.gointernal/cli/root.gointernal/cli/utils/exchange.gointernal/cli/utils/exchange_test.gointernal/server/api/auth/auth.gointernal/server/api/auth/controllers.gointernal/server/api/auth/repository.gointernal/server/api/auth/router.gointernal/server/api/auth/types.gointernal/server/api/projects/controllers.gointernal/server/api/projects/router.gointernal/server/api/pull/controllers.gointernal/server/api/pull/router.gointernal/server/api/push/controllers.gointernal/server/api/push/router.gointernal/server/auth/controllers.gointernal/server/database/database.gointernal/server/utils/exchange.gointernal/server/utils/exchange_test.go
💤 Files with no reviewable changes (2)
- internal/server/auth/controllers.go
- internal/cli/register.go
Comment on lines
+5
to
+8
| DB_PORT= | ||
| PORT= | ||
| GOOGLE_CLIENT_ID= | ||
| GOOGLE_CLIENT_SECRET= No newline at end of file |
There was a problem hiding this comment.
Add missing COOKIE_SESSION_KEY environment variable.
The cmd/server/main.go requires COOKIE_SESSION_KEY for auth.NewAuth(), but it's not documented in this example file.
📝 Proposed addition
DB_PORT=
PORT=
+COOKIE_SESSION_KEY=
GOOGLE_CLIENT_ID=
GOOGLE_CLIENT_SECRET=🧰 Tools
🪛 dotenv-linter (4.0.0)
[warning] 5-5: [UnorderedKey] The DB_PORT key should go before the DB_USER key
(UnorderedKey)
[warning] 7-7: [UnorderedKey] The GOOGLE_CLIENT_ID key should go before the PORT key
(UnorderedKey)
[warning] 8-8: [EndingBlankLine] No blank line at the end of the file
(EndingBlankLine)
[warning] 8-8: [UnorderedKey] The GOOGLE_CLIENT_SECRET key should go before the PORT key
(UnorderedKey)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/server/.env.example` around lines 5 - 8, The .env.example is missing the
COOKIE_SESSION_KEY variable required by auth.NewAuth() in cmd/server/main.go;
add a line "COOKIE_SESSION_KEY=" to the example file and include a short comment
if desired explaining it's a random secret used for session signing so reviewers
know to set a secure value before running the server.
Comment on lines
+148
to
+149
| fmt.Println("Google authentication successful.") | ||
| fmt.Println("Token:", token.AccessToken) |
There was a problem hiding this comment.
Reconsider printing access token to stdout.
Printing the raw access token to the terminal poses a security risk:
- Tokens may be captured in terminal history or logs
- Shoulder surfing exposure
- Accidental copy/paste into insecure channels
Consider storing the token securely (e.g., in a config file with restricted permissions) and displaying only a success message.
🤖 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 148 - 149, The code currently
prints the raw OAuth token (the fmt.Println lines that print "Token:" and
token.AccessToken) to stdout; remove those prints and instead persist the token
securely (e.g., call a SaveToken/StoreCredentials helper or write to the
existing config with restrictive permissions like 0600 or use the OS keyring).
Update the registration flow in the function handling Google auth to only output
a success message ("Google authentication successful.") and invoke the secure
storage routine with the token variable rather than emitting token.AccessToken
to the terminal.
Comment on lines
+55
to
+61
| func BuildServerGoogleAuthURL(callbackURL, state string) string { | ||
| baseURL := "http://localhost:8080/api/v1/auth/google" | ||
| values := url.Values{} | ||
| values.Set("cli_callback", callbackURL) | ||
| values.Set("cli_state", state) | ||
|
|
||
| return baseURL + "?" + values.Encode() |
There was a problem hiding this comment.
Don't hardcode the API base URL in the CLI flow.
cmd/server/main.go (Lines 28-51) already makes the server host and port configurable, but these helpers always talk to http://localhost:8080. The auth flow breaks as soon as the API runs anywhere else. Thread the server base URL in from CLI config or environment and build both endpoints from it.
Also applies to: 127-127
🤖 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 - 61, The helper
BuildServerGoogleAuthURL hardcodes "http://localhost:8080"; change it to use a
configurable server base URL (passed in or fetched from the CLI config/env) and
build the auth endpoint from that base. Concretely: thread a serverBaseURL into
BuildServerGoogleAuthURL (or read it via a shared GetServerBaseURL helper used
by cmd/server/main.go), replace baseURL := "http://localhost:8080" with the
provided serverBaseURL, and update any other endpoint helpers in this file (the
other helper at line ~127) to construct their URLs from the same serverBaseURL
so the CLI works when the API host/port changes.
Comment on lines
+165
to
+170
| func OpenInBrowser(targetURL string) error { | ||
| if err := exec.Command("rundll32", "url.dll,FileProtocolHandler", targetURL).Start(); err == nil { | ||
| return nil | ||
| } | ||
|
|
||
| return exec.Command("cmd", "/c", "start", "", fmt.Sprintf("\"%s\"", targetURL)).Start() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "exchange.go" -type fRepository: jasutiin/envlink
Length of output: 128
🏁 Script executed:
wc -l ./internal/cli/utils/exchange.goRepository: jasutiin/envlink
Length of output: 96
🏁 Script executed:
cat -n ./internal/cli/utils/exchange.goRepository: jasutiin/envlink
Length of output: 6878
Make browser launching portable across platforms.
This helper only supports Windows commands (rundll32 and cmd). On macOS and Linux, both fallback attempts will fail because these commands don't exist on those systems, causing the OAuth flow to return an error instead of opening the browser. Use platform-specific commands: open for macOS (darwin), xdg-open for Linux, and the existing commands for Windows. Import the runtime package to check runtime.GOOS for OS detection.
🤖 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 165 - 170, The OpenInBrowser
helper currently only runs Windows commands; update OpenInBrowser to detect OS
via runtime.GOOS and call the platform-appropriate opener: use "rundll32" +
"url.dll,FileProtocolHandler" / "cmd /c start" for windows, "open" for darwin
(macOS), and "xdg-open" for linux; ensure you import runtime and build the
exec.Command call per-OS and return its Start() error so the OAuth flow can open
the browser on all platforms.
Comment on lines
+35
to
+37
| if err := c.BindJSON(&requestBody); err != nil { | ||
| c.IndentedJSON(http.StatusBadRequest, gin.H{"error": "invalid request body"}) | ||
| return |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n internal/server/api/auth/controllers.go | head -160Repository: jasutiin/envlink
Length of output: 6840
🌐 Web query:
Gin BindJSON vs ShouldBindJSON documentation behavior
💡 Result:
In the Gin web framework, c.BindJSON(obj) and c.ShouldBindJSON(obj) both perform JSON unmarshaling from the request body into the given object with validation, but differ in error handling: - BindJSON (part of "Must bind" methods): Uses MustBindWith internally. On binding/validation error, it automatically calls c.AbortWithError(400, err).SetType(ErrorTypeBind), aborting the request, setting status 400, and writing a plain text error response. The developer cannot send further responses without warnings (e.g., headers already written). - ShouldBindJSON (part of "Should bind" methods): Uses ShouldBindWith internally. On error, it simply returns the error. The developer must manually handle it (e.g., if err != nil { c.JSON(400, gin.H{"error": err.Error}); return }), allowing custom error responses like JSON. Both consume the request body (cannot be called twice without ShouldBindBodyWith). Prefer ShouldBindJSON for custom error control, as recommended in docs. BindJSON strictly assumes JSON; non-JSON Content-Type fails with 400. Examples: var json Login if err := c.ShouldBindJSON(&json); err != nil { c.JSON(http.StatusBadRequest, gin.H{"error": err.Error}) return } // proceed This behavior is consistent across Gin v1.10+ to v1.12 (latest as of 2026).
Citations:
- 1: https://gin-gonic.com/en/docs/binding/binding-and-validation/
- 2: https://gin-gonic.com/en/docs/binding/
- 3: https://github.com/gin-gonic/gin/blob/master/binding/json.go
- 4: https://github.com/gin-gonic/gin/blob/3e44fdc4/binding/binding.go
Replace BindJSON with ShouldBindJSON to allow custom error responses.
When BindJSON encounters a binding error, it automatically calls AbortWithError(400, err), which writes a response and aborts the request. Attempting to write another response afterward causes issues. ShouldBindJSON returns the error without aborting, allowing you to send custom error responses.
Apply this change in postLogin (line 35), postRegister (line 50), and postCLIExchange (line 148).
Proposed fix
- if err := c.BindJSON(&requestBody); err != nil {
+ if err := c.ShouldBindJSON(&requestBody); err != nil {
c.IndentedJSON(http.StatusBadRequest, gin.H{"error": "invalid request body"})
return
}📝 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 err := c.BindJSON(&requestBody); err != nil { | |
| c.IndentedJSON(http.StatusBadRequest, gin.H{"error": "invalid request body"}) | |
| return | |
| if err := c.ShouldBindJSON(&requestBody); err != nil { | |
| c.IndentedJSON(http.StatusBadRequest, gin.H{"error": "invalid request body"}) | |
| return |
🤖 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 35 - 37, Replace calls
to c.BindJSON with c.ShouldBindJSON in the HTTP handlers postLogin,
postRegister, and postCLIExchange so binding errors are returned instead of
automatically aborting; update the error checks that follow each binding (the
existing if err != nil { c.IndentedJSON(...); return }) to remain as-is so you
can send custom error responses after ShouldBindJSON returns the error,
referencing the BindJSON occurrences inside postLogin, postRegister, and
postCLIExchange to locate and change them.
Comment on lines
+72
to
+88
| cliCallback := strings.TrimSpace(c.Query("cli_callback")) | ||
| cliState := strings.TrimSpace(c.Query("cli_state")) | ||
|
|
||
| if cliCallback != "" || cliState != "" { | ||
| if cliCallback == "" || cliState == "" { | ||
| c.JSON(http.StatusBadRequest, gin.H{"error": "cli_callback and cli_state are required together"}) | ||
| return | ||
| } | ||
|
|
||
| if !serverutils.IsAllowedCLICallback(cliCallback) { | ||
| c.JSON(http.StatusBadRequest, gin.H{"error": "invalid cli_callback"}) | ||
| return | ||
| } | ||
|
|
||
| // create httpOnly cookies to use on subsequent requests | ||
| serverutils.WriteCLIAuthContext(c, cliCallback, cliState) | ||
| } |
There was a problem hiding this comment.
Clear stale CLI auth context on non-CLI starts and callback failures.
A cancelled CLI attempt can leave callback context behind. Because a plain /auth/:provider request never clears it, and Line 105 returns before cleanup on CompleteUserAuth errors, the next successful browser auth can be misclassified as a CLI flow at Line 109 and redirected to an old callback.
🧹 Proposed fix
func (controller *AuthController) getAuthProvider(c *gin.Context) {
provider := c.Param("provider")
c.Request = c.Request.WithContext(context.WithValue(c.Request.Context(), "provider", provider))
cliCallback := strings.TrimSpace(c.Query("cli_callback"))
cliState := strings.TrimSpace(c.Query("cli_state"))
if cliCallback != "" || cliState != "" {
if cliCallback == "" || cliState == "" {
c.JSON(http.StatusBadRequest, gin.H{"error": "cli_callback and cli_state are required together"})
return
}
if !serverutils.IsAllowedCLICallback(cliCallback) {
c.JSON(http.StatusBadRequest, gin.H{"error": "invalid cli_callback"})
return
}
serverutils.WriteCLIAuthContext(c, cliCallback, cliState)
+ } else {
+ serverutils.ClearCLIAuthContext(c)
}
gothic.BeginAuthHandler(c.Writer, c.Request)
}
func (controller *AuthController) getAuthCallbackFunction(c *gin.Context) {
provider := c.Param("provider")
c.Request = c.Request.WithContext(context.WithValue(c.Request.Context(), "provider", provider))
user, err := gothic.CompleteUserAuth(c.Writer, c.Request)
if err != nil {
+ serverutils.ClearCLIAuthContext(c)
c.Data(http.StatusBadRequest, "text/html; charset=utf-8", []byte("<h1>Authentication failed</h1><p>Please return to your CLI and try again.</p>"))
return
}Also applies to: 103-106
🤖 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 72 - 88, The CLI
cookie/context is never cleared on non-CLI starts or when CompleteUserAuth
returns an error, allowing stale CLI flow to persist; update the auth handler to
call serverutils.ClearCLIAuthContext(c) (or equivalent) whenever cli_callback
and cli_state are both empty (i.e., non-CLI requests) and before every early
return after calling CompleteUserAuth (the error path that currently returns
before cleanup), and keep serverutils.WriteCLIAuthContext(c, cliCallback,
cliState) only when both values are present and valid; this ensures stale CLI
cookies are removed on normal browser auth starts and on callback failures.
Comment on lines
+109
to
+120
| if callbackURL, callbackState, found := serverutils.ReadCLIAuthContext(c); found { | ||
| exchangeCode, codeErr := serverutils.NewExchangeCode() | ||
| if codeErr == nil { | ||
| // save all of this information on the server so we can refer back to it later | ||
| serverutils.PendingCLIExchanges.Save(exchangeCode, callbackState, user.AccessToken, serverutils.CLIExchangeTTL) | ||
| if redirectURL, redirectErr := serverutils.BuildCLIRedirectURL(callbackURL, exchangeCode, callbackState); redirectErr == nil { | ||
| serverutils.ClearCLIAuthContext(c) | ||
| c.Redirect(http.StatusFound, redirectURL) | ||
| return | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Don't fall through to the success page when CLI redirect setup fails.
If NewExchangeCode or BuildCLIRedirectURL fails, this handler reaches the generic 200 success page even though no exchange_code was delivered to the CLI. Return an explicit failure instead.
🚦 Proposed fix
if callbackURL, callbackState, found := serverutils.ReadCLIAuthContext(c); found {
exchangeCode, codeErr := serverutils.NewExchangeCode()
- if codeErr == nil {
- // save all of this information on the server so we can refer back to it later
- serverutils.PendingCLIExchanges.Save(exchangeCode, callbackState, user.AccessToken, serverutils.CLIExchangeTTL)
- if redirectURL, redirectErr := serverutils.BuildCLIRedirectURL(callbackURL, exchangeCode, callbackState); redirectErr == nil {
- serverutils.ClearCLIAuthContext(c)
- c.Redirect(http.StatusFound, redirectURL)
- return
- }
+ if codeErr != nil {
+ serverutils.ClearCLIAuthContext(c)
+ c.Data(http.StatusInternalServerError, "text/html; charset=utf-8", []byte("<h1>Authentication failed</h1><p>Please return to your CLI and try again.</p>"))
+ return
+ }
+
+ serverutils.PendingCLIExchanges.Save(exchangeCode, callbackState, user.AccessToken, serverutils.CLIExchangeTTL)
+
+ redirectURL, redirectErr := serverutils.BuildCLIRedirectURL(callbackURL, exchangeCode, callbackState)
+ if redirectErr != nil {
+ serverutils.ClearCLIAuthContext(c)
+ c.Data(http.StatusInternalServerError, "text/html; charset=utf-8", []byte("<h1>Authentication failed</h1><p>Please return to your CLI and try again.</p>"))
+ return
}
+
+ serverutils.ClearCLIAuthContext(c)
+ c.Redirect(http.StatusFound, redirectURL)
+ return
}📝 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 callbackURL, callbackState, found := serverutils.ReadCLIAuthContext(c); found { | |
| exchangeCode, codeErr := serverutils.NewExchangeCode() | |
| if codeErr == nil { | |
| // save all of this information on the server so we can refer back to it later | |
| serverutils.PendingCLIExchanges.Save(exchangeCode, callbackState, user.AccessToken, serverutils.CLIExchangeTTL) | |
| if redirectURL, redirectErr := serverutils.BuildCLIRedirectURL(callbackURL, exchangeCode, callbackState); redirectErr == nil { | |
| serverutils.ClearCLIAuthContext(c) | |
| c.Redirect(http.StatusFound, redirectURL) | |
| return | |
| } | |
| } | |
| } | |
| if callbackURL, callbackState, found := serverutils.ReadCLIAuthContext(c); found { | |
| exchangeCode, codeErr := serverutils.NewExchangeCode() | |
| if codeErr != nil { | |
| serverutils.ClearCLIAuthContext(c) | |
| c.Data(http.StatusInternalServerError, "text/html; charset=utf-8", []byte("<h1>Authentication failed</h1><p>Please return to your CLI and try again.</p>")) | |
| return | |
| } | |
| serverutils.PendingCLIExchanges.Save(exchangeCode, callbackState, user.AccessToken, serverutils.CLIExchangeTTL) | |
| redirectURL, redirectErr := serverutils.BuildCLIRedirectURL(callbackURL, exchangeCode, callbackState) | |
| if redirectErr != nil { | |
| serverutils.ClearCLIAuthContext(c) | |
| c.Data(http.StatusInternalServerError, "text/html; charset=utf-8", []byte("<h1>Authentication failed</h1><p>Please return to your CLI and try again.</p>")) | |
| return | |
| } | |
| serverutils.ClearCLIAuthContext(c) | |
| c.Redirect(http.StatusFound, redirectURL) | |
| return | |
| } |
🤖 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 109 - 120, The CLI
redirect branch currently falls through to the generic 200 success page when
serverutils.NewExchangeCode or serverutils.BuildCLIRedirectURL fails; update the
handler so that if NewExchangeCode returns an error or BuildCLIRedirectURL
returns an error you immediately return an explicit HTTP error (e.g., c.String
or c.JSON with http.StatusInternalServerError and a short message) instead of
continuing to the success flow. Keep the existing save/clear logic
(serverutils.PendingCLIExchanges.Save and serverutils.ClearCLIAuthContext) only
on successful path and ensure errors from NewExchangeCode or BuildCLIRedirectURL
cause an immediate response to the client so no empty/missing exchange_code is
sent to the CLI.
Comment on lines
+170
to
+173
| func (controller *AuthController) getLogoutProvider(c *gin.Context) { | ||
| gothic.Logout(c.Writer, c.Request) | ||
| c.Writer.Header().Set("Location", "/") | ||
| c.Writer.WriteHeader(http.StatusTemporaryRedirect) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n internal/server/api/auth/controllers.go | sed -n '165,180p'Repository: jasutiin/envlink
Length of output: 382
🌐 Web query:
github.com/markbates/goth/gothic Logout function signature
💡 Result:
The Logout function in github.com/markbates/goth/gothic has the signature: func Logout(res http.ResponseWriter, req *http.Request) error It invalidates a user session by clearing the gothic session store.
Citations:
- 1: https://pkg.go.dev/github.com/markbates/goth/gothic
- 2: https://github.com/markbates/goth/blob/master/examples/main.go
Handle gothic.Logout errors to prevent silent logout failures.
gothic.Logout returns an error, but the handler ignores it and always redirects as if the logout succeeded. This can silently leave a user logged in when session-store access or deletion fails.
↩️ Proposed fix
func (controller *AuthController) getLogoutProvider(c *gin.Context) {
- gothic.Logout(c.Writer, c.Request)
- c.Writer.Header().Set("Location", "/")
- c.Writer.WriteHeader(http.StatusTemporaryRedirect)
+ if err := gothic.Logout(c.Writer, c.Request); err != nil {
+ c.JSON(http.StatusInternalServerError, gin.H{"error": "failed to logout"})
+ return
+ }
+
+ c.Redirect(http.StatusTemporaryRedirect, "/")
}📝 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) getLogoutProvider(c *gin.Context) { | |
| gothic.Logout(c.Writer, c.Request) | |
| c.Writer.Header().Set("Location", "/") | |
| c.Writer.WriteHeader(http.StatusTemporaryRedirect) | |
| func (controller *AuthController) getLogoutProvider(c *gin.Context) { | |
| if err := gothic.Logout(c.Writer, c.Request); err != nil { | |
| c.JSON(http.StatusInternalServerError, gin.H{"error": "failed to logout"}) | |
| return | |
| } | |
| c.Redirect(http.StatusTemporaryRedirect, "/") | |
| } |
🤖 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 170 - 173, The
getLogoutProvider handler ignores the error returned by gothic.Logout, causing
silent logout failures; update getLogoutProvider to capture the error from
gothic.Logout (in function getLogoutProvider), check if err != nil, log the
error (using the controller's logger or gin.Context), and return an appropriate
error response (e.g., set a 500 or 400 status and JSON error message) instead of
unconditionally setting Location and issuing the redirect; only set the Location
header and write the TemporaryRedirect when logout succeeds, and ensure any
session-store deletion failures are surfaced to the client or logged for
debugging.
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
New Features
Tests
Chores