-
Notifications
You must be signed in to change notification settings - Fork 2
feat: add Kilo Gastown methodology spec and SAST tooling #942
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
Changes from 3 commits
0f98bcc
3d28883
f44d1fd
fa9c9d5
07c09bc
faafc4f
52b6035
b460bab
576b830
eac2d7f
d530d80
39ba5a3
d9cff8a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,86 @@ | ||||||
| name: SAST Full Analysis | ||||||
|
|
||||||
| on: | ||||||
| schedule: | ||||||
| - cron: "0 2 * * *" | ||||||
| workflow_dispatch: | ||||||
|
|
||||||
| permissions: | ||||||
| contents: read | ||||||
| security-events: write | ||||||
|
|
||||||
| jobs: | ||||||
| codeql: | ||||||
| name: CodeQL Analysis | ||||||
| runs-on: ubuntu-latest | ||||||
| timeout-minutes: 30 | ||||||
| strategy: | ||||||
| matrix: | ||||||
| language: [python, cpp, javascript] | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. CodeQL scans wrong languages, misses Go entirelyHigh Severity The CodeQL language matrix is |
||||||
| steps: | ||||||
| - uses: actions/checkout@v4 | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. WARNING: CodeQL matrix missing Go language. The matrix lists
Suggested change
Note: also removed |
||||||
|
|
||||||
| - name: Initialize CodeQL | ||||||
| uses: github/codeql-action/init@v3 | ||||||
| with: | ||||||
| languages: ${{ matrix.language }} | ||||||
|
|
||||||
| - name: Perform CodeQL Analysis | ||||||
| uses: github/codeql-action/analyze@v3 | ||||||
|
||||||
|
|
||||||
|
Comment on lines
+23
to
+30
|
||||||
| trivy-repo: | ||||||
| name: Trivy Repository Scan | ||||||
| runs-on: ubuntu-latest | ||||||
| steps: | ||||||
| - uses: actions/checkout@v4 | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. WARNING: Using Pinning to a mutable branch ( Recommend pinning to a specific version tag or commit SHA: - uses: aquasecurity/trivy-action@v0.28.0 |
||||||
| - uses: aquasecurity/trivy-action@master | ||||||
| with: | ||||||
| scan-type: fs | ||||||
| scan-ref: . | ||||||
| format: sarif | ||||||
| output: trivy-results.sarif | ||||||
| continue-on-error: true | ||||||
|
||||||
|
|
||||||
| - name: Upload Trivy SARIF | ||||||
| uses: github/codeql-action/upload-sarif@v3 | ||||||
| if: always() | ||||||
| with: | ||||||
| sarif_file: trivy-results.sarif | ||||||
| category: trivy | ||||||
|
coderabbitai[bot] marked this conversation as resolved.
|
||||||
|
|
||||||
| full-semgrep: | ||||||
| name: Full Semgrep Analysis | ||||||
| runs-on: ubuntu-latest | ||||||
| timeout-minutes: 20 | ||||||
| steps: | ||||||
| - uses: actions/checkout@v4 | ||||||
|
|
||||||
| - uses: returntocorp/semgrep-action@v1 | ||||||
| with: | ||||||
| config: >- | ||||||
| p/security-audit | ||||||
| p/owasp-top-ten | ||||||
| p/cwe-top-25 | ||||||
| .semgrep-rules/ | ||||||
| generateSarif: true | ||||||
|
|
||||||
| - name: Upload SARIF | ||||||
| uses: github/codeql-action/upload-sarif@v3 | ||||||
| if: always() | ||||||
| with: | ||||||
| sarif_file: semgrep.sarif | ||||||
| category: semgrep-full | ||||||
|
|
||||||
| full-secrets: | ||||||
| name: Full Secret Scan | ||||||
| runs-on: ubuntu-latest | ||||||
| steps: | ||||||
| - uses: actions/checkout@v4 | ||||||
| with: | ||||||
| fetch-depth: 0 | ||||||
|
|
||||||
| - uses: trufflesecurity/trufflehog@main | ||||||
| with: | ||||||
| path: ./ | ||||||
| extra_args: --only-verified --fail | ||||||
| continue-on-error: true | ||||||
|
||||||
| continue-on-error: true |
Copilot
AI
Apr 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The TruffleHog action is referenced from the moving @main branch. Pin to a release tag or commit SHA for reproducibility and supply-chain safety.
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,69 @@ | ||||||
| name: SAST Quick Check | ||||||
|
|
||||||
| on: | ||||||
| pull_request: | ||||||
| push: | ||||||
| branches: [main] | ||||||
|
|
||||||
| permissions: | ||||||
| contents: read | ||||||
| security-events: write | ||||||
| pull-requests: write | ||||||
|
||||||
| pull-requests: write |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WARNING: returntocorp/semgrep-action@v1 is deprecated.
The returntocorp/semgrep-action has been deprecated in favor of running the Semgrep CLI directly or using the semgrep/semgrep-action container. The @v1 tag may stop receiving updates.
Recommended replacement:
- uses: semgrep/semgrep-action@v1Or run semgrep CLI directly:
- run: semgrep ci --config p/security-audit --config p/owasp-top-ten --config p/cwe-top-25 --config .semgrep-rules/ --sarif --output semgrep.sarifThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Security actions pinned to mutable branch references
Medium Severity
trufflesecurity/trufflehog@main and aquasecurity/trivy-action@master are pinned to mutable branch refs rather than commit SHAs or immutable version tags. A compromised upstream repo could inject malicious code that runs in CI with contents: read and security-events: write permissions. This is especially concerning for security-focused workflows running on every PR.
Additional Locations (2)
Copilot
AI
Apr 1, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
trufflehog is invoked with --fail, but the step is marked continue-on-error: true, so verified secrets won’t actually fail the workflow. If the goal is to gate PRs on verified findings, remove continue-on-error; if the goal is reporting-only, drop --fail to avoid implying enforcement.
| extra_args: --only-verified --fail | |
| extra_args: --only-verified |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Secret scan continue-on-error defeats --fail flag
Medium Severity
The trufflehog step uses --fail (exit non-zero on verified secrets) but also sets continue-on-error: true, which means the job still reports success. In sast-quick.yml this runs on PRs, so a pull request containing verified leaked secrets will not be blocked by this check — undermining the purpose of secret scanning in CI.
Additional Locations (1)
Copilot
AI
Apr 1, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
trufflesecurity/trufflehog@main is a floating ref. For supply-chain safety and reproducible results, pin to a version tag or commit SHA (especially in security workflows).
Copilot
AI
Apr 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This step sets --fail in TruffleHog args, but the action step is marked continue-on-error: true, which prevents failures from ever gating. Decide whether secret findings should block (remove continue-on-error) or be informational (remove --fail).
| continue-on-error: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WARNING: Using @main branch reference for trufflehog.
Pinning to @main introduces supply-chain risk and non-reproducible builds. Pin to a specific release tag or commit SHA for security:
- uses: trufflesecurity/trufflehog@v3.82.12There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rust lint job runs on Go-only repository
Medium Severity
The lint-rust job runs cargo clippy on every PR and push to main, but this repository contains no Rust code — no Cargo.toml, no .rs files. This job will either fail (wasting CI time and creating noise) or require cargo setup for nothing. For a Go repo, this likely intended to be a Go lint step.
Copilot
AI
Apr 1, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The lint-rust job runs cargo clippy, but this repo doesn’t contain a Cargo project (no Cargo.toml found). This job will fail on every run. Remove the Rust lint job, or guard it (e.g., only run when Cargo.toml exists / when Rust paths change).
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,52 @@ | ||
| rules: | ||
| - id: circular-module-dependency | ||
| patterns: | ||
| - pattern-either: | ||
| - pattern: | | ||
| use crate::domain::*; | ||
| - pattern: | | ||
| use crate::adapters::*; | ||
| message: Potential circular dependency. Check module import hierarchy. | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The rule |
||
| languages: [rust] | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The repository |
||
| severity: MEDIUM | ||
|
|
||
| - id: layer-violation-adapter-accessing-domain | ||
| patterns: | ||
| - pattern: | | ||
| mod adapters { | ||
| ... | ||
| use crate::domain::... | ||
| ... | ||
| } | ||
| message: Adapter layer should not directly access domain logic. Use ports/traits instead. | ||
| languages: [rust] | ||
| severity: MEDIUM | ||
|
|
||
| - id: mixed-concerns-in-handler | ||
| patterns: | ||
| - pattern: | | ||
| async fn $HANDLER(...) { | ||
| ... | ||
| database.query(...) | ||
| ... | ||
| api.call(...) | ||
| ... | ||
| filesystem.write(...) | ||
| ... | ||
| } | ||
| message: Handler mixes database, API, and filesystem concerns. Consider dependency injection. | ||
| languages: [rust] | ||
| severity: LOW | ||
|
|
||
| - id: direct-database-in-tests | ||
| patterns: | ||
| - pattern: | | ||
| #[test] | ||
| fn $TEST(...) { | ||
| ... | ||
| Database::connect(...) | ||
| ... | ||
| } | ||
| message: Tests should use mocks/fixtures, not direct database connections. | ||
| languages: [rust] | ||
| severity: MEDIUM | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,58 @@ | ||
| rules: | ||
| - id: hardcoded-aws-key | ||
| patterns: | ||
| - pattern-either: | ||
| - pattern: | | ||
| "AKIA[0-9A-Z]{16}" | ||
| - pattern: | | ||
| AKIA[0-9A-Z]{16} | ||
|
||
| message: Potential AWS Access Key detected. Use environment variables instead. | ||
| languages: [generic] | ||
| severity: CRITICAL | ||
|
|
||
| - id: hardcoded-api-key-env | ||
| patterns: | ||
| - pattern-either: | ||
| - pattern: | | ||
| api_key = "..." | ||
| - pattern: | | ||
| apiKey = "..." | ||
| - pattern: | | ||
| API_KEY = "..." | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The patterns for hardcoded keys use the |
||
| message: Hardcoded API key detected. Use environment variables or secrets management. | ||
| languages: [generic] | ||
| severity: HIGH | ||
|
Comment on lines
+8
to
+12
|
||
|
|
||
|
Comment on lines
+2
to
+18
|
||
| - id: hardcoded-password | ||
| patterns: | ||
| - pattern-either: | ||
| - pattern: | | ||
| password = "..." | ||
| - pattern: | | ||
| passwd = "..." | ||
| - pattern: | | ||
| pwd = "..." | ||
| message: Hardcoded password detected. Use environment variables or secrets management. | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| languages: [generic] | ||
| severity: CRITICAL | ||
|
Comment on lines
+19
to
+23
|
||
|
|
||
| - id: hardcoded-slack-webhook | ||
| patterns: | ||
| - pattern: | | ||
| https://hooks.slack.com/services/[A-Z0-9/]+ | ||
| message: Slack webhook URL detected. This should be in environment variables. | ||
| languages: [generic] | ||
| severity: HIGH | ||
|
|
||
| - id: hardcoded-github-token | ||
| patterns: | ||
| - pattern-either: | ||
| - pattern: | | ||
| ghp_[A-Za-z0-9_]{36,255} | ||
| - pattern: | | ||
| gho_[A-Za-z0-9_]{36,255} | ||
| - pattern: | | ||
| ghu_[A-Za-z0-9_]{36,255} | ||
| message: GitHub token detected. Never commit tokens to code. | ||
| languages: [generic] | ||
| severity: CRITICAL | ||
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,59 @@ | ||||||||||||
| rules: | ||||||||||||
| - id: unsafe-deserialization | ||||||||||||
| patterns: | ||||||||||||
| - pattern-either: | ||||||||||||
| - pattern: | | ||||||||||||
| serde_json::from_str($X) | ||||||||||||
| - pattern: | | ||||||||||||
| bincode::deserialize($X) | ||||||||||||
| message: Unsafe deserialization without validation. Consider using `serde` with validation attributes. | ||||||||||||
| languages: [rust] | ||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||||||||||||
| severity: MEDIUM | ||||||||||||
|
|
||||||||||||
| - id: unwrap-without-context | ||||||||||||
| patterns: | ||||||||||||
| - pattern: | | ||||||||||||
| $X.unwrap() | ||||||||||||
| - metavariable-comparison: | ||||||||||||
| metavariable: $X | ||||||||||||
| comparison: match_regex | ||||||||||||
| regex: | | ||||||||||||
| (?!(log|debug|info|warn|error)).* | ||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The regex |
||||||||||||
| message: Use of unwrap() without error handling context. Prefer `?` operator or explicit error handling. | ||||||||||||
| languages: [rust] | ||||||||||||
| severity: MEDIUM | ||||||||||||
|
|
||||||||||||
| - id: todo-without-issue | ||||||||||||
| patterns: | ||||||||||||
| - pattern: | | ||||||||||||
| todo!($MSG) | ||||||||||||
| message: Untracked todo!() macro. Please file an issue and reference it in a comment. | ||||||||||||
| languages: [rust] | ||||||||||||
| severity: LOW | ||||||||||||
|
|
||||||||||||
| - id: unsafe-sql | ||||||||||||
| patterns: | ||||||||||||
| - pattern: | | ||||||||||||
| format!($QUERY, $ARG) | ||||||||||||
|
||||||||||||
| format!($QUERY, $ARG) | |
| format!($QUERY, $ARG, ...) | |
| - metavariable-regex: | |
| metavariable: $QUERY | |
| regex: '(?i)\b(SELECT|INSERT|UPDATE|DELETE|FROM|WHERE|JOIN)\b' |
Copilot
AI
Apr 1, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some patterns here won’t match real code as intended because they use literal identifiers/strings instead of metavariables. For example, return Err(e) only matches when the variable is literally named e, and "localhost:$PORT" will only match that exact string (Semgrep doesn’t treat $PORT as a placeholder inside string literals). Use metavariables (e.g., return Err($E)) and/or pattern-regex for matching within string literal contents.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Semgrep custom rules target Rust, not Go
Medium Severity
All custom semgrep rules in unsafe-patterns.yml and architecture-violations.yml specify languages: [rust] with Rust-specific patterns (unwrap(), format!(), use crate::, etc.). This is a Go repository with no Rust code, so none of these rules will ever match anything. The entire custom rule suite is inert.
Additional Locations (1)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Custom rules not loaded by SAST workflows.
These Go-focused rules are well-designed, but neither sast-quick.yml nor sast-full.yml includes --config .semgrep-rules/ in the Semgrep scan command. The workflows only use the upstream packs (p/security-audit, p/owasp-top-ten, p/cwe-top-25).
To activate these custom rules, add the config flag to both workflow files:
semgrep scan \
--config p/security-audit \
--config p/owasp-top-ten \
--config p/cwe-top-25 \
+ --config .semgrep-rules/ \
--error \Alternatively, if the intent is to gate CI only on upstream packs initially (as mentioned in 04_IMPLEMENTATION_STRATEGY.md), consider documenting this explicitly so future maintainers know these rules exist but are intentionally not enforced yet.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.semgrep-rules/unsafe-patterns.yml around lines 1 - 26, The Semgrep custom
rules defined in .semgrep-rules/unsafe-patterns.yml are not being loaded because
the Semgrep scan steps in the CI workflows do not pass the --config
.semgrep-rules/ flag; update the Semgrep invocation in both sast-quick.yml and
sast-full.yml to include --config .semgrep-rules/ so these rules (ids
shell-command-with-shell, sql-query-built-with-sprintf,
direct-http-get-without-timeout) are applied, or alternatively add a short note
in 04_IMPLEMENTATION_STRATEGY.md indicating the intentional omission if you want
to keep CI gated only on upstream packs.
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,27 @@ | ||||||||||||||
| rules: | ||||||||||||||
| - id: config | ||||||||||||||
| include: | ||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. CRITICAL: Invalid Semgrep configuration structure. The Correct format: rules:
- .semgrep-rules/architecture-violations.yml
- .semgrep-rules/secrets-detection.yml
- .semgrep-rules/unsafe-patterns.yml |
||||||||||||||
| - .semgrep-rules/ | ||||||||||||||
|
||||||||||||||
| rules: | |
| - id: config | |
| include: | |
| - .semgrep-rules/ | |
| include: | |
| - .semgrep-rules/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is redundant exclusion logic in this configuration. The top-level exclude key (lines 6-17) and the paths: exclude: key (lines 20-27) contain overlapping entries (e.g., target/, node_modules/). It is recommended to consolidate all exclusions under paths: exclude: for better maintainability and adherence to standard Semgrep configuration patterns.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Semgrep config file has invalid rule structure
Medium Severity
The .semgrep.yaml rule entry id: config with include: is missing all required semgrep rule fields: pattern/patterns, message, severity, and languages. The include key is not a recognized rule field. Top-level keys like exclude: and max-lines-per-finding: are CLI flags, not valid YAML config keys. This file will cause parse errors when semgrep auto-discovers it (e.g., semgrep --config .), breaking local development usage.


There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
github/codeql-action/*typically requiresactions: readpermission (the existingcodeql.ymlincludes it).sast-full.ymlsets onlycontents: readandsecurity-events: write, which may cause the CodeQL steps to fail with insufficient permissions in some repo/org settings.