✨refactor: remove userID, add external secure hasher to Auth API call#2293
✨refactor: remove userID, add external secure hasher to Auth API call#2293
Conversation
There was a problem hiding this comment.
Pull request overview
Refactors the Egg Inc authenticated API call path by removing the explicit userID parameter and introducing an external “secure hasher” binary to generate the AuthenticatedMessage.code for requests.
Changes:
- Added
getSecureHash()which shells out to./secure_hasherto compute the authcode. - Updated
APIAuthenticatedCallsignature and changed how it populatesAuthenticatedMessage(removesuser_id, setscode, and changesmessagecontents). - Updated leaderboard API usage to the new
APIAuthenticatedCallsignature.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
src/ei/ei_api.go |
Introduces external-hasher execution and changes AuthenticatedMessage request-envelope construction. |
src/ei/ei_api_leaderboard.go |
Updates call site to match the new APIAuthenticatedCall signature. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| binaryPath := "./secure_hasher" | ||
| info, err := os.Stat(binaryPath) | ||
| if err != nil { | ||
| if os.IsNotExist(err) { | ||
| return "", fmt.Errorf("secure hasher binary not found at %s", binaryPath) | ||
| } | ||
| return "", fmt.Errorf("failed to stat secure hasher binary: %w", err) | ||
| } | ||
| if info.IsDir() { | ||
| return "", fmt.Errorf("secure hasher path is a directory: %s", binaryPath) | ||
| } |
There was a problem hiding this comment.
getSecureHash hard-codes a relative path (./secure_hasher). This is brittle in production (depends on current working directory) and makes deployments harder to reason about. Consider making the hasher path configurable (config/env/flag) and/or resolving it to an absolute path once at startup, rather than re-stat'ing a relative path on every call.
| cmd := exec.Command(binaryPath) | ||
|
|
||
| // Pipe the raw bytes into the binary's stdin | ||
| cmd.Stdin = bytes.NewReader(data) | ||
|
|
||
| var out bytes.Buffer | ||
| cmd.Stdout = &out | ||
|
|
||
| if err := cmd.Run(); err != nil { | ||
| return "", fmt.Errorf("failed to execute hasher: %w", err) | ||
| } |
There was a problem hiding this comment.
exec.Command(binaryPath) is run without any timeout/cancellation. If the external hasher hangs (or blocks on stdin), APIAuthenticatedCall will hang the request path. Use exec.CommandContext with a reasonable timeout (e.g., a few seconds) so the API call can fail fast and the bot remains responsive.
| if err := cmd.Run(); err != nil { | ||
| return "", fmt.Errorf("failed to execute hasher: %w", err) |
There was a problem hiding this comment.
On hasher failures, the error returned from cmd.Run() won't include any stderr output, which can make diagnosing deployment issues difficult. Consider capturing cmd.Stderr (buffer) and including it in the returned error when execution fails.
| if err := cmd.Run(); err != nil { | |
| return "", fmt.Errorf("failed to execute hasher: %w", err) | |
| var errBuf bytes.Buffer | |
| cmd.Stderr = &errBuf | |
| if err := cmd.Run(); err != nil { | |
| return "", fmt.Errorf("failed to execute hasher: %w; stderr: %s", err, errBuf.String()) |
| return "", fmt.Errorf("failed to execute hasher: %w", err) | ||
| } | ||
|
|
||
| return strings.TrimSpace(out.String()), nil |
There was a problem hiding this comment.
getSecureHash returns whatever the external process writes to stdout after trimming whitespace, but APIAuthenticatedCall assumes it's a valid auth code. Add basic validation (e.g., non-empty, expected hex length/charset) and return an error if the output is malformed to avoid sending bad auth requests upstream.
| return strings.TrimSpace(out.String()), nil | |
| hash := strings.TrimSpace(out.String()) | |
| if hash == "" { | |
| return "", fmt.Errorf("secure hasher returned empty output") | |
| } | |
| // Validate that the output looks like a hex-encoded hash (e.g., SHA-256). | |
| for i := 0; i < len(hash); i++ { | |
| c := hash[i] | |
| if !((c >= '0' && c <= '9') || | |
| (c >= 'a' && c <= 'f') || | |
| (c >= 'A' && c <= 'F')) { | |
| return "", fmt.Errorf("secure hasher returned non-hex character %q", c) | |
| } | |
| } | |
| // Enforce an expected hash length (64 characters is typical for SHA-256). | |
| if len(hash) != 64 { | |
| return "", fmt.Errorf("secure hasher returned unexpected length %d", len(hash)) | |
| } | |
| return hash, nil |
| authMsg := &AuthenticatedMessage{ | ||
| Message: []byte(enc.EncodeToString(innerBin)), | ||
| UserId: &userID, | ||
| Message: innerBin, | ||
| Code: &secureHash, | ||
| } |
There was a problem hiding this comment.
This change alters the request envelope semantics: AuthenticatedMessage.Message used to contain base64-encoded innerBin and UserId was populated; now the raw protobuf bytes are sent and user_id is omitted in favor of code. Because this is an external API contract, please ensure the server expects raw message bytes (not base64 text) and does not require user_id; otherwise these calls will fail at runtime. If both formats must be supported, consider keeping backward-compatible fields or gating behavior by endpoint/version.
| @@ -517,9 +546,15 @@ func APIAuthenticatedCall(reqURL string, userID string, request proto.Message) [ | |||
| return nil | |||
| } | |||
|
|
|||
| secureHash, err := getSecureHash(innerBin) | |||
| if err != nil { | |||
| log.Print(err) | |||
| return nil | |||
| } | |||
|
|
|||
| authMsg := &AuthenticatedMessage{ | |||
| Message: []byte(enc.EncodeToString(innerBin)), | |||
| UserId: &userID, | |||
| Message: innerBin, | |||
| Code: &secureHash, | |||
| } | |||
There was a problem hiding this comment.
APIAuthenticatedCall now depends on executing an external binary (secure_hasher), but there are no tests covering the request-envelope creation (hash generation + AuthenticatedMessage fields). Since there are existing tests in this package, consider adding a unit test that verifies APIAuthenticatedCall builds the correct AuthenticatedMessage (ideally by injecting/mocking the hash function so tests don't shell out).
No description provided.