Skip to content

Commit 098be9a

Browse files
netominetomi
authored
Fix: add consistent and strict http headers when serving files inside extensions (#1922)
* use consistant and strict http headers when serving user content * apply review suggestions * do not encode the filename in utf8 * revert to use metadata when uploading to S3 * add unit test, more defensive coding
1 parent b1b98c4 commit 098be9a

16 files changed

Lines changed: 460 additions & 113 deletions

‎server/gradle/libs.versions.toml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ rewrite-java = "3.26.0"
2525
spring-boot = "3.5.14"
2626
springdoc = "2.8.13"
2727
testcontainers = "1.21.4"
28-
tika = "3.2.2"
28+
tika = "3.3.1"
2929
woodstox = "6.4.0"
3030

3131
[plugins]

‎server/src/main/java/org/eclipse/openvsx/LocalRegistryService.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -233,7 +233,7 @@ public ResponseEntity<StreamingResponseBody> getFile(String namespace, String ex
233233
return storageUtil.getFileResponse(resource);
234234
}
235235

236-
public boolean isType (String fileName){
236+
public boolean isType(String fileName) {
237237
var expectedTypes = new ArrayList<>(List.of(MANIFEST, README, LICENSE, ICON, DOWNLOAD, DOWNLOAD_SHA256, CHANGELOG, VSIXMANIFEST));
238238
if(integrityService.isEnabled()) {
239239
expectedTypes.add(DOWNLOAD_SIG);

‎server/src/main/java/org/eclipse/openvsx/RegistryAPI.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -809,7 +809,7 @@ public ResponseEntity<SearchResultJson> search(
809809
result.setExtensions(resultExtensions);
810810
return ResponseEntity.ok()
811811
// do not cache to avoid stale data, elasticsearch in general is fast
812-
.cacheControl(CacheControl.noCache().cachePublic().mustRevalidate())
812+
.cacheControl(CacheControl.noCache().cachePublic())
813813
.body(result);
814814
}
815815

‎server/src/main/java/org/eclipse/openvsx/adapter/LocalVSCodeService.java‎

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,17 @@ public class LocalVSCodeService implements IVSCodeService {
6262
private final WebResourceService webResources;
6363
private final CacheService cache;
6464

65+
private final Map<String, String> assets = Map.of(
66+
FILE_VSIX, DOWNLOAD,
67+
FILE_MANIFEST, MANIFEST,
68+
FILE_DETAILS, README,
69+
FILE_CHANGELOG, CHANGELOG,
70+
FILE_LICENSE, LICENSE,
71+
FILE_ICON, ICON,
72+
FILE_VSIXMANIFEST, VSIXMANIFEST,
73+
FILE_SIGNATURE, DOWNLOAD_SIG
74+
);
75+
6576
@Value("${ovsx.webui.url:}")
6677
String webuiUrl;
6778

@@ -350,17 +361,6 @@ public ResponseEntity<StreamingResponseBody> getAsset(
350361
}
351362
}
352363

353-
var assets = Map.of(
354-
FILE_VSIX, DOWNLOAD,
355-
FILE_MANIFEST, MANIFEST,
356-
FILE_DETAILS, README,
357-
FILE_CHANGELOG, CHANGELOG,
358-
FILE_LICENSE, LICENSE,
359-
FILE_ICON, ICON,
360-
FILE_VSIXMANIFEST, VSIXMANIFEST,
361-
FILE_SIGNATURE, DOWNLOAD_SIG
362-
);
363-
364364
var type = assets.get(assetType);
365365
if (type != null) {
366366
var resource = repositories.findFileByType(namespace, extensionName, targetPlatform, version, type);

‎server/src/main/java/org/eclipse/openvsx/adapter/UpstreamVSCodeService.java‎

Lines changed: 11 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
package org.eclipse.openvsx.adapter;
1111

1212
import com.fasterxml.jackson.databind.ObjectMapper;
13+
import jakarta.annotation.Nullable;
1314
import org.apache.commons.io.IOUtils;
1415
import org.apache.commons.lang3.StringUtils;
1516
import org.eclipse.openvsx.ExtensionValidator;
@@ -157,28 +158,23 @@ public ResponseEntity<StreamingResponseBody> browse(String namespaceName, String
157158
@Override
158159
public ResponseEntity<StreamingResponseBody> extractData(ClientHttpResponse response) throws IOException {
159160
var statusCode = response.getStatusCode();
160-
if(statusCode.isError() && statusCode != HttpStatus.NOT_FOUND) {
161+
if (statusCode.isError() && statusCode != HttpStatus.NOT_FOUND) {
161162
handleResponseError(urlTemplate, uriVariables, response);
162163
}
163164

164165
var failed = !statusCode.is2xxSuccessful() && !statusCode.is3xxRedirection();
165-
if(failed) {
166+
if (failed) {
166167
throw new NotFoundException();
167168
}
168169

169-
var headers = new HttpHeaders();
170-
headers.addAll(response.getHeaders());
171-
headers.remove(HttpHeaders.ACCESS_CONTROL_ALLOW_ORIGIN);
172-
headers.remove(HttpHeaders.VARY);
173-
174-
if (proxy != null && MediaType.APPLICATION_JSON.equals(headers.getContentType())) {
170+
if (proxy != null && MediaType.APPLICATION_JSON.equals(response.getHeaders().getContentType())) {
175171
var mapper = new ObjectMapper();
176172
var json = proxy.rewriteUrls(mapper.readTree(response.getBody()));
177173
return ResponseEntity.status(statusCode)
178-
.headers(headers)
174+
.headers(HttpHeadersUtil.createJsonFileResponseHeaders())
179175
.body(outputStream -> mapper.writeValue(outputStream, json));
180176
} else {
181-
return streamResponse(response, headers, "browse");
177+
return streamResponse(response, org.springframework.util.StringUtils.getFilename(path), "browse");
182178
}
183179
}
184180
};
@@ -306,12 +302,8 @@ public ResponseEntity<StreamingResponseBody> extractData(ClientHttpResponse resp
306302
throw new NotFoundException();
307303
}
308304

309-
var headers = new HttpHeaders();
310-
headers.addAll(response.getHeaders());
311-
headers.remove(HttpHeaders.ACCESS_CONTROL_ALLOW_ORIGIN);
312-
headers.remove(HttpHeaders.VARY);
313-
314-
return streamResponse(response, headers, "asset");
305+
// we cant safely know the filename of the asset, so just pass null
306+
return streamResponse(response, null, "asset");
315307
}
316308
};
317309

@@ -348,7 +340,7 @@ private NotFoundException propagateRestException(RestClientException exc, HttpMe
348340

349341
private ResponseEntity<StreamingResponseBody> streamResponse(
350342
ClientHttpResponse response,
351-
HttpHeaders headers,
343+
@Nullable String fileName,
352344
String prefix
353345
) throws IOException {
354346
var tempFile = new TempFile(prefix, null);
@@ -357,6 +349,8 @@ private ResponseEntity<StreamingResponseBody> streamResponse(
357349
response.getBody().transferTo(out);
358350
}
359351

352+
var headers = HttpHeadersUtil.createFileResponseHeaders(tempFile.getPath(), fileName);
353+
360354
return ResponseEntity.status(response.getStatusCode())
361355
.headers(headers)
362356
.body(outputStream -> {

‎server/src/main/java/org/eclipse/openvsx/security/SecurityConfig.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@ public SecurityFilterChain filterChain(HttpSecurity http, OAuth2UserServices use
5656
.csrf(configurer -> configurer.ignoringRequestMatchers(pathMatchers("/api/-/publish", "/api/-/namespace/create", "/api/-/query", "/vscode/**", "/admin/api/**")))
5757
.exceptionHandling(configurer -> configurer.authenticationEntryPoint(new Http403ForbiddenEntryPoint()));
5858

59-
if(userServices.canLogin()) {
59+
if (userServices.canLogin()) {
6060
var redirectUrl = StringUtils.isEmpty(webuiUrl) ? "/" : webuiUrl;
6161
filterChain.oauth2Login(configurer -> {
6262
configurer.defaultSuccessUrl(redirectUrl);

‎server/src/main/java/org/eclipse/openvsx/storage/AwsStorageService.java‎

Lines changed: 14 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
import org.eclipse.openvsx.entities.FileResource;
1717
import org.eclipse.openvsx.entities.Namespace;
1818
import org.eclipse.openvsx.util.FileUtil;
19+
import org.eclipse.openvsx.util.HttpHeadersUtil;
1920
import org.eclipse.openvsx.util.TempFile;
2021
import org.springframework.beans.factory.annotation.Value;
2122
import org.springframework.cache.annotation.Cacheable;
@@ -214,12 +215,20 @@ public void uploadNamespaceLogo(TempFile logoFile) {
214215
}
215216

216217
protected void uploadFile(TempFile file, String fileName, String objectKey) {
218+
var headers = HttpHeadersUtil.createFileResponseHeaders(file.getPath(), fileName);
219+
220+
// see https://docs.aws.amazon.com/AmazonS3/latest/userguide/UsingMetadata.html
217221
var metadata = new HashMap<String, String>();
218-
metadata.put("Content-Type", StorageUtil.getFileType(fileName).toString());
219-
if (fileName.endsWith(".vsix")) {
220-
metadata.put("Content-Disposition", "attachment; filename=\"" + fileName + "\"");
221-
} else {
222-
metadata.put("Cache-Control", StorageUtil.getCacheControl(fileName).getHeaderValue());
222+
if (headers.getContentType() != null) {
223+
metadata.put("Content-Type", headers.getContentType().toString());
224+
}
225+
226+
if (StringUtils.isNotBlank(headers.getContentDisposition().toString())) {
227+
metadata.put("Content-Disposition", headers.getContentDisposition().toString());
228+
}
229+
230+
if (headers.getCacheControl() != null) {
231+
metadata.put("Cache-Control", headers.getCacheControl());
223232
}
224233

225234
var request = PutObjectRequest.builder()

‎server/src/main/java/org/eclipse/openvsx/storage/AzureBlobStorageService.java‎

Lines changed: 16 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
import org.eclipse.openvsx.entities.FileResource;
2323
import org.eclipse.openvsx.entities.Namespace;
2424
import org.eclipse.openvsx.util.FileUtil;
25+
import org.eclipse.openvsx.util.HttpHeadersUtil;
2526
import org.eclipse.openvsx.util.TempFile;
2627
import org.springframework.beans.factory.annotation.Value;
2728
import org.springframework.cache.annotation.Cacheable;
@@ -106,18 +107,25 @@ protected void uploadFile(TempFile file, String fileName, String blobName) {
106107
throw new IllegalStateException(missingEndpointMessage("Cannot upload file", blobName));
107108
}
108109

110+
var headers = HttpHeadersUtil.createFileResponseHeaders(file.getPath(), fileName);
111+
109112
var blobClient = getContainerClient().getBlobClient(blobName);
110-
var headers = new BlobHttpHeaders();
111-
headers.setContentType(StorageUtil.getFileType(fileName).toString());
112-
if (fileName.endsWith(".vsix") || fileName.endsWith(".sigzip")) {
113-
headers.setContentDisposition("attachment; filename=\"" + fileName + "\"");
114-
} else {
115-
var cacheControl = StorageUtil.getCacheControl(fileName);
116-
headers.setCacheControl(cacheControl.getHeaderValue());
113+
var blobHeaders = new BlobHttpHeaders();
114+
115+
if (headers.getContentType() != null) {
116+
blobHeaders.setContentType(headers.getContentType().toString());
117+
}
118+
119+
if (StringUtils.isNotBlank(headers.getContentDisposition().toString())) {
120+
blobHeaders.setContentDisposition(headers.getContentDisposition().toString());
121+
}
122+
123+
if (headers.getCacheControl() != null) {
124+
blobHeaders.setCacheControl(headers.getCacheControl());
117125
}
118126

119127
blobClient.uploadFromFile(file.getPath().toAbsolutePath().toString(), true);
120-
blobClient.setHttpHeaders(headers);
128+
blobClient.setHttpHeaders(blobHeaders);
121129
}
122130

123131
@Override

‎server/src/main/java/org/eclipse/openvsx/storage/GoogleCloudStorageService.java‎

Lines changed: 15 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
import org.eclipse.openvsx.entities.FileResource;
1616
import org.eclipse.openvsx.entities.Namespace;
1717
import org.eclipse.openvsx.util.FileUtil;
18+
import org.eclipse.openvsx.util.HttpHeadersUtil;
1819
import org.eclipse.openvsx.util.TempFile;
1920
import org.springframework.beans.factory.annotation.Value;
2021
import org.springframework.cache.annotation.Cacheable;
@@ -94,14 +95,21 @@ public void uploadNamespaceLogo(TempFile logoFile) {
9495
}
9596

9697
protected void uploadFile(TempFile file, String fileName, String objectId) {
97-
var blobInfoBuilder = BlobInfo.newBuilder(BlobId.of(bucketId, objectId))
98-
.setContentType(StorageUtil.getFileType(fileName).toString());
99-
if (fileName.endsWith(".vsix") || fileName.endsWith(".sigzip")) {
100-
blobInfoBuilder.setContentDisposition("attachment; filename=\"" + fileName + "\"");
101-
} else {
102-
var cacheControl = StorageUtil.getCacheControl(fileName);
103-
blobInfoBuilder.setCacheControl(cacheControl.getHeaderValue());
98+
var headers = HttpHeadersUtil.createFileResponseHeaders(file.getPath(), fileName);
99+
var blobInfoBuilder = BlobInfo.newBuilder(BlobId.of(bucketId, objectId));
100+
101+
if (headers.getContentType() != null) {
102+
blobInfoBuilder.setContentType(headers.getContentType().toString());
103+
}
104+
105+
if (StringUtils.isNotBlank(headers.getContentDisposition().toString())) {
106+
blobInfoBuilder.setContentDisposition(headers.getContentDisposition().toString());
104107
}
108+
109+
if (headers.getCacheControl() != null) {
110+
blobInfoBuilder.setCacheControl(headers.getCacheControl());
111+
}
112+
105113
try (
106114
var in = Files.newByteChannel(file.getPath());
107115
var out = getStorage().writer(blobInfoBuilder.build())

‎server/src/main/java/org/eclipse/openvsx/storage/LocalStorageService.java‎

Lines changed: 10 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -22,11 +22,11 @@
2222
import org.apache.commons.lang3.StringUtils;
2323
import org.eclipse.openvsx.entities.FileResource;
2424
import org.eclipse.openvsx.entities.Namespace;
25+
import org.eclipse.openvsx.util.HttpHeadersUtil;
2526
import org.eclipse.openvsx.util.TempFile;
2627
import org.eclipse.openvsx.util.UrlUtil;
2728
import org.springframework.beans.factory.annotation.Value;
2829
import org.springframework.data.util.Pair;
29-
import org.springframework.http.HttpHeaders;
3030
import org.springframework.http.ResponseEntity;
3131
import org.springframework.stereotype.Component;
3232
import org.springframework.web.server.ServerErrorException;
@@ -87,11 +87,10 @@ public void removeFile(FileResource resource) {
8787
}
8888

8989
public ResponseEntity<StreamingResponseBody> getFile(FileResource resource) {
90-
var headers = getFileResponseHeaders(resource.getName());
90+
var path = getPath(resource);
9191
return ResponseEntity.ok()
92-
.headers(headers)
92+
.headers(HttpHeadersUtil.createFileResponseHeaders(path))
9393
.body(outputStream -> {
94-
var path = getPath(resource);
9594
try (var in = Files.newInputStream(path)) {
9695
in.transferTo(outputStream);
9796
}
@@ -104,14 +103,15 @@ public URI getLocation(FileResource resource) {
104103
}
105104

106105
public ResponseEntity<StreamingResponseBody> getNamespaceLogo(Namespace namespace) {
107-
if(!isEnabled()) {
106+
if (!isEnabled()) {
108107
throw new IllegalStateException("Cannot determine location of logo. Configure the 'ovsx.storage.local.directory' property.");
109108
}
110109

110+
var path = getLogoPath(namespace);
111+
111112
return ResponseEntity.ok()
112-
.headers(getFileResponseHeaders(namespace.getLogoName()))
113+
.headers(HttpHeadersUtil.createFileResponseHeaders(path))
113114
.body(outputStream -> {
114-
var path = getLogoPath(namespace);
115115
try (var in = Files.newInputStream(path)) {
116116
in.transferTo(outputStream);
117117
}
@@ -121,17 +121,6 @@ public URI getNamespaceLogoLocation(Namespace namespace) {
121121
return URI.create(UrlUtil.createApiUrl(UrlUtil.getBaseUrl(), "api", namespace.getName(), "logo", namespace.getLogoName()));
122122
}
123123

124-
private HttpHeaders getFileResponseHeaders(String fileName) {
125-
var headers = new HttpHeaders();
126-
headers.setContentType(StorageUtil.getFileType(fileName));
127-
if (fileName.endsWith(".vsix")) {
128-
headers.add("Content-Disposition", "attachment; filename=\"" + fileName + "\"");
129-
} else {
130-
headers.setCacheControl(StorageUtil.getCacheControl(fileName));
131-
}
132-
return headers;
133-
}
134-
135124
@Override
136125
public void uploadNamespaceLogo(TempFile logoFile) {
137126
try {
@@ -182,15 +171,15 @@ public void copyNamespaceLogo(Namespace oldNamespace, Namespace newNamespace) {
182171
}
183172

184173
private Path getPath(FileResource resource) {
185-
if(!isEnabled()) {
174+
if (!isEnabled()) {
186175
throw new IllegalStateException("Cannot determine location of file. Configure the 'ovsx.storage.local.directory' property.");
187176
}
188177

189178
var extVersion = resource.getExtension();
190179
var extension = extVersion.getExtension();
191180
var namespace = extension.getNamespace();
192181
var segments = new ArrayList<>(List.of(storageDirectory, namespace.getName(), extension.getName()));
193-
if(!extVersion.isUniversalTargetPlatform()) {
182+
if (!extVersion.isUniversalTargetPlatform()) {
194183
segments.add(extVersion.getTargetPlatform());
195184
}
196185

@@ -201,7 +190,7 @@ private Path getPath(FileResource resource) {
201190
}
202191

203192
private Path getLogoPath(Namespace namespace) {
204-
if(!isEnabled()) {
193+
if (!isEnabled()) {
205194
throw new IllegalStateException("Cannot determine location of logo. Configure the 'ovsx.storage.local.directory' property.");
206195
}
207196

0 commit comments

Comments
 (0)