GitHub Issue #122: fix scrubbing and provide test coverage #126

ArturMoczulski · 2017-04-11T16:25:38Z

This pull request covers the following GitHub issues: #122 #52 #31 #15
It's still work in progress but I encourage collaboration.

It includes work on the following:

provide test coverage for scrubbing of $_GET, $_POST, query string, request extras, headers, request body context, $_SESSION, server extras, Data::getCustom(), args and kwargs in frames as mentioned in POST_FORMAT.md, ::log() $extra param
add recursive scrubbing,
add recursive scrubbing of query strings,
remove code duplication and use a single point of scrubbing logic,
refactor placement of scrubbing logic to follow the Python SDK standard

Uses manual recursion to enable scrubbing of entire array fields like "Cookie".

This wasn't quite working very well before, and was masking values that shouldn't have been masked. Turning on strict mode fixes this and should be fairly backward compatible.

…t's not instance dependent; overwriting a constructor in PHPUnit tests is not common practice and there is no need for it in DataBuilderTest, move initialization to setUp(); cover DataBuilder::scrub with detailed scrubbing test case data

…ill stick with previous approach of testing through makeData()

… testGetRequest; wrote a test for DataBuilder::getUrl

…pty, return an empty array - no need to force nulls

…ious commit

…es - better solution in progress

…ramesWithContext so that changing source of tests/DataBuilderTest.php doesn't break PHPUnit test run

…provide more input data coverage

…ffer style

…t before sending out the request to Rollbar so all data can be scrubbed; test coverage for the GET arguments

…oken

ArturMoczulski · 2017-04-14T04:26:52Z

This is ready for review

coryvirok · 2017-04-14T17:10:20Z

src/DataBuilder.php


- protected function scrub($arr, $fields, $replacement = '*')
+ public function scrubArray(&$arr, $replacement = '*')


Do these really need to be public?

ArturMoczulski · 2017-04-14T21:01:21Z

@coryvirok Good point. I made them public earlier for unit testing, but since tests for scrub() already cover the same data structures no need to expose those methods. Just pushed a commit with a change fo this.

…ollbarLogger

coryvirok · 2017-04-19T14:17:05Z

Is there anything blocking this PR from being merged? cc @rokob

rokob · 2017-04-19T15:30:36Z

src/DataBuilder.php

 $val = str_repeat($replacement, 8);
 }
+
+ $val = $dataBuilder->scrub($val, $replacement);


Why not have this in an else block? It would seem that it is okay to not have it in an else because $val just ends up as the string '********' which scrub will just ignore, but why not avoid that call to scrub altogether?

rokob · 2017-04-19T15:34:52Z

tests/DataBuilderTest.php

