Skip to content

Commit

Permalink
Check integer values before conversion
Browse files Browse the repository at this point in the history
- Fix gosec integer overflow conversion warnings (G115/CWE-190)
- Use the `int` type for primary keys in `datatype` structs to avoid
  having to convert values returned from the database layer
- Check that user supplied `int` values are positive before converting
  them to `uint` values
- Validate that the `a3m.Config.AipCompressionLevel` value is between
  zero and nine [0-9]

[skip-codecov]
  • Loading branch information
djjuhasz committed Sep 11, 2024
1 parent a0d454d commit 263818b
Show file tree
Hide file tree
Showing 36 changed files with 320 additions and 235 deletions.
6 changes: 3 additions & 3 deletions internal/a3m/a3m.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ type CreateAIPActivity struct {
type CreateAIPActivityParams struct {
Name string
Path string
PreservationActionID uint
PreservationActionID int
}

type CreateAIPActivityResult struct {
Expand Down Expand Up @@ -111,7 +111,7 @@ func (a *CreateAIPActivity) Execute(
TranscribeFiles: a.cfg.TranscribeFiles,
PerformPolicyChecksOnOriginals: a.cfg.PerformPolicyChecksOnOriginals,
PerformPolicyChecksOnPreservationDerivatives: a.cfg.PerformPolicyChecksOnPreservationDerivatives,
AipCompressionLevel: int32(a.cfg.AipCompressionLevel),
AipCompressionLevel: a.cfg.AipCompressionLevel,
AipCompressionAlgorithm: a.cfg.AipCompressionAlgorithm,
},
},
Expand Down Expand Up @@ -169,7 +169,7 @@ func savePreservationTasks(
tracer trace.Tracer,
jobs []*transferservice.Job,
pkgsvc package_.Service,
paID uint,
paID int,
) error {
ctx, span := tracer.Start(ctx, "savePreservationTasks")
defer span.End()
Expand Down
31 changes: 27 additions & 4 deletions internal/a3m/config.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,15 @@
package a3m

import transferservice "buf.build/gen/go/artefactual/a3m/protocolbuffers/go/a3m/api/transferservice/v1beta1"
import (
"fmt"

transferservice "buf.build/gen/go/artefactual/a3m/protocolbuffers/go/a3m/api/transferservice/v1beta1"
)

const (
minCompressionLevel = 0
maxCompressionLevel = 9
)

type Config struct {
Name string
Expand All @@ -14,8 +23,22 @@ type Config struct {
Processing
}

// The `Processing` struct represents a configuration for processing various tasks in the transferservice.
// It mirrors the processing configuration fields in transferservice.ProcessingConfig.
func (c *Config) Validate() error {
if c.AipCompressionAlgorithm < minCompressionLevel || c.AipCompressionLevel > maxCompressionLevel {
return fmt.Errorf(
"AipCompressionLevel: %d is outside valid range (%d to %d)",
c.AipCompressionLevel,
minCompressionLevel,
maxCompressionLevel,
)
}

return nil
}

// The `Processing` struct represents a configuration for processing various
// tasks in the transferservice. It mirrors the processing configuration fields
// in transferservice.ProcessingConfig.
type Processing struct {
AssignUuidsToDirectories bool
ExamineContents bool
Expand All @@ -30,7 +53,7 @@ type Processing struct {
TranscribeFiles bool
PerformPolicyChecksOnOriginals bool
PerformPolicyChecksOnPreservationDerivatives bool
AipCompressionLevel int
AipCompressionLevel int32
AipCompressionAlgorithm transferservice.ProcessingConfig_AIPCompressionAlgorithm
}

Expand Down
4 changes: 2 additions & 2 deletions internal/am/job_tracker.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ type JobTracker struct {

// presActionID is the PreservationAction ID that will be the parent ID for
// all saved preservation tasks.
presActionID uint
presActionID int

// savedIDs caches the ID of jobs that have already been saved so we don't
// create duplicates.
Expand All @@ -38,7 +38,7 @@ func NewJobTracker(
clock clockwork.Clock,
jobSvc amclient.JobsService,
pkgSvc package_.Service,
presActionID uint,
presActionID int,
) *JobTracker {
return &JobTracker{
clock: clock,
Expand Down
2 changes: 1 addition & 1 deletion internal/am/job_tracker_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ import (
func TestJobTracker(t *testing.T) {
t.Parallel()

paID := uint(1)
paID := 1
unitID := uuid.New().String()

clock := clockwork.NewFakeClock()
Expand Down
2 changes: 1 addition & 1 deletion internal/am/poll_ingest.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import (
const PollIngestActivityName = "poll-ingest-activity"

type PollIngestActivityParams struct {
PresActionID uint
PresActionID int
SIPID string
}

Expand Down
2 changes: 1 addition & 1 deletion internal/am/poll_ingest_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ func TestPollIngestActivity(t *testing.T) {

clock := clockwork.NewFakeClock()
path := "/var/archivematica/fake/sip"
presActionID := uint(2)
presActionID := 2
sipID := uuid.New().String()

httpError := func(m *amclienttest.MockIngestServiceMockRecorder, statusCode int) {
Expand Down
2 changes: 1 addition & 1 deletion internal/am/poll_transfer.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import (
const PollTransferActivityName = "poll-transfer-activity"

type PollTransferActivityParams struct {
PresActionID uint
PresActionID int
TransferID string
}

Expand Down
2 changes: 1 addition & 1 deletion internal/am/poll_transfer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ var (
func TestPollTransferActivity(t *testing.T) {
t.Parallel()
transferID := uuid.New().String()
presActionID := uint(1)
presActionID := 1
sipID := uuid.New().String()
path := "/var/archivematica/fake/sip"

Expand Down
1 change: 1 addition & 0 deletions internal/config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ type Configuration struct {
func (c *Configuration) Validate() error {
// TODO: should this validate all the fields in Configuration?
return errors.Join(
c.A3m.Validate(),
c.API.Auth.Validate(),
c.BagIt.Validate(),
c.Preprocessing.Validate(),
Expand Down
9 changes: 7 additions & 2 deletions internal/datatypes/package_.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import (

// Package represents a package in the package table.
type Package struct {
ID uint `db:"id"`
ID int `db:"id"`
Name string `db:"name"`
WorkflowID string `db:"workflow_id"`
RunID string `db:"run_id"`
Expand All @@ -34,8 +34,13 @@ type Package struct {

// Goa returns the API representation of the package.
func (p Package) Goa() *goapackage.EnduroStoredPackage {
var id uint
if p.ID > 0 {
id = uint(p.ID) // #nosec G115 -- range validated.
}

col := goapackage.EnduroStoredPackage{
ID: p.ID,
ID: id,
Name: db.FormatOptionalString(p.Name),
WorkflowID: db.FormatOptionalString(p.WorkflowID),
RunID: db.FormatOptionalString(p.RunID),
Expand Down
4 changes: 2 additions & 2 deletions internal/datatypes/preservation_action.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,11 @@ import (

// PreservationAction represents a preservation action in the preservation_action table.
type PreservationAction struct {
ID uint `db:"id"`
ID int `db:"id"`
WorkflowID string `db:"workflow_id"`
Type enums.PreservationActionType `db:"type"`
Status enums.PreservationActionStatus `db:"status"`
StartedAt sql.NullTime `db:"started_at"`
CompletedAt sql.NullTime `db:"completed_at"`
PackageID uint `db:"package_id"`
PackageID int `db:"package_id"`
}
6 changes: 3 additions & 3 deletions internal/datatypes/preservation_task.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,12 @@ import (
// PreservationTask represents a preservation action task in the
// preservation_task table.
type PreservationTask struct {
ID uint `db:"id"`
ID int `db:"id"`
TaskID string `db:"task_id"`
Name string `db:"name"`
Status enums.PreservationTaskStatus `db:"status"`
StartedAt sql.NullTime `db:"started_at"`
CompletedAt sql.NullTime `db:"completed_at"`
Note string
PreservationActionID uint `db:"preservation_action_id"`
Note string `db:"note"`
PreservationActionID int `db:"preservation_action_id"`
}
2 changes: 1 addition & 1 deletion internal/fsutil/fsutil.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ func Move(src, dst string) error {
}

// SetFileModes recursively sets the file mode of root and its contents.
func SetFileModes(root string, dirMode, fileMode int) error {
func SetFileModes(root string, dirMode, fileMode fs.FileMode) error {
return filepath.WalkDir(root,
func(path string, d os.DirEntry, err error) error {
if err != nil {
Expand Down
54 changes: 43 additions & 11 deletions internal/package_/convert.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,28 +10,60 @@ import (
"github.com/artefactual-sdps/enduro/internal/db"
)

func packageToGoaPackageCreatedEvent(p *datatypes.Package) *goapackage.PackageCreatedEvent {
var id uint
if p.ID > 0 {
id = uint(p.ID) // #nosec G115 -- range validated.
}

return &goapackage.PackageCreatedEvent{
ID: id,
Item: p.Goa(),
}
}

// preservationActionToGoa returns the API representation of a preservation task.
func preservationActionToGoa(pt *datatypes.PreservationAction) *goapackage.EnduroPackagePreservationAction {
func preservationActionToGoa(pa *datatypes.PreservationAction) *goapackage.EnduroPackagePreservationAction {
var startedAt string
if pt.StartedAt.Valid {
startedAt = pt.StartedAt.Time.Format(time.RFC3339)
if pa.StartedAt.Valid {
startedAt = pa.StartedAt.Time.Format(time.RFC3339)
}

var id uint
if pa.ID > 0 {
id = uint(pa.ID) // #nosec G115 -- range validated.
}

var packageID uint
if pa.PackageID > 0 {
packageID = uint(pa.PackageID) // #nosec G115 -- range validated.
}

return &goapackage.EnduroPackagePreservationAction{
ID: pt.ID,
WorkflowID: pt.WorkflowID,
Type: pt.Type.String(),
Status: pt.Status.String(),
ID: uint(id),
WorkflowID: pa.WorkflowID,
Type: pa.Type.String(),
Status: pa.Status.String(),
StartedAt: startedAt,
CompletedAt: db.FormatOptionalTime(pt.CompletedAt),
PackageID: &pt.PackageID,
CompletedAt: db.FormatOptionalTime(pa.CompletedAt),
PackageID: ref.New(packageID),
}
}

// preservationTaskToGoa returns the API representation of a preservation task.
func preservationTaskToGoa(pt *datatypes.PreservationTask) *goapackage.EnduroPackagePreservationTask {
var id uint
if pt.ID > 0 {
id = uint(pt.ID) // #nosec G115 -- range validated.
}

var paID uint
if pt.PreservationActionID > 0 {
paID = uint(pt.PreservationActionID) // #nosec G115 -- range validated.
}

return &goapackage.EnduroPackagePreservationTask{
ID: pt.ID,
ID: id,
TaskID: pt.TaskID,
Name: pt.Name,
Status: pt.Status.String(),
Expand All @@ -42,6 +74,6 @@ func preservationTaskToGoa(pt *datatypes.PreservationTask) *goapackage.EnduroPac

CompletedAt: db.FormatOptionalTime(pt.CompletedAt),
Note: &pt.Note,
PreservationActionID: &pt.PreservationActionID,
PreservationActionID: ref.New(paID),
}
}
Loading

0 comments on commit 263818b

Please sign in to comment.