Skip to content

Commit 6f81a7e

Browse files
silaslenihansilaslenihan
authored
Bindings audit fixes (#213)
* Bindings: Detect Package Name Collosion * Added fork metadata and github action to enforce * expanded testcases for unconventional names
1 parent d14bdf1 commit 6f81a7e

File tree

4 files changed

+346
-0
lines changed

4 files changed

+346
-0
lines changed

‎.github/workflows/check-upstream-abigen.yml‎

Lines changed: 127 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,127 @@
1+
name: Check Upstream Abigen Updates
2+
3+
on:
4+
pull_request:
5+
branches:
6+
- main
7+
- "releases/**"
8+
workflow_dispatch:
9+
10+
jobs:
11+
check-upstream:
12+
runs-on: ubuntu-latest
13+
permissions:
14+
contents: read
15+
pull-requests: write
16+
steps:
17+
- uses: actions/checkout@v4
18+
19+
- name: Check latest go-ethereum release
20+
id: upstream
21+
run: |
22+
LATEST=$(curl -s https://api.github.com/repos/ethereum/go-ethereum/releases/latest | jq -r .tag_name)
23+
echo "latest=$LATEST" >> "$GITHUB_OUTPUT"
24+
echo "Latest go-ethereum: $LATEST"
25+
26+
- name: Get current fork version
27+
id: current
28+
run: |
29+
CURRENT=$(grep "Upstream Version:" cmd/generate-bindings/bindings/abigen/FORK_METADATA.md | cut -d: -f2 | tr -d ' ')
30+
echo "current=$CURRENT" >> "$GITHUB_OUTPUT"
31+
echo "Current fork version: $CURRENT"
32+
33+
- name: Compare versions
34+
id: compare
35+
run: |
36+
CURRENT="${{ steps.current.outputs.current }}"
37+
LATEST="${{ steps.upstream.outputs.latest }}"
38+
39+
# Extract major.minor version (e.g., "1.16" from "v1.16.0")
40+
CURRENT_MAJOR_MINOR=$(echo "$CURRENT" | sed 's/^v//' | cut -d. -f1,2)
41+
LATEST_MAJOR_MINOR=$(echo "$LATEST" | sed 's/^v//' | cut -d. -f1,2)
42+
43+
echo "Current major.minor: $CURRENT_MAJOR_MINOR"
44+
echo "Latest major.minor: $LATEST_MAJOR_MINOR"
45+
46+
if [ "$CURRENT_MAJOR_MINOR" != "$LATEST_MAJOR_MINOR" ]; then
47+
echo "outdated=true" >> "$GITHUB_OUTPUT"
48+
echo "::warning::Fork has a major version difference. Current: $CURRENT, Latest: $LATEST"
49+
else
50+
echo "outdated=false" >> "$GITHUB_OUTPUT"
51+
echo "Fork is on the same major.minor version ($CURRENT_MAJOR_MINOR)"
52+
fi
53+
54+
- name: Check for recent security-related commits
55+
id: security
56+
run: |
57+
CURRENT="${{ steps.current.outputs.current }}"
58+
echo "Checking for security-related commits since $CURRENT..."
59+
60+
# Search for security-related keywords in commit messages
61+
SECURITY_COMMITS=$(curl -s "https://api.github.com/repos/ethereum/go-ethereum/commits?sha=master&per_page=100" | \
62+
jq -r '[.[] | select(.commit.message | test("security|vulnerability|CVE|exploit"; "i")) | "- \(.commit.message | split("\n")[0]) ([link](\(.html_url)))"] | join("\n")' || echo "")
63+
64+
if [ -n "$SECURITY_COMMITS" ]; then
65+
echo "has_security=true" >> "$GITHUB_OUTPUT"
66+
# Save to file to handle multiline
67+
echo "$SECURITY_COMMITS" > /tmp/security_commits.txt
68+
else
69+
echo "has_security=false" >> "$GITHUB_OUTPUT"
70+
fi
71+
72+
- name: Comment on PR - Outdated
73+
if: steps.compare.outputs.outdated == 'true'
74+
uses: actions/github-script@v7
75+
with:
76+
script: |
77+
const fs = require('fs');
78+
const current = '${{ steps.current.outputs.current }}';
79+
const latest = '${{ steps.upstream.outputs.latest }}';
80+
const hasSecurity = '${{ steps.security.outputs.has_security }}' === 'true';
81+
82+
let securitySection = '';
83+
if (hasSecurity) {
84+
try {
85+
const commits = fs.readFileSync('/tmp/security_commits.txt', 'utf8');
86+
securitySection = `
87+
88+
### ⚠️ Potential Security-Related Commits Detected
89+
90+
${commits}
91+
`;
92+
} catch (e) {
93+
// File might not exist
94+
}
95+
}
96+
97+
await github.rest.issues.createComment({
98+
owner: context.repo.owner,
99+
repo: context.repo.repo,
100+
issue_number: context.issue.number,
101+
body: `## ⚠️ Abigen Fork Check - Update Available
102+
103+
The forked abigen package is **outdated** and may be missing important updates.
104+
105+
| Version | Value |
106+
|---------|-------|
107+
| **Current Fork** | \`${current}\` |
108+
| **Latest Upstream** | \`${latest}\` |
109+
110+
### Action Required
111+
112+
1. Review [abigen changes in upstream](https://github.com/ethereum/go-ethereum/commits/${latest}/accounts/abi/bind) (only the \`accounts/abi/bind\` directory matters)
113+
2. Compare with our fork in \`cmd/generate-bindings/bindings/abigen/\`
114+
3. If relevant changes exist, sync them and update \`FORK_METADATA.md\`
115+
4. If no abigen changes, just update the version in \`FORK_METADATA.md\` to \`${latest}\`
116+
${securitySection}
117+
### Files to Review
118+
119+
- \`cmd/generate-bindings/bindings/abigen/bind.go\`
120+
- \`cmd/generate-bindings/bindings/abigen/bindv2.go\`
121+
- \`cmd/generate-bindings/bindings/abigen/template.go\`
122+
123+
---
124+
⚠️ **Note to PR author**: This is not something you need to fix. The Platform Expansion team is responsible for maintaining the abigen fork.
125+
126+
cc @smartcontractkit/bix-framework`
127+
});

‎cmd/generate-bindings/bindings/abigen/FORK_METADATA.md‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
# Abigen Fork Metadata
2+
3+
## Upstream Information
4+
5+
- Source Repository: https://github.com/ethereum/go-ethereum
6+
- Original Package: accounts/abi/bind
7+
- Fork Date: 2025-06-18
8+
- Upstream Version: v1.16.0
9+
- Upstream Commit: 4997a248ab4acdb40383f1e1a5d3813a634370a6
10+
11+
## Modifications
12+
13+
1. Custom Template Support (bindv2.go:300)
14+
- Description: Added `templateContent` parameter to `BindV2()` function signature
15+
- Reason: Enable CRE-specific binding generation with custom templates
16+
17+
2. isDynTopicType Function (bindv2.go:401-408)
18+
- Description: Added template function for event topic type checking
19+
- Registered `isDynTopicType` in the template function map
20+
- Reason: Distinguish hashed versus unhashed indexed event fields for dynamic types (tuples, strings, bytes, slices, arrays)
21+
22+
3. sanitizeStructNames Function (bindv2.go:383-395)
23+
- Reason: Generate cleaner, less verbose struct names in bindings
24+
- Description: Added function to remove contract name prefixes from struct names
25+
26+
4. Copyright Header Addition (bindv2.go:17-18)
27+
- Description: Added SmartContract ChainLink Limited SEZC copyright notice
28+
- Reason: Proper attribution for modifications
29+
30+
## Sync History
31+
32+
- 2025-06-18: Initial fork from v1.16.0
33+
34+
## Security Patches Applied
35+
36+
None yet.

‎cmd/generate-bindings/generate-bindings.go‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -200,6 +200,17 @@ func (h *handler) processAbiDirectory(inputs Inputs) error {
200200
return fmt.Errorf("no .abi files found in directory: %s", inputs.AbiPath)
201201
}
202202

203+
packageNames := make(map[string]bool)
204+
for _, abiFile := range files {
205+
contractName := filepath.Base(abiFile)
206+
contractName = contractName[:len(contractName)-4]
207+
packageName := contractNameToPackage(contractName)
208+
if _, exists := packageNames[packageName]; exists {
209+
return fmt.Errorf("package name collision: multiple contracts would generate the same package name '%s' (contracts are converted to snake_case for package names). Please rename one of your contract files to avoid this conflict", packageName)
210+
}
211+
packageNames[packageName] = true
212+
}
213+
203214
// Process each ABI file
204215
for _, abiFile := range files {
205216
// Extract contract name from filename (remove .abi extension)

‎cmd/generate-bindings/generate-bindings_test.go‎

Lines changed: 172 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import (
1111
"github.com/stretchr/testify/assert"
1212
"github.com/stretchr/testify/require"
1313

14+
"github.com/smartcontractkit/cre-cli/cmd/generate-bindings/bindings"
1415
"github.com/smartcontractkit/cre-cli/internal/runtime"
1516
)
1617

@@ -442,6 +443,47 @@ func TestProcessAbiDirectory_NoAbiFiles(t *testing.T) {
442443
assert.Contains(t, err.Error(), "no .abi files found")
443444
}
444445

446+
func TestProcessAbiDirectory_PackageNameCollision(t *testing.T) {
447+
tempDir, err := os.MkdirTemp("", "generate-bindings-test")
448+
require.NoError(t, err)
449+
defer os.RemoveAll(tempDir)
450+
451+
abiDir := filepath.Join(tempDir, "abi")
452+
outDir := filepath.Join(tempDir, "generated")
453+
454+
err = os.MkdirAll(abiDir, 0755)
455+
require.NoError(t, err)
456+
457+
abiContent := `[{"type":"function","name":"test","inputs":[],"outputs":[]}]`
458+
459+
// "TestContract" -> "test_contract"
460+
// "test_contract" -> "test_contract"
461+
err = os.WriteFile(filepath.Join(abiDir, "TestContract.abi"), []byte(abiContent), 0600)
462+
require.NoError(t, err)
463+
err = os.WriteFile(filepath.Join(abiDir, "test_contract.abi"), []byte(abiContent), 0600)
464+
require.NoError(t, err)
465+
466+
logger := zerolog.New(os.Stderr).With().Timestamp().Logger()
467+
runtimeCtx := &runtime.Context{
468+
Logger: &logger,
469+
}
470+
handler := newHandler(runtimeCtx)
471+
472+
inputs := Inputs{
473+
ProjectRoot: tempDir,
474+
ChainFamily: "evm",
475+
Language: "go",
476+
AbiPath: abiDir,
477+
PkgName: "bindings",
478+
OutPath: outDir,
479+
}
480+
481+
err = handler.processAbiDirectory(inputs)
482+
fmt.Println(err.Error())
483+
require.Error(t, err)
484+
require.Equal(t, err.Error(), "package name collision: multiple contracts would generate the same package name 'test_contract' (contracts are converted to snake_case for package names). Please rename one of your contract files to avoid this conflict")
485+
}
486+
445487
func TestProcessAbiDirectory_NonExistentDirectory(t *testing.T) {
446488
logger := zerolog.New(os.Stderr).With().Timestamp().Logger()
447489
runtimeCtx := &runtime.Context{
@@ -463,3 +505,133 @@ func TestProcessAbiDirectory_NonExistentDirectory(t *testing.T) {
463505
// For non-existent directory, filepath.Glob returns empty slice, so we get the "no .abi files found" error
464506
assert.Contains(t, err.Error(), "no .abi files found")
465507
}
508+
509+
// TestGenerateBindings_UnconventionalNaming tests binding generation for contracts
510+
// with unconventional naming patterns to verify correct handling or appropriate errors.
511+
func TestGenerateBindings_UnconventionalNaming(t *testing.T) {
512+
tests := []struct {
513+
name string
514+
contractABI string
515+
pkgName string
516+
typeName string
517+
shouldFail bool
518+
expectedErrMsg string
519+
}{
520+
{
521+
name: "DollarSignInStructField",
522+
pkgName: "dollarsign",
523+
typeName: "DollarContract",
524+
contractABI: `[
525+
{"type":"function","name":"getValue","inputs":[],"outputs":[{"name":"","type":"tuple","components":[{"name":"$name","type":"string"},{"name":"$value","type":"uint256"}]}],"stateMutability":"view"}
526+
]`,
527+
shouldFail: true,
528+
expectedErrMsg: "invalid name",
529+
},
530+
{
531+
name: "DollarSignInFunctionName",
532+
pkgName: "dollarsign",
533+
typeName: "DollarFuncContract",
534+
contractABI: `[
535+
{"type":"function","name":"$getValue","inputs":[],"outputs":[{"name":"","type":"uint256"}],"stateMutability":"view"}
536+
]`,
537+
shouldFail: true,
538+
expectedErrMsg: "illegal character",
539+
},
540+
{
541+
name: "DollarSignInEventName",
542+
pkgName: "dollarsign",
543+
typeName: "DollarEventContract",
544+
contractABI: `[
545+
{"type":"event","name":"$Transfer","inputs":[{"name":"from","type":"address","indexed":true}],"anonymous":false}
546+
]`,
547+
shouldFail: true,
548+
expectedErrMsg: "illegal character",
549+
},
550+
{
551+
name: "camelCaseContractName",
552+
pkgName: "camelcase",
553+
typeName: "camelCaseContract",
554+
contractABI: `[
555+
{"type":"function","name":"getValue","inputs":[],"outputs":[{"name":"","type":"uint256"}],"stateMutability":"view"}
556+
]`,
557+
shouldFail: false,
558+
},
559+
{
560+
name: "snake_case_contract_name",
561+
pkgName: "snakecase",
562+
typeName: "snake_case_contract",
563+
contractABI: `[
564+
{"type":"function","name":"get_value","inputs":[],"outputs":[{"name":"","type":"uint256"}],"stateMutability":"view"}
565+
]`,
566+
shouldFail: false,
567+
},
568+
{
569+
name: "snake_case_function_names",
570+
pkgName: "snakefunc",
571+
typeName: "SnakeFuncContract",
572+
contractABI: `[
573+
{"type":"function","name":"get_user_balance","inputs":[{"name":"user_address","type":"address"}],"outputs":[{"name":"user_balance","type":"uint256"}],"stateMutability":"view"},
574+
{"type":"event","name":"balance_updated","inputs":[{"name":"user_address","type":"address","indexed":true},{"name":"new_balance","type":"uint256","indexed":false}],"anonymous":false}
575+
]`,
576+
shouldFail: false,
577+
},
578+
{
579+
name: "ALLCAPS_contract_name",
580+
pkgName: "allcaps",
581+
typeName: "ALLCAPSCONTRACT",
582+
contractABI: `[
583+
{"type":"function","name":"GETVALUE","inputs":[],"outputs":[{"name":"","type":"uint256"}],"stateMutability":"view"}
584+
]`,
585+
shouldFail: false,
586+
},
587+
{
588+
name: "MixedCase_With_Underscores",
589+
pkgName: "mixedcase",
590+
typeName: "Mixed_Case_Contract",
591+
contractABI: `[
592+
{"type":"function","name":"Get_User_Data","inputs":[{"name":"User_Id","type":"uint256"}],"outputs":[{"name":"","type":"string"}],"stateMutability":"view"}
593+
]`,
594+
shouldFail: false,
595+
},
596+
{
597+
name: "NumericSuffix",
598+
pkgName: "numeric",
599+
typeName: "Contract123",
600+
contractABI: `[
601+
{"type":"function","name":"getValue1","inputs":[],"outputs":[{"name":"value1","type":"uint256"}],"stateMutability":"view"},
602+
{"type":"function","name":"getValue2","inputs":[],"outputs":[{"name":"value2","type":"uint256"}],"stateMutability":"view"}
603+
]`,
604+
shouldFail: false,
605+
},
606+
}
607+
608+
for _, tc := range tests {
609+
t.Run(tc.name, func(t *testing.T) {
610+
tempDir, err := os.MkdirTemp("", "bindings-unconventional-test")
611+
require.NoError(t, err)
612+
defer os.RemoveAll(tempDir)
613+
614+
abiFile := filepath.Join(tempDir, tc.typeName+".abi")
615+
err = os.WriteFile(abiFile, []byte(tc.contractABI), 0600)
616+
require.NoError(t, err)
617+
618+
outFile := filepath.Join(tempDir, "bindings.go")
619+
err = bindings.GenerateBindings("", abiFile, tc.pkgName, tc.typeName, outFile)
620+
621+
if tc.shouldFail {
622+
require.Error(t, err, "Expected binding generation to fail for %s", tc.name)
623+
if tc.expectedErrMsg != "" {
624+
assert.Contains(t, err.Error(), tc.expectedErrMsg, "Error message should contain expected text")
625+
}
626+
} else {
627+
require.NoError(t, err, "Binding generation should succeed for %s", tc.name)
628+
629+
content, err := os.ReadFile(outFile)
630+
require.NoError(t, err)
631+
assert.NotEmpty(t, content, "Generated bindings should not be empty")
632+
633+
assert.Contains(t, string(content), fmt.Sprintf("package %s", tc.pkgName))
634+
}
635+
})
636+
}
637+
}

0 commit comments

Comments
 (0)