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.

Sign up for GitHub

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

Jump to bottom

Deduplicate slashes for sigv4 signature #364

Merged
merged 2 commits into from Mar 29, 2022
Merged

Deduplicate slashes for sigv4 signature #364

merged 2 commits into from Mar 29, 2022

Conversation

importhuman
Copy link
Contributor

Signed-off-by: Ujjwal Goyal importujjwal@gmail.com

For issue 10200 on the prometheus repo.
Not sure if this is all that is required.

@roidelapluie
Copy link
Member

Can we have a test?

roidelapluie
roidelapluie reviewed Mar 18, 2022
View reviewed changes
sigv4/sigv4.go Outdated Show resolved Hide resolved
@importhuman
Copy link
Contributor Author

Did you mean a test like this?

It's failing currently, because it's not being escaped. req.URL.EscapedPath() uses the RawPath of the URL, which is coming out to be "", in which case it doesn't escape. Any suggestions?

SuperQ
SuperQ approved these changes Mar 28, 2022
View reviewed changes
Copy link
Member

@SuperQ SuperQ left a comment

Choose a reason for hiding this comment

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

LGTM

@importhuman
Copy link
Contributor Author

Were the tests not run in CI, because I expected the test I added to fail 🤔
Don't think this is ready to merge.

@SuperQ
Copy link
Member

SuperQ commented Mar 28, 2022

Yes, we run a make test on this package in CI.

@SuperQ
Copy link
Member

SuperQ commented Mar 28, 2022

Looks like the make test incorrectly skips go test.

Fixed in #365.

@importhuman
Copy link
Contributor Author

Ah, it's inside circleci, didn't realize that.

Screenshot from 2022-03-28 17-24-51
This is what I'm concerned about. Why is it passing on the CI?

@SuperQ
Copy link
Member

SuperQ commented Mar 28, 2022

Ok, a rebase here will make the tests run correctly.

importhuman and others added 2 commits March 29, 2022 01:27
@importhuman @roidelapluie
Deduplicate slashes for sigv4 signature
6008c06 
Signed-off-by: Ujjwal Goyal <importujjwal@gmail.com>
@roidelapluie
Use path.Clean to clean sigv4 path.
f1cb4b8 
sigv4 package already calls EscapeURL, the proper fix would be to use
path.Clean.

Signed-off-by: Julien Pivotto <roidelapluie@inuits.eu>
@roidelapluie
Copy link
Member

It turns out that the underlying API is already calling EscapePath. The fix for the user's issue is to use path.Clean. I have updated the pull request to reflect this and added a test.

WDYT @importhuman ?

@importhuman
Copy link
Contributor Author

LGTM @roidelapluie

@roidelapluie roidelapluie merged commit 840c039 into prometheus:main Mar 29, 2022
@roidelapluie
Copy link
Member

Thanks!!!

@importhuman importhuman deleted the sigv4-deduplicate-slash branch March 30, 2022 10:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kakkoyun kakkoyun kakkoyun approved these changes

@SuperQ SuperQ SuperQ approved these changes

@roidelapluie roidelapluie Awaiting requested review from roidelapluie

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@importhuman @roidelapluie @SuperQ @kakkoyun