fix: add nil safety guards and missing JWT validation#1062
Open
Kewe63 wants to merge 1 commit into
Open
Conversation
- fix(geth): add missing wildcard to apt lists cleanup in Dockerfile (rm -rf /var/lib/apt/lists/* instead of /var/lib/apt/lists) - fix(dependency_updater): guard against nil Commit on tag response Prevents panic when GitHub returns a tag without a commit object. - fix(dependency_updater): guard against empty commits list on branch tracking. Prevents index-out-of-bounds panic when ListCommits returns an empty response. - fix(entrypoints): add BASE_NODE_L2_ENGINE_AUTH_RAW validation to reth-entrypoint, op-node-entrypoint and base-consensus-entrypoint (nethermind-entrypoint already had this check)
Collaborator
🟡 Heimdall Review Status
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Four distinct issues were identified during code review of the Dockerfiles, entrypoint scripts, and the dependency updater tool:
geth/Dockerfile: The apt-get package list cleanup deletes the
/var/lib/apt/listsdirectory itself instead of its contents, which is inconsistent with the other Dockerfiles and deviates from Docker best practices.dependency_updater/dependency_updater.go (line 272):
selectedTag.Commit.SHAis dereferenced without checking whetherselectedTag.Commitis nil. The GitHub API may return a tag without a commit object in edge cases (e.g., lightweight tags, race conditions during tag creation), which would cause a nil pointer dereference panic.dependency_updater/dependency_updater.go (line 296):
branchCommit[0].SHAassumesListCommitsreturns at least one commit. If the branch exists but has no commits (empty repository, deleted branch race), this causes an index-out-of-bounds panic.reth-entrypoint, op-node-entrypoint, base-consensus-entrypoint:
BASE_NODE_L2_ENGINE_AUTH_RAWis written to the JWT secret file without validating it is set and non-empty. If misconfigured, the JWT file becomes empty and the Engine API authentication fails silently — the node starts but cannot communicate with the execution layer.Root Cause
geth/Dockerfile: A typo/oversight — the
/suffix was omitted during authoring while the sibling Dockerfiles (reth/Dockerfile,nethermind/Dockerfile) use the correctlists/pattern.dependency_updater nil commit: The GitHub client pagination code collects tags across pages but assumes every tag returned by
Repositories.ListTagshas a non-nilCommitpointer. GitHub's API contract permits nil commits on tags, but no defensive check was implemented.dependency_updater empty commits: The branch-tracking code path calls
Repositories.ListCommitsand immediately indexes into the result without checking the slice length. An empty branch produces an empty response, which triggers a runtime panic.Missing JWT validation: When the entrypoint scripts were authored, the JWT auth validation was added to
nethermind-entrypointbut was not replicated to the other three entrypoint scripts that also write the JWT secret.Fix
geth/Dockerfile (line 32): Changed
rm -rf /var/lib/apt/liststorm -rf /var/lib/apt/lists/— now matches the pattern used by the other Dockerfiles.dependency_updater (lines 269–273): Added a nil check for
selectedTag.CommitandselectedTag.Commit.SHAbefore dereferencing. When nil, the tag is logged and skipped — the current version is preserved instead of panicking.dependency_updater (lines 300–304): Added
len(branchCommit) == 0guard before indexing. When the commits list is empty, the current version is preserved and a warning is logged.reth-entrypoint, op-node-entrypoint, base-consensus-entrypoint: Added the same validation block that
nethermind-entrypointalready uses:Testing
TestParseVersion,TestValidateVersionUpgrade,TestCompareVersions,TestIsReleaseVersion,TestIsRCVersion,TestIsReleaseOrRCVersion,TestRCVersionOrdering) — 76 test cases total.go build ./...completes without errors.nethermind-entrypoint, which has been running in production.Result
After this PR:
No breaking changes. Behavior is preserved for all correctly configured deployments.