Skip to content

Include unanmed JSON values in unnamed ARGS #1577

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Include unanmed JSON values in unnamed ARGS #1577

wants to merge 1 commit into from

Conversation

marcstern
Copy link

Issue #1576: unanmed JSON values are not parsed because not included in ARGS.
Include these JSON values in unnamed ARGS to include in parsing

zimmerle pushed a commit that referenced this pull request Nov 10, 2017
It also address the issue #1576 and #1577
@marcstern
Copy link
Author

Remark: running in prod for more than a month on several (big) servers

@marcstern
Copy link
Author

Any chance to accept this PR?

@zimmerle zimmerle added the 2.x Related to ModSecurity version 2.x label Feb 28, 2018
@marcstern
Copy link
Author

JSON standrd (rfc8259) explicitely states that a JSON body can contain a stand-alone value.
Actually, a lot of libraries and widely used applications are using that on some places.
It's a major issue to not support this.

@zimmerle zimmerle self-requested a review September 6, 2018 12:36
@zimmerle
Copy link
Contributor

zimmerle commented Sep 6, 2018

This issue was already addressed in v3. To avoid different behaviors in between the version 2 and 3, we need the version 2 to work in the exactly same fashion as v3. There was a comment explain how it works on v3: #1576 (comment)

It important to keep v2 and v3 to working in the same fashion. Let me know your considerations on how it was implemented on v3.

@marcstern
Copy link
Author

From a functional point of view, it should be closer than the actual situation, no?
Why not implementing it until a complete rewrite is peformed?

@zimmerle
Copy link
Contributor

In branch the v2/dev/issue_1576 there is an alternative where the data structure is somewhat navigable, for instance, the JSON:

{  
   "foo":"bar",
   "mod":"sec",
   "ops":[  
      [  
         "um",
         "um e meio"
      ],
      "dois",
      "tres",
      {  
         "eins":[  
            "zwei",
            "drei"
         ]
      }
   ],
   "whee":"lhebs"
}

Will be mapped to:

ARGS:foo -> bar
ARGS:mod -> sec
ARGS:ops.ops.ops -> um
ARGS:ops.ops.ops -> um e meio
ARGS:ops.ops -> dois
ARGS:ops.ops -> tres
ARGS:ops.ops.eins.eins -> zwei
ARGS:ops.ops.eins.eins -> drei
ARGS:whee -> lhebs

This is not comprehensive as the v3 solution, but gives the user an opportunity to navigate in the JSON structure.

Just for the reference, that would be the mapping with the patch suggested here.

ARGS:foo -> bar
ARGS:mod -> sec
ARGS:ops -> um
ARGS:ops -> um e meio
ARGS:ops -> dois
ARGS:ops -> tres
ARGS:ops.eins -> zwei
ARGS:ops.eins -> drei
ARGS:whee -> lhebs

@victorhora victorhora added this to the v2.9.3 milestone Nov 24, 2018
zimmerle pushed a commit that referenced this pull request Nov 26, 2018
@zimmerle
Copy link
Contributor

Thanks @marcstern.

Merged as part of: 9b6d4b2 and 25e5543.

@zimmerle zimmerle closed this Nov 26, 2018
@marcstern
Copy link
Author

The implemented solution fixes most cases but not all.
Still missed, JSON body with only an int (e.g. 25) or only a string (e.g. "abc").
My pull request solves this.

@zimmerle
Copy link
Contributor

Hi @marcstern,

Do you mind to create a new issue with an example to illustrate the scenario?

@marcstern
Copy link
Author

A JSON request with only a number or a string, nothing else.
Example:
--29000000-B--
POST /test/json/test.html HTTP/1.1
content-type: application/json
Host: test
Content-Length: 2
User-Agent: Mozilla 4.0
Accept: text/html, image/gif, image/jpeg, *; q=.2, /; q=.2

--29000000-C--
25
--29000000-F--

@marcstern
Copy link
Author

Any feedback?

1 similar comment
@marcstern
Copy link
Author

Any feedback?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.x Related to ModSecurity version 2.x pr available
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants