Review comments

dev-packages/ovsx-client/src/ovsx-client.ts

@@ -34,7 +34,7 @@ export interface OVSXClientOptions {
 
 export class OVSXClient {
 
-    constructor(readonly options: OVSXClientOptions, readonly request: RequestService) { }
+    constructor(readonly options: OVSXClientOptions, protected readonly request: RequestService) { }
 
     async search(param?: VSXSearchParam): Promise<VSXSearchResult> {
         const searchUri = await this.buildSearchUri(param);
@@ -105,18 +105,16 @@ export class OVSXClient {
     }
 
     protected async fetchJson<R>(url: string): Promise<R> {
-        const e = await this.request.request({
+        const requestContext = await this.request.request({
             url,
             headers: { 'Accept': 'application/json' }
         });
-        return RequestContext.asJson<R>(e);
+        return RequestContext.asJson<R>(requestContext);
     }
 
     async fetchText(url: string): Promise<string> {
-        const e = await this.request.request({
-            url
-        });
-        return RequestContext.asText(e);
+        const requestContext = await this.request.request({ url });
+        return RequestContext.asText(requestContext);
     }
 
     /**

dev-packages/request-service/README.md

@@ -12,7 +12,7 @@
 
 ## Description
 
-The `@theia/request-service` package is used to send proxy-aware http requests to other service.
+The `@theia/request-service` package is used to send proxy-aware http requests to other services.
 
 ## Additional Information
 

dev-packages/request-service/src/request-service.ts → dev-packages/request-service/src/common-request-service.ts

@@ -75,6 +75,33 @@ export namespace RequestContext {
             throw err;
         }
     }
+
+    /**
+     * Convert the buffer to base64 before sending it to the frontend.
+     * This reduces the amount of JSON data transferred massively.
+     * Does nothing if the buffer is already compressed.
+     */
+    export function compress(context: RequestContext): RequestContext {
+        if (context.buffer instanceof Uint8Array && Buffer !== undefined) {
+            const base64Data = Buffer.from(context.buffer).toString('base64');
+            Object.assign(context, {
+                buffer: base64Data
+            });
+        }
+        return context;
+    }
+
+    /**
+     * Decompresses a base64 buffer into a normal array buffer
+     * Does nothing if the buffer is not compressed.
+     */
+    export function decompress(context: RequestContext): RequestContext {
+        const buffer = context.buffer;
+        if (typeof buffer === 'string' && typeof atob === 'function') {
+            context.buffer = Uint8Array.from(atob(buffer), c => c.charCodeAt(0));
+        }
+        return context;
+    }
 }
 
 export interface RequestConfiguration {

dev-packages/request-service/src/index.ts

@@ -14,4 +14,4 @@
  * SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0
  ********************************************************************************/
 
-export * from './request-service';
+export * from './common-request-service';

dev-packages/request-service/src/node-request-service.ts

@@ -16,9 +16,8 @@
 
 import * as http from 'http';
 import * as https from 'https';
-import { parse as parseUrl } from 'url';
 import { getProxyAgent, ProxyAgent } from './proxy';
-import { Headers, RequestConfiguration, RequestContext, RequestOptions, RequestService } from './request-service';
+import { Headers, RequestConfiguration, RequestContext, RequestOptions, RequestService } from './common-request-service';
 import { CancellationToken } from 'vscode-languageserver-protocol';
 
 export interface RawRequestFunction {
@@ -38,7 +37,7 @@ export class NodeRequestService implements RequestService {
     protected authorization?: string;
 
     protected getNodeRequest(options: RequestOptions): RawRequestFunction {
-        const endpoint = parseUrl(options.url!);
+        const endpoint = new URL(options.url);
         const module = endpoint.protocol === 'https:' ? https : http;
         return module.request;
     }
@@ -48,26 +47,25 @@ export class NodeRequestService implements RequestService {
     }
 
     async configure(config: RequestConfiguration): Promise<void> {
-        if ('proxyUrl' in config) {
+        if (config.proxyUrl !== undefined) {
             this.proxyUrl = config.proxyUrl;
         }
-        if ('strictSSL' in config) {
+        if (config.strictSSL !== undefined) {
             this.strictSSL = config.strictSSL;
         }
-        if ('proxyAuthorization' in config) {
+        if (config.proxyAuthorization !== undefined) {
             this.authorization = config.proxyAuthorization;
         }
     }
 
     protected async processOptions(options: NodeRequestOptions): Promise<NodeRequestOptions> {
         const { strictSSL } = this;
+        options.strictSSL = options.strictSSL ?? strictSSL;
         const agent = options.agent ? options.agent : getProxyAgent(options.url || '', process.env, {
             proxyUrl: await this.getProxyUrl(options.url),
-            strictSSL
+            strictSSL: options.strictSSL
         });
-
         options.agent = agent;
-        options.strictSSL = options.strictSSL ?? strictSSL;
 
         const authorization = options.proxyAuthorization || this.authorization;
         if (authorization) {
@@ -84,7 +82,7 @@ export class NodeRequestService implements RequestService {
         return new Promise(async (resolve, reject) => {
             options = await this.processOptions(options);
 
-            const endpoint = parseUrl(options.url);
+            const endpoint = new URL(options.url);
             const rawRequest = options.getRawRequest
                 ? options.getRawRequest(options)
                 : this.getNodeRequest(options);
@@ -93,7 +91,7 @@ export class NodeRequestService implements RequestService {
                 hostname: endpoint.hostname,
                 port: endpoint.port ? parseInt(endpoint.port) : (endpoint.protocol === 'https:' ? 443 : 80),
                 protocol: endpoint.protocol,
-                path: endpoint.path,
+                path: endpoint.pathname + endpoint.search,
                 method: options.type || 'GET',
                 headers: options.headers,
                 agent: options.agent,
@@ -105,11 +103,11 @@ export class NodeRequestService implements RequestService {
             }
 
             const req = rawRequest(opts, async res => {
-                const followRedirects: number = options.followRedirects ?? 3;
-                if (res.statusCode && res.statusCode >= 300 && res.statusCode < 400 && followRedirects > 0 && res.headers['location']) {
+                const followRedirects = options.followRedirects ?? 3;
+                if (res.statusCode && res.statusCode >= 300 && res.statusCode < 400 && followRedirects > 0 && res.headers.location) {
                     this.request({
                         ...options,
-                        url: res.headers['location'],
+                        url: res.headers.location,
                         followRedirects: followRedirects - 1
                     }, token).then(resolve, reject);
                 } else {

packages/core/src/browser/request/browser-request-service.ts

@@ -20,10 +20,7 @@ import { BackendRequestService, RequestConfiguration, RequestContext, RequestOpt
 import { PreferenceService } from '../preferences/preference-service';
 
 @injectable()
-export class DefaultBrowserRequestService implements RequestService {
-
-    @inject(BackendRequestService)
-    protected readonly backendRequestService: RequestService;
+export abstract class AbstractBrowserRequestService implements RequestService {
 
     @inject(PreferenceService)
     protected readonly preferenceService: PreferenceService;
@@ -43,20 +40,26 @@ export class DefaultBrowserRequestService implements RequestService {
             });
         });
         this.preferenceService.onPreferencesChanged(e => {
-            const config: RequestConfiguration = {};
-            if ('http.proxy' in e) {
-                config.proxyUrl = e['http.proxy'].newValue;
-            }
-            if ('http.proxyAuthorization' in e) {
-                config.proxyAuthorization = e['http.proxyAuthorization'].newValue;
-            }
-            if ('http.proxyStrictSSL' in e) {
-                config.strictSSL = e['http.proxyStrictSSL'].newValue;
-            }
-            this.configure(config);
+            this.configurePromise.then(() => this.configure({
+                proxyUrl: e['http.proxy']?.newValue,
+                proxyAuthorization: e['http.proxyAuthorization']?.newValue,
+                strictSSL: e['http.proxyStrictSSL']?.newValue
+            }));
         });
     }
 
+    abstract configure(config: RequestConfiguration): Promise<void>;
+    abstract request(options: RequestOptions, token?: CancellationToken): Promise<RequestContext>;
+    abstract resolveProxy(url: string): Promise<string | undefined>;
+
+}
+
+@injectable()
+export class ProxyingBrowserRequestService extends AbstractBrowserRequestService {
+
+    @inject(BackendRequestService)
+    protected readonly backendRequestService: RequestService;
+
     configure(config: RequestConfiguration): Promise<void> {
         return this.backendRequestService.configure(config);
     }
@@ -65,29 +68,21 @@ export class DefaultBrowserRequestService implements RequestService {
         return this.backendRequestService.resolveProxy(url);
     }
 
-    protected transformBackendResponse(context: RequestContext): RequestContext {
-        // In the `backend-request-facade` we transform the binary buffer into a base64 string to save space
-        // We need to tranform it back into a binary buffer here
-        const transferedBuffer = context.buffer as unknown as string;
-        context.buffer = Uint8Array.from(atob(transferedBuffer), c => c.charCodeAt(0));
-        return context;
-    }
-
     async request(options: RequestOptions): Promise<RequestContext> {
         // Wait for both the preferences and the configuration of the backend service
         await this.configurePromise;
         const backendResult = await this.backendRequestService.request(options);
-        return this.transformBackendResponse(backendResult);
+        return RequestContext.decompress(backendResult);
     }
 }
 
 @injectable()
-export class XHRBrowserRequestService extends DefaultBrowserRequestService {
+export class XHRBrowserRequestService extends ProxyingBrowserRequestService {
 
     protected authorization?: string;
 
     override configure(config: RequestConfiguration): Promise<void> {
-        if ('proxyAuthorization' in config) {
+        if (config.proxyAuthorization !== undefined) {
             this.authorization = config.proxyAuthorization;
         }
         return super.configure(config);
@@ -98,6 +93,8 @@ export class XHRBrowserRequestService extends DefaultBrowserRequestService {
             const xhrResult = await this.xhrRequest(options, token);
             const statusCode = xhrResult.res.statusCode ?? 200;
             if (statusCode >= 400) {
+                // We might've been blocked by the firewall
+                // Try it through the backend request service
                 return super.request(options);
             }
             return xhrResult;

packages/core/src/electron-browser/request/electron-browser-request-module.ts

@@ -15,12 +15,12 @@
  ********************************************************************************/
 
 import { ContainerModule } from 'inversify';
-import { DefaultBrowserRequestService } from '../../browser/request/browser-request-service';
+import { ProxyingBrowserRequestService } from '../../browser/request/browser-request-service';
 import { RequestService } from '@theia/request-service';
 
 export default new ContainerModule(bind => {
-    // We bind the non-xhr request service here. This will always proxy every request through the backend.
+    // This version of the request service will always proxy every request through the backend.
     // We do this since the backend currently cannot automatically resolve proxies, but the frontend can.
     // We try to avoid confusion with this where some (frontend) requests successfully go through the proxy, but some others (backend) don't.
-    bind(RequestService).to(DefaultBrowserRequestService).inSingletonScope();
+    bind(RequestService).to(ProxyingBrowserRequestService).inSingletonScope();
 });

packages/core/src/node/request/backend-request-facade.ts

@@ -30,13 +30,7 @@ export class BackendRequestFacade implements RequestService {
 
     async request(options: RequestOptions, token?: CancellationToken): Promise<RequestContext> {
         const context = await this.requestService.request(options, token);
-        // Convert the buffer to base64 before sending it to the frontend
-        // This reduces the amount of JSON data transferred massively
-        const base64Data = Buffer.from(context.buffer).toString('base64');
-        Object.assign(context, {
-            buffer: base64Data
-        });
-        return context;
+        return RequestContext.compress(context);
     }
 
     resolveProxy(url: string): Promise<string | undefined> {

packages/plugin-ext/src/hosted/node/plugin-host-proxy.ts

@@ -32,7 +32,7 @@ export function connectProxyResolver(configProvider: PreferenceRegistryExtImpl):
         env: process.env,
     });
     const lookup = createPatchedModules(configProvider, resolveProxy);
-    return configureModuleLoading(lookup);
+    configureModuleLoading(lookup);
 }
 
 interface PatchedModules {

packages/preferences/src/browser/util/preference-tree-generator.ts

@@ -57,8 +57,8 @@ export class PreferenceTreeGenerator {
         ['extensions', 'features'],
         ['files', 'editor'],
         ['hosted-plugin', 'features'],
-        ['keyboard', 'application'],
         ['http', 'application'],
+        ['keyboard', 'application'],
         ['output', 'features'],
         ['problems', 'features'],
         ['preview', 'features'],