Closed Bug 1199796 Opened 9 years ago Closed 9 years ago

Refactor Request and XHR request method validation

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: nsm, Assigned: droniakj1, Mentored)

Details

(Whiteboard: [good first bug][lang=c++])

Attachments

(1 file, 3 obsolete files)

Request and XHR duplicate request method validation [1,2]. It should be straightforward to split this out into FetchUtil::GetValidRequestMethod(inMethod)
that returns a void string if the method is not allowed or the normalized method if allowed.

[1]: https://dxr.mozilla.org/mozilla-central/source/dom/base/nsXMLHttpRequest.cpp?case=true&from=nsXMLHttpRequest.cpp#1620
[2]: https://dxr.mozilla.org/mozilla-central/source/dom/fetch/Request.cpp?case=true&from=Request.cpp#300
Whiteboard: [good first bug][lang=c++]
Hello,

I'd like to take up this bug and provide for a fix for it. Since this will be my first contribution to Mozilla's code base, I'd like some clarifications and help.

Based on your comments (and the linked code files), I assume we want to add a new class named FetchUtil (I couldn't find an existing class by that name) that contains a method GetValidRequestMethod which will perform the job that is currently being done by both nsXMLHttpRequest::Open and Request::constructor. If that is so, what should be the location of the FetchUtil class?
Hopefully nsm can help you here :)
Flags: needinfo?(nsm.nikhil)
(In reply to Najam Ahmed Ansari from comment #1)
> Hello,
> 
> I'd like to take up this bug and provide for a fix for it. Since this will
> be my first contribution to Mozilla's code base, I'd like some
> clarifications and help.
> 
> Based on your comments (and the linked code files), I assume we want to add
> a new class named FetchUtil (I couldn't find an existing class by that name)
> that contains a method GetValidRequestMethod which will perform the job that
> is currently being done by both nsXMLHttpRequest::Open and
> Request::constructor. If that is so, what should be the location of the
> FetchUtil class?

That is correct. You can put it in dom/fetch/.
Flags: needinfo?(nsm.nikhil)
Not completely sure that this is what you had in mind, but I basically saw some redundant code in XMLHttpRequest.cpp and Request.cpp, so I made a class that has that same code with a method GetValidRequestMethod.
Flags: needinfo?(nsm.nikhil)
Attachment #8658810 - Flags: review+
Attachment #8658810 - Flags: feedback+
Comment on attachment 8658810 [details] [diff] [review]
My attempt at fixing this (and my first contribution to Mozilla), hope it works!

Review of attachment 8658810 [details] [diff] [review]:
-----------------------------------------------------------------

The review+ flag is for the reviewer. When you upload a patch, please set the review flag to r? and enter the name/nick of the reviewer in the field so the reviewer is notified they have a review.

This patch is a great start, but needs some work before it can be accepted.

You need to add the FetchUtil.{h,cpp} file to dom/fetch/moz.build in the appropriate lists so that they are included in the build process.

::: dom/fetch/FetchUtil.cpp
@@ +2,5 @@
> +
> +namespace mozilla {
> +namespace dom {
> +
> +  class FetchUtil {

You don't need to redeclare the class in the .cpp file

    // static
    bool
    FetchUtil::IsValidRequestMethod(const nsACString& aMethod) {
      // ... logic
    }

@@ +9,5 @@
> +    nsACString
> +    GetValidRequestMethod(const nsACString& method) {
> +      nsACString method_copy = method;
> +      if (method.LowerCaseEqualsLiteral("trace") ||
> +        method.LowerCaseEqualsLiteral("connect") ||

Line up the indentation

@@ +11,5 @@
> +      nsACString method_copy = method;
> +      if (method.LowerCaseEqualsLiteral("trace") ||
> +        method.LowerCaseEqualsLiteral("connect") ||
> +        method.LowerCaseEqualsLiteral("track")) {
> +        method.AssignLiteral("");

Use the nsresult error codes.

@@ +14,5 @@
> +        method.LowerCaseEqualsLiteral("track")) {
> +        method.AssignLiteral("");
> +        return method;
> +      }
> +      if (method.LowerCaseEqualsLiteral("get"))

Create a copy of the string that is upper cased, then use uppercaseMethod.EqualsLiteral("FOO") instead of calling LowerCase every time.

@@ +15,5 @@
> +        method.AssignLiteral("");
> +        return method;
> +      }
> +      if (method.LowerCaseEqualsLiteral("get"))
> +        method.AssignLiteral("GET");

Instead of writing out these literals multiple times you can do

    if (uppercaseMethod.EqualsLiteral("X") ||
        uppercaseMethod.EqualsLiteral("Y")...) {
      outMethod = uppercaseMethod;
    } else {
      outMethod = aMethod; // Case unchanged for non-standard method.
    }

::: dom/fetch/FetchUtil.h
@@ +1,1 @@
> +#ifndef FETCHUTIL_H

Please follow the ifdef style that is in dom/fetch/Fetch.h like |#ifndef mozilla_dom_FetchUtil_h|

@@ +6,5 @@
> +
> +namespace mozilla {
> +namespace dom {
> +
> +  class FetchUtil {

Dedent the entire class declaration.

@@ +11,5 @@
> +  public:
> +    FetchUtil();
> +
> +    nsACString
> +    GetValidRequestMethod(nsACString inMethod);

This should be

    static nsresult
    GetValidRequestMethod(const nsACString& aMethod, const nsCString& outMethod);

which will return a NS_ERROR_DOM_SECURITY_ERR if it is trace/connect/track or NS_OK otherwise and store the normalized method in outMethod.

It will be called by callers like this:

    nsAutoCString method;
    rv = GetValidRequestMethod(inMethod, method);
    if (NS_WARN_IF(NS_FAILED(rv))) {
      return rv;
    }

    // Use method as the canonical method.

::: dom/fetch/Request.cpp
@@ +301,4 @@
>    if (aInit.mMethod.WasPassed()) {
>      nsAutoCString method(aInit.mMethod.Value());
>      nsAutoCString upperCaseMethod = method;
>      ToUpperCase(upperCaseMethod);

upperCaseMethod is no longer needed. This should be done by the utility method. Neither is the |nsAutoCString method(aInit.mMethod.Value())|, you can directly pass Value() to the utility.

@@ +314,5 @@
>        return nullptr;
>      }
>  
>      // Step 14.2
> +    if (!util.GetValidRequestMethod(upperCaseMethod).EqualsLiteral("")) {

You no longer need the if/else branch. Also, don't make two calls to GetValidRequestMethod. Make one check for NS_IsValidHTTPToken, then ask for the normalized method from GetValidRequestMethod() and if that fails, again fail with MSG_INVALID_REQUEST_METHOD.
Attachment #8658810 - Flags: review+
Attachment #8658810 - Flags: feedback+
Flags: needinfo?(nsm.nikhil)
Attached patch Made requested changes. (obsolete) — Splinter Review
I think I covered everything in this patch. Thanks for the quick review, by the way!
Attachment #8658875 - Flags: review?(nsm.nikhil)
Assignee: nobody → droniakj1
Comment on attachment 8658875 [details] [diff] [review]
Made requested changes.

Review of attachment 8658875 [details] [diff] [review]:
-----------------------------------------------------------------

Needs more changes.

Also, please set up your mercurial config correctly so that your name and email show up in the patch commit.
https://developer.mozilla.org/en-US/docs/Installing_Mercurial#Basic_configuration

The commit message should be of the form "Bug 1199796 - Refactor Request and XHR request method validation. r?nsm"
Please remove the "Added & coded" and "Modified" sections, mercurial already has that information.

::: dom/base/nsXMLHttpRequest.cpp
@@ +1625,2 @@
>    nsAutoCString method;
> +  nsresult rv = util.GetValidRequestMethod(inMethod, method);

Since it is a static method you don't need to create an instance of FetchUtil, just call FetchUtil::GetValidRequestMethod()

::: dom/base/nsXMLHttpRequest.h
@@ +6,5 @@
>  
>  #ifndef nsXMLHttpRequest_h__
>  #define nsXMLHttpRequest_h__
>  
> +

Nit: Extra blank line here can be removed.

@@ +11,2 @@
>  #include "mozilla/Attributes.h"
> +#include "mozilla/dom/FetchUtil.h"

This include can be moved to the XHR.cpp file since you don't use the method in the header.

::: dom/fetch/FetchUtil.cpp
@@ +5,5 @@
> +
> +  FetchUtil() {}
> +
> +  static nsresult
> +  GetValidRequestMethod(const nsACString& aMethod, const nsCString& outMethod) {

Dedent, and since this is part of the FetchUtil class.

The keyword static is not required in the .cpp if it is already in the .h. We have a convention though of having it in a comment, like so:

// static
nsresult
FetchUtil::GetValidRequestMethod(...)

@@ +6,5 @@
> +  FetchUtil() {}
> +
> +  static nsresult
> +  GetValidRequestMethod(const nsACString& aMethod, const nsCString& outMethod) {
> +    nsACString upperCaseMethod = ToUpperCase(aMethod);

nsAutoCString upperCaseMethod = aMethod;
ToUpperCase(upperCaseMethod);

Did you compile your changes before asking for review, because ToUpperCase returns void and should not have compiled.

ToUpperCase() is destructive and does not return anything. The nsA[C]String classes are not actually meant to be instantiated, but are more like a superclass used for passing in parameters.

@@ +7,5 @@
> +
> +  static nsresult
> +  GetValidRequestMethod(const nsACString& aMethod, const nsCString& outMethod) {
> +    nsACString upperCaseMethod = ToUpperCase(aMethod);
> +    if (upperCaseMethod.EqualsLiteral("TRACE") ||

In alphabetical order please.

@@ +10,5 @@
> +    nsACString upperCaseMethod = ToUpperCase(aMethod);
> +    if (upperCaseMethod.EqualsLiteral("TRACE") ||
> +        upperCaseMethod.EqualsLiteral("CONNECT") ||
> +        upperCaseMethod.EqualsLiteral("TRACK")) {
> +      return NS_ERROR_DOM_SECURITY_ERR;

Also call outMethod.SetIsVoid(true) before erroring out.

@@ +12,5 @@
> +        upperCaseMethod.EqualsLiteral("CONNECT") ||
> +        upperCaseMethod.EqualsLiteral("TRACK")) {
> +      return NS_ERROR_DOM_SECURITY_ERR;
> +    }
> +    if (upperCaseMethod.EqualsLiteral("GET") ||

Nit: Add a newline between the } and the if. Please arrange these checks in alphabetical order.

@@ +19,5 @@
> +        upperCaseMethod.EqualsLiteral("HEAD") ||
> +        upperCaseMethod.EqualsLiteral("OPTIONS") ||
> +        upperCaseMethod.EqualsLiteral("PUT")) {
> +      outMethod = upperCaseMethod;
> +      return NS_OK;

You don't need |return NS_OK| twice if you keep it at the end of the function and in the if {} else {} only update outMethod accordingly.

@@ +22,5 @@
> +      outMethod = upperCaseMethod;
> +      return NS_OK;
> +    }
> +    else {
> +      outMethod = aMethod; // case changed for non-standard method

unchanged, not changed.

::: dom/fetch/FetchUtil.h
@@ +6,5 @@
> +
> +namespace mozilla {
> +namespace dom {
> +
> +class FetchUtil {

|class FetchUtil final {|

prevents anybody from overriding this.

@@ +8,5 @@
> +namespace dom {
> +
> +class FetchUtil {
> +public:
> +  FetchUtil();

Constructor is no longer required since the class has only static members. In fact, I'd go as far as to mark this deleted rather than just have the compiler provide defaults so no one tries to instantiate it.

|FetchUtil() = delete;|

Also, make the constructor private.

@@ +10,5 @@
> +class FetchUtil {
> +public:
> +  FetchUtil();
> +
> +  static nsresult

Please add a documentation comment stating what this function does, something along the lines of quoting the fetch spec:

/**
 * Returns a valid HTTP method string based on an input method.
 * Implements checks and normalization as specified by the Fetch specification.
 *
 * Returns NS_ERROR_DOM_SECURITY_ERR if the method is a forbidden method.
 * Otherwise returns NS_OK and the normalized method via outMethod.
 */

@@ +14,5 @@
> +  static nsresult
> +  GetValidRequestMethod(const nsACString& aMethod, const nsCString& outMethod);
> +};
> +
> +}

Please add comments about which namespace is closed by each of these parantheses - see Fetch.h for example.

@@ +17,5 @@
> +
> +}
> +}
> +
> +#endif
\ No newline at end of file

#endif // mozilla_dom_FetchUtil_h

::: dom/fetch/Request.cpp
@@ +304,5 @@
>      // Step 14.1. Disallow forbidden methods, and anything that is not a HTTP
>      // token, since HTTP states that Method may be any of the defined values or
>      // a token (extension method).
> +    FetchUtil util = *(new FetchUtil());
> +    nsresult rv = util.GetValidRequestMethod(method, method);

Don't alias an existing variable name. Introduce a new variable for the out param which is another nsAutoCString.

@@ +305,5 @@
>      // token, since HTTP states that Method may be any of the defined values or
>      // a token (extension method).
> +    FetchUtil util = *(new FetchUtil());
> +    nsresult rv = util.GetValidRequestMethod(method, method);
> +    if (rv != NS_OK ||

this pattern is usually written as |if (NS_FAILED(rv) ... |

@@ +310,1 @@
>          !NS_IsValidHTTPToken(method)) {

Move the NS_IsValidHTTPToken() check to GetValidRequestMethod since XHR also requires it.
Attachment #8658875 - Flags: review?(nsm.nikhil)
Also, please obsolete earlier patches when you upload new ones.
Hopefully the final patch required. Let me know if there's anything that needs changing.
Attachment #8658875 - Attachment is obsolete: true
Attachment #8659632 - Flags: review?(nsm.nikhil)
Comment on attachment 8659632 [details] [diff] [review]
Bug 1199796: Refactor Request and XHR request method validation

Review of attachment 8659632 [details] [diff] [review]:
-----------------------------------------------------------------

This is looking great. Please upload a new patch with a few minor fixes and I will land it for you.

::: dom/base/nsXMLHttpRequest.cpp
@@ +1620,5 @@
>  
>    // Disallow HTTP/1.1 TRACE method (see bug 302489)
>    // and MS IIS equivalent TRACK (see bug 381264)
>    // and CONNECT
> +  // GET, POST, DELETE, HEAD, OPTIONS, PUT methods normalized to upper case

Nit: You can get rid of this entire comment (all 4 lines)

::: dom/fetch/FetchUtil.cpp
@@ +6,5 @@
> +namespace dom {
> +
> +// static
> +nsresult
> +FetchUtil::GetValidRequestMethod(const nsACString& aMethod, nsCString& outMethod) {

Nit: Move the { to a new line.

@@ +7,5 @@
> +
> +// static
> +nsresult
> +FetchUtil::GetValidRequestMethod(const nsACString& aMethod, nsCString& outMethod) {
> +  nsAutoCString upperCaseMethod = (nsAutoCString)aMethod;

This is incorrect. Try |nsAutoCString upperCaseMethod(aMethod)|

@@ +23,5 @@
> +      upperCaseMethod.EqualsLiteral("HEAD") ||
> +      upperCaseMethod.EqualsLiteral("OPTIONS") ||
> +      upperCaseMethod.EqualsLiteral("POST") ||
> +      upperCaseMethod.EqualsLiteral("PUT")) {
> +    outMethod = (nsCString)upperCaseMethod;

You should not need to cast this. Just outMethod = upperCaseMethod should work.

@@ +26,5 @@
> +      upperCaseMethod.EqualsLiteral("PUT")) {
> +    outMethod = (nsCString)upperCaseMethod;
> +  }
> +  else {
> +    outMethod = (nsCString)aMethod; // Case unchanged for non-standard methods

Same here.

::: dom/fetch/FetchUtil.h
@@ +6,5 @@
> +
> +namespace mozilla {
> +namespace dom {
> +
> +class FetchUtil  final {

Nit: there seem to be two spaces between FetchUtil and final.

@@ +12,5 @@
> +  FetchUtil() = delete;
> +
> +public:
> +  /**
> +  * Returns a valid HTTP request method string based on an input method.

Nit: Returns should be replace by "Sets outMethod to... [a valid HTTP request]"

@@ +15,5 @@
> +  /**
> +  * Returns a valid HTTP request method string based on an input method.
> +  * Implements checks and normalization as specified by the Fetch specification.
> +  * Returns NS_ERROR_DOM_SECURITY_ERR if the method is invalid.
> +  * Otherwise returns NS_OK and the nomralized method via outMethod.

Nit: normalized

::: dom/fetch/Request.cpp
@@ +312,5 @@
>      }
>  
>      // Step 14.2
> +    request->ClearCreatedByFetchEvent();
> +    request->SetMethod(method);

This should be outMethod.
Attachment #8659632 - Flags: review?(nsm.nikhil)
Made the requested changes.
Attachment #8659632 - Attachment is obsolete: true
Attachment #8660009 - Flags: review?(nsm.nikhil)
Attachment #8660009 - Flags: review?(nsm.nikhil) → review+
Thanks for the patch Jon. I have landed it with a few formatting fixes.
https://hg.mozilla.org/mozilla-central/rev/4d3bfb6ffeb1
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.