Polestar: fix resume path and adjust regex

[loebse]

This should fix #28380 as the resume path has changed. Besides this, also the regex had to change to make it work again.

[sourcery-ai]

Hey - I've found 3 issues, and left some high level feedback:

• The new regex (?:url|action):\s*"(.+)" is quite broad and greedy; consider tightening it (e.g., using a non-greedy quantifier and/or anchoring to the expected resume URL pattern) to avoid accidentally matching the wrong URL if the page structure changes or multiple url/action attributes exist.
• When parsing the Location header with url.Parse(location), the error is currently ignored; handle the error (or explicitly document why it's safe to ignore) to avoid silently falling back to resp.Request.URL on malformed redirects.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- The new regex `(?:url|action):\s*"(.+)"` is quite broad and greedy; consider tightening it (e.g., using a non-greedy quantifier and/or anchoring to the expected resume URL pattern) to avoid accidentally matching the wrong URL if the page structure changes or multiple url/action attributes exist.
- When parsing the `Location` header with `url.Parse(location)`, the error is currently ignored; handle the error (or explicitly document why it's safe to ignore) to avoid silently falling back to `resp.Request.URL` on malformed redirects.

## Individual Comments

### Comment 1
<location path="vehicle/polestar/identity.go" line_range="97" />
<code_context>
 	}

-	matches := regexp.MustCompile(`(?:url|action):\s*"/as/(.+?)/resume/as/authorization\.ping"`).FindStringSubmatch(string(body))
+	matches := regexp.MustCompile(`(?:url|action):\s*"(.+)"`).FindStringSubmatch(string(body))

 	if len(matches) < 2 {
</code_context>
<issue_to_address>
**issue (bug_risk):** The updated regex is too broad and greedy, which may capture unintended URLs or paths.

The new pattern now matches any `url` or `action` and uses a greedy `(.+)`, which, combined with later concatenation, can capture full absolute or unrelated URLs (e.g., logout). Please either make the group non-greedy (e.g. `(.+?)` or `[^\"]+`) and/or restrict it to the expected login/resume path (e.g., a relative path under `/as/`).
</issue_to_address>

### Comment 2
<location path="vehicle/polestar/identity.go" line_range="110" />
<code_context>
 	}

-	uri = fmt.Sprintf("%s/as/%s/resume/as/authorization.ping", OAuthURI, matches[1])
+	uri = OAuthURI + "/" + strings.TrimLeft(matches[1], "/") // Remove leading /
+
 	req, _ = request.New(http.MethodPost, uri, strings.NewReader(data.Encode()), map[string]string{
</code_context>
<issue_to_address>
**issue (bug_risk):** Blindly prefixing `OAuthURI` to the matched URL/path can break if the match is an absolute URL.

If `matches[1]` is a full URL (e.g. `https://idp.example.com/as/...`), concatenating `OAuthURI + "/" + ...` will yield something like `https://oauth.example.com/https://idp.example.com/...`. Consider parsing `matches[1]` (e.g. with `url.Parse`) and only prefixing `OAuthURI` when it’s a relative path (no scheme/host); use absolute URLs as-is.
</issue_to_address>

### Comment 3
<location path="vehicle/polestar/identity.go" line_range="123-125" />
<code_context>

-	// Extract authorization code from response
-	code := resp.Request.URL.Query().Get("code")
+	var code string
+	if location := resp.Header.Get("Location"); location != "" {
+		locURL, _ := url.Parse(location)
+		code = locURL.Query().Get("code")
+	} else {
</code_context>
<issue_to_address>
**issue (bug_risk):** Ignoring the error from `url.Parse` on the Location header may lead to hard-to-diagnose failures.

If `Location` is malformed, `url.Parse` may return a nil URL and a non-nil error; calling `locURL.Query()` would then panic. Please handle the parse error explicitly—for example, by falling back to `resp.Request.URL.Query()` or returning a clear error—instead of assuming `locURL` is always valid.
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[sourcery-ai]

issue (bug_risk): The updated regex is too broad and greedy, which may capture unintended URLs or paths.

The new pattern now matches any url or action and uses a greedy (.+), which, combined with later concatenation, can capture full absolute or unrelated URLs (e.g., logout). Please either make the group non-greedy (e.g. (.+?) or [^\"]+) and/or restrict it to the expected login/resume path (e.g., a relative path under /as/).

[sourcery-ai]

issue (bug_risk): Blindly prefixing OAuthURI to the matched URL/path can break if the match is an absolute URL.

If matches[1] is a full URL (e.g. https://idp.example.com/as/...), concatenating OAuthURI + "/" + ... will yield something like https://oauth.example.com/https://idp.example.com/.... Consider parsing matches[1] (e.g. with url.Parse) and only prefixing OAuthURI when it’s a relative path (no scheme/host); use absolute URLs as-is.

[andig]

These changes seems unrelated?

[loebse]

Correct, some changes I thought were relevant and forgot to undo. Changed back with 3ef936c

[loebse]

The scope change is actually required, otherwise we cannot get the resume path: 6326d8b

[andig]

Unrelated?

[loebse]

Correct, some changes I thought were relevant and forgot to undo. Changed back with 3ef936c

[andig]

Seems odd to have an or here for the same IDP?

[loebse]

Fair point, simplified in 4135a1d

[loebse]

@andig I mixed up a few changes I made when investigating and trying to make it work again. Now everything should be resolved. There were two changes required:

• scope had to change
• regex had to change