feat(allowedsymbols): add structural AST rules for Scanner.Buffer and OpenFile.Close

.github/workflows/allowed-symbols.yml → .github/workflows/analysis.yml

@@ -1,4 +1,4 @@
-name: Allowed Symbols Verification
+name: Static Analysis Verification
 
 # Use pull_request_target so the workflow definition always comes from the
 # base branch (main).  This prevents a PR from editing the workflow file to
@@ -14,8 +14,8 @@ permissions:
   pull-requests: read
 
 jobs:
-  check-allowed-symbols:
-    name: Allowed Symbols Label Check
+  check-analysis:
+    name: Static Analysis Label Check
     runs-on: ubuntu-latest
     timeout-minutes: 10
     steps:
@@ -28,27 +28,27 @@ jobs:
       - name: Fetch PR head
         run: git fetch origin "refs/pull/${{ github.event.pull_request.number }}/head"
 
-      - name: Check for allowedsymbols changes
+      - name: Check for analysis changes
         id: changes
         run: |
-          changed=$(git diff --name-only "${{ github.event.pull_request.base.sha }}...${{ github.event.pull_request.head.sha }}" -- 'allowedsymbols/')
+          changed=$(git diff --name-only "${{ github.event.pull_request.base.sha }}...${{ github.event.pull_request.head.sha }}" -- 'analysis/')
           if [ -n "$changed" ]; then
             echo "modified=true" >> "$GITHUB_OUTPUT"
-            echo "Changed allowedsymbols files:"
+            echo "Changed analysis files:"
             echo "$changed"
           else
             echo "modified=false" >> "$GITHUB_OUTPUT"
-            echo "No allowedsymbols files changed."
+            echo "No analysis files changed."
           fi
 
-      - name: Require verified/allowed_symbols label
+      - name: Require verified/analysis label
         if: steps.changes.outputs.modified == 'true'
         env:
           PR_LABELS: ${{ toJSON(github.event.pull_request.labels.*.name) }}
         run: |
-          if echo "$PR_LABELS" | jq -e 'index("verified/allowed_symbols")' > /dev/null 2>&1; then
-            echo "Label 'verified/allowed_symbols' is present. Check passed."
+          if echo "$PR_LABELS" | jq -e 'index("verified/analysis") // index("verified/allowed_symbols")' > /dev/null 2>&1; then
+            echo "Label present. Check passed."
           else
-            echo "::error::Files in allowedsymbols/ have been modified. A human must review the changes and add the 'verified/allowed_symbols' label to this PR before it can be merged."
+            echo "::error::Files in analysis/ have been modified. A human must review the changes and add the 'verified/analysis' label to this PR before it can be merged."
             exit 1
           fi

allowedsymbols/README.md → analysis/README.md

@@ -1,4 +1,4 @@
-# allowedsymbols
+# analysis
 
 Static enforcement of symbol-level import restrictions for the rshell interpreter.
 
@@ -56,8 +56,70 @@ The tests in this package use Go's `go/parser` and `go/ast` to walk source files
 
 Verification tests additionally inject banned imports or unlisted symbols into a temporary copy of the repo and assert the checker catches them.
 
