[core-rest-pipeline] handle x-www-form-urlencoded form data

[jeremymeng]

Currently formDataPolicy doesn't support x-www-form-urlencoded form data.
Sending request with such form data would cause write EPIPE error. This PR
adds handling of x-www-form-urlencoded form data. With this the workaround in
container registry library can be removed.

Related: #14767

[jeremymeng]

making it a draft PR for now. Will not merge before releasing ACR RC next week.

[check-enforcer]

This pull request is protected by Check Enforcer.

What is Check Enforcer?

Check Enforcer helps ensure all pull requests are covered by at least one check-run (typically an Azure Pipeline). When all check-runs associated with this pull request pass then Check Enforcer itself will pass.

Why am I getting this message?

You are getting this message because Check Enforcer did not detect any check-runs being associated with this pull request within five minutes. This may indicate that your pull request is not covered by any pipelines and so Check Enforcer is correctly blocking the pull request being merged.

What should I do now?

If the check-enforcer check-run is not passing and all other check-runs associated with this PR are passing (excluding license-cla) then you could try telling Check Enforcer to evaluate your pull request again. You can do this by adding a comment to this pull request as follows:
/check-enforcer evaluate
Typically evaulation only takes a few seconds. If you know that your pull request is not covered by a pipeline and this is expected you can override Check Enforcer using the following command:
/check-enforcer override
Note that using the override command triggers alerts so that follow-up investigations can occur (PRs still need to be approved as normal).

What if I am onboarding a new service?

Often, new services do not have validation pipelines associated with them, in order to bootstrap pipelines for a new service, you can issue the following command as a pull request comment:
/azp run prepare-pipelines
This will run a pipeline that analyzes the source tree and creates the pipelines necessary to build and validate your pull request. Once the pipeline has been created you can trigger the pipeline using the following comment:
/azp run js - [service] - ci

[xirzec]

Thanks for adding this support! It's strange that we never needed it in core-http, but I'm glad we're adding it now.

I think there is a simpler way than doing the urlencoding manually -- namely using URLSearchParams to do the heavy lifting. See the comment I left in the browser version of the policy. I think we can do the same thing in node.

[xirzec]

nit: can we name this something that indicates it is only for form data? e.g.

Suggested change

	export function urlEncode(formData: FormDataMap) {
	export function urlEncodeFormData(formData: FormDataMap) {

[xirzec]

this doesn't seem like a very useful assertion - can we grab out the form data to make sure it's what we expect?

[jeremymeng]

form-data's FormData has these

declare class FormData extends stream.Readable {
  constructor(options?: Options);
  append(key: string, value: any, options?: FormData.AppendOptions | string): void;
  getHeaders(userHeaders?: FormData.Headers): FormData.Headers;
  submit(
    params: string | FormData.SubmitOptions,
    callback?: (error: Error | null, response: http.IncomingMessage) => void
  ): http.ClientRequest;
  getBuffer(): Buffer;
  setBoundary(boundary: string): void;
  getBoundary(): string;
  getLength(callback: (err: Error | null, length: number) => void): void;
  getLengthSync(): number;
  hasKnownLength(): boolean;
}

which is different from browser's FormData interface

interface FormData {
    append(name: string, value: string | Blob, fileName?: string): void;
    delete(name: string): void;
    get(name: string): FormDataEntryValue | null;
    getAll(name: string): FormDataEntryValue[];
    has(name: string): boolean;
    set(name: string, value: string | Blob, fileName?: string): void;
    forEach(callbackfn: (value: FormDataEntryValue, key: string, parent: FormData) => void, thisArg?: any): void;
}

I guess I will have to do different tests for Node and browser.

[xirzec]

The node one supports 'toString', if that helps?

[jeremymeng]

not very helpful. it gave "[object FormData]" as in my first version of the test

[xirzec]

I think I found a much easier way to handle the encoding that avoids needing us to have a special helper:

async sendRequest(request: PipelineRequest, next: SendRequest): Promise<PipelineResponse> {
  if (request.formData) {
    const formData = request.formData;
    const requestForm = new FormData();
    for (const formKey of Object.keys(formData)) {
      const formValue = formData[formKey];
      if (Array.isArray(formValue)) {
        for (const subValue of formValue) {
          requestForm.append(formKey, subValue);
        }
      } else {
        requestForm.append(formKey, formValue);
      }
    }

    request.body = requestForm;
    request.formData = undefined;
    const contentType = request.headers.get("Content-Type");
    if (contentType && contentType.indexOf("application/x-www-form-urlencoded") !== -1) {
      request.body = new URLSearchParams(requestForm).toString();
    } else if (contentType && contentType.indexOf("multipart/form-data") !== -1) {
      // browser will automatically apply a suitable content-type header
      request.headers.delete("Content-Type");
    }
   
  }
  return next(request);
}

[jeremymeng]

This looks great! I didn't know that we can construct URLSearchParams with FormData.

[jeremymeng]

I think there is a simpler way than doing the urlencoding manually -- namely using URLSearchParams to do the heavy lifting. See the comment I left in the browser version of the policy. I think we can do the same thing in node.

I just gave it a try. request.body = new URLSearchParams(requestForm).toString(); works fine in the browser, but in NodeJS, it seems that URLSearchParams doesn't work well with requestForm, possibly because form-data doesn't implement iterable so I am getting error:

      AssertionError: expected '_overheadLength=210&_valueLength=63&_valuesToMeasure=&writable=false&readable=true&dataSize=0&maxDataSize=2097152&pauseStreams=true&_re
leased=false&_streams=----------------------------116609151712382571014677%0D%0AContent-Disposition%3A+form-data%3B+name%3D%22service%22%0D%0A%0D%0A%2Cregistry.azurecr
.io%2Cfunction+%28%29+%7B+%5Bnative+code%5D+%7D%2C----------------------------116609151712382571014677%0D%0AContent-Disposition%3A+form-data%3B+name%3D%22scope%22%0D%0
A%0D%0A%2Crepository%3Alibrary%2Fhello-world%3Ametadata_read%2Cfunction+%28%29+%7B+%5Bnative+code%5D+%7D&_currentStream=null&_insideLoop=false&_pendingNext=false&_boun
dary=--------------------------116609151712382571014677' to equal 'service=registry.azurecr.io&scope=repository%3Alibrary%2Fhello-world%3Ametadata_read'

[xirzec]

I just gave it a try. request.body = new URLSearchParams(requestForm).toString(); works fine in the browser, but in NodeJS, it seems that URLSearchParams doesn't work well with requestForm, possibly because form-data doesn't implement iterable so I am getting error:

Yeah I think in the node version we'll have to fork the logic here a little, but it's still easy to take the original formData object and use that:

new URLSearchParams(Object.entries(formData)).toString()

[jeremymeng]

it's still easy to take the original formData object and use that:

I will probably need to loop and append to URLSearchParams to handle the Array case but we don't need to add a helper method any more

[xirzec]

Very nice! Thanks for the update. 👍

[jeremymeng]

/azp run js - containerregistry - tests

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).