Skip to content
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

Add 'gas' for security problems scanning #830

Merged
merged 2 commits into from
May 18, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ GOTEST=go test -v $(RACE)
GOLINT=golint
GOVET=go vet
GOFMT=gofmt
GAS=gas -exclude=G104
FMT_LOG=fmt.log
LINT_LOG=lint.log
IMPORT_LOG=import.log
Expand Down Expand Up @@ -104,8 +105,12 @@ fmt:
$(GOFMT) -e -s -l -w $(ALL_SRC)
./scripts/updateLicenses.sh

.PHONY: gas
gas: install-gas
$(GAS) $(TOP_PKGS)

.PHONY: lint
lint:
lint: gas
$(GOVET) $(TOP_PKGS)
@cat /dev/null > $(LINT_LOG)
@$(foreach pkg, $(TOP_PKGS), $(GOLINT) $(pkg) | grep -v -e pkg/es/wrapper.go -e /mocks/ -e thrift-gen -e thrift-0.9.2 >> $(LINT_LOG) || true;)
Expand All @@ -128,6 +133,10 @@ install-go-bindata:
go get github.com/jteeuwen/go-bindata/...
go get github.com/elazarl/go-bindata-assetfs/...

.PHONY: install-gas
install-gas:
go get github.com/GoASTScanner/gas/cmd/gas/...
Copy link
Member

Choose a reason for hiding this comment

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

I am still not clear why we need the extra makefile targets instead of rolling it up into the main install and lint (extra targets increase noise / entropy)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

install is quite clean, doing only a glide install. It does delegate the installation of the dependent tool (glide) to a separate target, so, there's a precedent :-)

About gas getting called directly from lint, I guess it's a matter of taste. When working on fixing the warnings provided by gas, I find it easier to run it in isolation as make gas.


.PHONY: build-examples
build-examples: install-go-bindata
(cd ./examples/hotrod/services/frontend/ && go-bindata-assetfs -pkg frontend web_assets/...)
Expand Down
2 changes: 1 addition & 1 deletion cmd/query/app/static_handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ func loadUIConfig(uiConfig string) (map[string]interface{}, error) {
return nil, nil
}
ext := filepath.Ext(uiConfig)
bytes, err := ioutil.ReadFile(uiConfig)
bytes, err := ioutil.ReadFile(uiConfig) /* nolint #nosec , this comes from an admin, not user */
if err != nil {
return nil, errors.Wrapf(err, "Cannot read UI config file %v", uiConfig)
}
Expand Down
2 changes: 1 addition & 1 deletion cmd/query/app/static_handler_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ func TestNewStaticAssetsHandlerErrors(t *testing.T) {

for _, base := range []string{"x", "x/", "/x/"} {
_, err := NewStaticAssetsHandler("fixture", StaticAssetsHandlerOptions{UIConfigPath: "fixture/ui-config.json", BasePath: base})
require.Error(t, err, "basePath=%s", base)
require.Errorf(t, err, "basePath=%s", base)
assert.Contains(t, err.Error(), "Invalid base path")
}
}
Expand Down
20 changes: 0 additions & 20 deletions model/json/model.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,6 @@

package json

import (
"encoding/json"
"io/ioutil"
)

// ReferenceType is the reference type of one span to another
type ReferenceType string

Expand Down Expand Up @@ -113,18 +108,3 @@ type DependencyLink struct {
Child string `json:"child"`
CallCount uint64 `json:"callCount"`
}

// FromFile reads a Trace from a JSON file.
// Mostly this exists to have some code aside from struct declaration,
// as otherwise code coverate is reported as 0%.
func FromFile(filename string) (*Trace, error) {
in, err := ioutil.ReadFile(filename)
if err != nil {
return nil, err
}
var trace Trace
if err := json.Unmarshal(in, &trace); err != nil {
return nil, err
}
return &trace, nil
}
66 changes: 0 additions & 66 deletions model/json/model_test.go

This file was deleted.

2 changes: 1 addition & 1 deletion pkg/cassandra/gocql/testutils/udt.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ func (testCase UDTTestCase) Run(t *testing.T) {
proto: 0x03,
typ: field.Type,
}
typeInfo := *(*gocql.NativeType)(unsafe.Pointer(&nt))
typeInfo := *(*gocql.NativeType)(unsafe.Pointer(&nt)) /* nolint #nosec */
data, err := testCase.Obj.MarshalUDT(field.Name, typeInfo)
if field.Err {
assert.Error(t, err)
Expand Down
2 changes: 1 addition & 1 deletion plugin/sampling/strategystore/static/strategy_store.go
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ func loadStrategies(strategiesFile string) (*strategies, error) {
if strategiesFile == "" {
return nil, nil
}
bytes, err := ioutil.ReadFile(strategiesFile)
bytes, err := ioutil.ReadFile(strategiesFile) /* nolint #nosec , this comes from an admin, not user */
Copy link
Member

Choose a reason for hiding this comment

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

why does this require linter directives?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The theory behind it is that strategiesFile can be manipulated by an end user, so that a file like /etc/shadow could be read. I tracked all instances of this case (G304, IIRC) and they all come from the CLI, meaning that they are set by an admin, and not by an end user.

if err != nil {
return nil, errors.Wrap(err, "Failed to open strategies file")
}
Expand Down