+## Structural Rules
+
+In addition to symbol-level allowlist checking, the package enforces **structural rules** — code patterns that must (or must not) appear together in the same function scope. These are checked by `checkFileScannerBuffer` and `checkFileOpenFileClose` in `structural.go` and are applied automatically to every file that passes through `checkAllowedSymbols`.
+
+Both rules are also exposed as standalone `go/analysis` analyzers (`ScannerBufferAnalyzer`, `OpenFileCloseAnalyzer`) that can be registered with `go vet` or gopls.
+
+### Rule 1 — `bufio.NewScanner` must call `.Buffer()`
+
+**Why:** `bufio.Scanner` has a fixed default buffer of 64 KiB. Any line longer than that causes `Scanner.Scan()` to return `false` and `Scanner.Err()` to return `bufio.ErrTooLong`. In a shell that must handle arbitrary user input this is a reliability and DoS risk — a single long line silently truncates or aborts processing.
+
+**What is checked:** Every variable assigned from `bufio.NewScanner(...)` must have `.Buffer(...)` called on it within the same function body. Nested function literals (`func() { ... }`) are treated as independent scopes.
+
+**Compliant:**
+```go
+sc := bufio.NewScanner(r)
+sc.Buffer(make([]byte, 4096), maxLineBytes)
+for sc.Scan() { ... }
+```
+
+**Violation:**
+```go
+sc := bufio.NewScanner(r)  // flagged: no sc.Buffer() call
+for sc.Scan() { ... }
+```
+
+### Rule 2 — `callCtx.OpenFile` results must be closed
+
+**Why:** Every open file descriptor consumes a kernel resource. Over repeated script executions, unclosed handles exhaust the process file-descriptor limit and cause all subsequent I/O to fail.
+
+**What is checked:** Every variable assigned from a `.OpenFile(...)` call (any receiver — the check matches the method name, not the receiver type) must have `.Close()` called on it — directly or via `defer` — within the same function body. The checker also tracks **hand-off** assignments: if `f` is reassigned to `rc` and `rc.Close()` is called, `f` is considered closed.
+
+**Compliant — direct close:**
+```go
+f, err := callCtx.OpenFile(ctx, path, os.O_RDONLY, 0)
+if err != nil { return err }
+defer f.Close()
+```
+
+**Compliant — hand-off pattern:**
+```go
+f, err := callCtx.OpenFile(ctx, path, os.O_RDONLY, 0)
+if err != nil { return err }
+rc = f          // hand off to rc
+defer rc.Close() // closes f transitively
+```
+
+**Compliant — return ownership transfer:**
+```go
+func openHelper(callCtx cc) (io.ReadCloser, error) {
+    f, err := callCtx.OpenFile(ctx, path, os.O_RDONLY, 0)
+    if err != nil { return nil, err }
+    return f, nil  // caller is responsible for closing
+}
+```
+
+**Violation:**
+```go
+f, err := callCtx.OpenFile(ctx, path, os.O_RDONLY, 0)
+if err != nil { return err }
+_ = f  // flagged: f is never closed
+```
+
 ## Adding a New Symbol
 
 1. Add a line to the appropriate allowlist (and the per-command sublist if it's a builtin).
 2. Prefix the comment with the correct safety emoji.
-3. Run `go test ./allowedsymbols/` to verify the entry is valid and used.
+3. Run `go test ./analysis/` to verify the entry is valid and used.

allowedsymbols/analyzer.go → analysis/analyzer.go

@@ -3,14 +3,14 @@
 // This product includes software developed at Datadog (https://www.datadoghq.com/).
 // Copyright 2026-present Datadog, Inc.
 
-// Package allowedsymbols provides a go/analysis.Analyzer that enforces
+// Package analysis provides a go/analysis.Analyzer that enforces
 // symbol-level import restrictions on Go source files.
 //
 // The analyzer checks that every imported symbol is in a given allowlist, that
 // no permanently banned packages are imported, and that every symbol in the
 // allowlist is actually used. It reports violations with file:line:col
 // diagnostics.
-package allowedsymbols
+package analysis
 
 import (
 	"fmt"
@@ -39,10 +39,16 @@ type AnalyzerConfig struct {
 //
 // NewAnalyzer panics if any entry in cfg.Symbols is malformed (no dot
 // separator), matching the behaviour of the test-harness variant.
+//
+// NOTE: This analyzer only enforces symbol-level allowlist restrictions. For
+// full static analysis coverage, callers should also register
+// ScannerBufferAnalyzer and OpenFileCloseAnalyzer alongside this one. The
+// test-harness path (checkAllowedSymbols) already applies all three checks
+// automatically.
 func NewAnalyzer(cfg AnalyzerConfig) *analysis.Analyzer {
 	for _, entry := range cfg.Symbols {
 		if strings.LastIndexByte(entry, '.') <= 0 {
-			panic(fmt.Sprintf("allowedsymbols.NewAnalyzer: malformed allowlist entry (no dot): %q", entry))
+			panic(fmt.Sprintf("analysis.NewAnalyzer: malformed allowlist entry (no dot): %q", entry))
 		}
 	}
 
@@ -74,7 +80,7 @@ func NewAnalyzer(cfg AnalyzerConfig) *analysis.Analyzer {
 	}
 
 	return &analysis.Analyzer{
-		Name: "allowedsymbols",
+		Name: "analysis",
 		Doc:  "enforces symbol-level import restrictions via an allowlist",
 		Run:  run,
 	}

allowedsymbols/analyzer_test.go → analysis/analyzer_test.go

@@ -3,7 +3,7 @@
 // This product includes software developed at Datadog (https://www.datadoghq.com/).
 // Copyright 2026-present Datadog, Inc.
 
-package allowedsymbols
+package analysis
 
 import (
 	"testing"
@@ -18,8 +18,8 @@ func TestNewAnalyzer_Metadata(t *testing.T) {
 		Symbols:  []string{"fmt.Println"},
 		ListName: "test",
 	})
-	if a.Name != "allowedsymbols" {
-		t.Errorf("Name = %q, want \"allowedsymbols\"", a.Name)
+	if a.Name != "analysis" {
+		t.Errorf("Name = %q, want \"analysis\"", a.Name)
 	}
 	if len(a.Requires) != 0 {
 		t.Errorf("Requires has %d entries, want 0", len(a.Requires))