fix(API): encodeURIComponent for each path() element (#8646)

Re-submittal of reverted PR #8586

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

app/scripts/modules/core/src/api/ApiService.spec.ts

@@ -41,17 +41,34 @@ describe('RequestBuilder backend', () => {
     expect(backend.get).toHaveBeenCalledWith(jasmine.objectContaining({ url: 'http://different/path' }));
   });
 
-  it('trims all leading and trailing slashes', function () {
+  it('trims all leading and trailing slashes from the base url', function () {
     const backend = createBackend();
-    new RequestBuilder(undefined, backend, '///http://different/path/////').path('////foo///').get();
+    new RequestBuilder(undefined, backend, '///http://different/path/////').path('foo').get();
     expect(backend.get).toHaveBeenCalledWith(jasmine.objectContaining({ url: 'http://different/path/foo' }));
   });
 });
 
 describe('REST Service', function () {
-  const builder = () => new RequestBuilder(makeRequestBuilderConfig());
+  const builder = (pathPrefix?: string) => new RequestBuilder(makeRequestBuilderConfig(pathPrefix));
   afterEach(() => SETTINGS.resetToOriginal());
 
+  describe('makeRequestBuilderConfig', () => {
+    it('trims leading slashes from the path prefix', function () {
+      const result = builder('/foo');
+      expect(result['config'].url).toEqual(`foo`);
+    });
+
+    it('trims trailing slashes from the path prefix', function () {
+      const result = builder('foo/');
+      expect(result['config'].url).toEqual(`foo`);
+    });
+
+    it('trims repeated leading and trailing slashes from the path prefix', function () {
+      const result = builder('///foo///');
+      expect(result['config'].url).toEqual(`foo`);
+    });
+  });
+
   describe('RequestBuilder.path()', function () {
     it('joins a path to the current url using a /', function () {
       const result = builder().path('foo');
@@ -75,26 +92,10 @@ describe('REST Service', function () {
       expect(child['config'].url).toEqual(`foo/bar`);
     });
 
-    it('trims leading slashes in paths', function () {
-      const result = builder().path('/foo');
-      expect(result['config'].url).toEqual(`foo`);
-    });
-
-    it('trims trailing slashes in paths', function () {
-      const result = builder().path('foo/');
-      expect(result['config'].url).toEqual(`foo`);
+    it('uriencodes path() parameters', () => {
+      const result = builder().path('foo/bar');
+      expect(result['config'].url).toEqual(`foo%2Fbar`);
     });
-
-    it('trims repeated leading and trailing slashes from urls', function () {
-      const result = builder().path('///foo///');
-      expect(result['config'].url).toEqual(`foo`);
-    });
-
-    // coming soon:
-    // it('uriencodes path parameters', () => {
-    //   const result = builder().path('foo/bar');
-    //   expect(result['config'].url).toEqual(`${BASEURL}/foo%2fbar`);
-    // });
   });
 
   describe('RequestBuilder.query()', function () {

app/scripts/modules/core/src/api/ApiService.ts

@@ -169,7 +169,7 @@ export class RequestBuilder implements IRequestBuilder {
   }
 
   path(...paths: IPrimitive[]) {
-    const url = joinPaths(this.config.url, ...paths);
+    const url = joinPaths(this.config.url, ...paths.map((path) => encodeURIComponent(path)));
     return this.builder({ ...this.config, url });
   }
 
@@ -241,6 +241,18 @@ export class DeprecatedRequestBuilder extends RequestBuilder implements IDepreca
   useCache = (cache: boolean | ICache = true) => this.builder({ ...this.config, cache: cache as boolean });
 }
 
+class DeprecatedRequestBuilderRoot extends DeprecatedRequestBuilder {
+  // Do not encode paths for the root API.one() call
+  one = (...paths: string[]) => {
+    const url = joinPaths(this.config.url, ...paths);
+    return this.builder({ ...this.config, url });
+  };
+  all = (...paths: string[]) => {
+    const url = joinPaths(this.config.url, ...paths);
+    return this.builder({ ...this.config, url });
+  };
+}
+
 export const invalidContentMessage = 'API response was neither JSON nor zero-length html or text';
 
 export function makeRequestBuilderConfig(pathPrefix?: string): IRequestBuilderConfig {
@@ -255,7 +267,7 @@ export function makeRequestBuilderConfig(pathPrefix?: string): IRequestBuilderCo
 }
 
 /** @deprecated use REST('/path/to/gate/endpoint') */
-export const API: IDeprecatedRequestBuilder = new DeprecatedRequestBuilder(makeRequestBuilderConfig());
+export const API: IDeprecatedRequestBuilder = new DeprecatedRequestBuilderRoot(makeRequestBuilderConfig());
 
 /**
  * A REST client used to access Gate endpoints

app/scripts/modules/core/src/pipeline/config/services/PipelineConfigService.ts

@@ -46,7 +46,7 @@ export class PipelineConfigService {
 
   public static deletePipeline(applicationName: string, pipeline: IPipeline, pipelineName: string): PromiseLike<void> {
     const endpoint = pipeline.strategy ? 'strategies' : 'pipelines';
-    return REST(endpoint).path(applicationName, encodeURIComponent(pipelineName.trim())).delete();
+    return REST(endpoint).path(applicationName, pipelineName.trim()).delete();
   }
 
   public static savePipeline(toSave: IPipeline): PromiseLike<void> {
@@ -96,7 +96,7 @@ export class PipelineConfigService {
   public static triggerPipeline(applicationName: string, pipelineName: string, body: any = {}): PromiseLike<string> {
     body.user = AuthenticationService.getAuthenticatedUser().name;
     return REST('/pipelines/v2')
-      .path(applicationName, encodeURIComponent(pipelineName))
+      .path(applicationName, pipelineName)
       .post(body)
       .then((result: ITriggerPipelineResponse) => {
         return result.ref.split('/').pop();

app/scripts/modules/core/src/pipeline/service/execution.service.ts

@@ -105,7 +105,7 @@ export class ExecutionService {
         this.cleanExecutionForDiffing(execution);
         if (application && name) {
           return REST('/applications')
-            .path(application, 'pipelineConfigs', encodeURIComponent(name))
+            .path(application, 'pipelineConfigs', name)
             .get()
             .then((pipelineConfig: IPipeline) => {
               execution.pipelineConfig = pipelineConfig;