Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[feature] Allow using of the complex attribute names for encryptedNative and protectedNative #3032

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

[feature] Allow using of the complex attribute names for encryptedNative and protectedNative #3032

wants to merge 6 commits into from

Conversation

GermanBluefox
Copy link
Contributor

Some adapters, like "email" have complex attribute paths, like transportOptions.auth.pass it is not possible to use protectedNative and encryptedNative for that.

For Features:

Tests

  • I have added tests to test this feature
  • It is not possible to test this feature

Documentation
No documentation required

@GermanBluefox GermanBluefox requested a review from Copilot March 14, 2025 20:32
Copilot
Copilot AI reviewed Mar 14, 2025
View reviewed changes
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR introduces changes to support complex attribute names for the encrypted and protected native configuration properties by updating how attributes are accessed, set, and deleted. Key changes include replacing direct property access with helper functions (getObjectAttribute, setObjectAttribute, deleteObjectAttribute) and updating tests to verify that complex attributes (such as "complex.password") are handled correctly.

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

File Description
packages/adapter/src/lib/adapter/utils.ts Updated helper functions to support complex attribute names by adding wrapper functions for get, set, and delete operations.
packages/adapter/src/lib/adapter/adapter.ts Revised usage of attribute access in encryption/decryption flows and protected native deletion to use the new helper functions.
packages/controller/test/lib/testObjectsFunctions.ts
packages/controller/test/lib/testAdapterHelpers.ts		 Updated tests to include complex attribute scenarios for encryption and protection.

github-advanced-security[bot]
github-advanced-security bot found potential problems Mar 14, 2025
View reviewed changes
packages/adapter/src/lib/adapter/utils.ts
const attrParts = attr.split('.');
for (let j = 0; j < attrParts.length; j++) {
if (j === attrParts.length - 1) {
obj[attrParts[j]] = value;

Check warning

Code scanning / CodeQL

Prototype-polluting function Medium

The property chain
here
Loading
is recursively assigned to
obj
Loading
without guarding against prototype pollution.

Copilot Autofix

AI 23 days ago

To fix the problem, we need to ensure that the setObjectAttribute function does not allow setting properties like __proto__ or constructor that can lead to prototype pollution. We can achieve this by adding checks to skip these properties during the assignment process.

  • Modify the setObjectAttribute function to include checks that prevent setting __proto__ and constructor properties.
  • Ensure that these checks are applied both for direct assignments and for nested assignments within the loop.
Suggested changeset 1
packages/adapter/src/lib/adapter/utils.ts

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/packages/adapter/src/lib/adapter/utils.ts b/packages/adapter/src/lib/adapter/utils.ts
--- a/packages/adapter/src/lib/adapter/utils.ts
+++ b/packages/adapter/src/lib/adapter/utils.ts
@@ -188,2 +188,5 @@
     for (let j = 0; j < attrParts.length; j++) {
+        if (attrParts[j] === '__proto__' || attrParts[j] === 'constructor') {
+            return;
+        }
         if (j === attrParts.length - 1) {
EOF
@@ -188,2 +188,5 @@
for (let j = 0; j < attrParts.length; j++) {
if (attrParts[j] === '__proto__' || attrParts[j] === 'constructor') {
return;
}
if (j === attrParts.length - 1) {
Copilot is powered by AI and may make mistakes. Always verify output.
@mcm1957
Copy link
Contributor

mcm1957 commented Mar 14, 2025 •
edited
Loading

This PR looks like a solution to #846 (or at least as related to that issue). @GermanBluefox please confirm and check that this issue is covered by the PR changes.

Especially encryption of array related elements liek table fileds should be implemented too when changes are made to this area.

@GermanBluefox GermanBluefox marked this pull request as draft March 14, 2025 20:44
@GermanBluefox GermanBluefox marked this pull request as ready for review March 14, 2025 21:48
@GermanBluefox
Copy link
Contributor Author

This PR looks like a solution to #846 (or at least as related to that issue). @GermanBluefox please confirm and check that this issue is covered by the PR changes.

Especially encryption of array related elements liek table fileds should be implemented too when changes are made to this area.

This solves only second part of requirement

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

Copilot code review Copilot Copilot left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@GermanBluefox @mcm1957