Skip to content

Commit 339c68b

Browse files
Copilotconorheffrongithub-advanced-security[bot]
authored
Fix failing CI tests: GitClient allowlist stubs, integration test timeout properties, stable Selenium selectors (#663)
* Initial plan * refactor: migrate GitClient API calls to RestTemplate Agent-Logs-Url: https://github.com/conorheffron/ironoc/sessions/d12ea468-41d4-412d-b2d3-0de30faf76b1 Co-authored-by: conorheffron <8218626+conorheffron@users.noreply.github.com> * fix: harden GitClient URI validation for RestTemplate calls Agent-Logs-Url: https://github.com/conorheffron/ironoc/sessions/d12ea468-41d4-412d-b2d3-0de30faf76b1 Co-authored-by: conorheffron <8218626+conorheffron@users.noreply.github.com> * refactor: build GitHub API URIs in GitClient from templates Agent-Logs-Url: https://github.com/conorheffron/ironoc/sessions/d12ea468-41d4-412d-b2d3-0de30faf76b1 Co-authored-by: conorheffron <8218626+conorheffron@users.noreply.github.com> * fix: use short-circuit URL validation in GitDetailsService Agent-Logs-Url: https://github.com/conorheffron/ironoc/sessions/d12ea468-41d4-412d-b2d3-0de30faf76b1 Co-authored-by: conorheffron <8218626+conorheffron@users.noreply.github.com> * Potential fix for pull request finding 'CodeQL / Server-side request forgery' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com> * merge: resolve conflicts with main in GitDetailsServiceTest Agent-Logs-Url: https://github.com/conorheffron/ironoc/sessions/f283bc18-6796-4d53-9e08-8542e2a29816 Co-authored-by: conorheffron <8218626+conorheffron@users.noreply.github.com> * chore: restore frontend lockfile after merge conflict resolution Co-authored-by: conorheffron <8218626+conorheffron@users.noreply.github.com> * Initial plan * Initial plan for fixing failing tests in PR #657 Agent-Logs-Url: https://github.com/conorheffron/ironoc/sessions/e2b11e10-6ecc-4eaa-acc1-152b5e8534f2 Co-authored-by: conorheffron <8218626+conorheffron@users.noreply.github.com> * Fix failing tests: GitClient URL allowlist stubs, integration test timeout props, stable Selenium selectors Co-authored-by: conorheffron <8218626+conorheffron@users.noreply.github.com> * Fix failing tests: integration test timeout props, stable Selenium CSS selectors for iRonoc nav Agent-Logs-Url: https://github.com/conorheffron/ironoc/sessions/e2b11e10-6ecc-4eaa-acc1-152b5e8534f2 Co-authored-by: conorheffron <8218626+conorheffron@users.noreply.github.com> * Fix GitClient.java: initialize dtos as ArrayList and fix misleading blank-body error message Agent-Logs-Url: https://github.com/conorheffron/ironoc/sessions/e2b11e10-6ecc-4eaa-acc1-152b5e8534f2 Co-authored-by: conorheffron <8218626+conorheffron@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: conorheffron <8218626+conorheffron@users.noreply.github.com> Co-authored-by: Conor Heffron <conor.heffron@gmail.com> Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
1 parent 596585c commit 339c68b

10 files changed

Lines changed: 145 additions & 341 deletions

File tree

‎frontend/package-lock.json‎

Lines changed: 0 additions & 18 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎src/main/java/net/ironoc/portfolio/client/Client.java‎

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -4,11 +4,5 @@
44

55
public interface Client {
66

7-
<T> List<T> callGitHubApi(String apiUri, String uri, Class<T> type, String httpMethod);
8-
9-
HttpsURLConnection createConn(String url, String baseUrl, String httpMethod) throws IOException;
10-
11-
InputStream readInputStream(HttpsURLConnection conn) throws IOException;
12-
13-
void closeConn(InputStream inputStream) throws IOException;
7+
<T> List<T> callGitHubApi(String uri, Class<T> type, String httpMethod, Object... uriVariables);
148
}

‎src/main/java/net/ironoc/portfolio/client/GitClient.java‎

Lines changed: 66 additions & 62 deletions
Original file line numberDiff line numberDiff line change
@@ -9,9 +9,15 @@
99
import net.ironoc.portfolio.logger.AbstractLogger;
1010
import net.ironoc.portfolio.utils.UrlUtils;
1111
import lombok.extern.slf4j.Slf4j;
12-
import org.apache.commons.io.IOUtils;
1312
import org.apache.commons.lang3.StringUtils;
13+
import org.springframework.boot.restclient.RestTemplateBuilder;
14+
import org.springframework.http.HttpEntity;
15+
import org.springframework.http.HttpHeaders;
16+
import org.springframework.http.HttpMethod;
17+
import org.springframework.http.ResponseEntity;
1418
import org.springframework.stereotype.Component;
19+
import org.springframework.web.client.RestTemplate;
20+
import org.springframework.web.util.UriComponentsBuilder;
1521

1622
@Component
1723
@Slf4j
@@ -25,95 +31,93 @@ public class GitClient extends AbstractLogger implements Client {
2531

2632
private final ObjectMapper objectMapper;
2733

28-
public GitClient(PropertyConfigI propertyConfig,
34+
private final RestTemplate restTemplate;
35+
36+
public GitClient(RestTemplateBuilder restTemplateBuilder,
37+
PropertyConfigI propertyConfig,
2938
SecretManager secretManager,
3039
UrlUtils urlUtils,
3140
ObjectMapper objectMapper) {
3241
this.propertyConfig = propertyConfig;
3342
this.secretManager = secretManager;
3443
this.urlUtils = urlUtils;
3544
this.objectMapper = objectMapper;
45+
this.restTemplate = restTemplateBuilder
46+
.connectTimeout(Duration.ofMillis(propertyConfig.getGitTimeoutConnect()))
47+
.readTimeout(Duration.ofMillis(propertyConfig.getGitTimeoutRead()))
48+
.build();
3649
}
3750

3851
@Override
39-
public <T> List<T> callGitHubApi(String apiUri, String uri, Class<T> type, String httpMethod) {
40-
info("Triggering GET request: url={}", apiUri);
52+
public <T> List<T> callGitHubApi(String uri, Class<T> type, String httpMethod, Object... uriVariables) {
53+
URI validatedApiUri = null;
4154
List<T> dtos = new ArrayList<>();
42-
InputStream inputStream = null;
4355
try {
44-
HttpsURLConnection conn = this.createConn(apiUri, uri, httpMethod);
45-
if (conn == null) {
46-
error("Failed to created connection");
56+
validatedApiUri = getValidatedApiUri(uri, uriVariables);
57+
if (validatedApiUri == null) {
4758
return Collections.emptyList();
4859
}
49-
inputStream = this.readInputStream(conn);
50-
Map<String, List<String>> map = conn.getHeaderFields();
51-
List<String> linkHeader = map.get("Link");
60+
info("Triggering GET request: url={}", validatedApiUri);
61+
HttpHeaders headers = new HttpHeaders();
62+
String token = secretManager.getGitSecret();
63+
if (StringUtils.isBlank(token)) {
64+
log.warn("GIT token not set, the lower request rate will apply");
65+
} else {
66+
headers.set("Authorization", token);
67+
}
68+
HttpEntity<Void> entity = new HttpEntity<>(headers);
69+
ResponseEntity<String> response = restTemplate.exchange(
70+
validatedApiUri, HttpMethod.valueOf(httpMethod), entity, String.class);
71+
List<String> linkHeader = response.getHeaders().get("Link");
5272
if (linkHeader != null && !linkHeader.isEmpty()) {
5373
info("Link.Header: {}", linkHeader);
5474
}
55-
dtos = readJsonResponse(inputStream, type);
75+
if (StringUtils.isBlank(response.getBody())) {
76+
error("Received blank response body from GitHub API");
77+
return Collections.emptyList();
78+
}
79+
dtos = readJsonResponse(response.getBody(), type);
5680
} catch (Exception ex) {
5781
error("Unexpected error occurred while retrieving data.", ex);
58-
} finally {
59-
try {
60-
if (inputStream != null) {
61-
this.closeConn(inputStream);
62-
} else {
63-
warn("Input stream already closed.");
64-
}
65-
} catch (IOException ex) {
66-
error("Unexpected error occurred while closing input stream.", ex);
67-
}
6882
}
6983
return dtos;
7084
}
7185

72-
private <T> List<T> readJsonResponse(InputStream inputStream, Class<T> type) throws Exception {
73-
List<T> items;
74-
String jsonResponse = convertInputStreamToString(inputStream);
75-
CollectionType listType = objectMapper.getTypeFactory()
76-
.constructCollectionType(ArrayList.class, type);
77-
items = objectMapper.readValue(jsonResponse, listType);
78-
debug("List.of(DTO)={}", items);
79-
return items;
80-
}
81-
82-
@Override
83-
public HttpsURLConnection createConn(String url, String baseUrl, String httpMethod) throws IOException {
84-
URL urlBase = new URL(baseUrl);
85-
String base = urlBase.getProtocol() + "://" + urlBase.getHost();
86-
if (!urlUtils.isValidURL(url) || !url.startsWith(base)) {
87-
log.error("The url is not valid for GIT client connection, url={}", url);
86+
private URI getValidatedApiUri(String uri, Object... uriVariables) throws Exception {
87+
URI targetUri = UriComponentsBuilder.fromUriString(uri)
88+
.buildAndExpand(uriVariables)
89+
.encode()
90+
.toUri();
91+
if (!urlUtils.isValidURL(targetUri.toString())) {
92+
log.error("The url is not valid for GIT client connection, url={}", targetUri);
8893
return null;
8994
}
90-
URL apiUrlEndpoint = new URL(url);
91-
HttpsURLConnection conn = (HttpsURLConnection) apiUrlEndpoint.openConnection();
92-
String token = secretManager.getGitSecret();
93-
if (StringUtils.isBlank(token)) {
94-
log.warn("GIT token not set, the lower request rate will apply");
95-
} else {
96-
conn.setRequestProperty("Authorization", token);
97-
}
98-
conn.setRequestMethod(httpMethod);
99-
HttpURLConnection.setFollowRedirects(propertyConfig.getGitFollowRedirects());
100-
conn.setConnectTimeout(propertyConfig.getGitTimeoutConnect());
101-
conn.setReadTimeout(propertyConfig.getGitTimeoutRead());
102-
conn.setInstanceFollowRedirects(propertyConfig.getGitInstanceFollowRedirects());
103-
return conn;
104-
}
10595

106-
@Override
107-
public InputStream readInputStream(HttpsURLConnection conn) throws IOException {
108-
return conn.getInputStream();
109-
}
96+
String allowedBaseUrl = propertyConfig.getGitApiEndpointIssues();
97+
if (StringUtils.isBlank(allowedBaseUrl) || !urlUtils.isValidURL(allowedBaseUrl)) {
98+
log.error("The configured allowlist base URL is invalid, url={}", allowedBaseUrl);
99+
return null;
100+
}
110101

111-
@Override
112-
public void closeConn(InputStream inputStream) throws IOException {
113-
inputStream.close();
102+
URI allowedBaseUri = UriComponentsBuilder.fromUriString(allowedBaseUrl).build(false).toUri();
103+
if (!StringUtils.equalsIgnoreCase("https", targetUri.getScheme())
104+
|| !StringUtils.equalsIgnoreCase("https", allowedBaseUri.getScheme())
105+
|| !StringUtils.equalsIgnoreCase(allowedBaseUri.getHost(), targetUri.getHost())
106+
|| allowedBaseUri.getPort() != targetUri.getPort()
107+
|| StringUtils.isNotBlank(targetUri.getUserInfo())
108+
|| targetUri.getFragment() != null) {
109+
log.error("The url is not valid for GIT client connection, url={}", targetUri);
110+
return null;
111+
}
112+
return targetUri;
114113
}
115114

116-
protected String convertInputStreamToString(InputStream inputStream) throws Exception {
117-
return IOUtils.toString(inputStream, StandardCharsets.UTF_8);
115+
private <T> List<T> readJsonResponse(String jsonResponse, Class<T> type) throws Exception {
116+
List<T> items;
117+
CollectionType listType = objectMapper.getTypeFactory()
118+
.constructCollectionType(ArrayList.class, type);
119+
items = objectMapper.readValue(jsonResponse, listType);
120+
debug("List.of(DTO)={}", items);
121+
return items;
118122
}
119123
}

‎src/main/java/net/ironoc/portfolio/service/GitDetailsService.java‎

Lines changed: 8 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,6 @@
1515
import org.springframework.beans.factory.annotation.Autowired;
1616
import org.springframework.http.HttpMethod;
1717
import org.springframework.stereotype.Service;
18-
import org.springframework.web.util.UriComponentsBuilder;
1918

2019
@Service
2120
public class GitDetailsService extends AbstractLogger implements GitDetails {
@@ -59,20 +58,12 @@ public List<RepositoryDetailDto> getRepoDetails(String username, boolean isJob)
5958
String uri = propertyConfig.getGitApiEndpointRepos();
6059
Integer page = 1;
6160
Integer per_page = 100;
62-
String apiUri = "";
63-
try {
64-
apiUri = UriComponentsBuilder.fromUriString(uri)
65-
.buildAndExpand(username, per_page, page)
66-
.toUriString();
67-
} catch (IllegalArgumentException e) {
68-
error("Illegal argument passed for uri value: {}", uri);
69-
}
70-
if (StringUtils.isBlank(apiUri) | StringUtils.isBlank(uri)
71-
| !urlUtils.isValidURL(apiUri)) {
72-
warn("URL is not valid: url={}", apiUri);
61+
if (StringUtils.isBlank(uri) || !urlUtils.isValidURL(uri)) {
62+
warn("URL is not valid: url={}", uri);
7363
return Collections.emptyList();
7464
}
75-
return gitClient.callGitHubApi(apiUri, uri, RepositoryDetailDto.class, HttpMethod.GET.name());
65+
return gitClient.callGitHubApi(uri, RepositoryDetailDto.class, HttpMethod.GET.name(),
66+
username, per_page, page);
7667
}
7768

7869
@Override
@@ -121,20 +112,12 @@ public List<RepositoryIssueDto> getIssues(String userId, String repo, boolean is
121112
String uri = propertyConfig.getGitApiEndpointIssues();
122113
Integer page = 1;
123114
Integer per_page = 100;
124-
String apiUri = "";
125-
try {
126-
apiUri = UriComponentsBuilder.fromUriString(uri)
127-
.buildAndExpand(userId, repo, per_page, page)
128-
.toUriString();
129-
} catch (IllegalArgumentException e) {
130-
error("Illegal argument passed for uri value: {}", uri);
131-
}
132-
if (StringUtils.isBlank(apiUri) | StringUtils.isBlank(uri)
133-
| !urlUtils.isValidURL(apiUri)) {
134-
warn("URL is not valid: url={}", apiUri);
115+
if (StringUtils.isBlank(uri) || !urlUtils.isValidURL(uri)) {
116+
warn("URL is not valid: url={}", uri);
135117
return Collections.emptyList();
136118
}
137-
return gitClient.callGitHubApi(apiUri, uri, RepositoryIssueDto.class, HttpMethod.GET.name());
119+
return gitClient.callGitHubApi(uri, RepositoryIssueDto.class, HttpMethod.GET.name(),
120+
userId, repo, per_page, page);
138121
}
139122

140123
@Override

0 commit comments

Comments
 (0)