Skip to content

[lint, LS] Add security analyzers and protocol stringer support#614

Merged
turbolent merged 7 commits intomasterfrom
peter/add-security-analyzers
Apr 3, 2026
Merged

[lint, LS] Add security analyzers and protocol stringer support#614
turbolent merged 7 commits intomasterfrom
peter/add-security-analyzers

Conversation

@peterargue
Copy link
Copy Markdown
Contributor

Summary

Adds AST-based security lint analyzers and stringer-generated String() methods for protocol types, based on feedback from onflow/flow-cli#2306.

New lint analyzers:

  • permissive-access — flags access(all) var (mutable) fields in composites, which allow public write access
  • hardcoded-address — flags hex integer literals >= 8 digits as potential hardcoded addresses, suggesting named imports for portability
  • capability-publish — flags capabilities.publish() calls as a reminder to verify entitlements guard the capability (type-checked against Account.Capabilities to avoid false positives)
  • public-account-param — flags access(all) functions that accept auth(...) &Account parameters, exposing broad account access to any caller

Protocol stringer support:

  • Added go:generate stringer for SymbolKind and DiagnosticSeverity in languageserver/protocol
  • These types now have proper String() methods usable by downstream consumers

All analyzers are AST-based (not regex), follow existing analyzer patterns, self-register via init(), and include comprehensive tests.

Test plan

  • 18 new test cases across all 4 analyzers
  • Full existing lint test suite passes
  • Languageserver builds cleanly with generated stringer files

Copy link
Copy Markdown
Member

@SupunS SupunS left a comment

Choose a reason for hiding this comment

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

Thank you for adding these! 🙏
Overall LGTM! Left a couple of suggestions for improving the coverage for some of the linters. I can also help with getting those in, please let me know.

Copy link
Copy Markdown
Member

@turbolent turbolent left a comment

Choose a reason for hiding this comment

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

Thank you for porting these linters over from the CLI PR and rewriting them from regular expressions to analyzers! 👍

@peterargue peterargue requested review from SupunS and turbolent April 1, 2026 16:20
Copy link
Copy Markdown
Member

@SupunS SupunS left a comment

Choose a reason for hiding this comment

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

Nice! 👏

Copy link
Copy Markdown
Member

@turbolent turbolent left a comment

Choose a reason for hiding this comment

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

Nice! The improvements look good, just a couple minor things left.

I'm worried CapabilityPublishAnalyzer will be too noisy and will break developers' CI (e.g. FF DeFi repos), as they treat all warnings as errors and require all diagnostics to be addressed. Given the current lack of allowing these diagnostics to be addressed, we need to either provide a way to address them, or maybe exclude this analyzer from this PR and add it in another PR.

Copy link
Copy Markdown
Member

@turbolent turbolent left a comment

Choose a reason for hiding this comment

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

Thank you!

@turbolent turbolent changed the title Add security lint analyzers and protocol stringer support [lint] Add security lint analyzers and protocol stringer support Apr 3, 2026
@turbolent turbolent changed the title [lint] Add security lint analyzers and protocol stringer support [lint, LS] Add security analyzers and protocol stringer support Apr 3, 2026
@turbolent turbolent merged commit 7987884 into master Apr 3, 2026
10 checks passed
@turbolent turbolent deleted the peter/add-security-analyzers branch April 3, 2026 00:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants