FIX Invalid JSON (quotes not escaped) #2955 #3012

arigliano · 2017-10-10T15:42:40Z

This PR fixes #2955
As described in issue comments, this fix consists of:

The immediate solution would be to wrap the "ckey", when creating the details string, with either
jsonInvalidCharsTransformation() or better JSONHelper::toJsonString() (rest.cpp:114)

Let me know if this is ok or alternative approaches are preferable.

In addition, it was implemented a new functional test, which passes, as well as existing ones.

fgalan · 2017-10-10T16:31:09Z

src/lib/rest/rest.cpp

@@ -111,7 +111,7 @@ static int uriArgumentGet(void* cbDataP, MHD_ValueKind kind, const char* ckey, c

  if (val == NULL || *val == 0)
  {
-    std::string  errorString = std::string("Empty right-hand-side for URI param /") + ckey + "/";
+    std::string  errorString = std::string("Empty right-hand-side for URI param /") + jsonInvalidCharsTransformation(ckey) + "/";


As I mention in #2955 (comment), I think is better to fix this at OrionError::toJson(). I guess it should be easy to move the fix.

fgalan · 2017-10-10T16:33:43Z

src/lib/rest/rest.cpp

@@ -111,7 +111,7 @@ static int uriArgumentGet(void* cbDataP, MHD_ValueKind kind, const char* ckey, c



Please add a line to the CHANGE_NEXT_RELEASE file about this fix. Proposal:

- Fix: broken JSON due to unscaped quotes (") in NGSIv2 error description field (#2955)

fgalan

Thank you for your contribution! Please have a look to the line comment among the code and also the comments in the issue this PR is solving (#2955)

In addition, could you edit the PR body to include a reference to the issue which this PR is fixing (#2955), please? That helps a lot to cross-referencing.

Invalid chars transformation is now performed in the OrionError::toJson() method, by using the **htmlEscape** function. The test was modified accordingly.

- CHANGE_NEXT_RELEASE updated

fgalan · 2017-10-11T09:32:51Z

src/lib/rest/OrionError.cpp

+  free(detailsEscaped);
+
+  return out;
+//	return "{" + JSON_STR("error") + ":" + JSON_STR(reasonPhrase) + "," + JSON_STR("description") + ":" + JSON_STR(details) + "}";


Leftover comment and 3 blank lines should be removed, pls.

Code cleaned

fgalan · 2017-10-11T12:13:51Z

It looks good!

Howerver, note you have conflict with master branch that avoid merging this PR. I guess that is a small line disalingment in the CHANGES_NEXT_RELEASE file.

The solution is to upgrade your branch with master and solve the conflict.

into 2955_quote_in_url_not_escaped # Conflicts: # CHANGES_NEXT_RELEASE

fgalan · 2017-10-11T14:41:17Z

LGTM. Thx!

For the next PR, please end each line comment thread with a Fixed in <commit_hash> message, as detailed in the "pull request protocol" at https://github.com/telefonicaid/fiware-orion/blob/master/doc/manuals/contribution_guidelines.md#pull-request-protocol:

After discussion, the comment thread ends in one of the following ways:

Fixed in <commit hash> in case the discussion involves a fix in the PR branch (which commit hash is included as reference)

NTC, if finally nothing needs to be done (NTC = Nothing To Change)

kzangeli · 2017-10-11T15:25:07Z

LGTM

fgalan · 2017-10-13T09:42:38Z

@arigliano for next PRs please pay attention to execute a full regression of functional tests (i.e. make ft). I mean, not only check that the new or modified .test in the PR are working, but the full set of all them.

I'm saying this because after the changes merged in this PR, the following test were failing:

----------- Failing tests ------------------
  o  1080_batch_query_context/batch_query_context_error_conditions.test
  o  1328_pause_subscriptions/pause_subscriptions_errors.test
  o  1607_query_with_q_and_ranges_but_invalid_operator/query_with_q_and_ranges_but_invalid_operator.test
  o  1678_geosubscriptions/geosubscriptions_errors.test
  o  1705_csub_cache_objects/csub_cache_objects_errors.test

Not a big problem, as they have been quickly fixed in PR #3016, but please take this into account in the future.

Thank you very much for your collaboration!

kzangeli · 2017-10-13T16:33:50Z

For a full regression, better to use make test, as unit tests are not included in make ft.

Another way to run the functional tests is to use the testHarness.sh script directly, especially interesting when testing individual test cases, or a set of test cases.

The script testHarness.sh resides in test/functionsTest, and it has a number of arguments, use the -u argument to see a list of them:

testHarness.sh -u
Usage: testHarness.sh [-u (usage)]
                      [-v (verbose)]
                      [--filter <test filter>]
                      [--match <string for test to match>]
                      [--keep (don't remove output files)]
                      [--dryrun (don't execute any tests)]
                      [--dir <directory>]
                      [--fromIx <index of test where to start>]
                      [--ixList <list of test indexes>]
                      [--stopOnError (stop at first error encountered)]
                      [--no-duration (removes duration mark on successful tests)]
                      [--noCache (force broker to be started with the option --noCache)]
                      [--cache (force broker to be started without the option --noCache)]
                      [--noThreadpool (do not use a threadpool, unless specified by a test case. If not set, a thread pool of 200:20 is used by default in test cases which do not set notificationMode options)]
                      [ <directory or file> ]

I often use --ixList "1 2 3 n" after seeing errors in functests. As you might suspect, it executes only those functests with an index corresponding to one in the 'ixList'.

fgalan · 2017-10-13T16:39:42Z

Another way to run the functional tests is to use the testHarness.sh script directly

In this case, please ensure you have the contextBroker binary correctly installed in your execution path. If you forget that, you can get crazy after doing some changes in your code that compile (i.e. make works) but with test result that doesn't behave as if the changed were done because the compiled binary was not installed in the path... ;)

FIX Invalid JSON (quotes not escaped) telefonicaid#2955

7d4a077

fgalan reviewed Oct 10, 2017

View reviewed changes

fgalan requested changes Oct 10, 2017

View reviewed changes

arigliano added 2 commits October 11, 2017 11:26

FIX Updated - Invalid JSON (quotes not escaped) telefonicaid#2955

2d3a18d

Invalid chars transformation is now performed in the OrionError::toJson() method, by using the **htmlEscape** function. The test was modified accordingly.

FIX Update - Invalid JSON (quotes not escaped) telefonicaid#2955

c05c420

- CHANGE_NEXT_RELEASE updated

fgalan reviewed Oct 11, 2017

View reviewed changes

FIX Updated - telefonicaid#2955

cb112b0

Code cleaned

Merge branch 'master' of https://github.com/telefonicaid/fiware-orion

70801e8

into 2955_quote_in_url_not_escaped # Conflicts: # CHANGES_NEXT_RELEASE

fgalan approved these changes Oct 11, 2017

View reviewed changes

fgalan merged commit 0cc7fd6 into telefonicaid:master Oct 11, 2017

fgalan mentioned this pull request Nov 6, 2017

Invalid JSON (quotes not escaped) #2955

Closed

fgalan mentioned this pull request Jan 10, 2018

2991 invalid description query error response #3056

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FIX Invalid JSON (quotes not escaped) #2955 #3012

FIX Invalid JSON (quotes not escaped) #2955 #3012

arigliano commented Oct 10, 2017 •

edited

Loading

fgalan Oct 10, 2017

fgalan Oct 10, 2017

fgalan left a comment

fgalan Oct 11, 2017

fgalan commented Oct 11, 2017

fgalan commented Oct 11, 2017 •

edited

Loading

kzangeli commented Oct 11, 2017

fgalan commented Oct 13, 2017

kzangeli commented Oct 13, 2017 •

edited

Loading

fgalan commented Oct 13, 2017

		@@ -111,7 +111,7 @@ static int uriArgumentGet(void* cbDataP, MHD_ValueKind kind, const char* ckey, c

FIX Invalid JSON (quotes not escaped) #2955 #3012

FIX Invalid JSON (quotes not escaped) #2955 #3012

Conversation

arigliano commented Oct 10, 2017 • edited Loading

fgalan Oct 10, 2017

Choose a reason for hiding this comment

fgalan Oct 10, 2017

Choose a reason for hiding this comment

fgalan left a comment

Choose a reason for hiding this comment

fgalan Oct 11, 2017

Choose a reason for hiding this comment

fgalan commented Oct 11, 2017

fgalan commented Oct 11, 2017 • edited Loading

kzangeli commented Oct 11, 2017

fgalan commented Oct 13, 2017

kzangeli commented Oct 13, 2017 • edited Loading

fgalan commented Oct 13, 2017

arigliano commented Oct 10, 2017 •

edited

Loading

fgalan commented Oct 11, 2017 •

edited

Loading

kzangeli commented Oct 13, 2017 •

edited

Loading