Conversation
adilsitos
force-pushed
the
adilsitos/feat/ENG-4525
branch
from
aef3f54 to
b8c534a
Compare
adilsitos
force-pushed
the
adilsitos/feat/ENG-4525
branch
3 times, most recently
from
a015759 to
10a2ab1
Compare
…us events Co-Authored-By: arsh <arshsb1998@gmail.com>
…login event capture Co-Authored-By: arsh <arshsb1998@gmail.com>
…/Alias events are flushed Co-Authored-By: arsh <arshsb1998@gmail.com>
Previously, GetDistinctId returned errors from machineid.ID() even when a valid email-based distinctId was available from the config file. This caused CaptureEvent to early-return and never flush enqueued Identify/ Alias events. Now non-critical errors are logged at debug level and only a missing distinctId produces an error. Co-Authored-By: arsh <arshsb1998@gmail.com>
Co-Authored-By: arsh <arshsb1998@gmail.com>
…only specific envs
remove unecessary code
adilsitos
force-pushed
the
adilsitos/feat/ENG-4525
branch
from
10a2ab1 to
ba3ab79
Compare
Greptile SummaryThis PR adds two distinct features: (1) SSE-based cache invalidation for the proxy — when The core logic of both features is well-structured, but the PR has several issues that need to be resolved before merging:
Confidence Score: 2/5Not safe to merge — encrypted session binaries and a developer-local config file were accidentally committed, and there is a debug log and a context-less HTTP request in the SSE implementation. Two P0/P1 file inclusion issues (committed session binaries and local settings.json) must be resolved before merge. The debug log and missing context on the refetch request are correctness/reliability issues that also block production readiness. The remaining findings are P2 style/security hygiene items. session/*.enc (must be removed), .claude/settings.json (must be removed), packages/proxy/sse.go (debug log at line 403, context-less request at line 566) Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| case <-ctx.Done(): | ||
| return ctx.Err() | ||
| case event, ok := <-events: | ||
| log.Info().Str("event", fmt.Sprintf("%+v", event)).Msg("DEBUG") |
There was a problem hiding this comment.
This "DEBUG" log message at Info level is clearly a development artifact that should be removed before merging. It logs every SSE event (including sensitive event metadata like project ID, environment, and secret key) to stdout at info severity on every single event received.
| log.Info().Str("event", fmt.Sprintf("%+v", event)).Msg("DEBUG") | |
| if !ok { |
| } | ||
| secretURL.RawQuery = query.Encode() | ||
|
|
||
| req, err := http.NewRequest(http.MethodGet, secretURL.String(), nil) |
There was a problem hiding this comment.
getSecretByName creates the request with http.NewRequest instead of http.NewRequestWithContext, meaning it will never be cancelled even when the parent SSE connection context is cancelled (e.g. on shutdown or when the connection is dropped). This request could hang indefinitely, leaking goroutines in error scenarios.
| req, err := http.NewRequest(http.MethodGet, secretURL.String(), nil) | |
| req, err := http.NewRequestWithContext(ctx, http.MethodGet, secretURL.String(), nil) |
Note: you'll need to pass ctx context.Context into getSecretByName and thread it through from refetchSecretsAfterSSEEvent.
| Str("rawData", currentData.String()). | ||
| Msg("Failed to parse SSE event data") |
There was a problem hiding this comment.
When JSON parsing of an SSE event fails, the raw data payload is logged at Error level via Str("rawData", currentData.String()). Since SSE event data can include secret values or sensitive metadata, logging the entire raw body risks leaking secrets into log aggregation systems (e.g. Datadog, CloudWatch).
Consider logging only the length or a truncated/redacted version instead.
1 participant
Description 📣
Allow support to update secrets using SSE (server sent events)
Type ✨
Tests 🛠️