Opened 6 years ago

Closed 6 years ago

#7110 closed defect (fixed)

Do real parsing of boolean query string parameters

Reported by: dcf Owned by: dcf
Priority: Low Milestone:
Component: Archived/Flashproxy Version:
Severity: Keywords:
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

The debug query string parameter accepts anything, even values like "0" and false, as a way to turn on debug mode.

http://crypto.stanford.edu/flashproxy/embed.html?debug=0

Add a get_query_string_param_boolean function and use it to read the debug value. The function should return true when it receives any of these three strings (and only these strings; no case-folding or anything else):

  • "true"
  • "1"
  • ""

The function should return false iff it receives either of these strings:

  • "false"
  • "0"

The function should return null in all other cases.

There need to be a couple of simple test cases added to flashproxy-test.js.

Child Tickets

Attachments (4)

flashproxy.js (27.7 KB) - added by cypherpunks 6 years ago.
Added function get_query_string_param_boolean()
flashproxy-test.js (7.0 KB) - added by cypherpunks 6 years ago.
Set verbose back to false.
0001-Patch-for-ticket-7110-adding-get_query_param_boolean.patch (3.2 KB) - added by cypherpunks 6 years ago.
Resubmitting patch with issues fixed.
ticket#7110.patch (4.0 KB) - added by cypherpunks 6 years ago.
New patch fixed according to comments, git apply friendly.

Download all attachments as: .zip

Change History (7)

Changed 6 years ago by cypherpunks

Attachment: flashproxy.js added

Added function get_query_string_param_boolean()

Changed 6 years ago by cypherpunks

Attachment: flashproxy-test.js added

Set verbose back to false.

comment:1 Changed 6 years ago by dcf

Thank you for making these changes. It's easier if you attach patches instead; use

git format-patch HEAD^

or something similar.

get_query_string_param_boolean should work the same way and have the same signature as the other get_query_string_param_* functions. That is, it shouldn't just take a single value; it should take a query string, a parameter name, and a default value.

In other words, the test for the debug parameter should look something like this:

var query = parse_query_string(window.location.search.substr(1));
var DEBUG = get_query_string_param_boolean(query, "debug", false);
if (DEBUG) {
    ....
}

The switch you have written would be shorter (6 lines) and clearer as an if-else.

test_get_query_string_param_boolean looks good, but the test cases should use query strings to match the expected behavior of get_query_string_param_boolean as described above.

There are some places in the patch that add blank lines; avoid making these kinds of unrelated changes. Try git checkout -p to selectively discard parts of your changes.

Changed 6 years ago by cypherpunks

Resubmitting patch with issues fixed.

comment:2 Changed 6 years ago by dcf

Thanks, this looks better. A few more changes before I accept it:

Use four-space indents, not hard tabs.

Get rid of trailing whitespace. git apply complains:

$ git apply 0001-Patch-for-ticket-7110-adding-get_query_param_boolean.patch
0001-Patch-for-ticket-7110-adding-get_query_param_boolean.patch:28: space before tab in indent.
                 expected: true},
0001-Patch-for-ticket-7110-adding-get_query_param_boolean.patch:36: trailing whitespace.
                 expected: null},
0001-Patch-for-ticket-7110-adding-get_query_param_boolean.patch:39: trailing whitespace.
        ];
0001-Patch-for-ticket-7110-adding-get_query_param_boolean.patch:92: trailing whitespace.
        val = query[param]
0001-Patch-for-ticket-7110-adding-get_query_param_boolean.patch:93: trailing whitespace.
        if (val === undefined) {
warning: squelched 1 whitespace error
warning: 6 lines add whitespace errors.

I think get_query_param_boolean can be written more clearly:

    if (val === undefined)
        return default_val;
    else if (val === "true" || val === "1" || val === "")
        return true;
    else if (val === "false" || val === "0")
        return false;
    else
        return null;

The comment above get_query_param_boolean should state exactly which values are considered true, which are considered false, and that all other values return null.

The documentation for the debug parameter at the top of the file should say what values are true, because "(to any value)" is not the case anymore.

Changed 6 years ago by cypherpunks

Attachment: ticket#7110.patch added

New patch fixed according to comments, git apply friendly.

comment:3 Changed 6 years ago by dcf

Resolution: fixed
Status: newclosed
Note: See TracTickets for help on using tickets.