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

Parsing might be too strict #1

Open
bgentry opened this issue Jan 23, 2014 · 6 comments
Open

Parsing might be too strict #1

bgentry opened this issue Jan 23, 2014 · 6 comments
Labels

Comments

@bgentry
Copy link
Owner

bgentry commented Jan 23, 2014

@deafbybeheading had the following messed up .netrc file:

machine me.test.com
  login [email protected]
  password REDACTED
machine me2.test.com
  login [email protected]
  password 
REDACTED api.heroku.com
  login [email protected]
  password REDACTED
machine code.test.com
  login [email protected]
  password REDACTED

Where each use of REDACTED was a password. Somehow geemus/netrc was able to read hid creds from this file, but go-netrc wasn't.

I'm actually not sure that we should handle this. If the .netrc is clearly invalid, you should have to fix it. The netrc manpage certainly makes me think it's invalid.

@bgentry
Copy link
Owner Author

bgentry commented Jan 23, 2014

Maybe @geemus knows something about this.

@geemus
Copy link

geemus commented Jan 23, 2014

Not sure off hand I'm afraid. kr did the implementation work there (I just did a couple tweaks and was told to maintain). It certainly does appear invalid and is surprising that it wouldn't blow up more spectacularly even in the ruby netrc thing.

@bgentry
Copy link
Owner Author

bgentry commented Jan 30, 2014

For some reason @mfine had a similarly messed up .netrc file. I'm thinking this project might need to be just ignore stuff it can't parse (or at least offer that option).

@geemus
Copy link

geemus commented Jan 30, 2014

It is a mixed bag. Ignoring un-parseable stuff is great as long as that isn't the pair you are trying to look up (in which case having it tell you it doesn't exist is confusing in it's own right). Not sure if fail fast is better or fail accurate, so to speak. May depend on how widespread these issues come to be.

@billmoritz
Copy link

I've run in to this issue with Heroku and Terraform. Using the Heroku cli to login to an SSO endpoint creates method and org keywords for the machine api.heroku.com section.

machine api.heroku.com
  login [email protected]
  password xxxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxx
  method sso
  org orgname

Any time I use a go application that uses this library I get Error parsing netrc file at "/Users/wmoritz/.netrc": line 5: keyword expected; got method

@apparentlymart
Copy link

Hello all! I'm a member of the Terraform team, and we just saw a new issue hashicorp/terraform#18610 that seems to be covering the same problem as reported by @billmoritz above.

My thought to address this would be for the parser to attempt graceful recovery when it encounters something it can't parse. Specifically, when encountering an invalid token, it could discard the entry it's currently parsing and then seek forward to the next line that starts with the machine token, and resume parsing from there. That would avoid it misinterpreting machine sections containing directives it can't understand while still allowing the remainder of the file to be processed as expected.

I agree with the above discussion that it's not ideal for an error to just be silently ignored, and I have a possible compromise for that too: although the existing FindMachine method does not return an error (it assumes all error handling happened during ParseFile) -- perhaps a new method alongside it could be defined with the signature func (string) (*Machine, error) and then have this return an error if there was a parse error for that host. That way a calling application can get a good result in the case where the unparseable entry is not what was being requested, but generate an error if it was.

Does that seem like a reasonable path forward?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants