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

Allow non-trailing slash in escrow URL #124

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

arubdesu
Copy link
Contributor

Add if with return for case, tests that cover new behavior, assurance in test that only this path will lead to this behavior

We can definitely discuss how I separated out a new MockPref (I was following a colleagues guidance), it would make sense for the whole test struct to ride alongside the implementation in the other... package? Maybe? Anyway pardon I hacked it in, I can't even get a test build spit out at the moment because no homebrew/I've forgotten how to luggage 😅

Add if with return for case, tests that cover new behavior, assurance in test that only this path will lead to this behavior
@arubdesu
Copy link
Contributor Author

Oh, and as a reminder this is because I used an HTTP api gateway to 'forward' escrow's into the network our CryptServer is on, which doesn't allow the trailing slash and 'default' is the AWS... default naming of the stage we deployed.

@grahamgilbert
Copy link
Owner

I would prefer to not hard code specific implementation details for your environment. Is there any reason you cannot set your server url to what you need?

@arubdesu
Copy link
Contributor Author

It's certainly not the communities problem that our infrastructure implementation is weird, but linux and windows 'agent' interfaces are using this same URL so migrating has more friction/requires more coordination for something as vital as FDE escrow. I'm honestly not sure if the other API gateway options (lambda-less and REST-ful) would even fix the incompatibility with trailing slash.
Currently (and historically, which is why we had been maintaining a fork,) the checkin script breaks our usage because it defaults to appending/ensuring the trailing slash url format - even if it may be considered more 'correct' (and admittedly is definitely a single standard that was always supported) it's not impossible another environment will ever want to support something like this.

A flag to not validate/enforce trailing slash would work for me just the same, should I add a pref or something instead?

@grahamgilbert
Copy link
Owner

I think the pref to not enforce the trailing slash makes sense

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.

2 participants