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

Fixing the $' which causes malformed headers on the request. I belie… #11

Closed
wants to merge 5 commits into from

Conversation

johngluckmdsol
Copy link

I believe this is what was intended. This will throw errors otherwise

@jfeltesse-mdsol
Copy link
Contributor

Please don't commit .DS_Store and other IDE-specific files. A global gitignore might help.

@jfeltesse-mdsol
Copy link
Contributor

So I'm wondering a few things:

  1. what kind of errors we're talking about
  2. how come up until now such errors have not occurred
  3. what value this $' global variable can have a this time in the code

Other unrelated comments:

  • needs a changelog update and version bump
  • needs specs

@cabbott cabbott closed this Aug 31, 2018
@cabbott cabbott reopened this Aug 31, 2018
@cabbott
Copy link
Contributor

cabbott commented Aug 31, 2018

Sorry, meant to comment but clicked close instead. Please invalidate that mauth key (hoping it's just the randomly generated one that isn't registered)

@jfeltesse-mdsol
Copy link
Contributor

On the same topic, those mauth config files shouldn't be committed either.
Which leaves this PR with only that single line change in the proxy file.

@johngluckmdsol
Copy link
Author

johngluckmdsol commented Aug 31, 2018

@jfeltesse-mdsol
what kind of errors we're talking about?
The responding application returns a 404 - Bad URI

how come up until now such errors have not occurred
I don't think anyone uses this software. Do you have telemetry that says otherwise?

what value this $' global variable can have a this time in the code
Not sure what you mean. It was blank when I encountered it

needs a changelog update and version bump
I'm happy to do this

needs specs
It wasn't clear immediately how to do this. It's an integration test because your request headers are malformed and there's not way to do this. If you want me to set up a proper mock for you, I can do it but I don't have a lot of spare time these days so it's going to take a while. I'm happy to consult.

@jfeltesse-mdsol
Copy link
Contributor

404 - Bad URI

We'll need much more info and context to be able to confirm the bug lies in mauth-client-ruby, can you follow up by email with all the details (which are bound to contain non public info -- hence not here on the public space).

I don't think anyone uses this software. Do you have telemetry that says otherwise?

Please check pikapika. I'll let you appreciate that every single ruby platform service uses mauth.

Not sure what you mean. It was blank when I encountered it

What I mean is this $' global variable stands for "The string following the match in the last successful pattern match. This variable is local to the current scope. Read only. Thread local." so it can take different values. I'm wondering what is the typical value you'd encounter in a normal scenario. Could be the services you're trying to make interact are not supplying the mauth data correctly, which makes the match fail and "corrupts" the headers there. If this theory is true then this gem is not responsible although it should handle such cases more gracefully.

@jfeltesse-mdsol
Copy link
Contributor

The DS_Store file is still here btw. Please add this to your personal global gitignore file instead of this gem's gitignore file since it's macOS specific and it's not this gem's gitignore file responsibility to cater to the various OS metadata files out there.

@jfeltesse-mdsol
Copy link
Contributor

About the "needs specs" part, sorry I might have been unclear, if that change is indeed warranted it should have either more or at least modified specs. The fact that the code has changed but the specs pass mean that this area is not covered properly.

@johngluckmdsol
Copy link
Author

Nevermind. Closing

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

Successfully merging this pull request may close these issues.

3 participants