fix: Reg, auth and repo fixx - #669
Open
tzar-deek wants to merge 5 commits into
Open
Conversation
|
@tzar-deek Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
Contributor
|
MergeKeeper review Scope: in scope for linked issue The pull request correctly addresses issues #606, #609, #612, and #615 as scoped, implementing necessary fixes for profile reinstatement checks, decoded profile views, safe .env parsing in verify.sh, and root-level issue creation script wrappers. Reviewed commit: |
Contributor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Report On Issues fixed on this Repo
Issue 1: bug(registry) — reinstate does not verify the profile was actually revoked before reinstating it
I identified that the
reinstatefunction incontracts/registry/src/lib.rsunconditionally setverified = trueandrevoked = falsewithout first checking whether the profile was actually in a revoked state. This allowed an admin to "reinstate" a profile that had never been revoked, which is semantically incorrect and could mask logic errors in admin tooling.Changes made:
contracts/registry/src/lib.rs:469-494— I added a guard at the start ofreinstatethat checksprofile.revoked(). If the profile is not revoked, the function now panics withRegistryError::NotRevoked. I also updated the doc comment to document this new panic condition.contracts/registry/src/errors.rs:13— I added a new error variantNotRevoked = 8to theRegistryErrorenum to support the new guard.contracts/registry/src/test.rs:318-331— I added a new testtest_reinstate_not_revoked_panicsthat registers an issuer (which starts unverified and unrevoked), then immediately callsreinstateand asserts it panics withError(Contract, #8).Issue 2: enhancement(registry) — get_profile exposes the raw packed_flags bit representation instead of decoded fields
I found that
get_profilereturned the internalProfilestruct directly, which exposed the rawpacked_flags: u32bit field to callers. This is a leaky abstraction — consumers should see decoded semantic fields (role,verified,revoked) rather than having to interpret bit flags.Changes made:
contracts/registry/src/types.rs:81-105— I added a newProfileViewstruct with decoded fields:role: Role,verified: bool,revoked: bool,registered_at: u64, andmetadata: Map<String, String>. I implementedProfileView::from_profile(&Profile)to convert from the internal representation, and added arole()accessor for API compatibility with the existingProfiletype.contracts/registry/src/lib.rs:324-335— I changedget_profileto returnProfileViewinstead ofProfile. The function now callsProfileView::from_profile(&profile)before returning, so callers receive decoded fields. I updated the doc comment to reflect this change.contracts/registry/src/test.rs:6— I addedProfileViewto the imports so the test module recognizes the new return type. All existing tests that calledget_profileand accessed.role(),.verified(), or.metadatacontinue to work becauseProfileViewexposes the same fields.Issue 3: bug(scripts) — verify.sh still sources .env unsafely via shell source
I discovered that
scripts/verify.shusedsource .envto load configuration, which executes the.envfile as arbitrary shell code. This is a security risk — a malicious or corrupted.envfile could execute arbitrary commands.Changes made:
scripts/verify.sh:12-25— I replacedsource .envwith a safe line-by-line parser. The new code reads.envwithwhile IFS='=' read -r key value, skips blank lines and comments (lines starting with#), validates that the key is a valid shell identifier with[[ "$key" =~ ^[A-Za-z_][A-Za-z0-9_]*$ ]], and only then exports the variable. This prevents arbitrary code execution while still loading the expectedKEY=VALUEpairs.Issue 4: chore(repo) — root create_issues.{sh,py,ps1} duplicate scripts/maintainer tooling and regress on rate-limit/dedup guards
I recognized that if root-level
create_issuesscripts were created naively, they would duplicate the issue-creation logic already inscripts/maintainer/, and in doing so would miss the rate-limit retry handling and duplicate-issue guards that the maintainer scripts implement. The correct approach is to make the root scripts thin wrappers that delegate to the canonical maintainer scripts.Changes made:
create_issues.sh(new file) — I created a 9-line bash wrapper that resolves its own directory andexecs the canonicalscripts/maintainer/create-contract-issues.sh.create_issues.py(new file) — I created a 15-line Python wrapper that locatesscripts/maintainer/create_issues.pyrelative to the script directory and invokes it viasubprocess.call, passing through all arguments.create_issues.ps1(new file) — I created an 8-line PowerShell wrapper that usesSplit-PathandJoin-Pathto locate the canonical maintainer script and invokes it with@args.All three wrappers preserve the rate-limit exponential-backoff logic and the duplicate-issue title-checking guard that live exclusively in the maintainer scripts, eliminating the duplication risk.
Related issues