@@ -28,6 +24,9 @@ public function setUp()
 public function testMakeData()
 {
 $output = $this->dataBuilder->makeData(Level::fromName('error'), "testing", array());
+ /**
+ * @todo test for scrub data in makeData()


Do we need this?

No. I'm going to clean this up.

rokob · 2017-04-19T15:35:56Z

LGTM besides some minor comments

…sue rollbar#122

ArturMoczulski · 2017-04-19T23:39:27Z

@rokob @coryvirok
code climate brought up this code:
https://github.com/ArturMoczulski/rollbar-php/blob/github-122-scrubbing/src/RollbarLogger.php#L35-L40

with this message:
The method log uses an else expression. Else is never necessary and you can simplify the code to work without else.

Do you guys have any suggestion to how to structure this code to not use the else statement? The only way without an else block I can think of is, but that's worse in my opinion:

$response = new Response(0, "Ignored");     
   
if (!$this->config->checkIgnored($payload, $accessToken, $toLog)) {

            $scrubbed = $this->scrub($payload);

            $response = $this->config->send($scrubbed, $accessToken);

}

Also what do you guys about other items brought up by Code Climate?

coryvirok · 2017-04-19T23:59:34Z

I would disregard the CodeClimate suggestion in this instance. As for the other items from CC, they're not high priority as long as we have good coverage. i.e. we can do a cleanup round after release.

rokob · 2017-04-20T00:02:49Z

Also, "never use else" seems like terrible advice.

ArturMoczulski · 2017-04-21T04:04:57Z

@coryvirok @rokob All suggestions addressed so if there is nothing more this can be merged into master.

rokob · 2017-04-21T18:00:59Z

👍

MichaelMackus and others added 30 commits March 28, 2017 14:00

Only scrub fields set with setScrubFields

bfc5158

Use in_array to lookup scrub field

08137d8

Enable scrubbing of entire array fields

bfff32d

Uses manual recursion to enable scrubbing of entire array fields like "Cookie".

Fix in_array parameter order

9300095

Turn on strict check

2476983

This wasn't quite working very well before, and was masking values that shouldn't have been masked. Turning on strict mode fixes this and should be fairly backward compatible.

github-122: add a test for replacement character in the scrub method

4f79bcb

github-122: make getRequest and scrubUrl more testable

1c7e0b1

github-122: scrubUrl can be a static function just like scrub

7e8298b

github-122: switch getUrl to public so it's testable

13642af

github-122: do not expose more interface methods for now - revert - w…

5dddfdc

…ill stick with previous approach of testing through makeData()

github-122: shortened names of some methods; layed out a skeleton for…

79b91f2

… testGetRequest; wrote a test for DataBuilder::getUrl

github-122: test scrubbing , query string, and HTTP_* headers

e1816e2

github-122: scrub request extras recursively

75d48d4

github-122: test scrubbing request extras

043450e

github-122: no need to change the return type - if passed array is em…

84942a3

…pty, return an empty array - no need to force nulls

github-122: returning immediately is actually a better idea than prev…

220d8d4

…ious commit

github-122: test scrubbing body context and

a0c4fbb

github-122: bandaid testFramesWithoutContext by adding some blank lin…

bf0aaba

…es - better solution in progress

github-122: remove hardcoded line values and expected string in testF…

5a37ac4

…ramesWithContext so that changing source of tests/DataBuilderTest.php doesn't break PHPUnit test run

github-122: testMakeData got replaced with multiple other tests that …

1b754e3

…provide more input data coverage

github-122: make scrubbing server extras recursive

f5a3bf9

github-122: test scrubbing server extras; change style of todo items

4b34db8

github-122: fix assert messages in some texts

d749d58

github-122: remove server extras todo item

d413286

github-122: test custom in makeData

2714989

github-122-scrubbing: fix coding style with phpcbf to follow code sni…

e1d9951

…ffer style

github-122: make scrub method name more descriptive

02b8090

github-122: put scrubbing in json serialization at the very end, righ…

115af13

…t before sending out the request to Rollbar so all data can be scrubbed; test coverage for the GET arguments

github-122: only scrub the data part of the payload, not the access t…

923dd48

…oken

github-122: clean up code with phpcbf

b0af578

github-122: shorten data providers names

b665aab

This was referenced Apr 14, 2017

Test scrubbing #122

Closed

Option to exclude cookie data in request #52

Closed

Do scrub fields apply to $extra_data #31

Closed

coryvirok reviewed Apr 14, 2017

View reviewed changes

github-122: don't expose details of scrub method

f290356

ArturMoczulski added 5 commits April 14, 2017 21:08

github-122: no need for a separate url scrub test method

1cae7e4

github-122: take scrubbing invocation out of payload and move it to R…

5109d9a

…ollbarLogger

github-122: assign array reference to a variable for PHP5.3

6bf52e2

github-122: shorten method name

0968eaf

github-122: remove unused config

4de97aa

rokob reviewed Apr 19, 2017

View reviewed changes

ArturMoczulski added 2 commits April 19, 2017 23:26

github-122: place scrub in else as per @rokob suggestion in github is…

df8b6b5

…sue rollbar#122

github-122: clean up a @todo item

05accc3

ArturMoczulski self-assigned this Apr 19, 2017

ArturMoczulski added this to the v1.0.0 Release milestone Apr 19, 2017

ArturMoczulski mentioned this pull request Apr 21, 2017

GitHub Issue #127: Add the request body to the item payload #133

Merged

rokob merged commit dc50f08 into rollbar:master Apr 21, 2017

ArturMoczulski deleted the github-122-scrubbing branch April 29, 2017 02:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GitHub Issue #122: fix scrubbing and provide test coverage #126

GitHub Issue #122: fix scrubbing and provide test coverage #126

ArturMoczulski commented Apr 11, 2017 •

edited

Loading

ArturMoczulski commented Apr 14, 2017

coryvirok Apr 14, 2017

ArturMoczulski commented Apr 14, 2017

coryvirok commented Apr 19, 2017

rokob Apr 19, 2017

ArturMoczulski Apr 19, 2017

rokob Apr 19, 2017

ArturMoczulski Apr 19, 2017

rokob commented Apr 19, 2017

ArturMoczulski commented Apr 19, 2017 •

edited

Loading

coryvirok commented Apr 19, 2017

rokob commented Apr 20, 2017

ArturMoczulski commented Apr 21, 2017 •

edited

Loading

rokob commented Apr 21, 2017


		protected function scrub($arr, $fields, $replacement = '*')
		public function scrubArray(&$arr, $replacement = '*')

GitHub Issue #122: fix scrubbing and provide test coverage #126

GitHub Issue #122: fix scrubbing and provide test coverage #126

Conversation

ArturMoczulski commented Apr 11, 2017 • edited Loading

ArturMoczulski commented Apr 14, 2017

coryvirok Apr 14, 2017

Choose a reason for hiding this comment

ArturMoczulski commented Apr 14, 2017

coryvirok commented Apr 19, 2017

rokob Apr 19, 2017

Choose a reason for hiding this comment

ArturMoczulski Apr 19, 2017

Choose a reason for hiding this comment

rokob Apr 19, 2017

Choose a reason for hiding this comment

ArturMoczulski Apr 19, 2017

Choose a reason for hiding this comment

rokob commented Apr 19, 2017

ArturMoczulski commented Apr 19, 2017 • edited Loading

coryvirok commented Apr 19, 2017

rokob commented Apr 20, 2017

ArturMoczulski commented Apr 21, 2017 • edited Loading

rokob commented Apr 21, 2017

ArturMoczulski commented Apr 11, 2017 •

edited

Loading

ArturMoczulski commented Apr 19, 2017 •

edited

Loading

ArturMoczulski commented Apr 21, 2017 •

edited

Loading