Skip to content

Commit 845a1af

Browse files
outdooracornoutdooracorn
committed
REST: Handle invalid field type in JSON Patch
Respond with a more specific error when the JSON Patch contains a field with an invalid type. Handled in `JsonDiffPatchValidator` while we wait for the upstream PR to be merged. swaggest/json-diff#60 Bug: T320705 Change-Id: I92290f5791b14e28e5fcd87d28c8b26d160e899f
1 parent c128c50 commit 845a1af

File tree

8 files changed

+119
-0
lines changed

8 files changed

+119
-0
lines changed

‎repo/rest-api/src/Infrastructure/JsonDiffJsonPatchValidator.php

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
use Swaggest\JsonDiff\MissingFieldException;
88
use Swaggest\JsonDiff\UnknownOperationException;
99
use Wikibase\Repo\RestApi\Domain\Services\JsonPatchValidator;
10+
use Wikibase\Repo\RestApi\Validation\PatchInvalidFieldTypeValidationError;
1011
use Wikibase\Repo\RestApi\Validation\PatchInvalidOpValidationError;
1112
use Wikibase\Repo\RestApi\Validation\PatchMissingFieldValidationError;
1213
use Wikibase\Repo\RestApi\Validation\ValidationError;
@@ -17,6 +18,35 @@
1718
class JsonDiffJsonPatchValidator implements JsonPatchValidator {
1819

1920
public function validate( array $patch, string $source ): ?ValidationError {
21+
// TODO: remove foreach checks when upstream PR merged
22+
// https://github.com/swaggest/json-diff/pull/60
23+
foreach ( $patch as $operation ) {
24+
if ( !is_array( $operation ) ) {
25+
return new ValidationError( '', $source );
26+
}
27+
if ( array_key_exists( 'op', $operation ) && !is_string( $operation['op'] ) ) {
28+
return new PatchInvalidFieldTypeValidationError(
29+
'op',
30+
$source,
31+
[ 'operation' => $operation ]
32+
);
33+
}
34+
if ( array_key_exists( 'path', $operation ) && !is_string( $operation['path'] ) ) {
35+
return new PatchInvalidFieldTypeValidationError(
36+
'path',
37+
$source,
38+
[ 'operation' => $operation ]
39+
);
40+
}
41+
if ( array_key_exists( 'from', $operation ) && !is_string( $operation['from'] ) ) {
42+
return new PatchInvalidFieldTypeValidationError(
43+
'from',
44+
$source,
45+
[ 'operation' => $operation ]
46+
);
47+
}
48+
}
49+
2050
try {
2151
JsonPatch::import( $patch );
2252
} catch ( MissingFieldException $e ) {

‎repo/rest-api/src/Presentation/ErrorResponseToHttpStatus.php

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ class ErrorResponseToHttpStatus {
2222
ErrorResponse::INVALID_OPERATION_CHANGED_PROPERTY => 400,
2323
ErrorResponse::INVALID_PATCH => 400,
2424
ErrorResponse::INVALID_PATCH_OPERATION => 400,
25+
ErrorResponse::INVALID_PATCH_FIELD_TYPE => 400,
2526
ErrorResponse::MISSING_JSON_PATCH_FIELD => 400,
2627
ErrorResponse::PERMISSION_DENIED => 403,
2728
ErrorResponse::ITEM_NOT_FOUND => 404,

‎repo/rest-api/src/UseCases/ErrorResponse.php

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ class ErrorResponse {
2020
public const PATCH_TEST_FAILED = 'patch-test-failed';
2121
public const INVALID_PATCH = 'invalid-patch';
2222
public const INVALID_PATCH_OPERATION = 'invalid-patch-operation';
23+
public const INVALID_PATCH_FIELD_TYPE = 'invalid-patch-field-type';
2324
public const MISSING_JSON_PATCH_FIELD = 'missing-json-patch-field';
2425
public const ITEM_NOT_FOUND = 'item-not-found';
2526
public const ITEM_REDIRECTED = 'redirected-item';

‎repo/rest-api/src/UseCases/PatchItemStatement/PatchItemStatementErrorResponse.php

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44

55
use LogicException;
66
use Wikibase\Repo\RestApi\UseCases\ErrorResponse;
7+
use Wikibase\Repo\RestApi\Validation\PatchInvalidFieldTypeValidationError;
78
use Wikibase\Repo\RestApi\Validation\PatchInvalidOpValidationError;
89
use Wikibase\Repo\RestApi\Validation\PatchMissingFieldValidationError;
910
use Wikibase\Repo\RestApi\Validation\ValidationError;
@@ -37,6 +38,13 @@ public static function newFromValidationError( ValidationError $validationError
3738
"Incorrect JSON patch operation: '$op'",
3839
$validationError->getContext()
3940
);
41+
case $validationError instanceof PatchInvalidFieldTypeValidationError:
42+
$field = $validationError->getValue();
43+
return new self(
44+
ErrorResponse::INVALID_PATCH_FIELD_TYPE,
45+
"The value of '$field' must be of type string",
46+
$validationError->getContext()
47+
);
4048
case $validationError instanceof PatchMissingFieldValidationError:
4149
$field = $validationError->getValue();
4250
return new self(

‎repo/rest-api/src/Validation/PatchInvalidFieldTypeValidationError.php

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
<?php declare( strict_types=1 );
2+
3+
namespace Wikibase\Repo\RestApi\Validation;
4+
5+
/**
6+
* @license GPL-2.0-or-later
7+
*/
8+
class PatchInvalidFieldTypeValidationError extends ValidationError {
9+
10+
}

‎repo/rest-api/tests/mocha/api-testing/PatchItemStatementTest.js

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -244,6 +244,33 @@ describe( 'PATCH statement tests', () => {
244244
assert.include( response.body.message, "'foobar'" );
245245
} );
246246

247+
it( "invalid patch - 'op' is not a string", async () => {
248+
const invalidOperation = { op: { foo: [ 'bar' ], baz: 42 }, path: '/a/b/c', value: 'test' };
249+
const response = await newPatchRequestBuilder( testStatementId, [ invalidOperation ] )
250+
.assertInvalidRequest().makeRequest();
251+
252+
assertValid400Response( response, 'invalid-patch-field-type', { operation: invalidOperation } );
253+
assert.include( response.body.message, "'op'" );
254+
} );
255+
256+
it( "invalid patch - 'path' is not a string", async () => {
257+
const invalidOperation = { op: 'add', path: { foo: [ 'bar' ], baz: 42 }, value: 'test' };
258+
const response = await newPatchRequestBuilder( testStatementId, [ invalidOperation ] )
259+
.assertInvalidRequest().makeRequest();
260+
261+
assertValid400Response( response, 'invalid-patch-field-type', { operation: invalidOperation } );
262+
assert.include( response.body.message, "'path'" );
263+
} );
264+
265+
it( "invalid patch - 'from' is not a string", async () => {
266+
const invalidOperation = { op: 'move', from: { foo: [ 'bar' ], baz: 42 }, path: '/a/b/c' };
267+
const response = await newPatchRequestBuilder( testStatementId, [ invalidOperation ] )
268+
.makeRequest();
269+
270+
assertValid400Response( response, 'invalid-patch-field-type', { operation: invalidOperation } );
271+
assert.include( response.body.message, "'from'" );
272+
} );
273+
247274
it( 'invalid edit tag', async () => {
248275
const invalidEditTag = 'invalid tag';
249276
const response = await newPatchRequestBuilder( testStatementId, [] )

‎repo/rest-api/tests/phpunit/Infrastructure/JsonDiffJsonPatchValidatorTest.php

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
use PHPUnit\Framework\TestCase;
77
use Swaggest\JsonDiff\JsonDiff;
88
use Wikibase\Repo\RestApi\Infrastructure\JsonDiffJsonPatchValidator;
9+
use Wikibase\Repo\RestApi\Validation\PatchInvalidFieldTypeValidationError;
910
use Wikibase\Repo\RestApi\Validation\PatchInvalidOpValidationError;
1011
use Wikibase\Repo\RestApi\Validation\PatchMissingFieldValidationError;
1112

@@ -87,6 +88,38 @@ public function provideInvalidJsonPatch(): Generator {
8788
'invalid',
8889
[ 'operation' => $invalidOperationObject ]
8990
];
91+
92+
$invalidOperationObject = [ 'op' => true, 'path' => '/a/b/c', 'value' => 'foo' ];
93+
yield 'invalid field type - "op" is a boolean not a string' => [
94+
PatchInvalidFieldTypeValidationError::class,
95+
[ $invalidOperationObject ],
96+
'op',
97+
[ 'operation' => $invalidOperationObject ]
98+
];
99+
100+
$invalidOperationObject = [ 'op' => [ 'foo' => [ 'bar' ], 'baz' => 42 ], 'path' => '/a/b/c', 'value' => 'foo' ];
101+
yield 'invalid field type - "op" is an object not a string' => [
102+
PatchInvalidFieldTypeValidationError::class,
103+
[ $invalidOperationObject ],
104+
'op',
105+
[ 'operation' => $invalidOperationObject ]
106+
];
107+
108+
$invalidOperationObject = [ 'op' => 'add', 'path' => [ 'foo', 'bar', 'baz' ], 'value' => 'foo' ];
109+
yield 'invalid field type - "path" is not a string' => [
110+
PatchInvalidFieldTypeValidationError::class,
111+
[ $invalidOperationObject ],
112+
'path',
113+
[ 'operation' => $invalidOperationObject ]
114+
];
115+
116+
$invalidOperationObject = [ 'op' => 'move', 'from' => 42, 'path' => '/a/b/c' ];
117+
yield 'invalid field type - "from" is not a string' => [
118+
PatchInvalidFieldTypeValidationError::class,
119+
[ $invalidOperationObject ],
120+
'from',
121+
[ 'operation' => $invalidOperationObject ]
122+
];
90123
}
91124

92125
public function testValidJsonPatch(): void {

‎repo/rest-api/tests/phpunit/UseCases/PatchItemStatement/PatchItemStatementErrorResponseTest.php

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
use Wikibase\Repo\RestApi\UseCases\ErrorResponse;
77
use Wikibase\Repo\RestApi\UseCases\PatchItemStatement\PatchItemStatementErrorResponse;
88
use Wikibase\Repo\RestApi\UseCases\PatchItemStatement\PatchItemStatementValidator;
9+
use Wikibase\Repo\RestApi\Validation\PatchInvalidFieldTypeValidationError;
910
use Wikibase\Repo\RestApi\Validation\PatchInvalidOpValidationError;
1011
use Wikibase\Repo\RestApi\Validation\PatchMissingFieldValidationError;
1112
use Wikibase\Repo\RestApi\Validation\ValidationError;
@@ -70,6 +71,14 @@ public function provideValidationError(): \Generator {
7071
$context
7172
];
7273

74+
$context = [ 'operation' => [ 'op' => [ 'not', [ 'a' => 'string' ] ], 'path' => '/a/b/c', 'value' => 'test' ] ];
75+
yield 'from invalid patch field type' => [
76+
new PatchInvalidFieldTypeValidationError( 'op', PatchItemStatementValidator::SOURCE_PATCH, $context ),
77+
ErrorResponse::INVALID_PATCH_FIELD_TYPE,
78+
"The value of 'op' must be of type string",
79+
$context
80+
];
81+
7382
yield 'from comment too long' => [
7483
new ValidationError( '500', PatchItemStatementValidator::SOURCE_COMMENT ),
7584
ErrorResponse::COMMENT_TOO_LONG,

0 commit comments

Comments
 (0)