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.

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

fix: stricter regex for reserved keys #445

Merged
merged 13 commits into from
Nov 23, 2023
Merged

fix: stricter regex for reserved keys #445

merged 13 commits into from
Nov 23, 2023

Conversation

srieteja
Copy link
Contributor

@srieteja srieteja commented Nov 7, 2023

Closes #437

- What I did

  • modified the regex for reserved keys such that there is an explicit check for keys (including public/private tag and pre/postfix atsigns).
  • added a new variable in AtConstants for the key that stores the atsign's blocklist.
  • previously public hidden keys (public:__somekey@atsign) was considered reserved key, which it's not. With the current changes, it will NOT be considered a reserved key.

- How I did it

  • Divided the keys into 2 groups:
  1. Keys that do not contain atsign (e.g. privatekey:at_pkam_publickey, _latestNotificationId)
  2. Keys that contain atsign as prefix/postfix (e.g. '@'bob:shared_key'@'alice, public:publickey'@'alice)
  • These twogroups have their own regex and all these regexes are joined using an "OR" operator

  • Made sure that the visibility of each key (public/private/shared) is also explicitly mentioned in the regex

  • Introduced a new method 'isPartalMatch()' in class Regexutil that does not require a full string match (only for reserved keys). This is required as the public/private part of the key is a lookahead and will not be matched as part of the key

- How to verify it

  • positive and negative tests have been added to ensure that test reserved keys explicitly to ensure proper classification

- Description for the changelog

fix: stricter regex for reserved keys

Copy link
Contributor

@gkc gkc left a comment

Choose a reason for hiding this comment

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

Nice work!

I see atsign-foundation/at_client_sdk#1161 which demonstrates that at_client has / is being tested against this PR - very good. (Note: should test all packages which use at_commons to ensure no breakages anywhere else.)

However, it looks like we have a breaking change; the at_client PR testing shows that the current at_client will fail if using this version of at_commons. Please can you see if there's a reasonable way to avoid a breaking change? if not then we need to bump the major version of at_commons to 4

@srieteja
Copy link
Contributor Author

srieteja commented Nov 17, 2023

The only way we could avoid a breaking change would be to have a secondary check using the old regex in addition to a check with the new one. This would preserve backward compatibility but at the same time undermine the new regex. What do you think @gkc ?
(Note: Purnima and I have tested these changes with all major packages i.e. persistence-secondary, secondary-server and at-client we are currently testing it with all the other consumers of at_commons)

@gkc
Copy link
Contributor

gkc commented Nov 18, 2023

The only way we could avoid a breaking change would be to have a secondary check using the old regex in addition to a check with the new one. This would preserve backward compatibility but at the same time undermine the new regex. What do you think @gkc ? (Note: Purnima and I have tested these changes with all major packages i.e. persistence-secondary, secondary-server and at-client we are currently testing it with all the other consumers of at_commons)

Thanks @srieteja - I think we should bump the major version.

@srieteja
Copy link
Contributor Author

srieteja commented Nov 21, 2023

@purnimavenkatasubbu and I have completed another round of testing with buzz and onboarding_cli; everything looks good with the new regex. Also, I have increased the version to 4.0.0 and updated the changelog as part of 6816bf9.

@srieteja srieteja requested a review from gkc November 23, 2023 05:19
# Conflicts:
#	packages/at_commons/CHANGELOG.md
#	packages/at_commons/pubspec.yaml
Copy link
Contributor

@gkc gkc left a comment

Choose a reason for hiding this comment

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

LGTM - thank you @srieteja and @purnimavenkatasubbu

@gkc gkc merged commit 4cbec2a into trunk Nov 23, 2023
11 checks passed
@gkc gkc deleted the reserved_key_format branch November 23, 2023 13:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RegexUtil.keyType can incorrectly return reservedKey instead of invalidKey
2 participants