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 path validation to signers & verifiers #69

Open
7 of 8 tasks
alexheretic opened this issue Jun 29, 2022 · 3 comments
Open
7 of 8 tasks

Add path validation to signers & verifiers #69

alexheretic opened this issue Jun 29, 2022 · 3 comments
Labels
enhancement New feature or request

Comments

@alexheretic
Copy link
Contributor

alexheretic commented Jun 29, 2022

Since path should always start with / we can validate that this is the case and return a helpful error before an incorrect signature or incorrect verify usage ends in an opaque error.

E.g. trying to use the whole url .path("https://example.com/foo"). We could produce something like an error invalid path "https://example.com/foo" must start with '/'.

We can use the full url scenario as a new test for signing & verifying in all langs.

Checklist:

@alexheretic alexheretic added the enhancement New feature or request label Jun 29, 2022
@LukeMathWalker
Copy link

LukeMathWalker commented Jun 29, 2022

Perhaps invalid path, you passed an absolute URL. The path must be relative, starting with '/'?
It might even be possible to suggest the correct one by playing around with HTTP URL parsing (e.g. you probably wanted to pass '/foo'?).

@alexheretic
Copy link
Contributor Author

Perhaps invalid path, you passed an absolute URL. The path must be relative, starting with '/'? It might even be possible to suggest the correct one by playing around with HTTP URL parsing (e.g. you probably wanted to pass '/foo'?).

That would be very nice, but the biggest win is just having any error saying "path" instead of just generating an invalid, or failing verify a, signature which indicates very little. So the simplest impl is just ensuring the path starts with /.

@alexheretic
Copy link
Contributor Author

Also related to the proposed reason deduction, but this issue is much easier to achieve https://truelayer.slack.com/archives/C01UYLYQ2HE/p1655124955742179

^^ that would actually suggest the correct path in the signing case. I should move that proposal to an issue here...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

No branches or pull requests

2 participants