Skip to content

Commit

Permalink
ensure cfg.SiteDomain ends any URL we redirect to
Browse files Browse the repository at this point in the history
  • Loading branch information
@gedge
gedge committed Jul 23, 2024
1 parent 7a3ccda commit b373e11
Show file tree
Hide file tree
Showing 4 changed files with 100 additions and 61 deletions.
29 changes: 24 additions & 5 deletions config/config.go
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package config

import (
"net/url"
"strings"
"time"

"github.com/kelseyhightower/envconfig"
Expand Down Expand Up @@ -40,6 +42,10 @@ var cfg *Config
// Get returns the default config with any modifications through environment
// variables
func Get() (*Config, error) {
if cfg != nil {
return cfg, nil
}

envCfg, err := get()
if err != nil {
return nil, err
Expand All @@ -50,14 +56,12 @@ func Get() (*Config, error) {
} else {
envCfg.PatternLibraryAssetsPath = "//cdn.ons.gov.uk/dp-design-system/e0a75c3"
}
return envCfg, nil

cfg = envCfg
return cfg, nil
}

func get() (*Config, error) {
if cfg != nil {
return cfg, nil
}

cfg := &Config{
APIRouterURL: "http://localhost:23200/v1",
BindAddr: "localhost:25200",
Expand Down Expand Up @@ -86,3 +90,18 @@ func get() (*Config, error) {

return cfg, envconfig.Process("", cfg)
}

// IsSiteDomainURL is true when urlString is a URL and its host ends with `.`+siteDomain (when siteDomain is blank, uses cfg.SiteDomain)
func IsSiteDomainURL(urlString, siteDomain string) bool {
if urlString == "" {
return false
}
if siteDomain == "" {
siteDomain = cfg.SiteDomain
}
urlObject, err := url.ParseRequestURI(urlString)
if err != nil || !strings.HasSuffix(urlObject.Host, "."+siteDomain) {
return false
}
return true
}
72 changes: 29 additions & 43 deletions handlers/feedback.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import (
"context"
"fmt"
"net/http"
"net/url"
"regexp"
"strings"

Expand All @@ -24,25 +23,16 @@ import (
// FeedbackThanks loads the Feedback Thank you page
func (f *Feedback) FeedbackThanks() http.HandlerFunc {
return dphandlers.ControllerHandler(func(w http.ResponseWriter, req *http.Request, lang, collectionID, accessToken string) {
feedbackThanks(w, req, req.Referer(), f.Render, f.CacheService, lang)
feedbackThanks(w, req, req.Referer(), f.Render, f.CacheService, lang, f.Config.SiteDomain, f.Config.EnableNewNavBar)
})
}

func feedbackThanks(w http.ResponseWriter, req *http.Request, uri string, rend interfaces.Renderer, cacheHelperService *cacheHelper.Helper, lang string) {
ctx := req.Context()
var wholeSite string

cfg, err := config.Get()
if err != nil {
log.Warn(ctx, "Unable to retrieve configuration", log.FormatErrors([]error{err}))
} else {
wholeSite = cfg.SiteDomain
}

func feedbackThanks(w http.ResponseWriter, req *http.Request, uri string, rend interfaces.Renderer, cacheHelperService *cacheHelper.Helper, lang, siteDomain string, enableNewNavBar bool) {
basePage := rend.NewBasePageModel()
p := mapper.CreateGetFeedbackThanks(req, basePage, lang, uri, wholeSite)
p := mapper.CreateGetFeedbackThanks(req, basePage, lang, uri, siteDomain)

if cfg.EnableNewNavBar {
if enableNewNavBar {
ctx := req.Context()
mappedNavContent, err := cacheHelperService.GetMappedNavigationContent(ctx, lang)
if err == nil {
p.NavigationContent = mappedNavContent
Expand All @@ -55,20 +45,16 @@ func feedbackThanks(w http.ResponseWriter, req *http.Request, uri string, rend i
// GetFeedback handles the loading of a feedback page
func (f *Feedback) GetFeedback() http.HandlerFunc {
return dphandlers.ControllerHandler(func(w http.ResponseWriter, req *http.Request, lang, collectionID, accessToken string) {
getFeedback(w, req, []core.ErrorItem{}, model.FeedbackForm{URL: req.Referer()}, lang, f.Render, f.CacheService)
getFeedback(w, req, []core.ErrorItem{}, model.FeedbackForm{URL: req.Referer()}, lang, f.Render, f.CacheService, f.Config.EnableNewNavBar)
})
}

func getFeedback(w http.ResponseWriter, req *http.Request, validationErrors []core.ErrorItem, ff model.FeedbackForm, lang string, rend interfaces.Renderer, cacheHelperService *cacheHelper.Helper) {
func getFeedback(w http.ResponseWriter, req *http.Request, validationErrors []core.ErrorItem, ff model.FeedbackForm, lang string, rend interfaces.Renderer, cacheHelperService *cacheHelper.Helper, enableNewNavBar bool) {
basePage := rend.NewBasePageModel()
p := mapper.CreateGetFeedback(req, basePage, validationErrors, ff, lang)

ctx := context.Background()
cfg, err := config.Get()
if err != nil {
log.Warn(ctx, "Unable to retrieve configuration", log.FormatErrors([]error{err}))
}
if cfg.EnableNewNavBar {
if enableNewNavBar {
ctx := context.Background()
mappedNavContent, err := cacheHelperService.GetMappedNavigationContent(ctx, lang)
if err == nil {
p.NavigationContent = mappedNavContent
Expand All @@ -81,11 +67,11 @@ func getFeedback(w http.ResponseWriter, req *http.Request, validationErrors []co
// AddFeedback handles a users feedback request
func (f *Feedback) AddFeedback() http.HandlerFunc {
return dphandlers.ControllerHandler(func(w http.ResponseWriter, req *http.Request, lang, collectionID, accessToken string) {
addFeedback(w, req, f.Render, f.EmailSender, f.Config.FeedbackFrom, f.Config.FeedbackTo, lang, f.CacheService)
addFeedback(w, req, f.Render, f.EmailSender, f.Config.FeedbackFrom, f.Config.FeedbackTo, lang, f.Config.SiteDomain, f.CacheService)
})
}

func addFeedback(w http.ResponseWriter, req *http.Request, rend interfaces.Renderer, emailSender email.Sender, from, to, lang string, cacheService *cacheHelper.Helper) {
func addFeedback(w http.ResponseWriter, req *http.Request, rend interfaces.Renderer, emailSender email.Sender, from, to, lang, siteDomain string, cacheService *cacheHelper.Helper) {
ctx := req.Context()
if err := req.ParseForm(); err != nil {
log.Error(ctx, "unable to parse request form", err)
Expand All @@ -103,9 +89,9 @@ func addFeedback(w http.ResponseWriter, req *http.Request, rend interfaces.Rende
return
}

validationErrors := validateForm(&ff)
validationErrors := validateForm(&ff, siteDomain)
if len(validationErrors) > 0 {
getFeedback(w, req, validationErrors, ff, lang, rend, cacheService)
getFeedback(w, req, validationErrors, ff, lang, rend, cacheService, false)
return
}

Expand Down Expand Up @@ -134,7 +120,7 @@ func addFeedback(w http.ResponseWriter, req *http.Request, rend interfaces.Rende
}

// validateForm is a helper function that validates a slice of FeedbackForm to determine if there are form validation errors
func validateForm(ff *model.FeedbackForm) (validationErrors []core.ErrorItem) {
func validateForm(ff *model.FeedbackForm, siteDomain string) (validationErrors []core.ErrorItem) {
if ff.Type == "" && ff.FormLocation != "footer" {
validationErrors = append(validationErrors, core.ErrorItem{
Description: core.Localisation{
Expand All @@ -147,28 +133,28 @@ func validateForm(ff *model.FeedbackForm) (validationErrors []core.ErrorItem) {
}

ff.URL = strings.TrimSpace(ff.URL)
if ff.Type == "A specific page" && ff.URL == "" {
validationErrors = append(validationErrors, core.ErrorItem{
Description: core.Localisation{
LocaleKey: "FeedbackWhatEnterURL",
Plural: 1,
},
URL: "#type-error",
})
ff.IsURLErr = true
}

if ff.Type == "A specific page" && ff.URL != "" {
_, err := url.ParseRequestURI(ff.URL)
if err != nil {
if ff.Type == "A specific page" {
if ff.URL == "" {
validationErrors = append(validationErrors, core.ErrorItem{
Description: core.Localisation{
LocaleKey: "FeedbackValidURL",
LocaleKey: "FeedbackWhatEnterURL",
Plural: 1,
},
URL: "#type-error",
})
ff.IsURLErr = true

} else {
if !config.IsSiteDomainURL(ff.URL, siteDomain) {
validationErrors = append(validationErrors, core.ErrorItem{
Description: core.Localisation{
LocaleKey: "FeedbackValidURL",
Plural: 1,
},
URL: "#type-error",
})
ff.IsURLErr = true
}
}
}

Expand Down
51 changes: 41 additions & 10 deletions handlers/feedback_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (

cacheClient "github.com/ONSdigital/dp-frontend-cache-helper/pkg/navigation/client"
cacheHelper "github.com/ONSdigital/dp-frontend-cache-helper/pkg/navigation/helper"
"github.com/ONSdigital/dp-frontend-feedback-controller/config"
"github.com/ONSdigital/dp-frontend-feedback-controller/email/emailtest"
"github.com/ONSdigital/dp-frontend-feedback-controller/interfaces/interfacestest"
"github.com/ONSdigital/dp-frontend-feedback-controller/mocks"
Expand All @@ -24,6 +25,8 @@ import (
. "github.com/smartystreets/goconvey/convey"
)

const siteDomain = "ons.gov.uk"

func Test_getFeedback(t *testing.T) {
helper.InitialiseLocalisationsHelper(mocks.MockAssetFunction)
Convey("Given a valid request", t, func() {
Expand Down Expand Up @@ -53,7 +56,7 @@ func Test_getFeedback(t *testing.T) {
},
}}
Convey("When getFeedback is called", func() {
getFeedback(w, req, []coreModel.ErrorItem{}, ff, lang, mockRenderer, mockNagivationCache)
getFeedback(w, req, []coreModel.ErrorItem{}, ff, lang, mockRenderer, mockNagivationCache, false)
Convey("Then a 200 request is returned", func() {
So(w.Code, ShouldEqual, http.StatusOK)
})
Expand Down Expand Up @@ -98,7 +101,7 @@ func Test_addFeedback(t *testing.T) {
},
}}
Convey("When addFeedback is called", func() {
addFeedback(w, req, mockRenderer, mockSender, from, to, lang, mockNagivationCache)
addFeedback(w, req, mockRenderer, mockSender, from, to, lang, siteDomain, mockNagivationCache)
Convey("Then the renderer is not called", func() {
So(len(mockRenderer.BuildPageCalls()), ShouldEqual, 0)
})
Expand Down Expand Up @@ -146,7 +149,7 @@ func Test_addFeedback(t *testing.T) {
},
}}
Convey("When addFeedback is called", func() {
addFeedback(w, req, mockRenderer, mockSender, from, to, lang, mockNagivationCache)
addFeedback(w, req, mockRenderer, mockSender, from, to, lang, siteDomain, mockNagivationCache)
Convey("Then the renderer is not called", func() {
So(len(mockRenderer.BuildPageCalls()), ShouldEqual, 0)
})
Expand Down Expand Up @@ -194,7 +197,7 @@ func Test_addFeedback(t *testing.T) {
},
}}
Convey("When addFeedback is called", func() {
addFeedback(w, req, mockRenderer, mockSender, from, to, lang, mockNagivationCache)
addFeedback(w, req, mockRenderer, mockSender, from, to, lang, siteDomain, mockNagivationCache)
Convey("Then the renderer is not called", func() {
So(len(mockRenderer.BuildPageCalls()), ShouldEqual, 0)
})
Expand Down Expand Up @@ -244,7 +247,7 @@ func Test_addFeedback(t *testing.T) {
},
}}
Convey("When addFeedback is called", func() {
addFeedback(w, req, mockRenderer, mockSender, from, to, lang, mockNagivationCache)
addFeedback(w, req, mockRenderer, mockSender, from, to, lang, siteDomain, mockNagivationCache)
Convey("Then the renderer is called to render the feedback page", func() {
So(len(mockRenderer.BuildPageCalls()), ShouldEqual, 1)
})
Expand All @@ -261,6 +264,7 @@ func Test_addFeedback(t *testing.T) {
func Test_feedbackThanks(t *testing.T) {
helper.InitialiseLocalisationsHelper(mocks.MockAssetFunction)
lang := "en"
config.Get() // need to seed config
Convey("Given a valid request", t, func() {
req := httptest.NewRequest("GET", "http://localhost", nil)
w := httptest.NewRecorder()
Expand All @@ -286,7 +290,7 @@ func Test_feedbackThanks(t *testing.T) {
},
}}
Convey("When feedbackThanks is called", func() {
feedbackThanks(w, req, url, mockRenderer, mockNagivationCache, lang)
feedbackThanks(w, req, url, mockRenderer, mockNagivationCache, lang, siteDomain, false)
Convey("Then the renderer is called", func() {
So(len(mockRenderer.BuildPageCalls()), ShouldEqual, 1)
})
Expand Down Expand Up @@ -320,7 +324,7 @@ func Test_feedbackThanks(t *testing.T) {
},
}}
Convey("When feedbackThanks is called", func() {
feedbackThanks(w, req, url, mockRenderer, mockNagivationCache, lang)
feedbackThanks(w, req, url, mockRenderer, mockNagivationCache, lang, siteDomain, false)
Convey("Then the handler sanitises the request text", func() {
dataSentToRender := mockRenderer.BuildPageCalls()[0].PageModel.(model.Feedback)
returnToUrl := dataSentToRender.ReturnTo
Expand Down Expand Up @@ -399,11 +403,38 @@ func TestValidateForm(t *testing.T) {
},
},
{
givenDescription: "the a specific page/url type is chosen and the url is valid",
givenDescription: "the a specific page/url type is chosen and the url is valid but not allowed",
given: &model.FeedbackForm{
Type: "A specific page",
Description: "Some text",
URL: "https://not-site-domain.com",
},
expectedDescription: "a url validation error is returned",
expected: []coreModel.ErrorItem{
{
Description: coreModel.Localisation{
LocaleKey: "FeedbackValidURL",
Plural: 1,
},
URL: "#type-error",
},
}},
{
givenDescription: "the a specific page/url type is chosen and the url is valid without a path",
given: &model.FeedbackForm{
Type: "A specific page",
Description: "Some text",
URL: "https://cy.ons.gov.uk",
},
expectedDescription: "no validation errors are returned",
expected: []coreModel.ErrorItem(nil),
},
{
givenDescription: "the a specific page/url type is chosen and the url is valid and has a path",
given: &model.FeedbackForm{
Type: "A specific page",
Description: "Some text",
URL: "https://somewhere.com",
URL: "https://cy.ons.gov.uk/path",
},
expectedDescription: "no validation errors are returned",
expected: []coreModel.ErrorItem(nil),
Expand Down Expand Up @@ -525,7 +556,7 @@ func TestValidateForm(t *testing.T) {
for _, t := range testCases {
Convey(fmt.Sprintf("When %s", t.givenDescription), func() {
Convey(fmt.Sprintf("Then %s", t.expectedDescription), func() {
So(validateForm(t.given), ShouldResemble, t.expected)
So(validateForm(t.given, siteDomain), ShouldResemble, t.expected)
})
})
}
Expand Down
9 changes: 6 additions & 3 deletions mapper/mapper.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"html"
"net/http"

"github.com/ONSdigital/dp-frontend-feedback-controller/config"
"github.com/ONSdigital/dp-frontend-feedback-controller/model"
"github.com/ONSdigital/dp-renderer/v2/helper"
core "github.com/ONSdigital/dp-renderer/v2/model"
Expand Down Expand Up @@ -195,7 +196,7 @@ func CreateGetFeedback(req *http.Request, basePage core.Page, validationErrors [
return p
}

func CreateGetFeedbackThanks(req *http.Request, basePage core.Page, lang, url, wholeSite string) model.Feedback {
func CreateGetFeedbackThanks(req *http.Request, basePage core.Page, lang, referrer, wholeSite string) model.Feedback {
p := model.Feedback{
Page: basePage,
}
Expand All @@ -204,14 +205,16 @@ func CreateGetFeedbackThanks(req *http.Request, basePage core.Page, lang, url, w
p.Type = "feedback"
p.URI = req.URL.Path
p.Metadata.Title = helper.Localise("FeedbackThanks", lang, 1)
p.PreviousURL = url
p.PreviousURL = referrer

// returnTo is rendered on page so needs XSS protection
returnTo := html.EscapeString(req.URL.Query().Get("returnTo"))
if returnTo == "Whole site" {
returnTo = wholeSite
} else if returnTo == "" {
returnTo = url
returnTo = referrer
} else if !config.IsSiteDomainURL(returnTo, "") {
returnTo = referrer
}

p.ReturnTo = returnTo
Expand Down

0 comments on commit b373e11

Please sign in to comment.