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

fix(storage): use regex in v4SanitizeHeaders #6970

Closed
wants to merge 4 commits into from

Conversation

rotemlinik
Copy link

@rotemlinik rotemlinik commented Oct 31, 2022

The v4SanitizeHeaders() logic used split(":") func to separate the headers key:val, but in cases where the value itself contains : (such as dates in RFC3339 format as required), the value is partly lost.
I replaced the logic with a regex logic- same as the one used in v2SanitizeHeaders() and added a unit test for this case.

@rotemlinik rotemlinik requested review from a team as code owners October 31, 2022 08:12
@conventional-commit-lint-gcf
Copy link

conventional-commit-lint-gcf bot commented Oct 31, 2022

🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use automerge label. Good luck human!

-- conventional-commit-lint bot
https://conventionalcommits.org/

@product-auto-label product-auto-label bot added the size: s Pull request size is small. label Oct 31, 2022
@google-cla
Copy link

google-cla bot commented Oct 31, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@rotemlinik rotemlinik changed the title [] Bug fix for date value headers Oct 31, 2022
@rotemlinik rotemlinik changed the title Bug fix for date value headers fix: for date value headers Oct 31, 2022
@rotemlinik rotemlinik changed the title fix: for date value headers fix: use regex in v4SanitizeHeaders Oct 31, 2022
@codyoss codyoss changed the title fix: use regex in v4SanitizeHeaders fix(storage): use regex in v4SanitizeHeaders Nov 1, 2022
@product-auto-label product-auto-label bot added the api: storage Issues related to the Cloud Storage API. label Nov 2, 2022
@@ -535,13 +535,12 @@ func v4SanitizeHeaders(hdrs []string) []string {
sanitizedHeader := strings.TrimSpace(hdr)

var key, value string
headerMatches := strings.Split(sanitizedHeader, ":")
if len(headerMatches) < 2 {
headerMatches := canonicalHeaderRegexp.FindStringSubmatch(sanitizedHeader)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think not using the canonicalHeaderRegexp is actually an intentional difference between v2 and v4; see comments in L526-527.

However, maybe we should use strings.SplitN instead so that only the first : is split on. Does that sound like a good solution to you?

@product-auto-label product-auto-label bot added the stale: old Pull request is old and needs attention. label Nov 30, 2022
@product-auto-label product-auto-label bot added stale: extraold Pull request is critically old and needs prioritization. and removed stale: old Pull request is old and needs attention. labels Dec 30, 2022
@BrennaEpp
Copy link
Contributor

Closing this as #7603 contains the intended changes.

As of release v1.30.1, storage allows the use of values containing : in v4 headers

@BrennaEpp BrennaEpp closed this Apr 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: storage Issues related to the Cloud Storage API. size: s Pull request size is small. stale: extraold Pull request is critically old and needs prioritization.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants