Skip to content

feat: Enhance Security, HTTP Robustness, and Git Operations #2116

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

chmouel
Copy link
Member

@chmouel chmouel commented Jun 6, 2025

This PR bundles several important updates focused on enhancing the overall security, reliability, and consistency of the application's operations.

Key Changes:

  • Improved HTTP Server & Client Robustness:

    • HTTP Server Timeouts: Crucial timeouts (ReadHeaderTimeout, ReadTimeout, IdleTimeout) have been added to the HTTP server, significantly enhancing its security and reliability against slow connections or unresponsive clients.
    • HTTP Response Body Error Handling: Robust error handling for closing HTTP response bodies has been implemented, and the list function now properly propagates writer.Flush() errors, leading to more resilient network operations and better error reporting.
  • Stricter Directory Permissions:

    • Directory creation permissions have been tightened from 0755 to 0750. This change restricts read and execute access for 'other' users, improving the default security posture across various components, including sample pipeline generation, globbing tests, and SCM utility functions.
  • Consistent Git Execution Environment:

    • Environment variables (PATH, HOME, LC_ALL, LANG) are now explicitly set for git command execution. This ensures consistent behavior, prevents locale-related issues, and improves the overall reproducibility of git operations within the system.

These changes collectively contribute to a more secure, stable, and predictable application environment.

@Copilot Copilot AI review requested due to automatic review settings June 6, 2025 09:39
@chmouel
Copy link
Member Author

chmouel commented Jun 6, 2025

/retest

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR bundles several security and robustness improvements across HTTP handling, file system permissions, and git execution.

  • Improved HTTP server timeouts and error handling for network operations.
  • Tightened directory and file permissions to enhance security.
  • Set explicit environment variables for consistent git command execution.

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.

Show a summary per file
File Description
test/pkg/scm/scm.go Updated directory creation permissions from 0755 to 0750.
pkg/git/git.go Added explicit environment variable settings for git.
pkg/cmd/tknpac/list/list.go Propagated w.Flush() error directly.
pkg/cmd/tknpac/info/globbing_test.go Updated directory creation permissions from 0755 to 0750.
pkg/cmd/tknpac/generate/generate.go Updated directory and file permissions for enhanced security.
pkg/cmd/tknpac/bootstrap/web.go Introduced HTTP server timeouts for improved robustness.
pkg/cmd/tknpac/bootstrap/route.go Added error handling when closing the HTTP response body.
pkg/adapter/adapter.go Added HTTP server timeouts to the adapter’s listener.
Comments suppressed due to low confidence (2)

pkg/cmd/tknpac/generate/generate.go:198

  • Consider adding a brief comment to explain the rationale behind tightening the directory permissions from 0755 to 0750 to enhance clarity for future maintainers.
if err := os.MkdirAll(dirPath, 0o750); err != nil {

pkg/cmd/tknpac/generate/generate.go:229

  • It would be beneficial to document why the file permissions were changed from 0644 to 0600, noting any security trade-offs to help future developers understand this decision.
err = os.WriteFile(fpath, tmpl.Bytes(), 0o600)

@PuneetPunamiya
Copy link
Contributor

/retest

@zakisk zakisk force-pushed the stupid-security-ppl branch from 9d6609f to adb1337 Compare June 13, 2025 11:26
chmouel added 4 commits June 18, 2025 10:33
- Added ReadHeaderTimeout, ReadTimeout, and IdleTimeout to HTTP server
  for improved security and reliability
- Ensured consistent environment for git commands by explicitly setting
  PATH, HOME, LC_ALL, and LANG
- Prevented locale-related issues and improved reproducibility of git
  operations
* Changed directory creation permissions from 0755 to 0750.
* Restricted read and execute access for users other than the owner and group.
* Applied the permission change in sample pipeline generation, globbing tests,
and SCM utility functions.
* Improved the default security posture for created directories.

Signed-off-by: Chmouel Boudjnah <[email protected]>
- Added error handling for closing HTTP response body to improve robustness
- Changed list function to return result of writer flush for better
  error reporting

Signed-off-by: Chmouel Boudjnah <[email protected]>
@chmouel chmouel force-pushed the stupid-security-ppl branch from 915cc6f to dba844b Compare June 18, 2025 08:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants