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

account-data-matching and bump-seed-canonicalization lesson updates #353

Conversation

Epistetechnician
Copy link

Summary of Changes

Title and Objectives

  • Title and Objectives: The title and objectives remain the same, but the updated version uses "native Rust" instead of "long-form Rust."

Summary Section

  • Summary Section: The updated version provides a more concise and structured summary.
    • Emphasizes the importance of data validation checks.
    • Provides a brief example of how to implement them in both native Rust and Anchor.

Lesson Section

  • Lesson Section: The updated version introduces a new subsection titled "Why Account Data Matching Matters," explaining the risks of missing data validation checks.
    • The example of the update_admin instruction is restructured for clarity.
    • Provides a more detailed explanation of implementing data validation checks using both native Rust and Anchor constraints.

Example: Insecure Admin Update

  • Example: Insecure Admin Update: The updated version renames the program module from data_validation to insecure_admin.
    • The new_admin account is marked as UncheckedAccount with a CHECK comment to indicate it is not read or written in the instruction.

Implementing Data Validation Checks

  • Implementing Data Validation Checks: The updated version includes a more detailed explanation of using has_one and constraint attributes in Anchor.
    • Introduces a custom error enum MyError for better error handling.

Lab Section

  • Lab Section: The lab section is restructured for better readability and flow.
    • Includes a new subsection titled "Understand the Insecure Withdraw Instruction," explaining the critical flaw in the insecure_withdraw instruction.
    • The test for the insecure_withdraw instruction is updated to demonstrate the vulnerability more clearly.

Implementing Secure Withdraw Instruction

  • Implementing Secure Withdraw Instruction: The updated version provides a more detailed explanation of the secure withdraw instruction.
    • Includes the use of has_one constraints in the SecureWithdraw struct to ensure proper validation.

Testing Secure Withdraw Instruction

  • Testing Secure Withdraw Instruction: The updated version includes more detailed tests for the secure_withdraw instruction, demonstrating both unauthorized and authorized access scenarios.

Conclusion

  • Conclusion: The updated version includes a new conclusion section summarizing the importance of account data matching and encouraging readers to review their existing programs for potential vulnerabilities.

Callout Sections

  • Callout Sections: The updated version includes callout sections for additional emphasis on important points and challenges.

Updated lesson for account data matching
Up-to date compatibility and clean format
prettier --write "/Users/shaan.s.patel/Desktop/SF Bounty/account-data-matching-updated.md"
Checked formatting...
All matched files use Prettier code style!
@mikemaccana
Copy link
Collaborator

Thanks @Epistetechnician ! This looks good, expect a full review tomorrow NY time.

Copy link
Collaborator

@mikemaccana mikemaccana left a comment

Choose a reason for hiding this comment

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

An excellent start! I'm going to get a colleague to take a longer look (he's more experienced than Rust than I am) but this looks good,. There's a few small bits, also you need to update the linked unboxed-software repo. See the comments. But this looks good and I'm very confident we can get in. Feel free to explore the rest of the lessons in the program-security - you can see them live at https://solana.com/developers/courses/program-security (the content is hidden from the normal UI until we get all these updates done).

