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

fix: uri root path checking #5778

Merged
merged 6 commits into from
May 14, 2024
Merged

fix: uri root path checking #5778

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7bb98e8
Fix uri path root path checking
Folyd Apr 14, 2024
86d3747
Merge branch 'mainline' into fix-path-prefix
huanjani Apr 16, 2024
5648841
Fix unit testings
Folyd Apr 20, 2024
b9bc918
Merge branch 'mainline' into fix-path-prefix
Folyd Apr 29, 2024
f100716
Fix multiple slash case
Folyd Apr 30, 2024
a650f79
Merge branch 'mainline' into fix-path-prefix
mergify[bot] May 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion internal/pkg/describe/uri.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ package describe

import (
"fmt"
"regexp"
"strings"

"github.com/aws/copilot-cli/internal/pkg/term/color"
Expand Down Expand Up @@ -403,14 +404,17 @@ func (u *LBWebServiceURI) String() string {

func (u *accessURI) strings() []string {
var uris []string
re := regexp.MustCompile("/+")
for _, dnsName := range u.DNSNames {
protocol := "http://"
if u.HTTPS {
protocol = "https://"
}
path := ""
if u.Path != "/" {
Copy link
Contributor

@dannyrandall dannyrandall Apr 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

i think this misses a case with a path with multiple slashes, like ///foo. that should be solved by just changing the else if to an else (or drop the else like this)!

Suggested change
if u.Path != "/" {
path := u.Path
if !strings.HasPrefix(u.Path, "/") {
path = fmt.Sprintf("/%s", u.Path)
}

if !strings.HasPrefix(u.Path, "/") {
path = fmt.Sprintf("/%s", u.Path)
} else if u.Path != "/" {
path = re.ReplaceAllString(u.Path, "/")
}
uris = append(uris, color.HighlightResource(protocol+dnsName+path))
Copy link
Contributor

Choose a reason for hiding this comment

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

If the user adds a path without the / prefix, the path variable here will still be "". I think we need to catch this case. Thanks!

}
Expand Down
42 changes: 41 additions & 1 deletion internal/pkg/describe/uri_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,10 @@ package describe
import (
"errors"
"fmt"
"github.com/aws/copilot-cli/internal/pkg/aws/ecs"
"testing"

"github.com/aws/copilot-cli/internal/pkg/aws/ecs"

"github.com/aws/copilot-cli/internal/pkg/deploy/cloudformation/stack"
"github.com/aws/copilot-cli/internal/pkg/describe/mocks"
"github.com/aws/copilot-cli/internal/pkg/template"
Expand Down Expand Up @@ -680,6 +681,24 @@ func TestLBWebServiceURI_String(t *testing.T) {

wanted: "http://jobs.test.phonetool.com",
},
"http with /v2 path": {
accessDNSNames: []string{"jobs.test.phonetool.com"},
accessPath: "/v2",

wanted: "http://jobs.test.phonetool.com/v2",
},
"http with multiple slash path": {
accessDNSNames: []string{"jobs.test.phonetool.com"},
accessPath: "//v2",

wanted: "http://jobs.test.phonetool.com/v2",
},
"http with non-root path": {
accessDNSNames: []string{"jobs.test.phonetool.com"},
accessPath: "v2",

wanted: "http://jobs.test.phonetool.com/v2",
},
"cloudfront": {
accessDNSNames: []string{"abc.cloudfront.net"},
accessPath: "svc",
Expand Down Expand Up @@ -707,6 +726,27 @@ func TestLBWebServiceURI_String(t *testing.T) {

wanted: "https://jobs.test.phonetool.com",
},
"https with /v2 path": {
accessDNSNames: []string{"jobs.test.phonetool.com"},
accessPath: "/v2",
accessHTTPS: true,

wanted: "https://jobs.test.phonetool.com/v2",
},
"https with multiple slash path": {
accessDNSNames: []string{"jobs.test.phonetool.com"},
accessPath: "//v2",
accessHTTPS: true,

wanted: "https://jobs.test.phonetool.com/v2",
},
"https with non-root path": {
accessDNSNames: []string{"jobs.test.phonetool.com"},
accessPath: "v2",
accessHTTPS: true,

wanted: "https://jobs.test.phonetool.com/v2",
},
}

for name, tc := range testCases {
Expand Down
Loading