-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Subaru Solterra support #24269
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Subaru Solterra support #24269
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there - I've reviewed your changes - here's some feedback:
Blocking issues:
- Detected a Generic API Key, potentially exposing access to various services and sensitive operations. (link)
General comments:
- Avoid embedding client secrets (AppAuthorization, ApiKey, ClientRefKey) directly in source—consider loading these from configuration or environment variables.
- The recursive authenticate method has no depth limit—add a guard or error condition to prevent potential infinite recursion if the auth flow doesn’t progress as expected.
- There’s a lot of overlap between the Subaru and Toyota OAuth flows—extract shared logic into a common package to reduce duplication and ease future maintenance.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Avoid embedding client secrets (AppAuthorization, ApiKey, ClientRefKey) directly in source—consider loading these from configuration or environment variables.
- The recursive authenticate method has no depth limit—add a guard or error condition to prevent potential infinite recursion if the auth flow doesn’t progress as expected.
- There’s a lot of overlap between the Subaru and Toyota OAuth flows—extract shared logic into a common package to reduce duplication and ease future maintenance.
## Individual Comments
### Comment 1
<location> `vehicle/subaru/identity.go:38` </location>
<code_context>
+ }
+}
+
+func (v *Identity) authenticate(auth Auth, user, password string, passwordSet bool) (*Token, error) {
+ uri := fmt.Sprintf("%s/%s", BaseUrl, AuthenticationPath)
+
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring the `authenticate` function by splitting it into helper functions for callback handling and request processing, and replacing recursion with a loop.
Consider breaking `authenticate` into three focused helpers—one to apply credentials to callbacks, one to send the request and parse either an `Auth` or `Token` response, and a simple loop in place of recursion. For example:
```go
// apply user/password to callbacks, return updated passwordSet
func applyCallbacks(auth *Auth, user, pass string, pwdSet bool) bool {
for i, cb := range auth.Callbacks {
switch cb.Type {
case "NameCallback":
if out, ok := cb.Output[0].Value.(string); ok && out == "User Name" {
auth.Callbacks[i].Input[0].Value = user
}
case "PasswordCallback":
auth.Callbacks[i].Input[0].Value = pass
pwdSet = true
}
}
return pwdSet
}
```
```go
// send auth request, return either a token or the next Auth step
func (v *Identity) sendStep(auth Auth, pwdSet bool) (*Token, *Auth, error) {
uri := fmt.Sprintf("%s/%s", BaseUrl, AuthenticationPath)
req, err := request.New(http.MethodPost, uri, request.MarshalJSON(auth), request.JSONEncoding)
if err != nil {
return nil, nil, err
}
if pwdSet {
var tok Token
if err := v.DoJSON(req, &tok); err != nil {
return nil, nil, err
}
return &tok, nil, nil
}
var next Auth
if err := v.DoJSON(req, &next); err != nil {
return nil, nil, err
}
return nil, &next, nil
}
```
```go
// loop instead of recurse
func (v *Identity) authenticate(initial Auth, user, pass string) (*Token, error) {
auth := initial
pwdSet := false
for {
pwdSet = applyCallbacks(&auth, user, pass, pwdSet)
tok, nextAuth, err := v.sendStep(auth, pwdSet)
if err != nil {
return nil, err
}
if tok != nil {
return tok, nil
}
auth = *nextAuth
}
}
```
This keeps identical behavior but flattens recursion and separates concerns.
</issue_to_address>
### Comment 2
<location> `vehicle/subaru/api.go:26` </location>
<code_context>
tTZipv6liF74PwMfk9Ed68AQ0bISswwf3iHQdqcF
</code_context>
<issue_to_address>
**security (generic-api-key):** Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
*Source: gitleaks*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| AuthorizationPath = "oauth2/realms/root/realms/alliance-subaru/authorize?client_id=8c4921b0b08901fef389ce1af49c4e10.subaru.com&scope=openid+profile+write&response_type=code&redirect_uri=com.subaru.oneapp:/oauth2Callback&code_challenge=plain&code_challenge_method=plain" | ||
| VehicleGuidPath = "v2/vehicle/guid" | ||
| RemoteElectricStatusPath = "v1/global/remote/electric/status" | ||
| ApiKey = "tTZipv6liF74PwMfk9Ed68AQ0bISswwf3iHQdqcF" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
security (generic-api-key): Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
Source: gitleaks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a shared API Key that was used for Toyota implementation too, pretty easy to get that from the official app.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andig as you can see this is the only point sorcery is complaining about but it's an information hardcoded everywhere in toyota/subaru apps. In codebase there's exactly same implementation for Toyota already.
bf825e0 to
9d73e26
Compare
9d73e26 to
8f85434
Compare
|
@andig just to understand better for future contributions, why this PR was discarded? Thanks. |
|
Seems it timed out waiting to pass the quality checks? |
This is referring to an api key that is hardcoded inside Subaru phone app. It's very similar to Toyota code (they share same ecosystem even if in slight different way). How I'm supposed to fix this? |
|
Let's restart |
| "Accept": request.JSONContent, | ||
| "x-guid": v.identity.uuid, | ||
| "x-api-key": ApiKey, | ||
| "x-client-ref": v.clientRef, | ||
| "x-appversion": ClientRefKey, | ||
| "X-Appbrand": "S", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these for every api request? Then this should become a headers decorator.
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| func TestAPI(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this test is only useful for you I'd suggest to remove it
|
|
||
| var res struct { | ||
| oauth2.Token | ||
| IDToken string `json:"id_token"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can get the idtoken using oauth2.Token.Extra("id_token"), see cardata.
| return nil | ||
| } | ||
|
|
||
| func (v *Identity) RefreshToken(token *oauth2.Token) (*oauth2.Token, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks a lot like normal OAuth flow. If possible it would be nice to use an oauth2.Config and associated functions. The headers could be set using a headers decorator on the transport.
Implemented Subaru Solterra support for EU.
Tested and it works on my system, not sure if I need to create a PR for website too, basically configuration is the same as Toyota (they share same platform).