content/courses/program-security/account-data-matching.md Outdated Show resolved Hide resolved
content/courses/program-security/account-data-matching.md Outdated Show resolved Hide resolved
Comment on lines 116 to 117
Anchor provides a more declarative way to implement these checks using
constraints:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Anchor provides a more declarative way to implement these checks using
constraints:
Anchor provides a more declarative way to implement these checks using
[account constraints](https://www.anchor-lang.com/docs/account-constraints):

Copy link
Author

Choose a reason for hiding this comment

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

Smart!

content/courses/program-security/account-data-matching.md Outdated Show resolved Hide resolved
expect(balance.value.uiAmount).to.eq(0)
})
})
it("Insecure withdraw allows unauthorized access", async () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is very minor, but you're combining TSS/BDD style in your tests. It's super common everyone does it. it is supposed to read a little like a sentence, where it is the thing in the describe block, that's why it displays that way on the command line.

You should either make this test("Insecure withdraw allows unauthorized access" ... (probably simplest) or describe('insecure withdraw) ....it("allows unauthorized access" ...

Do that for all the tests.

content/courses/program-security/account-data-matching.md Outdated Show resolved Hide resolved
[guide to installing node in WSL2](https://learn.microsoft.com/en-us/windows/dev-environment/javascript/nodejs-on-wsl)
to install node.
[guide to installing node in WSL2](https://learn.microsoft.com/en-us/windows/dev-environment/javascript/nodejs-on-wsl) to
install node.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this do anything? Might want to get rid of it.

example programs within them:
Within
the repo you will find the following subfolder, each with assorted example
programs within them:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this do anything? Might want to get rid of it.

Resolving comments made by @mikemaccana
# Changes Made to arbitrary-cpi.md

1. **Line Length**: Adjusted lines exceeding 80 characters for improved
   readability and adherence to contributing guidelines.

2. **Consistency**: Ensured consistent capitalization in headers and throughout
   the document.

3. **Code Snippets**: Updated Rust code snippets for compatibility with Anchor
   0.29.0:
   - Used `Result<()>` instead of `ProgramResult` for return types
   - Applied `to_account_info()` method instead of `clone()` for account info
   - Utilized `key()` method instead of `key` for public keys

4. **Explanations**: Enhanced clarity in explanations, particularly for
   arbitrary CPIs and program checks concepts.

5. **Lab Instructions**: Refined step-by-step instructions in the lab section
   for clarity and ease of following.

6. **Links**: Added and reviewed cross-references, ensuring inclusion of all
   necessary links (e.g., link to previous Anchor CPIs lesson).

7. **Formatting**: Verified proper Markdown formatting throughout, including
   correct use of code blocks with language specifications.

8. **Content Structure**: Confirmed overall lesson structure (Summary, Lesson,
   Lab, Challenge) adheres to contributing guidelines.

9. **Callouts**: Verified presence of final callout encouraging user feedback
   on the lesson.
@Epistetechnician Epistetechnician changed the title account-data-matching-lesson-updates program-security-lesson-updates Aug 27, 2024
evaluates to true. Alternatively, you can use `has_one` to check that a target
account field stored on the account matches the key of an account in the
`Accounts` struct.
- Use Anchor constraints to simplify the process:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- Use Anchor constraints to simplify the process:
- Use [Anchor constraints](https://www.anchor-lang.com/docs/account-constraints) to simplify the process:

@@ -38,7 +38,8 @@ examples are self-contained and are available in native Rust (ie, with no
framework), [Anchor](https://www.anchor-lang.com/docs/installation),
[Seahorse](https://seahorse-lang.org/) and it also contains a list of examples
that we would love to
[see as contributions](https://github.com/solana-developers/program-examples?tab=readme-ov-file#examples-wed-love-to-see).
[see as contributions](https://github.com/solana-developers/program-examples?tab=readme-ov-file#examples-wed-love-to-see).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Likewise I'd remove these changes from this PR.

Copy link
Collaborator

@mikemaccana mikemaccana left a comment

Choose a reason for hiding this comment

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

Some small changes and we need to get the repo updates and this is done.

Also can you contact me on telegram or twitter (mikemaccana)? We can bounce ideas back and forth faster there and I might want you do update more lessons!

pub mod account_data_matching {
use super::*;
Clone the starter code from the `starter` branch of
[this repository](https://github.com/Unboxed-Software/solana-account-data-matching).
Copy link
Collaborator

Choose a reason for hiding this comment

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

I've cloned the repo to https://github.com/solana-developers/solana-account-data-matching and invited you as a contributor. Please update the repo then once complete we can get this PR in!

Updated hyperlinks
resolve merge conflicts with abritrary.cpi by reverting to original and fix links in others
Copy link
Collaborator

@mikemaccana mikemaccana left a comment

Choose a reason for hiding this comment

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

Excellent - just fix the lockfile conflict above and we're good to go.

@mikemaccana mikemaccana changed the title program-security-lesson-updates account-data-matching and bump-seed-canonicalization lesson updates Oct 2, 2024
@mikemaccana
Copy link
Collaborator

mikemaccana commented Oct 2, 2024

@Epistetechnician I just updated the title to reflect the files this PR is modifying, but then I also realised #407 modified the same files.

Can you please not suggest changes to the same file in multiple PRs? This makes the review process very complicated as the conversation is split between two different PRs.

Copy link

github-actions bot commented Dec 3, 2024

This pull request has been automatically marked as stale because it has not had recent activity. Remove stale label or comment or this will be closed in 7 days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants