HPCC-32041 WsStore support invalid IPT paths #18763

rpastrana · 2024-06-13T12:59:31Z

Encodes IPT path components
Decodes Key metadata

Type of change:

This change is a bug fix (non-breaking change which fixes an issue).
This change is a new feature (non-breaking change which adds functionality).
This change improves the code (refactor or other change that does not change the functionality)
This change fixes warnings (the fix does not alter the functionality or the generated code)
This change is a breaking change (fix or feature that will cause existing behavior to change).
This change alters the query API (existing queries will have to be recompiled)

Checklist:

Smoketest:

Send notifications about my Pull Request position in Smoketest queue.
Test my draft Pull Request.

Testing:

github-actions · 2024-06-13T13:03:47Z

Jira Issue: https://hpccsystems.atlassian.net//browse/HPCC-32041

Jirabot Action Result:
Workflow Transition: Merge Pending
Updated PR

ghalliday

I am not sure that it is necessary to encode the namespaces - they are explicitly under our control, and it would always report an error if an application had used an invalid value. We should be encoding the key though - as you have.

I would also look at using encodePTreeNameUtf8Char to encode the user name. It would need a slight modification to also encode an at at the start of the string (would be simple to implement by passing in first as a parameter and a little care inside the function)

rpastrana · 2024-06-14T15:22:29Z

I am not sure that it is necessary to encode the namespaces - they are explicitly under our control, and it would always report an error if an application had used an invalid value. We should be encoding the key though - as you have.

I would also look at using encodePTreeNameUtf8Char to encode the user name. It would need a slight modification to also encode an at at the start of the string (would be simple to implement by passing in first as a parameter and a little care inside the function)

The namespaces are provided by the caller, we're not likely going to encounter issues, but we might as well sanitize all user input.
What's the motivation for looking into encodePTreeNameUtf8Char? The function used now ultimately uses encodePTreeNameUtf8Char anyway.

Also, I need to decode the key names in a couple of the wsstore methods as well...

rpastrana · 2024-06-14T18:49:27Z

All pre-existing use cases succeeded, added new tests to account for encode-able IPT path components

rpastrana · 2024-06-17T15:15:23Z

corresponding junit tests: hpcc-systems/hpcc4j#717

ghalliday

I would be inclined to merge (with a fix for encoding ns) and ignore the problem of user names starting with an @

This file would benefit from a future PR that commoned up duplicate code. It would reduce the size and make changes like this easier to review.

esp/services/ws_store/espstorelib/daliKVStore.cpp

ghalliday · 2024-06-18T10:27:23Z

esp/services/ws_store/espstorelib/daliKVStore.cpp


-    xpath.appendf("/%s/%s", ns, key);
+    xpath.appendf("/%s/%s", ns, encodedKey.str());


name space not encoded.

ghalliday · 2024-06-28T10:45:10Z

@rpastrana this is waiting for you - one case of a namespace not being encoded (so it could be inconsistent)

rpastrana · 2024-06-28T13:29:05Z

@rpastrana this is waiting for you - one case of a namespace not being encoded (so it could be inconsistent)

Thanks. This is on my radar. How important is the leading '@' problem you mentioned. That fix involves heavily used logic and will take longer to test.

rpastrana · 2024-06-28T17:02:16Z

@rpastrana this is waiting for you - one case of a namespace not being encoded (so it could be inconsistent)

Thanks. This is on my radar. How important is the leading '@' problem you mentioned. That fix involves heavily used logic and will take longer to test.

UPDATE, just noticed your comment on this matter...

rpastrana · 2024-06-28T18:11:53Z

Updated test case: hpcc-systems/hpcc4j#717

ghalliday

Missed another case.

ghalliday · 2024-07-09T20:13:47Z

esp/services/ws_store/espstorelib/daliKVStore.cpp

