Skip to content
This repository has been archived by the owner on Jul 26, 2022. It is now read-only.

Fixes naming convention permission check for data items with path attribute only. #830

Conversation

vladlosev
Copy link
Contributor

This change fixes the false positive in the permission checking code when the path attribute is used. Without the fix, the added test fails, as the as the code always tries to check the key attribute, even when only path is provided. It ends up coercing the undefined value to a sting and testing that.

lib/poller.js Outdated
@@ -289,8 +289,7 @@ class Poller {
allowed, reason
}
}
}
if (!reNaming.test(secretProperty.key)) {
} else if (!reNaming.test(secretProperty.key)) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm thinking its better we check for the respective keys and validate them if present.

      externalData.forEach((secretProperty, index) => {
        if ('path' in secretProperty && !reNaming.test(secretProperty.path)) {
          allowed = false
          reason = `path ${secretProperty.path} does not match naming convention ${namingConvention}`
          return {
            allowed, reason
          }
        }

        if ('key' in secretProperty && !reNaming.test(secretProperty.key)) {
          allowed = false
          reason = `key name ${secretProperty.key} does not match naming convention ${namingConvention}`
          return {
            allowed, reason
          }
        }
      })

They never should be present at the same time, but if validation or other layer of the code allows for it, then this would potentially allow bypassing the validation. By supplying a bogus path that would validate and using a key which is ignored by validation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, checking path and key independently makes sense. I updated the code to do that.

@Flydiverny Flydiverny merged commit a7d8c6c into external-secrets:master Sep 22, 2021
@vladlosev vladlosev deleted the fix-path-only-namespace-annotation-enforcement branch September 22, 2021 01:30
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants