Skip to content

lang: Error on undocumented unsafe account types#1452

Merged
armaniferrante merged 31 commits into
otter-sec:masterfrom
tomlinton:tomlinton/error-on-missing-unsafe-docstring
Feb 17, 2022
Merged

lang: Error on undocumented unsafe account types#1452
armaniferrante merged 31 commits into
otter-sec:masterfrom
tomlinton:tomlinton/error-on-missing-unsafe-docstring

Conversation

@tomlinton

@tomlinton tomlinton commented Feb 16, 2022

Copy link
Copy Markdown
Contributor

Adds a documentation requirement on use of UncheckedAccount and AccountInfo.

edit: Unless the account has a seeds or init constraint in the account attribute.

Safety checks are run as part of the extract_idl function so they will run on anchor build or anchor idl parse commands. The extract IDL step comes after cargo build-bpf in the build command so this doesn't prevent the program from being built. We could do some additional things like remove the built program if it errors but it seemed a bit heavy handed.

There's also a flag for Anchor.toml:

[features]
safety_checks = false

I thought this might be handy for some of the tests (e.g. auction-house) but probably won't document it in case it gets used all the time. 🤣

Accompanying doc PR here.

Closes #1387

Comment thread lang/syn/src/parser/context.rs Outdated
Comment thread lang/syn/src/parser/context.rs
@tomlinton

Copy link
Copy Markdown
Contributor Author

@armaniferrante what is your preferred approach for fixing up auction-house for these tests? I can add

safety_checks = false

to features in Anchor.toml to skip it all, or add something like /// SAFETY: This is not intended as production code for each account.

I'm not familiar enough with the program to write sensible comments. I could try and figure it out but it'll hold this up for a while.

@armaniferrante

Copy link
Copy Markdown
Contributor

@armaniferrante what is your preferred approach for fixing up auction-house for these tests? I can add

safety_checks = false

to features in Anchor.toml to skip it all, or add something like /// SAFETY: This is not intended as production code for each account.

I'm not familiar enough with the program to write sensible comments. I could try and figure it out but it'll hold this up for a while.

Let's just add safety_checks = false.

Comment thread lang/syn/src/parser/context.rs Outdated
Comment thread lang/syn/src/parser/context.rs
@tomlinton tomlinton marked this pull request as ready for review February 17, 2022 21:10
Comment thread lang/syn/src/parser/context.rs

@armaniferrante armaniferrante left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We should add an integration test to assert that the build does actually fail if CHECK isn't provided to prevent regressions.

@armaniferrante

Copy link
Copy Markdown
Contributor

Writing the test now.

@armaniferrante armaniferrante merged commit 5ff9947 into otter-sec:master Feb 17, 2022
@mrmizz

mrmizz commented Feb 18, 2022

Copy link
Copy Markdown

will this go into 0.22.0 ?

@mrmizz

mrmizz commented Feb 18, 2022

Copy link
Copy Markdown

what is the proper way to use the program.invoke method ?

@mrmizz

mrmizz commented Feb 18, 2022

Copy link
Copy Markdown

what is the proper way to use the program.invoke method ?

okay I see, use the .to_account_info from an anchor checked account like Signer or the like. right?

@archseer

Copy link
Copy Markdown
Contributor

I seem to be getting a false positive on all of our PDAs.

    #[account(seeds = [b"foo", state.key().as_ref()], bump = state.load()?.foo_nonce)]
    pub foo_authority: AccountInfo<'info>,

triggers a warning

@armaniferrante

Copy link
Copy Markdown
Contributor

@archseer this is expected. Here's the rationale #1452 (comment). More discussion on this is welcomed.

@fastestmannr

Copy link
Copy Markdown

This results in a bunch of comments for accounts which are not read (just used for their public key) or for accounts in which we only read/change the lamport balance (should be supported by all accounts). Ideally, this would be a type that enforces just these access methods rather than comments. In my ideal world, UncheckedAccount would only expose lamports and public key and none of the other data in it, and then not need a comment.

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.

Compile time warning whenever AccountInfo or UncheckedAccount is used

7 participants