@@ -189,34 +213,42 @@ bool CDALIKVStore::fetchKeyProperty(StringBuffer & propval , const char * storen
    if (isEmptyString(storename))
        throw MakeStringException(-1, "DALI Keystore fetchKeyProperty(): Store name not provided");

+    StringBuffer encodedStoreName;
+    encodePTreeName(encodedStoreName, storename);


This isn't being used in line 230

rpastrana · 2024-07-09T22:03:44Z

@ghalliday good catch. Fixed, took another look and hopefully didn't miss any others.

ghalliday

@rpastrana unfortunately a couple of further issues.

One general comment - not to be address in this pr - is that the code would be clearer if the code to encode the values was adjacent to the code that used it.

ghalliday · 2024-07-10T18:14:46Z

esp/services/ws_store/ws_storeService.cpp

+    for (unsigned i = 0; i < keys.length(); i++)
+    {
+        StringBuffer decoded;
+        decodePtreeName(decoded, keys.item(i));


These have already been decoded within the function - so they will currently be decoded twice. That could potentially cause problems with some pathological key names.

ghalliday · 2024-07-10T18:17:11Z

esp/services/ws_store/ws_storeService.cpp

-            kvpair->setKey(attributes->queryName());
+            //it's possible this has been encoded, so decode it
+            StringBuffer decoded;
+            decodePtreeName(decoded, attributes->queryName());


The attribute names have not been encoded - so there should not be matching code to decode them.

The IPT attributes iterator contains all keys under a particular namespace branch, those keys would have been encoded in the set method. I believe decoding is necessary here.

When I read the code it looked like the attributes would be things like DALI_KVSTORE_CREATEDTIME_ATT which have not been encoded (and are also not user specified).

@rpastrana you haven't responded to this comment. I can merge as-is because the decoding will have no effect, but I also don't think it is ever necessary.

no need to decode. Will revert

ghalliday · 2024-07-10T18:19:21Z

esp/services/ws_store/ws_storeService.cpp

+            StringBuffer decoded;
+            decodePtreeName(decoded, name.str());
+
+            kvpair->setKey(decoded.str());


observation: No worth changing - but this breaks the encapsulation - the fact that the keys are encoded shouldn't really need to be known by the calling code.

agreed, I do wish the encoding decoding could have been hidden from the client (ws_store) in this case.

rpastrana · 2024-07-11T14:54:35Z

No longer re-decoding.

rpastrana · 2024-07-11T14:54:52Z

Added new junit test. Results:
Fetching all WsCli@ntT_estStore.Junit_t@estskeys...
All Keys: {a=ddfa, ecl.playground.sample.default=Java Simple, global.test=success, files.rowperpage.default=50, encod@ble=whatever}
Fetching encoded key attributes: WsCli@ntT_estStore.Junit_t@ests.encod@ble
Key Metadata: {@CreateTime=2024-07-11T14:51:17, @Createuser=Juni@tUser}
Fetching encoded key: WsCli@ntT_estStore.Junit_t@ests.encod@ble
Key/value: whatever

ghalliday

@rpastrana I think there is still some unnecessary decoding of attribute names, but I would merge as is since it has no ill effect (just confusing because not needed).
Please sqaush.

- Encodes all user provided IPT path components - Decodes all IPT paths provided to user Signed-off-by: Rodrigo Pastrana <[email protected]>

rpastrana requested a review from ghalliday June 14, 2024 13:08

ghalliday reviewed Jun 14, 2024

View reviewed changes

rpastrana force-pushed the HPCC-32041-WsStoreHandleInvalidIPTPaths branch from d1a442f to 52f90db Compare June 14, 2024 18:47

rpastrana requested a review from ghalliday June 14, 2024 18:48

ghalliday reviewed Jun 18, 2024

View reviewed changes

rpastrana requested a review from ghalliday June 28, 2024 18:11

ghalliday requested changes Jul 9, 2024

View reviewed changes

rpastrana requested a review from ghalliday July 9, 2024 22:02

rpastrana mentioned this pull request Jul 10, 2024

HPCC4J-614 WsStore IPT invalid character tests hpcc-systems/hpcc4j#717

Closed

22 tasks

ghalliday requested changes Jul 10, 2024

View reviewed changes

rpastrana requested a review from ghalliday July 11, 2024 14:55

ghalliday approved these changes Jul 16, 2024

View reviewed changes

HPCC-32041 WsStore support invalid IPT paths

a55eb1c

- Encodes all user provided IPT path components - Decodes all IPT paths provided to user Signed-off-by: Rodrigo Pastrana <[email protected]>

rpastrana force-pushed the HPCC-32041-WsStoreHandleInvalidIPTPaths branch from e1e785d to a55eb1c Compare July 16, 2024 14:10

ghalliday merged commit 4280470 into hpcc-systems:candidate-9.4.x Jul 17, 2024
48 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HPCC-32041 WsStore support invalid IPT paths #18763

HPCC-32041 WsStore support invalid IPT paths #18763

rpastrana commented Jun 13, 2024 •

edited

Loading

github-actions bot commented Jun 13, 2024

ghalliday left a comment

rpastrana commented Jun 14, 2024

rpastrana commented Jun 14, 2024

rpastrana commented Jun 17, 2024

ghalliday left a comment

ghalliday Jun 18, 2024

ghalliday commented Jun 28, 2024

rpastrana commented Jun 28, 2024

rpastrana commented Jun 28, 2024

rpastrana commented Jun 28, 2024

ghalliday left a comment

ghalliday Jul 9, 2024

rpastrana commented Jul 9, 2024

ghalliday left a comment

ghalliday Jul 10, 2024

ghalliday Jul 10, 2024

rpastrana Jul 11, 2024

ghalliday Jul 15, 2024

ghalliday Jul 16, 2024

rpastrana Jul 16, 2024

ghalliday Jul 10, 2024

rpastrana Jul 11, 2024

rpastrana commented Jul 11, 2024

rpastrana commented Jul 11, 2024

ghalliday left a comment


		xpath.appendf("/%s/%s", ns, key);
		xpath.appendf("/%s/%s", ns, encodedKey.str());

HPCC-32041 WsStore support invalid IPT paths #18763

HPCC-32041 WsStore support invalid IPT paths #18763

Conversation

rpastrana commented Jun 13, 2024 • edited Loading

Type of change:

Checklist:

Smoketest:

Testing:

github-actions bot commented Jun 13, 2024

ghalliday left a comment

Choose a reason for hiding this comment

rpastrana commented Jun 14, 2024

rpastrana commented Jun 14, 2024

rpastrana commented Jun 17, 2024

ghalliday left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghalliday commented Jun 28, 2024

rpastrana commented Jun 28, 2024

rpastrana commented Jun 28, 2024

rpastrana commented Jun 28, 2024

ghalliday left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rpastrana commented Jul 9, 2024

ghalliday left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rpastrana commented Jul 11, 2024

rpastrana commented Jul 11, 2024

ghalliday left a comment

Choose a reason for hiding this comment

rpastrana commented Jun 13, 2024 •

edited

Loading