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

HTTP User-Agent string-building no longer supports comments #2198

Closed
gdavison opened this issue Jul 20, 2023 · 6 comments
Closed

HTTP User-Agent string-building no longer supports comments #2198

gdavison opened this issue Jul 20, 2023 · 6 comments
Assignees
Labels
bug This issue is a bug. p2 This is a standard priority issue workaround-available

Comments

@gdavison
Copy link
Contributor

Describe the bug

In the HTTP spec, User-Agent strings support a combination of products and comments. A comment is text contained within parentheses.

Prior to aws-sdk-go-v2 v1.19.0, comments could be added using

middleware.AddSDKAgentKey("(" + comment + ")")

Expected Behavior

I expected

middleware.AddSDKAgentKey("(" + comment + ")")

to continue working.

For example, a comment with the text this is a comment should be included in the User-Agent string as

(this is a comment)

Current Behavior

Several characters in the comment are replaced by -. For example, a comment with the text this is a comment is included in the User-Agent string as

-this-is-a-comment-

instead of

(this is a comment)

Reproduction Steps

Generate an HTTP User-Agent string after setting

comment := "this is a comment"
middleware.AddSDKAgentKey("(" + comment + ")")

Possible Solution

One of two solutions:

  1. Directly support HTTP User-Agent comment with a function like middleware.AddUserAgentComment()

  2. Update the escaping rules to not escape keys that match the comment pattern defined in the HTTP spec

Additional Information/Context

No response

AWS Go SDK V2 Module Versions Used

aws-sdk-go-v2 v1.19.0

Compiler and Version used

go version go1.20.3 darwin/arm64

Operating System and version

macOS 12.6.7

@gdavison gdavison added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Jul 20, 2023
@lucix-aws lucix-aws removed the needs-triage This issue or PR still needs to be triaged. label Jul 20, 2023
@lucix-aws
Copy link
Contributor

lucix-aws commented Jul 20, 2023

This is definitely a side effect of a recent User-Agent change.

Note that for the time being you can reclaim this behavior through a build-phase middleware:

import (
	"context"
	"fmt"

	"github.com/aws/smithy-go/middleware"
	smithyhttp "github.com/aws/smithy-go/transport/http"
)

type userAgentAppender struct {
	v string
}

func (*userAgentAppender) ID() string {
	return "userAgentAppender"
}

func (m *userAgentAppender) HandleBuild(ctx context.Context, in middleware.BuildInput, next middleware.BuildHandler) (
	out middleware.BuildOutput, metadata middleware.Metadata, err error,
) {
	r, ok := in.Request.(*smithyhttp.Request)
	if !ok {
		return out, metadata, fmt.Errorf("unknown transport type %T", in.Request)
	}

	if ua := r.Header.Get("user-agent"); ua != "" {
		r.Header.Set("user-agent", fmt.Sprintf("%s %s", ua, m.v))
	}

	return next.HandleBuild(ctx, in)
}

func withUserAgentAppender(v string) func(*middleware.Stack) error {
	return func(stack *middleware.Stack) error {
		return stack.Build.Add(&userAgentAppender{v: v}, middleware.After)
	}
}

EDIT: added missing imports

@gdavison
Copy link
Contributor Author

Thanks, the workaround works

@lucix-aws
Copy link
Contributor

Ive created #2202 to re-add this through the new middleware.WithHeaderComment.

The AddUserAgentKey (and friends) APIs were created with the express purpose of allowing a user to add metadata to the User-Agent header in a manner befitting how it's handled throughout AWS (including encoding specifications, which as you can see have since tightened). The ability to inject comments through the AddUserAgentKey was an unintended side-effect of its existence.

For that reason I believe a new, separate API (which can also apply to any header generically) was the most sensible way forward here.

@RanVaknin RanVaknin added the p2 This is a standard priority issue label Sep 26, 2023
@lucix-aws
Copy link
Contributor

Hi @gdavison -- apologies for dropping the ball on this. I've just now re-spawned the PR in smithy-go, we decided to expose it there instead back in July and it never happened.

@lucix-aws lucix-aws self-assigned this Oct 6, 2023
@lucix-aws
Copy link
Contributor

Fixed by aws/smithy-go#461. This will be available in the next release.

@github-actions
Copy link

github-actions bot commented Oct 6, 2023

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug. p2 This is a standard priority issue workaround-available
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants