Skip to content

fix: resolve code quality and security issues in 5 oldest files#6258

Open
JiayangLai wants to merge 1 commit intoavelino:mainfrom
JiayangLai:fix/code-quality-and-security-issues
Open

fix: resolve code quality and security issues in 5 oldest files#6258
JiayangLai wants to merge 1 commit intoavelino:mainfrom
JiayangLai:fix/code-quality-and-security-issues

Conversation

@JiayangLai
Copy link
Copy Markdown

Summary

This PR fixes code quality and security issues in the 5 oldest code files in the repository:

Bug Fixes

  1. pkg/markdown/convert.go - Fix nil map panic in IDGenerator

    • The used map was never initialized, causing a panic when Put() was called
    • Added NewIDGenerator() constructor to properly initialize the map
  2. main.go - Fix redundant mkdirAll logic

    • os.MkdirAll already handles the case where the directory exists
    • Removed unnecessary os.Stat check

Security Fixes

  1. .github/scripts/check-quality/main.go - Prevent memory exhaustion

    • Added io.LimitReader(1<<20) (1MB limit) to io.ReadAll calls
    • Prevents DoS via large response bodies from GitHub API
  2. .github/scripts/check-pr-diff/main.go - Prevent command injection

    • Added isValidRef() function to validate GITHUB_BASE_REF and PR_HEAD_SHA
    • Validates that refs only contain alphanumeric, hyphen, underscore, and dot characters

Code Cleanup

  1. pkg/slug/generator.go - Fix misleading FIXME comment
    • Corrected grammatically incorrect comment ("this is should be" → proper description)

Test Plan

  • Run go build ./... to verify code compiles
  • Run existing tests to ensure no regressions

🤖 Generated with Claude Code

Bug fixes:
- Fix nil map panic in IDGenerator (pkg/markdown/convert.go): The 'used' map was never
  initialized, causing a panic when Put() was called. Added NewIDGenerator() constructor.
- Fix redundant mkdirAll logic (main.go): os.MkdirAll already handles existing directories,
  removed unnecessary os.Stat check.

Security fixes:
- Add io.LimitReader to prevent memory exhaustion (check-quality/main.go): Added 1MB limit
  to io.ReadAll calls to prevent DoS via large response bodies.
- Add input validation for git refs (check-pr-diff/main.go): Added isValidRef() function to
  validate GITHUB_BASE_REF and PR_HEAD_SHA environment variables before using in git commands,
  preventing potential command injection.

Code cleanup:
- Fix misleading FIXME comment in slug/generator.go: Corrected grammatically incorrect comment
  and clarified the code intent.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@github-actions
Copy link
Copy Markdown

Automated Quality Checks

Skipped — this PR does not modify README.md, so package quality checks do not apply.

This is expected for maintenance, documentation, or workflow PRs.

Comment on lines +206 to +212
func isValidRef(ref string) bool {
if ref == "" {
return false
}
// Only allow alphanumeric, hyphen, underscore, and dot (common in branch names)
for _, c := range ref {
if !((c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') || (c >= '0' && c <= '9') || c == '-' || c == '_' || c == '.') {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The isValidRef() function incorrectly rejects branch names with slashes (/), causing diffs to silently fall back to the main branch as a base.
Severity: HIGH

Suggested Fix

Update the isValidRef() function to allow the forward slash (/) character. Git branch names commonly use slashes for organization (e.g., feature/add-x). Since the script uses exec.Command, which separates arguments and prevents shell injection, this change is safe and will allow the script to correctly handle a common and valid branch naming convention.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location: .github/scripts/check-pr-diff/main.go#L206-L212

Potential issue: The `isValidRef()` function is overly restrictive and rejects valid Git
branch names that contain a forward slash (`/`), such as `release/1.0`. When a pull
request targets a branch with a slash, the validation fails, causing the script to
silently fall back to using `main` as the base for comparison. This results in the `git
diff` command running against an incorrect base branch, leading to inaccurate PR
validation checks. The check may approve changes that should be flagged or reject valid
changes because it is not comparing against the correct code history.

Did we get this right? 👍 / 👎 to inform future reviews.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants