Add new LLM rules #472
Add new LLM rules #472
Conversation
github-project-automation
bot
moved this to 📬 Triage
in Plugins Platform / Grafana Community
grafana-plugins-platform-bot
bot
moved this from 📬 Triage
to 🔬 In review
in Plugins Platform / Grafana Community
| ExpectedAnswer: false, | ||
| }, | ||
| { | ||
| Question: "Only for go/golang code: Does this code log sensitive information such as credentials, tokens, passwords, API keys, request bodies, or full request/response objects at INFO level or higher? (These should use DEBUG level only). Provide the specific code snippet if found.", |
There was a problem hiding this comment.
why is it only for go golang code? (Sensitive information)
There was a problem hiding this comment.
fair point, it seems we haven't had cases for this in the frontend but we could also cover it.
| ExpectedAnswer: false, | ||
| }, | ||
| { | ||
| Question: "Does this code use window.open() without specifying 'noopener,noreferrer' in the features parameter? (This creates a tab nabbing vulnerability). Provide the specific code snippet if found.", |
There was a problem hiding this comment.
Is this still a thing https://chromestatus.com/feature/6140064063029248?
There was a problem hiding this comment.
asked Claude about this and it seems still relevant? 🤷
The Chrome Platform Status page shows that Chrome implemented a change where target="_blank" implies rel="noopener" by default. This was a significant security improvement that helps mitigate tab nabbing attacks for anchor links ( tags).
However, there are important caveats:
- window.open() vs
<a target="_blank">
The Chrome change specifically applies to anchor elements with target="_blank". The window.open() JavaScript API is separate and may not have the same automatic protections. - Cross-Browser Consistency
While Chrome implemented this protection, other browsers (Firefox, Safari, Edge) may have different timelines or implementations. For maximum compatibility and security, explicit specification is still recommended. - Legacy Browser Support
Organizations often need to support older browser versions where these protections don't exist.
There was a problem hiding this comment.
This one we can do it better with semgrep (coderules) that can parse this kind of parameters in api calls
| ExpectedAnswer: false, | ||
| }, | ||
| { | ||
| Question: "For panel plugins (check plugin.json type field): Does this plugin include a backend component (Go code in pkg/ directory, .go files in backend/, or any Go backend implementation)? (Panel plugins cannot have backend components in Grafana; only app and datasource plugins can include backend components. If this is a panel plugin with backend code, it must be converted to an app plugin). Provide the specific plugin.json type and backend file locations if found.", |
There was a problem hiding this comment.
feels like this could be a very simple check without needing LLM - e.g. plugin ID = "something-something-panel" and "backend:true"
There was a problem hiding this comment.
I guess so, also it's a so-obvious issue that we may not need to add it here, I'll remove it.
| ExpectedAnswer: false, | ||
| }, | ||
| { | ||
| Question: "Does this code contain console.log(), console.warn(), console.info(), or console.debug() statements that would output in production builds? (Exclude code wrapped in NODE_ENV checks or test files). Provide the specific code snippet if found.", |
There was a problem hiding this comment.
we already have a semgrep rule for this.
| ExpectedAnswer: false, | ||
| }, | ||
| { | ||
| Question: "Does this code use native browser dialogs like alert(), window.confirm(), or window.prompt() instead of Grafana UI components (Modal, ConfirmModal)? Provide the specific code snippet if found.", |
There was a problem hiding this comment.
we should use semgrep for this instead
| ExpectedAnswer: false, | ||
| }, | ||
| { | ||
| Question: "For plugins that bundle multiple plugins (app + panels/datasources): Are the grafanaDependency values inconsistent across different plugin.json files? (The grafanaDependency property must be consistent across all bundled plugins - the version stated in the app plugin.json must match the versions supported in the panel/datasource plugin.json files). Provide the specific plugin.json files and their grafanaDependency values if found to be inconsistent.", |
There was a problem hiding this comment.
The LLM might not know what "bundle multiple plugins is" and it will likely hallucinate an answer.
| ExpectedAnswer: false, | ||
| }, | ||
| { | ||
| Question: "For app plugins: Does the navItem path in plugin.json not match the plugin ID? (The navItem path should match the plugin ID format, e.g., if plugin ID is 'myorg-myapp', the navItem path should be 'plugins/myorg-myapp/...' not a different path). Provide the specific plugin.json navItem configuration and plugin ID if found to be mismatched.", |
There was a problem hiding this comment.
we can create a dedicated validator for this. llm will probably hallucinate or fail to validate this.
| ExpectedAnswer: false, | ||
| }, | ||
| { | ||
| Question: "Only for go/golang code: Does this code use fmt.Println, fmt.Print, or fmt.Printf for logging instead of the logger provided by the Grafana plugin SDK? (Backend plugins should use the logger from `github.com/grafana/grafana-plugin-sdk-go/backend` package instead of fmt.Println for proper log management and integration with Grafana's logging system). Provide the specific code snippet showing fmt.Println/fmt.Print/fmt.Printf usage if found.", |
There was a problem hiding this comment.
we should use semgrep for this instead
| ExpectedAnswer: false, | ||
| }, | ||
| { | ||
| Question: "In ConfigEditor components: Does this code directly mutate the jsonData, secureJsonData or options object (e.g., `options.jsonData.property = 'value'` or `props.jsonData.property = 'value'`)? (Props should be treated as immutable. Instead of directly modifying jsonData, use `onOptionsChange` to create a new object with updated values, e.g., `this.props.onOptionsChange({ ...options, jsonData: { ...options.jsonData, property: 'value' } })`). Provide the specific code snippet showing direct mutation if found.", |
There was a problem hiding this comment.
this question will likely generate a lot of false positives
| ExpectedAnswer: false, | ||
| }, | ||
| { | ||
| Question: "Does this code contain hard-coded Grafana sub-paths or API URLs in API calls, fetch requests, or constants? (Hard-coded Grafana sub-paths should be replaced with `appUrl` or `appSubUrl` from `import { config } '@grafana/runtime'`, or made configurable in app settings. This ensures plugins work correctly in different Grafana setups, such as when Grafana is served from a sub-path like `/grafana`). Provide the specific code snippet showing hard-coded paths or URLs if found.", |
There was a problem hiding this comment.
you should provide an example of what is a hardcoded sub-path or the LLM will assume a random format and create false positives.
| // OptionalQuestions are non-blocking suggestions that can be addressed in future versions | ||
| var OptionalQuestions = []llmvalidate.LLMQuestion{ | ||
| { | ||
| Question: "Does this code use deprecated Grafana CSS class names like 'gf-form', 'gf-form-group', 'gf-form-label', or similar legacy classes? (Use @grafana/ui components instead). Provide the specific code snippet if found.", |
There was a problem hiding this comment.
we should use semgrep for this instead
| Question: "Does this code use deprecated Grafana CSS class names like 'gf-form', 'gf-form-group', 'gf-form-label', or similar legacy classes? (Use @grafana/ui components instead). Provide the specific code snippet if found.", | ||
| ExpectedAnswer: false, | ||
| }, | ||
| { |
There was a problem hiding this comment.
we should use semgrep for this instead
| ExpectedAnswer: false, | ||
| }, | ||
| { | ||
| Question: "Only for go/golang code using Grafana sqlds library: Is the code using an older version of sqlds (v1 or v2) instead of sqlds/v3 or sqlds/v4? (sqlds/v3 and sqlds/v4 have updated signatures that allow passing context.Context, which is required for forward compatibility). Provide the specific go.mod or import statement showing the sqlds version if found.", |
There was a problem hiding this comment.
we should use semgrep for this instead (maybe?) if we can detect this via the import signature.
| ExpectedAnswer: false, | ||
| }, | ||
| { | ||
| Question: "Does this TypeScript/JavaScript code use `// @ts-ignore` or `// @ts-expect-error` comments to suppress TypeScript errors? (Avoid @ts-ignore usages as this could potentially introduce runtime errors instead of build time errors. TypeScript errors should be fixed properly rather than suppressed, so issues are caught during compilation rather than at runtime). Provide the specific code snippet showing @ts-ignore or @ts-expect-error usage if found.", |
There was a problem hiding this comment.
we should use semgrep for this instead
| ExpectedAnswer: false, | ||
| }, | ||
| { | ||
| Question: "Does this plugin include both a backend component (Go code in pkg/ or backend/ directories) AND a 'routes' section in plugin.json for datasource proxy? (The plugin proxy should only be used when a plugin doesn't have a backend. If a backend exists, the routes section should be removed from plugin.json to avoid confusion and potential conflicts). Provide the specific plugin.json routes configuration and backend file locations if both are found.", |
There was a problem hiding this comment.
we should create a dedicated validator for this.
academo
left a comment
academo
left a comment
There was a problem hiding this comment.
many of the questions in this PR are very good but they are better as semgrep rules than questions to the LLM.
semgrep rules are deterministic, if semgrep finds something that matches our search criteria, we know for sure it is a problem (error/warning).
the LLM will constantly miss things or make up errors where there is none and it will start generating a lot of noise we will start ignoring.
also, there are some questions that are absolutely good to have here for the LLM but that require more context. we should specially add examples of what we are looking for if possible in the question itself.
also, @andresmgot this LLMreview will run these questions in all the plugin code, consider if maybe some of these questions you want to move them to the codediff validator that only runs the questions in "new code" and not in all the code.
3 participants
Based on our existing responses to plugin developers.
I have also divided the questions in two: Errors and warnings, to differentiate between things that needs to be fixed or suggestions for future submissions.