Skip to content
This repository has been archived by the owner on Feb 14, 2023. It is now read-only.

postgresstore: remove postgres URL from help output #511

Merged
merged 2 commits into from Feb 21, 2019

Conversation

t-bast
Copy link
Contributor

@t-bast t-bast commented Feb 21, 2019

It could leak the username/password.


This change is Reviewable

It could leak the username/password.
@t-bast t-bast requested a review from conord33 February 21, 2019 10:41
@ghost ghost assigned t-bast Feb 21, 2019
@ghost ghost added the review label Feb 21, 2019
@codecov
Copy link

codecov bot commented Feb 21, 2019

Codecov Report

Merging #511 into master will decrease coverage by 0.04%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #511      +/-   ##
==========================================
- Coverage   71.49%   71.44%   -0.05%     
==========================================
  Files         100      100              
  Lines        5563     5564       +1     
==========================================
- Hits         3977     3975       -2     
- Misses       1221     1223       +2     
- Partials      365      366       +1
Impacted Files Coverage Δ
postgresstore/cmd.go 0% <0%> (ø) ⬆️
validation/filewrapper.go 84.9% <0%> (-3.78%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2f94450...4d31113. Read the comment docs.

Copy link
Contributor

@conord33 conord33 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @conord33 and @t-bast)


postgresstore/cmd.go, line 138 at r1 (raw file):

// a postgres adapter using flag values.
func InitializeWithFlags(version, commit string) *Store {
	dbURL := util.OrStrings(os.Getenv("POSTGRESSTORE_URL"), DefaultURL)

cant you just use os.Getenv("POSTGRESSTORE_URL") if it is defined otherwise use url. seems weird to have a flag you can't use.

Copy link
Contributor Author

@t-bast t-bast left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @conord33 and @t-bast)


postgresstore/cmd.go, line 138 at r1 (raw file):

Previously, conord33 (Conor Dawson) wrote…

cant you just use os.Getenv("POSTGRESSTORE_URL") if it is defined otherwise use url. seems weird to have a flag you can't use.

If you prefer :)

Copy link
Contributor

@conord33 conord33 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @t-bast)


postgresstore/cmd.go, line 138 at r1 (raw file):

Previously, t-bast (Bastien Teinturier) wrote…

If you prefer :)

👍

Copy link
Contributor

@conord33 conord33 left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @t-bast)

Copy link
Contributor

@conord33 conord33 left a comment

Choose a reason for hiding this comment

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

im still confused why the help message even got logged in the first place...

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @t-bast)

Copy link
Contributor Author

@t-bast t-bast left a comment

Choose a reason for hiding this comment

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

Agreed, unless the pentesters found more than they told us and were able to get inside our AWS cluster :)

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @t-bast)

@t-bast t-bast merged commit 7147d64 into master Feb 21, 2019
@t-bast t-bast deleted the bt/postgresstore/remove-logged-postgres-url branch February 21, 2019 11:02
@ghost ghost removed the review label Feb 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants